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
15 changed files with 1752 additions and 0 deletions
+8
View File
@@ -85,6 +85,14 @@ BATTERIES (opt-in siblings, each nil-safe + a default):
(throttled Save/Complete/Fail) + Memory
budget/ DBBudget rolling-7d + NoOp (run.Budget); [P4 ✓]
BudgetStorage iface + Memory default
skillpack/ SKILL.md-subscription battery: Manifest + [P5 ✓]
Source (Dir/Git) + Subscription/Store +
content-addressed PackCache + Syncer
(pending-only; Apply re-pins) + Activate →
majordomo agent.Skill (catalog + skill_use,
progressive disclosure) + Memory defaults.
NOT executus/skill (saved-agent noun) nor
majordomo/skill (eager capability bundle).
contrib/store/ SECOND module (+ modernc.org/sqlite): [P4 ✓]
pure-Go SQLite impls of ALL store seams: budget +
+156
View File
@@ -0,0 +1,156 @@
package skillpack
import (
"context"
"errors"
"fmt"
"sort"
"strings"
mdagent "gitea.stevedudenhoeffer.com/steve/majordomo/agent"
"gitea.stevedudenhoeffer.com/steve/majordomo/llm"
mdskill "gitea.stevedudenhoeffer.com/steve/majordomo/skill"
)
// Resolve loads the pinned Pack for each enabled subscription from the cache. It
// is how a host turns "this agent subscribes to these packs" into activatable
// packs at run time without touching the network. A pinned digest missing from
// the cache is an error (the host should have cached it at pin/apply time).
// Disabled subscriptions are skipped.
func Resolve(ctx context.Context, cache PackCache, subs []Subscription) ([]*Pack, error) {
out := make([]*Pack, 0, len(subs))
for i := range subs {
s := &subs[i]
if !s.Enabled {
continue
}
tree, err := cache.Get(ctx, s.PinnedDigest)
if err != nil {
return nil, fmt.Errorf("skillpack: resolving %q: %w", s.Name, err)
}
pack, err := LoadPack(tree)
if err != nil {
return nil, fmt.Errorf("skillpack: loading %q: %w", s.Name, err)
}
out = append(out, pack)
}
return out, nil
}
// Catalog renders the always-in-prompt block for a set of packs: one line per
// pack (name + description) plus how to load one. This is the whole prompt cost
// of a subscription — the bodies stay out until skill_use is called.
func Catalog(packs []*Pack) string {
sorted := make([]*Pack, 0, len(packs))
for _, p := range packs {
if p != nil && p.Manifest != nil {
sorted = append(sorted, p)
Outdated
Review

🟠 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>
}
}
if len(sorted) == 0 {
return ""
}
sort.Slice(sorted, func(i, j int) bool { return sorted[i].Manifest.Name < sorted[j].Manifest.Name })
Review

🟠 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>
var b strings.Builder
b.WriteString("You have access to skills — packaged instructions for specific tasks. ")
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)
}
return strings.TrimRight(b.String(), "\n")
}
type skillUseArgs struct {
Name string `json:"name" description:"the exact name of the skill to load, from the Available skills list"`
}
// BundleStager makes a pack's bundled files available to the current run and
// returns a short note the model can act on (e.g. where the files are and how to
// reference them). It is called LAZILY, inside the skill_use tool, so a pack's
// files are staged only when the model actually loads that pack — not for every
// subscribed pack on every run. A host implements it over its own file plumbing
// (mort saves the files to run-scoped storage and returns their file_ids). nil =
// no staging: skill_use just lists the bundled file names.
type BundleStager func(ctx context.Context, p *Pack) (string, error)
// Activate turns a set of resolved packs into a majordomo agent.Skill: its
// Instructions are the Catalog, and it contributes a single skill_use tool that
Review

