fix(skill-loader): deterministic collision handling for skill names
- Separate directory and file entries, process directories first
- Use Map to deduplicate skills by name (first-wins)
- Directory skills (SKILL.md, {dir}.md) take precedence over file skills (*.md)
- Add test for collision scenario
Addresses Oracle P2 review feedback from PR #1254
This commit is contained in:
parent
dee8cf1720
commit
f79f164cd5
@ -516,5 +516,42 @@ Nested content.
|
|||||||
process.chdir(originalCwd)
|
process.chdir(originalCwd)
|
||||||
}
|
}
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it("prefers directory skill (SKILL.md) over file skill (*.md) on name collision", async () => {
|
||||||
|
// #given - both foo.md file AND foo/SKILL.md directory exist
|
||||||
|
// Directory skill should win (deterministic precedence: SKILL.md > {dir}.md > *.md)
|
||||||
|
const dirSkillDir = join(SKILLS_DIR, "collision-test")
|
||||||
|
mkdirSync(dirSkillDir, { recursive: true })
|
||||||
|
writeFileSync(join(dirSkillDir, "SKILL.md"), `---
|
||||||
|
name: collision-test
|
||||||
|
description: Directory-based skill (should win)
|
||||||
|
---
|
||||||
|
I am the directory skill.
|
||||||
|
`)
|
||||||
|
|
||||||
|
// Also create a file with same base name at parent level
|
||||||
|
writeFileSync(join(SKILLS_DIR, "collision-test.md"), `---
|
||||||
|
name: collision-test
|
||||||
|
description: File-based skill (should lose)
|
||||||
|
---
|
||||||
|
I am the file skill.
|
||||||
|
`)
|
||||||
|
|
||||||
|
// #when
|
||||||
|
const { discoverSkills } = await import("./loader")
|
||||||
|
const originalCwd = process.cwd()
|
||||||
|
process.chdir(TEST_DIR)
|
||||||
|
|
||||||
|
try {
|
||||||
|
const skills = await discoverSkills({ includeClaudeCodePaths: false })
|
||||||
|
|
||||||
|
// #then - only one skill should exist, and it should be the directory-based one
|
||||||
|
const matchingSkills = skills.filter(s => s.name === "collision-test")
|
||||||
|
expect(matchingSkills).toHaveLength(1)
|
||||||
|
expect(matchingSkills[0]?.definition.description).toContain("Directory-based skill")
|
||||||
|
} finally {
|
||||||
|
process.chdir(originalCwd)
|
||||||
|
}
|
||||||
|
})
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|||||||
@ -140,53 +140,59 @@ async function loadSkillsFromDir(
|
|||||||
maxDepth: number = 2
|
maxDepth: number = 2
|
||||||
): Promise<LoadedSkill[]> {
|
): Promise<LoadedSkill[]> {
|
||||||
const entries = await fs.readdir(skillsDir, { withFileTypes: true }).catch(() => [])
|
const entries = await fs.readdir(skillsDir, { withFileTypes: true }).catch(() => [])
|
||||||
const skills: LoadedSkill[] = []
|
const skillMap = new Map<string, LoadedSkill>()
|
||||||
|
|
||||||
for (const entry of entries) {
|
const directories = entries.filter(e => !e.name.startsWith(".") && (e.isDirectory() || e.isSymbolicLink()))
|
||||||
if (entry.name.startsWith(".")) continue
|
const files = entries.filter(e => !e.name.startsWith(".") && !e.isDirectory() && !e.isSymbolicLink() && isMarkdownFile(e))
|
||||||
|
|
||||||
|
for (const entry of directories) {
|
||||||
const entryPath = join(skillsDir, entry.name)
|
const entryPath = join(skillsDir, entry.name)
|
||||||
|
const resolvedPath = await resolveSymlinkAsync(entryPath)
|
||||||
|
const dirName = entry.name
|
||||||
|
|
||||||
if (entry.isDirectory() || entry.isSymbolicLink()) {
|
const skillMdPath = join(resolvedPath, "SKILL.md")
|
||||||
const resolvedPath = await resolveSymlinkAsync(entryPath)
|
try {
|
||||||
const dirName = entry.name
|
await fs.access(skillMdPath)
|
||||||
|
const skill = await loadSkillFromPath(skillMdPath, resolvedPath, dirName, scope, namePrefix)
|
||||||
const skillMdPath = join(resolvedPath, "SKILL.md")
|
if (skill && !skillMap.has(skill.name)) {
|
||||||
try {
|
skillMap.set(skill.name, skill)
|
||||||
await fs.access(skillMdPath)
|
|
||||||
const skill = await loadSkillFromPath(skillMdPath, resolvedPath, dirName, scope, namePrefix)
|
|
||||||
if (skill) skills.push(skill)
|
|
||||||
continue
|
|
||||||
} catch {
|
|
||||||
}
|
}
|
||||||
|
|
||||||
const namedSkillMdPath = join(resolvedPath, `${dirName}.md`)
|
|
||||||
try {
|
|
||||||
await fs.access(namedSkillMdPath)
|
|
||||||
const skill = await loadSkillFromPath(namedSkillMdPath, resolvedPath, dirName, scope, namePrefix)
|
|
||||||
if (skill) skills.push(skill)
|
|
||||||
continue
|
|
||||||
} catch {
|
|
||||||
}
|
|
||||||
|
|
||||||
// Recurse into subdirectories if no skill found and within depth limit
|
|
||||||
if (depth < maxDepth) {
|
|
||||||
const newPrefix = namePrefix ? `${namePrefix}/${dirName}` : dirName
|
|
||||||
const nestedSkills = await loadSkillsFromDir(resolvedPath, scope, newPrefix, depth + 1, maxDepth)
|
|
||||||
skills.push(...nestedSkills)
|
|
||||||
}
|
|
||||||
|
|
||||||
continue
|
continue
|
||||||
|
} catch {
|
||||||
}
|
}
|
||||||
|
|
||||||
if (isMarkdownFile(entry)) {
|
const namedSkillMdPath = join(resolvedPath, `${dirName}.md`)
|
||||||
const baseName = basename(entry.name, ".md")
|
try {
|
||||||
const skill = await loadSkillFromPath(entryPath, skillsDir, baseName, scope, namePrefix)
|
await fs.access(namedSkillMdPath)
|
||||||
if (skill) skills.push(skill)
|
const skill = await loadSkillFromPath(namedSkillMdPath, resolvedPath, dirName, scope, namePrefix)
|
||||||
|
if (skill && !skillMap.has(skill.name)) {
|
||||||
|
skillMap.set(skill.name, skill)
|
||||||
|
}
|
||||||
|
continue
|
||||||
|
} catch {
|
||||||
|
}
|
||||||
|
|
||||||
|
if (depth < maxDepth) {
|
||||||
|
const newPrefix = namePrefix ? `${namePrefix}/${dirName}` : dirName
|
||||||
|
const nestedSkills = await loadSkillsFromDir(resolvedPath, scope, newPrefix, depth + 1, maxDepth)
|
||||||
|
for (const nestedSkill of nestedSkills) {
|
||||||
|
if (!skillMap.has(nestedSkill.name)) {
|
||||||
|
skillMap.set(nestedSkill.name, nestedSkill)
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
return skills
|
for (const entry of files) {
|
||||||
|
const entryPath = join(skillsDir, entry.name)
|
||||||
|
const baseName = basename(entry.name, ".md")
|
||||||
|
const skill = await loadSkillFromPath(entryPath, skillsDir, baseName, scope, namePrefix)
|
||||||
|
if (skill && !skillMap.has(skill.name)) {
|
||||||
|
skillMap.set(skill.name, skill)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
return Array.from(skillMap.values())
|
||||||
}
|
}
|
||||||
|
|
||||||
function skillsToRecord(skills: LoadedSkill[]): Record<string, CommandDefinition> {
|
function skillsToRecord(skills: LoadedSkill[]): Record<string, CommandDefinition> {
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user