From 4bfbec00a6e3702b8cc404d428c5fbc85c897ca8 Mon Sep 17 00:00:00 2001 From: dacharyc Date: Thu, 15 Oct 2026 10:50:53 -0400 Subject: [PATCH 1/2] Discover skills under grouping directories, bounded and symlink-safe DetectSkills looked only at the immediate subdirectories, so a catalog laid out as skills/// reported "no skills found" and had to be validated one product at a time (#77). The agentskills.io client implementation guide tells scanners to walk 4-6 levels and stop at 2000 directories. Add skillcheck.DiscoverSkills(root, maxDepth), which searches breadth-first to a depth bound (default 6) and returns a Discovery: the mode, the sorted skill directories, each one's depth below the root, and whether the scan hit the directory budget. DetectSkills keeps its signature and calls it with the default depth. Symlinks are still followed, since a skills directory assembled from links to folders elsewhere is a supported layout, but each real directory is visited once, keyed by its resolved path, so cycles and aliases neither loop nor report a skill twice. A skill is detected the moment its parent is listed and the budget counts listings, so a wide or linked sibling tree cannot use up the budget before a real skill is found. A skill's own subtree is not searched, hidden directories are skipped, and so are node_modules and __pycache__. The CLI gains a persistent --max-depth flag, prints a warning when the budget was hit, and names the depth in the "no skills found" error. The README states that only the given path is searched: the guide's home-directory and .agents/skills scanning is an agent's concern, not a validator's, and .gitignore is not consulted. Fixes #77 --- README.md | 14 +- cmd/exitcode_integration_test.go | 39 +++++ cmd/root.go | 17 ++- skillcheck/validator.go | 175 ++++++++++++++++++++--- skillcheck/validator_test.go | 237 +++++++++++++++++++++++++++++++ 5 files changed, 455 insertions(+), 27 deletions(-) diff --git a/README.md b/README.md index 7537800..7d3cd2e 100644 --- a/README.md +++ b/README.md @@ -448,6 +448,8 @@ The `--compare` flag is useful for understanding how different models perceive y All commands accept `-o text` (default), `-o compact`, `-o json`, or `-o markdown` for output format. Use `--emit-annotations` with any format to emit GitHub Actions workflow annotations alongside normal output. +All commands that take a path also accept `--max-depth=N` to control how many directory levels below the path are searched for skills (see [Multi-skill directories](#multi-skill-directories)). + ### Compact output The default text output prints every passing check, token counts, and the @@ -658,12 +660,20 @@ For in-progress skills where you want warnings to be non-blocking, remove `--str ### Multi-skill directories -If the given path does not contain a `SKILL.md` but has subdirectories that do, the validator automatically detects and validates each skill. This is useful when skills are organized as sibling directories (e.g. `skills/algorithmic-art/`, `skills/brand-guidelines/`). Symlinks are followed during detection. +If the given path does not contain a `SKILL.md`, the validator searches its subdirectories for skills and validates each one. This covers skills organized as sibling directories (e.g. `skills/algorithmic-art/`, `skills/brand-guidelines/`) and skills grouped under intermediate directories (e.g. `skills///`), so one invocation can cover a whole catalog: ``` skill-validator check skills/ ``` +Only the given path is searched. The [agentskills.io implementation guide](https://agentskills.io/client-implementation/adding-skills-support.md) describes how an agent discovers skills at session start, by scanning fixed locations such as `~/.agents/skills/` and `/.agents/skills/`. A validator has no reason to look anywhere but where it is pointed, so skill-validator never scans the home directory or any other conventional location. What it does take from the guide is its scanning rules: + +- Subdirectories are searched up to 6 levels deep by default. Use `--max-depth=N` to change this; `--max-depth=1` searches only the immediate subdirectories, and `--max-depth=0` treats the given path as a single skill and nothing else. +- A directory that contains a `SKILL.md` is a skill, and nothing below it is searched, so a `SKILL.md` inside a skill's examples or references does not count as a second skill. +- Hidden directories (names starting with `.`, which covers `.git/`) and dependency directories (`node_modules/`, `__pycache__/`) are skipped. `.gitignore` is not consulted: honoring it would mean locating the repository root and evaluating nested ignore rules, and the directory budget already bounds the cost of scanning build output. +- Symlinks are followed, so a skills directory assembled from links to skill folders kept elsewhere works. Each real directory is examined once, which makes symlink cycles harmless and keeps a skill from being reported twice under two paths. +- At most 2000 directories are listed. A skill is found as soon as its parent directory is listed, and the search is breadth-first, so shallow skills are found before a large subtree can use up that budget. If the limit is reached, a warning on stderr says that skills below that point were not found. + Each skill is validated independently. The text output separates skills with a line and appends an overall summary. The JSON output wraps individual skill reports in a `skills` array: ```json @@ -678,7 +688,7 @@ Each skill is validated independently. The text output separates skills with a l } ``` -If no `SKILL.md` is found at the root or in any immediate subdirectory, the validator exits with code 3 (CLI error). +If no `SKILL.md` is found at the root or within the search depth, the validator exits with code 3 (CLI error). ## Examples diff --git a/cmd/exitcode_integration_test.go b/cmd/exitcode_integration_test.go index b26b5ef..24f80cc 100644 --- a/cmd/exitcode_integration_test.go +++ b/cmd/exitcode_integration_test.go @@ -312,3 +312,42 @@ func TestExcludeTokenPathsOutputFormats(t *testing.T) { }) } } + +// Issue #77: skills under intermediate directories are found by default, +// and --max-depth bounds the search. +func TestMaxDepth(t *testing.T) { + bin := buildBinary(t) + root := t.TempDir() + skillDir := filepath.Join(root, "product", "skill") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatal(err) + } + skill := "---\nname: skill\ndescription: Nested two levels down.\n---\n# Skill\n\nDo the thing.\n" + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte(skill), 0o644); err != nil { + t.Fatal(err) + } + + tests := []struct { + name string + args []string + wantCode int + want string + }{ + {"default finds depth 2", []string{"validate", "structure", "-o", "json", root}, 0, `"skill_dir"`}, + {"max-depth 1 does not", []string{"validate", "structure", "--max-depth=1", root}, 3, "up to 1 levels deep"}, + {"max-depth 0 on the skill", []string{"validate", "structure", "--max-depth=0", skillDir}, 0, ""}, + {"negative is a usage error", []string{"check", "--max-depth=-1", root}, 3, "invalid --max-depth"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cmd := exec.Command(bin, tt.args...) + out, _ := cmd.CombinedOutput() + if got := cmd.ProcessState.ExitCode(); got != tt.wantCode { + t.Errorf("exit code = %d, want %d\noutput: %s", got, tt.wantCode, out) + } + if tt.want != "" && !strings.Contains(string(out), tt.want) { + t.Errorf("expected output to contain %q, got:\n%s", tt.want, out) + } + }) + } +} diff --git a/cmd/root.go b/cmd/root.go index 1bf1e9f..0b52840 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -16,6 +16,7 @@ const version = "v1.6.2" var ( outputFormat string emitAnnotations bool + maxDepth int ) var rootCmd = &cobra.Command{ @@ -33,6 +34,7 @@ func init() { rootCmd.Version = version rootCmd.PersistentFlags().StringVarP(&outputFormat, "output", "o", "text", "output format: text, compact, json, or markdown") rootCmd.PersistentFlags().BoolVar(&emitAnnotations, "emit-annotations", false, "emit GitHub Actions workflow command annotations (::error/::warning) alongside normal output") + rootCmd.PersistentFlags().IntVar(&maxDepth, "max-depth", skillcheck.DefaultMaxDepth, "how many directory levels below to search for SKILL.md files (0 searches only)") } // Execute runs the root command. @@ -78,10 +80,17 @@ func detectAndResolve(args []string) (string, types.SkillMode, []string, error) return "", 0, nil, err } - mode, dirs := skillcheck.DetectSkills(absDir) - if mode == types.NoSkill { - return "", 0, nil, fmt.Errorf("no skills found in %s (expected SKILL.md or subdirectories containing SKILL.md)", args[0]) + if maxDepth < 0 { + return "", 0, nil, fmt.Errorf("invalid --max-depth: %d is negative", maxDepth) } - return absDir, mode, dirs, nil + d := skillcheck.DiscoverSkills(absDir, maxDepth) + if d.Truncated { + fmt.Fprintf(os.Stderr, "Warning: skill discovery stopped after listing %d directories under %s; skills below that point were not found\n", skillcheck.MaxDiscoveryDirs, args[0]) + } + if d.Mode == types.NoSkill { + return "", 0, nil, fmt.Errorf("no skills found in %s (expected SKILL.md in the directory or in a subdirectory up to %d levels deep)", args[0], maxDepth) + } + + return absDir, d.Mode, d.Dirs, nil } diff --git a/skillcheck/validator.go b/skillcheck/validator.go index 581e43c..4224553 100644 --- a/skillcheck/validator.go +++ b/skillcheck/validator.go @@ -15,42 +15,175 @@ import ( "github.com/agent-ecosystem/skill-validator/util" ) +// Discovery bounds. The agentskills.io client implementation guide +// recommends that scanners "set reasonable bounds (e.g., max depth of 4-6 +// levels, max 2000 directories) to prevent runaway scanning in large +// directory trees". +const ( + // DefaultMaxDepth is how many directory levels below the given path + // DetectSkills searches for SKILL.md files. Depth 1 is the immediate + // subdirectories. + DefaultMaxDepth = 6 + + // MaxDiscoveryDirs is the most directories a discovery scan lists + // before it stops. A scan that stops early reports Truncated. + MaxDiscoveryDirs = 2000 +) + +// skipDirs are directory names that never hold skills and can be large +// enough to use up the discovery budget. Hidden directories (".git", +// ".venv", ...) are skipped separately by name prefix. +var skipDirs = map[string]bool{ + "node_modules": true, + "__pycache__": true, +} + +// Discovery is the result of DiscoverSkills. +type Discovery struct { + Mode types.SkillMode + // Dirs holds the skill directories in sorted order. For SingleSkill it + // is the given path; for NoSkill it is nil. + Dirs []string + // Depths maps each entry of Dirs to how many directory levels below + // the given path it was found at: 0 for a single skill, 1 for an + // immediate subdirectory. Agents differ in how deep they look, so a + // skill found at depth 2 may be invisible to some of them. + Depths map[string]int + // Truncated reports that the scan stopped at MaxDiscoveryDirs before it + // had examined the whole tree, so skills below that point were missed. + Truncated bool +} + // DetectSkills determines whether dir is a single skill, a multi-skill -// parent, or contains no skills. It follows symlinks when checking -// subdirectories. +// parent, or contains no skills, searching DefaultMaxDepth levels deep. +// See DiscoverSkills. func DetectSkills(dir string) (types.SkillMode, []string) { - // If the directory itself contains SKILL.md, it's a single skill. - if _, err := os.Stat(filepath.Join(dir, "SKILL.md")); err == nil { - return types.SingleSkill, []string{dir} + d := DiscoverSkills(dir, DefaultMaxDepth) + return d.Mode, d.Dirs +} + +// DiscoverSkills determines whether root is a single skill, a multi-skill +// parent, or contains no skills. +// +// If root itself contains SKILL.md it is a single skill and nothing below +// it is examined. Otherwise the subdirectories are searched breadth-first, +// up to maxDepth levels below root (1 means the immediate subdirectories +// only), and every directory holding a SKILL.md is a skill. A skill's own +// subtree is not searched further, so a SKILL.md nested inside a skill's +// references or examples does not count as a second skill. Hidden +// directories and dependency directories such as node_modules are +// skipped. Only the given path is scanned: the user's home directory and +// the .agents/skills conventions are an agent's concern, not a +// validator's. +// +// Symlinks are followed, so a skills directory assembled from links to +// skill folders elsewhere works. Each real directory is examined once, +// which makes symlink cycles and aliases harmless and keeps a skill from +// being reported under two paths. The scan lists at most MaxDiscoveryDirs +// directories and reports Truncated if it had to stop. A skill is found +// as soon as its parent directory is listed, and the search is +// breadth-first, so shallow skills are found before a large or linked +// subtree can exhaust that budget. +func DiscoverSkills(root string, maxDepth int) Discovery { + return discoverSkills(root, maxDepth, MaxDiscoveryDirs) +} + +func discoverSkills(root string, maxDepth, maxDirs int) Discovery { + if hasSkillFile(root) { + return Discovery{Mode: types.SingleSkill, Dirs: []string{root}, Depths: map[string]int{root: 0}} + } + if maxDepth < 1 { + return Discovery{Mode: types.NoSkill} } - // Scan immediate subdirectories for SKILL.md. - entries, err := os.ReadDir(dir) + // A queued directory is one that will be listed. Its children are + // classified as they are listed: a child with SKILL.md is a skill and + // is not queued; any other child is queued if it is shallow enough to + // be listed in turn. So a skill is found as soon as its parent is + // listed, and the budget is spent on listings, not on skills. + type dirItem struct { + path string // path as reached from root, reported to the caller + real string // symlink-resolved path, used to visit each directory once + depth int + } + + visited := make(map[string]bool) + realRoot, err := filepath.EvalSymlinks(root) if err != nil { - return types.NoSkill, nil + return Discovery{Mode: types.NoSkill} } + visited[realRoot] = true + queue := []dirItem{{path: root, real: realRoot, depth: 0}} var skillDirs []string - for _, entry := range entries { - name := entry.Name() - if strings.HasPrefix(name, ".") { - continue + depths := make(map[string]int) + listed := 0 + truncated := false + + for len(queue) > 0 { + cur := queue[0] + queue = queue[1:] + + if listed >= maxDirs { + truncated = true + break } - subdir := filepath.Join(dir, name) - // Use os.Stat (not entry.IsDir()) to follow symlinks. - info, err := os.Stat(subdir) - if err != nil || !info.IsDir() { + listed++ + + entries, err := os.ReadDir(cur.path) + if err != nil { continue } - if _, err := os.Stat(filepath.Join(subdir, "SKILL.md")); err == nil { - skillDirs = append(skillDirs, subdir) + for _, entry := range entries { + name := entry.Name() + if strings.HasPrefix(name, ".") || skipDirs[name] { + continue + } + sub := filepath.Join(cur.path, name) + var real string + switch { + case entry.IsDir(): + real = filepath.Join(cur.real, name) + case entry.Type()&os.ModeSymlink != 0: + // Follow the link, but only into a directory that exists. + resolved, err := filepath.EvalSymlinks(sub) + if err != nil { + continue + } + if info, err := os.Stat(resolved); err != nil || !info.IsDir() { + continue + } + real = resolved + default: + continue + } + if visited[real] { + continue + } + visited[real] = true + + if hasSkillFile(sub) { + skillDirs = append(skillDirs, sub) + depths[sub] = cur.depth + 1 + continue + } + if cur.depth+1 < maxDepth { + queue = append(queue, dirItem{path: sub, real: real, depth: cur.depth + 1}) + } } } - if len(skillDirs) > 0 { - return types.MultiSkill, skillDirs + if len(skillDirs) == 0 { + return Discovery{Mode: types.NoSkill, Truncated: truncated} } - return types.NoSkill, nil + sort.Strings(skillDirs) + return Discovery{Mode: types.MultiSkill, Dirs: skillDirs, Depths: depths, Truncated: truncated} +} + +// hasSkillFile reports whether dir contains a SKILL.md (following symlinks). +func hasSkillFile(dir string) bool { + _, err := os.Stat(filepath.Join(dir, "SKILL.md")) + return err == nil } // ReadSkillRaw reads the raw SKILL.md content from a directory without parsing diff --git a/skillcheck/validator_test.go b/skillcheck/validator_test.go index 35d30a4..fad32e6 100644 --- a/skillcheck/validator_test.go +++ b/skillcheck/validator_test.go @@ -1,8 +1,10 @@ package skillcheck import ( + "fmt" "os" "path/filepath" + "reflect" "runtime" "strings" "testing" @@ -360,3 +362,238 @@ func TestDetectSkills(t *testing.T) { } }) } + +// Issue #77: skills grouped under intermediate directories (for example +// skills///SKILL.md) are discovered up to a depth bound. +func TestDiscoverSkills_Nested(t *testing.T) { + t.Run("two levels", func(t *testing.T) { + dir := t.TempDir() + writeSkill(t, filepath.Join(dir, "cuopt", "cuopt-developer"), "---\nname: a\n---\n") + writeSkill(t, filepath.Join(dir, "Megatron-Bridge", "bump-base-image"), "---\nname: b\n---\n") + writeSkill(t, filepath.Join(dir, "Megatron-Bridge", "update-golden-values"), "---\nname: c\n---\n") + d := DiscoverSkills(dir, DefaultMaxDepth) + if d.Mode != types.MultiSkill { + t.Fatalf("expected MultiSkill, got %d", d.Mode) + } + want := []string{ + filepath.Join(dir, "Megatron-Bridge", "bump-base-image"), + filepath.Join(dir, "Megatron-Bridge", "update-golden-values"), + filepath.Join(dir, "cuopt", "cuopt-developer"), + } + if !reflect.DeepEqual(d.Dirs, want) { + t.Errorf("dirs = %v, want %v", d.Dirs, want) + } + if d.Truncated { + t.Error("unexpected truncation") + } + }) + + t.Run("mixed depths", func(t *testing.T) { + dir := t.TempDir() + writeSkill(t, filepath.Join(dir, "top"), "---\nname: top\n---\n") + writeSkill(t, filepath.Join(dir, "group", "deep"), "---\nname: deep\n---\n") + d := DiscoverSkills(dir, DefaultMaxDepth) + want := []string{filepath.Join(dir, "group", "deep"), filepath.Join(dir, "top")} + if !reflect.DeepEqual(d.Dirs, want) { + t.Errorf("dirs = %v, want %v", d.Dirs, want) + } + wantDepths := map[string]int{want[0]: 2, want[1]: 1} + if !reflect.DeepEqual(d.Depths, wantDepths) { + t.Errorf("depths = %v, want %v", d.Depths, wantDepths) + } + }) + + t.Run("single skill has depth 0", func(t *testing.T) { + dir := t.TempDir() + writeSkill(t, dir, "---\nname: s\n---\n") + d := DiscoverSkills(dir, DefaultMaxDepth) + if want := map[string]int{dir: 0}; !reflect.DeepEqual(d.Depths, want) { + t.Errorf("depths = %v, want %v", d.Depths, want) + } + }) + + t.Run("depth bound", func(t *testing.T) { + dir := t.TempDir() + at := func(depth int) string { + p := dir + for i := 1; i <= depth; i++ { + p = filepath.Join(p, fmt.Sprintf("l%d", i)) + } + return p + } + writeSkill(t, at(DefaultMaxDepth), "---\nname: in\n---\n") + writeSkill(t, filepath.Join(at(DefaultMaxDepth-1), "other", "x"), "---\nname: out\n---\n") // depth 7 + d := DiscoverSkills(dir, DefaultMaxDepth) + if want := []string{at(DefaultMaxDepth)}; !reflect.DeepEqual(d.Dirs, want) { + t.Errorf("dirs = %v, want %v", d.Dirs, want) + } + if d.Truncated { + t.Error("a depth cutoff is not a truncation") + } + // maxDepth 1 is the pre-#77 behavior: immediate subdirectories only. + if d := DiscoverSkills(dir, 1); d.Mode != types.NoSkill { + t.Errorf("maxDepth 1: expected NoSkill, got %d %v", d.Mode, d.Dirs) + } + // maxDepth 0 looks at the given directory only. + if d := DiscoverSkills(at(DefaultMaxDepth), 0); d.Mode != types.SingleSkill { + t.Errorf("maxDepth 0 on a skill: expected SingleSkill, got %d", d.Mode) + } + if d := DiscoverSkills(dir, 0); d.Mode != types.NoSkill { + t.Errorf("maxDepth 0 on a parent: expected NoSkill, got %d", d.Mode) + } + }) + + t.Run("does not descend into a skill", func(t *testing.T) { + dir := t.TempDir() + writeSkill(t, filepath.Join(dir, "skill"), "---\nname: skill\n---\n") + writeSkill(t, filepath.Join(dir, "skill", "examples", "nested"), "---\nname: nested\n---\n") + d := DiscoverSkills(dir, DefaultMaxDepth) + if want := []string{filepath.Join(dir, "skill")}; !reflect.DeepEqual(d.Dirs, want) { + t.Errorf("dirs = %v, want %v", d.Dirs, want) + } + }) + + t.Run("skips hidden dirs at any depth", func(t *testing.T) { + dir := t.TempDir() + writeSkill(t, filepath.Join(dir, "group", ".hidden", "skill"), "---\nname: hidden\n---\n") + writeSkill(t, filepath.Join(dir, "group", "shown"), "---\nname: shown\n---\n") + d := DiscoverSkills(dir, DefaultMaxDepth) + if want := []string{filepath.Join(dir, "group", "shown")}; !reflect.DeepEqual(d.Dirs, want) { + t.Errorf("dirs = %v, want %v", d.Dirs, want) + } + }) + + t.Run("skips dependency dirs", func(t *testing.T) { + dir := t.TempDir() + writeSkill(t, filepath.Join(dir, "node_modules", "pkg", "skill"), "---\nname: nm\n---\n") + writeSkill(t, filepath.Join(dir, "__pycache__", "skill"), "---\nname: pyc\n---\n") + writeSkill(t, filepath.Join(dir, "web", "node_modules", "skill"), "---\nname: nested\n---\n") + writeSkill(t, filepath.Join(dir, "web", "skill"), "---\nname: web\n---\n") + d := DiscoverSkills(dir, DefaultMaxDepth) + if want := []string{filepath.Join(dir, "web", "skill")}; !reflect.DeepEqual(d.Dirs, want) { + t.Errorf("dirs = %v, want %v", d.Dirs, want) + } + }) + + t.Run("directory budget", func(t *testing.T) { + dir := t.TempDir() + // 50 directories sort before the one that holds a skill at depth 2. + // Listing the root finds the depth-1 skill at once; the budget of + // 20 listings then runs out before the depth-2 skill's parent is + // listed, and the scan reports the truncation. + for i := 0; i < 50; i++ { + if err := os.MkdirAll(filepath.Join(dir, fmt.Sprintf("a%02d", i), "sub"), 0o755); err != nil { + t.Fatal(err) + } + } + writeSkill(t, filepath.Join(dir, "shallow"), "---\nname: shallow\n---\n") + writeSkill(t, filepath.Join(dir, "zgroup", "deep"), "---\nname: deep\n---\n") + + d := discoverSkills(dir, DefaultMaxDepth, 20) + if want := []string{filepath.Join(dir, "shallow")}; !reflect.DeepEqual(d.Dirs, want) { + t.Errorf("dirs = %v, want %v", d.Dirs, want) + } + if !d.Truncated { + t.Error("expected Truncated when the budget runs out with directories left") + } + + // With budget for the whole tree, nothing is truncated. + d = discoverSkills(dir, DefaultMaxDepth, MaxDiscoveryDirs) + if len(d.Dirs) != 2 || d.Truncated { + t.Errorf("dirs = %v truncated = %v, want 2 dirs and no truncation", d.Dirs, d.Truncated) + } + }) +} + +func TestDiscoverSkills_Symlinks(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlinks require admin on Windows") + } + mustSymlink := func(t *testing.T, target, link string) { + t.Helper() + if err := os.Symlink(target, link); err != nil { + t.Fatal(err) + } + } + + t.Run("cycle to root", func(t *testing.T) { + dir := t.TempDir() + writeSkill(t, filepath.Join(dir, "team", "skill"), "---\nname: s\n---\n") + mustSymlink(t, dir, filepath.Join(dir, "loop")) + d := DiscoverSkills(dir, DefaultMaxDepth) + if want := []string{filepath.Join(dir, "team", "skill")}; !reflect.DeepEqual(d.Dirs, want) { + t.Errorf("dirs = %v, want %v", d.Dirs, want) + } + if d.Truncated { + t.Error("a cycle must not consume the directory budget") + } + }) + + t.Run("cycle to parent", func(t *testing.T) { + dir := t.TempDir() + writeSkill(t, filepath.Join(dir, "team", "skill"), "---\nname: s\n---\n") + mustSymlink(t, "..", filepath.Join(dir, "team", "up")) + d := DiscoverSkills(dir, DefaultMaxDepth) + if want := []string{filepath.Join(dir, "team", "skill")}; !reflect.DeepEqual(d.Dirs, want) { + t.Errorf("dirs = %v, want %v", d.Dirs, want) + } + }) + + t.Run("alias of a skill is reported once", func(t *testing.T) { + dir := t.TempDir() + writeSkill(t, filepath.Join(dir, "real"), "---\nname: s\n---\n") + mustSymlink(t, filepath.Join(dir, "real"), filepath.Join(dir, "alias")) + d := DiscoverSkills(dir, DefaultMaxDepth) + if len(d.Dirs) != 1 { + t.Errorf("dirs = %v, want exactly one", d.Dirs) + } + }) + + t.Run("follows link to a skill outside root", func(t *testing.T) { + // A skills directory assembled from links to skill folders kept + // elsewhere is a supported layout. + outside := t.TempDir() + writeSkill(t, filepath.Join(outside, "shared"), "---\nname: shared\n---\n") + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, "team"), 0o755); err != nil { + t.Fatal(err) + } + mustSymlink(t, filepath.Join(outside, "shared"), filepath.Join(dir, "team", "shared")) + d := DiscoverSkills(dir, DefaultMaxDepth) + if want := []string{filepath.Join(dir, "team", "shared")}; !reflect.DeepEqual(d.Dirs, want) { + t.Errorf("dirs = %v, want %v", d.Dirs, want) + } + }) + + t.Run("broken link and link to file are ignored", func(t *testing.T) { + dir := t.TempDir() + writeSkill(t, filepath.Join(dir, "skill"), "---\nname: s\n---\n") + mustSymlink(t, filepath.Join(dir, "missing"), filepath.Join(dir, "dangling")) + mustSymlink(t, filepath.Join(dir, "skill", "SKILL.md"), filepath.Join(dir, "filelink")) + d := DiscoverSkills(dir, DefaultMaxDepth) + if want := []string{filepath.Join(dir, "skill")}; !reflect.DeepEqual(d.Dirs, want) { + t.Errorf("dirs = %v, want %v", d.Dirs, want) + } + }) + + t.Run("linked large tree cannot starve a sibling skill", func(t *testing.T) { + big := t.TempDir() + for i := 0; i < 30; i++ { + if err := os.MkdirAll(filepath.Join(big, fmt.Sprintf("d%02d", i), "x", "y"), 0o755); err != nil { + t.Fatal(err) + } + } + dir := t.TempDir() + // "aaa-link" sorts before "team", so a depth-first scan would spend + // its budget inside the linked tree and report no skills. + mustSymlink(t, big, filepath.Join(dir, "aaa-link")) + writeSkill(t, filepath.Join(dir, "team", "skill"), "---\nname: s\n---\n") + d := discoverSkills(dir, DefaultMaxDepth, 40) + if want := []string{filepath.Join(dir, "team", "skill")}; !reflect.DeepEqual(d.Dirs, want) { + t.Errorf("dirs = %v, want %v", d.Dirs, want) + } + if !d.Truncated { + t.Error("expected Truncated") + } + }) +} From 13cac49a7479ca791e2781174e991d1cf7793cb7 Mon Sep 17 00:00:00 2001 From: dacharyc Date: Thu, 15 Oct 2026 11:00:28 -0400 Subject: [PATCH 2/2] Key visited directories by absolute path during discovery With a relative root, an ordinary child's identity key was built from the root's relative resolved path, while a symlink to an absolute target resolved to an absolute key. The same directory got two keys, so a skill and its alias were both reported. The CLI passes an absolute root and never hit this; library callers of DetectSkills with a relative path did. Make the root absolute before resolving it, and resolve symlinked children through the parent's resolved path, so every key is absolute. Reported paths stay as the caller gave them. Add a regression test with a relative root and an absolute-target alias. --- skillcheck/validator.go | 15 ++++++++++++--- skillcheck/validator_test.go | 15 +++++++++++++++ 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/skillcheck/validator.go b/skillcheck/validator.go index 4224553..447cefa 100644 --- a/skillcheck/validator.go +++ b/skillcheck/validator.go @@ -103,12 +103,20 @@ func discoverSkills(root string, maxDepth, maxDirs int) Discovery { // listed, and the budget is spent on listings, not on skills. type dirItem struct { path string // path as reached from root, reported to the caller - real string // symlink-resolved path, used to visit each directory once + real string // absolute, symlink-resolved path, used to visit each directory once depth int } + // Identity keys must be absolute: a relative root would otherwise give + // relative keys to ordinary children while a link to an absolute target + // resolves to an absolute key, and the same directory would be visited + // twice. Caller-facing paths are left as given. visited := make(map[string]bool) - realRoot, err := filepath.EvalSymlinks(root) + absRoot, err := filepath.Abs(root) + if err != nil { + return Discovery{Mode: types.NoSkill} + } + realRoot, err := filepath.EvalSymlinks(absRoot) if err != nil { return Discovery{Mode: types.NoSkill} } @@ -146,7 +154,8 @@ func discoverSkills(root string, maxDepth, maxDirs int) Discovery { real = filepath.Join(cur.real, name) case entry.Type()&os.ModeSymlink != 0: // Follow the link, but only into a directory that exists. - resolved, err := filepath.EvalSymlinks(sub) + // Resolving through cur.real keeps the key absolute. + resolved, err := filepath.EvalSymlinks(filepath.Join(cur.real, name)) if err != nil { continue } diff --git a/skillcheck/validator_test.go b/skillcheck/validator_test.go index fad32e6..82b52fe 100644 --- a/skillcheck/validator_test.go +++ b/skillcheck/validator_test.go @@ -549,6 +549,21 @@ func TestDiscoverSkills_Symlinks(t *testing.T) { } }) + t.Run("alias is reported once from a relative root", func(t *testing.T) { + // With a relative root, ordinary children resolve to relative paths + // while a link to an absolute target resolves to an absolute one. + // Identity keys must not depend on that, or the alias is counted as + // a second skill. Reported paths stay relative, as given. + base := t.TempDir() + writeSkill(t, filepath.Join(base, "catalog", "real"), "---\nname: s\n---\n") + mustSymlink(t, filepath.Join(base, "catalog", "real"), filepath.Join(base, "catalog", "alias")) + t.Chdir(base) + d := DiscoverSkills("catalog", DefaultMaxDepth) + if want := []string{filepath.Join("catalog", "alias")}; !reflect.DeepEqual(d.Dirs, want) { + t.Errorf("dirs = %v, want %v", d.Dirs, want) + } + }) + t.Run("follows link to a skill outside root", func(t *testing.T) { // A skills directory assembled from links to skill folders kept // elsewhere is a supported layout.