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>

skillpack/activation.go

@@ -2,6 +2,7 @@ package skillpack
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"sort"
 	"strings"
@@ -40,10 +41,15 @@ func Resolve(ctx context.Context, cache PackCache, subs []Subscription) ([]*Pack
 // pack (name + description) plus how to load one. This is the whole prompt cost
 // of a subscription — the bodies stay out until skill_use is called.
 func Catalog(packs []*Pack) string {
-	if len(packs) == 0 {
+	sorted := make([]*Pack, 0, len(packs))
+	for _, p := range packs {
+		if p != nil && p.Manifest != nil {
+			sorted = append(sorted, p)
+		}
+	}
+	if len(sorted) == 0 {
 		return ""
 	}
-	sorted := append([]*Pack(nil), packs...)
 	sort.Slice(sorted, func(i, j int) bool { return sorted[i].Manifest.Name < sorted[j].Manifest.Name })
 
 	var b strings.Builder
@@ -71,12 +77,14 @@ type skillUseArgs struct {
 // where to read scripts/references with its file tools. Leave it empty when the
 // host has no staging.
 func Activate(packs []*Pack, stagedDir string) mdagent.Skill {
-	if len(packs) == 0 {
-		return nil
-	}
 	byName := make(map[string]*Pack, len(packs))
 	for _, p := range packs {
-		byName[p.Manifest.Name] = p
+		if p != nil && p.Manifest != nil {
+			byName[p.Manifest.Name] = p
+		}
+	}
+	if len(byName) == 0 {
+		return nil
 	}
 
 	tool := llm.DefineTool("skill_use",
@@ -99,6 +107,9 @@ func Activate(packs []*Pack, stagedDir string) mdagent.Skill {
 // renderPackBody is what skill_use returns: the pack's instructions plus a
 // pointer to its bundled files (with the staged path when known).
 func renderPackBody(p *Pack, stagedDir string) string {
+	if p == nil || p.Manifest == nil {
+		return "Error: invalid skill pack."
+	}
 	var b strings.Builder
 	fmt.Fprintf(&b, "# Skill: %s\n\n%s\n", p.Manifest.Name, p.Manifest.Body)
 	if len(p.Bundled) > 0 {
@@ -118,6 +129,9 @@ func renderPackBody(p *Pack, stagedDir string) string {
 // mount them (read-only is the host's concern) into a sandbox the agent's file
 // tools can read. Returns the pack's staged directory.
 func Stage(p *Pack, baseDir string) (string, error) {
+	if p == nil || p.Manifest == nil {
+		return "", errors.New("skillpack: Stage requires a non-nil pack")
+	}
 	dir := baseDir + "/" + p.Manifest.Name
 	if err := p.Tree.WriteTo(dir); err != nil {
 		return "", err

skillpack/activation_test.go

@@ -86,6 +86,25 @@ func TestActivate_Empty(t *testing.T) {
 	}
 }
 
+func TestNilPackElementsAreSafe(t *testing.T) {
+	packs := []*Pack{nil, mustPack(t, "real", "b", nil), {Manifest: nil}}
+	// Neither Catalog nor Activate may panic on nil / malformed elements.
+	if got := Catalog(packs); !strings.Contains(got, "real") {
+		t.Errorf("catalog should include the valid pack and skip nils: %q", got)
+	}
+	sk := Activate(packs, "")
+	if sk == nil {
+		t.Fatal("a valid pack among nils should still activate")
+	}
+	if _, ok := sk.Tools().Get("skill_use"); !ok {
+		t.Error("skill_use missing")
+	}
+	// All-nil activates to nothing rather than panicking.
+	if Activate([]*Pack{nil, {Manifest: nil}}, "") != nil {
+		t.Error("only-nil packs should activate to nil")
+	}
+}
+
 func TestResolveFromCache(t *testing.T) {
 	ctx := context.Background()
 	cache := NewMemoryPackCache()

skillpack/manifest.go

@@ -112,6 +112,10 @@ func (m *Manifest) Validate() error {
 // Leading blank lines/BOM are tolerated. A missing or unterminated block is an
 // error — a SKILL.md without frontmatter has no name/description to catalog.
 func splitFrontmatter(raw []byte) (front, body []byte, err error) {
+	// Strip a leading UTF-8 BOM: editors on some platforms prepend one, and
+	// bytes.TrimSpace (used below) does not remove it, so a BOM would otherwise
+	// make the first "---" fence unrecognizable.
+	raw = bytes.TrimPrefix(raw, []byte{0xEF, 0xBB, 0xBF})
 	s := bufio.NewScanner(bytes.NewReader(raw))
 	s.Buffer(make([]byte, 0, 64*1024), maxBodyBytes+64*1024)
 
@@ -180,13 +184,25 @@ func trimAll(in []string) []string {
 	return out
 }
 
+// isKebab reports whether s is strict lowercase kebab-case: [a-z0-9] segments
+// joined by single hyphens, with no leading, trailing, or consecutive hyphens.
 func isKebab(s string) bool {
+	if s == "" || s[0] == '-' || s[len(s)-1] == '-' {
+		return false
+	}
+	prevHyphen := false
 	for _, r := range s {
 		switch {
-		case r >= 'a' && r <= 'z', r >= '0' && r <= '9', r == '-':
+		case r >= 'a' && r <= 'z', r >= '0' && r <= '9':
+			prevHyphen = false
+		case r == '-':
+			if prevHyphen {
+				return false
+			}
+			prevHyphen = true
 		default:
 			return false
 		}
 	}
-	return s != ""
+	return true
 }

skillpack/manifest_test.go

@@ -61,6 +61,9 @@ func TestParseManifest_Errors(t *testing.T) {
 		"missing desc":       "---\nname: x\n---\nb\n",
 		"bad name uppercase": "---\nname: PdfProcessing\ndescription: d\n---\nb\n",
 		"bad name space":     "---\nname: pdf processing\ndescription: d\n---\nb\n",
+		"bad name leading -": "---\nname: -pdf\ndescription: d\n---\nb\n",
+		"bad name trailing-": "---\nname: pdf-\ndescription: d\n---\nb\n",
+		"bad name double -":  "---\nname: pdf--tools\ndescription: d\n---\nb\n",
 		"bad yaml":           "---\nname: [unclosed\n---\nb\n",
 	}
 	for label, in := range cases {
@@ -80,3 +83,14 @@ func TestParseManifest_LeadingBlanksAndCRLF(t *testing.T) {
 		t.Errorf("got name=%q body=%q", m.Name, m.Body)
 	}
 }
+
+func TestParseManifest_BOM(t *testing.T) {
+	in := append([]byte{0xEF, 0xBB, 0xBF}, []byte("---\nname: bom-ok\ndescription: d\n---\nbody\n")...)
+	m, err := ParseManifest(in)
+	if err != nil {
+		t.Fatalf("BOM-prefixed SKILL.md should parse: %v", err)
+	}
+	if m.Name != "bom-ok" {
+		t.Errorf("name = %q", m.Name)
+	}
+}

skillpack/pack.go

@@ -107,6 +107,14 @@ func readTree(fsys fs.FS) (Tree, error) {
 		if d.IsDir() {
 			return nil
 		}
+		// Skip symlinks. A pack must be self-contained; os.DirFS + ReadFile
+		// follows symlinks, so a malicious pack with `SKILL.md -> /etc/passwd`
+		// or `scripts/x -> ../../.ssh/id_rsa` would otherwise read host files
+		// into the tree. WalkDir yields a symlink-to-dir as a non-dir entry
+		// carrying ModeSymlink, so this one check covers file and dir symlinks.
+		if d.Type()&fs.ModeSymlink != 0 {
+			return nil
+		}
 		b, err := fs.ReadFile(fsys, p)
 		if err != nil {
 			return err

skillpack/pack_test.go

@@ -68,6 +68,31 @@ func TestTreeWriteTo(t *testing.T) {
 	}
 }
 
+func TestReadTree_SkipsSymlinks(t *testing.T) {
+	dir := t.TempDir()
+	if err := os.WriteFile(filepath.Join(dir, ManifestName), []byte(goodManifest), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	// A malicious pack pointing at a host file must NOT be read into the tree.
+	secret := filepath.Join(t.TempDir(), "secret")
+	if err := os.WriteFile(secret, []byte("TOPSECRET"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Symlink(secret, filepath.Join(dir, "leak")); err != nil {
+		t.Skipf("symlink unsupported: %v", err)
+	}
+	tree, err := readTree(os.DirFS(dir))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, ok := tree["leak"]; ok {
+		t.Fatal("symlink was followed into the tree — arbitrary host file read")
+	}
+	if _, ok := tree[ManifestName]; !ok {
+		t.Fatal("real file should still be read")
+	}
+}
+
 func TestTreeWriteTo_RejectsTraversal(t *testing.T) {
 	dir := t.TempDir()
 	evil := Tree{"../escape.txt": []byte("nope")}

skillpack/source.go

@@ -88,17 +88,33 @@ func (g GitSource) run(ctx context.Context, dir string, args ...string) ([]byte,
 }
 
 func (g GitSource) Fetch(ctx context.Context, ref string) (Tree, string, error) {
+	// Argument-injection guard: a URL or ref beginning with "-" would be parsed
+	// by git as an option (e.g. --upload-pack=…), not a value. Reject it rather
+	// than rely solely on the "--" separator, which checkout does not honor for
+	// a rev. Hosts should also allow-list sources, but this is defense-in-depth
+	// for a library.
+	if strings.HasPrefix(g.URL, "-") {
+		return nil, "", fmt.Errorf("skillpack: git url must not start with '-': %q", g.URL)
+	}
+	if strings.HasPrefix(ref, "-") {
+		return nil, "", fmt.Errorf("skillpack: git ref must not start with '-': %q", ref)
+	}
+
 	tmp, err := os.MkdirTemp("", "skillpack-git-*")
 	if err != nil {
 		return nil, "", err
 	}
 	defer os.RemoveAll(tmp)
 
-	if _, err := g.run(ctx, "", "clone", "--quiet", g.URL, tmp); err != nil {
+	// --filter=blob:none: a blobless partial clone gets the ref graph cheaply
+	// and fetches only the blobs the checkout needs — much less than the full
+	// history, while still supporting an arbitrary commit-ish ref. "--" ends
+	// option parsing before the URL.
+	if _, err := g.run(ctx, "", "clone", "--quiet", "--filter=blob:none", "--", g.URL, tmp); err != nil {
 		return nil, "", err
 	}
 	if ref != "" {
-		if _, err := g.run(ctx, tmp, "checkout", "--quiet", ref); err != nil {
+		if _, err := g.run(ctx, tmp, "checkout", "--quiet", "--detach", ref); err != nil {
 			return nil, "", err
 		}
 	}

skillpack/subscription.go

@@ -50,9 +50,10 @@ func (s *Subscription) HasPending() bool {
 }
 
 // pinTo advances the active pin to a fetched pack and clears any pending state.
-// Used by initial pin and by Apply.
+// Used by initial pin and by Apply. It does NOT set Name: a subscription's name
+// is its stable host handle, fixed at Subscribe time — letting an upstream pack
+// rename move it would silently collide with another subscription on Apply.
 func (s *Subscription) pinTo(p *Pack, sourceRef, by string, now time.Time) {
-	s.Name = p.Manifest.Name
 	s.Description = p.Manifest.Description
 	s.PinnedDigest = p.Digest
 	s.PinnedSourceRef = sourceRef

skillpack/sync.go

@@ -2,6 +2,7 @@ package skillpack
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"time"
 
@@ -14,7 +15,7 @@ import (
 // — the only call that changes the bytes an agent runs is Apply, always with an
 // actor recorded.
 type Syncer struct {
-	Store PackCache // content store for pinned trees
+	Cache PackCache // content store for pinned trees
 	Subs  Store     // subscription metadata store
 
 	// SourceFor builds the Source for a stored subscription. A host overrides
@@ -73,7 +74,7 @@ func (y *Syncer) fetchPack(ctx context.Context, src Source, ref string) (*Pack,
 	if err != nil {
 		return nil, "", err
 	}
-	if err := y.Store.Put(ctx, pack.Digest, pack.Tree); err != nil {
+	if err := y.Cache.Put(ctx, pack.Digest, pack.Tree); err != nil {
 		return nil, "", err
 	}
 	return pack, sourceRef, nil
@@ -87,21 +88,32 @@ func (y *Syncer) Subscribe(ctx context.Context, src Source, trackRef, by string)
 	if err != nil {
 		return nil, err
 	}
-	if existing, err := y.Subs.GetByName(ctx, pack.Manifest.Name); err == nil {
+	existing, err := y.Subs.GetByName(ctx, pack.Manifest.Name)
+	if err == nil {
 		return nil, fmt.Errorf("skillpack: already subscribed to %q (id %s)", pack.Manifest.Name, existing.ID)
 	}
+	if !errors.Is(err, ErrNotFound) {
+		// A transient store error must NOT fall through to creating a row — that
+		// would produce a duplicate subscription the uniqueness check missed.
+		return nil, fmt.Errorf("skillpack: checking for existing subscription %q: %w", pack.Manifest.Name, err)
+	}
 
 	sub := &Subscription{
 		ID:         y.newID(),
+		Name:       pack.Manifest.Name,
 		SourceKind: src.Kind(),
 		SourceURL:  src.String(),
 		TrackRef:   trackRef,
 		Enabled:    true,
 	}
-	// Store the raw URL/subpath (String() may combine them for display).
-	if gs, ok := src.(GitSource); ok {
-		sub.SourceURL = gs.URL
-		sub.Subpath = gs.Subpath
+	// Store the raw URL + subpath separately (String() may combine them for
+	// display). GitSource methods have value receivers, so a caller may pass
+	// either GitSource or *GitSource — handle both.
+	switch gs := src.(type) {
+	case GitSource:
+		sub.SourceURL, sub.Subpath = gs.URL, gs.Subpath
+	case *GitSource:
+		sub.SourceURL, sub.Subpath = gs.URL, gs.Subpath
 	}
 	sub.pinTo(pack, sourceRef, by, y.now())
 	if err := y.Subs.Save(ctx, sub); err != nil {
@@ -175,7 +187,7 @@ func (y *Syncer) Apply(ctx context.Context, id, by string) (*Subscription, error
 	if !sub.HasPending() {
 		return nil, fmt.Errorf("skillpack: %q has no pending update to apply", sub.Name)
 	}
-	tree, err := y.Store.Get(ctx, sub.PendingDigest)
+	tree, err := y.Cache.Get(ctx, sub.PendingDigest)
 	if err != nil {
 		return nil, fmt.Errorf("skillpack: pending tree for %q missing from cache: %w", sub.Name, err)
 	}

skillpack/sync_test.go

@@ -2,6 +2,7 @@ package skillpack
 
 import (
 	"context"
+	"fmt"
 	"testing"
 	"time"
 )
@@ -27,10 +28,10 @@ func packTree(name, body string) Tree {
 func newTestSyncer(src *fakeSource) *Syncer {
 	n := 0
 	return &Syncer{
-		Store:     NewMemoryPackCache(),
+		Cache:     NewMemoryPackCache(),
 		Subs:      NewMemory(),
 		Now:       func() time.Time { return time.Unix(1000, 0) },
-		NewID:     func() string { n++; return "id-1" },
+		NewID:     func() string { n++; return fmt.Sprintf("id-%d", n) },
 		SourceFor: func(*Subscription) (Source, error) { return src, nil },
 	}
 }
@@ -51,7 +52,7 @@ func TestSubscribeAndPin(t *testing.T) {
 		t.Fatal("fresh subscription should have no pending update")
 	}
 	// pinned tree is cached under its digest
-	if _, err := y.Store.Get(ctx, sub.PinnedDigest); err != nil {
+	if _, err := y.Cache.Get(ctx, sub.PinnedDigest); err != nil {
 		t.Fatalf("pinned tree not cached: %v", err)
 	}
 }
@@ -93,7 +94,7 @@ func TestCheck_RecordsPendingButDoesNotMovePin(t *testing.T) {
 		t.Errorf("pending ref = %q", updated.PendingSourceRef)
 	}
 	// the pending tree is cached, ready for Apply
-	if _, err := y.Store.Get(ctx, updated.PendingDigest); err != nil {
+	if _, err := y.Cache.Get(ctx, updated.PendingDigest); err != nil {
 		t.Fatalf("pending tree not cached: %v", err)
 	}
 }