🟠 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>
// returns a named pack's full body (progressive disclosure). Attach the result
// to an agent with agent.WithSkill. Returns nil when there are no packs, which
// agent.WithSkill tolerates (a nil skill contributes nothing).
//
// stager, if non-nil, is invoked when skill_use loads a pack with bundled files;
// its returned note is appended to the body so the model knows how to reach the
// staged scripts/references. A stager error degrades gracefully (the
// instructions still return, with a note that the files are unavailable).
func Activate(packs []*Pack, stager BundleStager) mdagent.Skill {
byName := make(map[string]*Pack, len(packs))
for _, p := range packs {
if p != nil && p.Manifest != nil {
byName[p.Manifest.Name] = p
}
}
if len(byName) == 0 {
return nil
}
tool := llm.DefineTool("skill_use",
"Load the full instructions for a skill by name before doing a task it covers. Returns the skill's instructions and, if it has bundled files, how to access them.",
func(ctx context.Context, args skillUseArgs) (any, error) {
p, ok := byName[strings.TrimSpace(args.Name)]
if !ok {
Review

🟠 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>
return fmt.Sprintf("No skill named %q. Use one of the names from the Available skills list.", args.Name), nil
}
body := renderPackBody(p)
if stager != nil && len(p.Bundled) > 0 {
note, err := stager(ctx, p)
switch {
case err != nil:
body += "\n\n(bundled files could not be staged: " + err.Error() + ")"
case note != "":
body += "\n\n" + note
}
}
return body, nil
})
tb := llm.NewToolbox("skillpack", tool)
return mdskill.New("skillpacks",
mdskill.WithInstructions(Catalog(packs)),
Review

🟠 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>
mdskill.WithToolbox(tb),
)
}
// renderPackBody is the base skill_use payload: the pack's instructions plus, if
// it has any, a list of its bundled file names. A stager (see Activate) appends
// the concrete access note.
func renderPackBody(p *Pack) string {
if p == nil || p.Manifest == nil {
return "Error: invalid skill pack."
}
var b strings.Builder
fmt.Fprintf(&b, "# Skill: %s\n\n%s\n", p.Manifest.Name, p.Manifest.Body)
if len(p.Bundled) > 0 {
b.WriteString("\nBundled files:\n")
for _, f := range p.Bundled {
fmt.Fprintf(&b, "- %s\n", f)
}
}
return strings.TrimRight(b.String(), "\n")
}
// Stage materializes a pack's files under baseDir/<pack name>/ so a host can
// mount them (read-only is the host's concern) into a sandbox the agent's file
// tools can read. Returns the pack's staged directory.
func Stage(p *Pack, baseDir string) (string, error) {
if p == nil || p.Manifest == nil {
return "", errors.New("skillpack: Stage requires a non-nil pack")
}
dir := baseDir + "/" + p.Manifest.Name
if err := p.Tree.WriteTo(dir); err != nil {
return "", err
}
return dir, nil
}
+154
View File
@@ -0,0 +1,154 @@
package skillpack
import (
"context"
"encoding/json"
"strings"
"testing"
)
func mustPack(t *testing.T, name, body string, extra map[string]string) *Pack {
t.Helper()
tr := packTree(name, body)
for k, v := range extra {
tr[k] = []byte(v)
}
p, err := LoadPack(tr)
if err != nil {
t.Fatal(err)
}
return p
}
func TestCatalog(t *testing.T) {
packs := []*Pack{
mustPack(t, "zebra", "z", nil),
mustPack(t, "alpha", "a", nil),
}
cat := Catalog(packs)
if !strings.Contains(cat, "skill_use") {
t.Error("catalog should tell the model how to load a skill")
}
ai := strings.Index(cat, "alpha")
zi := strings.Index(cat, "zebra")
if ai < 0 || zi < 0 || ai > zi {
t.Errorf("catalog should list packs sorted by name:\n%s", cat)
}
if Catalog(nil) != "" {
t.Error("empty catalog should be empty string")
}
}
func TestActivate_SkillUseTool(t *testing.T) {
ctx := context.Background()
packs := []*Pack{
mustPack(t, "pdf", "Use pdfplumber.", map[string]string{"scripts/x.py": "print()"}),
}
staged := 0
stager := func(_ context.Context, p *Pack) (string, error) {
staged++
return "staged " + p.Manifest.Name + " (file_id=abc)", nil
}
sk := Activate(packs, stager)
if sk == nil {
t.Fatal("expected a non-nil skill")
}
if sk.Instructions() != Catalog(packs) {
t.Error("skill instructions should be the catalog")
}
tb := sk.Tools()
tool, ok := tb.Get("skill_use")
if !ok {
t.Fatal("skill_use tool missing from toolbox")
}
if staged != 0 {
t.Error("stager must be lazy — not called until skill_use runs")
}
// load an existing pack
out, err := tool.Handler(ctx, json.RawMessage(`{"name":"pdf"}`))
if err != nil {
t.Fatal(err)
}
body, _ := out.(string)
if !strings.Contains(body, "Use pdfplumber.") {
t.Errorf("skill_use body missing instructions: %q", body)
}
if !strings.Contains(body, "scripts/x.py") {
t.Errorf("skill_use should list bundled files: %q", body)
}
if staged != 1 || !strings.Contains(body, "file_id=abc") {
t.Errorf("stager should run on load and its note append to the body: staged=%d body=%q", staged, body)
}
// unknown pack returns guidance, not an error
out, err = tool.Handler(ctx, json.RawMessage(`{"name":"nope"}`))
if err != nil {
t.Fatal(err)
}
if s, _ := out.(string); !strings.Contains(s, "No skill named") {
t.Errorf("unknown skill should return guidance: %q", s)
}
}
func TestActivate_Empty(t *testing.T) {
if Activate(nil, nil) != nil {
t.Error("no packs should activate to a nil skill")
}
}
func TestNilPackElementsAreSafe(t *testing.T) {
packs := []*Pack{nil, mustPack(t, "real", "b", nil), {Manifest: nil}}
// Neither Catalog nor Activate may panic on nil / malformed elements.
if got := Catalog(packs); !strings.Contains(got, "real") {
t.Errorf("catalog should include the valid pack and skip nils: %q", got)
}
sk := Activate(packs, nil)
if sk == nil {
t.Fatal("a valid pack among nils should still activate")
}
if _, ok := sk.Tools().Get("skill_use"); !ok {
t.Error("skill_use missing")
}
// All-nil activates to nothing rather than panicking.
if Activate([]*Pack{nil, {Manifest: nil}}, nil) != nil {
t.Error("only-nil packs should activate to nil")
}
}
func TestResolveFromCache(t *testing.T) {
ctx := context.Background()
cache := NewMemoryPackCache()
p := mustPack(t, "alpha", "a", nil)
cache.Put(ctx, p.Digest, p.Tree)
subs := []Subscription{
{Name: "alpha", PinnedDigest: p.Digest, Enabled: true},
{Name: "disabled", PinnedDigest: p.Digest, Enabled: false},
}
packs, err := Resolve(ctx, cache, subs)
if err != nil {
t.Fatal(err)
}
if len(packs) != 1 || packs[0].Manifest.Name != "alpha" {
t.Fatalf("resolve should skip disabled subs; got %d packs", len(packs))
}
// missing from cache is an error
subs = []Subscription{{Name: "ghost", PinnedDigest: "deadbeef", Enabled: true}}
if _, err := Resolve(ctx, cache, subs); err == nil {
t.Fatal("expected error resolving an uncached pin")
}
}
func TestStage(t *testing.T) {
dir := t.TempDir()
p := mustPack(t, "pdf", "b", map[string]string{"scripts/x.py": "print()"})
staged, err := Stage(p, dir)
if err != nil {
t.Fatal(err)
}
if !strings.HasSuffix(staged, "/pdf") {
t.Errorf("staged dir = %q", staged)
}
}
+208
View File
@@ -0,0 +1,208 @@
package skillpack
import (
"bufio"
"bytes"
"fmt"
"strings"
"gopkg.in/yaml.v3"
)
// ManifestName is the required filename at a pack's root.
const ManifestName = "SKILL.md"
// Limits on manifest fields, matching the Anthropic agent-skills constraints so
// packs authored against that ecosystem validate here unchanged.
const (
maxNameLen = 64
maxDescriptionLen = 1024
maxBodyBytes = 1 << 20 // 1 MiB of instruction text is already excessive
)
// Manifest is a parsed SKILL.md: YAML frontmatter plus the markdown body. Only
// Name and Description are required; everything else is optional and passes
// through so a host can honor it (or ignore it) without this package growing a
// policy opinion.
type Manifest struct {
// Name is the pack's stable identifier (kebab-case, unique within a host's
// subscriptions). It is what the model passes to skill_use.
Name string
// Description is the one-liner shown in the catalog — the ONLY text loaded
// into the prompt up front, so it must convey when to reach for the skill.
Description string
// License is an optional SPDX-ish tag, informational only.
License string
// AllowedTools is the pack author's declared tool allow-list. It is advisory
// here: a host MAY intersect it with the agent's real toolset, but it can
// only ever NARROW, never grant (see the host wiring, not this package).
AllowedTools []string
// Metadata is arbitrary passthrough frontmatter (e.g. version) the host may
// use; this package does not interpret it.
Metadata map[string]string
// Body is the markdown instruction text after the frontmatter — the payload
// skill_use returns on demand.
Body string
}
// ParseManifest parses a SKILL.md byte slice into a validated Manifest. The
// input must begin with a `---` YAML frontmatter block; the remainder is the
// body. It returns a descriptive error on malformed frontmatter or a field that
// violates the limits, so a bad pack fails loudly at subscribe/sync time rather
// than silently activating.
func ParseManifest(raw []byte) (*Manifest, error) {
front, body, err := splitFrontmatter(raw)
if err != nil {
return nil, err
}
// Decode into a permissive intermediate: SKILL.md uses hyphenated keys
// (allowed-tools) and lets metadata values be scalars of any type.
var fm struct {
Name string `yaml:"name"`
Description string `yaml:"description"`
License string `yaml:"license"`
AllowedTools stringList `yaml:"allowed-tools"`
Metadata map[string]any `yaml:"metadata"`
}
if err := yaml.Unmarshal(front, &fm); err != nil {
return nil, fmt.Errorf("skillpack: invalid SKILL.md frontmatter: %w", err)
}
m := &Manifest{
Name: strings.TrimSpace(fm.Name),
Description: strings.TrimSpace(fm.Description),
License: strings.TrimSpace(fm.License),
AllowedTools: []string(fm.AllowedTools),
Body: strings.TrimSpace(string(body)),
}
if len(fm.Metadata) > 0 {
Review

🟡 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>
m.Metadata = make(map[string]string, len(fm.Metadata))
for k, v := range fm.Metadata {
m.Metadata[k] = fmt.Sprintf("%v", v)
}
}
if err := m.Validate(); err != nil {
return nil, err
}
return m, nil
}
// Validate reports the first field that violates the manifest contract.
func (m *Manifest) Validate() error {
switch {
case m.Name == "":
return fmt.Errorf("skillpack: SKILL.md missing required 'name'")
case len(m.Name) > maxNameLen:
return fmt.Errorf("skillpack: name %q exceeds %d chars", m.Name, maxNameLen)
case !isKebab(m.Name):
return fmt.Errorf("skillpack: name %q must be lowercase kebab-case (a-z, 0-9, -)", m.Name)
case m.Description == "":
return fmt.Errorf("skillpack: SKILL.md missing required 'description'")
case len(m.Description) > maxDescriptionLen:
return fmt.Errorf("skillpack: description exceeds %d chars", maxDescriptionLen)
case len(m.Body) > maxBodyBytes:
return fmt.Errorf("skillpack: body exceeds %d bytes", maxBodyBytes)
}
return nil
}
// splitFrontmatter separates a leading `---`-delimited YAML block from the body.
// Leading blank lines/BOM are tolerated. A missing or unterminated block is an
Review

