Files
executus/skillpack/sync_test.go
T
steve 9bb5d143f7
executus CI / test (pull_request) Successful in 3m30s
fix(skillpack): address review — symlink read, git arg-injection, dup-subscribe, nil panics
Real issues from the PR review:
- security: readTree now skips symlinks (a pack with SKILL.md -> /etc/passwd
  or scripts/x -> ../../.ssh/id_rsa could read host files); covers file and
  dir symlinks, incl. within a git subpath
- security: GitSource rejects url/ref beginning with '-' (git arg injection)
  and clones with '--' separator; --filter=blob:none (blobless partial clone)
  instead of full-history clone
- correctness: Subscribe no longer swallows a non-ErrNotFound store error from
  GetByName (would create a duplicate subscription); handles *GitSource as well
  as GitSource in the URL/subpath extraction
- correctness: pinTo no longer renames a subscription, so Apply can't silently
  collide two subscriptions when an upstream pack changes its name
- validation: isKebab rejects leading/trailing/consecutive hyphens; BOM-
  prefixed SKILL.md now parses (matches the doc comment)
- robustness: Catalog/Activate/renderPackBody/Stage guard nil/malformed packs
- test cleanup: Syncer.Store field renamed Cache (collided with the Store
  interface); test NewID returns distinct ids
- tests: symlink-skip, BOM, strict-kebab, nil-pack-safety

Deferred (advisory perf, documented): PackCache stores raw trees so activation
re-parses; CheckAll is serial. Both fine at expected scale.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-04 20:41:44 -04:00

178 lines
5.0 KiB
Go

