diff --git a/src/services/command/__tests__/frontmatter-commands.spec.ts b/src/services/command/__tests__/frontmatter-commands.spec.ts index 3f93b55f94..4d249c5ccc 100644 --- a/src/services/command/__tests__/frontmatter-commands.spec.ts +++ b/src/services/command/__tests__/frontmatter-commands.spec.ts @@ -1,6 +1,7 @@ import fs from "fs/promises" import * as path from "path" +import { getBuiltInCommands } from "../built-in-commands" import { getCommand, getCommands } from "../commands" // Mock fs and path modules @@ -471,4 +472,139 @@ Deploy the app.` ) }) }) + + describe("getCommands source precedence", () => { + const mockGetBuiltInCommands = vi.mocked(getBuiltInCommands) + + it("should list a global command over a colliding built-in command", async () => { + const globalInitContent = `# Global Init + +Global init instructions.` + + mockGetBuiltInCommands.mockResolvedValueOnce([ + { + name: "init", + content: "Built-in init instructions.", + source: "built-in", + filePath: "built-in://init", + }, + ]) + mockFs.stat = vi.fn().mockResolvedValue({ isDirectory: () => true }) + mockFs.readdir = vi + .fn() + .mockResolvedValueOnce([{ name: "init.md", isFile: () => true }]) + .mockResolvedValueOnce([]) + mockFs.readFile = vi.fn().mockResolvedValue(globalInitContent) + + const result = await getCommands("/test/cwd") + const init = result.filter((command) => command.name === "init") + + // A duplicate init row would mean the built-in was listed twice. + expect(init).toHaveLength(1) + expect(init[0]).toEqual( + expect.objectContaining({ + source: "global", + content: "# Global Init\n\nGlobal init instructions.", + filePath: expect.stringContaining(path.join(".roo", "commands", "init.md")), + }), + ) + }) + + it("should list a project command over a colliding global command", async () => { + const globalInitContent = `# Global Init + +Global init instructions.` + const projectInitContent = `# Project Init + +Project init instructions.` + + mockFs.stat = vi.fn().mockResolvedValue({ isDirectory: () => true }) + mockFs.readdir = vi + .fn() + .mockResolvedValueOnce([{ name: "init.md", isFile: () => true }]) + .mockResolvedValueOnce([{ name: "init.md", isFile: () => true }]) + mockFs.readFile = vi.fn().mockResolvedValueOnce(globalInitContent).mockResolvedValueOnce(projectInitContent) + + const result = await getCommands("/test/cwd") + const init = result.filter((command) => command.name === "init") + + expect(init).toHaveLength(1) + expect(init[0]).toEqual( + expect.objectContaining({ + source: "project", + content: "# Project Init\n\nProject init instructions.", + filePath: path.resolve(path.join("/test/cwd", ".roo", "commands", "init.md")), + }), + ) + }) + + it("should list a project command over a colliding built-in command when no global command exists", async () => { + const projectInitContent = `# Project Init + +Project init instructions.` + + mockGetBuiltInCommands.mockResolvedValueOnce([ + { + name: "init", + content: "Built-in init instructions.", + source: "built-in", + filePath: "built-in://init", + }, + ]) + mockFs.stat = vi.fn().mockResolvedValue({ isDirectory: () => true }) + mockFs.readdir = vi + .fn() + .mockResolvedValueOnce([]) // global commands directory is empty + .mockResolvedValueOnce([{ name: "init.md", isFile: () => true }]) + mockFs.readFile = vi.fn().mockResolvedValue(projectInitContent) + + const result = await getCommands("/test/cwd") + const init = result.filter((command) => command.name === "init") + + expect(init).toHaveLength(1) + expect(init[0]).toEqual( + expect.objectContaining({ + source: "project", + content: "# Project Init\n\nProject init instructions.", + filePath: path.resolve(path.join("/test/cwd", ".roo", "commands", "init.md")), + }), + ) + }) + + it("should list exactly one command for a three-way built-in/global/project collision", async () => { + const globalInitContent = `# Global Init + +Global init instructions.` + const projectInitContent = `# Project Init + +Project init instructions.` + + mockGetBuiltInCommands.mockResolvedValueOnce([ + { + name: "init", + content: "Built-in init instructions.", + source: "built-in", + filePath: "built-in://init", + }, + ]) + mockFs.stat = vi.fn().mockResolvedValue({ isDirectory: () => true }) + mockFs.readdir = vi + .fn() + .mockResolvedValueOnce([{ name: "init.md", isFile: () => true }]) + .mockResolvedValueOnce([{ name: "init.md", isFile: () => true }]) + mockFs.readFile = vi.fn().mockResolvedValueOnce(globalInitContent).mockResolvedValueOnce(projectInitContent) + + const result = await getCommands("/test/cwd") + const init = result.filter((command) => command.name === "init") + + expect(init).toHaveLength(1) + expect(init[0]).toEqual( + expect.objectContaining({ + source: "project", + content: "# Project Init\n\nProject init instructions.", + filePath: path.resolve(path.join("/test/cwd", ".roo", "commands", "init.md")), + }), + ) + }) + }) }) diff --git a/src/services/command/__tests__/symlink-commands.spec.ts b/src/services/command/__tests__/symlink-commands.spec.ts index 0d77967484..e0d0762a80 100644 --- a/src/services/command/__tests__/symlink-commands.spec.ts +++ b/src/services/command/__tests__/symlink-commands.spec.ts @@ -1,5 +1,4 @@ import fs from "fs/promises" -import * as path from "path" import { getCommand, getCommands } from "../commands" @@ -156,7 +155,7 @@ description: Symlinked command expect(symlinkCmd?.content).toContain("Symlinked Command") }) - it.skipIf(process.platform === "win32")("should discover commands from symlinked directories", async () => { + it.skipIf(process.platform === "win32")("should not discover commands from symlinked directories", async () => { const nestedContent = `# Nested Command from Symlinked Dir` // Mock lstat for symlink target type checking (lstat doesn't follow symlinks) @@ -220,21 +219,24 @@ description: Symlinked command // Mock readlink for symlink to directory mockFs.readlink = vi.fn().mockResolvedValue("/mock/shared-commands") - // Mock readFile for content + // Mock readFile for content: nested.md only exists inside the symlink + // target, never as a direct file in a commands directory. mockFs.readFile = vi.fn().mockImplementation((filePath: string) => { const normalizedPath = filePath.toString().replace(/\\/g, "/") - if (normalizedPath.includes("nested.md")) { + if (normalizedPath.includes("shared-commands") && normalizedPath.includes("nested.md")) { return Promise.resolve(nestedContent) } return Promise.reject(new Error("File not found")) }) + // The listing must not surface commands that are only reachable through a + // directory symlink: getCommand() only probes direct command files and + // file symlinks, so getCommands() applies the same limits. const result = await getCommands("/test/cwd") + expect(result.find((c) => c.name === "nested")).toBeUndefined() - // Find a command that was discovered from the symlinked directory - const nestedCmd = result.find((c) => c.name === "nested") - expect(nestedCmd).toBeDefined() - expect(nestedCmd?.content).toContain("Nested Command from Symlinked Dir") + // Execution agrees: the command cannot be resolved either. + expect(await getCommand("/test/cwd", "nested")).toBeUndefined() }) // Note: Nested symlinks (symlink -> symlink -> file) are automatically followed by fs.stat, diff --git a/src/services/command/__tests__/symlink-directory-consistency.spec.ts b/src/services/command/__tests__/symlink-directory-consistency.spec.ts new file mode 100644 index 0000000000..2e4aa00ad1 --- /dev/null +++ b/src/services/command/__tests__/symlink-directory-consistency.spec.ts @@ -0,0 +1,86 @@ +import fs from "fs/promises" +import * as path from "path" +import { tmpdir } from "node:os" + +const mockHome = vi.hoisted(() => ({ path: "" })) + +vi.mock("os", async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + homedir: () => mockHome.path, + } +}) + +import { getCommand, getCommands } from "../commands" + +/** + * Real-filesystem tests for listing/execution consistency. getCommands() (the + * listing) must surface exactly what getCommand() (execution) can resolve: + * direct .md files and file symlinks in the commands directory. Directory + * symlinks are not followed by either path. + */ +describe("command listing and execution consistency for symlinks", () => { + let tempDir: string + let cwd: string + let globalCommandsDir: string + + const dirLinkType = process.platform === "win32" ? "junction" : "dir" + + beforeEach(async () => { + tempDir = await fs.mkdtemp(path.join(tmpdir(), "zoo-command-symlink-")) + mockHome.path = path.join(tempDir, "home") + cwd = path.join(tempDir, "workspace") + globalCommandsDir = path.join(mockHome.path, ".roo", "commands") + await fs.mkdir(globalCommandsDir, { recursive: true }) + await fs.mkdir(cwd, { recursive: true }) + }) + + afterEach(async () => { + await fs.rm(tempDir, { recursive: true, force: true }) + }) + + it("selects the built-in command in both listing and execution when a global directory symlink collides with it", async () => { + const sharedDir = path.join(tempDir, "shared") + await fs.mkdir(sharedDir, { recursive: true }) + await fs.writeFile(path.join(sharedDir, "init.md"), "# Global Init via directory symlink") + await fs.symlink(sharedDir, path.join(globalCommandsDir, "shared-link"), dirLinkType) + + const listed = (await getCommands(cwd)).filter((command) => command.name === "init") + expect(listed).toHaveLength(1) + expect(listed[0].source).toBe("built-in") + + const executed = await getCommand(cwd, "init") + expect(executed?.source).toBe("built-in") + + // Listing and execution must select the same command. + expect(listed[0].content).toBe(executed?.content) + expect(listed[0].filePath).toBe(executed?.filePath) + }) + + it("does not list or execute a command that is only reachable through a directory symlink", async () => { + const sharedDir = path.join(tempDir, "shared") + await fs.mkdir(sharedDir, { recursive: true }) + await fs.writeFile(path.join(sharedDir, "nested.md"), "# Nested Command") + await fs.symlink(sharedDir, path.join(globalCommandsDir, "shared-link"), dirLinkType) + + const listed = await getCommands(cwd) + expect(listed.find((command) => command.name === "nested")).toBeUndefined() + + expect(await getCommand(cwd, "nested")).toBeUndefined() + }) + + it.skipIf(process.platform === "win32")("lists and executes file symlinks at the commands directory root", async () => { + const sharedDir = path.join(mockHome.path, ".roo", "shared") + await fs.mkdir(sharedDir, { recursive: true }) + await fs.writeFile(path.join(sharedDir, "aliased-target.md"), "# Aliased Command") + await fs.symlink(path.join(sharedDir, "aliased-target.md"), path.join(globalCommandsDir, "aliased.md"), "file") + + const listed = (await getCommands(cwd)).filter((command) => command.name === "aliased") + expect(listed).toHaveLength(1) + expect(listed[0].content).toContain("Aliased Command") + + const executed = await getCommand(cwd, "aliased") + expect(executed?.content).toContain("Aliased Command") + }) +}) diff --git a/src/services/command/commands.ts b/src/services/command/commands.ts index 4e69558dc1..8bf569f16a 100644 --- a/src/services/command/commands.ts +++ b/src/services/command/commands.ts @@ -52,18 +52,13 @@ async function resolveCommandSymLink(symlinkPath: string, fileInfo: CommandFileI // For symlinks to files, store the symlink path as original and target as resolved fileInfo.push({ originalPath: symlinkPath, resolvedPath: resolvedTarget }) } - } else if (stats.isDirectory()) { - // Read the target directory and process its entries - const entries = await fs.readdir(resolvedTarget, { withFileTypes: true }) - const directoryPromises: Promise[] = [] - for (const entry of entries) { - directoryPromises.push(resolveCommandDirectoryEntry(entry, resolvedTarget, fileInfo, depth + 1)) - } - await Promise.all(directoryPromises) } else if (stats.isSymbolicLink()) { // Handle nested symlinks await resolveCommandSymLink(resolvedTarget, fileInfo, depth + 1) } + // Directory symlink targets are intentionally not descended into, so the + // listing surfaces exactly what getCommand() can execute: direct command + // files and file symlinks at the commands directory root. } catch { // Skip invalid symlinks } @@ -328,18 +323,15 @@ async function scanCommandDirectory( commandContent = content.trim() } - // Project commands override global ones - if (source === "project" || !commands.has(commandName)) { - commands.set(commandName, { - name: commandName, - content: commandContent, - source, - filePath: resolvedPath, - description, - argumentHint, - mode, - }) - } + commands.set(commandName, { + name: commandName, + content: commandContent, + source, + filePath: resolvedPath, + description, + argumentHint, + mode, + }) } catch (error) { console.warn(`Failed to read command file ${resolvedPath}:`, error) }