🟡 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>
// error — a SKILL.md without frontmatter has no name/description to catalog.
func splitFrontmatter(raw []byte) (front, body []byte, err error) {
// Strip a leading UTF-8 BOM: editors on some platforms prepend one, and
// bytes.TrimSpace (used below) does not remove it, so a BOM would otherwise
// make the first "---" fence unrecognizable.
raw = bytes.TrimPrefix(raw, []byte{0xEF, 0xBB, 0xBF})
s := bufio.NewScanner(bytes.NewReader(raw))
s.Buffer(make([]byte, 0, 64*1024), maxBodyBytes+64*1024)
var frontLines [][]byte
var bodyLines [][]byte
state := 0 // 0=before open fence, 1=in frontmatter, 2=in body
sawOpen := false
for s.Scan() {
line := s.Bytes()
trimmed := bytes.TrimRight(line, "\r")
switch state {
case 0:
if len(bytes.TrimSpace(trimmed)) == 0 {
continue // skip leading blanks
}
if string(bytes.TrimSpace(trimmed)) != "---" {
return nil, nil, fmt.Errorf("skillpack: SKILL.md must start with a '---' frontmatter block")
}
sawOpen = true
state = 1
case 1:
if string(bytes.TrimSpace(trimmed)) == "---" {
state = 2
continue
}
frontLines = append(frontLines, append([]byte(nil), trimmed...))
case 2:
bodyLines = append(bodyLines, append([]byte(nil), trimmed...))
}
}
if err := s.Err(); err != nil {
return nil, nil, fmt.Errorf("skillpack: reading SKILL.md: %w", err)
}
if !sawOpen || state != 2 {
return nil, nil, fmt.Errorf("skillpack: SKILL.md frontmatter block is not terminated by a closing '---'")
}
return bytes.Join(frontLines, []byte("\n")), bytes.Join(bodyLines, []byte("\n")), nil
}
// stringList decodes either a YAML sequence or a comma-separated scalar into a
// []string, so `allowed-tools: [Read, Bash]` and `allowed-tools: "Read, Bash"`
// both work.
type stringList []string
func (l *stringList) UnmarshalYAML(node *yaml.Node) error {
var seq []string
if err := node.Decode(&seq); err == nil {
*l = trimAll(seq)
return nil
}
var scalar string
if err := node.Decode(&scalar); err != nil {
return err
}
*l = trimAll(strings.Split(scalar, ","))
return nil
}
func trimAll(in []string) []string {
out := in[:0]
for _, s := range in {
if t := strings.TrimSpace(s); t != "" {
out = append(out, t)
}
}
Review

🔴 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>
return out
}
// isKebab reports whether s is strict lowercase kebab-case: [a-z0-9] segments
// joined by single hyphens, with no leading, trailing, or consecutive hyphens.
func isKebab(s string) bool {
if s == "" || s[0] == '-' || s[len(s)-1] == '-' {
return false
}
prevHyphen := false
for _, r := range s {
switch {
case r >= 'a' && r <= 'z', r >= '0' && r <= '9':
prevHyphen = false
case r == '-':
if prevHyphen {
return false
}
prevHyphen = true
default:
return false
}
}
return true
}
+96
View File
@@ -0,0 +1,96 @@
package skillpack
import (
"strings"
"testing"
)
const goodManifest = `---
name: pdf-processing
description: Extract text and tables from PDF files and fill forms.
license: MIT
allowed-tools: [Read, Bash]
metadata:
version: 1.2.0
---
# PDF Processing
Use pdfplumber for extraction.
`
func TestParseManifest_Good(t *testing.T) {
m, err := ParseManifest([]byte(goodManifest))
if err != nil {
t.Fatalf("ParseManifest: %v", err)
}
if m.Name != "pdf-processing" {
t.Errorf("name = %q", m.Name)
}
if !strings.HasPrefix(m.Description, "Extract text") {
t.Errorf("description = %q", m.Description)
}
if m.License != "MIT" {
t.Errorf("license = %q", m.License)
}
if len(m.AllowedTools) != 2 || m.AllowedTools[0] != "Read" || m.AllowedTools[1] != "Bash" {
t.Errorf("allowed-tools = %v", m.AllowedTools)
}
if m.Metadata["version"] != "1.2.0" {
t.Errorf("metadata version = %q", m.Metadata["version"])
}
if !strings.Contains(m.Body, "pdfplumber") || strings.Contains(m.Body, "---") {
t.Errorf("body not cleanly extracted: %q", m.Body)
}
}
func TestParseManifest_AllowedToolsScalar(t *testing.T) {
m, err := ParseManifest([]byte("---\nname: n\ndescription: d\nallowed-tools: \"Read, Bash , Grep\"\n---\nbody\n"))
if err != nil {
t.Fatal(err)
}
if len(m.AllowedTools) != 3 || m.AllowedTools[2] != "Grep" {
t.Errorf("scalar allowed-tools = %v", m.AllowedTools)
}
}
func TestParseManifest_Errors(t *testing.T) {
cases := map[string]string{
"no frontmatter": "# just a heading\n",
"unterminated": "---\nname: x\ndescription: y\n",
"missing name": "---\ndescription: y\n---\nb\n",
"missing desc": "---\nname: x\n---\nb\n",
"bad name uppercase": "---\nname: PdfProcessing\ndescription: d\n---\nb\n",
"bad name space": "---\nname: pdf processing\ndescription: d\n---\nb\n",
"bad name leading -": "---\nname: -pdf\ndescription: d\n---\nb\n",
"bad name trailing-": "---\nname: pdf-\ndescription: d\n---\nb\n",
"bad name double -": "---\nname: pdf--tools\ndescription: d\n---\nb\n",
"bad yaml": "---\nname: [unclosed\n---\nb\n",
}
for label, in := range cases {
if _, err := ParseManifest([]byte(in)); err == nil {
t.Errorf("%s: expected error, got nil", label)
}
}
}
func TestParseManifest_LeadingBlanksAndCRLF(t *testing.T) {
in := "\r\n\n---\r\nname: ok-name\r\ndescription: fine\r\n---\r\nbody line\r\n"
m, err := ParseManifest([]byte(in))
if err != nil {
t.Fatalf("tolerant parse: %v", err)
}
if m.Name != "ok-name" || m.Body != "body line" {
t.Errorf("got name=%q body=%q", m.Name, m.Body)
}
}
func TestParseManifest_BOM(t *testing.T) {
in := append([]byte{0xEF, 0xBB, 0xBF}, []byte("---\nname: bom-ok\ndescription: d\n---\nbody\n")...)
m, err := ParseManifest(in)
if err != nil {
t.Fatalf("BOM-prefixed SKILL.md should parse: %v", err)
}
if m.Name != "bom-ok" {
t.Errorf("name = %q", m.Name)
}
}
+114
View File
@@ -0,0 +1,114 @@
package skillpack
import (
"context"
"sort"
"sync"
)
// Memory is a zero-dependency in-process Store — a light host or a test gets
// subscription persistence with no DB. Returned values are copies, so callers
// can mutate them without corrupting the store.
type Memory struct {
mu sync.RWMutex
subs map[string]*Subscription // by ID
}
// NewMemory returns an empty in-memory Store.
func NewMemory() *Memory {
return &Memory{subs: map[string]*Subscription{}}
}
var _ Store = (*Memory)(nil)
func (m *Memory) Initialize(context.Context) error { return nil }
func (m *Memory) Save(_ context.Context, s *Subscription) error {
m.mu.Lock()
defer m.mu.Unlock()
cp := *s
m.subs[s.ID] = &cp
return nil
}
func (m *Memory) Get(_ context.Context, id string) (*Subscription, error) {
m.mu.RLock()
defer m.mu.RUnlock()
s, ok := m.subs[id]
if !ok {
return nil, ErrNotFound
}
cp := *s
return &cp, nil
}
func (m *Memory) GetByName(_ context.Context, name string) (*Subscription, error) {
Review

🟠 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>
m.mu.RLock()
defer m.mu.RUnlock()
for _, s := range m.subs {
if s.Name == name {
cp := *s
return &cp, nil
}
}
return nil, ErrNotFound
}
func (m *Memory) List(context.Context) ([]Subscription, error) {
m.mu.RLock()
defer m.mu.RUnlock()
out := make([]Subscription, 0, len(m.subs))
for _, s := range m.subs {
out = append(out, *s)
}
sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name })
return out, nil
}
func (m *Memory) Delete(_ context.Context, id string) error {
m.mu.Lock()
defer m.mu.Unlock()
delete(m.subs, id)
return nil
}
// MemoryPackCache is a zero-dependency in-process PackCache. Trees are copied on
// the way in and out so a cached pin is immutable in practice.
type MemoryPackCache struct {
mu sync.RWMutex
trees map[string]Tree
}
// NewMemoryPackCache returns an empty in-memory PackCache.
func NewMemoryPackCache() *MemoryPackCache {
return &MemoryPackCache{trees: map[string]Tree{}}
}
var _ PackCache = (*MemoryPackCache)(nil)
func (c *MemoryPackCache) Put(_ context.Context, digest string, t Tree) error {
c.mu.Lock()
defer c.mu.Unlock()
c.trees[digest] = cloneTree(t)
return nil
}
func (c *MemoryPackCache) Get(_ context.Context, digest string) (Tree, error) {
c.mu.RLock()
defer c.mu.RUnlock()
t, ok := c.trees[digest]
if !ok {
return nil, ErrNotFound
}
return cloneTree(t), nil
}
func cloneTree(t Tree) Tree {
cp := make(Tree, len(t))
for k, v := range t {
b := make([]byte, len(v))
copy(b, v)
cp[k] = b
}
return cp
}
+139
View File
@@ -0,0 +1,139 @@
package skillpack
import (
"crypto/sha256"
"encoding/hex"
"fmt"
"io/fs"
"os"
"path"
"path/filepath"
"sort"
"strings"
)
// Tree is a pack's file set: relative slash-separated path -> file bytes,
// including the SKILL.md itself. It is self-contained (no live filesystem
// handle) so it can be cached, digested, and staged without worrying about the
// lifetime of a clone or temp dir.
type Tree map[string][]byte
// Digest is the content address of the tree: a SHA-256 over every file's path
// and bytes, order-independent. Two trees with identical contents produce the
// same digest regardless of how they were fetched — this is the pin identity
// and the change-detection signal (a git SHA is provenance, but the digest is
// what says "the bytes an agent runs changed").
func (t Tree) Digest() string {
paths := t.Paths()
h := sha256.New()
for _, p := range paths {
fh := sha256.Sum256(t[p])
// path \x00 filehash \n — the NUL prevents path/content boundary games.
fmt.Fprintf(h, "%s\x00%s\n", p, hex.EncodeToString(fh[:]))
}
return hex.EncodeToString(h.Sum(nil))
}
// Paths returns the tree's file paths, sorted.
func (t Tree) Paths() []string {
out := make([]string, 0, len(t))
for p := range t {
out = append(out, p)
}
sort.Strings(out)
return out
}
// WriteTo materializes the tree under dir (creating it and any parents). It is
// 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 {
Review

