feat(skillpack): SKILL.md-subscription battery #22
@@ -85,6 +85,14 @@ BATTERIES (opt-in siblings, each nil-safe + a default):
|
|||||||
(throttled Save/Complete/Fail) + Memory
|
(throttled Save/Complete/Fail) + Memory
|
||||||
budget/ DBBudget rolling-7d + NoOp (run.Budget); [P4 ✓]
|
budget/ DBBudget rolling-7d + NoOp (run.Budget); [P4 ✓]
|
||||||
BudgetStorage iface + Memory default
|
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 ✓]
|
contrib/store/ SECOND module (+ modernc.org/sqlite): [P4 ✓]
|
||||||
pure-Go SQLite impls of ALL store seams: budget +
|
pure-Go SQLite impls of ALL store seams: budget +
|
||||||
|
|||||||
@@ -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)
|
||||||
|
|
|||||||
|
}
|
||||||
|
}
|
||||||
|
if len(sorted) == 0 {
|
||||||
|
return ""
|
||||||
|
}
|
||||||
|
sort.Slice(sorted, func(i, j int) bool { return sorted[i].Manifest.Name < sorted[j].Manifest.Name })
|
||||||
|
|
||||||
|
gitea-actions
commented
🟠 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
|
||||||
|
gitea-actions
commented
🟠 Activate panics on nil pack element in slice error-handling · flagged by 1 model
🪰 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 {
|
||||||
|
gitea-actions
commented
🟠 renderPackBody panics on nil Pack or nil Manifest error-handling · flagged by 1 model
🪰 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)),
|
||||||
|
gitea-actions
commented
🟠 Stage panics on nil Pack or nil Manifest error-handling, maintainability, security · flagged by 4 models
🪰 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
|
||||||
|
}
|
||||||
@@ -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)
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -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 {
|
||||||
|
gitea-actions
commented
🟡 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
🪰 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
|
||||||
|
gitea-actions
commented
🟡 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
🪰 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)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
gitea-actions
commented
🔴 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
|
||||||
|
}
|
||||||
@@ -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)
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -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) {
|
||||||
|
gitea-actions
commented
🟠 GetByName linear scan causes O(n²) on bulk Subscribe performance · flagged by 1 model
🪰 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
|
||||||
|
}
|
||||||
@@ -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 {
|
||||||
|
gitea-actions
commented
🟠 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
|
||||||
|
gitea-actions
commented
🟠 LoadPack re-computes content digest that is already known to the caller (the PackCache key) performance · flagged by 1 model
🪰 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) {
|
||||||
|
gitea-actions
commented
🔴 readTree follows symlinks allowing arbitrary file read from DirSource/GitSource security · flagged by 2 models
🪰 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 {
|
||||||
|
gitea-actions
commented
🟡 within() duplicates filepath.IsLocal() from Go 1.20+ maintainability · flagged by 1 model
🪰 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))
|
||||||
|
}
|
||||||
@@ -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")
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -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")
|
||||||
@@ -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)
|
||||||
|
gitea-actions
commented
🔴 Git command injection via g.URL in git clone — no -- separator before user-controlled URL performance, security · flagged by 2 models
🪰 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)
|
||||||
|
}
|
||||||
|
gitea-actions
commented
🔴 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 {
|
||||||
|
gitea-actions
commented
🟠 GitSource subpath validation does not account for symlinks within subpath security · flagged by 1 model
🪰 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
|
||||||
|
}
|
||||||
@@ -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)
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -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 {
|
||||||
|
gitea-actions
commented
🔴 PackCache stores raw Tree forcing YAML re-parse + SHA-256 re-hash on every agent activation via Resolve→LoadPack performance · flagged by 1 model
🪰 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)
|
||||||
|
}
|
||||||
@@ -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{}
|
||||||
|
}
|
||||||
@@ -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.
|
||||||
|
gitea-actions
commented
🟠 Syncer field Store has type PackCache, colliding with Store interface name maintainability · flagged by 4 models
🪰 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
|
||||||
|
}
|
||||||
|
gitea-actions
commented
🟠 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
🪰 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
|
||||||
|
}
|
||||||
|
gitea-actions
commented
🔴 GetByName error treated as 'name available' — any non-ErrNotFound error silently allows duplicate subscriptions correctness, error-handling · flagged by 2 models
🪰 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)
|
||||||
|
}
|
||||||
|
gitea-actions
commented
🟡 Subscribe has awkward URL assignment that overwrites src.String() for GitSource maintainability · flagged by 2 models
🪰 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(),
|
||||||
|
gitea-actions
commented
🟠 *GitSource type assertion only matches value type; silently drops URL/subpath for GitSource correctness, maintainability · flagged by 2 models
🪰 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
|
||||||
|
}
|
||||||
|
gitea-actions
commented
🟠 CheckAll processes subscriptions serially, blocking on slow sources performance · flagged by 1 model
🪰 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
|
||||||
|
}
|
||||||
|
gitea-actions
commented
🟠 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
|
||||||
|
}
|
||||||
@@ -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) },
|
||||||
|
gitea-actions
commented
🟡 NewID test closure increments a dead counter n but always returns constant "id-1" maintainability · flagged by 2 models
🪰 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)
|
||||||
|
}
|
||||||
|
}
|
||||||
🟠 Catalog panics on nil pack element in slice
error-handling · flagged by 1 model
skillpack/activation.go:47,54—Catalog(packs []*Pack)will panic if any element in the slice isnil. The function checkslen(packs) == 0but does not defend against[]*Pack{nil}. A caller constructing a slice with a nil element (or a bug inResolvethat appends nil) would cause a runtime panic atsorted[i].Manifest.Name. Fix: Add a nil check in the loop (if p == nil { continue }) or document that nil elements are not permitted.🪰 Gadfly · advisory