Preserve real skill paths when following directory links
- commit
- e0ea86f4ad6ad3576e2babd37a1707ad571f5f9b
- author
- patriceckhart · · 2026-09-28 04:27 UTC
- parent
- 29f5fcb
- signature
- unsigned
3 files · +56 -15
M docs/skills.md +7 -7
| @@ -75,13 +75,13 @@ compatibility sources. Duplicate names from different files keep the | |||
| 75 | 75 | higher-precedence file and report a diagnostic. Rediscovering the same file | |
| 76 | 76 | under the same skill name (including through a symlink) is silently ignored. | |
| 77 | 77 | Skill directories may themselves be symlinks, so a checkout can be kept in one | |
| 78 | - | place and linked into a skills directory. Link cycles are ignored, a directory | |
| 79 | - | reached twice through links is scanned once, and a link that points at nothing | |
| 80 | - | is skipped. | |
| 81 | - | This includes overlapping project and global locations when running from your | |
| 82 | - | home directory. `--no-ext` omits implicit extension bundles; | |
| 83 | - | `--no-ext --ext PATH` loads only the explicitly named bundle. `--no-skill` | |
| 84 | - | disables all skills, including extension and built-in skills. | |
| 78 | + | place and linked into a skills directory. Real directories are scanned before | |
| 79 | + | links, so an alias cannot rename an existing unnamed skill. Link cycles are | |
| 80 | + | ignored, a directory reached twice through links is scanned once, and a link | |
| 81 | + | that points at nothing is skipped. This includes overlapping project and global | |
| 82 | + | locations when running from your home directory. `--no-ext` omits implicit | |
| 83 | + | extension bundles. `--no-ext --ext PATH` loads only the explicitly named bundle. | |
| 84 | + | `--no-skill` disables all skills, including extension and built-in skills. | |
| 85 | 85 | ||
| 86 | 86 | When `XDG_STATE_HOME` is set on any platform, `$ZOT_HOME` defaults to | |
| 87 | 87 | `$XDG_STATE_HOME/zot`. Otherwise it defaults to `~/Library/Application Support/zot/` | |
| @@ -75,13 +75,13 @@ compatibility sources. Duplicate names from different files keep the | |||||
| 75 | higher-precedence file and report a diagnostic. Rediscovering the same file | 75 | higher-precedence file and report a diagnostic. Rediscovering the same file | ||
| 76 | under the same skill name (including through a symlink) is silently ignored. | 76 | under the same skill name (including through a symlink) is silently ignored. | ||
| 77 | Skill directories may themselves be symlinks, so a checkout can be kept in one | 77 | Skill directories may themselves be symlinks, so a checkout can be kept in one | ||
| 78 | - | place and linked into a skills directory. Link cycles are ignored, a directory | 78 | + | place and linked into a skills directory. Real directories are scanned before |
| 79 | - | reached twice through links is scanned once, and a link that points at nothing | 79 | + | links, so an alias cannot rename an existing unnamed skill. Link cycles are |
| 80 | - | is skipped. | 80 | + | ignored, a directory reached twice through links is scanned once, and a link |
| 81 | - | This includes overlapping project and global locations when running from your | 81 | + | that points at nothing is skipped. This includes overlapping project and global |
| 82 | - | home directory. `--no-ext` omits implicit extension bundles; | 82 | + | locations when running from your home directory. `--no-ext` omits implicit |
| 83 | - | `--no-ext --ext PATH` loads only the explicitly named bundle. `--no-skill` | 83 | + | extension bundles. `--no-ext --ext PATH` loads only the explicitly named bundle. |
| 84 | - | disables all skills, including extension and built-in skills. | 84 | + | `--no-skill` disables all skills, including extension and built-in skills. |
| 85 | 85 | ||||
| 86 | When `XDG_STATE_HOME` is set on any platform, `$ZOT_HOME` defaults to | 86 | When `XDG_STATE_HOME` is set on any platform, `$ZOT_HOME` defaults to | ||
| 87 | `$XDG_STATE_HOME/zot`. Otherwise it defaults to `~/Library/Application Support/zot/` | 87 | `$XDG_STATE_HOME/zot`. Otherwise it defaults to `~/Library/Application Support/zot/` | ||
M packages/agent/skills/skills.go +12 -6
| @@ -35,6 +35,7 @@ import ( | |||
| 35 | 35 | "path/filepath" | |
| 36 | 36 | "sort" | |
| 37 | 37 | "strings" | |
| 38 | + | "syscall" | |
| 38 | 39 | ) | |
| 39 | 40 | ||
| 40 | 41 | // Skill is one discovered SKILL.md file. | |
| @@ -215,9 +216,11 @@ func scanSource(source Source) ([]*Skill, []error) { | |||
| 215 | 216 | } | |
| 216 | 217 | // filepath.WalkDir does not descend into symlinked directories, which | |
| 217 | 218 | // would silently drop a skill directory that links into a shared | |
| 218 | - | // checkout. The walk below follows those links and records every | |
| 219 | - | // resolved directory so a link cycle cannot recurse forever. | |
| 219 | + | // checkout. Walk real directories before following links so an alias | |
| 220 | + | // cannot rename an existing unnamed skill. Record resolved directories | |
| 221 | + | // so a link cycle cannot recurse forever. | |
| 220 | 222 | visited := map[string]bool{} | |
| 223 | + | var linkedDirs []string | |
| 221 | 224 | var walk func(dir string) | |
| 222 | 225 | walk = func(dir string) { | |
| 223 | 226 | resolved, err := filepath.EvalSymlinks(dir) | |
| @@ -243,15 +246,15 @@ func scanSource(source Source) ([]*Skill, []error) { | |||
| 243 | 246 | if d.Type()&fs.ModeSymlink != 0 { | |
| 244 | 247 | info, statErr := os.Stat(path) // resolves the link | |
| 245 | 248 | if statErr != nil { | |
| 246 | - | // A dangling link is not a skill. Anything else | |
| 247 | - | // (permissions, I/O) is worth reporting. | |
| 248 | - | if !errors.Is(statErr, fs.ErrNotExist) { | |
| 249 | + | // Dangling and cyclic links are not skills. Anything | |
| 250 | + | // else (permissions, I/O) is worth reporting. | |
| 251 | + | if !errors.Is(statErr, fs.ErrNotExist) && !errors.Is(statErr, syscall.ELOOP) { | |
| 249 | 252 | errs = append(errs, fmt.Errorf("%s: %w", path, statErr)) | |
| 250 | 253 | } | |
| 251 | 254 | continue | |
| 252 | 255 | } | |
| 253 | 256 | if info.IsDir() { | |
| 254 | - | walk(path) | |
| 257 | + | linkedDirs = append(linkedDirs, path) | |
| 255 | 258 | continue | |
| 256 | 259 | } | |
| 257 | 260 | } | |
| @@ -278,6 +281,9 @@ func scanSource(source Source) ([]*Skill, []error) { | |||
| 278 | 281 | } | |
| 279 | 282 | } | |
| 280 | 283 | walk(root) | |
| 284 | + | for i := 0; i < len(linkedDirs); i++ { | |
| 285 | + | walk(linkedDirs[i]) | |
| 286 | + | } | |
| 281 | 287 | return out, errs | |
| 282 | 288 | } | |
| 283 | 289 | ||
| @@ -35,6 +35,7 @@ import ( | |||||
| 35 | "path/filepath" | 35 | "path/filepath" | ||
| 36 | "sort" | 36 | "sort" | ||
| 37 | "strings" | 37 | "strings" | ||
| 38 | + | "syscall" | |||
| 38 | ) | 39 | ) | ||
| 39 | 40 | ||||
| 40 | // Skill is one discovered SKILL.md file. | 41 | // Skill is one discovered SKILL.md file. | ||
| @@ -215,9 +216,11 @@ func scanSource(source Source) ([]*Skill, []error) { | |||||
| 215 | } | 216 | } | ||
| 216 | // filepath.WalkDir does not descend into symlinked directories, which | 217 | // filepath.WalkDir does not descend into symlinked directories, which | ||
| 217 | // would silently drop a skill directory that links into a shared | 218 | // would silently drop a skill directory that links into a shared | ||
| 218 | - | // checkout. The walk below follows those links and records every | 219 | + | // checkout. Walk real directories before following links so an alias |
| 219 | - | // resolved directory so a link cycle cannot recurse forever. | 220 | + | // cannot rename an existing unnamed skill. Record resolved directories |
| 221 | + | // so a link cycle cannot recurse forever. | |||
| 220 | visited := map[string]bool{} | 222 | visited := map[string]bool{} | ||
| 223 | + | var linkedDirs []string | |||
| 221 | var walk func(dir string) | 224 | var walk func(dir string) | ||
| 222 | walk = func(dir string) { | 225 | walk = func(dir string) { | ||
| 223 | resolved, err := filepath.EvalSymlinks(dir) | 226 | resolved, err := filepath.EvalSymlinks(dir) | ||
| @@ -243,15 +246,15 @@ func scanSource(source Source) ([]*Skill, []error) { | |||||
| 243 | if d.Type()&fs.ModeSymlink != 0 { | 246 | if d.Type()&fs.ModeSymlink != 0 { | ||
| 244 | info, statErr := os.Stat(path) // resolves the link | 247 | info, statErr := os.Stat(path) // resolves the link | ||
| 245 | if statErr != nil { | 248 | if statErr != nil { | ||
| 246 | - | // A dangling link is not a skill. Anything else | 249 | + | // Dangling and cyclic links are not skills. Anything |
| 247 | - | // (permissions, I/O) is worth reporting. | 250 | + | // else (permissions, I/O) is worth reporting. |
| 248 | - | if !errors.Is(statErr, fs.ErrNotExist) { | 251 | + | if !errors.Is(statErr, fs.ErrNotExist) && !errors.Is(statErr, syscall.ELOOP) { |
| 249 | errs = append(errs, fmt.Errorf("%s: %w", path, statErr)) | 252 | errs = append(errs, fmt.Errorf("%s: %w", path, statErr)) | ||
| 250 | } | 253 | } | ||
| 251 | continue | 254 | continue | ||
| 252 | } | 255 | } | ||
| 253 | if info.IsDir() { | 256 | if info.IsDir() { | ||
| 254 | - | walk(path) | 257 | + | linkedDirs = append(linkedDirs, path) |
| 255 | continue | 258 | continue | ||
| 256 | } | 259 | } | ||
| 257 | } | 260 | } | ||
| @@ -278,6 +281,9 @@ func scanSource(source Source) ([]*Skill, []error) { | |||||
| 278 | } | 281 | } | ||
| 279 | } | 282 | } | ||
| 280 | walk(root) | 283 | walk(root) | ||
| 284 | + | for i := 0; i < len(linkedDirs); i++ { | |||
| 285 | + | walk(linkedDirs[i]) | |||
| 286 | + | } | |||
| 281 | return out, errs | 287 | return out, errs | ||
| 282 | } | 288 | } | ||
| 283 | 289 | ||||
M packages/agent/skills/skills_test.go +37 -2
| @@ -160,7 +160,7 @@ func TestDiscoverSourcesFollowsSymlinkedSkillDir(t *testing.T) { | |||
| 160 | 160 | if got[0].Name != "tdd" || got[0].Body != "shared body" || got[0].Source != "global" { | |
| 161 | 161 | t.Errorf("skill = %#v", got[0]) | |
| 162 | 162 | } | |
| 163 | - | if want := filepath.Join(root, "tdd", "SKILL.md"); got[0].Path != want { | |
| 163 | + | if want := filepath.Join(mustEvalSymlinks(t, root), "tdd", "SKILL.md"); got[0].Path != want { | |
| 164 | 164 | t.Errorf("path = %q, want the link location %q", got[0].Path, want) | |
| 165 | 165 | } | |
| 166 | 166 | } | |
| @@ -215,11 +215,46 @@ func TestDiscoverSourcesWalksLinkedDirOnce(t *testing.T) { | |||
| 215 | 215 | if len(errs) != 0 || len(got) != 1 { | |
| 216 | 216 | t.Fatalf("skills=%#v errs=%v", got, errs) | |
| 217 | 217 | } | |
| 218 | - | if want := filepath.Join(root, "alias", "SKILL.md"); got[0].Path != want { | |
| 218 | + | if want := filepath.Join(mustEvalSymlinks(t, root), "shared", "SKILL.md"); got[0].Path != want { | |
| 219 | 219 | t.Errorf("path = %q, want %q", got[0].Path, want) | |
| 220 | 220 | } | |
| 221 | 221 | } | |
| 222 | 222 | ||
| 223 | + | func TestDiscoverSourcesKeepsUnnamedRealSkillWhenAliasSortsFirst(t *testing.T) { | |
| 224 | + | root := t.TempDir() | |
| 225 | + | real := filepath.Join(root, "z", "review") | |
| 226 | + | writeSkill(t, real, "", "body") | |
| 227 | + | linkDir(t, real, filepath.Join(root, "alias")) | |
| 228 | + | ||
| 229 | + | got, errs := DiscoverSources([]Source{{Root: root}}, false) | |
| 230 | + | if len(errs) != 0 || len(got) != 1 { | |
| 231 | + | t.Fatalf("skills=%#v errs=%v", got, errs) | |
| 232 | + | } | |
| 233 | + | if got[0].Name != "z-review" || got[0].Path != filepath.Join(mustEvalSymlinks(t, root), "z", "review", "SKILL.md") { | |
| 234 | + | t.Errorf("skill = %#v, want the real directory name and path", got[0]) | |
| 235 | + | } | |
| 236 | + | } | |
| 237 | + | ||
| 238 | + | func TestDiscoverSourcesIgnoresCyclicSymlink(t *testing.T) { | |
| 239 | + | root := t.TempDir() | |
| 240 | + | writeSkill(t, filepath.Join(root, "review"), "name: review", "body") | |
| 241 | + | linkDir(t, filepath.Join(root, "loop"), filepath.Join(root, "loop")) | |
| 242 | + | ||
| 243 | + | got, errs := DiscoverSources([]Source{{Root: root}}, false) | |
| 244 | + | if len(errs) != 0 || len(got) != 1 || got[0].Name != "review" { | |
| 245 | + | t.Fatalf("skills=%#v errs=%v", got, errs) | |
| 246 | + | } | |
| 247 | + | } | |
| 248 | + | ||
| 249 | + | func mustEvalSymlinks(t *testing.T, path string) string { | |
| 250 | + | t.Helper() | |
| 251 | + | resolved, err := filepath.EvalSymlinks(path) | |
| 252 | + | if err != nil { | |
| 253 | + | t.Fatal(err) | |
| 254 | + | } | |
| 255 | + | return resolved | |
| 256 | + | } | |
| 257 | + | ||
| 223 | 258 | func writeSkill(t *testing.T, dir, front, body string) { | |
| 224 | 259 | t.Helper() | |
| 225 | 260 | if err := os.MkdirAll(dir, 0o755); err != nil { | |
| @@ -160,7 +160,7 @@ func TestDiscoverSourcesFollowsSymlinkedSkillDir(t *testing.T) { | |||||
| 160 | if got[0].Name != "tdd" || got[0].Body != "shared body" || got[0].Source != "global" { | 160 | if got[0].Name != "tdd" || got[0].Body != "shared body" || got[0].Source != "global" { | ||
| 161 | t.Errorf("skill = %#v", got[0]) | 161 | t.Errorf("skill = %#v", got[0]) | ||
| 162 | } | 162 | } | ||
| 163 | - | if want := filepath.Join(root, "tdd", "SKILL.md"); got[0].Path != want { | 163 | + | if want := filepath.Join(mustEvalSymlinks(t, root), "tdd", "SKILL.md"); got[0].Path != want { |
| 164 | t.Errorf("path = %q, want the link location %q", got[0].Path, want) | 164 | t.Errorf("path = %q, want the link location %q", got[0].Path, want) | ||
| 165 | } | 165 | } | ||
| 166 | } | 166 | } | ||
| @@ -215,11 +215,46 @@ func TestDiscoverSourcesWalksLinkedDirOnce(t *testing.T) { | |||||
| 215 | if len(errs) != 0 || len(got) != 1 { | 215 | if len(errs) != 0 || len(got) != 1 { | ||
| 216 | t.Fatalf("skills=%#v errs=%v", got, errs) | 216 | t.Fatalf("skills=%#v errs=%v", got, errs) | ||
| 217 | } | 217 | } | ||
| 218 | - | if want := filepath.Join(root, "alias", "SKILL.md"); got[0].Path != want { | 218 | + | if want := filepath.Join(mustEvalSymlinks(t, root), "shared", "SKILL.md"); got[0].Path != want { |
| 219 | t.Errorf("path = %q, want %q", got[0].Path, want) | 219 | t.Errorf("path = %q, want %q", got[0].Path, want) | ||
| 220 | } | 220 | } | ||
| 221 | } | 221 | } | ||
| 222 | 222 | ||||
| 223 | + | func TestDiscoverSourcesKeepsUnnamedRealSkillWhenAliasSortsFirst(t *testing.T) { | |||
| 224 | + | root := t.TempDir() | |||
| 225 | + | real := filepath.Join(root, "z", "review") | |||
| 226 | + | writeSkill(t, real, "", "body") | |||
| 227 | + | linkDir(t, real, filepath.Join(root, "alias")) | |||
| 228 | + | ||||
| 229 | + | got, errs := DiscoverSources([]Source{{Root: root}}, false) | |||
| 230 | + | if len(errs) != 0 || len(got) != 1 { | |||
| 231 | + | t.Fatalf("skills=%#v errs=%v", got, errs) | |||
| 232 | + | } | |||
| 233 | + | if got[0].Name != "z-review" || got[0].Path != filepath.Join(mustEvalSymlinks(t, root), "z", "review", "SKILL.md") { | |||
| 234 | + | t.Errorf("skill = %#v, want the real directory name and path", got[0]) | |||
| 235 | + | } | |||
| 236 | + | } | |||
| 237 | + | ||||
| 238 | + | func TestDiscoverSourcesIgnoresCyclicSymlink(t *testing.T) { | |||
| 239 | + | root := t.TempDir() | |||
| 240 | + | writeSkill(t, filepath.Join(root, "review"), "name: review", "body") | |||
| 241 | + | linkDir(t, filepath.Join(root, "loop"), filepath.Join(root, "loop")) | |||
| 242 | + | ||||
| 243 | + | got, errs := DiscoverSources([]Source{{Root: root}}, false) | |||
| 244 | + | if len(errs) != 0 || len(got) != 1 || got[0].Name != "review" { | |||
| 245 | + | t.Fatalf("skills=%#v errs=%v", got, errs) | |||
| 246 | + | } | |||
| 247 | + | } | |||
| 248 | + | ||||
| 249 | + | func mustEvalSymlinks(t *testing.T, path string) string { | |||
| 250 | + | t.Helper() | |||
| 251 | + | resolved, err := filepath.EvalSymlinks(path) | |||
| 252 | + | if err != nil { | |||
| 253 | + | t.Fatal(err) | |||
| 254 | + | } | |||
| 255 | + | return resolved | |||
| 256 | + | } | |||
| 257 | + | ||||
| 223 | func writeSkill(t *testing.T, dir, front, body string) { | 258 | func writeSkill(t *testing.T, dir, front, body string) { | ||
| 224 | t.Helper() | 259 | t.Helper() | ||
| 225 | if err := os.MkdirAll(dir, 0o755); err != nil { | 260 | if err := os.MkdirAll(dir, 0o755); err != nil { | ||