🟠 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>
for _, p := range t.Paths() {
dest := filepath.Join(dir, filepath.FromSlash(p))
if !within(dir, dest) {
return fmt.Errorf("skillpack: refusing to stage %q outside %q", p, dir)
}
if err := os.MkdirAll(filepath.Dir(dest), 0o755); err != nil {
return err
}
if err := os.WriteFile(dest, t[p], 0o644); err != nil {
return err
}
}
return nil
}
// Pack is a fetched, parsed pack: its manifest, its file tree, the tree's
// content digest, and the non-manifest ("bundled") file paths a host can stage.
type Pack struct {
Manifest *Manifest
Tree Tree
Digest string
// Bundled is every tree path except the SKILL.md, sorted — the scripts and
// reference files skill_use points the model at.
Bundled []string
}
// LoadPack parses a fetched Tree into a Pack: it requires a root SKILL.md,
// parses+validates it, computes the digest, and lists the bundled files.
func LoadPack(t Tree) (*Pack, error) {
raw, ok := t[ManifestName]
if !ok {
return nil, ErrNoManifest
}
m, err := ParseManifest(raw)
if err != nil {
return nil, err
}
bundled := make([]string, 0, len(t))
for _, p := range t.Paths() {
if p != ManifestName {
bundled = append(bundled, p)
}
}
return &Pack{Manifest: m, Tree: t, Digest: t.Digest(), Bundled: bundled}, nil
Review

🟠 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>
}
// 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) {
Review

