Skip to content

Commit fcc317b

Browse files
roomoteroomote
committed
fix: address multi-root review feedback
1 parent 0121d96 commit fcc317b

11 files changed

Lines changed: 253 additions & 35 deletions

‎apps/vscode-e2e/src/suite/multi-root-read-file-content.test.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,11 @@ suite("Multi-root readFileContent repro", function () {
1212
setDefaultSuiteTimeout(this)
1313

1414
test("should read a file that exists only in the secondary workspace root", async () => {
15+
await waitFor(() => (vscode.workspace.workspaceFolders?.length ?? 0) >= 2, {
16+
timeout: 15_000,
17+
interval: 100,
18+
})
19+
1520
const primaryWorkspace = vscode.workspace.workspaceFolders?.[0]
1621
assert.ok(primaryWorkspace, "Expected a primary workspace folder")
1722
const secondaryWorkspace = vscode.workspace.workspaceFolders?.[1]

‎src/core/ignore/RooIgnoreController.ts‎

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,20 @@ export class RooIgnoreController {
127127
return { ignoreInstance, content: undefined }
128128
}
129129

130+
private getKnownWorkspaceRoots(): string[] {
131+
const roots = new Set<string>([this.cwd])
132+
for (const folder of vscode.workspace.workspaceFolders ?? []) {
133+
roots.add(folder.uri.fsPath)
134+
}
135+
return [...roots]
136+
}
137+
138+
private getAvailableIgnoreContents(): Array<{ rootPath: string; content: string }> {
139+
return this.getKnownWorkspaceRoots()
140+
.map((rootPath) => ({ rootPath, content: this.getIgnoreStateForRoot(rootPath).content }))
141+
.filter((entry): entry is { rootPath: string; content: string } => typeof entry.content === "string")
142+
}
143+
130144
/**
131145
* Check if a file should be accessible to the LLM
132146
* Automatically resolves symlinks
@@ -176,11 +190,6 @@ export class RooIgnoreController {
176190
* @returns path of file that is being accessed if it is being accessed, undefined if command is allowed
177191
*/
178192
validateCommand(command: string): string | undefined {
179-
// Always allow if no .rooignore exists
180-
if (!this.rooIgnoreContent) {
181-
return undefined
182-
}
183-
184193
// Split command into parts and get the base command
185194
const parts = command.trim().split(/\s+/)
186195
const baseCommand = parts[0].toLowerCase()
@@ -209,7 +218,7 @@ export class RooIgnoreController {
209218
for (let i = 1; i < parts.length; i++) {
210219
const arg = parts[i]
211220
// Skip command flags/options (both Unix and PowerShell style)
212-
if (arg.startsWith("-") || arg.startsWith("/")) {
221+
if (arg.startsWith("-") || (arg.startsWith("/") && !path.isAbsolute(arg))) {
213222
continue
214223
}
215224
// Ignore PowerShell parameter names
@@ -259,10 +268,21 @@ export class RooIgnoreController {
259268
* @returns Formatted instructions or undefined if .rooignore doesn't exist
260269
*/
261270
getInstructions(): string | undefined {
262-
if (!this.rooIgnoreContent) {
271+
const ignoreEntries = this.getAvailableIgnoreContents()
272+
if (ignoreEntries.length === 0) {
263273
return undefined
264274
}
265275

266-
return `# .rooignore\n\n(The following is provided by a root-level .rooignore file where the user has specified files and directories that should not be accessed. When using list_files, you'll notice a ${LOCK_TEXT_SYMBOL} next to files that are blocked. Attempting to access the file's contents e.g. through read_file will result in an error.)\n\n${this.rooIgnoreContent}\n.rooignore`
276+
const sections = ignoreEntries
277+
.map(({ rootPath, content }) => {
278+
const workspaceName =
279+
vscode.workspace.workspaceFolders?.find((folder) => folder.uri.fsPath === rootPath)?.name ??
280+
path.basename(rootPath) ??
281+
rootPath
282+
return `## ${workspaceName}\n\n${content}\n.rooignore`
283+
})
284+
.join("\n\n")
285+
286+
return `# .rooignore\n\n(The following is provided by workspace-root .rooignore files where the user has specified files and directories that should not be accessed. When using list_files, you'll notice a ${LOCK_TEXT_SYMBOL} next to files that are blocked. Attempting to access the file's contents e.g. through read_file will result in an error.)\n\n${sections}`
267287
}
268288
}

‎src/core/ignore/__tests__/RooIgnoreController.spec.ts‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -338,6 +338,32 @@ describe("RooIgnoreController", () => {
338338
expect(emptyController.validateCommand("cat node_modules/package.json")).toBeUndefined()
339339
expect(emptyController.validateCommand("grep pattern .git/config")).toBeUndefined()
340340
})
341+
342+
it("should enforce a secondary workspace .rooignore for execute_command paths", async () => {
343+
const secondaryRoot = "/test/secondary"
344+
;(vscode.workspace as any).workspaceFolders = [
345+
{ uri: { fsPath: TEST_CWD }, name: "primary", index: 0 },
346+
{ uri: { fsPath: secondaryRoot }, name: "secondary", index: 1 },
347+
]
348+
349+
mockFileExists.mockImplementation(async (filePath) => filePath === path.join(TEST_CWD, ".rooignore"))
350+
mockReadFile.mockResolvedValue("node_modules\n.git\nsecrets/**\n*.log")
351+
await controller.initialize()
352+
353+
vi.mocked(fsSync.existsSync).mockImplementation(
354+
(filePath) => filePath === path.join(secondaryRoot, ".rooignore"),
355+
)
356+
vi.mocked(fsSync.readFileSync).mockImplementation((filePath) => {
357+
if (filePath === path.join(secondaryRoot, ".rooignore")) {
358+
return "private/**"
359+
}
360+
return ""
361+
})
362+
363+
expect(controller.validateCommand(`cat ${path.join(secondaryRoot, "private", "secret.txt")}`)).toBe(
364+
path.join(secondaryRoot, "private", "secret.txt"),
365+
)
366+
})
341367
})
342368

343369
describe("filterPaths", () => {
@@ -434,6 +460,40 @@ describe("RooIgnoreController", () => {
434460
const instructions = controller.getInstructions()
435461
expect(instructions).toBeUndefined()
436462
})
463+
464+
it("should include secondary workspace .rooignore content in instructions", async () => {
465+
const secondaryRoot = "/test/secondary"
466+
;(vscode.workspace as any).workspaceFolders = [
467+
{ uri: { fsPath: TEST_CWD }, name: "primary", index: 0 },
468+
{ uri: { fsPath: secondaryRoot }, name: "secondary", index: 1 },
469+
]
470+
471+
mockFileExists.mockImplementation(async (filePath) => filePath === path.join(TEST_CWD, ".rooignore"))
472+
mockReadFile.mockResolvedValue("node_modules")
473+
await controller.initialize()
474+
475+
vi.mocked(fsSync.existsSync).mockImplementation(
476+
(filePath) =>
477+
filePath === path.join(secondaryRoot, ".rooignore") ||
478+
filePath === path.join(TEST_CWD, ".rooignore"),
479+
)
480+
vi.mocked(fsSync.readFileSync).mockImplementation((filePath) => {
481+
if (filePath === path.join(secondaryRoot, ".rooignore")) {
482+
return "private/**"
483+
}
484+
if (filePath === path.join(TEST_CWD, ".rooignore")) {
485+
return "node_modules"
486+
}
487+
return ""
488+
})
489+
490+
const instructions = controller.getInstructions()
491+
492+
expect(instructions).toContain("## primary")
493+
expect(instructions).toContain("## secondary")
494+
expect(instructions).toContain("node_modules")
495+
expect(instructions).toContain("private/**")
496+
})
437497
})
438498

439499
describe("dispose", () => {

‎src/core/tools/__tests__/applyPatchTool.partial.spec.ts‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,21 @@ import { isPathOutsideWorkspace } from "../../../utils/pathUtils"
77
import type { Task } from "../../task/Task"
88
import { ApplyPatchTool } from "../ApplyPatchTool"
99

10-
vi.mock("../../../utils/pathUtils", () => ({
11-
isPathOutsideWorkspace: vi.fn(),
12-
}))
10+
vi.mock("../../../utils/pathUtils", async () => {
11+
const actual = await vi.importActual<typeof import("../../../utils/pathUtils")>("../../../utils/pathUtils")
12+
return {
13+
...actual,
14+
isPathOutsideWorkspace: vi.fn(),
15+
resolvePathInWorkspace: vi
16+
.fn()
17+
.mockImplementation(async (cwd: string, filePath: string) => path.resolve(cwd, filePath)),
18+
getWorkspaceReadablePath: vi
19+
.fn()
20+
.mockImplementation(
21+
(cwd: string, _absolutePath: string, fallbackPath?: string) => fallbackPath ?? path.basename(cwd),
22+
),
23+
}
24+
})
1325

1426
interface PartialApplyPatchPayload {
1527
tool: string

‎src/core/tools/__tests__/editFileTool.spec.ts‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,21 @@ vi.mock("../../prompts/responses", () => ({
4444
},
4545
}))
4646

47-
vi.mock("../../../utils/pathUtils", () => ({
48-
isPathOutsideWorkspace: vi.fn().mockReturnValue(false),
49-
}))
47+
vi.mock("../../../utils/pathUtils", async () => {
48+
const actual = await vi.importActual<typeof import("../../../utils/pathUtils")>("../../../utils/pathUtils")
49+
return {
50+
...actual,
51+
isPathOutsideWorkspace: vi.fn().mockReturnValue(false),
52+
resolvePathInWorkspace: vi
53+
.fn()
54+
.mockImplementation(async (cwd: string, filePath: string) => path.resolve(cwd, filePath)),
55+
getWorkspaceReadablePath: vi
56+
.fn()
57+
.mockImplementation(
58+
(_cwd: string, _absolutePath: string, fallbackPath?: string) => fallbackPath ?? "test/path.txt",
59+
),
60+
}
61+
})
5062

5163
vi.mock("../../../utils/path", () => ({
5264
getReadablePath: vi.fn().mockReturnValue("test/path.txt"),

‎src/core/tools/__tests__/editTool.spec.ts‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,21 @@ vi.mock("../../prompts/responses", () => ({
4444
},
4545
}))
4646

47-
vi.mock("../../../utils/pathUtils", () => ({
48-
isPathOutsideWorkspace: vi.fn().mockReturnValue(false),
49-
}))
47+
vi.mock("../../../utils/pathUtils", async () => {
48+
const actual = await vi.importActual<typeof import("../../../utils/pathUtils")>("../../../utils/pathUtils")
49+
return {
50+
...actual,
51+
isPathOutsideWorkspace: vi.fn().mockReturnValue(false),
52+
resolvePathInWorkspace: vi
53+
.fn()
54+
.mockImplementation(async (cwd: string, filePath: string) => path.resolve(cwd, filePath)),
55+
getWorkspaceReadablePath: vi
56+
.fn()
57+
.mockImplementation(
58+
(_cwd: string, _absolutePath: string, fallbackPath?: string) => fallbackPath ?? "test/path.txt",
59+
),
60+
}
61+
})
5062

5163
vi.mock("../../../utils/path", () => ({
5264
getReadablePath: vi.fn().mockReturnValue("test/path.txt"),

‎src/core/tools/__tests__/searchReplaceTool.spec.ts‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,21 @@ vi.mock("../../prompts/responses", () => ({
4444
},
4545
}))
4646

47-
vi.mock("../../../utils/pathUtils", () => ({
48-
isPathOutsideWorkspace: vi.fn().mockReturnValue(false),
49-
}))
47+
vi.mock("../../../utils/pathUtils", async () => {
48+
const actual = await vi.importActual<typeof import("../../../utils/pathUtils")>("../../../utils/pathUtils")
49+
return {
50+
...actual,
51+
isPathOutsideWorkspace: vi.fn().mockReturnValue(false),
52+
resolvePathInWorkspace: vi
53+
.fn()
54+
.mockImplementation(async (cwd: string, filePath: string) => path.resolve(cwd, filePath)),
55+
getWorkspaceReadablePath: vi
56+
.fn()
57+
.mockImplementation(
58+
(_cwd: string, _absolutePath: string, fallbackPath?: string) => fallbackPath ?? "test/path.txt",
59+
),
60+
}
61+
})
5062

5163
vi.mock("../../../utils/path", () => ({
5264
getReadablePath: vi.fn().mockReturnValue("test/path.txt"),

‎src/core/tools/__tests__/writeToFileTool.spec.ts‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,21 @@ vi.mock("../../prompts/responses", () => ({
3939
},
4040
}))
4141

42-
vi.mock("../../../utils/pathUtils", () => ({
43-
isPathOutsideWorkspace: vi.fn().mockReturnValue(false),
44-
}))
42+
vi.mock("../../../utils/pathUtils", async () => {
43+
const actual = await vi.importActual<typeof import("../../../utils/pathUtils")>("../../../utils/pathUtils")
44+
return {
45+
...actual,
46+
isPathOutsideWorkspace: vi.fn().mockReturnValue(false),
47+
resolvePathInWorkspace: vi
48+
.fn()
49+
.mockImplementation(async (cwd: string, filePath: string) => path.resolve(cwd, filePath)),
50+
getWorkspaceReadablePath: vi
51+
.fn()
52+
.mockImplementation(
53+
(_cwd: string, _absolutePath: string, fallbackPath?: string) => fallbackPath ?? "test/path.txt",
54+
),
55+
}
56+
})
4557

4658
vi.mock("../../../utils/path", () => ({
4759
getReadablePath: vi.fn().mockReturnValue("test/path.txt"),
@@ -239,7 +251,7 @@ describe("writeToFileTool", () => {
239251
it("validates and allows access when rooIgnoreController permits", async () => {
240252
await executeWriteFileTool({}, { accessAllowed: true })
241253

242-
expect(mockCline.rooIgnoreController.validateAccess).toHaveBeenCalledWith(testFilePath)
254+
expect(mockCline.rooIgnoreController.validateAccess).toHaveBeenCalledWith(absoluteFilePath)
243255
expect(mockCline.diffViewProvider.open).toHaveBeenCalledWith(testFilePath)
244256
})
245257
})

‎src/core/webview/__tests__/webviewMessageHandler.readFileContent.spec.ts‎

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -40,17 +40,30 @@ vi.mock("../../../utils/fs")
4040
vi.mock("../../../utils/path")
4141
vi.mock("../../../utils/globalContext")
4242

43-
vi.mock("../../../utils/pathUtils", () => ({
44-
isPathOutsideWorkspace: vi.fn((filePath: string) => {
45-
const nodePath = require("path")
46-
const normalized = nodePath.resolve(filePath)
47-
const workspaceRoot = nodePath.resolve("/mock/workspace")
48-
// Path is inside workspace if it equals or is under workspace root
49-
if (normalized === workspaceRoot) return false
50-
if (normalized.startsWith(workspaceRoot + nodePath.sep)) return false
51-
return true
52-
}),
53-
}))
43+
vi.mock("../../../utils/pathUtils", async () => {
44+
const actual = await vi.importActual<typeof import("../../../utils/pathUtils")>("../../../utils/pathUtils")
45+
return {
46+
...actual,
47+
isPathOutsideWorkspace: vi.fn((filePath: string) => {
48+
const nodePath = require("path")
49+
const normalized = nodePath.resolve(filePath)
50+
const workspaceRoot = nodePath.resolve("/mock/workspace")
51+
// Path is inside workspace if it equals or is under workspace root
52+
if (normalized === workspaceRoot) return false
53+
if (normalized.startsWith(workspaceRoot + nodePath.sep)) return false
54+
return true
55+
}),
56+
resolvePathInWorkspace: vi.fn().mockImplementation(async (cwd: string, filePath: string) => {
57+
const nodePath = require("path")
58+
return nodePath.resolve(cwd, filePath)
59+
}),
60+
getWorkspaceReadablePath: vi
61+
.fn()
62+
.mockImplementation(
63+
(_cwd: string, _absolutePath: string, fallbackPath?: string) => fallbackPath ?? "mock/workspace",
64+
),
65+
}
66+
})
5467

5568
vi.mock("../../mentions/resolveImageMentions", () => ({
5669
resolveImageMentions: vi.fn(async ({ text, images }: { text: string; images?: string[] }) => ({

‎src/utils/__tests__/pathUtils.spec.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,22 @@ describe("pathUtils", () => {
5959
)
6060
})
6161

62+
it("resolves a new file into the only workspace root that already contains the parent directory", async () => {
63+
const primaryRoot = path.join(tempDir, "primary")
64+
const secondaryRoot = path.join(tempDir, "secondary")
65+
await fs.mkdir(path.join(primaryRoot, "src"), { recursive: true })
66+
await fs.mkdir(path.join(secondaryRoot, "nested", "dir"), { recursive: true })
67+
68+
mockWorkspace.workspaceFolders = [
69+
{ uri: { fsPath: primaryRoot }, name: "primary", index: 0 },
70+
{ uri: { fsPath: secondaryRoot }, name: "secondary", index: 1 },
71+
]
72+
73+
await expect(resolvePathInWorkspace(primaryRoot, path.join("nested", "dir", "new-file.txt"))).resolves.toBe(
74+
path.join(secondaryRoot, "nested", "dir", "new-file.txt"),
75+
)
76+
})
77+
6278
it("does not allow workspace-folder-name prefixes to escape the selected root", async () => {
6379
const primaryRoot = path.join(tempDir, "primary")
6480
const secondaryRoot = path.join(tempDir, "secondary")

0 commit comments

Comments
 (0)