feat(skillpack): SKILL.md-subscription battery #22

Merged
steve merged 3 commits from feat/skillpack-battery into main 2026-07-05 01:28:57 +00:00
Owner

Adds a new additive, nil-safe battery executus/skillpack for subscribing to skill packages published in the Anthropic agent-skills format (a SKILL.md manifest 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 a Source and 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 single skill_use tool.

What's here

  • ManifestSKILL.md frontmatter+body parse & validation (name/description required, kebab-case + length limits matching the agent-skills constraints, allowed-tools passthrough).
  • Tree / Pack / LoadPack — a self-contained file set with an order-independent SHA-256 content digest (the pin identity and the change signal), bundled-file listing, and traversal-safe staging.
  • SourceDirSource (local/builtin) and GitSource (clones to temp, reads the subpath into memory, cleans up → self-contained tree); Fetch returns the tree + the source's resolved ref.
  • Subscription + Store + content-addressed PackCache — with zero-dependency Memory defaults, mirroring the other batteries.
  • SyncerSubscribe pins; Check records a pending update but never moves the pin; Apply is the only re-pin (supply-chain guard: upstream can't silently change what an agent runs), always with an actor recorded.
  • Activate — resolved packs → a majordomo/agent.Skill (catalog instructions + one skill_use tool); Stage materializes bundled files for a sandbox.

Notes

  • majordomo needs no change — an earlier design assumed a new Describer interface; Activate instead builds the skill via majordomo/skill.New with the catalog as its instructions, so the boundary is cleaner than planned.
  • Host wiring (allow-listed sources, .skillpack commands, an Agent.SkillPacks field, allowed-tools narrowing, sandbox staging, a CheckAll ticker) is a separate, host-side step — this PR is mechanism only.
  • Optional follow-up: a contrib/store SQLite impl of Store/PackCache for the light-tier/canary hosts (mort backs these with its own DB).

Full unit + hermetic local-git tests; gofmt/vet clean; -race green. ~1,600 lines across 14 files.

🤖 Generated with Claude Code

Adds a new additive, nil-safe battery `executus/skillpack` for subscribing to skill packages published in the Anthropic agent-skills format (a `SKILL.md` manifest 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 a `Source` and 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 single `skill_use` tool. ## What's here - **Manifest** — `SKILL.md` frontmatter+body parse & validation (name/description required, kebab-case + length limits matching the agent-skills constraints, `allowed-tools` passthrough). - **Tree / Pack / LoadPack** — a self-contained file set with an order-independent SHA-256 **content digest** (the pin identity and the change signal), bundled-file listing, and traversal-safe staging. - **Source** — `DirSource` (local/builtin) and `GitSource` (clones to temp, reads the subpath into memory, cleans up → self-contained tree); `Fetch` returns the tree + the source's resolved ref. - **Subscription + Store + content-addressed PackCache** — with zero-dependency `Memory` defaults, mirroring the other batteries. - **Syncer** — `Subscribe` pins; `Check` records a **pending** update but never moves the pin; `Apply` is the **only** re-pin (supply-chain guard: upstream can't silently change what an agent runs), always with an actor recorded. - **Activate** — resolved packs → a `majordomo/agent.Skill` (catalog instructions + one `skill_use` tool); `Stage` materializes bundled files for a sandbox. ## Notes - **majordomo needs no change** — an earlier design assumed a new `Describer` interface; `Activate` instead builds the skill via `majordomo/skill.New` with the catalog as its instructions, so the boundary is cleaner than planned. - Host wiring (allow-listed sources, `.skillpack` commands, an `Agent.SkillPacks` field, `allowed-tools` narrowing, sandbox staging, a `CheckAll` ticker) is a **separate, host-side step** — this PR is mechanism only. - Optional follow-up: a `contrib/store` SQLite impl of `Store`/`PackCache` for the light-tier/canary hosts (mort backs these with its own DB). Full unit + hermetic local-git tests; `gofmt`/`vet` clean; `-race` green. ~1,600 lines across 14 files. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
steve added 1 commit 2026-07-05 00:00:12 +00:00
feat(skillpack): SKILL.md-subscription battery
executus CI / test (pull_request) Successful in 1m6s
Adversarial Review (Gadfly) / review (pull_request) Successful in 15m50s
bf0b67f9af
New additive, nil-safe battery for subscribing to skill packages in the
Anthropic agent-skills format (SKILL.md manifest + bundled files):

- Manifest/ParseManifest: SKILL.md frontmatter+body parse & validation
  (name/description required, allowed-tools passthrough, kebab/length limits)
- Tree/Pack/LoadPack: self-contained file set, order-independent content
  digest (the pin identity + change signal), bundled-file listing, traversal-
  safe staging
- Source (DirSource, GitSource): Fetch returns tree + resolved ref; git clones
  to temp, reads subpath into memory, cleans up (self-contained tree)
- Subscription + Store + content-addressed PackCache, with Memory defaults
- Syncer: Subscribe pins; Check records a PENDING update but never moves the
  pin; Apply is the only re-pin (supply-chain guard — upstream can't silently
  change what an agent runs)
- Activate: resolved packs -> majordomo agent.Skill (catalog instructions +
  one skill_use tool) for progressive disclosure; Stage materializes files

Third distinct 'skill' concept, deliberately separate from executus/skill
(saved-agent noun) and majordomo/skill (eager capability bundle). Mort-side
wiring (convars, .skillpack commands, Agent.SkillPacks, allowed-tools shim)
is a later, separate step. Full unit + hermetic local-git tests; gofmt/vet
clean; race-tested.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

🪰 Gadfly — live review status

4/4 reviewers finished · updated 2026-07-05 00:16:01Z

deepseek-v4-pro:cloud · ollama-cloud — done

  • security — Blocking issues found
  • correctness — Blocking issues found
  • maintainability — Minor issues
  • performance — Blocking issues found
  • error-handling — Minor issues

glm-5.2:cloud · ollama-cloud — done

  • security — Minor issues
  • correctness — Reviewed
  • maintainability — Minor issues
  • performance — Minor issues
  • error-handling — Minor issues

kimi-k2.6:cloud · ollama-cloud — done

  • ⚠️ security — could not complete
  • ⚠️ correctness — could not complete
  • maintainability — Minor issues
  • performance — Minor issues
  • ⚠️ error-handling — could not complete

qwen3.5:397b-cloud · ollama-cloud — done

  • security — Blocking issues found
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — No material issues found
  • error-handling — Minor issues

Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.

<!-- gadfly-status-board --> ## 🪰 Gadfly — live review status 4/4 reviewers finished · updated 2026-07-05 00:16:01Z #### `deepseek-v4-pro:cloud` · ollama-cloud — ✅ done - ✅ **security** — Blocking issues found - ✅ **correctness** — Blocking issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — Blocking issues found - ✅ **error-handling** — Minor issues #### `glm-5.2:cloud` · ollama-cloud — ✅ done - ✅ **security** — Minor issues - ✅ **correctness** — Reviewed - ✅ **maintainability** — Minor issues - ✅ **performance** — Minor issues - ✅ **error-handling** — Minor issues #### `kimi-k2.6:cloud` · ollama-cloud — ✅ done - ⚠️ **security** — could not complete - ⚠️ **correctness** — could not complete - ✅ **maintainability** — Minor issues - ✅ **performance** — Minor issues - ⚠️ **error-handling** — could not complete #### `qwen3.5:397b-cloud` · ollama-cloud — ✅ done - ✅ **security** — Blocking issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues <sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
gitea-actions bot reviewed 2026-07-05 00:16:01 +00:00
gitea-actions bot left a comment

🪰 Gadfly consensus review — 25 inline findings on changed lines. See the consensus comment for the full ranked summary.

Advisory only — does not block merge.

<!-- gadfly-inline-review --> 🪰 **Gadfly consensus review** — 25 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -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,54Catalog(packs []*Pack) will panic if any element in the slice is nil. The function checks len(packs) == 0 but does not defend against []*Pack{nil}. A caller constructing a slice with a nil element (or a bug in Resolve that appends nil) would cause a runtime panic at sorted[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

🟠 **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 is `nil`. The function checks `len(packs) == 0` but does not defend against `[]*Pack{nil}`. A caller constructing a slice with a nil element (or a bug in `Resolve` that appends nil) would cause a runtime panic at `sorted[i].Manifest.Name`. **Fix:** Add a nil check in the loop (`if p == nil { continue }`) or document that nil elements are not permitted. <sub>🪰 Gadfly · advisory</sub>
@@ -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

🟠 **Prompt injection via unsanitized Description field injected directly into system prompt Catalog** _security · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -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:79Activate(packs []*Pack, stagedDir string) will panic if any pack in the input slice is nil. Same issue: byName[p.Manifest.Name] dereferences without checking p != nil. Fix: Skip nil packs in the loop or return an error.

🪰 Gadfly · advisory

🟠 **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 is `nil`. Same issue: `byName[p.Manifest.Name]` dereferences without checking `p != nil`. **Fix:** Skip nil packs in the loop or return an error. <sub>🪰 Gadfly · advisory</sub>
@@ -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.Builder
fmt.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-104renderPackBody(p *Pack, stagedDir string) will panic if p == nil or p.Manifest == nil. This is called from the skill_use tool handler; while Activate builds the map from valid packs, a future refactor or direct call could pass nil. Fix: Add if p == nil || p.Manifest == nil { return "Error: invalid pack" }.

🪰 Gadfly · advisory

🟠 **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** if `p == nil` or `p.Manifest == nil`. This is called from the `skill_use` tool handler; while `Activate` builds the map from valid packs, a future refactor or direct call could pass nil. **Fix:** Add `if p == nil || p.Manifest == nil { return "Error: invalid pack" }`. <sub>🪰 Gadfly · advisory</sub>
@@ -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:121Stage(p *Pack, baseDir string) will panic if p == nil or p.Manifest == nil at p.Manifest.Name. Fix: Add nil guard at function entry.

🪰 Gadfly · advisory

🟠 **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** if `p == nil` or `p.Manifest == nil` at `p.Manifest.Name`. **Fix:** Add nil guard at function entry. <sub>🪰 Gadfly · advisory</sub>
@@ -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-83Metadata values stringified via fmt.Sprintf("%v", ...) (low). Arbitrary YAML scalars/sequences in metadata are flattened to strings. Not a security hole per se (no eval, no template), but a malicious pack can stuff unbounded metadata content into what a host may render; there's no per-value length cap (only body and name/description are bounded). The scanner buffer (~1 MiB) indirectly bounds total frontmatter, so this is minor — flagging only becau…

🪰 Gadfly · advisory

🟡 **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` — `Metadata` values stringified via `fmt.Sprintf("%v", ...)` (low).** Arbitrary YAML scalars/sequences in `metadata` are flattened to strings. Not a security hole per se (no eval, no template), but a malicious pack can stuff unbounded `metadata` content into what a host may render; there's no per-value length cap (only `body` and name/description are bounded). The scanner buffer (~1 MiB) indirectly bounds total frontmatter, so this is minor — flagging only becau… <sub>🪰 Gadfly · advisory</sub>
@@ -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 on splitFrontmatter says "Leading blank lines/BOM are tolerated," but the scan loop (lines 126-134) only skips blank lines (len(bytes.TrimSpace(trimmed)) == 0) and then checks string(bytes.TrimSpace(trimmed)) != "---". bytes.TrimSpace does 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

🟡 **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 on `splitFrontmatter` says "Leading blank lines/BOM are tolerated," but the scan loop (lines 126-134) only skips blank lines (`len(bytes.TrimSpace(trimmed)) == 0`) and then checks `string(bytes.TrimSpace(trimmed)) != "---"`. `bytes.TrimSpace` does 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… <sub>🪰 Gadfly · advisory</sub>
@@ -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

🔴 **isKebab allows leading/trailing/consecutive hyphens, violating Anthropic agent-skills kebab-case constraints** _correctness · flagged by 2 models_ <sub>🪰 Gadfly · advisory</sub>
@@ -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:45GetByName linear scan causes O(n²) on bulk Subscribe. Memory.GetByName iterates over all subscriptions to find a match by name, and Subscribe calls this to reject duplicates. If a host creates many subscriptions, the overall work is quadratic. Add a name→subscription index or document that hosts with many subscriptions should use a DB-backed Store.

🪰 Gadfly · advisory

🟠 **GetByName linear scan causes O(n²) on bulk Subscribe** _performance · flagged by 1 model_ - **`skillpack/memory.go:45`** — `GetByName` linear scan causes O(n²) on bulk `Subscribe`. `Memory.GetByName` iterates over all subscriptions to find a match by name, and `Subscribe` calls this to reject duplicates. If a host creates many subscriptions, the overall work is quadratic. Add a `name→subscription` index or document that hosts with many subscriptions should use a DB-backed `Store`. <sub>🪰 Gadfly · advisory</sub>
@@ -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

🟠 **WriteTo has no rollback on partial failure — leaves partially-staged directory on error** _error-handling · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -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

  • LoadPack re-computes digest that is already the cache key (skillpack/pack.go:95). Resolve retrieves a tree by digest d, then LoadPack calls t.Digest() which re-hashes every file to produce… the same digest d. The digest could be passed through (e.g., LoadPackWithDigest(t, d)) or, better, eliminated entirely if the PackCache stores 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

🟠 **LoadPack re-computes content digest that is already known to the caller (the PackCache key)** _performance · flagged by 1 model_ - **`LoadPack` re-computes digest that is already the cache key** (`skillpack/pack.go:95`). `Resolve` retrieves a tree by digest `d`, then `LoadPack` calls `t.Digest()` which re-hashes every file to produce… the same digest `d`. The digest could be passed through (e.g., `LoadPackWithDigest(t, d)`) or, better, eliminated entirely if the `PackCache` stores 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… <sub>🪰 Gadfly · advisory</sub>
@@ -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 in readTree allows arbitrary file read (HIGH, verified) The readTree function uses os.DirFS + fs.ReadFile which follows symlinks. The walk callback checks d.IsDir() but never checks d.Type()&fs.ModeSymlink != 0. An attacker controlling a DirSource directory (or a git repo with symlinks) can place a symlink like SKILL.md -> /etc/passwd or scripts/leak -> ../../.ssh/id_rsa. When Fetch reads the tree, sensitive host files…

🪰 Gadfly · advisory

🔴 **readTree follows symlinks allowing arbitrary file read from DirSource/GitSource** _security · flagged by 2 models_ - **`skillpack/pack.go:101-120` — Symlink following in `readTree` allows arbitrary file read (HIGH, verified)** The `readTree` function uses `os.DirFS` + `fs.ReadFile` which **follows symlinks**. The walk callback checks `d.IsDir()` but never checks `d.Type()&fs.ModeSymlink != 0`. An attacker controlling a `DirSource` directory (or a git repo with symlinks) can place a symlink like `SKILL.md -> /etc/passwd` or `scripts/leak -> ../../.ssh/id_rsa`. When `Fetch` reads the tree, sensitive host files… <sub>🪰 Gadfly · advisory</sub>
@@ -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

  • skillpack/pack.go:125-131 — The within() helper duplicates standard library functionality. Go 1.20+ provides filepath.IsLocal() which performs the same path-traversal check. Keeping a custom implementation adds maintenance burden and risks divergence. Suggested fix: Replace with filepath.IsLocal(rel) after the filepath.Rel call (or inline the check using filepath.IsLocal(dest) directly if on Go 1.20+). Verified: Read pack.go lines 123-131; confirmed the logic matches `IsL…

🪰 Gadfly · advisory

🟡 **within() duplicates filepath.IsLocal() from Go 1.20+** _maintainability · flagged by 1 model_ - **skillpack/pack.go:125-131** — The `within()` helper duplicates standard library functionality. Go 1.20+ provides `filepath.IsLocal()` which performs the same path-traversal check. Keeping a custom implementation adds maintenance burden and risks divergence. **Suggested fix:** Replace with `filepath.IsLocal(rel)` after the `filepath.Rel` call (or inline the check using `filepath.IsLocal(dest)` directly if on Go 1.20+). **Verified:** Read pack.go lines 123-131; confirmed the logic matches `IsL… <sub>🪰 Gadfly · advisory</sub>
@@ -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.Fetch clones full repo history — no --depth 1 (skillpack/source.go:97). The clone command is git clone --quiet g.URL tmp with no --depth 1 and no --single-branch. Every Subscribe and every Check (which runs on a host ticker via CheckAll) 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

🔴 **Git command injection via g.URL in git clone — no -- separator before user-controlled URL** _performance, security · flagged by 2 models_ - **`GitSource.Fetch` clones full repo history — no `--depth 1`** (`skillpack/source.go:97`). The clone command is `git clone --quiet g.URL tmp` with no `--depth 1` and no `--single-branch`. Every `Subscribe` and every `Check` (which runs on a host ticker via `CheckAll`) 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… <sub>🪰 Gadfly · advisory</sub>
@@ -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

🔴 **Git command injection via ref in git checkout — no -- separator before user-controlled ref** _security · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +110,4 @@
root := tmp
if 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 after path.Clean, but symlink in subpath target not checked (MEDIUM, verified) The GitSource.Fetch validates that the subpath doesn't escape via within(tmp, root), but this only checks the path string. If the subpath directory itself contains symlinks (e.g., a data -> /etc symlink inside the valid subpath), those are followed by readTree. Combined with the symlink issue above, this allows reading files outside the repo.…

🪰 Gadfly · advisory

🟠 **GitSource subpath validation does not account for symlinks within subpath** _security · flagged by 1 model_ - **`skillpack/source.go:113-121` — Subpath validation occurs after `path.Clean`, but symlink in subpath target not checked (MEDIUM, verified)** The `GitSource.Fetch` validates that the subpath doesn't escape via `within(tmp, root)`, but this only checks the *path string*. If the subpath directory itself contains symlinks (e.g., a `data -> /etc` symlink inside the valid subpath), those are followed by `readTree`. Combined with the symlink issue above, this allows reading files outside the repo.… <sub>🪰 Gadfly · advisory</sub>
@@ -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

  • PackCache stores raw Tree instead 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). The PackCache interface stores Tree (raw bytes), but Resolve — which is on the activation hot path (every agent run) — calls LoadPack(tree) for each subscription. LoadPack re-parses the YAML frontmatter via ParseManifest and re-computes the SHA-256 content digest via t.Digest() (wh…

🪰 Gadfly · advisory

🔴 **PackCache stores raw Tree forcing YAML re-parse + SHA-256 re-hash on every agent activation via Resolve→LoadPack** _performance · flagged by 1 model_ - **`PackCache` stores raw `Tree` instead 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`). The `PackCache` interface stores `Tree` (raw bytes), but `Resolve` — which is on the **activation hot path** (every agent run) — calls `LoadPack(tree)` for each subscription. `LoadPack` re-parses the YAML frontmatter via `ParseManifest` and re-computes the SHA-256 content digest via `t.Digest()` (wh… <sub>🪰 Gadfly · advisory</sub>
@@ -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-18Syncer has a field named Store whose type is PackCache, not Store. In the same package there is a Store interface (for subscriptions), so every reader will assume y.Store is 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 to Cache to match its type.

🪰 Gadfly · advisory

🟠 **Syncer field Store has type PackCache, colliding with Store interface name** _maintainability · flagged by 4 models_ - `skillpack/sync.go:16-18` — `Syncer` has a field named `Store` whose type is `PackCache`, not `Store`. In the same package there is a `Store` interface (for subscriptions), so every reader will assume `y.Store` is 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 to `Cache` to match its type. <sub>🪰 Gadfly · advisory</sub>
@@ -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) — every Check re-caches the pack tree even when nothing changed. fetchPack unconditionally calls y.Store.Put(ctx, pack.Digest, pack.Tree) at line 76. Check calls fetchPack on every tick (line 126), and the digest comparison that would skip work happens after the Put (line 130). MemoryPackCache.Put (skillpack/memory.go:92) then cloneTrees the entire tree — a fresh map plus a new byte slice per file — overwriting the identical entry. S…

🪰 Gadfly · advisory

🟠 **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`) — every `Check` re-caches the pack tree even when nothing changed.** `fetchPack` unconditionally calls `y.Store.Put(ctx, pack.Digest, pack.Tree)` at line 76. `Check` calls `fetchPack` on every tick (line 126), and the digest comparison that would skip work happens *after* the `Put` (line 130). `MemoryPackCache.Put` (`skillpack/memory.go:92`) then `cloneTree`s the entire tree — a fresh map plus a new byte slice per file — overwriting the identical entry. S… <sub>🪰 Gadfly · advisory</sub>
@@ -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. Subscribe tests for an existing subscription with if existing, err := y.Subs.GetByName(ctx, pack.Manifest.Name); err == nil { ... }. Only the err == nil branch is handled; any other error from GetByName (a transient DB failure, connection error, anything that isn't ErrNotFound) is silently dropped and execution falls through to create a new subscription. That can produce a duplicate subscription (the ve…

🪰 Gadfly · advisory

🔴 **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.** `Subscribe` tests for an existing subscription with `if existing, err := y.Subs.GetByName(ctx, pack.Manifest.Name); err == nil { ... }`. Only the `err == nil` branch is handled; any *other* error from `GetByName` (a transient DB failure, connection error, anything that isn't `ErrNotFound`) is silently dropped and execution falls through to create a new subscription. That can produce a duplicate subscription (the ve… <sub>🪰 Gadfly · advisory</sub>
@@ -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

  • skillpack/sync.go:94-105 — Awkward URL handling in Subscribe. The code assigns src.String() to sub.SourceURL, then immediately overwrites it for GitSource to extract raw URL and Subpath fields. 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

🟡 **Subscribe has awkward URL assignment that overwrites src.String() for GitSource** _maintainability · flagged by 2 models_ - **skillpack/sync.go:94-105** — Awkward URL handling in `Subscribe`. The code assigns `src.String()` to `sub.SourceURL`, then immediately overwrites it for `GitSource` to extract raw `URL` and `Subpath` fields. 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. <sub>🪰 Gadfly · advisory</sub>
@@ -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-105Subscribe does src.(GitSource) to extract the raw URL and Subpath fields. Because all GitSource methods have value receivers, *GitSource also satisfies Source; if a caller passes &GitSource{...}, the type assertion fails silently. The subscription then stores src.String() (the display form, e.g. url//subpath) as SourceURL, and later DefaultSourceFor reconstructs the source with the combined string instead of the bare URL. Handle both `GitSo…

🪰 Gadfly · advisory

🟠 **GitSource type assertion only matches value type; silently drops URL/subpath for *GitSource** _correctness, maintainability · flagged by 2 models_ - `skillpack/sync.go:102-105` — `Subscribe` does `src.(GitSource)` to extract the raw `URL` and `Subpath` fields. Because all `GitSource` methods have value receivers, `*GitSource` also satisfies `Source`; if a caller passes `&GitSource{...}`, the type assertion fails silently. The subscription then stores `src.String()` (the display form, e.g. `url//subpath`) as `SourceURL`, and later `DefaultSourceFor` reconstructs the source with the combined string instead of the bare URL. Handle both `GitSo… <sub>🪰 Gadfly · advisory</sub>
@@ -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:154CheckAll processes subscriptions serially, blocking on slow sources. It loops over all subscriptions and calls Check (which may git clone) one at a time. A slow or unreachable source delays the entire sweep. Consider bounding concurrency (e.g., errgroup with a semaphore) so checks overlap.

🪰 Gadfly · advisory

🟠 **CheckAll processes subscriptions serially, blocking on slow sources** _performance · flagged by 1 model_ - **`skillpack/sync.go:154`** — `CheckAll` processes subscriptions serially, blocking on slow sources. It loops over all subscriptions and calls `Check` (which may `git clone`) one at a time. A slow or unreachable source delays the entire sweep. Consider bounding concurrency (e.g., `errgroup` with a semaphore) so checks overlap. <sub>🪰 Gadfly · advisory</sub>
@@ -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

🟠 **Apply silently renames subscription via pinTo when upstream changes pack name, without re-checking name uniqueness** _correctness · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -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:33NewID closure has a dead counter. n is incremented but the return is the constant "id-1", so n is never read. It reads like it's meant to produce unique IDs and silently doesn't. Either drop n and return a constant, or generate distinct ids (fmt.Sprintf("id-%d", n)). Verified at sync_test.go:27-35.

🪰 Gadfly · advisory

🟡 **NewID test closure increments a dead counter n but always returns constant "id-1"** _maintainability · flagged by 2 models_ - **`skillpack/sync_test.go:33` — `NewID` closure has a dead counter.** `n` is incremented but the return is the constant `"id-1"`, so `n` is never read. It reads like it's meant to produce unique IDs and silently doesn't. Either drop `n` and return a constant, or generate distinct ids (`fmt.Sprintf("id-%d", n)`). Verified at `sync_test.go:27-35`. <sub>🪰 Gadfly · advisory</sub>

🪰 Gadfly review — consensus across 4 models

Verdict: Blocking issues found · 27 findings (10 with multi-model agreement)

Finding Where Models Lens
🟠 Stage panics on nil Pack or nil Manifest skillpack/activation.go:121 4/4 error-handling, maintainability, security
🟠 Syncer field Store has type PackCache, colliding with Store interface name skillpack/sync.go:16 4/4 maintainability
🔴 isKebab allows leading/trailing/consecutive hyphens, violating Anthropic agent-skills kebab-case constraints skillpack/manifest.go:183 2/4 correctness
🔴 readTree follows symlinks allowing arbitrary file read from DirSource/GitSource skillpack/pack.go:101 2/4 security
🔴 Git command injection via g.URL in git clone — no -- separator before user-controlled URL skillpack/source.go:97 2/4 performance, security
🔴 GetByName error treated as 'name available' — any non-ErrNotFound error silently allows duplicate subscriptions skillpack/sync.go:90 2/4 correctness, error-handling
🟠 GitSource type assertion only matches value type; silently drops URL/subpath for *GitSource skillpack/sync.go:102 2/4 correctness, maintainability
🟡 metadata passthrough values are unbounded (only name/description/body are length-capped); untrusted author can embed large arbitrary metadata skillpack/manifest.go:79 2/4 error-handling, security
🟡 Subscribe has awkward URL assignment that overwrites src.String() for GitSource skillpack/sync.go:94 2/4 maintainability
🟡 NewID test closure increments a dead counter n but always returns constant "id-1" skillpack/sync_test.go:33 2/4 maintainability
17 single-model findings (lower confidence)
Finding Where Model Lens
🔴 Git command injection via ref in git checkout — no -- separator before user-controlled ref skillpack/source.go:101 deepseek-v4-pro:cloud security
🔴 PackCache stores raw Tree forcing YAML re-parse + SHA-256 re-hash on every agent activation via Resolve→LoadPack skillpack/store.go:24 deepseek-v4-pro:cloud performance
🟠 Catalog panics on nil pack element in slice skillpack/activation.go:47 qwen3.5:397b-cloud error-handling
🟠 Prompt injection via unsanitized Description field injected directly into system prompt Catalog skillpack/activation.go:54 deepseek-v4-pro:cloud security
🟠 Activate panics on nil pack element in slice skillpack/activation.go:79 qwen3.5:397b-cloud error-handling
🟠 renderPackBody panics on nil Pack or nil Manifest skillpack/activation.go:103 qwen3.5:397b-cloud error-handling
🟠 GetByName linear scan causes O(n²) on bulk Subscribe skillpack/memory.go:45 kimi-k2.6:cloud performance
🟠 WriteTo has no rollback on partial failure — leaves partially-staged directory on error skillpack/pack.go:51 deepseek-v4-pro:cloud error-handling
🟠 LoadPack re-computes content digest that is already known to the caller (the PackCache key) skillpack/pack.go:95 deepseek-v4-pro:cloud performance
🟠 GitSource subpath validation does not account for symlinks within subpath skillpack/source.go:113 qwen3.5:397b-cloud security
🟠 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 skillpack/sync.go:76 glm-5.2:cloud performance
🟠 CheckAll processes subscriptions serially, blocking on slow sources skillpack/sync.go:154 kimi-k2.6:cloud performance
🟠 Apply silently renames subscription via pinTo when upstream changes pack name, without re-checking name uniqueness skillpack/sync.go:186 glm-5.2:cloud correctness
🟡 splitFrontmatter claims BOM tolerance but does not strip a leading BOM, so BOM-prefixed SKILL.md fails to parse skillpack/manifest.go:112 glm-5.2:cloud error-handling
🟡 within() duplicates filepath.IsLocal() from Go 1.20+ skillpack/pack.go:125 qwen3.5:397b-cloud maintainability
skillUseArgs uses camelCase inconsistent with package PascalCase convention skillpack/activation.go:59 qwen3.5:397b-cloud maintainability
Check re-caches the unchanged tree on every no-op check via fetchPack's unconditional Store.Put skillpack/sync.go:126 glm-5.2:cloud correctness
Per-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 — skillpack battery

Verdict: Blocking issues found


  • source.go:97 — Git command injection via g.URL in git clone
    g.URL is passed directly as a positional argument to git clone --quiet <url> <dir>. If the URL string starts with - (e.g., --config=protocol.ext.allow=always or --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 sources GitSource.URL from 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 reading source.go:97 — no -- separator present.)

  • source.go:101 — Git command injection via ref in git checkout
    Same pattern: ref is passed directly to git checkout --quiet <ref>. A ref value like --help, -b evil, or --config=... would be interpreted as a flag. The ref comes from sub.TrackRef in Syncer.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 -- before ref: "checkout", "--quiet", "--", ref. (Verified by reading source.go:101 — no -- separator present.)

  • pack.go:101–121 (readTree) — Symlink traversal reads arbitrary files from the filesystem
    readTree uses fs.WalkDir on an os.DirFS, which follows symlinks by default. A malicious git repository containing a symlink (e.g., escape -> /etc/passwd or secrets -> ../../.ssh) would cause fs.ReadFile to read the symlink target's contents and store them in the returned Tree under the innocent-looking symlink name. The within check in WriteTo only 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, use fs.WalkDir with a check on each DirEntry — skip symlinks (check d.Type() & fs.ModeSymlink) or resolve and validate the target. (Verified by reading pack.go:101-121 and confirming no symlink check exists; confirmed os.DirFS + fs.WalkDir follows symlinks per Go stdlib behavior.)

  • activation.go:54 — Prompt injection via Description in Catalog
    Catalog writes p.Manifest.Description directly into the system prompt with no sanitization. The only constraint is maxDescriptionLen (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 to p.Manifest.Description.)


🎯 Correctness — Blocking issues found

Both findings are confirmed against the actual code. The isKebab function at manifest.go:183-192 indeed only checks character set and non-emptiness, missing the positional constraints (no leading/trailing/consecutive hyphens) that the Anthropic agent-skills spec requires. The GitSource type assertion at sync.go:102 only matches the value type, so a host returning &GitSource{...} from SourceFor would silently lose URL/subpath decomposition.


Verdict: Blocking issues found

Findings

  • manifest.go:183-192isKebab is 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 -bad would be accepted here but fail elsewhere.

    Fix: Add positional checks:

    func isKebab(s string) bool {
        if s == "" || s[0] == '-' || s[len(s)-1] == '-' {
            return false
        }
        prev := rune(0)
        for _, r := range s {
            if !((r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') || r == '-') {
                return false
            }
            if r == '-' && prev == '-' {
                return false
            }
            prev = r
        }
        return true
    }
    
  • sync.go:102GitSource type assertion only matches value type, silently drops URL/subpath for pointer-typed sources. DefaultSourceFor returns GitSource{...} (a value), so the assertion src.(GitSource) works for the default path. But a host that sets Syncer.SourceFor and returns &GitSource{...} (a pointer) will silently fail the type assertion — sub.SourceURL stays as the combined String() output (e.g. url//subpath) and sub.Subpath stays empty. When DefaultSourceFor later reconstructs the source from the stored subscription, it will produce a GitSource with a mangled URL and no subpath, breaking future Check/Apply calls.

    Fix: Use a type switch or also check *GitSource:

    switch gs := src.(type) {
    case GitSource:
        sub.SourceURL = gs.URL
        sub.Subpath = gs.Subpath
    case *GitSource:
        sub.SourceURL = gs.URL
        sub.Subpath = gs.Subpath
    }
    
🧹 Code cleanliness & maintainability — Minor issues

Verdict: Minor issues

Findings

  • skillpack/sync.go:16-18Syncer field names are swapped relative to their types
    Store is typed PackCache and Subs is typed Store. A reader sees y.Store.Get(...) and naturally expects a Store interface call, but it's actually PackCache.Get. Conversely, y.Subs.Get(...) is the real Store.Get. The comments ("content store for pinned trees" / "subscription metadata store") confirm the intent, but the field names mislead. Rename StoreCache (or PackCache) and SubsStore so the names match the types.
    Verified by reading sync.go:16-18 and cross-referencing all y.Store. / y.Subs. call sites.

  • skillpack/sync_test.go:33 — dead increment in NewID closure
    NewID: func() string { n++; return "id-1" } — the n variable is incremented on every call but the return value is always the literal "id-1". The increment serves no purpose; n is never read. Either the intent was fmt.Sprintf("id-%d", n) (and the literal is a bug), or the n variable and n++ are dead code that should be removed.
    Verified by reading sync_test.go:27-35.

  • skillpack/activation.go:121Stage uses string concatenation instead of filepath.Join
    dir := baseDir + "/" + p.Manifest.Name constructs a filesystem path with a hardcoded / separator. The rest of the codebase consistently uses filepath.Join for real filesystem paths (e.g. pack.go:53, source.go:114). On Windows this would produce a path with mixed separators. Use filepath.Join(baseDir, p.Manifest.Name). (The similar pattern in renderPackBody:107 is a display string, not a filesystem path, so that one is fine.)
    Verified by reading activation.go:120-121 and comparing against pack.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-27PackCache interface stores Tree. activation.go:26-30Resolve calls Get then LoadPack. pack.go:80-96LoadPack calls ParseManifest + t.Digest(). sync.go:67-80fetchPack has *Pack but stores only pack.Tree. All confirmed.

  • Finding 2: source.go:97g.run(ctx, "", "clone", "--quiet", g.URL, tmp) with no --depth 1 or --single-branch. Confirmed.

  • Finding 3: pack.go:95Digest: t.Digest() inside LoadPack. Caller in Resolve (activation.go:26-30) has s.PinnedDigest in hand but doesn't pass it through. Also applies to Apply at sync.go:182. Confirmed.

All three survive verification.

Verdict: Blocking issues found

  • PackCache stores raw Tree instead 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). The PackCache interface stores Tree (raw bytes), but Resolve — which is on the activation hot path (every agent run) — calls LoadPack(tree) for each subscription. LoadPack re-parses the YAML frontmatter via ParseManifest and re-computes the SHA-256 content digest via t.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). The fetchPack method in sync.go:67-80 already has the fully parsed *Pack in hand but deliberately throws it away, storing only pack.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 change PackCache to store *Pack (or a serializable equivalent), or at minimum store the pre-computed digest + parsed manifest + bundled list alongside the tree so Resolve can 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.Fetch clones full repo history — no --depth 1 (skillpack/source.go:97). The clone command is git clone --quiet g.URL tmp with no --depth 1 and no --single-branch. Every Subscribe and every Check (which runs on a host ticker via CheckAll) 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 all readTree ever consumes. This multiplies with the issue above: every periodic Check pays 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, tmp with no depth/single-branch flags.

  • LoadPack re-computes digest that is already the cache key (skillpack/pack.go:95). Resolve retrieves a tree by digest d, then LoadPack calls t.Digest() which re-hashes every file to produce… the same digest d. The digest could be passed through (e.g., LoadPackWithDigest(t, d)) or, better, eliminated entirely if the PackCache stores 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()) and activation.go:26-30 (caller has s.PinnedDigest in 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:90GetByName error treated as "name available" (high): The Subscribe method checks if existing, err := y.Subs.GetByName(ctx, pack.Manifest.Name); err == nil. Any non-nil error from GetByName — 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. Only ErrNotFound should permit creation. A SQLite-backed store returning a transient I/O error would silently create duplicates. Fix: use errors.Is(err, ErrNotFound) to gate the "already exists" branch, and propagate any other error. (Confirmed: sync.go:90 uses err == nil; memory.go:45-55 returns ErrNotFound for misses; skillpack.go:51 defines ErrNotFound; no errors.Is usage exists in the package.)

  • pack.go:51-64WriteTo has no rollback on partial failure (medium): The loop writes files sequentially; if os.WriteFile fails 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-64 has no cleanup on error.)

  • sync.go:90 — TOCTOU race on duplicate-name check (medium): The GetByName check and subsequent Save are not atomic. Two concurrent Subscribe calls for the same pack name can both pass the GetByName check (both see "not found") and both call Save, creating two subscriptions with the same name. The Memory store locks per-operation (memory.go:26-32), not across the check-then-save span. Fix: the Save path (or the store itself) should enforce uniqueness, e.g., by checking within the same lock critical section. (Confirmed: sync.go:85-111 and memory.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: } or metadata: { 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 within check, the subpath path.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) and g.run(ctx, tmp, "checkout", "--quiet", ref) pass g.URL and ref directly to git with no -- terminator before them. A URL or ref beginning with - (e.g. --config=..., --upload-pack=..., or an option-like TrackRef) 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 at Subscribe time, 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 whose TrackRef/SourceURL is later mutated — gets option injection. Fix: insert -- before the user-controlled positional, e.g. "clone", "--quiet", "--", g.URL, tmp and "checkout", "--quiet", "--", ref. (Verified by reading source.go lines 77-104; exec.CommandContext invokes git with these args verbatim, no shell, so the only injection vector is option parsing — which -- closes.)

  • skillpack/manifest.go:79-83Metadata values stringified via fmt.Sprintf("%v", ...) (low). Arbitrary YAML scalars/sequences in metadata are flattened to strings. Not a security hole per se (no eval, no template), but a malicious pack can stuff unbounded metadata content into what a host may render; there's no per-value length cap (only body and 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-supplied args.Name is only used as a map key, never as a path or command), and Stage/WriteTo traversal protection verified sound.

🎯 Correctness — Reviewed

All findings confirmed against the actual code.

Finding 1 (sync.go:90): The duplicate-name check only branches on err == nil (reject) vs. anything else (proceed). A non-ErrNotFound error from a real DB-backed Store.GetByName is silently swallowed, so Subscribe proceeds to Save, potentially creating a duplicate-name subscription. Memory only returns ErrNotFound, so this only bites non-memory stores — but Store is explicitly a DB seam. Confirmed.

Finding 2 (sync.go:186 + subscription.go:55): pinTo unconditionally sets s.Name = p.Manifest.Name. Apply calls pinTo without any name-uniqueness re-check (unlike Subscribe). An upstream rename colliding with an existing subscription's name produces a duplicate. Confirmed.

Finding 3 (manifest.go:186): isKebab only 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): fetchPack calls y.Store.Put unconditionally; Check calls fetchPack even 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-18Store/Subs field names invert their interface types, a readability trap. Syncer.Store is a PackCache (not the Store), and Syncer.Subs is the Store. Every reader has to mentally swap the names: y.Store.Put(...) puts a tree, and y.Subs.Save(...) saves a subscription. The doc comment on Store compensates ("content store for pinned trees") but the field name actively misleads. Suggested fix: name them by role, e.g. Cache PackCache and Subs Store (or MetaStore), so the type and the name agree. Verified at sync.go:17-18 and call sites in fetchPack/Subscribe/Check/Apply.

  • skillpack/sync_test.go:33NewID closure has a dead counter. n is incremented but the return is the constant "id-1", so n is never read. It reads like it's meant to produce unique IDs and silently doesn't. Either drop n and return a constant, or generate distinct ids (fmt.Sprintf("id-%d", n)). Verified at sync_test.go:27-35.

  • skillpack/activation.go:107,121 — path string concatenation instead of filepath.Join, inconsistent with the same package's Tree.WriteTo. Stage builds baseDir + "/" + p.Manifest.Name by hand while pack.go:49 uses filepath.Join(dir, filepath.FromSlash(p)) for the same conceptual job. renderPackBody does similar manual joining with strings.TrimRight(stagedDir, "/")+"/"+.... These are prompt/URL-ish strings rather than filesystem writes (for renderPackBody), though Stage does feed WriteTo, so it's cosmetic but a small maintainability wart. Verified at activation.go:107,121 vs pack.go:49.

  • skillpack/sync.go:97 — set-then-overwrite for SourceURL/Subpath. SourceURL is initialized to src.String() and then immediately overwritten when src is a GitSource. The branch is the only place the real coordinates are extracted, so the initial assignment is dead for git sources and the String() form is silently discarded. A small extraction helper would remove the type-assertion special-case and the dead initial assignment. Verified at sync.go:94-105.

Performance — Minor issues

Both findings confirmed against the actual code.

  • Finding 1: fetchPack (sync.go:76) unconditionally calls y.Store.Put before Check compares digests at line 130. So every Check/CheckAll tick re-caches, and MemoryPackCache.Put (memory.go:92) calls cloneTree which deep-copies the whole tree even when the digest is unchanged. Confirmed.
  • Finding 2: GitSource.Fetch (source.go:97) runs git clone --quiet with no --depth/shallow option and no ls-remote short-circuit; Check re-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) — every Check re-caches the pack tree even when nothing changed. fetchPack unconditionally calls y.Store.Put(ctx, pack.Digest, pack.Tree) at line 76. Check calls fetchPack on every tick (line 126), and the digest comparison that would skip work happens after the Put (line 130). MemoryPackCache.Put (skillpack/memory.go:92) then cloneTrees the entire tree — a fresh map plus a new byte slice per file — overwriting the identical entry. So a no-op CheckAll over N subscriptions deep-copies every pack's full byte set on every tick; for a durable PackCache impl the same call becomes a redundant persisted write. Fix: in Check, compare the fetched digest to sub.PinnedDigest before caching and only Put when they differ (the unchanged path already just clears pending state at lines 130-132); or have fetchPack take a "cache only if needed" flag.

  • skillpack/source.go:97 (GitSource.Fetch) — full git clone on every Check, no shallow option. The clone is git clone --quiet <url> <tmp> — full history, no --depth/filter flags, no ls-remote short-circuit. Check/CheckAll re-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 1 would avoid transferring history and is a large network/disk win; the subsequent rev-parse HEAD and subpath read still work on a shallow clone. (For explicit SHAs, shallow needs more care, so gating --depth 1 on ref == "" or a branch is reasonable.) Medium; suggest adding shallow-clone for the default-branch path and/or a cheap ls-remote SHA compare before cloning when the SHA is already the pinned ref.

Nothing else in-lens stood out: Resolve is cache-only (no network), Catalog/Activate are one-shot per agent build, Tree.Digest is O(files) and only runs at fetch/load (not per activation), and Memory.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. Subscribe tests for an existing subscription with if existing, err := y.Subs.GetByName(ctx, pack.Manifest.Name); err == nil { ... }. Only the err == nil branch is handled; any other error from GetByName (a transient DB failure, connection error, anything that isn't ErrNotFound) 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 subsequent Save. The correct pattern is to branch on err == nil (duplicate), errors.Is(err, ErrNotFound) (proceed), and otherwise return the error. Confirmed against sync.go:85-111; the Memory impl (memory.go:45-55) only ever returns ErrNotFound, so this bites any non-memory Store implementation — 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 on splitFrontmatter says "Leading blank lines/BOM are tolerated," but the scan loop (lines 126-134) only skips blank lines (len(bytes.TrimSpace(trimmed)) == 0) and then checks string(bytes.TrimSpace(trimmed)) != "---". bytes.TrimSpace does 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-18Syncer has a field named Store whose type is PackCache, not Store. In the same package there is a Store interface (for subscriptions), so every reader will assume y.Store is 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 to Cache to match its type.

  • skillpack/sync.go:102-105Subscribe does src.(GitSource) to extract the raw URL and Subpath fields. Because all GitSource methods have value receivers, *GitSource also satisfies Source; if a caller passes &GitSource{...}, the type assertion fails silently. The subscription then stores src.String() (the display form, e.g. url//subpath) as SourceURL, and later DefaultSourceFor reconstructs the source with the combined string instead of the bare URL. Handle both GitSource and *GitSource (or add an interface method for raw coordinates).

  • skillpack/activation.go:121Stage builds a path with baseDir + "/" + p.Manifest.Name, while Tree.WriteTo in pack.go:53 uses filepath.Join. The manual concatenation is inconsistent with the rest of the package and the broader codebase; prefer filepath.Join(baseDir, p.Manifest.Name) for platform-appropriate path construction.

Performance — Minor issues

Verdict: Minor issues

  • skillpack/sync.go:154CheckAll processes subscriptions serially, blocking on slow sources. It loops over all subscriptions and calls Check (which may git clone) one at a time. A slow or unreachable source delays the entire sweep. Consider bounding concurrency (e.g., errgroup with a semaphore) so checks overlap.

  • skillpack/memory.go:45GetByName linear scan causes O(n²) on bulk Subscribe. Memory.GetByName iterates over all subscriptions to find a match by name, and Subscribe calls this to reject duplicates. If a host creates many subscriptions, the overall work is quadratic. Add a name→subscription index or document that hosts with many subscriptions should use a DB-backed Store.

🧯 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 in readTree allows arbitrary file read (HIGH, verified)
    The readTree function uses os.DirFS + fs.ReadFile which follows symlinks. The walk callback checks d.IsDir() but never checks d.Type()&fs.ModeSymlink != 0. An attacker controlling a DirSource directory (or a git repo with symlinks) can place a symlink like SKILL.md -> /etc/passwd or scripts/leak -> ../../.ssh/id_rsa. When Fetch reads the tree, sensitive host files are pulled into the Tree and cached. The within() check in WriteTo only validates paths after they're already in memory — the damage is done at read time.
    Fix: In readTree, check d.Type()&fs.ModeSymlink != 0 and reject symlinks explicitly, or use os.Lstat to detect and skip them.

  • skillpack/source.go:113-121 — Subpath validation occurs after path.Clean, but symlink in subpath target not checked (MEDIUM, verified)
    The GitSource.Fetch validates that the subpath doesn't escape via within(tmp, root), but this only checks the path string. If the subpath directory itself contains symlinks (e.g., a data -> /etc symlink inside the valid subpath), those are followed by readTree. 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 of filepath.Join (small, verified)
    dir := baseDir + "/" + p.Manifest.Name works but is non-idiomatic and could be confusing. While isKebab() validation on Name prevents traversal here, using filepath.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): Verified splitFrontmatter correctly handles YAML frontmatter with --- delimiters, isKebab properly validates kebab-case names (a-z, 0-9, -), and all limits (maxNameLen=64, maxDescriptionLen=1024, maxBodyBytes=1MiB) are correctly enforced in Validate().

Digest computation (skillpack/pack.go:26-35): The SHA-256 digest correctly uses sorted paths, hashes each file's content separately, and uses path\x00hash\n format to prevent boundary attacks. Order-independent and content-sensitive as claimed.

Path traversal protection (skillpack/pack.go:125-131): The within() function correctly uses filepath.Rel and rejects paths starting with ... Verified test TestTreeWriteTo_RejectsTraversal confirms ../escape.txt is rejected.

Git subpath handling (skillpack/source.go:112-120): Uses path.Clean("/" + g.Subpath) to normalize and strip leading ../, then validates with within(tmp, root) before accessing. Correctly prevents escaping the repo.

Sync workflow (skillpack/sync.go): Verified SubscribeCheckApply flow. Check correctly records pending updates without moving the pin (supply-chain guard), and Apply is the only function that advances the pin.

Activation (skillpack/activation.go): Catalog correctly sorts packs by name, Activate builds the skill_use tool with proper name lookup, and Stage correctly materializes files under baseDir/<pack name>/.

Memory stores (skillpack/memory.go): cloneTree correctly 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 Syncer struct. The field Store is of type PackCache, while Subs is of type Store. This inversion (Store holding a PackCache, Subs holding a Store) is counterintuitive and will cause maintenance confusion. Suggested fix: Rename to Cache PackCache and Store Store (or Subs StoreSubs Store is acceptable, but Store PackCache should become Cache PackCache). Verified: Read sync.go lines 11-28.

  • skillpack/pack.go:125-131 — The within() helper duplicates standard library functionality. Go 1.20+ provides filepath.IsLocal() which performs the same path-traversal check. Keeping a custom implementation adds maintenance burden and risks divergence. Suggested fix: Replace with filepath.IsLocal(rel) after the filepath.Rel call (or inline the check using filepath.IsLocal(dest) directly if on Go 1.20+). Verified: Read pack.go lines 123-131; confirmed the logic matches IsLocal semantics.

  • skillpack/sync.go:94-105 — Awkward URL handling in Subscribe. The code assigns src.String() to sub.SourceURL, then immediately overwrites it for GitSource to extract raw URL and Subpath fields. 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. skillUseArgs uses 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 to SkillUseArgs for 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): cloneTree copies 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): LoadPack calls t.Digest() even when loading from cache. This is redundant but not a regression — SHA-256 over typical pack sizes is fast, and LoadPack runs at activation/sync time, not in a hot loop.

  • Git clone per fetch (source.go:85-120): GitSource.Fetch clones 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 Catalog is 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,54Catalog(packs []*Pack) will panic if any element in the slice is nil. The function checks len(packs) == 0 but does not defend against []*Pack{nil}. A caller constructing a slice with a nil element (or a bug in Resolve that appends nil) would cause a runtime panic at sorted[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:79Activate(packs []*Pack, stagedDir string) will panic if any pack in the input slice is nil. Same issue: byName[p.Manifest.Name] dereferences without checking p != nil. Fix: Skip nil packs in the loop or return an error.

  • skillpack/activation.go:103-104renderPackBody(p *Pack, stagedDir string) will panic if p == nil or p.Manifest == nil. This is called from the skill_use tool handler; while Activate builds the map from valid packs, a future refactor or direct call could pass nil. Fix: Add if p == nil || p.Manifest == nil { return "Error: invalid pack" }.

  • skillpack/activation.go:121Stage(p *Pack, baseDir string) will panic if p == nil or p.Manifest == nil at p.Manifest.Name. Fix: Add nil guard at function entry.

Verification: I read activation.go lines 42-126 and confirmed all four functions access p.Manifest fields without nil checks. The LoadPack function (verified in pack.go:80-95) always returns a Pack with a non-nil Manifest, but these four functions are public and accept *Pack directly, so they should defend against malformed input.

Outside my lens: The Syncer struct field naming (Store PackCache vs Subs 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.

<!-- gadfly-consensus --> ## 🪰 Gadfly review — consensus across 4 models **Verdict: Blocking issues found** · 27 findings (10 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🟠 | Stage panics on nil Pack or nil Manifest | `skillpack/activation.go:121` | 4/4 | error-handling, maintainability, security | | 🟠 | Syncer field Store has type PackCache, colliding with Store interface name | `skillpack/sync.go:16` | 4/4 | maintainability | | 🔴 | isKebab allows leading/trailing/consecutive hyphens, violating Anthropic agent-skills kebab-case constraints | `skillpack/manifest.go:183` | 2/4 | correctness | | 🔴 | readTree follows symlinks allowing arbitrary file read from DirSource/GitSource | `skillpack/pack.go:101` | 2/4 | security | | 🔴 | Git command injection via g.URL in git clone — no -- separator before user-controlled URL | `skillpack/source.go:97` | 2/4 | performance, security | | 🔴 | GetByName error treated as 'name available' — any non-ErrNotFound error silently allows duplicate subscriptions | `skillpack/sync.go:90` | 2/4 | correctness, error-handling | | 🟠 | GitSource type assertion only matches value type; silently drops URL/subpath for *GitSource | `skillpack/sync.go:102` | 2/4 | correctness, maintainability | | 🟡 | metadata passthrough values are unbounded (only name/description/body are length-capped); untrusted author can embed large arbitrary metadata | `skillpack/manifest.go:79` | 2/4 | error-handling, security | | 🟡 | Subscribe has awkward URL assignment that overwrites src.String() for GitSource | `skillpack/sync.go:94` | 2/4 | maintainability | | 🟡 | NewID test closure increments a dead counter n but always returns constant "id-1" | `skillpack/sync_test.go:33` | 2/4 | maintainability | <details><summary>17 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🔴 | Git command injection via ref in git checkout — no -- separator before user-controlled ref | `skillpack/source.go:101` | deepseek-v4-pro:cloud | security | | 🔴 | PackCache stores raw Tree forcing YAML re-parse + SHA-256 re-hash on every agent activation via Resolve→LoadPack | `skillpack/store.go:24` | deepseek-v4-pro:cloud | performance | | 🟠 | Catalog panics on nil pack element in slice | `skillpack/activation.go:47` | qwen3.5:397b-cloud | error-handling | | 🟠 | Prompt injection via unsanitized Description field injected directly into system prompt Catalog | `skillpack/activation.go:54` | deepseek-v4-pro:cloud | security | | 🟠 | Activate panics on nil pack element in slice | `skillpack/activation.go:79` | qwen3.5:397b-cloud | error-handling | | 🟠 | renderPackBody panics on nil Pack or nil Manifest | `skillpack/activation.go:103` | qwen3.5:397b-cloud | error-handling | | 🟠 | GetByName linear scan causes O(n²) on bulk Subscribe | `skillpack/memory.go:45` | kimi-k2.6:cloud | performance | | 🟠 | WriteTo has no rollback on partial failure — leaves partially-staged directory on error | `skillpack/pack.go:51` | deepseek-v4-pro:cloud | error-handling | | 🟠 | LoadPack re-computes content digest that is already known to the caller (the PackCache key) | `skillpack/pack.go:95` | deepseek-v4-pro:cloud | performance | | 🟠 | GitSource subpath validation does not account for symlinks within subpath | `skillpack/source.go:113` | qwen3.5:397b-cloud | security | | 🟠 | 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 | `skillpack/sync.go:76` | glm-5.2:cloud | performance | | 🟠 | CheckAll processes subscriptions serially, blocking on slow sources | `skillpack/sync.go:154` | kimi-k2.6:cloud | performance | | 🟠 | Apply silently renames subscription via pinTo when upstream changes pack name, without re-checking name uniqueness | `skillpack/sync.go:186` | glm-5.2:cloud | correctness | | 🟡 | splitFrontmatter claims BOM tolerance but does not strip a leading BOM, so BOM-prefixed SKILL.md fails to parse | `skillpack/manifest.go:112` | glm-5.2:cloud | error-handling | | 🟡 | within() duplicates filepath.IsLocal() from Go 1.20+ | `skillpack/pack.go:125` | qwen3.5:397b-cloud | maintainability | | ⚪ | skillUseArgs uses camelCase inconsistent with package PascalCase convention | `skillpack/activation.go:59` | qwen3.5:397b-cloud | maintainability | | ⚪ | Check re-caches the unchanged tree on every no-op check via fetchPack's unconditional Store.Put | `skillpack/sync.go:126` | glm-5.2:cloud | correctness | </details> <details><summary>Per-model detail</summary> <details><summary><b>deepseek-v4-pro:cloud</b> (ollama-cloud) — Blocking issues found</summary> **Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — Blocking issues found</summary> All four findings are confirmed against the actual code. Here is the corrected review: --- # Security Review — `skillpack` battery **Verdict: Blocking issues found** --- - **`source.go:97` — Git command injection via `g.URL` in `git clone`** `g.URL` is passed directly as a positional argument to `git clone --quiet <url> <dir>`. If the URL string starts with `-` (e.g., `--config=protocol.ext.allow=always` or `--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 sources `GitSource.URL` from 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 reading `source.go:97` — no `--` separator present.) - **`source.go:101` — Git command injection via `ref` in `git checkout`** Same pattern: `ref` is passed directly to `git checkout --quiet <ref>`. A `ref` value like `--help`, `-b evil`, or `--config=...` would be interpreted as a flag. The `ref` comes from `sub.TrackRef` in `Syncer.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 `--` before `ref`: `"checkout", "--quiet", "--", ref`. (Verified by reading `source.go:101` — no `--` separator present.) - **`pack.go:101–121` (`readTree`) — Symlink traversal reads arbitrary files from the filesystem** `readTree` uses `fs.WalkDir` on an `os.DirFS`, which follows symlinks by default. A malicious git repository containing a symlink (e.g., `escape -> /etc/passwd` or `secrets -> ../../.ssh`) would cause `fs.ReadFile` to read the symlink target's contents and store them in the returned `Tree` under the innocent-looking symlink name. The `within` check in `WriteTo` only 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, use `fs.WalkDir` with a check on each `DirEntry` — skip symlinks (check `d.Type() & fs.ModeSymlink`) or resolve and validate the target. (Verified by reading `pack.go:101-121` and confirming no symlink check exists; confirmed `os.DirFS` + `fs.WalkDir` follows symlinks per Go stdlib behavior.) - **`activation.go:54` — Prompt injection via `Description` in `Catalog`** `Catalog` writes `p.Manifest.Description` directly into the system prompt with no sanitization. The only constraint is `maxDescriptionLen` (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 to `p.Manifest.Description`.) --- </details> <details><summary><b>🎯 Correctness</b> — Blocking issues found</summary> Both findings are confirmed against the actual code. The `isKebab` function at `manifest.go:183-192` indeed only checks character set and non-emptiness, missing the positional constraints (no leading/trailing/consecutive hyphens) that the Anthropic agent-skills spec requires. The `GitSource` type assertion at `sync.go:102` only matches the value type, so a host returning `&GitSource{...}` from `SourceFor` would silently lose URL/subpath decomposition. --- # Verdict: Blocking issues found ## Findings - **`manifest.go:183-192` — `isKebab` is 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 `-bad` would be accepted here but fail elsewhere. **Fix**: Add positional checks: ```go func isKebab(s string) bool { if s == "" || s[0] == '-' || s[len(s)-1] == '-' { return false } prev := rune(0) for _, r := range s { if !((r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') || r == '-') { return false } if r == '-' && prev == '-' { return false } prev = r } return true } ``` - **`sync.go:102` — `GitSource` type assertion only matches value type, silently drops URL/subpath for pointer-typed sources.** `DefaultSourceFor` returns `GitSource{...}` (a value), so the assertion `src.(GitSource)` works for the default path. But a host that sets `Syncer.SourceFor` and returns `&GitSource{...}` (a pointer) will silently fail the type assertion — `sub.SourceURL` stays as the combined `String()` output (e.g. `url//subpath`) and `sub.Subpath` stays empty. When `DefaultSourceFor` later reconstructs the source from the stored subscription, it will produce a `GitSource` with a mangled URL and no subpath, breaking future `Check`/`Apply` calls. **Fix**: Use a type switch or also check `*GitSource`: ```go switch gs := src.(type) { case GitSource: sub.SourceURL = gs.URL sub.Subpath = gs.Subpath case *GitSource: sub.SourceURL = gs.URL sub.Subpath = gs.Subpath } ``` </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> # Verdict: Minor issues ## Findings - **`skillpack/sync.go:16-18` — `Syncer` field names are swapped relative to their types** `Store` is typed `PackCache` and `Subs` is typed `Store`. A reader sees `y.Store.Get(...)` and naturally expects a `Store` interface call, but it's actually `PackCache.Get`. Conversely, `y.Subs.Get(...)` is the real `Store.Get`. The comments ("content store for pinned trees" / "subscription metadata store") confirm the intent, but the field names mislead. Rename `Store` → `Cache` (or `PackCache`) and `Subs` → `Store` so the names match the types. *Verified by reading `sync.go:16-18` and cross-referencing all `y.Store.` / `y.Subs.` call sites.* - **`skillpack/sync_test.go:33` — dead increment in `NewID` closure** `NewID: func() string { n++; return "id-1" }` — the `n` variable is incremented on every call but the return value is always the literal `"id-1"`. The increment serves no purpose; `n` is never read. Either the intent was `fmt.Sprintf("id-%d", n)` (and the literal is a bug), or the `n` variable and `n++` are dead code that should be removed. *Verified by reading `sync_test.go:27-35`.* - **`skillpack/activation.go:121` — `Stage` uses string concatenation instead of `filepath.Join`** `dir := baseDir + "/" + p.Manifest.Name` constructs a filesystem path with a hardcoded `/` separator. The rest of the codebase consistently uses `filepath.Join` for real filesystem paths (e.g. `pack.go:53`, `source.go:114`). On Windows this would produce a path with mixed separators. Use `filepath.Join(baseDir, p.Manifest.Name)`. (The similar pattern in `renderPackBody:107` is a display string, not a filesystem path, so that one is fine.) *Verified by reading `activation.go:120-121` and comparing against `pack.go:53`.* </details> <details><summary><b>⚡ Performance</b> — Blocking issues found</summary> All three findings are factually confirmed against the actual code. Let me verify the line numbers are precise: - **Finding 1**: `store.go:24-27` — `PackCache` interface stores `Tree`. `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`. All confirmed. - **Finding 2**: `source.go:97` — `g.run(ctx, "", "clone", "--quiet", g.URL, tmp)` with no `--depth 1` or `--single-branch`. Confirmed. - **Finding 3**: `pack.go:95` — `Digest: t.Digest()` inside `LoadPack`. Caller in `Resolve` (`activation.go:26-30`) has `s.PinnedDigest` in hand but doesn't pass it through. Also applies to `Apply` at `sync.go:182`. Confirmed. All three survive verification. ## Verdict: Blocking issues found - **`PackCache` stores raw `Tree` instead 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`). The `PackCache` interface stores `Tree` (raw bytes), but `Resolve` — which is on the **activation hot path** (every agent run) — calls `LoadPack(tree)` for each subscription. `LoadPack` re-parses the YAML frontmatter via `ParseManifest` and re-computes the SHA-256 content digest via `t.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). The `fetchPack` method in `sync.go:67-80` already has the fully parsed `*Pack` in hand but deliberately throws it away, storing only `pack.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 change `PackCache` to store `*Pack` (or a serializable equivalent), or at minimum store the pre-computed digest + parsed manifest + bundled list alongside the tree so `Resolve` can 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.Fetch` clones full repo history — no `--depth 1`** (`skillpack/source.go:97`). The clone command is `git clone --quiet g.URL tmp` with no `--depth 1` and no `--single-branch`. Every `Subscribe` and every `Check` (which runs on a host ticker via `CheckAll`) 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 all `readTree` ever consumes. This multiplies with the issue above: every periodic `Check` pays 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, tmp` with no depth/single-branch flags.* - **`LoadPack` re-computes digest that is already the cache key** (`skillpack/pack.go:95`). `Resolve` retrieves a tree by digest `d`, then `LoadPack` calls `t.Digest()` which re-hashes every file to produce… the same digest `d`. The digest could be passed through (e.g., `LoadPackWithDigest(t, d)`) or, better, eliminated entirely if the `PackCache` stores 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()`) and `activation.go:26-30` (caller has `s.PinnedDigest` in hand but doesn't pass it through).* </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> All four findings are confirmed against the actual code. Here is the corrected review: **Verdict: Minor issues** - **`sync.go:90` — `GetByName` error treated as "name available" (high):** The `Subscribe` method checks `if existing, err := y.Subs.GetByName(ctx, pack.Manifest.Name); err == nil`. Any non-nil error from `GetByName` — 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. Only `ErrNotFound` should permit creation. A SQLite-backed store returning a transient I/O error would silently create duplicates. Fix: use `errors.Is(err, ErrNotFound)` to gate the "already exists" branch, and propagate any other error. (Confirmed: `sync.go:90` uses `err == nil`; `memory.go:45-55` returns `ErrNotFound` for misses; `skillpack.go:51` defines `ErrNotFound`; no `errors.Is` usage exists in the package.) - **`pack.go:51-64` — `WriteTo` has no rollback on partial failure (medium):** The loop writes files sequentially; if `os.WriteFile` fails 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-64` has no cleanup on error.) - **`sync.go:90` — TOCTOU race on duplicate-name check (medium):** The `GetByName` check and subsequent `Save` are not atomic. Two concurrent `Subscribe` calls for the same pack name can both pass the `GetByName` check (both see "not found") and both call `Save`, creating two subscriptions with the same name. The `Memory` store locks per-operation (`memory.go:26-32`), not across the check-then-save span. Fix: the `Save` path (or the store itself) should enforce uniqueness, e.g., by checking within the same lock critical section. (Confirmed: `sync.go:85-111` and `memory.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: }` or `metadata: { 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`.) </details> </details> <details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — Minor issues</summary> VERDICT: Minor issues Through the security lens, I verified the traversal defenses (WriteTo's `within` check, the subpath `path.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)` and `g.run(ctx, tmp, "checkout", "--quiet", ref)` pass `g.URL` and `ref` directly to `git` with no `--` terminator before them. A URL or ref beginning with `-` (e.g. `--config=...`, `--upload-pack=...`, or an option-like `TrackRef`) 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 at `Subscribe` time, 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 whose `TrackRef`/`SourceURL` is later mutated — gets option injection. Fix: insert `--` before the user-controlled positional, e.g. `"clone", "--quiet", "--", g.URL, tmp` and `"checkout", "--quiet", "--", ref`. (Verified by reading `source.go` lines 77-104; `exec.CommandContext` invokes `git` with these args verbatim, no shell, so the only injection vector is option parsing — which `--` closes.) - **`skillpack/manifest.go:79-83` — `Metadata` values stringified via `fmt.Sprintf("%v", ...)` (low).** Arbitrary YAML scalars/sequences in `metadata` are flattened to strings. Not a security hole per se (no eval, no template), but a malicious pack can stuff unbounded `metadata` content into what a host may render; there's no per-value length cap (only `body` and 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-supplied `args.Name` is only used as a map key, never as a path or command), and `Stage`/`WriteTo` traversal protection verified sound. </details> <details><summary><b>🎯 Correctness</b> — Reviewed</summary> All findings confirmed against the actual code. **Finding 1** (`sync.go:90`): The duplicate-name check only branches on `err == nil` (reject) vs. anything else (proceed). A non-`ErrNotFound` error from a real DB-backed `Store.GetByName` is silently swallowed, so `Subscribe` proceeds to `Save`, potentially creating a duplicate-name subscription. `Memory` only returns `ErrNotFound`, so this only bites non-memory stores — but `Store` is explicitly a DB seam. Confirmed. **Finding 2** (`sync.go:186` + `subscription.go:55`): `pinTo` unconditionally sets `s.Name = p.Manifest.Name`. `Apply` calls `pinTo` without any name-uniqueness re-check (unlike `Subscribe`). An upstream rename colliding with an existing subscription's name produces a duplicate. Confirmed. **Finding 3** (`manifest.go:186`): `isKebab` only 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`): `fetchPack` calls `y.Store.Put` unconditionally; `Check` calls `fetchPack` even on the no-change path, so unchanged trees are re-cached on every check. Trivial waste, not a correctness bug. Confirmed. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> All four findings are confirmed against the actual code. **VERDICT: Minor issues** - **`skillpack/sync.go:17-18` — `Store`/`Subs` field names invert their interface types, a readability trap.** `Syncer.Store` is a `PackCache` (not the `Store`), and `Syncer.Subs` is the `Store`. Every reader has to mentally swap the names: `y.Store.Put(...)` puts a *tree*, and `y.Subs.Save(...)` saves a *subscription*. The doc comment on `Store` compensates ("content store for pinned trees") but the field name actively misleads. Suggested fix: name them by role, e.g. `Cache PackCache` and `Subs Store` (or `MetaStore`), so the type and the name agree. Verified at `sync.go:17-18` and call sites in `fetchPack`/`Subscribe`/`Check`/`Apply`. - **`skillpack/sync_test.go:33` — `NewID` closure has a dead counter.** `n` is incremented but the return is the constant `"id-1"`, so `n` is never read. It reads like it's meant to produce unique IDs and silently doesn't. Either drop `n` and return a constant, or generate distinct ids (`fmt.Sprintf("id-%d", n)`). Verified at `sync_test.go:27-35`. - **`skillpack/activation.go:107,121` — path string concatenation instead of `filepath.Join`, inconsistent with the same package's `Tree.WriteTo`.** `Stage` builds `baseDir + "/" + p.Manifest.Name` by hand while `pack.go:49` uses `filepath.Join(dir, filepath.FromSlash(p))` for the same conceptual job. `renderPackBody` does similar manual joining with `strings.TrimRight(stagedDir, "/")+"/"+...`. These are prompt/URL-ish strings rather than filesystem writes (for `renderPackBody`), though `Stage` does feed `WriteTo`, so it's cosmetic but a small maintainability wart. Verified at `activation.go:107,121` vs `pack.go:49`. - **`skillpack/sync.go:97` — set-then-overwrite for `SourceURL`/`Subpath`.** `SourceURL` is initialized to `src.String()` and then immediately overwritten when `src` is a `GitSource`. The branch is the only place the real coordinates are extracted, so the initial assignment is dead for git sources and the `String()` form is silently discarded. A small extraction helper would remove the type-assertion special-case and the dead initial assignment. Verified at `sync.go:94-105`. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> Both findings confirmed against the actual code. - Finding 1: `fetchPack` (sync.go:76) unconditionally calls `y.Store.Put` before `Check` compares digests at line 130. So every `Check`/`CheckAll` tick re-caches, and `MemoryPackCache.Put` (memory.go:92) calls `cloneTree` which deep-copies the whole tree even when the digest is unchanged. Confirmed. - Finding 2: `GitSource.Fetch` (source.go:97) runs `git clone --quiet` with no `--depth`/shallow option and no `ls-remote` short-circuit; `Check` re-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`) — every `Check` re-caches the pack tree even when nothing changed.** `fetchPack` unconditionally calls `y.Store.Put(ctx, pack.Digest, pack.Tree)` at line 76. `Check` calls `fetchPack` on every tick (line 126), and the digest comparison that would skip work happens *after* the `Put` (line 130). `MemoryPackCache.Put` (`skillpack/memory.go:92`) then `cloneTree`s the entire tree — a fresh map plus a new byte slice per file — overwriting the identical entry. So a no-op `CheckAll` over N subscriptions deep-copies every pack's full byte set on every tick; for a durable `PackCache` impl the same call becomes a redundant persisted write. Fix: in `Check`, compare the fetched digest to `sub.PinnedDigest` before caching and only `Put` when they differ (the unchanged path already just clears pending state at lines 130-132); or have `fetchPack` take a "cache only if needed" flag. - **`skillpack/source.go:97` (`GitSource.Fetch`) — full `git clone` on every `Check`, no shallow option.** The clone is `git clone --quiet <url> <tmp>` — full history, no `--depth`/filter flags, no `ls-remote` short-circuit. `Check`/`CheckAll` re-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 1` would avoid transferring history and is a large network/disk win; the subsequent `rev-parse HEAD` and subpath read still work on a shallow clone. (For explicit SHAs, shallow needs more care, so gating `--depth 1` on `ref == ""` or a branch is reasonable.) Medium; suggest adding shallow-clone for the default-branch path and/or a cheap `ls-remote` SHA compare before cloning when the SHA is already the pinned ref. Nothing else in-lens stood out: `Resolve` is cache-only (no network), `Catalog`/`Activate` are one-shot per agent build, `Tree.Digest` is O(files) and only runs at fetch/load (not per activation), and `Memory.GetByName`'s linear scan is fine for the in-memory default's expected scale. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> VERDICT: Minor issues - **`skillpack/sync.go:90` — swallowed store error in the duplicate-name check.** `Subscribe` tests for an existing subscription with `if existing, err := y.Subs.GetByName(ctx, pack.Manifest.Name); err == nil { ... }`. Only the `err == nil` branch is handled; any *other* error from `GetByName` (a transient DB failure, connection error, anything that isn't `ErrNotFound`) 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 subsequent `Save`. The correct pattern is to branch on `err == nil` (duplicate), `errors.Is(err, ErrNotFound)` (proceed), and otherwise return the error. Confirmed against `sync.go:85-111`; the `Memory` impl (`memory.go:45-55`) only ever returns `ErrNotFound`, so this bites any non-memory `Store` implementation — 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 on `splitFrontmatter` says "Leading blank lines/BOM are tolerated," but the scan loop (lines 126-134) only skips blank lines (`len(bytes.TrimSpace(trimmed)) == 0`) and then checks `string(bytes.TrimSpace(trimmed)) != "---"`. `bytes.TrimSpace` does 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). </details> </details> <details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues · ⚠️ 3/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: agent: step 5: all chain targets failed ollama-cloud/kimi-k2.6:cloud: model returned an empty response </details> <details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: agent: step 3: all chain targets failed ollama-cloud/kimi-k2.6:cloud: model returned an empty response </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **Minor issues** - `skillpack/sync.go:16-18` — `Syncer` has a field named `Store` whose type is `PackCache`, not `Store`. In the same package there is a `Store` interface (for subscriptions), so every reader will assume `y.Store` is 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 to `Cache` to match its type. - `skillpack/sync.go:102-105` — `Subscribe` does `src.(GitSource)` to extract the raw `URL` and `Subpath` fields. Because all `GitSource` methods have value receivers, `*GitSource` also satisfies `Source`; if a caller passes `&GitSource{...}`, the type assertion fails silently. The subscription then stores `src.String()` (the display form, e.g. `url//subpath`) as `SourceURL`, and later `DefaultSourceFor` reconstructs the source with the combined string instead of the bare URL. Handle both `GitSource` and `*GitSource` (or add an interface method for raw coordinates). - `skillpack/activation.go:121` — `Stage` builds a path with `baseDir + "/" + p.Manifest.Name`, while `Tree.WriteTo` in `pack.go:53` uses `filepath.Join`. The manual concatenation is inconsistent with the rest of the package and the broader codebase; prefer `filepath.Join(baseDir, p.Manifest.Name)` for platform-appropriate path construction. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> **Verdict:** Minor issues - **`skillpack/sync.go:154`** — `CheckAll` processes subscriptions serially, blocking on slow sources. It loops over all subscriptions and calls `Check` (which may `git clone`) one at a time. A slow or unreachable source delays the entire sweep. Consider bounding concurrency (e.g., `errgroup` with a semaphore) so checks overlap. - **`skillpack/memory.go:45`** — `GetByName` linear scan causes O(n²) on bulk `Subscribe`. `Memory.GetByName` iterates over all subscriptions to find a match by name, and `Subscribe` calls this to reject duplicates. If a host creates many subscriptions, the overall work is quadratic. Add a `name→subscription` index or document that hosts with many subscriptions should use a DB-backed `Store`. </details> <details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed ollama-cloud/kimi-k2.6:cloud: skipped (backed off until 00:06:08.346) </details> </details> <details><summary><b>qwen3.5:397b-cloud</b> (ollama-cloud) — Blocking issues found</summary> **Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — Blocking issues found</summary> **VERDICT: Blocking issues found** - **`skillpack/pack.go:101-120` — Symlink following in `readTree` allows arbitrary file read (HIGH, verified)** The `readTree` function uses `os.DirFS` + `fs.ReadFile` which **follows symlinks**. The walk callback checks `d.IsDir()` but never checks `d.Type()&fs.ModeSymlink != 0`. An attacker controlling a `DirSource` directory (or a git repo with symlinks) can place a symlink like `SKILL.md -> /etc/passwd` or `scripts/leak -> ../../.ssh/id_rsa`. When `Fetch` reads the tree, sensitive host files are pulled into the `Tree` and cached. The `within()` check in `WriteTo` only validates paths *after* they're already in memory — the damage is done at read time. **Fix:** In `readTree`, check `d.Type()&fs.ModeSymlink != 0` and reject symlinks explicitly, or use `os.Lstat` to detect and skip them. - **`skillpack/source.go:113-121` — Subpath validation occurs after `path.Clean`, but symlink in subpath target not checked (MEDIUM, verified)** The `GitSource.Fetch` validates that the subpath doesn't escape via `within(tmp, root)`, but this only checks the *path string*. If the subpath directory itself contains symlinks (e.g., a `data -> /etc` symlink inside the valid subpath), those are followed by `readTree`. 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 of `filepath.Join` (small, verified)** `dir := baseDir + "/" + p.Manifest.Name` works but is non-idiomatic and could be confusing. While `isKebab()` validation on `Name` prevents traversal here, using `filepath.Join(baseDir, p.Manifest.Name)` is clearer and safer by convention. **Fix:** Use `filepath.Join`. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> ## No material issues found After thorough review through the correctness lens, I verified the following key areas: **Manifest parsing (`skillpack/manifest.go`)**: Verified `splitFrontmatter` correctly handles YAML frontmatter with `---` delimiters, `isKebab` properly validates kebab-case names (a-z, 0-9, -), and all limits (maxNameLen=64, maxDescriptionLen=1024, maxBodyBytes=1MiB) are correctly enforced in `Validate()`. **Digest computation (`skillpack/pack.go:26-35`)**: The SHA-256 digest correctly uses sorted paths, hashes each file's content separately, and uses `path\x00hash\n` format to prevent boundary attacks. Order-independent and content-sensitive as claimed. **Path traversal protection (`skillpack/pack.go:125-131`)**: The `within()` function correctly uses `filepath.Rel` and rejects paths starting with `..`. Verified test `TestTreeWriteTo_RejectsTraversal` confirms `../escape.txt` is rejected. **Git subpath handling (`skillpack/source.go:112-120`)**: Uses `path.Clean("/" + g.Subpath)` to normalize and strip leading `../`, then validates with `within(tmp, root)` before accessing. Correctly prevents escaping the repo. **Sync workflow (`skillpack/sync.go`)**: Verified `Subscribe` → `Check` → `Apply` flow. `Check` correctly records pending updates without moving the pin (supply-chain guard), and `Apply` is the only function that advances the pin. **Activation (`skillpack/activation.go`)**: `Catalog` correctly sorts packs by name, `Activate` builds the skill_use tool with proper name lookup, and `Stage` correctly materializes files under `baseDir/<pack name>/`. **Memory stores (`skillpack/memory.go`)**: `cloneTree` correctly 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. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> ## Verdict **Minor issues found** ### Findings - **skillpack/sync.go:17-18** — Confusing field naming in `Syncer` struct. The field `Store` is of type `PackCache`, while `Subs` is of type `Store`. This inversion (`Store` holding a `PackCache`, `Subs` holding a `Store`) is counterintuitive and will cause maintenance confusion. **Suggested fix:** Rename to `Cache PackCache` and `Store Store` (or `Subs Store` → `Subs Store` is acceptable, but `Store PackCache` should become `Cache PackCache`). **Verified:** Read sync.go lines 11-28. - **skillpack/pack.go:125-131** — The `within()` helper duplicates standard library functionality. Go 1.20+ provides `filepath.IsLocal()` which performs the same path-traversal check. Keeping a custom implementation adds maintenance burden and risks divergence. **Suggested fix:** Replace with `filepath.IsLocal(rel)` after the `filepath.Rel` call (or inline the check using `filepath.IsLocal(dest)` directly if on Go 1.20+). **Verified:** Read pack.go lines 123-131; confirmed the logic matches `IsLocal` semantics. - **skillpack/sync.go:94-105** — Awkward URL handling in `Subscribe`. The code assigns `src.String()` to `sub.SourceURL`, then immediately overwrites it for `GitSource` to extract raw `URL` and `Subpath` fields. 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. `skillUseArgs` uses 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 to `SkillUseArgs` for consistency with package conventions. **Verified:** Read activation.go lines 59-61; confirmed it's the only camelCase type name in the package. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **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)**: `cloneTree` copies 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)**: `LoadPack` calls `t.Digest()` even when loading from cache. This is redundant but not a regression — SHA-256 over typical pack sizes is fast, and `LoadPack` runs at activation/sync time, not in a hot loop. - **Git clone per fetch (source.go:85-120)**: `GitSource.Fetch` clones 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 `Catalog` is 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. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> ## 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 is `nil`. The function checks `len(packs) == 0` but does not defend against `[]*Pack{nil}`. A caller constructing a slice with a nil element (or a bug in `Resolve` that appends nil) would cause a runtime panic at `sorted[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 is `nil`. Same issue: `byName[p.Manifest.Name]` dereferences without checking `p != nil`. **Fix:** Skip nil packs in the loop or return an error. - **`skillpack/activation.go:103-104`** — `renderPackBody(p *Pack, stagedDir string)` will **panic** if `p == nil` or `p.Manifest == nil`. This is called from the `skill_use` tool handler; while `Activate` builds the map from valid packs, a future refactor or direct call could pass nil. **Fix:** Add `if p == nil || p.Manifest == nil { return "Error: invalid pack" }`. - **`skillpack/activation.go:121`** — `Stage(p *Pack, baseDir string)` will **panic** if `p == nil` or `p.Manifest == nil` at `p.Manifest.Name`. **Fix:** Add nil guard at function entry. **Verification:** I read `activation.go` lines 42-126 and confirmed all four functions access `p.Manifest` fields without nil checks. The `LoadPack` function (verified in `pack.go:80-95`) always returns a `Pack` with a non-nil `Manifest`, but these four functions are public and accept `*Pack` directly, so they should defend against malformed input. **Outside my lens:** The `Syncer` struct field naming (`Store PackCache` vs `Subs Store`) is confusing but not an error-handling issue. </details> </details> </details> <sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
steve added 1 commit 2026-07-05 00:41:46 +00:00
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>
steve added 1 commit 2026-07-05 00:56:06 +00:00
feat(skillpack): lazy BundleStager for bundled files in skill_use
executus CI / test (pull_request) Successful in 2m19s
29598df814
Replace Activate's stagedDir string with a BundleStager callback invoked
lazily inside skill_use: when the model loads a pack with bundled files, the
host stages them (mort: into run-scoped file storage) and the returned note is
appended to the body so the model knows how to reach them. A nil stager (or a
stager error) degrades gracefully to just listing the file names.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
steve merged commit d5ea9b6e5e into main 2026-07-05 01:28:57 +00:00
Sign in to join this conversation.
No Reviewers
No Label
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: steve/executus#22