package skillpack
import (
"context"
"fmt"
"testing"
"time"
)
// fakeSource returns a caller-controlled tree, so sync behavior is tested with
// no filesystem or git.
type fakeSource struct {
tree Tree
ref string
err error
}
func (f *fakeSource) Fetch(context.Context, string) (Tree, string, error) {
return f.tree, f.ref, f.err
}
func (f *fakeSource) Kind() string { return "fake" }
func (f *fakeSource) String() string { return "fake://pack" }
func packTree(name, body string) Tree {
return Tree{ManifestName: []byte("---\nname: " + name + "\ndescription: does " + name + "\n---\n" + body + "\n")}
}
func newTestSyncer(src *fakeSource) *Syncer {
n := 0
return &Syncer{
Cache: NewMemoryPackCache(),
Subs: NewMemory(),
Now: func() time.Time { return time.Unix(1000, 0) },
NewID: func() string { n++; return fmt.Sprintf("id-%d", n) },
SourceFor: func(*Subscription) (Source, error) { return src, nil },
}
}
func TestSubscribeAndPin(t *testing.T) {
ctx := context.Background()
src := &fakeSource{tree: packTree("alpha", "v1"), ref: "sha-v1"}
y := newTestSyncer(src)
sub, err := y.Subscribe(ctx, src, "main", "steve")
if err != nil {
t.Fatal(err)
}
if sub.Name != "alpha" || sub.PinnedSourceRef != "sha-v1" || sub.PinnedBy != "steve" {
t.Fatalf("bad pin: %+v", sub)
}
if sub.HasPending() {
t.Fatal("fresh subscription should have no pending update")
}
// pinned tree is cached under its digest
if _, err := y.Cache.Get(ctx, sub.PinnedDigest); err != nil {
t.Fatalf("pinned tree not cached: %v", err)
}
}
func TestSubscribe_DuplicateName(t *testing.T) {
ctx := context.Background()
src := &fakeSource{tree: packTree("alpha", "v1"), ref: "r"}
y := newTestSyncer(src)
if _, err := y.Subscribe(ctx, src, "", "s"); err != nil {
t.Fatal(err)
}
if _, err := y.Subscribe(ctx, src, "", "s"); err == nil {
t.Fatal("expected duplicate-name error")
}
}
func TestCheck_RecordsPendingButDoesNotMovePin(t *testing.T) {
ctx := context.Background()
src := &fakeSource{tree: packTree("alpha", "v1"), ref: "sha-v1"}
y := newTestSyncer(src)
sub, _ := y.Subscribe(ctx, src, "main", "s")
pinnedBefore := sub.PinnedDigest
// upstream changes
src.tree = packTree("alpha", "v2-new-instructions")
src.ref = "sha-v2"
updated, err := y.Check(ctx, sub.ID)
if err != nil {
t.Fatal(err)
}
if !updated.HasPending() {
t.Fatal("expected a pending update after upstream change")
}
if updated.PinnedDigest != pinnedBefore {
t.Fatal("Check must NOT move the pin — that is the supply-chain guard")
}
if updated.PendingSourceRef != "sha-v2" {
t.Errorf("pending ref = %q", updated.PendingSourceRef)
}
// the pending tree is cached, ready for Apply
if _, err := y.Cache.Get(ctx, updated.PendingDigest); err != nil {
t.Fatalf("pending tree not cached: %v", err)
}
}
func TestCheck_ClearsStalePendingWhenUpstreamMatches(t *testing.T) {
ctx := context.Background()
src := &fakeSource{tree: packTree("alpha", "v1"), ref: "r1"}
y := newTestSyncer(src)
sub, _ := y.Subscribe(ctx, src, "main", "s")
src.tree = packTree("alpha", "v2")
src.ref = "r2"
sub, _ = y.Check(ctx, sub.ID) // records pending
if !sub.HasPending() {
t.Fatal("precondition: pending expected")
}
// upstream reverts to the pinned content
src.tree = packTree("alpha", "v1")
src.ref = "r1"
sub, _ = y.Check(ctx, sub.ID)
if sub.HasPending() {
t.Fatal("pending should be cleared once upstream matches the pin again")
}
}
func TestApply_MovesPinAndClearsPending(t *testing.T) {
ctx := context.Background()
src := &fakeSource{tree: packTree("alpha", "v1"), ref: "sha-v1"}
y := newTestSyncer(src)
sub, _ := y.Subscribe(ctx, src, "main", "s")
src.tree = packTree("alpha", "v2")
src.ref = "sha-v2"
sub, _ = y.Check(ctx, sub.ID)
pendingDigest := sub.PendingDigest
applied, err := y.Apply(ctx, sub.ID, "admin")
if err != nil {
t.Fatal(err)
}
if applied.PinnedDigest != pendingDigest {
t.Fatal("Apply must move the pin to the pending digest")
}
if applied.PinnedSourceRef != "sha-v2" || applied.PinnedBy != "admin" {
t.Errorf("bad post-apply pin: %+v", applied)
}
if applied.HasPending() {
t.Fatal("Apply must clear the pending update")
}
}
func TestApply_NoPending(t *testing.T) {
ctx := context.Background()
src := &fakeSource{tree: packTree("alpha", "v1"), ref: "r"}
y := newTestSyncer(src)
sub, _ := y.Subscribe(ctx, src, "", "s")
if _, err := y.Apply(ctx, sub.ID, "admin"); err == nil {
t.Fatal("expected error applying with no pending update")
}
}
func TestCheckAll(t *testing.T) {
ctx := context.Background()
src := &fakeSource{tree: packTree("alpha", "v1"), ref: "r1"}
y := newTestSyncer(src)
sub, _ := y.Subscribe(ctx, src, "main", "s")
if pend, errs := y.CheckAll(ctx); len(pend) != 0 || len(errs) != 0 {
t.Fatalf("no change: pend=%v errs=%v", pend, errs)
}
src.tree = packTree("alpha", "v2")
src.ref = "r2"
pend, errs := y.CheckAll(ctx)
if len(errs) != 0 {
t.Fatalf("errs: %v", errs)
}
if len(pend) != 1 || pend[0].ID != sub.ID {
t.Fatalf("expected 1 pending, got %v", pend)
}
}