From bf0b67f9af2e7d68e1a66a8f77d7a04f9bdc1ede Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Sat, 4 Jul 2026 16:46:37 -0400 Subject: [PATCH 1/3] feat(skillpack): SKILL.md-subscription battery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CLAUDE.md | 8 ++ skillpack/activation.go | 126 +++++++++++++++++++++++ skillpack/activation_test.go | 124 ++++++++++++++++++++++ skillpack/manifest.go | 192 +++++++++++++++++++++++++++++++++++ skillpack/manifest_test.go | 82 +++++++++++++++ skillpack/memory.go | 114 +++++++++++++++++++++ skillpack/pack.go | 131 ++++++++++++++++++++++++ skillpack/pack_test.go | 80 +++++++++++++++ skillpack/skillpack.go | 54 ++++++++++ skillpack/source.go | 133 ++++++++++++++++++++++++ skillpack/source_test.go | 97 ++++++++++++++++++ skillpack/store.go | 27 +++++ skillpack/subscription.go | 64 ++++++++++++ skillpack/sync.go | 191 ++++++++++++++++++++++++++++++++++ skillpack/sync_test.go | 176 ++++++++++++++++++++++++++++++++ 15 files changed, 1599 insertions(+) create mode 100644 skillpack/activation.go create mode 100644 skillpack/activation_test.go create mode 100644 skillpack/manifest.go create mode 100644 skillpack/manifest_test.go create mode 100644 skillpack/memory.go create mode 100644 skillpack/pack.go create mode 100644 skillpack/pack_test.go create mode 100644 skillpack/skillpack.go create mode 100644 skillpack/source.go create mode 100644 skillpack/source_test.go create mode 100644 skillpack/store.go create mode 100644 skillpack/subscription.go create mode 100644 skillpack/sync.go create mode 100644 skillpack/sync_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 3b6af72..a626b78 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -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 + diff --git a/skillpack/activation.go b/skillpack/activation.go new file mode 100644 index 0000000..abe8515 --- /dev/null +++ b/skillpack/activation.go @@ -0,0 +1,126 @@ +package skillpack + +import ( + "context" + "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 { + if len(packs) == 0 { + return "" + } + sorted := append([]*Pack(nil), packs...) + sort.Slice(sorted, func(i, j int) bool { return sorted[i].Manifest.Name < sorted[j].Manifest.Name }) + + var b strings.Builder + 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"` +} + +// 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 +// 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). +// +// stagedDir, if non-empty, is the directory a host has staged the packs' bundled +// files into (see Stage); skill_use appends the concrete path so the model knows +// where to read scripts/references with its file tools. Leave it empty when the +// host has no staging. +func Activate(packs []*Pack, stagedDir string) mdagent.Skill { + if len(packs) == 0 { + return nil + } + byName := make(map[string]*Pack, len(packs)) + for _, p := range packs { + byName[p.Manifest.Name] = p + } + + 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 a list of any bundled files.", + func(_ context.Context, args skillUseArgs) (any, error) { + p, ok := byName[strings.TrimSpace(args.Name)] + if !ok { + return fmt.Sprintf("No skill named %q. Use one of the names from the Available skills list.", args.Name), nil + } + return renderPackBody(p, stagedDir), nil + }) + + tb := llm.NewToolbox("skillpack", tool) + return mdskill.New("skillpacks", + mdskill.WithInstructions(Catalog(packs)), + mdskill.WithToolbox(tb), + ) +} + +// renderPackBody is what skill_use returns: the pack's instructions plus a +// pointer to its bundled files (with the staged path when known). +func renderPackBody(p *Pack, stagedDir string) string { + 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") + if stagedDir != "" { + fmt.Fprintf(&b, " (under %s)", strings.TrimRight(stagedDir, "/")+"/"+p.Manifest.Name) + } + b.WriteString(":\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// 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) { + dir := baseDir + "/" + p.Manifest.Name + if err := p.Tree.WriteTo(dir); err != nil { + return "", err + } + return dir, nil +} diff --git a/skillpack/activation_test.go b/skillpack/activation_test.go new file mode 100644 index 0000000..eb13e08 --- /dev/null +++ b/skillpack/activation_test.go @@ -0,0 +1,124 @@ +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()"}), + } + sk := Activate(packs, "/stage") + 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") + } + + // 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") || !strings.Contains(body, "/stage/pdf") { + t.Errorf("skill_use should list bundled files under the staged dir: %q", 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 { + t.Error("no packs should activate to a nil skill") + } +} + +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) + } +} diff --git a/skillpack/manifest.go b/skillpack/manifest.go new file mode 100644 index 0000000..9216aab --- /dev/null +++ b/skillpack/manifest.go @@ -0,0 +1,192 @@ +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 { + 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 +// error — a SKILL.md without frontmatter has no name/description to catalog. +func splitFrontmatter(raw []byte) (front, body []byte, err error) { + 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) + } + } + return out +} + +func isKebab(s string) bool { + for _, r := range s { + switch { + case r >= 'a' && r <= 'z', r >= '0' && r <= '9', r == '-': + default: + return false + } + } + return s != "" +} diff --git a/skillpack/manifest_test.go b/skillpack/manifest_test.go new file mode 100644 index 0000000..9bbcc71 --- /dev/null +++ b/skillpack/manifest_test.go @@ -0,0 +1,82 @@ +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 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) + } +} diff --git a/skillpack/memory.go b/skillpack/memory.go new file mode 100644 index 0000000..ce9c3dc --- /dev/null +++ b/skillpack/memory.go @@ -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) { + 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 +} diff --git a/skillpack/pack.go b/skillpack/pack.go new file mode 100644 index 0000000..009e073 --- /dev/null +++ b/skillpack/pack.go @@ -0,0 +1,131 @@ +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 { + 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 +} + +// 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) { + 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 + } + b, err := fs.ReadFile(fsys, p) + if err != nil { + return err + } + t[path.Clean(p)] = b + return nil + }) + if err != nil { + 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)) +} diff --git a/skillpack/pack_test.go b/skillpack/pack_test.go new file mode 100644 index 0000000..159d8a9 --- /dev/null +++ b/skillpack/pack_test.go @@ -0,0 +1,80 @@ +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 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") + } +} diff --git a/skillpack/skillpack.go b/skillpack/skillpack.go new file mode 100644 index 0000000..de95694 --- /dev/null +++ b/skillpack/skillpack.go @@ -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") diff --git a/skillpack/source.go b/skillpack/source.go new file mode 100644 index 0000000..45f0878 --- /dev/null +++ b/skillpack/source.go @@ -0,0 +1,133 @@ +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) { + tmp, err := os.MkdirTemp("", "skillpack-git-*") + if err != nil { + return nil, "", err + } + defer os.RemoveAll(tmp) + + if _, err := g.run(ctx, "", "clone", "--quiet", g.URL, tmp); err != nil { + return nil, "", err + } + if ref != "" { + if _, err := g.run(ctx, tmp, "checkout", "--quiet", 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 +} diff --git a/skillpack/source_test.go b/skillpack/source_test.go new file mode 100644 index 0000000..59b7899 --- /dev/null +++ b/skillpack/source_test.go @@ -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) + } +} diff --git a/skillpack/store.go b/skillpack/store.go new file mode 100644 index 0000000..e8e047f --- /dev/null +++ b/skillpack/store.go @@ -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 { + Put(ctx context.Context, digest string, t Tree) error + Get(ctx context.Context, digest string) (Tree, error) +} diff --git a/skillpack/subscription.go b/skillpack/subscription.go new file mode 100644 index 0000000..46ec7f8 --- /dev/null +++ b/skillpack/subscription.go @@ -0,0 +1,64 @@ +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. +func (s *Subscription) pinTo(p *Pack, sourceRef, by string, now time.Time) { + s.Name = p.Manifest.Name + s.Description = p.Manifest.Description + s.PinnedDigest = p.Digest + s.PinnedSourceRef = sourceRef + s.PinnedAt = now + s.PinnedBy = by + s.PendingDigest = "" + s.PendingSourceRef = "" + s.PendingAt = time.Time{} +} diff --git a/skillpack/sync.go b/skillpack/sync.go new file mode 100644 index 0000000..89a9c70 --- /dev/null +++ b/skillpack/sync.go @@ -0,0 +1,191 @@ +package skillpack + +import ( + "context" + "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. +type Syncer struct { + Store 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 + } + if err := y.Store.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 + } + if existing, err := y.Subs.GetByName(ctx, pack.Manifest.Name); err == nil { + return nil, fmt.Errorf("skillpack: already subscribed to %q (id %s)", pack.Manifest.Name, existing.ID) + } + + sub := &Subscription{ + ID: y.newID(), + SourceKind: src.Kind(), + SourceURL: src.String(), + TrackRef: trackRef, + Enabled: true, + } + // Store the raw URL/subpath (String() may combine them for display). + if gs, ok := src.(GitSource); ok { + sub.SourceURL = gs.URL + sub.Subpath = gs.Subpath + } + 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 +} + +// 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 + } + if !sub.HasPending() { + return nil, fmt.Errorf("skillpack: %q has no pending update to apply", sub.Name) + } + tree, err := y.Store.Get(ctx, sub.PendingDigest) + 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 +} diff --git a/skillpack/sync_test.go b/skillpack/sync_test.go new file mode 100644 index 0000000..ce74133 --- /dev/null +++ b/skillpack/sync_test.go @@ -0,0 +1,176 @@ +package skillpack + +import ( + "context" + "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{ + Store: NewMemoryPackCache(), + Subs: NewMemory(), + Now: func() time.Time { return time.Unix(1000, 0) }, + NewID: func() string { n++; return "id-1" }, + 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.Store.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.Store.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) + } +} From 9bb5d143f7b02503d812547e48ede8589f4891a1 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Sat, 4 Jul 2026 20:41:44 -0400 Subject: [PATCH 2/3] =?UTF-8?q?fix(skillpack):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20symlink=20read,=20git=20arg-injection,=20dup-subscr?= =?UTF-8?q?ibe,=20nil=20panics?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- skillpack/activation.go | 26 ++++++++++++++++++++------ skillpack/activation_test.go | 19 +++++++++++++++++++ skillpack/manifest.go | 20 ++++++++++++++++++-- skillpack/manifest_test.go | 14 ++++++++++++++ skillpack/pack.go | 8 ++++++++ skillpack/pack_test.go | 25 +++++++++++++++++++++++++ skillpack/source.go | 20 ++++++++++++++++++-- skillpack/subscription.go | 5 +++-- skillpack/sync.go | 28 ++++++++++++++++++++-------- skillpack/sync_test.go | 9 +++++---- 10 files changed, 150 insertions(+), 24 deletions(-) diff --git a/skillpack/activation.go b/skillpack/activation.go index abe8515..e22699a 100644 --- a/skillpack/activation.go +++ b/skillpack/activation.go @@ -2,6 +2,7 @@ package skillpack import ( "context" + "errors" "fmt" "sort" "strings" @@ -40,10 +41,15 @@ func Resolve(ctx context.Context, cache PackCache, subs []Subscription) ([]*Pack // pack (name + description) plus how to load one. This is the whole prompt cost // of a subscription — the bodies stay out until skill_use is called. func Catalog(packs []*Pack) string { - if len(packs) == 0 { + sorted := make([]*Pack, 0, len(packs)) + for _, p := range packs { + if p != nil && p.Manifest != nil { + sorted = append(sorted, p) + } + } + if len(sorted) == 0 { return "" } - sorted := append([]*Pack(nil), packs...) sort.Slice(sorted, func(i, j int) bool { return sorted[i].Manifest.Name < sorted[j].Manifest.Name }) var b strings.Builder @@ -71,12 +77,14 @@ type skillUseArgs struct { // where to read scripts/references with its file tools. Leave it empty when the // host has no staging. func Activate(packs []*Pack, stagedDir string) mdagent.Skill { - if len(packs) == 0 { - return nil - } byName := make(map[string]*Pack, len(packs)) for _, p := range packs { - byName[p.Manifest.Name] = p + if p != nil && p.Manifest != nil { + byName[p.Manifest.Name] = p + } + } + if len(byName) == 0 { + return nil } tool := llm.DefineTool("skill_use", @@ -99,6 +107,9 @@ func Activate(packs []*Pack, stagedDir string) mdagent.Skill { // renderPackBody is what skill_use returns: the pack's instructions plus a // pointer to its bundled files (with the staged path when known). func renderPackBody(p *Pack, stagedDir string) string { + if p == nil || p.Manifest == nil { + return "Error: invalid skill pack." + } var b strings.Builder fmt.Fprintf(&b, "# Skill: %s\n\n%s\n", p.Manifest.Name, p.Manifest.Body) if len(p.Bundled) > 0 { @@ -118,6 +129,9 @@ func renderPackBody(p *Pack, stagedDir string) string { // mount them (read-only is the host's concern) into a sandbox the agent's file // tools can read. Returns the pack's staged directory. func Stage(p *Pack, baseDir string) (string, error) { + if p == nil || p.Manifest == nil { + return "", errors.New("skillpack: Stage requires a non-nil pack") + } dir := baseDir + "/" + p.Manifest.Name if err := p.Tree.WriteTo(dir); err != nil { return "", err diff --git a/skillpack/activation_test.go b/skillpack/activation_test.go index eb13e08..0849a81 100644 --- a/skillpack/activation_test.go +++ b/skillpack/activation_test.go @@ -86,6 +86,25 @@ func TestActivate_Empty(t *testing.T) { } } +func TestNilPackElementsAreSafe(t *testing.T) { + packs := []*Pack{nil, mustPack(t, "real", "b", nil), {Manifest: nil}} + // Neither Catalog nor Activate may panic on nil / malformed elements. + if got := Catalog(packs); !strings.Contains(got, "real") { + t.Errorf("catalog should include the valid pack and skip nils: %q", got) + } + sk := Activate(packs, "") + if sk == nil { + t.Fatal("a valid pack among nils should still activate") + } + if _, ok := sk.Tools().Get("skill_use"); !ok { + t.Error("skill_use missing") + } + // All-nil activates to nothing rather than panicking. + if Activate([]*Pack{nil, {Manifest: nil}}, "") != nil { + t.Error("only-nil packs should activate to nil") + } +} + func TestResolveFromCache(t *testing.T) { ctx := context.Background() cache := NewMemoryPackCache() diff --git a/skillpack/manifest.go b/skillpack/manifest.go index 9216aab..14b259e 100644 --- a/skillpack/manifest.go +++ b/skillpack/manifest.go @@ -112,6 +112,10 @@ func (m *Manifest) Validate() error { // Leading blank lines/BOM are tolerated. A missing or unterminated block is an // error — a SKILL.md without frontmatter has no name/description to catalog. func splitFrontmatter(raw []byte) (front, body []byte, err error) { + // Strip a leading UTF-8 BOM: editors on some platforms prepend one, and + // bytes.TrimSpace (used below) does not remove it, so a BOM would otherwise + // make the first "---" fence unrecognizable. + raw = bytes.TrimPrefix(raw, []byte{0xEF, 0xBB, 0xBF}) s := bufio.NewScanner(bytes.NewReader(raw)) s.Buffer(make([]byte, 0, 64*1024), maxBodyBytes+64*1024) @@ -180,13 +184,25 @@ func trimAll(in []string) []string { return out } +// isKebab reports whether s is strict lowercase kebab-case: [a-z0-9] segments +// joined by single hyphens, with no leading, trailing, or consecutive hyphens. func isKebab(s string) bool { + if s == "" || s[0] == '-' || s[len(s)-1] == '-' { + return false + } + prevHyphen := false for _, r := range s { switch { - case r >= 'a' && r <= 'z', r >= '0' && r <= '9', r == '-': + case r >= 'a' && r <= 'z', r >= '0' && r <= '9': + prevHyphen = false + case r == '-': + if prevHyphen { + return false + } + prevHyphen = true default: return false } } - return s != "" + return true } diff --git a/skillpack/manifest_test.go b/skillpack/manifest_test.go index 9bbcc71..b4a97da 100644 --- a/skillpack/manifest_test.go +++ b/skillpack/manifest_test.go @@ -61,6 +61,9 @@ func TestParseManifest_Errors(t *testing.T) { "missing desc": "---\nname: x\n---\nb\n", "bad name uppercase": "---\nname: PdfProcessing\ndescription: d\n---\nb\n", "bad name space": "---\nname: pdf processing\ndescription: d\n---\nb\n", + "bad name leading -": "---\nname: -pdf\ndescription: d\n---\nb\n", + "bad name trailing-": "---\nname: pdf-\ndescription: d\n---\nb\n", + "bad name double -": "---\nname: pdf--tools\ndescription: d\n---\nb\n", "bad yaml": "---\nname: [unclosed\n---\nb\n", } for label, in := range cases { @@ -80,3 +83,14 @@ func TestParseManifest_LeadingBlanksAndCRLF(t *testing.T) { t.Errorf("got name=%q body=%q", m.Name, m.Body) } } + +func TestParseManifest_BOM(t *testing.T) { + in := append([]byte{0xEF, 0xBB, 0xBF}, []byte("---\nname: bom-ok\ndescription: d\n---\nbody\n")...) + m, err := ParseManifest(in) + if err != nil { + t.Fatalf("BOM-prefixed SKILL.md should parse: %v", err) + } + if m.Name != "bom-ok" { + t.Errorf("name = %q", m.Name) + } +} diff --git a/skillpack/pack.go b/skillpack/pack.go index 009e073..543eaf4 100644 --- a/skillpack/pack.go +++ b/skillpack/pack.go @@ -107,6 +107,14 @@ func readTree(fsys fs.FS) (Tree, error) { if d.IsDir() { return nil } + // Skip symlinks. A pack must be self-contained; os.DirFS + ReadFile + // follows symlinks, so a malicious pack with `SKILL.md -> /etc/passwd` + // or `scripts/x -> ../../.ssh/id_rsa` would otherwise read host files + // into the tree. WalkDir yields a symlink-to-dir as a non-dir entry + // carrying ModeSymlink, so this one check covers file and dir symlinks. + if d.Type()&fs.ModeSymlink != 0 { + return nil + } b, err := fs.ReadFile(fsys, p) if err != nil { return err diff --git a/skillpack/pack_test.go b/skillpack/pack_test.go index 159d8a9..78b5188 100644 --- a/skillpack/pack_test.go +++ b/skillpack/pack_test.go @@ -68,6 +68,31 @@ func TestTreeWriteTo(t *testing.T) { } } +func TestReadTree_SkipsSymlinks(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, ManifestName), []byte(goodManifest), 0o644); err != nil { + t.Fatal(err) + } + // A malicious pack pointing at a host file must NOT be read into the tree. + secret := filepath.Join(t.TempDir(), "secret") + if err := os.WriteFile(secret, []byte("TOPSECRET"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink(secret, filepath.Join(dir, "leak")); err != nil { + t.Skipf("symlink unsupported: %v", err) + } + tree, err := readTree(os.DirFS(dir)) + if err != nil { + t.Fatal(err) + } + if _, ok := tree["leak"]; ok { + t.Fatal("symlink was followed into the tree — arbitrary host file read") + } + if _, ok := tree[ManifestName]; !ok { + t.Fatal("real file should still be read") + } +} + func TestTreeWriteTo_RejectsTraversal(t *testing.T) { dir := t.TempDir() evil := Tree{"../escape.txt": []byte("nope")} diff --git a/skillpack/source.go b/skillpack/source.go index 45f0878..29bf31d 100644 --- a/skillpack/source.go +++ b/skillpack/source.go @@ -88,17 +88,33 @@ func (g GitSource) run(ctx context.Context, dir string, args ...string) ([]byte, } func (g GitSource) Fetch(ctx context.Context, ref string) (Tree, string, error) { + // Argument-injection guard: a URL or ref beginning with "-" would be parsed + // by git as an option (e.g. --upload-pack=…), not a value. Reject it rather + // than rely solely on the "--" separator, which checkout does not honor for + // a rev. Hosts should also allow-list sources, but this is defense-in-depth + // for a library. + if strings.HasPrefix(g.URL, "-") { + return nil, "", fmt.Errorf("skillpack: git url must not start with '-': %q", g.URL) + } + if strings.HasPrefix(ref, "-") { + return nil, "", fmt.Errorf("skillpack: git ref must not start with '-': %q", ref) + } + tmp, err := os.MkdirTemp("", "skillpack-git-*") if err != nil { return nil, "", err } defer os.RemoveAll(tmp) - if _, err := g.run(ctx, "", "clone", "--quiet", g.URL, tmp); err != nil { + // --filter=blob:none: a blobless partial clone gets the ref graph cheaply + // and fetches only the blobs the checkout needs — much less than the full + // history, while still supporting an arbitrary commit-ish ref. "--" ends + // option parsing before the URL. + if _, err := g.run(ctx, "", "clone", "--quiet", "--filter=blob:none", "--", g.URL, tmp); err != nil { return nil, "", err } if ref != "" { - if _, err := g.run(ctx, tmp, "checkout", "--quiet", ref); err != nil { + if _, err := g.run(ctx, tmp, "checkout", "--quiet", "--detach", ref); err != nil { return nil, "", err } } diff --git a/skillpack/subscription.go b/skillpack/subscription.go index 46ec7f8..1bf1851 100644 --- a/skillpack/subscription.go +++ b/skillpack/subscription.go @@ -50,9 +50,10 @@ func (s *Subscription) HasPending() bool { } // pinTo advances the active pin to a fetched pack and clears any pending state. -// Used by initial pin and by Apply. +// Used by initial pin and by Apply. It does NOT set Name: a subscription's name +// is its stable host handle, fixed at Subscribe time — letting an upstream pack +// rename move it would silently collide with another subscription on Apply. func (s *Subscription) pinTo(p *Pack, sourceRef, by string, now time.Time) { - s.Name = p.Manifest.Name s.Description = p.Manifest.Description s.PinnedDigest = p.Digest s.PinnedSourceRef = sourceRef diff --git a/skillpack/sync.go b/skillpack/sync.go index 89a9c70..88d048d 100644 --- a/skillpack/sync.go +++ b/skillpack/sync.go @@ -2,6 +2,7 @@ package skillpack import ( "context" + "errors" "fmt" "time" @@ -14,7 +15,7 @@ import ( // — the only call that changes the bytes an agent runs is Apply, always with an // actor recorded. type Syncer struct { - Store PackCache // content store for pinned trees + Cache PackCache // content store for pinned trees Subs Store // subscription metadata store // SourceFor builds the Source for a stored subscription. A host overrides @@ -73,7 +74,7 @@ func (y *Syncer) fetchPack(ctx context.Context, src Source, ref string) (*Pack, if err != nil { return nil, "", err } - if err := y.Store.Put(ctx, pack.Digest, pack.Tree); err != nil { + if err := y.Cache.Put(ctx, pack.Digest, pack.Tree); err != nil { return nil, "", err } return pack, sourceRef, nil @@ -87,21 +88,32 @@ func (y *Syncer) Subscribe(ctx context.Context, src Source, trackRef, by string) if err != nil { return nil, err } - if existing, err := y.Subs.GetByName(ctx, pack.Manifest.Name); err == nil { + existing, err := y.Subs.GetByName(ctx, pack.Manifest.Name) + if err == nil { return nil, fmt.Errorf("skillpack: already subscribed to %q (id %s)", pack.Manifest.Name, existing.ID) } + if !errors.Is(err, ErrNotFound) { + // A transient store error must NOT fall through to creating a row — that + // would produce a duplicate subscription the uniqueness check missed. + return nil, fmt.Errorf("skillpack: checking for existing subscription %q: %w", pack.Manifest.Name, err) + } sub := &Subscription{ ID: y.newID(), + Name: pack.Manifest.Name, SourceKind: src.Kind(), SourceURL: src.String(), TrackRef: trackRef, Enabled: true, } - // Store the raw URL/subpath (String() may combine them for display). - if gs, ok := src.(GitSource); ok { - sub.SourceURL = gs.URL - sub.Subpath = gs.Subpath + // Store the raw URL + subpath separately (String() may combine them for + // display). GitSource methods have value receivers, so a caller may pass + // either GitSource or *GitSource — handle both. + switch gs := src.(type) { + case GitSource: + sub.SourceURL, sub.Subpath = gs.URL, gs.Subpath + case *GitSource: + sub.SourceURL, sub.Subpath = gs.URL, gs.Subpath } sub.pinTo(pack, sourceRef, by, y.now()) if err := y.Subs.Save(ctx, sub); err != nil { @@ -175,7 +187,7 @@ func (y *Syncer) Apply(ctx context.Context, id, by string) (*Subscription, error if !sub.HasPending() { return nil, fmt.Errorf("skillpack: %q has no pending update to apply", sub.Name) } - tree, err := y.Store.Get(ctx, sub.PendingDigest) + tree, err := y.Cache.Get(ctx, sub.PendingDigest) if err != nil { return nil, fmt.Errorf("skillpack: pending tree for %q missing from cache: %w", sub.Name, err) } diff --git a/skillpack/sync_test.go b/skillpack/sync_test.go index ce74133..b9abaea 100644 --- a/skillpack/sync_test.go +++ b/skillpack/sync_test.go @@ -2,6 +2,7 @@ package skillpack import ( "context" + "fmt" "testing" "time" ) @@ -27,10 +28,10 @@ func packTree(name, body string) Tree { func newTestSyncer(src *fakeSource) *Syncer { n := 0 return &Syncer{ - Store: NewMemoryPackCache(), + Cache: NewMemoryPackCache(), Subs: NewMemory(), Now: func() time.Time { return time.Unix(1000, 0) }, - NewID: func() string { n++; return "id-1" }, + NewID: func() string { n++; return fmt.Sprintf("id-%d", n) }, SourceFor: func(*Subscription) (Source, error) { return src, nil }, } } @@ -51,7 +52,7 @@ func TestSubscribeAndPin(t *testing.T) { t.Fatal("fresh subscription should have no pending update") } // pinned tree is cached under its digest - if _, err := y.Store.Get(ctx, sub.PinnedDigest); err != nil { + if _, err := y.Cache.Get(ctx, sub.PinnedDigest); err != nil { t.Fatalf("pinned tree not cached: %v", err) } } @@ -93,7 +94,7 @@ func TestCheck_RecordsPendingButDoesNotMovePin(t *testing.T) { t.Errorf("pending ref = %q", updated.PendingSourceRef) } // the pending tree is cached, ready for Apply - if _, err := y.Store.Get(ctx, updated.PendingDigest); err != nil { + if _, err := y.Cache.Get(ctx, updated.PendingDigest); err != nil { t.Fatalf("pending tree not cached: %v", err) } } From 29598df81451e5d22c1022bcf279af28e42e59a8 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Sat, 4 Jul 2026 20:56:05 -0400 Subject: [PATCH 3/3] feat(skillpack): lazy BundleStager for bundled files in skill_use 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 --- skillpack/activation.go | 48 ++++++++++++++++++++++++------------ skillpack/activation_test.go | 23 ++++++++++++----- 2 files changed, 49 insertions(+), 22 deletions(-) diff --git a/skillpack/activation.go b/skillpack/activation.go index e22699a..c1b4886 100644 --- a/skillpack/activation.go +++ b/skillpack/activation.go @@ -66,17 +66,26 @@ 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 // 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). // -// stagedDir, if non-empty, is the directory a host has staged the packs' bundled -// files into (see Stage); skill_use appends the concrete path so the model knows -// where to read scripts/references with its file tools. Leave it empty when the -// host has no staging. -func Activate(packs []*Pack, stagedDir string) mdagent.Skill { +// 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 { @@ -88,13 +97,23 @@ func Activate(packs []*Pack, stagedDir string) mdagent.Skill { } 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 a list of any bundled files.", - func(_ context.Context, args skillUseArgs) (any, error) { + "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 { return fmt.Sprintf("No skill named %q. Use one of the names from the Available skills list.", args.Name), nil } - return renderPackBody(p, stagedDir), 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) @@ -104,20 +123,17 @@ func Activate(packs []*Pack, stagedDir string) mdagent.Skill { ) } -// renderPackBody is what skill_use returns: the pack's instructions plus a -// pointer to its bundled files (with the staged path when known). -func renderPackBody(p *Pack, stagedDir string) string { +// 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") - if stagedDir != "" { - fmt.Fprintf(&b, " (under %s)", strings.TrimRight(stagedDir, "/")+"/"+p.Manifest.Name) - } - b.WriteString(":\n") + b.WriteString("\nBundled files:\n") for _, f := range p.Bundled { fmt.Fprintf(&b, "- %s\n", f) } diff --git a/skillpack/activation_test.go b/skillpack/activation_test.go index 0849a81..7fafedc 100644 --- a/skillpack/activation_test.go +++ b/skillpack/activation_test.go @@ -44,7 +44,12 @@ func TestActivate_SkillUseTool(t *testing.T) { packs := []*Pack{ mustPack(t, "pdf", "Use pdfplumber.", map[string]string{"scripts/x.py": "print()"}), } - sk := Activate(packs, "/stage") + 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") } @@ -56,6 +61,9 @@ func TestActivate_SkillUseTool(t *testing.T) { 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"}`)) @@ -66,8 +74,11 @@ func TestActivate_SkillUseTool(t *testing.T) { if !strings.Contains(body, "Use pdfplumber.") { t.Errorf("skill_use body missing instructions: %q", body) } - if !strings.Contains(body, "scripts/x.py") || !strings.Contains(body, "/stage/pdf") { - t.Errorf("skill_use should list bundled files under the staged dir: %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 @@ -81,7 +92,7 @@ func TestActivate_SkillUseTool(t *testing.T) { } func TestActivate_Empty(t *testing.T) { - if Activate(nil, "") != nil { + if Activate(nil, nil) != nil { t.Error("no packs should activate to a nil skill") } } @@ -92,7 +103,7 @@ func TestNilPackElementsAreSafe(t *testing.T) { if got := Catalog(packs); !strings.Contains(got, "real") { t.Errorf("catalog should include the valid pack and skip nils: %q", got) } - sk := Activate(packs, "") + sk := Activate(packs, nil) if sk == nil { t.Fatal("a valid pack among nils should still activate") } @@ -100,7 +111,7 @@ func TestNilPackElementsAreSafe(t *testing.T) { t.Error("skill_use missing") } // All-nil activates to nothing rather than panicking. - if Activate([]*Pack{nil, {Manifest: nil}}, "") != nil { + if Activate([]*Pack{nil, {Manifest: nil}}, nil) != nil { t.Error("only-nil packs should activate to nil") } }