🔴 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>
t := Tree{}
err := fs.WalkDir(fsys, ".", func(p string, d fs.DirEntry, err error) error {
if err != nil {
return err
}
if d.IsDir() {
return nil
}
// Skip symlinks. A pack must be self-contained; os.DirFS + ReadFile
// follows symlinks, so a malicious pack with `SKILL.md -> /etc/passwd`
// or `scripts/x -> ../../.ssh/id_rsa` would otherwise read host files
// into the tree. WalkDir yields a symlink-to-dir as a non-dir entry
// carrying ModeSymlink, so this one check covers file and dir symlinks.
if d.Type()&fs.ModeSymlink != 0 {
return nil
}
b, err := fs.ReadFile(fsys, p)
if err != nil {
return err
}
t[path.Clean(p)] = b
return nil
})
if err != nil {
Review

🟡 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>
return nil, err
}
return t, nil
}
// within reports whether dest is inside dir (defense against path traversal in
// a staged tree).
func within(dir, dest string) bool {
rel, err := filepath.Rel(dir, dest)
if err != nil {
return false
}
return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator))
}
+105
View File
@@ -0,0 +1,105 @@
package skillpack
import (
"os"
"path/filepath"
"testing"
)
func sampleTree() Tree {
return Tree{
ManifestName: []byte(goodManifest),
"scripts/fill.py": []byte("print('hi')\n"),
"references/spec.md": []byte("# spec\n"),
}
}
func TestTreeDigest_StableAndContentSensitive(t *testing.T) {
a := sampleTree()
b := sampleTree()
if a.Digest() != b.Digest() {
t.Fatal("identical trees must share a digest")
}
b["scripts/fill.py"] = []byte("print('bye')\n")
if a.Digest() == b.Digest() {
t.Fatal("content change must change the digest")
}
// Adding a file changes the digest.
c := sampleTree()
c["extra.txt"] = []byte("x")
if a.Digest() == c.Digest() {
t.Fatal("added file must change the digest")
}
}
func TestLoadPack(t *testing.T) {
p, err := LoadPack(sampleTree())
if err != nil {
t.Fatal(err)
}
if p.Manifest.Name != "pdf-processing" {
t.Errorf("name = %q", p.Manifest.Name)
}
if len(p.Bundled) != 2 || p.Bundled[0] != "references/spec.md" || p.Bundled[1] != "scripts/fill.py" {
t.Errorf("bundled = %v (want sorted, sans SKILL.md)", p.Bundled)
}
if p.Digest == "" {
t.Error("digest empty")
}
}
func TestLoadPack_NoManifest(t *testing.T) {
if _, err := LoadPack(Tree{"readme.md": []byte("x")}); err != ErrNoManifest {
t.Fatalf("want ErrNoManifest, got %v", err)
}
}
func TestTreeWriteTo(t *testing.T) {
dir := t.TempDir()
if err := sampleTree().WriteTo(dir); err != nil {
t.Fatal(err)
}
got, err := os.ReadFile(filepath.Join(dir, "scripts", "fill.py"))
if err != nil {
t.Fatal(err)
}
if string(got) != "print('hi')\n" {
t.Errorf("staged content = %q", got)
}
}
func TestReadTree_SkipsSymlinks(t *testing.T) {
dir := t.TempDir()
if err := os.WriteFile(filepath.Join(dir, ManifestName), []byte(goodManifest), 0o644); err != nil {
t.Fatal(err)
}
// A malicious pack pointing at a host file must NOT be read into the tree.
secret := filepath.Join(t.TempDir(), "secret")
if err := os.WriteFile(secret, []byte("TOPSECRET"), 0o600); err != nil {
t.Fatal(err)
}
if err := os.Symlink(secret, filepath.Join(dir, "leak")); err != nil {
t.Skipf("symlink unsupported: %v", err)
}
tree, err := readTree(os.DirFS(dir))
if err != nil {
t.Fatal(err)
}
if _, ok := tree["leak"]; ok {
t.Fatal("symlink was followed into the tree — arbitrary host file read")
}
if _, ok := tree[ManifestName]; !ok {
t.Fatal("real file should still be read")
}
}
func TestTreeWriteTo_RejectsTraversal(t *testing.T) {
dir := t.TempDir()
evil := Tree{"../escape.txt": []byte("nope")}
if err := evil.WriteTo(dir); err == nil {
t.Fatal("expected traversal rejection")
}
if _, err := os.Stat(filepath.Join(filepath.Dir(dir), "escape.txt")); err == nil {
t.Fatal("traversal file was written outside dir")
}
}
+54
View File
@@ -0,0 +1,54 @@
// Package skillpack is the SKILL.md-subscription battery: it lets an agent host
// subscribe to skill packages published as directories/git repos in the
// Anthropic "agent skills" format (a SKILL.md manifest plus optional bundled
// scripts and reference files) and activate them for a run with progressive
// disclosure.
//
// It is a THIRD, distinct concept from the two "skill" nouns already in the
// stack — do not conflate them:
//
// - majordomo/skill — a lightweight capability bundle (instructions + tools)
// appended to an agent eagerly at construction.
// - executus/skill — a heavyweight persisted "saved agent" noun.
// - executus/skillpack (this package) — an externally-authored, versioned,
// on-demand-loaded instruction pack fetched from a Source and pinned by
// content digest.
//
// Progressive disclosure is the reason this is not just a majordomo/skill:
// majordomo skills inject their whole instruction text into the system prompt
// up front, which does not scale to a catalog of large third-party packs. Here
// only each pack's name+description sits in the prompt permanently (the
// Catalog); the full body is loaded lazily when the model calls the single
// skill_use tool (see Activate).
//
// Design shape (each piece is nil-safe / host-agnostic, mirroring the other
// executus batteries):
//
// - Manifest / ParseManifest — parse+validate a SKILL.md.
// - Tree / Pack / LoadPack — a fetched pack's files, content digest, and
// parsed manifest.
// - Source (Dir, Git) — where packs come from; Fetch returns the file
// tree and the source's resolved ref.
// - Subscription + Store — the persisted "this host tracks this pack at
// this pinned digest" record; Memory is the zero-dep default.
// - PackCache — content-addressed store of pinned pack trees
// so activation never re-fetches; Memory default.
// - Syncer — checks the tracked ref and records a PENDING
// update; applying it is an explicit, separate re-pin (supply-chain guard —
// upstream can never silently change what an agent runs).
// - Catalog / Activate / Stage — turn a set of resolved packs into a
// majordomo agent.Skill (catalog instructions + skill_use tool) and
// materialize a pack's files for a sandbox.
//
// The host (e.g. mort) supplies policy: which sources are allowed, who may
// subscribe, and where staged files are mounted. This package supplies only the
// mechanism.
package skillpack
import "errors"
// ErrNotFound is returned when a subscription or cached pack lookup misses.
var ErrNotFound = errors.New("skillpack: not found")
// ErrNoManifest is returned when a fetched tree has no SKILL.md at its root.
var ErrNoManifest = errors.New("skillpack: tree has no SKILL.md")
+149
View File
@@ -0,0 +1,149 @@
package skillpack
import (
"context"
"fmt"
"os"
"os/exec"
"path"
"path/filepath"
"strings"
)
// Source is where a pack's files come from. Fetch retrieves the tree at ref and
// returns it together with the source's own resolved ref (a git commit SHA, or
// the content digest for a plain directory) — provenance a host can show and
// pin against. ref semantics are source-specific and may be empty ("the
// default": a dir's current contents, a repo's default branch).
type Source interface {
Fetch(ctx context.Context, ref string) (Tree, string, error)
// Kind is a short stable tag ("dir", "git") for persistence + display.
Kind() string
// String is a human-readable identifier (path or URL[/subpath]).
String() string
}
// DirSource reads a pack from a local directory. ref is ignored (a directory
// has no versions); the resolved ref is the content digest. Useful for
// first-party/builtin packs shipped on disk and for tests.
type DirSource struct {
Path string
}
func (d DirSource) Kind() string { return "dir" }
func (d DirSource) String() string { return d.Path }
func (d DirSource) Fetch(_ context.Context, _ string) (Tree, string, error) {
info, err := os.Stat(d.Path)
if err != nil {
return nil, "", fmt.Errorf("skillpack: dir source %q: %w", d.Path, err)
}
if !info.IsDir() {
return nil, "", fmt.Errorf("skillpack: dir source %q is not a directory", d.Path)
}
t, err := readTree(os.DirFS(d.Path))
if err != nil {
return nil, "", err
}
return t, t.Digest(), nil
}
// GitSource fetches a pack from a git repository, optionally from a Subpath
// within it (for repos that publish several packs). ref is any git commit-ish
// (branch, tag, or SHA); empty means the default branch. The resolved ref is
// the checked-out commit SHA.
//
// Fetch clones into a temp dir, reads the subpath tree into memory, and removes
// the clone before returning — the returned Tree is self-contained, so there is
// no clone lifetime to manage and nothing left on disk. Git runs via the system
// `git`; GitRunner is overridable for tests.
type GitSource struct {
URL string
Subpath string
// GitRunner runs a git command in dir and returns combined output. Nil uses
// the system git.
GitRunner func(ctx context.Context, dir string, args ...string) ([]byte, error)
}
func (g GitSource) Kind() string { return "git" }
func (g GitSource) String() string {
if g.Subpath != "" {
return g.URL + "//" + g.Subpath
}
return g.URL
}
func (g GitSource) run(ctx context.Context, dir string, args ...string) ([]byte, error) {
if g.GitRunner != nil {
return g.GitRunner(ctx, dir, args...)
}
cmd := exec.CommandContext(ctx, "git", args...)
cmd.Dir = dir
out, err := cmd.CombinedOutput()
if err != nil {
return out, fmt.Errorf("skillpack: git %s: %w: %s", strings.Join(args, " "), err, strings.TrimSpace(string(out)))
}
return out, nil
}
func (g GitSource) Fetch(ctx context.Context, ref string) (Tree, string, error) {
// Argument-injection guard: a URL or ref beginning with "-" would be parsed
// by git as an option (e.g. --upload-pack=…), not a value. Reject it rather
// than rely solely on the "--" separator, which checkout does not honor for
// a rev. Hosts should also allow-list sources, but this is defense-in-depth
// for a library.
if strings.HasPrefix(g.URL, "-") {
return nil, "", fmt.Errorf("skillpack: git url must not start with '-': %q", g.URL)
Outdated
Review

🔴 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>
}
if strings.HasPrefix(ref, "-") {
return nil, "", fmt.Errorf("skillpack: git ref must not start with '-': %q", ref)
}
Outdated
Review

🔴 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>
tmp, err := os.MkdirTemp("", "skillpack-git-*")
if err != nil {
return nil, "", err
}
defer os.RemoveAll(tmp)
// --filter=blob:none: a blobless partial clone gets the ref graph cheaply
// and fetches only the blobs the checkout needs — much less than the full
// history, while still supporting an arbitrary commit-ish ref. "--" ends
// option parsing before the URL.
if _, err := g.run(ctx, "", "clone", "--quiet", "--filter=blob:none", "--", g.URL, tmp); err != nil {
Outdated
Review

🟠 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>
return nil, "", err
}
if ref != "" {
if _, err := g.run(ctx, tmp, "checkout", "--quiet", "--detach", ref); err != nil {
return nil, "", err
}
}
shaOut, err := g.run(ctx, tmp, "rev-parse", "HEAD")
if err != nil {
return nil, "", err
}
sha := strings.TrimSpace(string(shaOut))
root := tmp
if g.Subpath != "" {
clean := path.Clean("/" + g.Subpath) // normalize, strip leading ../
root = filepath.Join(tmp, filepath.FromSlash(strings.TrimPrefix(clean, "/")))
if !within(tmp, root) {
return nil, "", fmt.Errorf("skillpack: subpath %q escapes the repo", g.Subpath)
}
if info, err := os.Stat(root); err != nil || !info.IsDir() {
return nil, "", fmt.Errorf("skillpack: subpath %q not found in %s", g.Subpath, g.URL)
}
}
t, err := readTree(os.DirFS(root))
if err != nil {
return nil, "", err
}
// Drop a nested .git if the subpath was the repo root.
for p := range t {
if p == ".git" || strings.HasPrefix(p, ".git/") {
delete(t, p)
}
}
return t, sha, nil
}
+97
View File
@@ -0,0 +1,97 @@
package skillpack
import (
"context"
"os"
"os/exec"
"path/filepath"
"testing"
)
func writePack(t *testing.T, dir string) {
t.Helper()
if err := os.MkdirAll(filepath.Join(dir, "scripts"), 0o755); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(filepath.Join(dir, ManifestName), []byte(goodManifest), 0o644); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(filepath.Join(dir, "scripts", "fill.py"), []byte("print('hi')\n"), 0o644); err != nil {
t.Fatal(err)
}
}
func TestDirSource(t *testing.T) {
dir := t.TempDir()
writePack(t, dir)
tree, ref, err := DirSource{Path: dir}.Fetch(context.Background(), "")
if err != nil {
t.Fatal(err)
}
if ref != tree.Digest() {
t.Errorf("dir resolved ref should be the content digest")
}
p, err := LoadPack(tree)
if err != nil {
t.Fatal(err)
}
if p.Manifest.Name != "pdf-processing" || len(p.Bundled) != 1 {
t.Errorf("loaded pack wrong: name=%q bundled=%v", p.Manifest.Name, p.Bundled)
}
}
func TestDirSource_NotADir(t *testing.T) {
f := filepath.Join(t.TempDir(), "file")
os.WriteFile(f, []byte("x"), 0o644)
if _, _, err := (DirSource{Path: f}).Fetch(context.Background(), ""); err == nil {
t.Fatal("expected error for non-directory source")
}
}
// TestGitSource drives a real local git repo (no network) to exercise clone +
// checkout + subpath + SHA resolution. Skipped when git is unavailable.
func TestGitSource(t *testing.T) {
if _, err := exec.LookPath("git"); err != nil {
t.Skip("git not installed")
}
repo := t.TempDir()
git := func(args ...string) {
t.Helper()
cmd := exec.Command("git", args...)
cmd.Dir = repo
cmd.Env = append(os.Environ(),
"GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@t", "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@t")
if out, err := cmd.CombinedOutput(); err != nil {
t.Fatalf("git %v: %v: %s", args, err, out)
}
}
git("init", "-q", "-b", "main")
// pack lives under packs/pdf/
sub := filepath.Join(repo, "packs", "pdf")
writePack(t, sub)
git("add", "-A")
git("commit", "-q", "-m", "add pack")
src := GitSource{URL: repo, Subpath: "packs/pdf"}
tree, sha, err := src.Fetch(context.Background(), "main")
if err != nil {
t.Fatal(err)
}
if len(sha) != 40 {
t.Errorf("resolved ref should be a full SHA, got %q", sha)
}
if _, ok := tree[ManifestName]; !ok {
t.Errorf("subpath tree missing SKILL.md; got %v", tree.Paths())
}
if _, ok := tree[".git"]; ok {
t.Error(".git leaked into the tree")
}
p, err := LoadPack(tree)
if err != nil {
t.Fatal(err)
}
if p.Manifest.Name != "pdf-processing" {
t.Errorf("name = %q", p.Manifest.Name)
}
}
+27
View File
@@ -0,0 +1,27 @@
package skillpack
import "context"
// Store is the persistence seam for subscriptions (metadata + the current pin).
// It is deliberately small; a host backs it with its DB, Memory is the zero-dep
// default, and contrib/store can add durable SQLite alongside the other
// executus store impls.
type Store interface {
Initialize(ctx context.Context) error
Save(ctx context.Context, s *Subscription) error
Get(ctx context.Context, id string) (*Subscription, error)
GetByName(ctx context.Context, name string) (*Subscription, error)
List(ctx context.Context) ([]Subscription, error)
Delete(ctx context.Context, id string) error
}
// PackCache is the content-addressed store of pinned pack trees, keyed by
// content digest. It exists so activating an agent never re-fetches from the
// Source (no clone per run) and so a pinned digest's exact bytes survive even if
// 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 {
Review

