feat(skillpack): SKILL.md-subscription battery #22
Reference in New Issue
Block a user
Delete Branch "feat/skillpack-battery"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Adds a new additive, nil-safe battery
executus/skillpackfor subscribing to skill packages published in the Anthropic agent-skills format (aSKILL.mdmanifest plus optional bundled scripts/reference files), with progressive-disclosure activation.Why
This is a third, distinct "skill" concept, deliberately separate from the two that already collide in the stack:
majordomo/skill— a lightweight capability bundle injected into the prompt eagerly.executus/skill— a heavyweight persisted saved-agent noun.executus/skillpack(this) — an externally-authored, versioned, on-demand-loaded instruction pack fetched from aSourceand pinned by content digest.Progressive disclosure is the reason it isn't just a
majordomo/skill: a catalog of large third-party packs can't all sit in the system prompt. Only each pack's name+description does; the body loads lazily via a singleskill_usetool.What's here
SKILL.mdfrontmatter+body parse & validation (name/description required, kebab-case + length limits matching the agent-skills constraints,allowed-toolspassthrough).DirSource(local/builtin) andGitSource(clones to temp, reads the subpath into memory, cleans up → self-contained tree);Fetchreturns the tree + the source's resolved ref.Memorydefaults, mirroring the other batteries.Subscribepins;Checkrecords a pending update but never moves the pin;Applyis the only re-pin (supply-chain guard: upstream can't silently change what an agent runs), always with an actor recorded.majordomo/agent.Skill(catalog instructions + oneskill_usetool);Stagematerializes bundled files for a sandbox.Notes
Describerinterface;Activateinstead builds the skill viamajordomo/skill.Newwith the catalog as its instructions, so the boundary is cleaner than planned..skillpackcommands, anAgent.SkillPacksfield,allowed-toolsnarrowing, sandbox staging, aCheckAllticker) is a separate, host-side step — this PR is mechanism only.contrib/storeSQLite impl ofStore/PackCachefor the light-tier/canary hosts (mort backs these with its own DB).Full unit + hermetic local-git tests;
gofmt/vetclean;-racegreen. ~1,600 lines across 14 files.🤖 Generated with Claude Code
🪰 Gadfly — live review status
4/4 reviewers finished · updated 2026-07-05 00:16:01Z
deepseek-v4-pro:cloud· ollama-cloud — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneqwen3.5:397b-cloud· ollama-cloud — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 25 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -0,0 +44,4 @@return ""}sorted := append([]*Pack(nil), packs...)sort.Slice(sorted, func(i, j int) bool { return sorted[i].Manifest.Name < sorted[j].Manifest.Name })🟠 Catalog panics on nil pack element in slice
error-handling · flagged by 1 model
skillpack/activation.go:47,54—Catalog(packs []*Pack)will panic if any element in the slice isnil. The function checkslen(packs) == 0but does not defend against[]*Pack{nil}. A caller constructing a slice with a nil element (or a bug inResolvethat appends nil) would cause a runtime panic atsorted[i].Manifest.Name. Fix: Add a nil check in the loop (if p == nil { continue }) or document that nil elements are not permitted.🪰 Gadfly · advisory
@@ -0,0 +51,4 @@b.WriteString("When a task matches one, call skill_use with its name to load its full instructions before proceeding.\n\n")b.WriteString("Available skills:\n")for _, p := range sorted {fmt.Fprintf(&b, "- %s: %s\n", p.Manifest.Name, p.Manifest.Description)🟠 Prompt injection via unsanitized Description field injected directly into system prompt Catalog
security · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +76,4 @@}byName := make(map[string]*Pack, len(packs))for _, p := range packs {byName[p.Manifest.Name] = p🟠 Activate panics on nil pack element in slice
error-handling · flagged by 1 model
skillpack/activation.go:79—Activate(packs []*Pack, stagedDir string)will panic if any pack in the input slice isnil. Same issue:byName[p.Manifest.Name]dereferences without checkingp != nil. Fix: Skip nil packs in the loop or return an error.🪰 Gadfly · advisory
@@ -0,0 +100,4 @@// pointer to its bundled files (with the staged path when known).func renderPackBody(p *Pack, stagedDir string) string {var b strings.Builderfmt.Fprintf(&b, "# Skill: %s\n\n%s\n", p.Manifest.Name, p.Manifest.Body)🟠 renderPackBody panics on nil Pack or nil Manifest
error-handling · flagged by 1 model
skillpack/activation.go:103-104—renderPackBody(p *Pack, stagedDir string)will panic ifp == nilorp.Manifest == nil. This is called from theskill_usetool handler; whileActivatebuilds the map from valid packs, a future refactor or direct call could pass nil. Fix: Addif p == nil || p.Manifest == nil { return "Error: invalid pack" }.🪰 Gadfly · advisory
@@ -0,0 +118,4 @@// 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) {dir := baseDir + "/" + p.Manifest.Name🟠 Stage panics on nil Pack or nil Manifest
error-handling, maintainability, security · flagged by 4 models
skillpack/activation.go:121—Stage(p *Pack, baseDir string)will panic ifp == nilorp.Manifest == nilatp.Manifest.Name. Fix: Add nil guard at function entry.🪰 Gadfly · advisory
@@ -0,0 +76,4 @@AllowedTools: []string(fm.AllowedTools),Body: strings.TrimSpace(string(body)),}if len(fm.Metadata) > 0 {🟡 metadata passthrough values are unbounded (only name/description/body are length-capped); untrusted author can embed large arbitrary metadata
error-handling, security · flagged by 2 models
skillpack/manifest.go:79-83—Metadatavalues stringified viafmt.Sprintf("%v", ...)(low). Arbitrary YAML scalars/sequences inmetadataare flattened to strings. Not a security hole per se (no eval, no template), but a malicious pack can stuff unboundedmetadatacontent into what a host may render; there's no per-value length cap (onlybodyand name/description are bounded). The scanner buffer (~1 MiB) indirectly bounds total frontmatter, so this is minor — flagging only becau…🪰 Gadfly · advisory
@@ -0,0 +109,4 @@}// splitFrontmatter separates a leading `---`-delimited YAML block from the body.// Leading blank lines/BOM are tolerated. A missing or unterminated block is an🟡 splitFrontmatter claims BOM tolerance but does not strip a leading BOM, so BOM-prefixed SKILL.md fails to parse
error-handling · flagged by 1 model
skillpack/manifest.go:112-131— BOM is claimed tolerated but isn't. The doc comment onsplitFrontmattersays "Leading blank lines/BOM are tolerated," but the scan loop (lines 126-134) only skips blank lines (len(bytes.TrimSpace(trimmed)) == 0) and then checksstring(bytes.TrimSpace(trimmed)) != "---".bytes.TrimSpacedoes not strip a UTF-8 BOM (0xEF 0xBB 0xBF), so a SKILL.md saved with a BOM (\xEF\xBB\xBF---) fails with "must start with a '---' frontmatter block." There is no…🪰 Gadfly · advisory
@@ -0,0 +180,4 @@return out}func isKebab(s string) bool {🔴 isKebab allows leading/trailing/consecutive hyphens, violating Anthropic agent-skills kebab-case constraints
correctness · flagged by 2 models
🪰 Gadfly · advisory
@@ -0,0 +42,4 @@return &cp, nil}func (m *Memory) GetByName(_ context.Context, name string) (*Subscription, error) {🟠 GetByName linear scan causes O(n²) on bulk Subscribe
performance · flagged by 1 model
skillpack/memory.go:45—GetByNamelinear scan causes O(n²) on bulkSubscribe.Memory.GetByNameiterates over all subscriptions to find a match by name, andSubscribecalls this to reject duplicates. If a host creates many subscriptions, the overall work is quadratic. Add aname→subscriptionindex or document that hosts with many subscriptions should use a DB-backedStore.🪰 Gadfly · advisory
@@ -0,0 +48,4 @@// how a host stages a pack's files for a sandbox; the host owns mount/read-only// policy. Paths are cleaned and constrained to dir — a tree entry that escapes// (via .. or an absolute path) is rejected rather than written outside dir.func (t Tree) WriteTo(dir string) error {🟠 WriteTo has no rollback on partial failure — leaves partially-staged directory on error
error-handling · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +92,4 @@bundled = append(bundled, p)}}return &Pack{Manifest: m, Tree: t, Digest: t.Digest(), Bundled: bundled}, nil🟠 LoadPack re-computes content digest that is already known to the caller (the PackCache key)
performance · flagged by 1 model
LoadPackre-computes digest that is already the cache key (skillpack/pack.go:95).Resolveretrieves a tree by digestd, thenLoadPackcallst.Digest()which re-hashes every file to produce… the same digestd. The digest could be passed through (e.g.,LoadPackWithDigest(t, d)) or, better, eliminated entirely if thePackCachestores the pre-parsed pack (see first finding). On its own this is a subset of the first issue, but it's independently fixable even if the cache inter…🪰 Gadfly · advisory
@@ -0,0 +98,4 @@// readTree reads an entire fs.FS (rooted at ".") into a Tree, skipping// directories. It is the shared reader for DirSource and GitSource, so both// produce identical self-contained trees.func readTree(fsys fs.FS) (Tree, error) {🔴 readTree follows symlinks allowing arbitrary file read from DirSource/GitSource
security · flagged by 2 models
skillpack/pack.go:101-120— Symlink following inreadTreeallows arbitrary file read (HIGH, verified) ThereadTreefunction usesos.DirFS+fs.ReadFilewhich follows symlinks. The walk callback checksd.IsDir()but never checksd.Type()&fs.ModeSymlink != 0. An attacker controlling aDirSourcedirectory (or a git repo with symlinks) can place a symlink likeSKILL.md -> /etc/passwdorscripts/leak -> ../../.ssh/id_rsa. WhenFetchreads the tree, sensitive host files…🪰 Gadfly · advisory
@@ -0,0 +122,4 @@// within reports whether dest is inside dir (defense against path traversal in// a staged tree).func within(dir, dest string) bool {🟡 within() duplicates filepath.IsLocal() from Go 1.20+
maintainability · flagged by 1 model
within()helper duplicates standard library functionality. Go 1.20+ providesfilepath.IsLocal()which performs the same path-traversal check. Keeping a custom implementation adds maintenance burden and risks divergence. Suggested fix: Replace withfilepath.IsLocal(rel)after thefilepath.Relcall (or inline the check usingfilepath.IsLocal(dest)directly if on Go 1.20+). Verified: Read pack.go lines 123-131; confirmed the logic matches `IsL…🪰 Gadfly · advisory
@@ -0,0 +94,4 @@}defer os.RemoveAll(tmp)if _, err := g.run(ctx, "", "clone", "--quiet", g.URL, tmp); err != nil {🔴 Git command injection via g.URL in git clone — no -- separator before user-controlled URL
performance, security · flagged by 2 models
GitSource.Fetchclones full repo history — no--depth 1(skillpack/source.go:97). The clone command isgit clone --quiet g.URL tmpwith no--depth 1and no--single-branch. EverySubscribeand everyCheck(which runs on a host ticker viaCheckAll) clones the entire repository history when only the current tree state is needed. For a pack published from a repo with years of history, this downloads orders of magnitude more data than necessary and burns CPU on delta reso…🪰 Gadfly · advisory
@@ -0,0 +98,4 @@return nil, "", err}if ref != "" {if _, err := g.run(ctx, tmp, "checkout", "--quiet", ref); err != nil {🔴 Git command injection via ref in git checkout — no -- separator before user-controlled ref
security · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +110,4 @@root := tmpif g.Subpath != "" {clean := path.Clean("/" + g.Subpath) // normalize, strip leading ../🟠 GitSource subpath validation does not account for symlinks within subpath
security · flagged by 1 model
skillpack/source.go:113-121— Subpath validation occurs afterpath.Clean, but symlink in subpath target not checked (MEDIUM, verified) TheGitSource.Fetchvalidates that the subpath doesn't escape viawithin(tmp, root), but this only checks the path string. If the subpath directory itself contains symlinks (e.g., adata -> /etcsymlink inside the valid subpath), those are followed byreadTree. Combined with the symlink issue above, this allows reading files outside the repo.…🪰 Gadfly · advisory
@@ -0,0 +21,4 @@// upstream later force-pushes or disappears. A host may back it with disk;// Memory is the default. Because the key IS the content digest, entries are// immutable and safe to share across subscriptions that pin the same bytes.type PackCache interface {🔴 PackCache stores raw Tree forcing YAML re-parse + SHA-256 re-hash on every agent activation via Resolve→LoadPack
performance · flagged by 1 model
PackCachestores rawTreeinstead of parsed*Pack— forces re-parse + re-hash on every activation (skillpack/store.go:24-27,skillpack/activation.go:19-37,skillpack/pack.go:80-96). ThePackCacheinterface storesTree(raw bytes), butResolve— which is on the activation hot path (every agent run) — callsLoadPack(tree)for each subscription.LoadPackre-parses the YAML frontmatter viaParseManifestand re-computes the SHA-256 content digest viat.Digest()(wh…🪰 Gadfly · advisory
@@ -0,0 +13,4 @@// the pin), and apply (the explicit re-pin). It owns the supply-chain invariant// — the only call that changes the bytes an agent runs is Apply, always with an// actor recorded.type Syncer struct {🟠 Syncer field Store has type PackCache, colliding with Store interface name
maintainability · flagged by 4 models
skillpack/sync.go:16-18—Syncerhas a field namedStorewhose type isPackCache, notStore. In the same package there is aStoreinterface (for subscriptions), so every reader will assumey.Storeis subscription metadata storage. The comment on the field even says "content store for pinned trees", but the name collision makes the code harder to read and easier to misuse. Rename the field toCacheto match its type.🪰 Gadfly · advisory
@@ -0,0 +73,4 @@if err != nil {return nil, "", err}if err := y.Store.Put(ctx, pack.Digest, pack.Tree); err != nil {🟠 fetchPack unconditionally re-caches the pack tree on every Check/CheckAll tick, deep-copying identical bytes via MemoryPackCache.Put even when the digest is unchanged
performance · flagged by 1 model
skillpack/sync.go:76(fetchPack) — everyCheckre-caches the pack tree even when nothing changed.fetchPackunconditionally callsy.Store.Put(ctx, pack.Digest, pack.Tree)at line 76.CheckcallsfetchPackon every tick (line 126), and the digest comparison that would skip work happens after thePut(line 130).MemoryPackCache.Put(skillpack/memory.go:92) thencloneTrees the entire tree — a fresh map plus a new byte slice per file — overwriting the identical entry. S…🪰 Gadfly · advisory
@@ -0,0 +87,4 @@if err != nil {return nil, err}if existing, err := y.Subs.GetByName(ctx, pack.Manifest.Name); err == nil {🔴 GetByName error treated as 'name available' — any non-ErrNotFound error silently allows duplicate subscriptions
correctness, error-handling · flagged by 2 models
skillpack/sync.go:90— swallowed store error in the duplicate-name check.Subscribetests for an existing subscription withif existing, err := y.Subs.GetByName(ctx, pack.Manifest.Name); err == nil { ... }. Only theerr == nilbranch is handled; any other error fromGetByName(a transient DB failure, connection error, anything that isn'tErrNotFound) is silently dropped and execution falls through to create a new subscription. That can produce a duplicate subscription (the ve…🪰 Gadfly · advisory
@@ -0,0 +91,4 @@return nil, fmt.Errorf("skillpack: already subscribed to %q (id %s)", pack.Manifest.Name, existing.ID)}sub := &Subscription{🟡 Subscribe has awkward URL assignment that overwrites src.String() for GitSource
maintainability · flagged by 2 models
Subscribe. The code assignssrc.String()tosub.SourceURL, then immediately overwrites it forGitSourceto extract rawURLandSubpathfields. This roundabout pattern obscures intent. Suggested fix: Build the subscription's source fields directly from the concrete source type before the type switch, or have each source type expose its components via an interface method. Verified: Read sync.go lines 85-111.🪰 Gadfly · advisory
@@ -0,0 +99,4 @@Enabled: true,}// Store the raw URL/subpath (String() may combine them for display).if gs, ok := src.(GitSource); ok {🟠 *GitSource type assertion only matches value type; silently drops URL/subpath for GitSource
correctness, maintainability · flagged by 2 models
skillpack/sync.go:102-105—Subscribedoessrc.(GitSource)to extract the rawURLandSubpathfields. Because allGitSourcemethods have value receivers,*GitSourcealso satisfiesSource; if a caller passes&GitSource{...}, the type assertion fails silently. The subscription then storessrc.String()(the display form, e.g.url//subpath) asSourceURL, and laterDefaultSourceForreconstructs the source with the combined string instead of the bare URL. Handle both `GitSo…🪰 Gadfly · advisory
@@ -0,0 +151,4 @@if err != nil {return nil, []error{err}}for i := range subs {🟠 CheckAll processes subscriptions serially, blocking on slow sources
performance · flagged by 1 model
skillpack/sync.go:154—CheckAllprocesses subscriptions serially, blocking on slow sources. It loops over all subscriptions and callsCheck(which maygit clone) one at a time. A slow or unreachable source delays the entire sweep. Consider bounding concurrency (e.g.,errgroupwith a semaphore) so checks overlap.🪰 Gadfly · advisory
@@ -0,0 +183,4 @@if err != nil {return nil, err}sub.pinTo(pack, sub.PendingSourceRef, by, y.now())🟠 Apply silently renames subscription via pinTo when upstream changes pack name, without re-checking name uniqueness
correctness · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +30,4 @@Store: NewMemoryPackCache(),Subs: NewMemory(),Now: func() time.Time { return time.Unix(1000, 0) },NewID: func() string { n++; return "id-1" },🟡 NewID test closure increments a dead counter n but always returns constant "id-1"
maintainability · flagged by 2 models
skillpack/sync_test.go:33—NewIDclosure has a dead counter.nis incremented but the return is the constant"id-1", sonis never read. It reads like it's meant to produce unique IDs and silently doesn't. Either dropnand return a constant, or generate distinct ids (fmt.Sprintf("id-%d", n)). Verified atsync_test.go:27-35.🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 4 models
Verdict: Blocking issues found · 27 findings (10 with multi-model agreement)
skillpack/activation.go:121skillpack/sync.go:16skillpack/manifest.go:183skillpack/pack.go:101skillpack/source.go:97skillpack/sync.go:90skillpack/sync.go:102skillpack/manifest.go:79skillpack/sync.go:94skillpack/sync_test.go:3317 single-model findings (lower confidence)
skillpack/source.go:101skillpack/store.go:24skillpack/activation.go:47skillpack/activation.go:54skillpack/activation.go:79skillpack/activation.go:103skillpack/memory.go:45skillpack/pack.go:51skillpack/pack.go:95skillpack/source.go:113skillpack/sync.go:76skillpack/sync.go:154skillpack/sync.go:186skillpack/manifest.go:112skillpack/pack.go:125skillpack/activation.go:59skillpack/sync.go:126Per-model detail
deepseek-v4-pro:cloud (ollama-cloud) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Blocking issues found
All four findings are confirmed against the actual code. Here is the corrected review:
Security Review —
skillpackbatteryVerdict: Blocking issues found
source.go:97— Git command injection viag.URLingit cloneg.URLis passed directly as a positional argument togit clone --quiet <url> <dir>. If the URL string starts with-(e.g.,--config=protocol.ext.allow=alwaysor--upload-pack=/tmp/evil.sh), git interprets it as an option rather than a positional argument. This is a well-known attack class against git wrappers. A host that sourcesGitSource.URLfrom any user-influenced input (even indirectly, e.g. a stored subscription record) would be vulnerable.Fix: Insert
--before the URL:"clone", "--quiet", "--", g.URL, tmp. (Verified by readingsource.go:97— no--separator present.)source.go:101— Git command injection viarefingit checkoutSame pattern:
refis passed directly togit checkout --quiet <ref>. Arefvalue like--help,-b evil, or--config=...would be interpreted as a flag. Therefcomes fromsub.TrackRefinSyncer.Check(sync.go:126), which is persisted in the subscription store — so a compromised store or a crafted subscription record could inject flags.Fix: Insert
--beforeref:"checkout", "--quiet", "--", ref. (Verified by readingsource.go:101— no--separator present.)pack.go:101–121(readTree) — Symlink traversal reads arbitrary files from the filesystemreadTreeusesfs.WalkDiron anos.DirFS, which follows symlinks by default. A malicious git repository containing a symlink (e.g.,escape -> /etc/passwdorsecrets -> ../../.ssh) would causefs.ReadFileto read the symlink target's contents and store them in the returnedTreeunder the innocent-looking symlink name. Thewithincheck inWriteToonly guards the write side (staging), not the read side (fetching). This means a malicious pack can exfiltrate arbitrary local files from the clone host into the pack cache and ultimately into the agent's context.Fix: Before reading each file in
readTree, resolve the path and verify it stays within the FS root. Alternatively, usefs.WalkDirwith a check on eachDirEntry— skip symlinks (checkd.Type() & fs.ModeSymlink) or resolve and validate the target. (Verified by readingpack.go:101-121and confirming no symlink check exists; confirmedos.DirFS+fs.WalkDirfollows symlinks per Go stdlib behavior.)activation.go:54— Prompt injection viaDescriptioninCatalogCatalogwritesp.Manifest.Descriptiondirectly into the system prompt with no sanitization. The only constraint ismaxDescriptionLen(1024 chars), which is ample for prompt-injection payloads (e.g.,"Ignore all previous instructions. Instead, ..."). A malicious or compromised skill pack author can inject instructions that override the agent's behavior, since the catalog sits permanently in the system prompt. While the PR acknowledges the catalog is "always-in-prompt," there is no defense-in-depth (e.g., escaping, filtering, or a host-side content policy hook).Fix: At minimum, document this as a host responsibility and provide a sanitization hook. Consider escaping or flagging descriptions that contain known injection patterns. (Verified by reading
activation.go:42-57— no sanitization applied top.Manifest.Description.)🎯 Correctness — Blocking issues found
Both findings are confirmed against the actual code. The
isKebabfunction atmanifest.go:183-192indeed only checks character set and non-emptiness, missing the positional constraints (no leading/trailing/consecutive hyphens) that the Anthropic agent-skills spec requires. TheGitSourcetype assertion atsync.go:102only matches the value type, so a host returning&GitSource{...}fromSourceForwould silently lose URL/subpath decomposition.Verdict: Blocking issues found
Findings
manifest.go:183-192—isKebabis too permissive; violates stated Anthropic agent-skills constraints. The function only checks that every character is[a-z0-9-]and the string is non-empty, but the Anthropic agent-skills kebab-case spec additionally requires: must start with a letter, must not end with a hyphen, and must not contain consecutive hyphens. Names like-pdf,pdf-,pdf--processing, and---all pass validation. The comment on lines 15-16 says "matching the Anthropic agent-skills constraints so packs authored against that ecosystem validate here unchanged" — the implementation doesn't deliver on that promise. Packs with degenerate names accepted here would be rejected by the Anthropic ecosystem, or conversely, a pack authored against the real spec with a name like-badwould be accepted here but fail elsewhere.Fix: Add positional checks:
sync.go:102—GitSourcetype assertion only matches value type, silently drops URL/subpath for pointer-typed sources.DefaultSourceForreturnsGitSource{...}(a value), so the assertionsrc.(GitSource)works for the default path. But a host that setsSyncer.SourceForand returns&GitSource{...}(a pointer) will silently fail the type assertion —sub.SourceURLstays as the combinedString()output (e.g.url//subpath) andsub.Subpathstays empty. WhenDefaultSourceForlater reconstructs the source from the stored subscription, it will produce aGitSourcewith a mangled URL and no subpath, breaking futureCheck/Applycalls.Fix: Use a type switch or also check
*GitSource:🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
Findings
skillpack/sync.go:16-18—Syncerfield names are swapped relative to their typesStoreis typedPackCacheandSubsis typedStore. A reader seesy.Store.Get(...)and naturally expects aStoreinterface call, but it's actuallyPackCache.Get. Conversely,y.Subs.Get(...)is the realStore.Get. The comments ("content store for pinned trees" / "subscription metadata store") confirm the intent, but the field names mislead. RenameStore→Cache(orPackCache) andSubs→Storeso the names match the types.Verified by reading
sync.go:16-18and cross-referencing ally.Store./y.Subs.call sites.skillpack/sync_test.go:33— dead increment inNewIDclosureNewID: func() string { n++; return "id-1" }— thenvariable is incremented on every call but the return value is always the literal"id-1". The increment serves no purpose;nis never read. Either the intent wasfmt.Sprintf("id-%d", n)(and the literal is a bug), or thenvariable andn++are dead code that should be removed.Verified by reading
sync_test.go:27-35.skillpack/activation.go:121—Stageuses string concatenation instead offilepath.Joindir := baseDir + "/" + p.Manifest.Nameconstructs a filesystem path with a hardcoded/separator. The rest of the codebase consistently usesfilepath.Joinfor real filesystem paths (e.g.pack.go:53,source.go:114). On Windows this would produce a path with mixed separators. Usefilepath.Join(baseDir, p.Manifest.Name). (The similar pattern inrenderPackBody:107is a display string, not a filesystem path, so that one is fine.)Verified by reading
activation.go:120-121and comparing againstpack.go:53.⚡ Performance — Blocking issues found
All three findings are factually confirmed against the actual code. Let me verify the line numbers are precise:
Finding 1:
store.go:24-27—PackCacheinterface storesTree.activation.go:26-30—ResolvecallsGetthenLoadPack.pack.go:80-96—LoadPackcallsParseManifest+t.Digest().sync.go:67-80—fetchPackhas*Packbut stores onlypack.Tree. All confirmed.Finding 2:
source.go:97—g.run(ctx, "", "clone", "--quiet", g.URL, tmp)with no--depth 1or--single-branch. Confirmed.Finding 3:
pack.go:95—Digest: t.Digest()insideLoadPack. Caller inResolve(activation.go:26-30) hass.PinnedDigestin hand but doesn't pass it through. Also applies toApplyatsync.go:182. Confirmed.All three survive verification.
Verdict: Blocking issues found
PackCachestores rawTreeinstead of parsed*Pack— forces re-parse + re-hash on every activation (skillpack/store.go:24-27,skillpack/activation.go:19-37,skillpack/pack.go:80-96). ThePackCacheinterface storesTree(raw bytes), butResolve— which is on the activation hot path (every agent run) — callsLoadPack(tree)for each subscription.LoadPackre-parses the YAML frontmatter viaParseManifestand re-computes the SHA-256 content digest viat.Digest()(which sorts all paths, then hashes every file). All of this work is pure waste: the pack is immutable once fetched, and the digest is already known (it's the cache key used to retrieve the tree). ThefetchPackmethod insync.go:67-80already has the fully parsed*Packin hand but deliberately throws it away, storing onlypack.Tree. A host with N subscriptions re-does N YAML parses + N SHA-256 passes over all bundled files on every single agent run. The fix: either changePackCacheto store*Pack(or a serializable equivalent), or at minimum store the pre-computed digest + parsed manifest + bundled list alongside the tree soResolvecan skip re-derivation.*Verified by reading
store.go:24-27(PackCache interface),activation.go:26-30(Resolve calls Get then LoadPack),pack.go:80-96(LoadPack calls ParseManifest + t.Digest),sync.go:67-80(fetchPack has Pack but stores only pack.Tree).GitSource.Fetchclones full repo history — no--depth 1(skillpack/source.go:97). The clone command isgit clone --quiet g.URL tmpwith no--depth 1and no--single-branch. EverySubscribeand everyCheck(which runs on a host ticker viaCheckAll) clones the entire repository history when only the current tree state is needed. For a pack published from a repo with years of history, this downloads orders of magnitude more data than necessary and burns CPU on delta resolution. A shallow clone (--depth 1) would fetch only the tip commit's tree, which is allreadTreeever consumes. This multiplies with the issue above: every periodicCheckpays the full-clone cost just to discover the digest hasn't changed.Verified by reading
source.go:97— the clone args are"clone", "--quiet", g.URL, tmpwith no depth/single-branch flags.LoadPackre-computes digest that is already the cache key (skillpack/pack.go:95).Resolveretrieves a tree by digestd, thenLoadPackcallst.Digest()which re-hashes every file to produce… the same digestd. The digest could be passed through (e.g.,LoadPackWithDigest(t, d)) or, better, eliminated entirely if thePackCachestores the pre-parsed pack (see first finding). On its own this is a subset of the first issue, but it's independently fixable even if the cache interface stays as-is.Verified by reading
pack.go:95(Digest: t.Digest()) andactivation.go:26-30(caller hass.PinnedDigestin hand but doesn't pass it through).🧯 Error handling & edge cases — Minor issues
All four findings are confirmed against the actual code. Here is the corrected review:
Verdict: Minor issues
sync.go:90—GetByNameerror treated as "name available" (high): TheSubscribemethod checksif existing, err := y.Subs.GetByName(ctx, pack.Manifest.Name); err == nil. Any non-nil error fromGetByName— including transient DB errors, connection failures, etc. — is treated as "name not found," and the code proceeds to create what could be a duplicate subscription. OnlyErrNotFoundshould permit creation. A SQLite-backed store returning a transient I/O error would silently create duplicates. Fix: useerrors.Is(err, ErrNotFound)to gate the "already exists" branch, and propagate any other error. (Confirmed:sync.go:90useserr == nil;memory.go:45-55returnsErrNotFoundfor misses;skillpack.go:51definesErrNotFound; noerrors.Isusage exists in the package.)pack.go:51-64—WriteTohas no rollback on partial failure (medium): The loop writes files sequentially; ifos.WriteFilefails on the 3rd file (e.g., disk full), the first 2 files remain on disk with no cleanup. The caller gets an error but the directory is left in a partially-staged, inconsistent state. A host that retries staging could encounter stale files from the previous failed attempt. Fix: either write to a temp dir and rename, or clean up written files on error. (Confirmed:pack.go:51-64has no cleanup on error.)sync.go:90— TOCTOU race on duplicate-name check (medium): TheGetByNamecheck and subsequentSaveare not atomic. Two concurrentSubscribecalls for the same pack name can both pass theGetByNamecheck (both see "not found") and both callSave, creating two subscriptions with the same name. TheMemorystore locks per-operation (memory.go:26-32), not across the check-then-save span. Fix: theSavepath (or the store itself) should enforce uniqueness, e.g., by checking within the same lock critical section. (Confirmed:sync.go:85-111andmemory.go:26-55.)manifest.go:82— nil YAML metadata values become"<nil>"string (small):fmt.Sprintf("%v", v)for a nil YAML metadata value (e.g.,metadata: { key: }ormetadata: { key: null }) produces the literal string"<nil>"rather than an empty string or omitting the key. This is surprising and could leak into catalog rendering or host display. Fix: check for nil before formatting, or use a type switch. (Confirmed:manifest.go:79-84.)glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
VERDICT: Minor issues
Through the security lens, I verified the traversal defenses (WriteTo's
withincheck, the subpathpath.Clean("/"+...)neutralization) and they hold. The manifest parser is bounded. The main genuine finding is an argument-injection gap in git invocation.skillpack/source.go:97,101— git argument injection via un----separated positional args.g.run(ctx, "", "clone", "--quiet", g.URL, tmp)andg.run(ctx, tmp, "checkout", "--quiet", ref)passg.URLandrefdirectly togitwith no--terminator before them. A URL or ref beginning with-(e.g.--config=...,--upload-pack=..., or an option-likeTrackRef) is parsed by git as an option, not a positional argument. This is the same class as historical git argument-injection CVEs. The inputs normally come from the operator atSubscribetime, but this package is explicitly built for external/third-party sources and the PR's own threat model includes a "compromised upstream" / careless-actor posture; a host that ever lets a user (vs. a trusted operator) supply a URL or ref — or a subscription whoseTrackRef/SourceURLis later mutated — gets option injection. Fix: insert--before the user-controlled positional, e.g."clone", "--quiet", "--", g.URL, tmpand"checkout", "--quiet", "--", ref. (Verified by readingsource.golines 77-104;exec.CommandContextinvokesgitwith these args verbatim, no shell, so the only injection vector is option parsing — which--closes.)skillpack/manifest.go:79-83—Metadatavalues stringified viafmt.Sprintf("%v", ...)(low). Arbitrary YAML scalars/sequences inmetadataare flattened to strings. Not a security hole per se (no eval, no template), but a malicious pack can stuff unboundedmetadatacontent into what a host may render; there's no per-value length cap (onlybodyand name/description are bounded). The scanner buffer (~1 MiB) indirectly bounds total frontmatter, so this is minor — flagging only because it's untrusted-author content flowing into a passthrough field with no individual limit.No authz/injection issues in
Resolve/Activate/skill_use(the model-suppliedargs.Nameis only used as a map key, never as a path or command), andStage/WriteTotraversal protection verified sound.🎯 Correctness — Reviewed
All findings confirmed against the actual code.
Finding 1 (
sync.go:90): The duplicate-name check only branches onerr == nil(reject) vs. anything else (proceed). A non-ErrNotFounderror from a real DB-backedStore.GetByNameis silently swallowed, soSubscribeproceeds toSave, potentially creating a duplicate-name subscription.Memoryonly returnsErrNotFound, so this only bites non-memory stores — butStoreis explicitly a DB seam. Confirmed.Finding 2 (
sync.go:186+subscription.go:55):pinTounconditionally setss.Name = p.Manifest.Name.ApplycallspinTowithout any name-uniqueness re-check (unlikeSubscribe). An upstream rename colliding with an existing subscription's name produces a duplicate. Confirmed.Finding 3 (
manifest.go:186):isKebabonly checks each rune is[a-z0-9-]and non-empty — allows-foo,foo-,a--b. No in-repo spec reference to confirm the exact Anthropic constraint, so low confidence. Confirmed as described.Finding 4 (
sync.go:126):fetchPackcallsy.Store.Putunconditionally;CheckcallsfetchPackeven on the no-change path, so unchanged trees are re-cached on every check. Trivial waste, not a correctness bug. Confirmed.🧹 Code cleanliness & maintainability — Minor issues
All four findings are confirmed against the actual code.
VERDICT: Minor issues
skillpack/sync.go:17-18—Store/Subsfield names invert their interface types, a readability trap.Syncer.Storeis aPackCache(not theStore), andSyncer.Subsis theStore. Every reader has to mentally swap the names:y.Store.Put(...)puts a tree, andy.Subs.Save(...)saves a subscription. The doc comment onStorecompensates ("content store for pinned trees") but the field name actively misleads. Suggested fix: name them by role, e.g.Cache PackCacheandSubs Store(orMetaStore), so the type and the name agree. Verified atsync.go:17-18and call sites infetchPack/Subscribe/Check/Apply.skillpack/sync_test.go:33—NewIDclosure has a dead counter.nis incremented but the return is the constant"id-1", sonis never read. It reads like it's meant to produce unique IDs and silently doesn't. Either dropnand return a constant, or generate distinct ids (fmt.Sprintf("id-%d", n)). Verified atsync_test.go:27-35.skillpack/activation.go:107,121— path string concatenation instead offilepath.Join, inconsistent with the same package'sTree.WriteTo.StagebuildsbaseDir + "/" + p.Manifest.Nameby hand whilepack.go:49usesfilepath.Join(dir, filepath.FromSlash(p))for the same conceptual job.renderPackBodydoes similar manual joining withstrings.TrimRight(stagedDir, "/")+"/"+.... These are prompt/URL-ish strings rather than filesystem writes (forrenderPackBody), thoughStagedoes feedWriteTo, so it's cosmetic but a small maintainability wart. Verified atactivation.go:107,121vspack.go:49.skillpack/sync.go:97— set-then-overwrite forSourceURL/Subpath.SourceURLis initialized tosrc.String()and then immediately overwritten whensrcis aGitSource. The branch is the only place the real coordinates are extracted, so the initial assignment is dead for git sources and theString()form is silently discarded. A small extraction helper would remove the type-assertion special-case and the dead initial assignment. Verified atsync.go:94-105.⚡ Performance — Minor issues
Both findings confirmed against the actual code.
fetchPack(sync.go:76) unconditionally callsy.Store.PutbeforeCheckcompares digests at line 130. So everyCheck/CheckAlltick re-caches, andMemoryPackCache.Put(memory.go:92) callscloneTreewhich deep-copies the whole tree even when the digest is unchanged. Confirmed.GitSource.Fetch(source.go:97) runsgit clone --quietwith no--depth/shallow option and nols-remoteshort-circuit;Checkre-runs this every tick for every git-backed subscription. Confirmed (line is 97, not 88 as in the draft JSON — correcting).VERDICT: Minor issues
skillpack/sync.go:76(fetchPack) — everyCheckre-caches the pack tree even when nothing changed.fetchPackunconditionally callsy.Store.Put(ctx, pack.Digest, pack.Tree)at line 76.CheckcallsfetchPackon every tick (line 126), and the digest comparison that would skip work happens after thePut(line 130).MemoryPackCache.Put(skillpack/memory.go:92) thencloneTrees the entire tree — a fresh map plus a new byte slice per file — overwriting the identical entry. So a no-opCheckAllover N subscriptions deep-copies every pack's full byte set on every tick; for a durablePackCacheimpl the same call becomes a redundant persisted write. Fix: inCheck, compare the fetched digest tosub.PinnedDigestbefore caching and onlyPutwhen they differ (the unchanged path already just clears pending state at lines 130-132); or havefetchPacktake a "cache only if needed" flag.skillpack/source.go:97(GitSource.Fetch) — fullgit cloneon everyCheck, no shallow option. The clone isgit clone --quiet <url> <tmp>— full history, no--depth/filter flags, nols-remoteshort-circuit.Check/CheckAllre-run this on each tick for every git-backed subscription, so periodic sync pulls the complete repo history of every tracked pack each sweep. For the common "track default branch" case (ref == ""),--depth 1would avoid transferring history and is a large network/disk win; the subsequentrev-parse HEADand subpath read still work on a shallow clone. (For explicit SHAs, shallow needs more care, so gating--depth 1onref == ""or a branch is reasonable.) Medium; suggest adding shallow-clone for the default-branch path and/or a cheapls-remoteSHA compare before cloning when the SHA is already the pinned ref.Nothing else in-lens stood out:
Resolveis cache-only (no network),Catalog/Activateare one-shot per agent build,Tree.Digestis O(files) and only runs at fetch/load (not per activation), andMemory.GetByName's linear scan is fine for the in-memory default's expected scale.🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
skillpack/sync.go:90— swallowed store error in the duplicate-name check.Subscribetests for an existing subscription withif existing, err := y.Subs.GetByName(ctx, pack.Manifest.Name); err == nil { ... }. Only theerr == nilbranch is handled; any other error fromGetByName(a transient DB failure, connection error, anything that isn'tErrNotFound) is silently dropped and execution falls through to create a new subscription. That can produce a duplicate subscription (the very thing the guard exists to prevent) or just defer the failure to the subsequentSave. The correct pattern is to branch onerr == nil(duplicate),errors.Is(err, ErrNotFound)(proceed), and otherwise return the error. Confirmed againstsync.go:85-111; theMemoryimpl (memory.go:45-55) only ever returnsErrNotFound, so this bites any non-memoryStoreimplementation — exactly the DB-backed hosts this package is designed for.skillpack/manifest.go:112-131— BOM is claimed tolerated but isn't. The doc comment onsplitFrontmattersays "Leading blank lines/BOM are tolerated," but the scan loop (lines 126-134) only skips blank lines (len(bytes.TrimSpace(trimmed)) == 0) and then checksstring(bytes.TrimSpace(trimmed)) != "---".bytes.TrimSpacedoes not strip a UTF-8 BOM (0xEF 0xBB 0xBF), so a SKILL.md saved with a BOM (\xEF\xBB\xBF---) fails with "must start with a '---' frontmatter block." There is no BOM stripping anywhere in the package. Either strip a leading BOM before the scan loop or drop the claim from the comment.Both are edge cases on unhappy paths; nothing else in the package's error handling looked wrong after checking (LoadPack/empty-tree, WriteTo traversal guard, GitSource temp cleanup + subpath escape check, Resolve missing-cache, Apply no-pending, CheckAll error collection, and the frontmatter unterminated/empty cases are all handled).
kimi-k2.6:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues · ⚠️ 3/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 5: all chain targets failed
ollama-cloud/kimi-k2.6☁️ model returned an empty response
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 3: all chain targets failed
ollama-cloud/kimi-k2.6☁️ model returned an empty response
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
skillpack/sync.go:16-18—Syncerhas a field namedStorewhose type isPackCache, notStore. In the same package there is aStoreinterface (for subscriptions), so every reader will assumey.Storeis subscription metadata storage. The comment on the field even says "content store for pinned trees", but the name collision makes the code harder to read and easier to misuse. Rename the field toCacheto match its type.skillpack/sync.go:102-105—Subscribedoessrc.(GitSource)to extract the rawURLandSubpathfields. Because allGitSourcemethods have value receivers,*GitSourcealso satisfiesSource; if a caller passes&GitSource{...}, the type assertion fails silently. The subscription then storessrc.String()(the display form, e.g.url//subpath) asSourceURL, and laterDefaultSourceForreconstructs the source with the combined string instead of the bare URL. Handle bothGitSourceand*GitSource(or add an interface method for raw coordinates).skillpack/activation.go:121—Stagebuilds a path withbaseDir + "/" + p.Manifest.Name, whileTree.WriteToinpack.go:53usesfilepath.Join. The manual concatenation is inconsistent with the rest of the package and the broader codebase; preferfilepath.Join(baseDir, p.Manifest.Name)for platform-appropriate path construction.⚡ Performance — Minor issues
Verdict: Minor issues
skillpack/sync.go:154—CheckAllprocesses subscriptions serially, blocking on slow sources. It loops over all subscriptions and callsCheck(which maygit clone) one at a time. A slow or unreachable source delays the entire sweep. Consider bounding concurrency (e.g.,errgroupwith a semaphore) so checks overlap.skillpack/memory.go:45—GetByNamelinear scan causes O(n²) on bulkSubscribe.Memory.GetByNameiterates over all subscriptions to find a match by name, andSubscribecalls this to reject duplicates. If a host creates many subscriptions, the overall work is quadratic. Add aname→subscriptionindex or document that hosts with many subscriptions should use a DB-backedStore.🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6☁️ skipped (backed off until 00:06:08.346)
qwen3.5:397b-cloud (ollama-cloud) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Blocking issues found
VERDICT: Blocking issues found
skillpack/pack.go:101-120— Symlink following inreadTreeallows arbitrary file read (HIGH, verified)The
readTreefunction usesos.DirFS+fs.ReadFilewhich follows symlinks. The walk callback checksd.IsDir()but never checksd.Type()&fs.ModeSymlink != 0. An attacker controlling aDirSourcedirectory (or a git repo with symlinks) can place a symlink likeSKILL.md -> /etc/passwdorscripts/leak -> ../../.ssh/id_rsa. WhenFetchreads the tree, sensitive host files are pulled into theTreeand cached. Thewithin()check inWriteToonly validates paths after they're already in memory — the damage is done at read time.Fix: In
readTree, checkd.Type()&fs.ModeSymlink != 0and reject symlinks explicitly, or useos.Lstatto detect and skip them.skillpack/source.go:113-121— Subpath validation occurs afterpath.Clean, but symlink in subpath target not checked (MEDIUM, verified)The
GitSource.Fetchvalidates that the subpath doesn't escape viawithin(tmp, root), but this only checks the path string. If the subpath directory itself contains symlinks (e.g., adata -> /etcsymlink inside the valid subpath), those are followed byreadTree. Combined with the symlink issue above, this allows reading files outside the repo.Fix: Same as above — reject symlinks in
readTree.skillpack/activation.go:121— Path construction uses string concatenation instead offilepath.Join(small, verified)dir := baseDir + "/" + p.Manifest.Nameworks but is non-idiomatic and could be confusing. WhileisKebab()validation onNameprevents traversal here, usingfilepath.Join(baseDir, p.Manifest.Name)is clearer and safer by convention.Fix: Use
filepath.Join.🎯 Correctness — No material issues found
No material issues found
After thorough review through the correctness lens, I verified the following key areas:
Manifest parsing (
skillpack/manifest.go): VerifiedsplitFrontmattercorrectly handles YAML frontmatter with---delimiters,isKebabproperly validates kebab-case names (a-z, 0-9, -), and all limits (maxNameLen=64, maxDescriptionLen=1024, maxBodyBytes=1MiB) are correctly enforced inValidate().Digest computation (
skillpack/pack.go:26-35): The SHA-256 digest correctly uses sorted paths, hashes each file's content separately, and usespath\x00hash\nformat to prevent boundary attacks. Order-independent and content-sensitive as claimed.Path traversal protection (
skillpack/pack.go:125-131): Thewithin()function correctly usesfilepath.Reland rejects paths starting with... Verified testTestTreeWriteTo_RejectsTraversalconfirms../escape.txtis rejected.Git subpath handling (
skillpack/source.go:112-120): Usespath.Clean("/" + g.Subpath)to normalize and strip leading../, then validates withwithin(tmp, root)before accessing. Correctly prevents escaping the repo.Sync workflow (
skillpack/sync.go): VerifiedSubscribe→Check→Applyflow.Checkcorrectly records pending updates without moving the pin (supply-chain guard), andApplyis the only function that advances the pin.Activation (
skillpack/activation.go):Catalogcorrectly sorts packs by name,Activatebuilds the skill_use tool with proper name lookup, andStagecorrectly materializes files underbaseDir/<pack name>/.Memory stores (
skillpack/memory.go):cloneTreecorrectly deep-copies both the map and byte slices, ensuring cached trees are immutable.All logic paths verified against the checked-out code. No correctness bugs found.
🧹 Code cleanliness & maintainability — Minor issues
Verdict
Minor issues found
Findings
skillpack/sync.go:17-18 — Confusing field naming in
Syncerstruct. The fieldStoreis of typePackCache, whileSubsis of typeStore. This inversion (Storeholding aPackCache,Subsholding aStore) is counterintuitive and will cause maintenance confusion. Suggested fix: Rename toCache PackCacheandStore Store(orSubs Store→Subs Storeis acceptable, butStore PackCacheshould becomeCache PackCache). Verified: Read sync.go lines 11-28.skillpack/pack.go:125-131 — The
within()helper duplicates standard library functionality. Go 1.20+ providesfilepath.IsLocal()which performs the same path-traversal check. Keeping a custom implementation adds maintenance burden and risks divergence. Suggested fix: Replace withfilepath.IsLocal(rel)after thefilepath.Relcall (or inline the check usingfilepath.IsLocal(dest)directly if on Go 1.20+). Verified: Read pack.go lines 123-131; confirmed the logic matchesIsLocalsemantics.skillpack/sync.go:94-105 — Awkward URL handling in
Subscribe. The code assignssrc.String()tosub.SourceURL, then immediately overwrites it forGitSourceto extract rawURLandSubpathfields. This roundabout pattern obscures intent. Suggested fix: Build the subscription's source fields directly from the concrete source type before the type switch, or have each source type expose its components via an interface method. Verified: Read sync.go lines 85-111.skillpack/activation.go:59 — Inconsistent naming convention.
skillUseArgsuses camelCase while all other public-facing types in the package (Manifest, Pack, Tree, Subscription, etc.) use PascalCase. While unexported types can use camelCase, this struct is serialized to JSON for tool arguments and benefits from clearer naming. Suggested fix: Rename toSkillUseArgsfor consistency with package conventions. Verified: Read activation.go lines 59-61; confirmed it's the only camelCase type name in the package.⚡ Performance — No material issues found
VERDICT: No material issues found
After reviewing the change through the Performance lens, I found no material efficiency regressions:
Tree copying (memory.go:106-114):
cloneTreecopies all file bytes on cache Get/Put. This is an intentional immutability safeguard, not a regression — the alternative (sharing mutable trees) would introduce race conditions and corruption risks.Digest recomputation (pack.go:78):
LoadPackcallst.Digest()even when loading from cache. This is redundant but not a regression — SHA-256 over typical pack sizes is fast, andLoadPackruns at activation/sync time, not in a hot loop.Git clone per fetch (source.go:85-120):
GitSource.Fetchclones the entire repo on each Subscribe/Check. This is by design for self-contained trees and supply-chain safety; the repo is deleted immediately after reading. Not a regression — it's the mechanism.Catalog sorting (activation.go:46-47): Copies the slice before sorting. Minor allocation, but
Catalogis called at activation time with typically few packs — not a hot path.No unbounded growth or N+1 patterns: The sync/check loops iterate subscriptions once; cache lookups are O(1); no quadratic behavior introduced.
The design accepts some one-time costs (clone, digest, copy) to achieve content-addressed immutability and supply-chain safety. These are architectural trade-offs, not performance bugs.
🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues found
Error handling & edge case issues within my lens:
skillpack/activation.go:47,54—Catalog(packs []*Pack)will panic if any element in the slice isnil. The function checkslen(packs) == 0but does not defend against[]*Pack{nil}. A caller constructing a slice with a nil element (or a bug inResolvethat appends nil) would cause a runtime panic atsorted[i].Manifest.Name. Fix: Add a nil check in the loop (if p == nil { continue }) or document that nil elements are not permitted.skillpack/activation.go:79—Activate(packs []*Pack, stagedDir string)will panic if any pack in the input slice isnil. Same issue:byName[p.Manifest.Name]dereferences without checkingp != nil. Fix: Skip nil packs in the loop or return an error.skillpack/activation.go:103-104—renderPackBody(p *Pack, stagedDir string)will panic ifp == nilorp.Manifest == nil. This is called from theskill_usetool handler; whileActivatebuilds the map from valid packs, a future refactor or direct call could pass nil. Fix: Addif p == nil || p.Manifest == nil { return "Error: invalid pack" }.skillpack/activation.go:121—Stage(p *Pack, baseDir string)will panic ifp == nilorp.Manifest == nilatp.Manifest.Name. Fix: Add nil guard at function entry.Verification: I read
activation.golines 42-126 and confirmed all four functions accessp.Manifestfields without nil checks. TheLoadPackfunction (verified inpack.go:80-95) always returns aPackwith a non-nilManifest, but these four functions are public and accept*Packdirectly, so they should defend against malformed input.Outside my lens: The
Syncerstruct field naming (Store PackCachevsSubs Store) is confusing but not an error-handling issue.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.