Skip to content

Commit 44868e4

Browse files
committed
fix(skills): tighten review nits from refactor
Two small follow-ups from reviewing the refactor: - Load: add a FilePath tiebreaker to the sort. slices.SortedFunc uses pdqsort which is unstable, so two skills sharing a Name (e.g. a local and a remote source both exposing 'foo') would otherwise have non-deterministic relative ordering across runs. FilePath is unique, so this gives Load a fully deterministic output. - local.go: clarify the symlink behavior of loadSkillsFlat (skips symlinks explicitly) and loadSkillsRecursive (filepath.WalkDir does not follow symlinks; the visited map is defensive against bind mounts and symlinked roots, not symlinks inside the tree). Assisted-By: docker-agent
1 parent 94f3b37 commit 44868e4

2 files changed

Lines changed: 19 additions & 4 deletions

File tree

pkg/skills/local.go

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,9 @@ func loadSkillsFromDir(dir string, recursive bool) []Skill {
134134
return loadSkillsFlat(dir)
135135
}
136136

137-
// loadSkillsFlat loads skills from immediate subdirectories only (Claude format).
137+
// loadSkillsFlat loads skills from immediate subdirectories only (Claude
138+
// format). Symlinks are explicitly skipped here because flat-mode skills are
139+
// expected to live as plain directories under the search root.
138140
func loadSkillsFlat(dir string) []Skill {
139141
entries, err := os.ReadDir(dir)
140142
if err != nil {
@@ -154,8 +156,15 @@ func loadSkillsFlat(dir string) []Skill {
154156
return skills
155157
}
156158

157-
// loadSkillsRecursive walks dir for SKILL.md files (Codex format), tracking
158-
// real directory paths so symlink cycles can't loop forever.
159+
// loadSkillsRecursive walks dir for SKILL.md files (Codex format).
160+
//
161+
// filepath.WalkDir does not follow symlinks inside the tree (it uses
162+
// fs.DirEntry which is Lstat-based), so a symlink-to-directory has
163+
// IsDir() == false and is naturally not entered. The visited map is
164+
// defensive: it catches the (rare) cases where the same real directory
165+
// is reachable via two non-symlink paths — e.g. Linux bind mounts, or
166+
// when the search root itself is a symlink whose target also appears
167+
// as a sibling deeper in the tree.
159168
func loadSkillsRecursive(dir string) []Skill {
160169
visited := make(map[string]bool)
161170
if realDir, err := filepath.EvalSymlinks(dir); err == nil {

pkg/skills/skills.go

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,13 @@ func Load(sources []string) []Skill {
7272
}
7373

7474
return slices.SortedFunc(maps.Values(skillMap), func(a, b Skill) int {
75-
return strings.Compare(a.Name, b.Name)
75+
if c := strings.Compare(a.Name, b.Name); c != 0 {
76+
return c
77+
}
78+
// FilePath is unique per skill, so this gives a fully
79+
// deterministic ordering even when a local and a remote source
80+
// expose a skill with the same name.
81+
return strings.Compare(a.FilePath, b.FilePath)
7682
})
7783
}
7884

0 commit comments

Comments
 (0)