From b948efc7e487b53340cad65d49fde0fae04fe585 Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Fri, 27 Feb 2026 20:46:42 +0200 Subject: [PATCH] Implement symlinks for skills --- src/services/skills/SkillsManager.ts | 257 +++++- .../skills/__tests__/SkillsManager.spec.ts | 851 ++++++++++++++++++ 2 files changed, 1085 insertions(+), 23 deletions(-) diff --git a/src/services/skills/SkillsManager.ts b/src/services/skills/SkillsManager.ts index 0959b977c9..5f07106539 100644 --- a/src/services/skills/SkillsManager.ts +++ b/src/services/skills/SkillsManager.ts @@ -20,6 +20,13 @@ export type { SkillMetadata, SkillContent } export class SkillsManager { private skills: Map = new Map() + /** + * Configured top-level skills root (e.g. ~/.roo/skills or /.agents/skills-code) + * each discovered skill key was loaded from. Discovered skill paths are + * realpath-resolved and may live outside the configured root when reached + * through a symlinked container, so the root is tracked explicitly. + */ + private skillRoots: Map = new Map() private providerRef: WeakRef private disposables: vscode.Disposable[] = [] private isDisposed = false @@ -42,20 +49,66 @@ export class SkillsManager { */ async discoverSkills(): Promise { this.skills.clear() + this.skillRoots.clear() const skillsDirs = await this.getSkillsDirectories() for (const { dir, source, mode } of skillsDirs) { - await this.scanSkillsDirectory(dir, source, mode) + // Use a fresh visited set and claim map for each top-level scan so + // traversal is deduplicated within a single root without sharing + // state across different source or mode roots. Later roots still + // override earlier ones (e.g. .roo over .agents) via Map.set. + await this.scanSkillsDirectory(dir, source, mode, 0, new Set(), new Map(), dir) } } + /** + * Maximum depth for recursive scanning of skill container directories. + * Prevents infinite loops from circular symlinks. + */ + private static readonly MAX_SCAN_DEPTH = 5 + /** * Scan a skills directory for skill subdirectories. - * Handles two symlink cases: + * Handles symlink cases: * 1. The skills directory itself is a symlink (resolved by directoryExists using realpath) * 2. Individual skill subdirectories are symlinks + * 3. Symlinked container directories that contain skill subdirectories (e.g., ln -s ../../repo/skills .roo/skills/) + * + * A directory that contains a SKILL.md is treated as a skill. A directory + * without one is treated as a container and scanned recursively (up to + * {@link MAX_SCAN_DEPTH}) so skills nested inside a symlinked container are + * discovered. + * + * Symlinks are followed even when they point outside the configured skills + * root. This is not a security concern: the user controls these directories + * and could place skill files there directly, so a symlink is just a + * convenient way to share a skills repo (the point of issue #1842). + * + * Within a single root, skills sharing the same identity (name, source and + * mode key) are resolved deterministically rather than by scan order: the + * shallowest skill wins (so a direct-root skill beats one nested inside a + * container), and ties at the same depth are broken by sorted entry name + * (first wins). + * + * @param visited - Real paths already scanned in this run, used to avoid + * redundant work and infinite loops from circular symlinks. + * @param claimedDepths - Skill keys discovered in this root, mapped to the + * depth at which they were found. + * @param rootDir - The configured top-level skills root this scan started from. */ - private async scanSkillsDirectory(dirPath: string, source: "global" | "project", mode?: string): Promise { + private async scanSkillsDirectory( + dirPath: string, + source: "global" | "project", + mode: string | undefined, + depth: number = 0, + visited: Set = new Set(), + claimedDepths: Map = new Map(), + rootDir: string = dirPath, + ): Promise { + if (depth > SkillsManager.MAX_SCAN_DEPTH) { + return + } + if (!(await directoryExists(dirPath))) { return } @@ -64,8 +117,16 @@ export class SkillsManager { // Get the real path (resolves if dirPath is a symlink) const realDirPath = await fs.realpath(dirPath) - // Read directory entries - const entries = await fs.readdir(realDirPath) + // Skip directories we've already visited (by real path) to avoid + // redundant scanning and infinite loops from circular symlinks. + if (visited.has(realDirPath)) { + return + } + visited.add(realDirPath) + + // Read directory entries, sorted so same-depth collisions resolve + // deterministically regardless of filesystem ordering. + const entries = [...(await fs.readdir(realDirPath))].sort() for (const entryName of entries) { const entryPath = path.join(realDirPath, entryName) @@ -74,8 +135,25 @@ export class SkillsManager { const stats = await fs.stat(entryPath).catch(() => null) if (!stats?.isDirectory()) continue - // Load skill metadata - the skill name comes from the entry name (symlink name if symlinked) - await this.loadSkillMetadata(entryPath, source, mode, entryName) + // Check if this directory contains a SKILL.md (i.e., it's a skill directory) + const skillMdPath = path.join(entryPath, "SKILL.md") + if (await fileExists(skillMdPath)) { + // Load skill metadata - the skill name comes from the entry name (symlink name if symlinked) + await this.loadSkillMetadata(entryPath, source, mode, entryName, depth, claimedDepths, rootDir) + } else { + // No SKILL.md found - this might be a container directory (e.g., a symlinked repo of skills) + // Recursively scan it for skill subdirectories, sharing the same + // visited set so traversal stays deduplicated and bounded. + await this.scanSkillsDirectory( + entryPath, + source, + mode, + depth + 1, + visited, + claimedDepths, + rootDir, + ) + } } } catch { // Directory doesn't exist or can't be read - this is fine @@ -88,12 +166,18 @@ export class SkillsManager { * @param source - Whether this is a global or project skill * @param mode - The mode this skill is specific to (undefined for generic skills) * @param skillName - The skill name (from symlink name if symlinked, otherwise from directory name) + * @param depth - Depth within the current root at which the skill was found + * @param claimedDepths - Skill keys already discovered in the current root and their depths + * @param rootDir - The configured top-level skills root the skill was discovered under */ private async loadSkillMetadata( - skillDir: string, - source: "global" | "project", - mode?: string, - skillName?: string, + skillDir: string, + source: "global" | "project", + mode: string | undefined, + skillName?: string, + depth: number = 0, + claimedDepths?: Map, + rootDir?: string, ): Promise { const skillMdPath = path.join(skillDir, "SKILL.md") if (!(await fileExists(skillMdPath))) return @@ -162,6 +246,24 @@ export class SkillsManager { const primaryMode = modeSlugs?.[0] const skillKey = this.getSkillKey(effectiveSkillName, source, primaryMode) + // Within the same root, a skill already found at the same or a + // shallower depth takes priority (direct-root beats nested). + const claimedDepth = claimedDepths?.get(skillKey) + if (claimedDepth !== undefined) { + const existingPath = this.skills.get(skillKey)?.path ?? "another skill" + if (claimedDepth <= depth) { + console.warn( + `Skill "${effectiveSkillName}" at ${skillMdPath} is shadowed by ${existingPath} with the same identity`, + ) + return + } + console.warn( + `Skill "${effectiveSkillName}" at ${existingPath} is shadowed by ${skillMdPath} with the same identity`, + ) + } + claimedDepths?.set(skillKey, depth) + + this.skillRoots.set(skillKey, rootDir ?? path.dirname(skillDir)) this.skills.set(skillKey, { name: effectiveSkillName, description, @@ -380,11 +482,26 @@ export class SkillsManager { const skillDir = path.join(skillsDir, name) const skillMdPath = path.join(skillDir, "SKILL.md") - // Check if skill already exists + // Check if skill already exists on disk at the direct location if (await fileExists(skillMdPath)) { throw new Error(t("skills:errors.already_exists", { name, path: skillMdPath })) } + // Check if a skill with the same identity was already discovered elsewhere + // within the destination .roo root (e.g., nested inside a symlinked + // container directory). Creating another skill with the same + // name/source/mode key there would let scan order decide which skill + // remains available, so reject the collision explicitly. Duplicates from + // the lower-priority .agents roots are allowed: the new .roo skill + // deterministically overrides them. + const primaryMode = modeSlugs?.[0] + const skillKey = this.getSkillKey(name, source, primaryMode) + const existingSkill = this.skills.get(skillKey) + const existingRoot = this.skillRoots.get(skillKey) + if (existingSkill && existingRoot && this.isPathWithin(existingRoot, baseDir)) { + throw new Error(t("skills:errors.already_exists", { name, path: existingSkill.path })) + } + // Create the skill directory await fs.mkdir(skillDir, { recursive: true }) @@ -484,10 +601,13 @@ Add your skill instructions here. baseDir = path.join(provider.cwd, ".roo") } - // Determine source and destination directories - const sourceDirName = currentMode ? `skills-${currentMode}` : "skills" + // Determine source and destination directories. + // Derive the source directory from the discovered skill's path rather than + // rebuilding it from the name, so nested skills (e.g., discovered inside a + // symlinked container like skills/repo/my-skill) move from their actual + // on-disk location instead of a non-existent skills/my-skill path. const destDirName = newMode ? `skills-${newMode}` : "skills" - const sourceDir = path.join(baseDir, sourceDirName, name) + const sourceDir = path.dirname(skill.path) const destSkillsDir = path.join(baseDir, destDirName) const destDir = path.join(destSkillsDir, name) const destSkillMdPath = path.join(destDir, "SKILL.md") @@ -500,15 +620,23 @@ Add your skill instructions here. // Ensure destination skills directory exists await fs.mkdir(destSkillsDir, { recursive: true }) - // Move the skill directory - await fs.rename(sourceDir, destDir) - - // Clean up empty source skills directory - const sourceSkillsDir = path.join(baseDir, sourceDirName) + // Move the skill directory (falls back to copy+remove across filesystems) + await this.moveDirectory(sourceDir, destDir) + + // Clean up the now-empty source parent directory (e.g., the emptied + // skills-{mode} directory). Uses the actual parent of the moved skill so + // nested skills clean up their real container rather than an assumed path, + // but only when that parent lies within the configured source skills root. + // Nested skills discovered through a symlinked container may live outside + // it (e.g., in a shared repo), and those directories must not be removed. + const sourceSkillsDir = path.dirname(sourceDir) + const sourceRoot = path.join(baseDir, currentMode ? `skills-${currentMode}` : "skills") try { - const entries = await fs.readdir(sourceSkillsDir) - if (entries.length === 0) { - await fs.rmdir(sourceSkillsDir) + if (await this.isSafeCleanupTarget(sourceSkillsDir, sourceRoot)) { + const entries = await fs.readdir(sourceSkillsDir) + if (entries.length === 0) { + await fs.rmdir(sourceSkillsDir) + } } } catch { // Ignore errors - directory might not exist or have permission issues @@ -518,6 +646,88 @@ Add your skill instructions here. await this.discoverSkills() } + /** + * Move a directory. Uses fs.rename when possible and falls back to a + * recursive copy followed by removal of the source when the source and + * destination are on different filesystems (EXDEV), which can happen when + * a skill was discovered through a symlinked container on another device. + * + * The fallback copies into a unique staging directory next to `destDir` + * (on the destination filesystem) and then promotes it with a same-device + * rename. Failures only ever clean up the staging directory, so any + * pre-existing `destDir` and its contents are never removed. + */ + private async moveDirectory(sourceDir: string, destDir: string): Promise { + try { + await fs.rename(sourceDir, destDir) + return + } catch (error) { + if ((error as NodeJS.ErrnoException)?.code !== "EXDEV") { + throw error + } + } + + const stagingDir = path.join( + path.dirname(destDir), + `.${path.basename(destDir)}.moving-${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2, 10)}`, + ) + + try { + await fs.cp(sourceDir, stagingDir, { recursive: true, errorOnExist: true, force: false }) + + // Refuse to promote over anything that already exists at the destination. + // The rename itself also cannot overwrite a non-empty directory. + const destExists = await fs.lstat(destDir).then( + () => true, + (error: NodeJS.ErrnoException) => { + if (error?.code === "ENOENT") { + return false + } + throw error + }, + ) + if (destExists) { + throw Object.assign(new Error(`Destination already exists: ${destDir}`), { code: "EEXIST" }) + } + + await fs.rename(stagingDir, destDir) + } catch (copyError) { + // Remove only the staging copy so the source and any existing destination stay untouched. + await fs.rm(stagingDir, { recursive: true, force: true }).catch(() => {}) + throw copyError + } + + await fs.rm(sourceDir, { recursive: true, force: true }) + } + + /** + * Whether `target` is `root` or nested inside it (lexical comparison). + */ + private isPathWithin(target: string, root: string): boolean { + const relative = path.relative(path.resolve(root), path.resolve(target)) + return relative === "" || (!relative.startsWith("..") && !path.isAbsolute(relative)) + } + + /** + * Whether an emptied source parent directory may be removed after a move. + * The directory must resolve within the configured skills root (compared + * both lexically and against the root's real path, since discovered skill + * paths are realpath-resolved). When the root itself is a symlink, its + * target is never removed, to avoid leaving a dangling symlink. + */ + private async isSafeCleanupTarget(dir: string, root: string): Promise { + if (this.isPathWithin(dir, root)) { + return true + } + + const realRoot = await fs.realpath(root).catch(() => undefined) + if (!realRoot || path.resolve(realRoot) === path.resolve(root)) { + return false + } + + return this.isPathWithin(dir, realRoot) && path.resolve(dir) !== path.resolve(realRoot) + } + /** * Update the mode associations for a skill by modifying its SKILL.md frontmatter. * @param name - Skill name @@ -715,5 +925,6 @@ Add your skill instructions here. this.disposables.forEach((d) => d.dispose()) this.disposables = [] this.skills.clear() + this.skillRoots.clear() } } diff --git a/src/services/skills/__tests__/SkillsManager.spec.ts b/src/services/skills/__tests__/SkillsManager.spec.ts index d36582d893..2bce97266b 100644 --- a/src/services/skills/__tests__/SkillsManager.spec.ts +++ b/src/services/skills/__tests__/SkillsManager.spec.ts @@ -14,7 +14,11 @@ const { mockRm, mockRename, mockRmdir, + mockCp, + mockLstat, } = vi.hoisted(() => ({ + mockCp: vi.fn(), + mockLstat: vi.fn(), mockStat: vi.fn(), mockReadFile: vi.fn(), mockReaddir: vi.fn(), @@ -50,6 +54,8 @@ vi.mock("fs/promises", () => ({ rm: mockRm, rename: mockRename, rmdir: mockRmdir, + cp: mockCp, + lstat: mockLstat, }, stat: mockStat, readFile: mockReadFile, @@ -60,6 +66,8 @@ vi.mock("fs/promises", () => ({ rm: mockRm, rename: mockRename, rmdir: mockRmdir, + cp: mockCp, + lstat: mockLstat, })) // Mock os module @@ -616,6 +624,507 @@ Instructions here...` expect(skills[0].source).toBe("global") }) + it("should discover skills from symlinked container directory with multiple skills", async () => { + // Simulates: ln -s ../../REPO/skills .roo/skills/ + // This creates .roo/skills/skills -> ../../REPO/skills + // Inside REPO/skills/ there are skill subdirectories: skill-a/ and skill-b/ + const containerDir = p(globalSkillsDir, "skills") // the symlinked container + const repoSkillsDir = p("/repo", "skills") // the actual target + const skillADir = p(repoSkillsDir, "skill-a") + const skillAMd = p(skillADir, "SKILL.md") + const skillBDir = p(repoSkillsDir, "skill-b") + const skillBMd = p(skillBDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => { + return dir === globalSkillsDir || dir === containerDir + }) + + mockRealpath.mockImplementation(async (pathArg: string) => { + if (pathArg === globalSkillsDir) return globalSkillsDir + if (pathArg === containerDir) return repoSkillsDir + return pathArg + }) + + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) return ["skills"] // the symlinked container entry + if (dir === repoSkillsDir) return ["skill-a", "skill-b"] + return [] + }) + + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === containerDir) return { isDirectory: () => true } + if (pathArg === skillADir) return { isDirectory: () => true } + if (pathArg === skillBDir) return { isDirectory: () => true } + throw new Error("Not found") + }) + + mockFileExists.mockImplementation(async (file: string) => { + return file === skillAMd || file === skillBMd + }) + + mockReadFile.mockImplementation(async (file: string) => { + if (file === skillAMd) { + return `--- +name: skill-a +description: First skill from symlinked repo +--- + +# Skill A` + } + if (file === skillBMd) { + return `--- +name: skill-b +description: Second skill from symlinked repo +--- + +# Skill B` + } + throw new Error("File not found") + }) + + await skillsManager.discoverSkills() + + const skills = skillsManager.getAllSkills() + expect(skills).toHaveLength(2) + const names = skills.map((s) => s.name).sort() + expect(names).toEqual(["skill-a", "skill-b"]) + expect(skills.every((s) => s.source === "global")).toBe(true) + }) + + it("should discover skills from nested container directories", async () => { + // Simulates a deeper nesting: .roo/skills/repo/category/my-skill/SKILL.md + const repoDir = p(globalSkillsDir, "repo") + const categoryDir = p(repoDir, "category") + const skillDir = p(categoryDir, "nested-skill") + const skillMd = p(skillDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => { + return [globalSkillsDir, repoDir, categoryDir].includes(dir) + }) + + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) return ["repo"] + if (dir === repoDir) return ["category"] + if (dir === categoryDir) return ["nested-skill"] + return [] + }) + + mockStat.mockImplementation(async (pathArg: string) => { + if ([repoDir, categoryDir, skillDir].includes(pathArg)) { + return { isDirectory: () => true } + } + throw new Error("Not found") + }) + + mockFileExists.mockImplementation(async (file: string) => { + return file === skillMd + }) + + mockReadFile.mockImplementation(async (file: string) => { + if (file === skillMd) { + return `--- +name: nested-skill +description: A deeply nested skill +--- + +# Nested Skill` + } + throw new Error("File not found") + }) + + await skillsManager.discoverSkills() + + const skills = skillsManager.getAllSkills() + expect(skills).toHaveLength(1) + expect(skills[0].name).toBe("nested-skill") + }) + + it.each([ + ["nested container listed first", ["a-repo", "my-skill"]], + ["direct-root skill listed first", ["my-skill", "z-repo"]], + ])( + "should prefer a direct-root skill over a nested skill with the same identity (%s)", + async (_label, rootEntries) => { + // Layout within one root: + // skills/my-skill/SKILL.md (direct-root, depth 0) + // skills//my-skill/SKILL.md (nested, depth 1) + // Both share name/source/mode key. The direct-root skill must win + // regardless of the order the filesystem returns entries in. + const containerName = rootEntries.find((e) => e !== "my-skill")! + const containerDir = p(globalSkillsDir, containerName) + const directSkillDir = p(globalSkillsDir, "my-skill") + const directSkillMd = p(directSkillDir, "SKILL.md") + const nestedSkillDir = p(containerDir, "my-skill") + const nestedSkillMd = p(nestedSkillDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => { + return dir === globalSkillsDir || dir === containerDir + }) + + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) return rootEntries + if (dir === containerDir) return ["my-skill"] + return [] + }) + + mockStat.mockImplementation(async (pathArg: string) => { + if ([containerDir, directSkillDir, nestedSkillDir].includes(pathArg)) { + return { isDirectory: () => true } + } + throw new Error("Not found") + }) + + mockFileExists.mockImplementation(async (file: string) => { + return file === directSkillMd || file === nestedSkillMd + }) + + mockReadFile.mockImplementation(async (file: string) => { + if (file === directSkillMd || file === nestedSkillMd) { + return `--- +name: my-skill +description: ${file === directSkillMd ? "Direct" : "Nested"} skill +--- + +# My Skill` + } + throw new Error("File not found") + }) + + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + + await skillsManager.discoverSkills() + + const skills = skillsManager.getAllSkills() + expect(skills).toHaveLength(1) + expect(skills[0].path).toBe(directSkillMd) + expect(skills[0].description).toBe("Direct skill") + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("is shadowed by")) + + warnSpy.mockRestore() + }, + ) + + it("should break equal-depth collisions by sorted entry name regardless of readdir order", async () => { + // Layout within one root: + // skills/a-repo/my-skill/SKILL.md (nested, depth 1) + // skills/b-repo/my-skill/SKILL.md (nested, depth 1) + // readdir returns containers in reverse lexical order; the lexically + // first container (a-repo) must still win. + const aRepoDir = p(globalSkillsDir, "a-repo") + const bRepoDir = p(globalSkillsDir, "b-repo") + const aSkillDir = p(aRepoDir, "my-skill") + const bSkillDir = p(bRepoDir, "my-skill") + const aSkillMd = p(aSkillDir, "SKILL.md") + const bSkillMd = p(bSkillDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => { + return [globalSkillsDir, aRepoDir, bRepoDir].includes(dir) + }) + + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) return ["b-repo", "a-repo"] + if (dir === aRepoDir || dir === bRepoDir) return ["my-skill"] + return [] + }) + + mockStat.mockImplementation(async (pathArg: string) => { + if ([aRepoDir, bRepoDir, aSkillDir, bSkillDir].includes(pathArg)) { + return { isDirectory: () => true } + } + throw new Error("Not found") + }) + + mockFileExists.mockImplementation(async (file: string) => { + return file === aSkillMd || file === bSkillMd + }) + + mockReadFile.mockImplementation(async (file: string) => { + if (file === aSkillMd || file === bSkillMd) { + return `--- +name: my-skill +description: ${file === aSkillMd ? "From a-repo" : "From b-repo"} +--- + +# My Skill` + } + throw new Error("File not found") + }) + + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + + await skillsManager.discoverSkills() + + const skills = skillsManager.getAllSkills() + expect(skills).toHaveLength(1) + expect(skills[0].path).toBe(aSkillMd) + expect(skills[0].description).toBe("From a-repo") + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("is shadowed by")) + + warnSpy.mockRestore() + }) + + it("should stop scanning at max depth to prevent circular symlinks", async () => { + // Simulate a directory structure that goes deeper than MAX_SCAN_DEPTH (5) + // Each level is a directory without SKILL.md, forcing recursion + const levels: string[] = [globalSkillsDir] + for (let i = 0; i < 8; i++) { + levels.push(p(levels[levels.length - 1], `level-${i}`)) + } + // Put a skill at the deepest level (beyond max depth) + const deepSkillDir = p(levels[levels.length - 1], "deep-skill") + const deepSkillMd = p(deepSkillDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => { + return levels.includes(dir) + }) + + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + + mockReaddir.mockImplementation(async (dir: string) => { + const idx = levels.indexOf(dir) + if (idx >= 0 && idx < levels.length - 1) { + // Return the next level directory name + const nextLevel = levels[idx + 1] + return [path.basename(nextLevel)] + } + if (dir === levels[levels.length - 1]) { + return ["deep-skill"] + } + return [] + }) + + mockStat.mockImplementation(async (pathArg: string) => { + if (levels.includes(pathArg) || pathArg === deepSkillDir) { + return { isDirectory: () => true } + } + throw new Error("Not found") + }) + + mockFileExists.mockImplementation(async (file: string) => { + return file === deepSkillMd + }) + + mockReadFile.mockImplementation(async (file: string) => { + if (file === deepSkillMd) { + return `--- +name: deep-skill +description: A skill too deep to find +--- + +# Deep Skill` + } + throw new Error("File not found") + }) + + await skillsManager.discoverSkills() + + // The skill should NOT be found because it's beyond max depth + const skills = skillsManager.getAllSkills() + expect(skills).toHaveLength(0) + }) + + it("should discover a skill in a container scanned at the inclusive max depth boundary", async () => { + // The top-level skills directory is scanned at depth 0. Each nested + // container without a SKILL.md increments the depth. A container that + // is scanned at depth === MAX_SCAN_DEPTH (5) should still be scanned + // (the guard only stops when depth > MAX_SCAN_DEPTH), so a skill + // inside it must be discovered. This distinguishes the inclusive + // depth-5 boundary from a `>=` guard that would stop one level early. + // + // Layout (depth at which scanSkillsDirectory runs on each dir): + // globalSkillsDir (0) -> level-0 (1) -> level-1 (2) -> level-2 (3) + // -> level-3 (4) -> level-4 (5) -> boundary-skill/SKILL.md + const containers: string[] = [globalSkillsDir] + for (let i = 0; i < 5; i++) { + containers.push(p(containers[containers.length - 1], `level-${i}`)) + } + // containers[5] (level-4) is the container scanned at depth 5. + const boundarySkillDir = p(containers[containers.length - 1], "boundary-skill") + const boundarySkillMd = p(boundarySkillDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => { + return containers.includes(dir) + }) + + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + + mockReaddir.mockImplementation(async (dir: string) => { + const idx = containers.indexOf(dir) + if (idx >= 0 && idx < containers.length - 1) { + return [path.basename(containers[idx + 1])] + } + if (dir === containers[containers.length - 1]) { + return ["boundary-skill"] + } + return [] + }) + + mockStat.mockImplementation(async (pathArg: string) => { + if (containers.includes(pathArg) || pathArg === boundarySkillDir) { + return { isDirectory: () => true } + } + throw new Error("Not found") + }) + + mockFileExists.mockImplementation(async (file: string) => { + return file === boundarySkillMd + }) + + mockReadFile.mockImplementation(async (file: string) => { + if (file === boundarySkillMd) { + return `--- +name: boundary-skill +description: A skill located exactly at the max scan depth boundary +--- + +# Boundary Skill` + } + throw new Error("File not found") + }) + + await skillsManager.discoverSkills() + + // The skill IS found because its container is scanned at depth === 5, + // which is within the inclusive limit. + const skills = skillsManager.getAllSkills() + expect(skills).toHaveLength(1) + expect(skills[0].name).toBe("boundary-skill") + }) + + it("should not scan the same real directory twice within a single scan", async () => { + // A container holds two entries whose symlinks resolve (via realpath) + // to the same real directory. The scanner should only scan that real + // directory once, so its single skill is discovered exactly once and + // readdir is not invoked repeatedly for the deduplicated real path. + const containerDir = p(globalSkillsDir, "repo-skills") + const linkA = p(containerDir, "link-a") + const linkB = p(containerDir, "link-b") + const sharedRealDir = p(containerDir, "shared-container") + const sharedSkillDir = p(sharedRealDir, "shared-skill") + const sharedSkillMd = p(sharedSkillDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => { + return ( + dir === globalSkillsDir || + dir === containerDir || + dir === linkA || + dir === linkB || + dir === sharedRealDir + ) + }) + + // Both link-a and link-b resolve to the same shared real directory. + mockRealpath.mockImplementation(async (pathArg: string) => { + if (pathArg === linkA || pathArg === linkB) return sharedRealDir + return pathArg + }) + + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) return ["repo-skills"] + if (dir === containerDir) return ["link-a", "link-b"] + if (dir === sharedRealDir) return ["shared-skill"] + return [] + }) + + mockStat.mockImplementation(async (pathArg: string) => { + if ( + pathArg === containerDir || + pathArg === linkA || + pathArg === linkB || + pathArg === sharedSkillDir + ) { + return { isDirectory: () => true } + } + throw new Error("Not found") + }) + + mockFileExists.mockImplementation(async (file: string) => { + return file === sharedSkillMd + }) + + mockReadFile.mockImplementation(async (file: string) => { + if (file === sharedSkillMd) { + return `--- +name: shared-skill +description: A skill reachable through two symlinks to the same directory +--- + +# Shared Skill` + } + throw new Error("File not found") + }) + + await skillsManager.discoverSkills() + + const skills = skillsManager.getAllSkills() + expect(skills).toHaveLength(1) + expect(skills[0].name).toBe("shared-skill") + + // The shared real directory should only be read once despite being + // reachable through two different symlinks. + const sharedReaddirCalls = mockReaddir.mock.calls.filter((call) => call[0] === sharedRealDir) + expect(sharedReaddirCalls).toHaveLength(1) + }) + + it("should handle broken symlinks in container directories gracefully", async () => { + // Simulate a container directory with a broken symlink entry + const containerDir = p(globalSkillsDir, "repo-skills") + const brokenDir = p(containerDir, "broken-link") + const validSkillDir = p(containerDir, "valid-skill") + const validSkillMd = p(validSkillDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => { + return dir === globalSkillsDir || dir === containerDir + }) + + mockRealpath.mockImplementation(async (pathArg: string) => { + if (pathArg === containerDir) return containerDir + return pathArg + }) + + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) return ["repo-skills"] + if (dir === containerDir) return ["broken-link", "valid-skill"] + return [] + }) + + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === p(globalSkillsDir, "repo-skills")) return { isDirectory: () => true } + if (pathArg === brokenDir) throw new Error("ENOENT: no such file or directory") + if (pathArg === validSkillDir) return { isDirectory: () => true } + throw new Error("Not found") + }) + + mockFileExists.mockImplementation(async (file: string) => { + return file === validSkillMd + }) + + mockReadFile.mockImplementation(async (file: string) => { + if (file === validSkillMd) { + return `--- +name: valid-skill +description: A valid skill next to a broken symlink +--- + +# Valid Skill` + } + throw new Error("File not found") + }) + + await skillsManager.discoverSkills() + + // Should still find the valid skill despite the broken symlink + const skills = skillsManager.getAllSkills() + expect(skills).toHaveLength(1) + expect(skills[0].name).toBe("valid-skill") + }) + it("should discover skills from global .agents directory", async () => { const agentSkillDir = p(globalAgentsSkillsDir, "agent-skill") const agentSkillMd = p(agentSkillDir, "SKILL.md") @@ -1297,6 +1806,96 @@ Instructions`) "already exists", ) }) + + it("should reject a name already discovered in a nested container", async () => { + // A skill with the same name was discovered inside a symlinked container + // (e.g., skills/repo/my-skill/SKILL.md). createSkill must reject the + // collision instead of creating a second skill that shares the same + // name/source/mode key, which would let scan order decide the winner. + const containerDir = p(globalSkillsDir, "repo") + const nestedSkillDir = p(containerDir, "my-skill") + const nestedSkillMd = p(nestedSkillDir, "SKILL.md") + // The direct-root location createSkill would write to. + const directSkillMd = p(globalSkillsDir, "my-skill", "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => { + return dir === globalSkillsDir || dir === containerDir + }) + + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) return ["repo"] + if (dir === containerDir) return ["my-skill"] + return [] + }) + + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === containerDir || pathArg === nestedSkillDir) { + return { isDirectory: () => true } + } + throw new Error("Not found") + }) + + mockFileExists.mockImplementation(async (file: string) => { + // The nested skill has a SKILL.md so it is discovered as a skill. + if (file === nestedSkillMd) return true + // The direct-root target does NOT exist on disk, so the on-disk + // check alone would not catch the collision. + if (file === directSkillMd) return false + return false + }) + + mockReadFile.mockResolvedValue(`--- +name: my-skill +description: A nested skill +--- +Instructions`) + + await skillsManager.discoverSkills() + + // Confirm the nested skill was discovered at its container path. + const discovered = skillsManager.getSkill("my-skill", "global") + expect(discovered).toBeDefined() + expect(discovered?.path).toBe(nestedSkillMd) + + await expect(skillsManager.createSkill("my-skill", "global", "Description")).rejects.toThrow( + "already exists", + ) + + // The skill was never written because the collision was rejected. + expect(mockWriteFile).not.toHaveBeenCalled() + }) + + it("should allow creating a .roo skill when the duplicate lives in the lower-priority .agents root", async () => { + const agentsSkillDir = p(globalAgentsSkillsDir, "my-skill") + const agentsSkillMd = p(agentsSkillDir, "SKILL.md") + const rooSkillMd = p(globalSkillsDir, "my-skill", "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalAgentsSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => (dir === globalAgentsSkillsDir ? ["my-skill"] : [])) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === agentsSkillDir) return { isDirectory: () => true } + throw new Error("Not found") + }) + mockFileExists.mockImplementation(async (file: string) => file === agentsSkillMd) + mockReadFile.mockResolvedValue(`--- +name: my-skill +description: An agents skill +--- +Instructions`) + mockMkdir.mockResolvedValue(undefined) + mockWriteFile.mockResolvedValue(undefined) + + await skillsManager.discoverSkills() + expect(skillsManager.getSkill("my-skill", "global")?.path).toBe(agentsSkillMd) + + const created = await skillsManager.createSkill("my-skill", "global", "Description") + + expect(created).toBe(rooSkillMd) + expect(mockWriteFile).toHaveBeenCalledWith(rooSkillMd, expect.any(String), "utf-8") + }) }) describe("deleteSkill", () => { @@ -1755,5 +2354,257 @@ Instructions`) // Verify directory was NOT cleaned up (still has other skills) expect(mockRmdir).not.toHaveBeenCalled() }) + + it("should move a nested skill from its discovered path", async () => { + // The skill was discovered nested inside a symlinked container + // (e.g., skills/repo/my-skill/SKILL.md). moveSkill must rename from the + // discovered path, not a rebuilt skills/my-skill path that does not exist. + const containerDir = p(globalSkillsDir, "repo") + const nestedSkillDir = p(containerDir, "my-skill") + const nestedSkillMd = p(nestedSkillDir, "SKILL.md") + const destSkillsDir = p(GLOBAL_ROO_DIR, "skills-code") + const destDir = p(destSkillsDir, "my-skill") + + mockDirectoryExists.mockImplementation(async (dir: string) => { + return dir === globalSkillsDir || dir === containerDir + }) + + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) return ["repo"] + if (dir === containerDir) return ["my-skill"] + return [] + }) + + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === containerDir || pathArg === nestedSkillDir) { + return { isDirectory: () => true } + } + throw new Error("Not found") + }) + + mockFileExists.mockImplementation(async (file: string) => { + if (file === nestedSkillMd) return true + // Skill does not exist at destination + if (file === p(destDir, "SKILL.md")) return false + return false + }) + + mockReadFile.mockResolvedValue(`--- +name: my-skill +description: A nested skill +--- +Instructions`) + + mockMkdir.mockResolvedValue(undefined) + mockRename.mockResolvedValue(undefined) + mockRmdir.mockResolvedValue(undefined) + + await skillsManager.discoverSkills() + + // Confirm the skill was discovered at its nested container path. + const discovered = skillsManager.getSkill("my-skill", "global") + expect(discovered?.path).toBe(nestedSkillMd) + + // Move with an undefined current mode; the source must come from the + // discovered path (nestedSkillDir), not skills/my-skill. + await skillsManager.moveSkill("my-skill", "global", undefined, "code") + + expect(mockMkdir).toHaveBeenCalledWith(destSkillsDir, { recursive: true }) + expect(mockRename).toHaveBeenCalledWith(nestedSkillDir, destDir) + }) + + describe("cross-filesystem and cleanup safety", () => { + const sourceSkillsDir = p(GLOBAL_ROO_DIR, "skills-code") + const sourceDir = p(sourceSkillsDir, "test-skill") + const destDir = p(GLOBAL_ROO_DIR, "skills-architect", "test-skill") + + const setupCodeSkill = () => { + mockDirectoryExists.mockImplementation(async (dir: string) => dir === sourceSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => (dir === sourceSkillsDir ? ["test-skill"] : [])) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === sourceDir) return { isDirectory: () => true } + throw new Error("Not found") + }) + mockFileExists.mockImplementation(async (file: string) => file === p(sourceDir, "SKILL.md")) + mockReadFile.mockResolvedValue(`--- +name: test-skill +description: A test skill +--- +Instructions`) + mockMkdir.mockResolvedValue(undefined) + mockRm.mockResolvedValue(undefined) + mockRmdir.mockResolvedValue(undefined) + } + + const exdevError = () => Object.assign(new Error("cross-device link not permitted"), { code: "EXDEV" }) + const enoentError = () => Object.assign(new Error("no such file or directory"), { code: "ENOENT" }) + + // Rename from the source fails across devices; promoting the staging copy succeeds. + const setupExdevRename = () => { + mockRename.mockImplementation(async (from: string) => { + if (from === sourceDir) { + throw exdevError() + } + }) + } + + const getStagingDir = (): string => { + const call = mockCp.mock.calls[0] + expect(call).toBeDefined() + return call[1] as string + } + + it("should fall back to copy into a staging dir and promote it when rename fails with EXDEV", async () => { + setupCodeSkill() + setupExdevRename() + mockCp.mockResolvedValue(undefined) + mockLstat.mockRejectedValue(enoentError()) + + await skillsManager.discoverSkills() + await skillsManager.moveSkill("test-skill", "global", "code", "architect") + + expect(mockRename).toHaveBeenCalledWith(sourceDir, destDir) + const stagingDir = getStagingDir() + expect(stagingDir).not.toBe(destDir) + expect(path.dirname(stagingDir)).toBe(path.dirname(destDir)) + expect(mockCp).toHaveBeenCalledWith(sourceDir, stagingDir, { + recursive: true, + errorOnExist: true, + force: false, + }) + expect(mockRename).toHaveBeenCalledWith(stagingDir, destDir) + expect(mockRm).toHaveBeenCalledWith(sourceDir, { recursive: true, force: true }) + expect(mockRm).not.toHaveBeenCalledWith(destDir, expect.anything()) + }) + + it("should not touch an existing destination directory when the EXDEV fallback finds it", async () => { + setupCodeSkill() + setupExdevRename() + mockCp.mockResolvedValue(undefined) + // destDir exists (e.g., contains unrelated files but no SKILL.md) + mockLstat.mockResolvedValue({ isDirectory: () => true }) + + await skillsManager.discoverSkills() + await expect(skillsManager.moveSkill("test-skill", "global", "code", "architect")).rejects.toThrow( + "Destination already exists", + ) + + const stagingDir = getStagingDir() + expect(mockRename).not.toHaveBeenCalledWith(stagingDir, destDir) + expect(mockRm).toHaveBeenCalledWith(stagingDir, { recursive: true, force: true }) + expect(mockRm).not.toHaveBeenCalledWith(destDir, expect.anything()) + expect(mockRm).not.toHaveBeenCalledWith(sourceDir, expect.anything()) + }) + + it("should clean up only the staging dir when promoting the copy fails", async () => { + setupCodeSkill() + mockRename.mockImplementation(async (from: string) => { + if (from === sourceDir) { + throw exdevError() + } + throw Object.assign(new Error("directory not empty"), { code: "ENOTEMPTY" }) + }) + mockCp.mockResolvedValue(undefined) + mockLstat.mockRejectedValue(enoentError()) + + await skillsManager.discoverSkills() + await expect(skillsManager.moveSkill("test-skill", "global", "code", "architect")).rejects.toThrow( + "directory not empty", + ) + + const stagingDir = getStagingDir() + expect(mockRm).toHaveBeenCalledWith(stagingDir, { recursive: true, force: true }) + expect(mockRm).not.toHaveBeenCalledWith(destDir, expect.anything()) + expect(mockRm).not.toHaveBeenCalledWith(sourceDir, expect.anything()) + }) + + it("should not copy when rename succeeds on the same filesystem", async () => { + setupCodeSkill() + mockRename.mockResolvedValue(undefined) + + await skillsManager.discoverSkills() + await skillsManager.moveSkill("test-skill", "global", "code", "architect") + + expect(mockCp).not.toHaveBeenCalled() + expect(mockRm).not.toHaveBeenCalled() + }) + + it("should remove only the partial staging copy and keep the source when the EXDEV copy fails", async () => { + setupCodeSkill() + setupExdevRename() + mockCp.mockRejectedValue(new Error("copy failed")) + + await skillsManager.discoverSkills() + await expect(skillsManager.moveSkill("test-skill", "global", "code", "architect")).rejects.toThrow( + "copy failed", + ) + + const stagingDir = getStagingDir() + expect(mockRm).toHaveBeenCalledWith(stagingDir, { recursive: true, force: true }) + expect(mockRm).not.toHaveBeenCalledWith(destDir, expect.anything()) + expect(mockRm).not.toHaveBeenCalledWith(sourceDir, expect.anything()) + }) + + it("should rethrow non-EXDEV rename errors without copying", async () => { + setupCodeSkill() + mockRename.mockRejectedValue(Object.assign(new Error("permission denied"), { code: "EACCES" })) + + await skillsManager.discoverSkills() + await expect(skillsManager.moveSkill("test-skill", "global", "code", "architect")).rejects.toThrow( + "permission denied", + ) + + expect(mockCp).not.toHaveBeenCalled() + }) + + it("should not remove an emptied parent that resolves outside the configured skills root", async () => { + // skills/shared is a symlink to /shared/skills, a container outside the + // configured root. After moving the only nested skill out, the external + // container must not be removed. + const symlinkEntry = p(globalSkillsDir, "shared") + const externalSkillDir = p(SHARED_DIR, "ext-skill") + const externalSkillMd = p(externalSkillDir, "SKILL.md") + const destSkillsDir = p(GLOBAL_ROO_DIR, "skills-code") + + mockDirectoryExists.mockImplementation( + async (dir: string) => dir === globalSkillsDir || dir === p(globalSkillsDir, "shared"), + ) + mockRealpath.mockImplementation(async (pathArg: string) => + pathArg === symlinkEntry ? SHARED_DIR : pathArg, + ) + // Discovery scans the realpath-resolved container; after the move it is empty + let discovering = true + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) return ["shared"] + if (dir === SHARED_DIR) return discovering ? ["ext-skill"] : [] + return [] + }) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === symlinkEntry || pathArg === externalSkillDir) return { isDirectory: () => true } + throw new Error("Not found") + }) + mockFileExists.mockImplementation(async (file: string) => file === externalSkillMd) + mockReadFile.mockResolvedValue(`--- +name: ext-skill +description: External skill +--- +Instructions`) + mockMkdir.mockResolvedValue(undefined) + mockRename.mockResolvedValue(undefined) + mockRmdir.mockResolvedValue(undefined) + + await skillsManager.discoverSkills() + expect(skillsManager.getSkill("ext-skill", "global")?.path).toBe(externalSkillMd) + discovering = false + + await skillsManager.moveSkill("ext-skill", "global", undefined, "code") + + expect(mockRename).toHaveBeenCalledWith(externalSkillDir, p(destSkillsDir, "ext-skill")) + expect(mockRmdir).not.toHaveBeenCalled() + }) + }) }) })