feat(skills): SKILL.md deps/exclude_deps frontmatter with validation

Add manifest-origin dependency declarations in SKILL.md frontmatter.
New fields: deps (pip/npm/system) and exclude_deps (override search results).
Validates dependencies against per-category regex allowlists to prevent injection.
Manifest overrides can extend or reduce auto-discovered deps.
Backward compatible: skills without deps:/exclude_deps: behave identically.
Includes regression guard (bundled_smoke_test.go) for 5 bundled skills.
This commit is contained in:
viettranx
2026-04-17 19:44:50 +07:00
parent 526f6c11a3
commit 3d6723e7c0
8 changed files with 1048 additions and 3 deletions
+73
View File
@@ -208,3 +208,76 @@ security posture, and troubleshooting (especially musl/glibc compatibility).
## 8. Skill Search (v3)
Skills are searchable via BM25 keyword + semantic similarity matching (in `internal/skills/search.go`). The skill loader indexes all available skills from workspace/project/global/builtin sources. Skill discovery combines keyword matching with embeddings for improved recall of relevant tools to agent tasks.
---
## 9. Declaring Dependencies in SKILL.md
Auto-scan (`internal/skills/dep_scanner.go`) parses Python imports and npm requires from `scripts/` — adequate for most cases but has two limitations:
1. **Import name ≠ pip package name** for many packages (e.g. `import psycopg2` → must `pip install psycopg2-binary` because the sdist-only `psycopg2` package requires `pg_config` at build time). An import-to-pip alias table in `dep_checker.go` handles common cases (`psycopg2→psycopg2-binary`, `psycopg→psycopg[binary]`, `MySQLdb→mysqlclient`, `Crypto→pycryptodome`, `serial→pyserial`, `skimage→scikit-image`, `Levenshtein→python-Levenshtein`, plus the existing `cv2/PIL/yaml/sklearn/bs4/dateutil/dotenv/pptx/docx/attr/gi` set).
2. **False positives** — local helper modules detected as external deps.
Skill authors can override auto-scan with two optional frontmatter fields:
```yaml
---
name: my-skill
description: does things
deps: # authoritative: when present, supersedes auto-scan for install
- pip:psycopg2-binary
- pip:requests>=2.31
- pip:psycopg[binary]
- npm:typescript
- system:ffmpeg
- github:cli/cli@v2.40.0
exclude_deps: # filter false positives from auto-scan; ignored when deps: is set
- pip:my_local_helper
---
```
**Prefix semantics:**
| Prefix | Effect | Example |
|--------|--------|---------|
| `pip:` | Python pip install | `pip:psycopg2-binary`, `pip:requests>=2.31` |
| `npm:` | Global npm install | `npm:typescript` |
| `github:` | GitHub Releases installer (admin) | `github:cli/cli@v2.40.0` |
| `system:` | apk package via pkg-helper | `system:ffmpeg` |
| (bare) | Treated as system binary | `pandoc` |
**Precedence:**
| `deps:` | `exclude_deps:` | Behavior |
|---------|-----------------|----------|
| absent | absent | Auto-scan as today |
| absent | present | Auto-scan minus `exclude_deps` entries |
| present | — | Explicit deps used (authoritative); auto-scan kept only for advisory log |
**v1 limitations:**
- Version pins in `pip:requests>=2.31` are stripped when checking whether the import is available (checker imports `requests`); the installer currently installs latest. Full pin pass-through is planned for v2.
- `deps:` bypasses the import-to-pip alias map, so authors must declare the exact pip package name (e.g. `pip:psycopg2-binary`, not `pip:psycopg2`).
- Unknown prefixes in `deps:` are treated as system binaries.
- `exclude_deps` matches surface in `slog.Debug` only; no UI diagnostic yet.
**Validation & safety:**
Manifest dep strings are passed to `python3 -c` / `node -e` at check time, so each entry is validated against a per-category allowlist before use:
| Category | Allowed chars | Example reject |
|----------|---------------|----------------|
| `pip:` | `[A-Za-z_][A-Za-z0-9_.-]*` | `pip:foo;__import__('os')...` |
| `npm:` | `^(@scope/)?[a-z0-9][a-z0-9_.-]*` | `npm:a');require(...` |
| `system:` / bare | `[A-Za-z0-9][A-Za-z0-9._+-]*` | `rm -rf /`, `$(evil)` |
Invalid entries are dropped with `slog.Warn("skills: dropping invalid manifest dep", ...)`. Malformed specs like `pip:>=1.0` (no package name) or `pip:[binary]` (extras only) are also dropped.
**YAML grammar subset accepted by the loader:**
- Flat list only: `deps:\n - item1\n - item2`
- Quoted items OK (`"..."` or `'...'`)
- CRLF normalized
- Flow-style `[a, b]` NOT supported (returns empty)
- Dash without space `-item` NOT supported
- Nested maps dropped with warning (avoids silent prefix-loss miscategorization)
+43
View File
@@ -0,0 +1,43 @@
package skills
import (
"os"
"path/filepath"
"testing"
)
// TestBundledSkills_NoRegression verifies every bundled skill scans successfully
// after Phase 02 changes. No skill currently uses deps:/exclude_deps: (verified
// via grep), so FromManifest must be false across the board.
func TestBundledSkills_NoRegression(t *testing.T) {
bundled := "../../skills"
entries, err := os.ReadDir(bundled)
if err != nil {
t.Skip("bundled skills dir not found:", err)
return
}
for _, e := range entries {
if !e.IsDir() || e.Name() == "_shared" {
continue
}
name := e.Name()
t.Run(name, func(t *testing.T) {
skillDir := filepath.Join(bundled, name)
m := ScanSkillDeps(skillDir)
if m == nil {
t.Fatal("ScanSkillDeps returned nil")
}
if m.FromManifest {
t.Errorf("%s: FromManifest=true (unexpected — bundled skills don't use deps: yet)", name)
}
if len(m.Explicit) != 0 {
t.Errorf("%s: Explicit non-empty: %v", name, m.Explicit)
}
if len(m.ExcludeDeps) != 0 {
t.Errorf("%s: ExcludeDeps non-empty: %v", name, m.ExcludeDeps)
}
t.Logf("%s: py=%d node=%d sys=%d python_deps=%v",
name, len(m.RequiresPython), len(m.RequiresNode), len(m.Requires), m.RequiresPython)
})
}
}
+223
View File
@@ -0,0 +1,223 @@
package skills
import (
"log/slog"
"os"
"path/filepath"
"regexp"
"strings"
)
// Identifier allowlists for manifest-declared deps. Manifest strings flow
// into python3/node subprocesses via fmt.Sprintf — an unvalidated name would
// let a SKILL.md author inject arbitrary code (e.g. "foo;__import__('os').system(...)").
// Auto-scan already sanitizes via regex capture (\w+); these guards only apply
// to manifest-origin data.
//
// Note: python import allows hyphen/dot even though "import psycopg2-binary"
// yields a SyntaxError at python parse time — that's a SAFE failure (no exec),
// and the installer still treats the package as missing → installs it, matching
// the author's intent when declaring pip install names like "psycopg2-binary".
var (
pythonIdentRe = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_.\-]*$`)
npmPkgNameRe = regexp.MustCompile(`^(@[a-z0-9][a-z0-9_.\-]*/)?[a-z0-9][a-z0-9_.\-]*$`)
sysBinRe = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._+\-]*$`)
)
// isValidDepName returns true when name passes the per-category allowlist.
// Called before manifest strings reach exec subprocesses. Empty names fail.
func isValidDepName(category, name string) bool {
if name == "" {
return false
}
switch category {
case "pip":
return pythonIdentRe.MatchString(name)
case "npm":
return npmPkgNameRe.MatchString(name)
case "system":
return sysBinRe.MatchString(name)
case "github":
return true // github spec validated by ParseGitHubSpec at install time
}
return false
}
// ParsedDep describes a manifest-declared dependency after prefix categorization.
type ParsedDep struct {
Raw string // original manifest string (e.g. "pip:requests>=2.31")
Category string // "pip" | "npm" | "system" | "github"
ImportName string // bare name used for import-check (e.g. "requests")
InstallSpec string // pass-through string for installer (e.g. "requests>=2.31")
}
// categorizeManifestDep parses a raw manifest dep string into its category
// (pip/npm/system/github) and normalized import/install names.
//
// Accepted forms:
//
// pip:<spec> e.g. pip:psycopg2-binary, pip:requests>=2.31, pip:psycopg[binary]
// npm:<pkg> e.g. npm:typescript
// github:<spec> e.g. github:cli/cli@v2.40.0
// system:<bin> e.g. system:ffmpeg
// <bare-name> treated as system binary (matches installer default branch)
func categorizeManifestDep(raw string) ParsedDep {
p := ParsedDep{Raw: raw}
switch {
case strings.HasPrefix(raw, "pip:"):
p.Category = "pip"
spec := strings.TrimPrefix(raw, "pip:")
p.ImportName, p.InstallSpec = splitPipSpec(spec)
case strings.HasPrefix(raw, "npm:"):
p.Category = "npm"
p.ImportName = strings.TrimPrefix(raw, "npm:")
p.InstallSpec = p.ImportName
case strings.HasPrefix(raw, "github:"):
p.Category = "github"
p.InstallSpec = raw
case strings.HasPrefix(raw, "system:"):
p.Category = "system"
p.ImportName = strings.TrimPrefix(raw, "system:")
p.InstallSpec = p.ImportName
default:
p.Category = "system"
p.ImportName = raw
p.InstallSpec = raw
}
return p
}
// splitPipSpec separates the import-check name from the install spec.
// Strips version operators (>=, <=, ==, !=, ~=, >, <) and pip extras ([binary]).
//
// Returns an empty importName when the spec is malformed (e.g. ">=1.0" with no
// package name, or a leading operator at index 0). Callers MUST check and skip
// empty-name entries to avoid feeding syntax errors into python subprocesses.
//
// "requests>=2.31" → ("requests", "requests>=2.31")
// "psycopg[binary]" → ("psycopg", "psycopg[binary]")
// "psycopg2-binary" → ("psycopg2-binary", "psycopg2-binary")
// ">=1.0" → ("", ">=1.0") — malformed, skip
// "[binary]" → ("", "[binary]") — malformed, skip
func splitPipSpec(spec string) (importName, installSpec string) {
installSpec = spec
importName = spec
for _, op := range []string{">=", "<=", "==", "!=", "~=", ">", "<"} {
i := strings.Index(importName, op)
if i == 0 {
return "", installSpec
}
if i > 0 {
importName = strings.TrimSpace(importName[:i])
break
}
}
if i := strings.IndexByte(importName, '['); i == 0 {
return "", installSpec
} else if i > 0 {
importName = importName[:i]
}
return importName, installSpec
}
// skillMdPath returns the absolute path to SKILL.md in a skill directory.
func skillMdPath(skillDir string) string {
return filepath.Join(skillDir, "SKILL.md")
}
// parseSkillManifestFile reads SKILL.md and extracts deps: / exclude_deps:
// lists from its YAML frontmatter. Returns zero slices if file absent,
// frontmatter missing, or fields absent.
func parseSkillManifestFile(skillMdPath string) (deps []string, excludeDeps []string) {
data, err := os.ReadFile(skillMdPath)
if err != nil {
return nil, nil
}
fm := extractFrontmatter(string(data))
if fm == "" {
return nil, nil
}
lists := parseSimpleYAMLLists(fm)
return lists["deps"], lists["exclude_deps"]
}
// applyManifestOverride merges a scan result with manifest-declared deps.
//
// When explicit deps are present, they become authoritative: scan-derived
// slices (Requires, RequiresPython, RequiresNode) are replaced with
// manifest-categorized entries and FromManifest flips to true.
//
// When only excludeDeps are present, the scan result is filtered in place.
//
// When both are empty, the scan result is returned unchanged.
func applyManifestOverride(scan *SkillManifest, explicit, excludeDeps []string) *SkillManifest {
if scan == nil {
scan = &SkillManifest{}
}
scan.ExcludeDeps = excludeDeps
if len(explicit) == 0 {
if len(excludeDeps) > 0 {
scan.RequiresPython = filterOutByImportName(scan.RequiresPython, excludeDeps, "pip")
scan.RequiresNode = filterOutByImportName(scan.RequiresNode, excludeDeps, "npm")
scan.Requires = filterOutByImportName(scan.Requires, excludeDeps, "system")
}
return scan
}
scan.FromManifest = true
scan.Explicit = explicit
var sysReq, pyReq, nodeReq []string
for _, raw := range explicit {
p := categorizeManifestDep(raw)
if p.Category != "github" && !isValidDepName(p.Category, p.ImportName) {
slog.Warn("skills: dropping invalid manifest dep",
"raw", raw, "category", p.Category, "import_name", p.ImportName)
continue
}
switch p.Category {
case "pip":
pyReq = append(pyReq, p.ImportName)
case "npm":
nodeReq = append(nodeReq, p.ImportName)
case "system":
sysReq = append(sysReq, p.ImportName)
}
}
scan.Requires = sysReq
scan.RequiresPython = pyReq
scan.RequiresNode = nodeReq
return scan
}
// filterOutByImportName removes entries whose prefixed form appears in
// excludeDeps. For category "pip"/"npm" the prefix is "<category>:".
// For "system" both "system:<name>" and bare "<name>" are accepted.
func filterOutByImportName(names, excludeDeps []string, category string) []string {
if len(names) == 0 || len(excludeDeps) == 0 {
return names
}
blocked := make(map[string]bool)
prefix := category + ":"
for _, e := range excludeDeps {
switch {
case strings.HasPrefix(e, prefix):
name, _ := splitPipSpec(strings.TrimPrefix(e, prefix))
if name != "" {
blocked[name] = true
}
case category == "system" && !strings.Contains(e, ":"):
blocked[e] = true
}
}
if len(blocked) == 0 {
return names
}
out := make([]string, 0, len(names))
for _, n := range names {
if !blocked[n] {
out = append(out, n)
}
}
return out
}
+376
View File
@@ -0,0 +1,376 @@
package skills
import (
"os"
"path/filepath"
"reflect"
"slices"
"testing"
)
func TestSplitPipSpec(t *testing.T) {
cases := []struct {
spec string
wantImport string
wantInstall string
}{
{"requests", "requests", "requests"},
{"requests>=2.31", "requests", "requests>=2.31"},
{"requests==2.31.0", "requests", "requests==2.31.0"},
{"numpy<2.0", "numpy", "numpy<2.0"},
{"pkg~=1.2", "pkg", "pkg~=1.2"},
{"pkg!=1.0", "pkg", "pkg!=1.0"},
{"pkg<=3", "pkg", "pkg<=3"},
{"psycopg[binary]", "psycopg", "psycopg[binary]"},
{"psycopg[binary]>=3.1", "psycopg", "psycopg[binary]>=3.1"},
{"psycopg2-binary", "psycopg2-binary", "psycopg2-binary"},
}
for _, tc := range cases {
t.Run(tc.spec, func(t *testing.T) {
imp, ins := splitPipSpec(tc.spec)
if imp != tc.wantImport || ins != tc.wantInstall {
t.Errorf("splitPipSpec(%q) = (%q,%q), want (%q,%q)",
tc.spec, imp, ins, tc.wantImport, tc.wantInstall)
}
})
}
}
func TestCategorizeManifestDep(t *testing.T) {
cases := []struct {
raw string
wantCategory string
wantImportName string
wantInstall string
}{
{"pip:psycopg2-binary", "pip", "psycopg2-binary", "psycopg2-binary"},
{"pip:requests>=2.31", "pip", "requests", "requests>=2.31"},
{"pip:psycopg[binary]", "pip", "psycopg", "psycopg[binary]"},
{"npm:typescript", "npm", "typescript", "typescript"},
{"github:cli/cli@v2.40.0", "github", "", "github:cli/cli@v2.40.0"},
{"system:ffmpeg", "system", "ffmpeg", "ffmpeg"},
{"ffmpeg", "system", "ffmpeg", "ffmpeg"},
{"pandoc", "system", "pandoc", "pandoc"},
}
for _, tc := range cases {
t.Run(tc.raw, func(t *testing.T) {
p := categorizeManifestDep(tc.raw)
if p.Category != tc.wantCategory {
t.Errorf("category = %q, want %q", p.Category, tc.wantCategory)
}
if p.ImportName != tc.wantImportName {
t.Errorf("importName = %q, want %q", p.ImportName, tc.wantImportName)
}
if p.InstallSpec != tc.wantInstall {
t.Errorf("installSpec = %q, want %q", p.InstallSpec, tc.wantInstall)
}
if p.Raw != tc.raw {
t.Errorf("raw = %q, want %q", p.Raw, tc.raw)
}
})
}
}
func TestParseSkillManifestFile(t *testing.T) {
dir := t.TempDir()
cases := []struct {
name string
content string
wantDeps []string
wantExcludeDeps []string
}{
{
name: "deps_only",
content: `---
name: test
description: test skill
deps:
- pip:psycopg2-binary
- system:ffmpeg
---
body`,
wantDeps: []string{"pip:psycopg2-binary", "system:ffmpeg"},
wantExcludeDeps: nil,
},
{
name: "exclude_deps_only",
content: `---
name: test
exclude_deps:
- pip:my_local
---
`,
wantDeps: nil,
wantExcludeDeps: []string{"pip:my_local"},
},
{
name: "both",
content: `---
name: test
deps:
- pip:requests
exclude_deps:
- pip:foo
---`,
wantDeps: []string{"pip:requests"},
wantExcludeDeps: []string{"pip:foo"},
},
{
name: "neither",
content: `---
name: test
description: plain
---`,
wantDeps: nil,
wantExcludeDeps: nil,
},
{
name: "no_frontmatter",
content: "plain content",
wantDeps: nil,
wantExcludeDeps: nil,
},
{
name: "quoted_items",
content: `---
deps:
- "pip:psycopg2-binary"
- 'npm:typescript'
---`,
wantDeps: []string{"pip:psycopg2-binary", "npm:typescript"},
wantExcludeDeps: nil,
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
path := filepath.Join(dir, tc.name+"-SKILL.md")
if err := os.WriteFile(path, []byte(tc.content), 0o644); err != nil {
t.Fatal(err)
}
deps, excl := parseSkillManifestFile(path)
if !slices.Equal(deps, tc.wantDeps) {
t.Errorf("deps = %v, want %v", deps, tc.wantDeps)
}
if !slices.Equal(excl, tc.wantExcludeDeps) {
t.Errorf("exclude_deps = %v, want %v", excl, tc.wantExcludeDeps)
}
})
}
t.Run("missing_file", func(t *testing.T) {
deps, excl := parseSkillManifestFile(filepath.Join(dir, "nope.md"))
if deps != nil || excl != nil {
t.Errorf("missing file → deps=%v excl=%v, want nil nil", deps, excl)
}
})
}
func TestApplyManifestOverride_NoOp(t *testing.T) {
scan := &SkillManifest{
Requires: []string{"ffmpeg"},
RequiresPython: []string{"requests", "psycopg2"},
}
got := applyManifestOverride(scan, nil, nil)
if got.FromManifest {
t.Error("FromManifest should be false when no explicit deps")
}
if !slices.Equal(got.RequiresPython, []string{"requests", "psycopg2"}) {
t.Errorf("RequiresPython altered: %v", got.RequiresPython)
}
if !slices.Equal(got.Requires, []string{"ffmpeg"}) {
t.Errorf("Requires altered: %v", got.Requires)
}
}
func TestApplyManifestOverride_Explicit(t *testing.T) {
scan := &SkillManifest{
Requires: []string{"python3"},
RequiresPython: []string{"auto_detected"},
RequiresNode: []string{"leftover"},
}
explicit := []string{
"pip:psycopg2-binary",
"pip:requests>=2.31",
"npm:typescript",
"system:ffmpeg",
"github:cli/cli@v2.40.0",
}
got := applyManifestOverride(scan, explicit, nil)
if !got.FromManifest {
t.Error("FromManifest should be true")
}
wantPy := []string{"psycopg2-binary", "requests"}
if !reflect.DeepEqual(got.RequiresPython, wantPy) {
t.Errorf("RequiresPython = %v, want %v", got.RequiresPython, wantPy)
}
wantNode := []string{"typescript"}
if !reflect.DeepEqual(got.RequiresNode, wantNode) {
t.Errorf("RequiresNode = %v, want %v", got.RequiresNode, wantNode)
}
wantSys := []string{"ffmpeg"}
if !reflect.DeepEqual(got.Requires, wantSys) {
t.Errorf("Requires = %v, want %v", got.Requires, wantSys)
}
if !reflect.DeepEqual(got.Explicit, explicit) {
t.Errorf("Explicit not preserved: %v", got.Explicit)
}
}
func TestApplyManifestOverride_ExcludeOnly(t *testing.T) {
scan := &SkillManifest{
Requires: []string{"ffmpeg", "pandoc"},
RequiresPython: []string{"requests", "my_local_module", "psycopg2"},
RequiresNode: []string{"typescript", "my_local_js"},
}
exclude := []string{
"pip:my_local_module",
"npm:my_local_js",
"pandoc",
}
got := applyManifestOverride(scan, nil, exclude)
if got.FromManifest {
t.Error("FromManifest should be false for exclude-only")
}
wantPy := []string{"requests", "psycopg2"}
if !slices.Equal(got.RequiresPython, wantPy) {
t.Errorf("RequiresPython = %v, want %v", got.RequiresPython, wantPy)
}
wantNode := []string{"typescript"}
if !slices.Equal(got.RequiresNode, wantNode) {
t.Errorf("RequiresNode = %v, want %v", got.RequiresNode, wantNode)
}
wantSys := []string{"ffmpeg"}
if !slices.Equal(got.Requires, wantSys) {
t.Errorf("Requires = %v, want %v", got.Requires, wantSys)
}
}
func TestApplyManifestOverride_NilScan(t *testing.T) {
got := applyManifestOverride(nil, []string{"pip:requests"}, nil)
if got == nil {
t.Fatal("nil result")
}
if !got.FromManifest || !slices.Equal(got.RequiresPython, []string{"requests"}) {
t.Errorf("unexpected result: %+v", got)
}
}
func TestIsValidDepName(t *testing.T) {
cases := []struct {
category, name string
want bool
}{
// pip — python import / pip package names
{"pip", "requests", true},
{"pip", "psycopg2-binary", true},
{"pip", "google.cloud", true},
{"pip", "_private", true},
{"pip", "", false},
{"pip", "1leading_digit", false},
{"pip", "foo;__import__('os').system('pwn')", false}, // C1 injection
{"pip", "foo\nbar", false}, // newline injection
{"pip", "foo)", false}, // paren break-out
{"pip", "foo'; x='", false}, // quote break-out
// npm
{"npm", "typescript", true},
{"npm", "@scope/pkg-name", true},
{"npm", "lodash.debounce", true},
{"npm", "", false},
{"npm", "Upper", false}, // npm pkgs are lowercase
{"npm", "a');require('child_process').exec('evil", false}, // C2 injection
{"npm", "a';b('", false},
// system
{"system", "ffmpeg", true},
{"system", "gcc-13", true},
{"system", "lib_foo+bar.1", true},
{"system", "", false},
{"system", "rm -rf /", false}, // space
{"system", "foo;bar", false}, // semicolon
{"system", "$(evil)", false}, // command substitution
{"system", "`bad`", false}, // backtick
{"system", "a|b", false}, // pipe
// github — opaque spec, validated downstream
{"github", "anything/goes@v1", true},
// unknown category
{"bogus", "foo", false},
}
for _, tc := range cases {
t.Run(tc.category+"/"+tc.name, func(t *testing.T) {
got := isValidDepName(tc.category, tc.name)
if got != tc.want {
t.Errorf("isValidDepName(%q, %q) = %v, want %v", tc.category, tc.name, got, tc.want)
}
})
}
}
func TestApplyManifestOverride_DropsInjection(t *testing.T) {
scan := &SkillManifest{}
malicious := []string{
"pip:foo;__import__('os').system('pwn')",
"pip:good_one",
"npm:a');require('child_process').exec('evil",
"npm:typescript",
"system:rm -rf /",
"system:ffmpeg",
"pip:", // empty spec
"pip:>=1.0", // version only, no name
"pip:[binary]", // extras only, no name
}
got := applyManifestOverride(scan, malicious, nil)
if !got.FromManifest {
t.Fatal("FromManifest should be true")
}
wantPy := []string{"good_one"}
if !slices.Equal(got.RequiresPython, wantPy) {
t.Errorf("RequiresPython = %v, want %v (injection should be dropped)", got.RequiresPython, wantPy)
}
wantNode := []string{"typescript"}
if !slices.Equal(got.RequiresNode, wantNode) {
t.Errorf("RequiresNode = %v, want %v", got.RequiresNode, wantNode)
}
wantSys := []string{"ffmpeg"}
if !slices.Equal(got.Requires, wantSys) {
t.Errorf("Requires = %v, want %v", got.Requires, wantSys)
}
}
func TestSplitPipSpec_MalformedReturnsEmpty(t *testing.T) {
cases := []string{">=1.0", "<=2", "==3", "!=4", "~=1", "<5", ">6", "[binary]"}
for _, spec := range cases {
t.Run(spec, func(t *testing.T) {
imp, ins := splitPipSpec(spec)
if imp != "" {
t.Errorf("splitPipSpec(%q) importName = %q, want empty", spec, imp)
}
if ins != spec {
t.Errorf("installSpec = %q, want %q", ins, spec)
}
})
}
}
func TestFilterOutByImportName_DoesNotMutateInput(t *testing.T) {
original := []string{"a", "b", "c"}
snapshot := append([]string(nil), original...)
_ = filterOutByImportName(original, []string{"pip:b"}, "pip")
if !slices.Equal(original, snapshot) {
t.Errorf("input mutated: got %v, want %v", original, snapshot)
}
}
func TestFilterOutByImportName(t *testing.T) {
names := []string{"requests", "psycopg2", "bad"}
excl := []string{"pip:bad", "pip:unused_other"}
got := filterOutByImportName(names, excl, "pip")
want := []string{"requests", "psycopg2"}
if !slices.Equal(got, want) {
t.Errorf("filter = %v, want %v", got, want)
}
}
+30 -3
View File
@@ -9,12 +9,17 @@ import (
)
// SkillManifest holds dependency info for a skill.
// Populated by ScanSkillDeps via static analysis of scripts/ directory.
// Populated by ScanSkillDeps via static analysis of scripts/ directory,
// optionally overridden/filtered by SKILL.md frontmatter (deps: / exclude_deps:).
type SkillManifest struct {
Requires []string `json:"requires,omitempty"` // system binaries (python3, pandoc, ffmpeg)
RequiresPython []string `json:"requires_python,omitempty"` // raw Python import names (e.g. "openpyxl", "cv2")
RequiresNode []string `json:"requires_node,omitempty"` // npm package names (e.g. "docx", "pptxgenjs")
ScriptsDir string `json:"-"` // absolute path to scripts/ dir, used for PYTHONPATH
// Manifest-origin fields — populated when SKILL.md declares deps:/exclude_deps:.
Explicit []string `json:"explicit,omitempty"` // raw dep strings from SKILL.md deps: (e.g. "pip:psycopg2-binary")
ExcludeDeps []string `json:"exclude_deps,omitempty"` // filter list from SKILL.md exclude_deps:
FromManifest bool `json:"from_manifest,omitempty"` // true when Explicit was the authoritative source
}
// IsEmpty returns true if the manifest has no dependencies.
@@ -22,9 +27,26 @@ func (m *SkillManifest) IsEmpty() bool {
return len(m.Requires) == 0 && len(m.RequiresPython) == 0 && len(m.RequiresNode) == 0
}
// ScanSkillDeps auto-detects dependencies by statically analyzing the scripts/ directory.
// ScanSkillDeps auto-detects dependencies by statically analyzing the scripts/ directory,
// then applies any SKILL.md frontmatter overrides (deps: / exclude_deps:).
func ScanSkillDeps(skillDir string) *SkillManifest {
return scanScriptsDir(filepath.Join(skillDir, "scripts"))
scan := scanScriptsDir(filepath.Join(skillDir, "scripts"))
deps, excludeDeps := parseSkillManifestFile(skillMdPath(skillDir))
if len(deps) == 0 && len(excludeDeps) == 0 {
return scan
}
merged := applyManifestOverride(scan, deps, excludeDeps)
if merged.FromManifest {
slog.Debug("dep_scanner: manifest override applied",
"dir", skillDir,
"explicit_count", len(deps),
"scan_py", len(scan.RequiresPython),
"scan_node", len(scan.RequiresNode))
} else if len(excludeDeps) > 0 {
slog.Debug("dep_scanner: manifest exclude applied",
"dir", skillDir, "exclude_count", len(excludeDeps))
}
return merged
}
// scanScriptsDir statically analyzes script files to detect dependencies.
@@ -182,6 +204,8 @@ func normalizeNodePkg(pkg string) string {
}
// MergeDeps merges two manifests, deduplicating entries.
// Manifest-origin fields (Explicit, ExcludeDeps, FromManifest) are OR-folded /
// unioned so the merged result remains authoritative if either side was.
func MergeDeps(a, b *SkillManifest) *SkillManifest {
if a == nil {
return b
@@ -193,6 +217,9 @@ func MergeDeps(a, b *SkillManifest) *SkillManifest {
Requires: mergeUnique(a.Requires, b.Requires),
RequiresPython: mergeUnique(a.RequiresPython, b.RequiresPython),
RequiresNode: mergeUnique(a.RequiresNode, b.RequiresNode),
Explicit: mergeUnique(a.Explicit, b.Explicit),
ExcludeDeps: mergeUnique(a.ExcludeDeps, b.ExcludeDeps),
FromManifest: a.FromManifest || b.FromManifest,
}
}
+142
View File
@@ -0,0 +1,142 @@
package skills
import (
"os"
"path/filepath"
"slices"
"testing"
)
func writeFile(t *testing.T, path, content string) {
t.Helper()
if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(path, []byte(content), 0o644); err != nil {
t.Fatal(err)
}
}
// Regression guard: existing skill without deps:/exclude_deps: fields must
// produce the same scan output as pre-manifest behavior.
func TestScanSkillDeps_NoManifestFields_Unchanged(t *testing.T) {
dir := t.TempDir()
writeFile(t, filepath.Join(dir, "SKILL.md"), `---
name: sample
description: sample skill
---
body`)
writeFile(t, filepath.Join(dir, "scripts", "run.py"),
"import requests\nimport json\n")
got := ScanSkillDeps(dir)
if got.FromManifest {
t.Error("FromManifest should be false")
}
if len(got.Explicit) != 0 {
t.Errorf("Explicit should be empty: %v", got.Explicit)
}
if !slices.Contains(got.RequiresPython, "requests") {
t.Errorf("expected requests in RequiresPython, got %v", got.RequiresPython)
}
// json is stdlib → must be excluded
if slices.Contains(got.RequiresPython, "json") {
t.Errorf("stdlib json leaked into RequiresPython: %v", got.RequiresPython)
}
}
func TestScanSkillDeps_ExplicitDeps_Authoritative(t *testing.T) {
dir := t.TempDir()
writeFile(t, filepath.Join(dir, "SKILL.md"), `---
name: sample
deps:
- pip:psycopg2-binary
- pip:requests>=2.31
- system:ffmpeg
---
`)
// Auto-scan would pick up 'numpy' but manifest overrides.
writeFile(t, filepath.Join(dir, "scripts", "run.py"),
"import numpy\nimport psycopg2\n")
got := ScanSkillDeps(dir)
if !got.FromManifest {
t.Fatal("FromManifest should be true")
}
wantPy := []string{"psycopg2-binary", "requests"}
if !slices.Equal(got.RequiresPython, wantPy) {
t.Errorf("RequiresPython = %v, want %v (auto-scanned numpy should be overridden)", got.RequiresPython, wantPy)
}
if !slices.Contains(got.Requires, "ffmpeg") {
t.Errorf("system ffmpeg missing: %v", got.Requires)
}
// Auto-scan numpy must NOT leak through when explicit is set.
if slices.Contains(got.RequiresPython, "numpy") {
t.Errorf("numpy leaked despite explicit override: %v", got.RequiresPython)
}
}
func TestScanSkillDeps_ExcludeDeps_Filters(t *testing.T) {
dir := t.TempDir()
writeFile(t, filepath.Join(dir, "SKILL.md"), `---
name: sample
exclude_deps:
- pip:my_local_helper
---
`)
writeFile(t, filepath.Join(dir, "scripts", "run.py"),
"import requests\nimport my_local_helper\n")
got := ScanSkillDeps(dir)
if got.FromManifest {
t.Error("FromManifest should be false for exclude-only")
}
if !slices.Contains(got.RequiresPython, "requests") {
t.Errorf("requests missing: %v", got.RequiresPython)
}
if slices.Contains(got.RequiresPython, "my_local_helper") {
t.Errorf("my_local_helper should be filtered: %v", got.RequiresPython)
}
if !slices.Equal(got.ExcludeDeps, []string{"pip:my_local_helper"}) {
t.Errorf("ExcludeDeps = %v", got.ExcludeDeps)
}
}
func TestScanSkillDeps_NoSKILLmd(t *testing.T) {
dir := t.TempDir()
writeFile(t, filepath.Join(dir, "scripts", "run.py"), "import requests\n")
got := ScanSkillDeps(dir)
if got.FromManifest {
t.Error("FromManifest should be false without SKILL.md")
}
if !slices.Contains(got.RequiresPython, "requests") {
t.Errorf("requests missing: %v", got.RequiresPython)
}
}
func TestMergeDeps_PreservesManifestFields(t *testing.T) {
a := &SkillManifest{
RequiresPython: []string{"requests"},
Explicit: []string{"pip:requests"},
FromManifest: true,
}
b := &SkillManifest{
RequiresPython: []string{"numpy"},
ExcludeDeps: []string{"pip:foo"},
}
got := MergeDeps(a, b)
if !got.FromManifest {
t.Error("FromManifest OR-fold failed")
}
if !slices.Equal(got.Explicit, []string{"pip:requests"}) {
t.Errorf("Explicit = %v", got.Explicit)
}
if !slices.Equal(got.ExcludeDeps, []string{"pip:foo"}) {
t.Errorf("ExcludeDeps = %v", got.ExcludeDeps)
}
wantPy := []string{"requests", "numpy"}
if !slices.Equal(got.RequiresPython, wantPy) {
t.Errorf("RequiresPython = %v, want %v", got.RequiresPython, wantPy)
}
}
+72
View File
@@ -13,6 +13,7 @@ import (
"context"
"encoding/json"
"fmt"
"log/slog"
"os"
"path/filepath"
"regexp"
@@ -524,6 +525,77 @@ func stripFrontmatter(content string) string {
return frontmatterRe.ReplaceAllString(normalizeLineEndings(content), "")
}
// parseSimpleYAMLLists parses YAML list fields into separate []string values keyed
// by top-level key. Scalars and block scalars are ignored. Complements
// parseSimpleYAML (which joins list items with spaces). Used by dep_manifest.go.
//
// Supported grammar (subset):
// - Flat list: key:\n - item1\n - item2
// - Quoted items: key:\n - "value"
// - CRLF line endings are normalized
//
// Not supported — misuse is logged at debug level and the key is skipped:
// - Nested maps (key:\n subkey:\n - item) — values would lose prefix semantics
// - Flow-style lists (key: [a, b]) — silently returns empty
// - Dash without space (-item) — silently returns empty
//
// Example:
//
// deps:
// - pip:psycopg2-binary
// - system:ffmpeg
//
// Returns: {"deps": ["pip:psycopg2-binary", "system:ffmpeg"]}
func parseSimpleYAMLLists(content string) map[string][]string {
result := make(map[string][]string)
lines := strings.Split(normalizeLineEndings(content), "\n")
var currentKey string
for _, line := range lines {
trimmed := strings.TrimSpace(line)
if trimmed == "" || strings.HasPrefix(trimmed, "#") {
continue
}
// Indented line — could be a list item for currentKey.
if len(line) > 0 && (line[0] == ' ' || line[0] == '\t') {
if currentKey == "" {
continue
}
if strings.HasPrefix(trimmed, "- ") {
val := strings.TrimSpace(trimmed[2:])
val = strings.Trim(val, "\"'")
if val != "" {
result[currentKey] = append(result[currentKey], val)
}
continue
}
// Indented non-list line under a tracked key — e.g. nested map:
// deps:\n pip:\n - requests
// Silent flatten would drop the "pip:" prefix and miscategorize. Skip + clear key.
if strings.Contains(trimmed, ":") {
slog.Debug("skills: parseSimpleYAMLLists skipped nested map",
"key", currentKey, "nested", trimmed)
delete(result, currentKey)
currentKey = ""
}
continue
}
// Top-level key — reset list tracking.
idx := strings.IndexByte(trimmed, ':')
if idx < 0 {
currentKey = ""
continue
}
key := strings.TrimSpace(trimmed[:idx])
val := strings.TrimSpace(trimmed[idx+1:])
if val == "" {
currentKey = key
} else {
currentKey = ""
}
}
return result
}
// parseSimpleYAML parses a subset of YAML: simple key: value pairs,
// multiline block scalars (| and >), and list values (- item).
func parseSimpleYAML(content string) map[string]string {
+89
View File
@@ -567,6 +567,95 @@ func TestLoader_ManagedSkills_WorkspaceTakesPriority(t *testing.T) {
// --- Dirs ---
func TestParseSimpleYAMLLists(t *testing.T) {
cases := []struct {
name string
content string
key string
want []string
}{
{
name: "deps list",
content: `name: test
deps:
- pip:psycopg2-binary
- system:ffmpeg
`,
key: "deps",
want: []string{"pip:psycopg2-binary", "system:ffmpeg"},
},
{
name: "quoted items",
content: `deps:
- "pip:requests"
- 'npm:typescript'
`,
key: "deps",
want: []string{"pip:requests", "npm:typescript"},
},
{
name: "empty key",
content: `name: test
description: plain
`,
key: "deps",
want: nil,
},
{
name: "crlf",
content: "deps:\r\n - pip:a\r\n - pip:b\r\n",
key: "deps",
want: []string{"pip:a", "pip:b"},
},
{
name: "scalar skipped",
content: `deps: inline
other:
- x
`,
key: "deps",
want: nil,
},
{
name: "multiple keys",
content: `deps:
- pip:a
exclude_deps:
- pip:b
`,
key: "exclude_deps",
want: []string{"pip:b"},
},
{
// H2 regression: nested-map under tracked key must drop the key to
// avoid silent prefix-loss ("pip:" stripped → miscategorized as system).
name: "nested_map_dropped",
content: `deps:
pip:
- requests
system:
- ffmpeg
`,
key: "deps",
want: nil,
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
got := parseSimpleYAMLLists(tc.content)
gv := got[tc.key]
if len(gv) != len(tc.want) {
t.Fatalf("len = %d, want %d; got=%v", len(gv), len(tc.want), gv)
}
for i, v := range gv {
if v != tc.want[i] {
t.Errorf("[%d] = %q, want %q", i, v, tc.want[i])
}
}
})
}
}
func TestLoader_Dirs(t *testing.T) {
ws := t.TempDir()
global := t.TempDir()