🔴 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>
Put(ctx context.Context, digest string, t Tree) error
Get(ctx context.Context, digest string) (Tree, error)
}
+65
View File
@@ -0,0 +1,65 @@
package skillpack
import "time"
// Subscription is a host's persisted "I track this pack, pinned here" record. It
// is metadata only — the pinned pack's bytes live in a PackCache keyed by
// PinnedDigest. A subscription is only ever advanced to new content by an
// explicit Apply (see Syncer): a sync records a PendingDigest, it never moves
// the pin. That is the supply-chain guard — a compromised or careless upstream
// cannot change what an agent runs without a human re-pin.
type Subscription struct {
// ID is a stable host-assigned identifier.
ID string
// Name is the pack's manifest name (unique per host); what an agent lists in
// its SkillPacks and what skill_use receives.
Name string
// Description is the pinned manifest's description, cached so the catalog
// renders without opening the PackCache.
Description string
// Source coordinates.
SourceKind string // "dir" | "git"
SourceURL string // dir path or git URL
Subpath string // git subpath, if any
// TrackRef is the git commit-ish the host follows (branch/tag/SHA); empty =
// default branch. Sync fetches THIS; the pin only moves on Apply.
TrackRef string
// Pinned* describe the currently-active content.
PinnedDigest string // content digest = PackCache key + change signal
PinnedSourceRef string // source's resolved ref (git SHA) — provenance
PinnedAt time.Time
PinnedBy string
// Pending* describe an update a sync found but has NOT applied. Empty
// PendingDigest = no pending update. A pending digest equal to the pinned
// one is impossible by construction (Syncer clears it).
PendingDigest string
PendingSourceRef string
PendingAt time.Time
// Enabled lets a host keep a subscription but deactivate it without
// deleting the pin/history.
Enabled bool
}
// HasPending reports whether a sync found an unapplied update.
func (s *Subscription) HasPending() bool {
return s.PendingDigest != "" && s.PendingDigest != s.PinnedDigest
}
// pinTo advances the active pin to a fetched pack and clears any pending state.
// Used by initial pin and by Apply. It does NOT set Name: a subscription's name
// is its stable host handle, fixed at Subscribe time — letting an upstream pack
// rename move it would silently collide with another subscription on Apply.
func (s *Subscription) pinTo(p *Pack, sourceRef, by string, now time.Time) {
s.Description = p.Manifest.Description
s.PinnedDigest = p.Digest
s.PinnedSourceRef = sourceRef
s.PinnedAt = now
s.PinnedBy = by
s.PendingDigest = ""
s.PendingSourceRef = ""
s.PendingAt = time.Time{}
}
+203
View File
@@ -0,0 +1,203 @@
package skillpack
import (
"context"
"errors"
"fmt"
"time"
"github.com/google/uuid"
)
// Syncer ties a Store, a PackCache, and Sources together into the subscription
// lifecycle: subscribe (initial pin), check (record a PENDING update, never move
// 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.
Review

