Fix skill discovery inside a symlinked container directory

src/services/skills/SkillsManager.ts

@@ -20,6 +20,13 @@
 
 export class SkillsManager {
 	private skills: Map<string, SkillMetadata> = new Map()
+	/**
+	 * Configured top-level skills root (e.g. ~/.roo/skills or <cwd>/.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<string, string> = new Map()
 	private providerRef: WeakRef<ClineProvider>
 	private disposables: vscode.Disposable[] = []
 	private isDisposed = false
@@ -42,20 +49,66 @@
 	 */
 	async discoverSkills(): Promise<void> {
 		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<string>(), new Map<string, number>(), 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<void> {
+	private async scanSkillsDirectory(
+	 dirPath: string,
+	 source: "global" | "project",
+	 mode: string | undefined,
+	 depth: number = 0,
+	 visited: Set<string> = new Set<string>(),
+	 claimedDepths: Map<string, number> = new Map<string, number>(),
+	 rootDir: string = dirPath,
+	): Promise<void> {
+		if (depth > SkillsManager.MAX_SCAN_DEPTH) {
+			return
+		}
+
 		if (!(await directoryExists(dirPath))) {
 			return
 		}
@@ -64,8 +117,16 @@
 			// 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 @@
 				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 @@
 	 * @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<string, number>,
+	 rootDir?: string,
 	): Promise<void> {
 		const skillMdPath = path.join(skillDir, "SKILL.md")
 		if (!(await fileExists(skillMdPath))) return
@@ -162,6 +246,24 @@
 			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 @@
 		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 @@
 			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 @@
 		// 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 @@
 		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<void> {
+		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<boolean> {
+		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 @@
 		this.disposables.forEach((d) => d.dispose())
 		this.disposables = []
 		this.skills.clear()
+		this.skillRoots.clear()
 	}
 }