From 3d6723e7c04b52bf5bc85ae87f067cde59032cc0 Mon Sep 17 00:00:00 2001 From: viettranx Date: Fri, 17 Apr 2026 19:44:50 +0700 Subject: [PATCH] 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. --- docs/14-skills-runtime.md | 73 +++++ internal/skills/bundled_smoke_test.go | 43 +++ internal/skills/dep_manifest.go | 223 +++++++++++++++ internal/skills/dep_manifest_test.go | 376 ++++++++++++++++++++++++++ internal/skills/dep_scanner.go | 33 ++- internal/skills/dep_scanner_test.go | 142 ++++++++++ internal/skills/loader.go | 72 +++++ internal/skills/loader_test.go | 89 ++++++ 8 files changed, 1048 insertions(+), 3 deletions(-) create mode 100644 internal/skills/bundled_smoke_test.go create mode 100644 internal/skills/dep_manifest.go create mode 100644 internal/skills/dep_manifest_test.go create mode 100644 internal/skills/dep_scanner_test.go diff --git a/docs/14-skills-runtime.md b/docs/14-skills-runtime.md index e142833f..b2f93d3f 100644 --- a/docs/14-skills-runtime.md +++ b/docs/14-skills-runtime.md @@ -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) diff --git a/internal/skills/bundled_smoke_test.go b/internal/skills/bundled_smoke_test.go new file mode 100644 index 00000000..25d49f62 --- /dev/null +++ b/internal/skills/bundled_smoke_test.go @@ -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) + }) + } +} diff --git a/internal/skills/dep_manifest.go b/internal/skills/dep_manifest.go new file mode 100644 index 00000000..2a734aa4 --- /dev/null +++ b/internal/skills/dep_manifest.go @@ -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: e.g. pip:psycopg2-binary, pip:requests>=2.31, pip:psycopg[binary] +// npm: e.g. npm:typescript +// github: e.g. github:cli/cli@v2.40.0 +// system: e.g. system:ffmpeg +// 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 ":". +// For "system" both "system:" and bare "" 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 +} diff --git a/internal/skills/dep_manifest_test.go b/internal/skills/dep_manifest_test.go new file mode 100644 index 00000000..859dd9ae --- /dev/null +++ b/internal/skills/dep_manifest_test.go @@ -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) + } +} diff --git a/internal/skills/dep_scanner.go b/internal/skills/dep_scanner.go index 57455598..e828ab89 100644 --- a/internal/skills/dep_scanner.go +++ b/internal/skills/dep_scanner.go @@ -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, } } diff --git a/internal/skills/dep_scanner_test.go b/internal/skills/dep_scanner_test.go new file mode 100644 index 00000000..10452edf --- /dev/null +++ b/internal/skills/dep_scanner_test.go @@ -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) + } +} diff --git a/internal/skills/loader.go b/internal/skills/loader.go index af64602b..dbae7c11 100644 --- a/internal/skills/loader.go +++ b/internal/skills/loader.go @@ -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 { diff --git a/internal/skills/loader_test.go b/internal/skills/loader_test.go index 7bb4f5bd..6c4e7788 100644 --- a/internal/skills/loader_test.go +++ b/internal/skills/loader_test.go @@ -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()