🟠 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>
type Syncer struct {
Cache PackCache // content store for pinned trees
Subs Store // subscription metadata store
// SourceFor builds the Source for a stored subscription. A host overrides
// this to enforce its allow-list (reject a disallowed URL/kind before any
// fetch). Nil uses DefaultSourceFor (dir + git, no allow-list).
SourceFor func(*Subscription) (Source, error)
// Now/NewID are injectable for deterministic tests.
Now func() time.Time
NewID func() string
}
func (y *Syncer) now() time.Time {
if y.Now != nil {
return y.Now()
}
return time.Now()
}
func (y *Syncer) newID() string {
if y.NewID != nil {
return y.NewID()
}
return uuid.NewString()
}
func (y *Syncer) sourceFor(s *Subscription) (Source, error) {
if y.SourceFor != nil {
return y.SourceFor(s)
}
return DefaultSourceFor(s)
}
// DefaultSourceFor reconstructs a Source from a subscription's stored
// coordinates, with no allow-list. A host that cares about which sources are
// permitted should set Syncer.SourceFor instead of using this.
func DefaultSourceFor(s *Subscription) (Source, error) {
switch s.SourceKind {
case "dir":
return DirSource{Path: s.SourceURL}, nil
case "git":
return GitSource{URL: s.SourceURL, Subpath: s.Subpath}, nil
default:
return nil, fmt.Errorf("skillpack: unknown source kind %q", s.SourceKind)
}
}
// fetchPack fetches src at ref, caches the resulting tree, and returns the
// parsed pack plus the source's resolved ref.
func (y *Syncer) fetchPack(ctx context.Context, src Source, ref string) (*Pack, string, error) {
tree, sourceRef, err := src.Fetch(ctx, ref)
if err != nil {
return nil, "", err
}
pack, err := LoadPack(tree)
if err != nil {
return nil, "", err
}
Review

🟠 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>
if err := y.Cache.Put(ctx, pack.Digest, pack.Tree); err != nil {
return nil, "", err
}
return pack, sourceRef, nil
}
// Subscribe fetches a pack from src at trackRef, caches it, and persists a new
// Subscription pinned to that exact content, attributed to by. It rejects a
// second subscription to the same pack name.
func (y *Syncer) Subscribe(ctx context.Context, src Source, trackRef, by string) (*Subscription, error) {
pack, sourceRef, err := y.fetchPack(ctx, src, trackRef)
if err != nil {
return nil, err
}
Review

🔴 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>
existing, err := y.Subs.GetByName(ctx, pack.Manifest.Name)
if err == nil {
return nil, fmt.Errorf("skillpack: already subscribed to %q (id %s)", pack.Manifest.Name, existing.ID)
}
Review

🟡 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>
if !errors.Is(err, ErrNotFound) {
// A transient store error must NOT fall through to creating a row — that
// would produce a duplicate subscription the uniqueness check missed.
return nil, fmt.Errorf("skillpack: checking for existing subscription %q: %w", pack.Manifest.Name, err)
}
sub := &Subscription{
ID: y.newID(),
Review

🟠 *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>
Name: pack.Manifest.Name,
SourceKind: src.Kind(),
SourceURL: src.String(),
TrackRef: trackRef,
Enabled: true,
}
// Store the raw URL + subpath separately (String() may combine them for
// display). GitSource methods have value receivers, so a caller may pass
// either GitSource or *GitSource — handle both.
switch gs := src.(type) {
case GitSource:
sub.SourceURL, sub.Subpath = gs.URL, gs.Subpath
case *GitSource:
sub.SourceURL, sub.Subpath = gs.URL, gs.Subpath
}
sub.pinTo(pack, sourceRef, by, y.now())
if err := y.Subs.Save(ctx, sub); err != nil {
return nil, err
}
return sub, nil
}
// Check fetches the subscription's tracked ref and, if the content digest
// differs from the current pin, caches the new tree and records it as PENDING —
// it never moves the pin. If the tracked ref matches the pin, any stale pending
// state is cleared. The updated subscription is saved and returned.
func (y *Syncer) Check(ctx context.Context, id string) (*Subscription, error) {
sub, err := y.Subs.Get(ctx, id)
if err != nil {
return nil, err
}
src, err := y.sourceFor(sub)
if err != nil {
return nil, err
}
pack, sourceRef, err := y.fetchPack(ctx, src, sub.TrackRef)
if err != nil {
return nil, err
}
if pack.Digest == sub.PinnedDigest {
// No change upstream; drop any previously-recorded pending update.
sub.PendingDigest, sub.PendingSourceRef, sub.PendingAt = "", "", time.Time{}
} else {
sub.PendingDigest = pack.Digest
sub.PendingSourceRef = sourceRef
sub.PendingAt = y.now()
}
if err := y.Subs.Save(ctx, sub); err != nil {
return nil, err
}
return sub, nil
}
Review

🟠 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>
// CheckAll runs Check on every subscription and returns the ones that now have a
// pending update. Errors on individual subscriptions are collected, not fatal —
// one unreachable source shouldn't stop the sweep. A host calls this on its own
// ticker (skillpack has no cron opinion; the update is never auto-applied so the
// cadence only affects how fresh the "pending" signal is).
func (y *Syncer) CheckAll(ctx context.Context) (pending []Subscription, errs []error) {
subs, err := y.Subs.List(ctx)
if err != nil {
return nil, []error{err}
}
for i := range subs {
updated, err := y.Check(ctx, subs[i].ID)
if err != nil {
errs = append(errs, fmt.Errorf("skillpack: check %q: %w", subs[i].Name, err))
continue
}
if updated.HasPending() {
pending = append(pending, *updated)
}
}
return pending, errs
}
// Apply promotes a subscription's pending update to the active pin, attributed
// to by. This is the ONLY call that changes what an agent runs. It errors if
// there is no pending update or the pending tree is missing from the cache.
func (y *Syncer) Apply(ctx context.Context, id, by string) (*Subscription, error) {
sub, err := y.Subs.Get(ctx, id)
if err != nil {
return nil, err
}
Review

🟠 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>
if !sub.HasPending() {
return nil, fmt.Errorf("skillpack: %q has no pending update to apply", sub.Name)
}
tree, err := y.Cache.Get(ctx, sub.PendingDigest)
if err != nil {
return nil, fmt.Errorf("skillpack: pending tree for %q missing from cache: %w", sub.Name, err)
}
pack, err := LoadPack(tree)
if err != nil {
return nil, err
}
sub.pinTo(pack, sub.PendingSourceRef, by, y.now())
if err := y.Subs.Save(ctx, sub); err != nil {
return nil, err
}
return sub, nil
}
+177
View File
@@ -0,0 +1,177 @@
package skillpack
import (
"context"
"fmt"
"testing"
"time"
)
// fakeSource returns a caller-controlled tree, so sync behavior is tested with
// no filesystem or git.
type fakeSource struct {
tree Tree
ref string
err error
}
func (f *fakeSource) Fetch(context.Context, string) (Tree, string, error) {
return f.tree, f.ref, f.err
}
func (f *fakeSource) Kind() string { return "fake" }
func (f *fakeSource) String() string { return "fake://pack" }
func packTree(name, body string) Tree {
return Tree{ManifestName: []byte("---\nname: " + name + "\ndescription: does " + name + "\n---\n" + body + "\n")}
}
func newTestSyncer(src *fakeSource) *Syncer {
n := 0
return &Syncer{
Cache: NewMemoryPackCache(),
Subs: NewMemory(),
Now: func() time.Time { return time.Unix(1000, 0) },
Review

🟡 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>
NewID: func() string { n++; return fmt.Sprintf("id-%d", n) },
SourceFor: func(*Subscription) (Source, error) { return src, nil },
}
}
func TestSubscribeAndPin(t *testing.T) {
ctx := context.Background()
src := &fakeSource{tree: packTree("alpha", "v1"), ref: "sha-v1"}
y := newTestSyncer(src)
sub, err := y.Subscribe(ctx, src, "main", "steve")
if err != nil {
t.Fatal(err)
}
if sub.Name != "alpha" || sub.PinnedSourceRef != "sha-v1" || sub.PinnedBy != "steve" {
t.Fatalf("bad pin: %+v", sub)
}
if sub.HasPending() {
t.Fatal("fresh subscription should have no pending update")
}
// pinned tree is cached under its digest
if _, err := y.Cache.Get(ctx, sub.PinnedDigest); err != nil {
t.Fatalf("pinned tree not cached: %v", err)
}
}
func TestSubscribe_DuplicateName(t *testing.T) {
ctx := context.Background()
src := &fakeSource{tree: packTree("alpha", "v1"), ref: "r"}
y := newTestSyncer(src)
if _, err := y.Subscribe(ctx, src, "", "s"); err != nil {
t.Fatal(err)
}
if _, err := y.Subscribe(ctx, src, "", "s"); err == nil {
t.Fatal("expected duplicate-name error")
}
}
func TestCheck_RecordsPendingButDoesNotMovePin(t *testing.T) {
ctx := context.Background()
src := &fakeSource{tree: packTree("alpha", "v1"), ref: "sha-v1"}
y := newTestSyncer(src)
sub, _ := y.Subscribe(ctx, src, "main", "s")
pinnedBefore := sub.PinnedDigest
// upstream changes
src.tree = packTree("alpha", "v2-new-instructions")
src.ref = "sha-v2"
updated, err := y.Check(ctx, sub.ID)
if err != nil {
t.Fatal(err)
}
if !updated.HasPending() {
t.Fatal("expected a pending update after upstream change")
}
if updated.PinnedDigest != pinnedBefore {
t.Fatal("Check must NOT move the pin — that is the supply-chain guard")
}
if updated.PendingSourceRef != "sha-v2" {
t.Errorf("pending ref = %q", updated.PendingSourceRef)
}
// the pending tree is cached, ready for Apply
if _, err := y.Cache.Get(ctx, updated.PendingDigest); err != nil {
t.Fatalf("pending tree not cached: %v", err)
}
}
func TestCheck_ClearsStalePendingWhenUpstreamMatches(t *testing.T) {
ctx := context.Background()
src := &fakeSource{tree: packTree("alpha", "v1"), ref: "r1"}
y := newTestSyncer(src)
sub, _ := y.Subscribe(ctx, src, "main", "s")
src.tree = packTree("alpha", "v2")
src.ref = "r2"
sub, _ = y.Check(ctx, sub.ID) // records pending
if !sub.HasPending() {
t.Fatal("precondition: pending expected")
}
// upstream reverts to the pinned content
src.tree = packTree("alpha", "v1")
src.ref = "r1"
sub, _ = y.Check(ctx, sub.ID)
if sub.HasPending() {
t.Fatal("pending should be cleared once upstream matches the pin again")
}
}
func TestApply_MovesPinAndClearsPending(t *testing.T) {
ctx := context.Background()
src := &fakeSource{tree: packTree("alpha", "v1"), ref: "sha-v1"}
y := newTestSyncer(src)
sub, _ := y.Subscribe(ctx, src, "main", "s")
src.tree = packTree("alpha", "v2")
src.ref = "sha-v2"
sub, _ = y.Check(ctx, sub.ID)
pendingDigest := sub.PendingDigest
applied, err := y.Apply(ctx, sub.ID, "admin")
if err != nil {
t.Fatal(err)
}
if applied.PinnedDigest != pendingDigest {
t.Fatal("Apply must move the pin to the pending digest")
}
if applied.PinnedSourceRef != "sha-v2" || applied.PinnedBy != "admin" {
t.Errorf("bad post-apply pin: %+v", applied)
}
if applied.HasPending() {
t.Fatal("Apply must clear the pending update")
}
}
func TestApply_NoPending(t *testing.T) {
ctx := context.Background()
src := &fakeSource{tree: packTree("alpha", "v1"), ref: "r"}
y := newTestSyncer(src)
sub, _ := y.Subscribe(ctx, src, "", "s")
if _, err := y.Apply(ctx, sub.ID, "admin"); err == nil {
t.Fatal("expected error applying with no pending update")
}
}
func TestCheckAll(t *testing.T) {
ctx := context.Background()
src := &fakeSource{tree: packTree("alpha", "v1"), ref: "r1"}
y := newTestSyncer(src)
sub, _ := y.Subscribe(ctx, src, "main", "s")
if pend, errs := y.CheckAll(ctx); len(pend) != 0 || len(errs) != 0 {
t.Fatalf("no change: pend=%v errs=%v", pend, errs)
}
src.tree = packTree("alpha", "v2")
src.ref = "r2"
pend, errs := y.CheckAll(ctx)
if len(errs) != 0 {
t.Fatalf("errs: %v", errs)
}
if len(pend) != 1 || pend[0].ID != sub.ID {
t.Fatalf("expected 1 pending, got %v", pend)
}
}