diff --git a/src/core/auto-approval/WorkspaceRootCanonicalizer.ts b/src/core/auto-approval/WorkspaceRootCanonicalizer.ts new file mode 100644 index 0000000000..a63ee4392c --- /dev/null +++ b/src/core/auto-approval/WorkspaceRootCanonicalizer.ts @@ -0,0 +1,44 @@ +import type { WorkspaceRootStrategy } from "./strategies/WorkspaceRootStrategy" +import { WorkspaceRootStrategyFactory } from "./strategies/WorkspaceRootStrategyFactory" + +/** Translate only a verified workspace-root spelling, never a file symlink. */ +export class WorkspaceRootCanonicalizer { + private constructor( + private readonly root: string | undefined, + private readonly canonicalRoot: string | undefined, + private readonly strategy: WorkspaceRootStrategy, + ) {} + + static forStrategy( + cwd: string | undefined, + canonicalCwd: string | undefined, + strategy: WorkspaceRootStrategy, + ): WorkspaceRootCanonicalizer { + const root = strategy.normalizeAbsoluteRoot(cwd) + const canonicalRoot = strategy.normalizeAbsoluteRoot(canonicalCwd) + return new WorkspaceRootCanonicalizer(root, canonicalRoot, strategy) + } + + static forPlatform( + cwd: string | undefined, + canonicalCwd: string | undefined, + isWindows: boolean, + ): WorkspaceRootCanonicalizer { + const strategy = WorkspaceRootStrategyFactory.create(isWindows) + return WorkspaceRootCanonicalizer.forStrategy(cwd, canonicalCwd, strategy) + } + + canonicalize(value: string): string { + if (this.root === undefined || this.canonicalRoot === undefined) return value + + return this.replaceWorkspaceRootPrefix(value, this.root, this.canonicalRoot) + } + + private replaceWorkspaceRootPrefix(value: string, root: string, canonicalRoot: string): string { + const comparableValue = this.strategy.comparisonKey(value) + const comparableRoot = this.strategy.comparisonKey(root) + return comparableValue === comparableRoot || comparableValue.startsWith(`${comparableRoot}/`) + ? `${canonicalRoot}${value.slice(root.length)}` + : value + } +} diff --git a/src/core/auto-approval/__tests__/WorkspaceRootCanonicalizer.spec.ts b/src/core/auto-approval/__tests__/WorkspaceRootCanonicalizer.spec.ts new file mode 100644 index 0000000000..d0492903f0 --- /dev/null +++ b/src/core/auto-approval/__tests__/WorkspaceRootCanonicalizer.spec.ts @@ -0,0 +1,166 @@ +import { PosixWorkspaceRootStrategy } from "../strategies/PosixWorkspaceRootStrategy" +import { WindowsWorkspaceRootStrategy } from "../strategies/WindowsWorkspaceRootStrategy" +import type { WorkspaceRootStrategy } from "../strategies/WorkspaceRootStrategy" +import { WorkspaceRootCanonicalizer } from "../WorkspaceRootCanonicalizer" + +it("normalizes roots once in the factory before any path comparisons", () => { + const normalizeAbsoluteRoot = vi + .fn() + .mockReturnValueOnce("/aliases/project") + .mockReturnValueOnce("/storage/project") + const comparisonKey = vi.fn((value: string) => value) + const strategy: WorkspaceRootStrategy = { + normalizeAbsoluteRoot, + comparisonKey, + toMatcherAbsolutePath: vi.fn((value: string) => value), + } + + const canonicalizer = WorkspaceRootCanonicalizer.forStrategy("/aliases/project/", "/storage/project/", strategy) + + expect(normalizeAbsoluteRoot).toHaveBeenCalledTimes(2) + expect(normalizeAbsoluteRoot).toHaveBeenNthCalledWith(1, "/aliases/project/") + expect(normalizeAbsoluteRoot).toHaveBeenNthCalledWith(2, "/storage/project/") + expect(comparisonKey).not.toHaveBeenCalled() + + expect(canonicalizer.canonicalize("/aliases/project/docs/file.md")).toBe("/storage/project/docs/file.md") + expect(canonicalizer.canonicalize("/aliases/project/notes.md")).toBe("/storage/project/notes.md") + expect(normalizeAbsoluteRoot).toHaveBeenCalledTimes(2) +}) + +describe.each([ + { + platform: "POSIX", + isWindows: false, + cwd: "/aliases/project", + canonicalCwd: "/storage/project", + root: "/aliases/project", + canonicalRoot: "/storage/project", + }, + { + platform: "Windows", + isWindows: true, + cwd: "C:\\Aliases\\Project", + canonicalCwd: "D:\\Storage\\Project", + root: "/C/Aliases/Project", + canonicalRoot: "/D/Storage/Project", + }, +])("workspace-root canonicalization on $platform", ({ isWindows, cwd, canonicalCwd, root, canonicalRoot }) => { + const canonicalizer = WorkspaceRootCanonicalizer.forPlatform(cwd, canonicalCwd, isWindows) + + it("translates an exact root and preserves descendant spelling and globs", () => { + expect(canonicalizer.canonicalize(root)).toBe(canonicalRoot) + expect(canonicalizer.canonicalize(`${root}/Docs/NOTES.md`)).toBe(`${canonicalRoot}/Docs/NOTES.md`) + expect(canonicalizer.canonicalize(`${root}/docs/**`)).toBe(`${canonicalRoot}/docs/**`) + }) + + it("does not translate similarly prefixed siblings or lexical parents", () => { + for (const value of [`${root}-other/docs/file.md`, `${root}2/file.md`, "/elsewhere/docs/file.md"]) { + expect(canonicalizer.canonicalize(value)).toBe(value) + } + }) + + it("leaves canonical-root paths and relative values unchanged", () => { + for (const value of [canonicalRoot, `${canonicalRoot}/docs/file.md`, "docs/file.md", "../shared/file.md"]) { + expect(canonicalizer.canonicalize(value)).toBe(value) + } + }) + + it.each([undefined, "", "relative/project", "C:relative"])( + "does not infer equivalence from an unknown or relative root: %s", + (invalidRoot) => { + const value = `${root}/docs/file.md` + expect( + WorkspaceRootCanonicalizer.forPlatform(invalidRoot, canonicalCwd, isWindows).canonicalize(value), + ).toBe(value) + expect(WorkspaceRootCanonicalizer.forPlatform(cwd, invalidRoot, isWindows).canonicalize(value)).toBe(value) + }, + ) +}) + +describe("POSIX workspace-root strategy", () => { + it("compares case-sensitively and keeps backslashes as filename characters", () => { + const canonicalizer = WorkspaceRootCanonicalizer.forStrategy( + "/aliases/My\\Project", + "/storage/My\\Project", + new PosixWorkspaceRootStrategy(), + ) + expect(canonicalizer.canonicalize("/aliases/My\\Project/docs/file.md")).toBe( + "/storage/My\\Project/docs/file.md", + ) + expect(canonicalizer.canonicalize("/aliases/my\\project/docs/file.md")).toBe( + "/aliases/my\\project/docs/file.md", + ) + expect(canonicalizer.canonicalize("/aliases/My/Project/docs/file.md")).toBe("/aliases/My/Project/docs/file.md") + }) + + it("normalizes root dot segments and trailing separators", () => { + const canonicalizer = WorkspaceRootCanonicalizer.forStrategy( + "/aliases/temporary/../project/", + "/storage/./project/", + new PosixWorkspaceRootStrategy(), + ) + expect(canonicalizer.canonicalize("/aliases/project/docs/file.md")).toBe("/storage/project/docs/file.md") + }) + + it("does not interpret Windows drive paths as absolute roots", () => { + const canonicalizer = WorkspaceRootCanonicalizer.forStrategy( + "C:/aliases/project", + "D:/storage/project", + new PosixWorkspaceRootStrategy(), + ) + expect(canonicalizer.canonicalize("/C/aliases/project/docs/file.md")).toBe("/C/aliases/project/docs/file.md") + }) +}) + +describe("Windows workspace-root strategy", () => { + it("compares case-insensitively across drives without changing descendant case", () => { + const canonicalizer = WorkspaceRootCanonicalizer.forStrategy( + "C:\\Aliases\\Project", + "D:\\Storage\\Project", + new WindowsWorkspaceRootStrategy(), + ) + expect(canonicalizer.canonicalize("/c/ALIASES/project/Docs/NOTES.md")).toBe("/D/Storage/Project/Docs/NOTES.md") + expect(canonicalizer.canonicalize("/E/Aliases/Project/Docs/NOTES.md")).toBe("/E/Aliases/Project/Docs/NOTES.md") + }) + + it("normalizes mixed root separators, dot segments, and trailing separators", () => { + const canonicalizer = WorkspaceRootCanonicalizer.forStrategy( + "C:\\Aliases/temporary/../Project\\", + "D:/Storage/./Project/", + new WindowsWorkspaceRootStrategy(), + ) + expect(canonicalizer.canonicalize("/C/Aliases/Project/docs/file.md")).toBe("/D/Storage/Project/docs/file.md") + }) + + it("supports drive roots and drive-less absolute roots in matcher notation", () => { + const strategy = new WindowsWorkspaceRootStrategy() + expect(WorkspaceRootCanonicalizer.forStrategy("C:", "D:/", strategy).canonicalize("/C/docs/file.md")).toBe( + "/D/docs/file.md", + ) + expect( + WorkspaceRootCanonicalizer.forStrategy("\\Aliases\\Project", "/Storage/Project", strategy).canonicalize( + "/Aliases/Project/docs/file.md", + ), + ).toBe("/Storage/Project/docs/file.md") + }) + + it("normalizes network roots into the shared UNC matcher namespace", () => { + const canonicalizer = WorkspaceRootCanonicalizer.forStrategy( + "\\\\server\\share\\project", + "\\\\server\\storage\\project", + new WindowsWorkspaceRootStrategy(), + ) + expect(canonicalizer.canonicalize("/UNC/SERVER/SHARE/project/docs/file.md")).toBe( + "/UNC/server/storage/project/docs/file.md", + ) + }) +}) + +it.each([false, true])("preserves filesystem-root equivalence (Windows=%s)", (isWindows) => { + expect(WorkspaceRootCanonicalizer.forPlatform("/", "/storage", isWindows).canonicalize("/docs/file.md")).toBe( + "/storage/docs/file.md", + ) + expect( + WorkspaceRootCanonicalizer.forPlatform("/aliases", "/", isWindows).canonicalize("/aliases/docs/file.md"), + ).toBe("/docs/file.md") +}) diff --git a/src/core/auto-approval/__tests__/WorkspaceRootStrategyFactory.spec.ts b/src/core/auto-approval/__tests__/WorkspaceRootStrategyFactory.spec.ts new file mode 100644 index 0000000000..6d36c5bf43 --- /dev/null +++ b/src/core/auto-approval/__tests__/WorkspaceRootStrategyFactory.spec.ts @@ -0,0 +1,62 @@ +import { isFileMatchedByPatterns } from "../filePatterns" +import type { WorkspaceRootStrategy } from "../strategies/WorkspaceRootStrategy" +import { WorkspaceRootStrategyFactory } from "../strategies/WorkspaceRootStrategyFactory" + +describe("WorkspaceRootStrategyFactory", () => { + afterEach(() => vi.restoreAllMocks()) + + it("selects POSIX behavior without interpreting drives, network prefixes, or backslashes", () => { + const strategy = WorkspaceRootStrategyFactory.create(false) + + expect(strategy.toMatcherAbsolutePath("/tmp/notes.md", "/C")).toBe("/tmp/notes.md") + expect(strategy.toMatcherAbsolutePath("//server/share/notes.md", "/C")).toBe("//server/share/notes.md") + expect(strategy.toMatcherAbsolutePath("/tmp/my\\file.md", "/C")).toBe("/tmp/my\\file.md") + expect(strategy.comparisonKey("/tmp/NOTES.md")).toBe("/tmp/NOTES.md") + }) + + it("selects Windows behavior for drives, rooted paths, and the distinct UNC namespace", () => { + const strategy = WorkspaceRootStrategyFactory.create(true) + + expect(strategy.toMatcherAbsolutePath("D:/tmp/notes.md", "/C")).toBe("/D/tmp/notes.md") + expect(strategy.toMatcherAbsolutePath("/tmp/notes.md", "/C")).toBe("/C/tmp/notes.md") + expect(strategy.toMatcherAbsolutePath("//server/share/notes.md", "/C")).toBe("/UNC/server/share/notes.md") + expect(strategy.toMatcherAbsolutePath("//C/share/notes.md", "/C")).not.toBe("/C/share/notes.md") + expect(strategy.comparisonKey("/D/Tmp/NOTES.md")).toBe("/d/tmp/notes.md") + }) + + it.each([false, true])("returns independent instances (Windows=%s)", (isWindows) => { + expect(WorkspaceRootStrategyFactory.create(isWindows)).not.toBe(WorkspaceRootStrategyFactory.create(isWindows)) + }) + + it("uses one injected strategy for candidates, every pattern, and both workspace roots", () => { + const toMatcherAbsolutePath = vi.fn((value: string) => value) + const normalizeAbsoluteRoot = vi + .fn() + .mockReturnValueOnce("/aliases/project") + .mockReturnValueOnce("/storage/project") + const comparisonKey = vi.fn((value: string) => value) + const strategy: WorkspaceRootStrategy = { toMatcherAbsolutePath, normalizeAbsoluteRoot, comparisonKey } + const create = vi.spyOn(WorkspaceRootStrategyFactory, "create").mockReturnValue(strategy) + + expect( + isFileMatchedByPatterns({ + filePath: "/storage/project/docs/notes.md", + cwd: "/aliases/project", + canonicalCwd: "/storage/project", + patterns: ["docs/**", "!/aliases/project/docs/private.md", "/aliases/project/docs/notes.md"], + isWindows: false, + }), + ).toBe(true) + + expect(create).toHaveBeenCalledOnce() + expect(create).toHaveBeenCalledWith(false) + expect(toMatcherAbsolutePath).toHaveBeenCalledWith("/storage/project/docs/notes.md", "") + expect(toMatcherAbsolutePath).toHaveBeenCalledWith("/aliases/project", "") + expect(toMatcherAbsolutePath).toHaveBeenCalledWith("/aliases/project/docs/private.md", "") + expect(toMatcherAbsolutePath).toHaveBeenCalledWith("/aliases/project/docs/notes.md", "") + expect(normalizeAbsoluteRoot).toHaveBeenCalledTimes(2) + expect(normalizeAbsoluteRoot).toHaveBeenNthCalledWith(1, "/aliases/project") + expect(normalizeAbsoluteRoot).toHaveBeenNthCalledWith(2, "/storage/project") + expect(comparisonKey).toHaveBeenCalled() + }) +}) diff --git a/src/core/auto-approval/__tests__/filePatterns.spec.ts b/src/core/auto-approval/__tests__/filePatterns.spec.ts index 6b1c3ff6b9..23ceaa1089 100644 --- a/src/core/auto-approval/__tests__/filePatterns.spec.ts +++ b/src/core/auto-approval/__tests__/filePatterns.spec.ts @@ -86,7 +86,7 @@ describe("toMatcherPattern", () => { // name, so `C:/tmp/notes.md` names a file inside it, relative to the workspace. describe("on Windows", () => { // The colon is dropped, leaving the drive as an ordinary leading path - // segment; see `toPosixAbsolutePath` for why it cannot stay. + // segment: the Windows strategy avoids drive-rooted paths rejected by `ignore`. it("keeps the drive of an absolute pattern as its first path segment", () => { expect(toWindowsPattern("D:/tmp/notes.md")).toBe("/D/tmp/notes.md") }) diff --git a/src/core/auto-approval/__tests__/windowsPaths.spec.ts b/src/core/auto-approval/__tests__/windowsPaths.spec.ts index acedfa726c..2b34f4eb96 100644 --- a/src/core/auto-approval/__tests__/windowsPaths.spec.ts +++ b/src/core/auto-approval/__tests__/windowsPaths.spec.ts @@ -30,6 +30,63 @@ describe("matching on Windows", () => { const matches = (filePath: string, patterns: string[], cwd: string | undefined = WINDOWS_CWD) => matcher().isFileMatchedByPatterns({ filePath, cwd, patterns, homeDir: WINDOWS_HOME }) + it.each(["\\\\server\\share\\project\\docs\\notes.md", "//server/share/project/docs/notes.md"])( + "matches an absolute UNC candidate against a relative grant: %s", + (filePath) => { + expect(matches(filePath, ["docs/**"], "\\\\server\\share\\project")).toBe(true) + }, + ) + + it("matches explicit UNC patterns from a local-drive workspace and without a workspace", () => { + const filePath = "\\\\server\\share\\project\\docs\\notes.md" + const patterns = ["//server/share/project/docs/**"] + expect(matches(filePath, patterns)).toBe(true) + expect(matcher().isFileMatchedByPatterns({ filePath, patterns, homeDir: WINDOWS_HOME })).toBe(true) + }) + + it.each([ + { cwd: "\\\\alias\\share\\project", canonicalCwd: "\\\\server\\storage\\project" }, + { cwd: "C:\\Aliases\\Project", canonicalCwd: "\\\\server\\storage\\project" }, + { cwd: "\\\\alias\\share\\project", canonicalCwd: "D:\\Storage\\Project" }, + ])("matches equivalent local and UNC roots ($cwd -> $canonicalCwd)", ({ cwd, canonicalCwd }) => { + const filePath = `${canonicalCwd}\\DOCS\\NOTES.md` + const rootMatch = (patterns: string[]) => + matcher().isFileMatchedByPatterns({ filePath, cwd, canonicalCwd, patterns, homeDir: WINDOWS_HOME }) + expect(rootMatch(["docs/**"])).toBe(true) + expect(rootMatch([`${cwd}/docs/**`])).toBe(true) + expect(rootMatch([`${canonicalCwd}/docs/**`])).toBe(true) + expect(rootMatch(["docs/**", `!${cwd}/docs/notes.md`])).toBe(false) + expect(rootMatch(["docs/**", `!${canonicalCwd}/docs/notes.md`])).toBe(false) + expect(rootMatch(["docs/**", `!${cwd}/docs/notes.md`, `${canonicalCwd}/docs/notes.md`])).toBe(true) + }) + + it("honors UNC exclusions for both relative and absolute candidates", () => { + const cwd = "\\\\server\\share\\project" + const patterns = ["docs/**", "!//server/share/project/docs/private.md"] + expect(matches("docs/private.md", patterns, cwd)).toBe(false) + expect(matches(`${cwd}\\docs\\private.md`, patterns, cwd)).toBe(false) + expect(matches(`${cwd}\\docs\\notes.md`, patterns, cwd)).toBe(true) + expect(matches("docs/private.md", [...patterns, "//server/share/project/docs/private.md"], cwd)).toBe(true) + }) + + it("keeps different UNC servers, shares, and similarly prefixed siblings separate", () => { + const cwd = "\\\\server\\share\\project" + for (const filePath of [ + "\\\\other-server\\share\\project\\docs\\notes.md", + "\\\\server\\other-share\\project\\docs\\notes.md", + "\\\\server\\share\\project-other\\docs\\notes.md", + ]) { + expect(matches(filePath, ["docs/**"], cwd)).toBe(false) + } + }) + + it("does not confuse a UNC server named C with the local C drive", () => { + expect(matches("C:\\share\\project\\docs\\notes.md", ["docs/**"], "\\\\C\\share\\project")).toBe(false) + expect(matches("\\\\C\\share\\project\\docs\\notes.md", ["docs/**"], "C:\\share\\project")).toBe(false) + expect(matches("C:\\share\\project\\docs\\notes.md", ["//C/share/project/docs/**"])).toBe(false) + expect(matches("\\\\C\\share\\project\\docs\\notes.md", ["C:/share/project/docs/**"])).toBe(false) + }) + it("matches a workspace-relative path against a bare pattern", () => { expect(matches("notes.md", ["notes.md"])).toBe(true) expect(matches("docs\\notes.md", ["notes.md"])).toBe(true) @@ -99,4 +156,34 @@ describe("matching on Windows", () => { expect(matches("C:/path/to/repo/etc/passwd", ["passwd"])).toBe(true) expect(matches("C:/etc/passwd", ["passwd"])).toBe(false) }) + + it("matches equivalent workspace roots across drives with Windows case and ordered exclusions", () => { + const rootMatch = (patterns: string[]) => + matcher().isFileMatchedByPatterns({ + filePath: "d:\\storage\\project\\DOCS\\NOTES.md", + cwd: "C:\\Aliases\\Project", + canonicalCwd: "D:\\Storage\\Project", + patterns, + }) + expect(rootMatch(["docs/**"])).toBe(true) + expect(rootMatch(["c:/aliases/PROJECT/docs/**"])).toBe(true) + expect(rootMatch(["d:/STORAGE/project/docs/**"])).toBe(true) + expect(rootMatch(["docs/**", "!c:/aliases/project/docs/notes.md"])).toBe(false) + expect(rootMatch(["docs/**", "!D:/Storage/Project/docs/notes.md"])).toBe(false) + }) + + it("keeps Windows siblings and lexical-parent outside patterns separate from canonical roots", () => { + const rootMatch = (filePath: string, patterns: string[]) => + matcher().isFileMatchedByPatterns({ + filePath, + cwd: "C:\\Aliases\\Project", + canonicalCwd: "D:\\Storage\\Project", + patterns, + }) + expect(rootMatch("C:/Aliases/shared/file.md", ["../shared/file.md"])).toBe(true) + expect(rootMatch("D:/Storage/shared/file.md", ["../shared/file.md"])).toBe(false) + expect(rootMatch("D:/Storage/shared/file.md", ["D:/Storage/shared/file.md"])).toBe(true) + expect(rootMatch("D:/Storage/Project-other/docs/file.md", ["docs/**"])).toBe(false) + expect(rootMatch("D:/Storage/Project-other/docs/file.md", ["C:/Aliases/Project-other/docs/**"])).toBe(false) + }) }) diff --git a/src/core/auto-approval/__tests__/workspaceRootEquivalence.spec.ts b/src/core/auto-approval/__tests__/workspaceRootEquivalence.spec.ts new file mode 100644 index 0000000000..91948c5eac --- /dev/null +++ b/src/core/auto-approval/__tests__/workspaceRootEquivalence.spec.ts @@ -0,0 +1,164 @@ +import * as fs from "fs/promises" +import type { ClineSayTool } from "@roo-code/types" +import { checkAutoApproval } from "../index" +import { isFileMatchedByPatterns } from "../filePatterns" +import { baseState, type State } from "./fixtures" + +vi.mock("fs/promises", async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, realpath: vi.fn() } +}) + +const CWD = "/aliases/project" +const CANONICAL_CWD = "/storage/project" + +function matches(filePath: string, patterns: string[]) { + return isFileMatchedByPatterns({ filePath, cwd: CWD, canonicalCwd: CANONICAL_CWD, patterns, isWindows: false }) +} + +function approval(tool: ClineSayTool, state: Partial, cwd: string | undefined = CWD, isProtected = false) { + return checkAutoApproval({ + state: { ...baseState, ...state }, + cwd, + ask: "tool", + text: JSON.stringify(tool), + isProtected, + }) +} + +beforeEach(() => { + vi.mocked(fs.realpath).mockReset().mockResolvedValue(CANONICAL_CWD) +}) + +afterEach(() => vi.restoreAllMocks()) + +describe("read matcher workspace-root equivalence", () => { + it.each([CWD, CANONICAL_CWD])("honors %s exclusions and later re-inclusion in one ordered matcher", (root) => { + const candidate = `${CANONICAL_CWD}/docs/private.md` + const exclusion = `!${root}/docs/private.md` + + expect(matches(candidate, ["docs/**", exclusion])).toBe(false) + expect(matches(candidate, [exclusion, "docs/**"])).toBe(true) + expect(matches(candidate, ["docs/**", exclusion, `${CWD}/docs/private.md`])).toBe(true) + }) + + it("resolves escaping patterns against the lexical parent, not the canonical parent", () => { + expect(matches("/aliases/shared/file.md", ["../shared/file.md"])).toBe(true) + expect(matches("/storage/shared/file.md", ["../shared/file.md"])).toBe(false) + expect(matches("/storage/shared/file.md", ["/storage/shared/file.md"])).toBe(true) + expect(matches("/aliases/shared/file.md", ["/storage/shared/file.md"])).toBe(false) + expect(matches(`${CANONICAL_CWD}/docs/file.md`, ["../project/docs/file.md"])).toBe(true) + }) + + it("does not translate similarly prefixed siblings or permit relative rules to cover other roots", () => { + expect(matches(`${CANONICAL_CWD}-other/docs/file.md`, ["docs/**"])).toBe(false) + expect(matches(`${CWD}-other/docs/file.md`, ["docs/**"])).toBe(false) + expect(matches(`${CANONICAL_CWD}-other/docs/file.md`, [`${CWD}-other/docs/**`])).toBe(false) + expect(matches(`${CANONICAL_CWD}/docs/file.md`, [`${CWD}-other/docs/**`])).toBe(false) + expect(matches(`${CWD}-other/docs/file.md`, [`${CWD}-other/docs/**`])).toBe(true) + }) +}) + +describe("read-only root equivalence approval", () => { + it("keeps read and write allowlist exclusions independent while writes still imply reads", async () => { + const tool: ClineSayTool = { tool: "readFile", path: `${CANONICAL_CWD}/docs/private.md` } + const excludedRead = ["docs/**", `!${CWD}/docs/private.md`] + const excludedWrite = ["docs/**", `!${CANONICAL_CWD}/docs/private.md`] + + expect(await approval(tool, { allowedReadFiles: excludedRead, allowedWriteFiles: excludedWrite })).toEqual({ + decision: "ask", + }) + expect(await approval(tool, { allowedReadFiles: excludedRead, allowedWriteFiles: ["docs/**"] })).toEqual({ + decision: "approve", + }) + expect(await approval(tool, { allowedReadFiles: ["docs/**"], allowedWriteFiles: excludedWrite })).toEqual({ + decision: "approve", + }) + }) + + it("does not extend write matching or protected-file permission when roots are equivalent", async () => { + const canonicalTool: ClineSayTool = { tool: "newFileCreated", path: `${CANONICAL_CWD}/AGENTS.md` } + expect(await approval(canonicalTool, { allowedReadFiles: ["*.md"], allowedWriteFiles: ["*.md"] })).toEqual({ + decision: "ask", + }) + expect(await approval(canonicalTool, { allowedWriteFiles: [`${CANONICAL_CWD}/AGENTS.md`] }, CWD, true)).toEqual( + { decision: "ask" }, + ) + expect( + await approval({ tool: "newFileCreated", path: "AGENTS.md" }, { allowedWriteFiles: ["*.md"] }, CWD, true), + ).toEqual({ decision: "ask" }) + expect( + await approval( + { tool: "listFilesRecursive", path: `${CANONICAL_CWD}/docs` }, + { allowedReadFiles: ["docs/**"] }, + ), + ).toEqual({ decision: "ask" }) + expect(fs.realpath).not.toHaveBeenCalled() + }) + + it("requires every named batch target to match and refuses unnamed targets", async () => { + const state = { allowedReadFiles: ["docs/**", `!${CWD}/docs/private.md`] } + const listed = { path: `${CANONICAL_CWD}/docs/notes.md`, lineSnippet: "", key: "notes" } + const excluded = { path: `${CANONICAL_CWD}/docs/private.md`, lineSnippet: "", key: "private" } + const outside = { path: "/elsewhere/docs/notes.md", lineSnippet: "", key: "outside" } + + expect( + await approval( + { + tool: "readFile", + batchFiles: [listed, { path: `${CWD}/docs/other.md`, lineSnippet: "", key: "other" }], + }, + state, + ), + ).toEqual({ decision: "approve" }) + expect(await approval({ tool: "readFile", batchFiles: [listed, excluded] }, state)).toEqual({ decision: "ask" }) + expect(await approval({ tool: "readFile", batchFiles: [listed, outside] }, state)).toEqual({ decision: "ask" }) + expect(await approval({ tool: "readFile", batchFiles: [listed], additionalFileCount: 1 }, state)).toEqual({ + decision: "ask", + }) + }) + + it.each(["ENOENT", "EACCES"])( + "does not infer root equivalence when task root resolution fails with %s", + async (code) => { + const error = Object.assign(new Error("root unavailable"), { code }) + const errorLog = vi.spyOn(console, "error").mockImplementation(() => {}) + vi.mocked(fs.realpath).mockRejectedValue(error) + const tool: ClineSayTool = { + tool: "readFile", + path: `${CANONICAL_CWD}/docs/notes.md`, + isOutsideWorkspace: true, + } + + expect(await approval(tool, { allowedReadFiles: ["docs/**"] })).toEqual({ decision: "ask" }) + expect(await approval(tool, { allowedReadFiles: [`${CWD}/docs/**`] })).toEqual({ decision: "ask" }) + expect(await approval(tool, { allowedReadFiles: [`${CANONICAL_CWD}/docs/**`] })).toEqual({ + decision: "approve", + }) + expect(errorLog).toHaveBeenCalledTimes(3) + expect(errorLog).toHaveBeenCalledWith(`[auto-approval] Failed to resolve workspace root ${CWD}:`, error) + }, + ) + + it.each([undefined, ""])("does not resolve workspace-relative grants without a task root: %s", async (cwd) => { + const errorLog = vi.spyOn(console, "error").mockImplementation(() => {}) + const text = JSON.stringify({ + tool: "readFile", + path: `${CANONICAL_CWD}/docs/notes.md`, + isOutsideWorkspace: true, + }) + expect( + await checkAutoApproval({ state: { ...baseState, allowedReadFiles: ["docs/**"] }, cwd, ask: "tool", text }), + ).toEqual({ decision: "ask" }) + expect( + await checkAutoApproval({ + state: { ...baseState, allowedReadFiles: [`${CANONICAL_CWD}/docs/**`] }, + cwd, + ask: "tool", + text, + }), + ).toEqual({ decision: "approve" }) + expect(fs.realpath).not.toHaveBeenCalled() + expect(errorLog).not.toHaveBeenCalled() + }) +}) diff --git a/src/core/auto-approval/filePatterns.ts b/src/core/auto-approval/filePatterns.ts index 923d29815d..372c0a3503 100644 --- a/src/core/auto-approval/filePatterns.ts +++ b/src/core/auto-approval/filePatterns.ts @@ -3,6 +3,10 @@ import path from "path" import ignore from "ignore" +import { WorkspaceRootCanonicalizer } from "./WorkspaceRootCanonicalizer" +import type { WorkspaceRootStrategy } from "./strategies/WorkspaceRootStrategy" +import { WorkspaceRootStrategyFactory } from "./strategies/WorkspaceRootStrategyFactory" + /** * Matching of file paths against user-configured file patterns. * @@ -72,7 +76,7 @@ import ignore from "ignore" * Both the configured patterns and the path being checked are therefore * rewritten into a single form the library accepts: a path relative to the * filesystem root (`/`), with any Windows drive as its first segment, in the - * spelling `toPosixAbsolutePath` gives it. + * spelling supplied by the selected path strategy. * * An alternative implementation would be to remember for each pattern whether * it's a workspace-scoped pattern or an absolute-filesystem pattern, and then @@ -164,41 +168,8 @@ function pathsepsToPosix(value: string, isWindows: boolean): string { const DRIVE_PREFIX = /^([a-zA-Z]):(?=\/|$)/ /** - * Rewrite an absolute path into the plain POSIX form the rest of this module - * works in, where a Windows drive is an ordinary first path segment. - * - * - `"C:/tmp/x"` -> `"/C/tmp/x"` - * - `"/tmp/x"` with `workspaceDrive = "/D"` -> `"/D/tmp/x"`, since Windows reads a - * path written without a drive as being on the current one - * - `"/tmp/x"` off Windows -> `"/tmp/x"`, where there are no drives to place it on - * - * The colon has to go, working around `ignore` behaviour: - * On Windows, `ignore` rejects any path that looks drive-rooted - * (it tests `/^[a-z]:\//i` and throws `RangeError`, - * "path should be a `path.relative()`d string"). - * - * The bare letter cannot be mistaken for a directory of the same name, because it - * only ever goes where Windows puts the drive, at the very root of the path: a - * directory called `C` always sits in a later segment. - * - * Its case is left as typed, since a drive only occurs on Windows, where the - * matcher ignores case anyway (see the case-sensitivity note at the top), so - * `C:/x` and `c:/x` already agree. - * - * A drive is only read as one when reading paths as Windows does. Elsewhere `C:` - * is an ordinary directory name (`mkdir 'C:'` succeeds on Linux), so `C:/notes.md` - * is a *relative* path naming a file in it, and taking it for a drive would anchor - * it to the filesystem root instead of the workspace. - */ -function toPosixAbsolutePath(absolutePath: string, workspaceDrive: string, isWindows: boolean): string { - const drive = isWindows ? DRIVE_PREFIX.exec(absolutePath) : null - - return drive ? `/${drive[1]}${absolutePath.slice(drive[0].length)}` : `${workspaceDrive}${absolutePath}` -} - -/** - * The drive a path is on, spelled as the path segment that `toPosixAbsolutePath` - * gives it: `"/C"` for `"C:/tmp/x"`. Empty when the path names no drive, as is + * The drive a path is on, spelled as the path segment used by the path strategy: + * `"/C"` for `"C:/tmp/x"`. Empty when the path names no drive, as is * always the case off Windows. */ function driveSegmentOf(absolutePath: string, isWindows: boolean): string { @@ -270,7 +241,7 @@ function toRootRelativePath(posixAbsolutePath: string): string { * - `"../shared/notes.md"` -> `"/path/to/shared/notes.md"` * resolved against the workspace root, so it lands outside the workspace * - `"~/notes.md"` -> `"/home/me/notes.md"` - * - `"C:/tmp/notes.md"` on Windows -> `"/C/tmp/notes.md"`, see `toPosixAbsolutePath()` + * - `"C:/tmp/notes.md"` on Windows -> `"/C/tmp/notes.md"`, using the path strategy * - `"!docs/secret.md"` -> `"!/path/to/repo/docs/secret.md"` * - `"mydir/"` -> `undefined` (because it names a dir) * - `"~"` -> `undefined` (because it names a dir) @@ -297,6 +268,22 @@ export function toMatcherPattern( cwd?: string, isWindows = runningOnWindows(), homeDir = os.homedir(), +): string | undefined { + return toMatcherPatternWithStrategy( + pattern, + cwd, + isWindows, + homeDir, + WorkspaceRootStrategyFactory.create(isWindows), + ) +} + +function toMatcherPatternWithStrategy( + pattern: string, + cwd: string | undefined, + isWindows: boolean, + homeDir: string, + strategy: WorkspaceRootStrategy, ): string | undefined { // Set gitignore's negation aside so the path is rewritten on its own merits, // then restore it, so that a negation is anchored exactly like the pattern it @@ -323,7 +310,7 @@ export function toMatcherPattern( } if (isAbsolutePath(normalized, isWindows)) { - return `${negation}${toPosixAbsolutePath(normalized, workspaceDrive, isWindows)}` + return `${negation}${strategy.toMatcherAbsolutePath(normalized, workspaceDrive)}` } if (!cwd) { @@ -334,7 +321,7 @@ export function toMatcherPattern( // `resolve` also normalises, so that a `cwd` with a trailing slash or a `..` // segment does not reach the patterns below. - const workspaceRoot = path.posix.resolve(toPosixAbsolutePath(workspacePosix, workspaceDrive, isWindows)) + const workspaceRoot = path.posix.resolve(strategy.toMatcherAbsolutePath(workspacePosix, workspaceDrive)) if (escapesWorkspace(normalized)) { return `${negation}${path.posix.resolve(workspaceRoot, normalized)}` @@ -365,7 +352,12 @@ export function toMatcherPattern( * @returns The rewritten path, or `undefined` when it names no file, or when it * is relative and there is no workspace root to resolve it against. */ -function toMatcherPath(filePath: string, cwd: string | undefined, isWindows: boolean): string | undefined { +function toMatcherPath( + filePath: string, + cwd: string | undefined, + isWindows: boolean, + strategy: WorkspaceRootStrategy, +): string | undefined { // Not trimmed: whitespace can be part of a filename. const normalized = pathsepsToPosix(filePath, isWindows) @@ -377,7 +369,7 @@ function toMatcherPath(filePath: string, cwd: string | undefined, isWindows: boo const workspaceDrive = driveSegmentOf(workspacePosix, isWindows) if (isAbsolutePath(normalized, isWindows)) { - return toRootRelativePath(toPosixAbsolutePath(normalized, workspaceDrive, isWindows)) + return toRootRelativePath(strategy.toMatcherAbsolutePath(normalized, workspaceDrive)) } if (!cwd) { @@ -387,7 +379,7 @@ function toMatcherPath(filePath: string, cwd: string | undefined, isWindows: boo return undefined } - const workspaceRoot = toPosixAbsolutePath(workspacePosix, workspaceDrive, isWindows) + const workspaceRoot = strategy.toMatcherAbsolutePath(workspacePosix, workspaceDrive) return toRootRelativePath(path.posix.resolve(workspaceRoot, normalized)) } @@ -411,12 +403,15 @@ function toMatcherPath(filePath: string, cwd: string | undefined, isWindows: boo export function isFileMatchedByPatterns({ filePath, cwd, + canonicalCwd, patterns, isWindows = runningOnWindows(), homeDir = os.homedir(), }: { filePath?: string cwd?: string + /** Verified realpath of cwd, used only by read approval. */ + canonicalCwd?: string patterns?: string[] isWindows?: boolean homeDir?: string @@ -425,15 +420,23 @@ export function isFileMatchedByPatterns({ return false } - const candidate = toMatcherPath(filePath, cwd, isWindows) + const strategy = WorkspaceRootStrategyFactory.create(isWindows) + const candidate = toMatcherPath(filePath, cwd, isWindows, strategy) if (!candidate) { return false } + const workspaceRoot = WorkspaceRootCanonicalizer.forStrategy(cwd, canonicalCwd, strategy) const matcherPatterns = patterns - .map((pattern) => toMatcherPattern(pattern, cwd, isWindows, homeDir)) + .map((pattern) => toMatcherPatternWithStrategy(pattern, cwd, isWindows, homeDir, strategy)) .filter((pattern): pattern is string => !!pattern) + .map((pattern) => { + const negation = pattern.startsWith("!") ? "!" : "" + // Resolve against the original cwd BEFORE translation: ../shared names + // its lexical parent, not the canonical workspace's parent. + return `${negation}${workspaceRoot.canonicalize(pattern.slice(negation.length))}` + }) if (!matcherPatterns.length) { return false @@ -442,7 +445,7 @@ export function isFileMatchedByPatterns({ try { return ignore({ ignoreCase: isWindows }) .add([...matcherPatterns, MATCH_DIRECTORIES_ONLY_PATTERN]) - .ignores(candidate) + .ignores(toRootRelativePath(workspaceRoot.canonicalize(`/${candidate}`))) } catch (error) { // A path the matcher rejects cannot be confirmed as matching, so treat it // as unmatched. diff --git a/src/core/auto-approval/index.ts b/src/core/auto-approval/index.ts index 2aa5008fc8..e92e609032 100644 --- a/src/core/auto-approval/index.ts +++ b/src/core/auto-approval/index.ts @@ -9,6 +9,7 @@ import { } from "@roo-code/types" import type { DcgDecision } from "../../services/destructive-command-guard/runner" +import { realpath } from "fs/promises" import { ClineAskResponse } from "../../shared/WebviewMessage" @@ -87,6 +88,17 @@ function areAllNamedFilesMatched(tool: ClineSayTool, matchFun: (filePath: string return paths.every((filePath) => matchFun(filePath)) } +async function resolveCanonicalWorkspaceRoot(cwd: string | undefined): Promise { + if (!cwd) return undefined + + try { + return await realpath(cwd) + } catch (error) { + console.error(`[auto-approval] Failed to resolve workspace root ${cwd}:`, error) + return undefined + } +} + /** * Whether a read-only tool action is fully covered by the read allowlist patterns. * @@ -105,20 +117,23 @@ function areAllNamedFilesMatched(tool: ClineSayTool, matchFun: (filePath: string * narrows the list it appears in. This also means that negating a read is * ineffective while a non-negated write pattern still matches the file. */ -function isReadAllowedByPatterns( +async function isReadAllowedByPatterns( tool: ClineSayTool, cwd: string | undefined, state: Pick, -): boolean { - if (tool.tool !== "readFile") { +): Promise { + if (tool.tool !== "readFile" || (!state.allowedReadFiles?.length && !state.allowedWriteFiles?.length)) { return false } + // Task-local root only. Unknown roots keep lexical matching; never resolve + // individual file aliases here, since those can point outside the workspace. + const canonicalCwd = await resolveCanonicalWorkspaceRoot(cwd) return areAllNamedFilesMatched( tool, (filePath) => - isFileMatchedByPatterns({ filePath, cwd, patterns: state.allowedReadFiles }) || - isFileMatchedByPatterns({ filePath, cwd, patterns: state.allowedWriteFiles }), + isFileMatchedByPatterns({ filePath, cwd, canonicalCwd, patterns: state.allowedReadFiles }) || + isFileMatchedByPatterns({ filePath, cwd, canonicalCwd, patterns: state.allowedWriteFiles }), ) } @@ -392,7 +407,7 @@ export async function checkAutoApproval({ // `alwaysAllowReadOnly` permission. Such a pattern names its // location, including outside the workspace, so it also stands in for // `alwaysAllowReadOnlyOutsideWorkspace`. - const isAllowedReadFile = isReadAllowedByPatterns(tool, cwd, state) + const isAllowedReadFile = await isReadAllowedByPatterns(tool, cwd, state) const isReadAllowed = isAllowedReadFile || diff --git a/src/core/auto-approval/strategies/PosixWorkspaceRootStrategy.ts b/src/core/auto-approval/strategies/PosixWorkspaceRootStrategy.ts new file mode 100644 index 0000000000..799f3e75c8 --- /dev/null +++ b/src/core/auto-approval/strategies/PosixWorkspaceRootStrategy.ts @@ -0,0 +1,19 @@ +import path from "path" + +import type { WorkspaceRootStrategy } from "./WorkspaceRootStrategy" + +export class PosixWorkspaceRootStrategy implements WorkspaceRootStrategy { + toMatcherAbsolutePath(absolutePath: string): string { + return absolutePath + } + + normalizeAbsoluteRoot(root: string | undefined): string | undefined { + if (!root?.startsWith("/")) return undefined + + return path.posix.resolve(this.toMatcherAbsolutePath(root)).replace(/\/$/, "") + } + + comparisonKey(value: string): string { + return value + } +} diff --git a/src/core/auto-approval/strategies/WindowsWorkspaceRootStrategy.ts b/src/core/auto-approval/strategies/WindowsWorkspaceRootStrategy.ts new file mode 100644 index 0000000000..b5fc1828f0 --- /dev/null +++ b/src/core/auto-approval/strategies/WindowsWorkspaceRootStrategy.ts @@ -0,0 +1,28 @@ +import path from "path" + +import type { WorkspaceRootStrategy } from "./WorkspaceRootStrategy" + +export class WindowsWorkspaceRootStrategy implements WorkspaceRootStrategy { + /** Drop drive colons rejected by the Windows matcher while keeping UNC paths in their own namespace. */ + toMatcherAbsolutePath(absolutePath: string, workspaceDrive: string): string { + // Keep UNC paths independent of the current drive and distinct from drive letters. + if (absolutePath.startsWith("//")) return `/UNC/${absolutePath.replace(/^\/+/, "")}` + + const drive = /^([a-zA-Z]):(?=\/|$)/.exec(absolutePath) + return drive ? `/${drive[1]}${absolutePath.slice(drive[0].length)}` : `${workspaceDrive}${absolutePath}` + } + + normalizeAbsoluteRoot(root: string | undefined): string | undefined { + const normalized = (root ?? "").replace(/\\/g, "/") + const drive = /^([a-zA-Z]):(?=\/|$)/.exec(normalized) + if (!normalized.startsWith("/") && !drive) return undefined + + // Match the allowlist representation: a Windows drive is a POSIX path segment. + const absolutePath = this.toMatcherAbsolutePath(normalized, "") + return path.posix.resolve(absolutePath).replace(/\/$/, "") + } + + comparisonKey(value: string): string { + return value.toLowerCase() + } +} diff --git a/src/core/auto-approval/strategies/WorkspaceRootStrategy.ts b/src/core/auto-approval/strategies/WorkspaceRootStrategy.ts new file mode 100644 index 0000000000..6c98a1724b --- /dev/null +++ b/src/core/auto-approval/strategies/WorkspaceRootStrategy.ts @@ -0,0 +1,6 @@ +export interface WorkspaceRootStrategy { + /** Convert a slash-normalized absolute path into the matcher's platform-specific namespace. */ + toMatcherAbsolutePath(absolutePath: string, workspaceDrive: string): string + normalizeAbsoluteRoot(root: string | undefined): string | undefined + comparisonKey(value: string): string +} diff --git a/src/core/auto-approval/strategies/WorkspaceRootStrategyFactory.ts b/src/core/auto-approval/strategies/WorkspaceRootStrategyFactory.ts new file mode 100644 index 0000000000..f39d583677 --- /dev/null +++ b/src/core/auto-approval/strategies/WorkspaceRootStrategyFactory.ts @@ -0,0 +1,9 @@ +import { PosixWorkspaceRootStrategy } from "./PosixWorkspaceRootStrategy" +import { WindowsWorkspaceRootStrategy } from "./WindowsWorkspaceRootStrategy" +import type { WorkspaceRootStrategy } from "./WorkspaceRootStrategy" + +export class WorkspaceRootStrategyFactory { + static create(isWindows: boolean): WorkspaceRootStrategy { + return isWindows ? new WindowsWorkspaceRootStrategy() : new PosixWorkspaceRootStrategy() + } +} diff --git a/src/core/tools/ReadFileTool.ts b/src/core/tools/ReadFileTool.ts index 00fc507692..a85ffc8c8d 100644 --- a/src/core/tools/ReadFileTool.ts +++ b/src/core/tools/ReadFileTool.ts @@ -20,7 +20,7 @@ import { formatResponse } from "../prompts/responses" import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { isPathOutsideWorkspace } from "../../utils/pathUtils" import { getReadablePath } from "../../utils/path" -import { extractTextFromFile, addLineNumbers, getSupportedBinaryFormats } from "../../integrations/misc/extract-text" +import { extractTextFromBuffer, addLineNumbers, getSupportedBinaryFormats } from "../../integrations/misc/extract-text" import { readWithIndentation, readWithSlice } from "../../integrations/misc/indentation-reader" import { DEFAULT_LINE_LIMIT } from "../prompts/tools/native-tools/read_file" import type { ToolUse, PushToolResult } from "../../shared/tools" @@ -34,6 +34,8 @@ import { ImageMemoryTracker, } from "./helpers/imageHelpers" import { BaseTool, ToolCallbacks } from "./BaseTool" +import { ReadFileTarget } from "./file-reading/ReadFileTarget" +import { ReadFileCancellation } from "./file-reading/ReadFileCancellation" // ─── Types ──────────────────────────────────────────────────────────────────── @@ -62,8 +64,10 @@ interface FileResult { imageDataUrl?: string feedbackText?: string feedbackImages?: string[] + approvalStatus?: "approved" | "denied" // Store the original entry for mode processing entry?: InternalFileEntry + target?: ReadFileTarget } // ─── Tool Implementation ────────────────────────────────────────────────────── @@ -84,6 +88,8 @@ export class ReadFileTool extends BaseTool<"read_file"> { * Execute new single-file format with slice/indentation mode support. */ private async executeNew(params: ReadFileParams, task: Task, callbacks: ToolCallbacks): Promise { + const cancellation = new ReadFileCancellation(task) + if (cancellation.isCancelled()) return const { pushToolResult } = callbacks const modelInfo = task.api.getModel().info const filePath = params.path @@ -148,7 +154,10 @@ export class ReadFileTool extends BaseTool<"read_file"> { // RooIgnore validation const accessAllowed = task.rooIgnoreController?.validateAccess(relPath) - if (!accessAllowed) { + if (accessAllowed) { + fileResult.target = await ReadFileTarget.resolve(path.resolve(task.cwd, relPath), cancellation) + } + if (!accessAllowed || !task.rooIgnoreController?.validateAccess(fileResult.target!.fullPath)) { await task.say("rooignore_error", relPath) const errorMsg = formatResponse.rooIgnoreError(relPath) updateFileResult(relPath, { @@ -163,7 +172,8 @@ export class ReadFileTool extends BaseTool<"read_file"> { } // Phase 2: Request user approval - await this.requestApproval(task, filesToApprove, updateFileResult) + await this.requestApproval(task, filesToApprove, updateFileResult, cancellation) + cancellation.throwIfCancelled() // Phase 3: Process approved files const imageMemoryTracker = new ImageMemoryTracker() @@ -181,49 +191,58 @@ export class ReadFileTool extends BaseTool<"read_file"> { const entry = fileResult.entry! try { - // Check if path is a directory - const stats = await fs.stat(fullPath) - if (stats.isDirectory()) { - const errorMsg = `Cannot read '${relPath}' because it is a directory. Use list_files tool instead.` - updateFileResult(relPath, { - status: "error", - error: errorMsg, - nativeContent: `File: ${relPath}\nError: ${errorMsg}`, - }) - await task.say("error", `Error reading file ${relPath}: ${errorMsg}`) - continue - } + await fileResult.target!.withHandle(async (file, stats) => { + // Check if path is a directory + if (stats.isDirectory()) { + const errorMsg = `Cannot read '${relPath}' because it is a directory. Use list_files tool instead.` + updateFileResult(relPath, { + status: "error", + error: errorMsg, + nativeContent: `File: ${relPath}\nError: ${errorMsg}`, + }) + await task.say("error", `Error reading file ${relPath}: ${errorMsg}`) + return + } - // Check for binary file - const isBinary = await isBinaryFile(fullPath) - - if (isBinary) { - await this.handleBinaryFile( - task, - relPath, - fullPath, - supportsImages, - maxImageFileSize, - maxTotalImageSize, - imageMemoryTracker, - updateFileResult, - ) - continue - } + // Check for binary file + const probe = Buffer.alloc(512) + const { bytesRead } = await file.read(probe, 0, probe.length, 0) + cancellation.throwIfCancelled() + const isBinary = await isBinaryFile(probe, bytesRead) + cancellation.throwIfCancelled() + + if (isBinary) { + await this.handleBinaryFile( + task, + relPath, + fullPath, + supportsImages, + maxImageFileSize, + maxTotalImageSize, + imageMemoryTracker, + updateFileResult, + file, + cancellation, + ) + return + } - // Read text file content with lossy UTF-8 conversion - // Reading as Buffer first allows graceful handling of non-UTF8 bytes - // (they become U+FFFD replacement characters instead of throwing) - const buffer = await fs.readFile(fullPath) - const fileContent = buffer.toString("utf-8") - const result = this.processTextFile(fileContent, entry) + // Read text file content with lossy UTF-8 conversion + // Reading as Buffer first allows graceful handling of non-UTF8 bytes + // (they become U+FFFD replacement characters instead of throwing) + const buffer = await file.readFile() + cancellation.throwIfCancelled() + const fileContent = buffer.toString("utf-8") + const result = this.processTextFile(fileContent, entry) - await task.fileContextTracker.trackFileContext(relPath, "read_tool" as RecordSource) + await task.fileContextTracker.trackFileContext(relPath, "read_tool" as RecordSource) - updateFileResult(relPath, { - nativeContent: `File: ${relPath}\n${result}`, + updateFileResult(relPath, { + nativeContent: `File: ${relPath}\n${result}`, + }) }) } catch (error) { + if (cancellation.isCancelled()) throw error const errorMsg = error instanceof Error ? error.message : String(error) updateFileResult(relPath, { status: "error", @@ -235,13 +254,18 @@ export class ReadFileTool extends BaseTool<"read_file"> { } // Phase 4: Build and return result + cancellation.throwIfCancelled() const hasErrors = fileResults.some((r) => r.status === "error" || r.status === "blocked") if (hasErrors) { task.didToolFailInCurrentTurn = true } - this.buildAndPushResult(task, fileResults, pushToolResult) + this.buildAndPushResult(task, fileResults, pushToolResult, cancellation) } catch (error) { + if (cancellation.isCancelled()) { + this.buildAndPushResult(task, fileResults, pushToolResult, cancellation) + return + } const relPath = filePath || "unknown" const errorMsg = error instanceof Error ? error.message : String(error) @@ -251,7 +275,15 @@ export class ReadFileTool extends BaseTool<"read_file"> { nativeContent: `File: ${relPath}\nError: ${errorMsg}`, }) - await task.say("error", `Error reading file ${relPath}: ${errorMsg}`) + try { + await task.say("error", `Error reading file ${relPath}: ${errorMsg}`) + } catch (diagnosticError) { + if (!cancellation.isCancelled()) throw diagnosticError + } + if (cancellation.isCancelled()) { + this.buildAndPushResult(task, fileResults, pushToolResult, cancellation) + return + } task.didToolFailInCurrentTurn = true const errorResult = fileResults @@ -341,6 +373,8 @@ export class ReadFileTool extends BaseTool<"read_file"> { maxTotalImageSize: number, imageMemoryTracker: ImageMemoryTracker, updateFileResult: (path: string, updates: Partial) => void, + file: fs.FileHandle, + cancellation: ReadFileCancellation, ): Promise { const fileExtension = path.extname(relPath).toLowerCase() const supportedBinaryFormats = getSupportedBinaryFormats() @@ -354,7 +388,9 @@ export class ReadFileTool extends BaseTool<"read_file"> { maxImageFileSize, maxTotalImageSize, imageMemoryTracker.getTotalMemoryUsed(), + file, ) + cancellation.throwIfCancelled() if (!validationResult.isValid) { await task.fileContextTracker.trackFileContext(relPath, "read_tool" as RecordSource) @@ -364,7 +400,8 @@ export class ReadFileTool extends BaseTool<"read_file"> { return } - const imageResult = await processImageFile(fullPath) + const imageResult = await processImageFile(fullPath, file, cancellation) + cancellation.throwIfCancelled() imageMemoryTracker.addMemoryUsage(imageResult.sizeInMB) await task.fileContextTracker.trackFileContext(relPath, "read_tool" as RecordSource) @@ -374,6 +411,7 @@ export class ReadFileTool extends BaseTool<"read_file"> { }) return } catch (error) { + if (cancellation.isCancelled()) throw error const errorMsg = error instanceof Error ? error.message : String(error) updateFileResult(relPath, { status: "error", @@ -388,7 +426,10 @@ export class ReadFileTool extends BaseTool<"read_file"> { // Handle other supported binary formats (PDF, DOCX, etc.) if (supportedBinaryFormats && supportedBinaryFormats.includes(fileExtension)) { try { - const content = await extractTextFromFile(fullPath) + const source = await file.readFile() + cancellation.throwIfCancelled() + const content = await extractTextFromBuffer(source, fullPath) + cancellation.throwIfCancelled() const numberedContent = addLineNumbers(content) const lineCount = content.split("\n").length @@ -402,6 +443,7 @@ export class ReadFileTool extends BaseTool<"read_file"> { }) return } catch (error) { + if (cancellation.isCancelled()) throw error const errorMsg = error instanceof Error ? error.message : String(error) updateFileResult(relPath, { status: "error", @@ -428,8 +470,10 @@ export class ReadFileTool extends BaseTool<"read_file"> { task: Task, filesToApprove: FileResult[], updateFileResult: (path: string, updates: Partial) => void, + cancellation: ReadFileCancellation, ): Promise { if (filesToApprove.length === 0) return + cancellation.throwIfCancelled() if (filesToApprove.length > 1) { // Batch approval @@ -500,15 +544,16 @@ export class ReadFileTool extends BaseTool<"read_file"> { // Single file approval const fileResult = filesToApprove[0] const relPath = fileResult.path - const fullPath = path.resolve(task.cwd, relPath) - const isOutsideWorkspace = isPathOutsideWorkspace(fullPath) + const fullPath = fileResult.target!.fullPath + const isOutsideWorkspace = await ReadFileTarget.isOutsideWorkspace(fullPath) + cancellation.throwIfCancelled() const lineSnippet = this.getLineSnippet(fileResult.entry!) const startLine = this.getStartLine(fileResult.entry!) const completeMessage = JSON.stringify({ tool: "readFile", - path: getReadablePath(task.cwd, relPath), + path: getReadablePath(task.cwd, fullPath), isOutsideWorkspace, content: fullPath, reason: lineSnippet, @@ -517,8 +562,16 @@ export class ReadFileTool extends BaseTool<"read_file"> { const { response, text, images } = await task.ask("tool", completeMessage, false) + updateFileResult(relPath, { + approvalStatus: response === "yesButtonClicked" ? "approved" : "denied", + feedbackText: text, + feedbackImages: images, + }) + cancellation.throwIfCancelled() + if (text) await task.say("user_feedback", text, images) + cancellation.throwIfCancelled() + if (response !== "yesButtonClicked") { - if (text) await task.say("user_feedback", text, images) task.didRejectTool = true updateFileResult(relPath, { status: "denied", @@ -527,7 +580,6 @@ export class ReadFileTool extends BaseTool<"read_file"> { feedbackImages: images, }) } else { - if (text) await task.say("user_feedback", text, images) updateFileResult(relPath, { status: "approved", feedbackText: text, feedbackImages: images }) } } @@ -569,7 +621,22 @@ export class ReadFileTool extends BaseTool<"read_file"> { /** * Build and push the final result to the tool output. */ - private buildAndPushResult(task: Task, fileResults: FileResult[], pushToolResult: PushToolResult): void { + private buildAndPushResult( + task: Task, + fileResults: FileResult[], + pushToolResult: PushToolResult, + cancellation: ReadFileCancellation, + ): void { + const cancelled = cancellation.isCancelled() + if (cancelled) { + fileResults = fileResults.map((result) => ({ + ...result, + status: result.approvalStatus ?? "pending", + nativeContent: undefined, + imageDataUrl: undefined, + })) + if (!fileResults.some((result) => result.feedbackText || result.feedbackImages?.length)) return + } const finalResult = fileResults .filter((r) => r.nativeContent) .map((r) => r.nativeContent) @@ -578,7 +645,7 @@ export class ReadFileTool extends BaseTool<"read_file"> { const fileImageUrls = fileResults.filter((r) => r.imageDataUrl).map((r) => r.imageDataUrl as string) let statusMessage = "" - let feedbackImages: string[] = [] + let feedbackImages: string[] = cancelled ? fileResults.flatMap((result) => result.feedbackImages ?? []) : [] const deniedWithFeedback = fileResults.find((r) => r.status === "denied" && r.feedbackText) @@ -665,151 +732,205 @@ export class ReadFileTool extends BaseTool<"read_file"> { * This handles the old format: { files: [{ path: string, lineRanges?: [...] }] } */ private async executeLegacy(fileEntries: FileEntry[], task: Task, callbacks: ToolCallbacks): Promise { + const cancellation = new ReadFileCancellation(task) + if (cancellation.isCancelled()) return const { pushToolResult } = callbacks const modelInfo = task.api.getModel().info - - if (!fileEntries || fileEntries.length === 0) { - task.consecutiveMistakeCount++ - task.recordToolError("read_file") - const errorMsg = await task.sayAndCreateMissingParamError("read_file", "files") - pushToolResult(`Error: ${errorMsg}`) - return - } - - const supportsImages = modelInfo.supportsImages ?? false - - // Process each file sequentially (legacy behavior) const results: string[] = [] + const approvalResults: FileResult[] = [] - for (const entry of fileEntries) { - const relPath = entry.path - const fullPath = path.resolve(task.cwd, relPath) - - // RooIgnore validation - const accessAllowed = task.rooIgnoreController?.validateAccess(relPath) - if (!accessAllowed) { - await task.say("rooignore_error", relPath) - const errorMsg = formatResponse.rooIgnoreError(relPath) - results.push(`File: ${relPath}\nError: ${errorMsg}`) - // Mirror the native path: a blocked file marks the tool turn as failed. - task.didToolFailInCurrentTurn = true - continue + try { + if (!fileEntries || fileEntries.length === 0) { + task.consecutiveMistakeCount++ + task.recordToolError("read_file") + const errorMsg = await cancellation.run(() => task.sayAndCreateMissingParamError("read_file", "files")) + pushToolResult(`Error: ${errorMsg}`) + return } - // Request approval for single file - const isOutsideWorkspace = isPathOutsideWorkspace(fullPath) - let lineSnippet = "" - if (entry.lineRanges && entry.lineRanges.length > 0) { - const ranges = entry.lineRanges.map((range: LineRange) => `(lines ${range.start}-${range.end})`) - lineSnippet = ranges.join(", ") - } + const supportsImages = modelInfo.supportsImages ?? false - const completeMessage = JSON.stringify({ - tool: "readFile", - path: getReadablePath(task.cwd, relPath), - isOutsideWorkspace, - content: fullPath, - reason: lineSnippet || undefined, - } satisfies ClineSayTool) - - const { response, text, images } = await task.ask("tool", completeMessage, false) - - if (response !== "yesButtonClicked") { - if (text) await task.say("user_feedback", text, images) - task.didRejectTool = true - results.push(`File: ${relPath}\nStatus: Denied by user`) - continue - } - - if (text) await task.say("user_feedback", text, images) + // Process each file sequentially (legacy behavior). + for (const entry of fileEntries) { + cancellation.throwIfCancelled() + const relPath = entry.path + const fullPath = path.resolve(task.cwd, relPath) - try { - // Check if the path is a directory - const stats = await fs.stat(fullPath) - if (stats.isDirectory()) { - const errorMsg = `Cannot read '${relPath}' because it is a directory.` + // RooIgnore validation + const accessAllowed = task.rooIgnoreController?.validateAccess(relPath) + let target: ReadFileTarget | undefined + try { + if (accessAllowed) { + target = await ReadFileTarget.resolve(fullPath, cancellation) + } + } catch (error) { + if (cancellation.isCancelled()) throw error + const errorMsg = error instanceof Error ? error.message : String(error) results.push(`File: ${relPath}\nError: ${errorMsg}`) - await task.say("error", `Error reading file ${relPath}: ${errorMsg}`) - // Mirror the native path: a failed read marks the tool turn as failed. + await cancellation.run(() => task.say("error", `Error reading file ${relPath}: ${errorMsg}`)) + task.didToolFailInCurrentTurn = true + continue + } + if (!accessAllowed || !task.rooIgnoreController?.validateAccess(target!.fullPath)) { + await cancellation.run(() => task.say("rooignore_error", relPath)) + const errorMsg = formatResponse.rooIgnoreError(relPath) + results.push(`File: ${relPath}\nError: ${errorMsg}`) + // Mirror the native path: a blocked file marks the tool turn as failed. task.didToolFailInCurrentTurn = true continue } - const isBinary = await isBinaryFile(fullPath).catch(() => false) - - if (isBinary) { - // Handle binary files (images) - const fileExtension = path.extname(relPath).toLowerCase() - if (supportsImages && isSupportedImageFormat(fileExtension)) { - const state = await task.providerRef.deref()?.getState() - const { - maxImageFileSize = DEFAULT_MAX_IMAGE_FILE_SIZE_MB, - maxTotalImageSize = DEFAULT_MAX_TOTAL_IMAGE_SIZE_MB, - } = state ?? {} - const validation = await validateImageForProcessing( - fullPath, - supportsImages, - maxImageFileSize, - maxTotalImageSize, - 0, // Legacy path doesn't track cumulative memory - ) - if (!validation.isValid) { - results.push(`File: ${relPath}\nNotice: ${validation.notice ?? "Image validation failed"}`) - continue - } - const imageResult = await processImageFile(fullPath) - if (imageResult) { - results.push(`File: ${relPath}\n[Image file - content processed for vision model]`) - } - } else { - results.push(`File: ${relPath}\nError: Cannot read binary file`) - } + // Request approval for single file + const isOutsideWorkspace = await cancellation.run(() => + ReadFileTarget.isOutsideWorkspace(target!.fullPath), + ) + let lineSnippet = "" + if (entry.lineRanges && entry.lineRanges.length > 0) { + const ranges = entry.lineRanges.map((range: LineRange) => `(lines ${range.start}-${range.end})`) + lineSnippet = ranges.join(", ") + } + + const completeMessage = JSON.stringify({ + tool: "readFile", + path: getReadablePath(task.cwd, target!.fullPath), + isOutsideWorkspace, + content: target!.fullPath, + reason: lineSnippet || undefined, + } satisfies ClineSayTool) + + cancellation.throwIfCancelled() + const { response, text, images } = await task.ask("tool", completeMessage, false) + approvalResults.push({ + path: relPath, + status: "pending", + approvalStatus: response === "yesButtonClicked" ? "approved" : "denied", + feedbackText: text, + feedbackImages: images, + }) + cancellation.throwIfCancelled() + + if (response !== "yesButtonClicked") { + if (text) await cancellation.run(() => task.say("user_feedback", text, images)) + task.didRejectTool = true + results.push(`File: ${relPath}\nStatus: Denied by user`) continue } - // Read text file - const rawContent = await fs.readFile(fullPath, "utf8") + if (text) await cancellation.run(() => task.say("user_feedback", text, images)) - // Handle line ranges if specified - let content: string - if (entry.lineRanges && entry.lineRanges.length > 0) { - const lines = rawContent.split("\n") - const selectedLines: string[] = [] + try { + await target!.withHandle(async (file, stats) => { + // Check if the path is a directory + if (stats.isDirectory()) { + const errorMsg = `Cannot read '${relPath}' because it is a directory.` + results.push(`File: ${relPath}\nError: ${errorMsg}`) + await cancellation.run(() => + task.say("error", `Error reading file ${relPath}: ${errorMsg}`), + ) + // Mirror the native path: a failed read marks the tool turn as failed. + task.didToolFailInCurrentTurn = true + return + } - for (const range of entry.lineRanges) { - // Convert to 0-based index, ranges are 1-based inclusive - const startIdx = Math.max(0, range.start - 1) - const endIdx = Math.min(lines.length - 1, range.end - 1) + let isBinary = false + try { + const probe = Buffer.alloc(512) + const { bytesRead } = await cancellation.run(() => file.read(probe, 0, probe.length, 0)) + isBinary = await cancellation.run(() => isBinaryFile(probe, bytesRead)) + } catch (error) { + if (cancellation.isCancelled()) throw error + // Preserve the historical legacy text fallback when binary probing fails. + } - for (let i = startIdx; i <= endIdx; i++) { - selectedLines.push(`${i + 1} | ${lines[i]}`) + if (isBinary) { + // Handle binary files (images) + const fileExtension = path.extname(relPath).toLowerCase() + if (supportsImages && isSupportedImageFormat(fileExtension)) { + const state = await cancellation.run(async () => task.providerRef.deref()?.getState()) + const { + maxImageFileSize = DEFAULT_MAX_IMAGE_FILE_SIZE_MB, + maxTotalImageSize = DEFAULT_MAX_TOTAL_IMAGE_SIZE_MB, + } = state ?? {} + const validation = await cancellation.run(() => + validateImageForProcessing( + fullPath, + supportsImages, + maxImageFileSize, + maxTotalImageSize, + 0, // Legacy path doesn't track cumulative memory + file, + ), + ) + if (!validation.isValid) { + results.push( + `File: ${relPath}\nNotice: ${validation.notice ?? "Image validation failed"}`, + ) + return + } + const imageResult = await cancellation.run(() => + processImageFile(fullPath, file, cancellation), + ) + if (imageResult) { + results.push(`File: ${relPath}\n[Image file - content processed for vision model]`) + } + } else { + results.push(`File: ${relPath}\nError: Cannot read binary file`) + } + return } - } - content = selectedLines.join("\n") - } else { - // Read with default limits using slice mode - const result = readWithSlice(rawContent, 0, DEFAULT_LINE_LIMIT) - content = result.content - if (result.wasTruncated) { - content += `\n\n[File truncated: showing ${result.returnedLines} of ${result.totalLines} total lines]` - } - } - results.push(`File: ${relPath}\n${content}`) + // Read text file + const rawContent = await cancellation.run(() => file.readFile("utf8")) + + // Handle line ranges if specified + let content: string + if (entry.lineRanges && entry.lineRanges.length > 0) { + const lines = rawContent.split("\n") + const selectedLines: string[] = [] + + for (const range of entry.lineRanges) { + // Convert to 0-based index, ranges are 1-based inclusive + const startIdx = Math.max(0, range.start - 1) + const endIdx = Math.min(lines.length - 1, range.end - 1) + + for (let i = startIdx; i <= endIdx; i++) { + selectedLines.push(`${i + 1} | ${lines[i]}`) + } + } + content = selectedLines.join("\n") + } else { + // Read with default limits using slice mode + const result = readWithSlice(rawContent, 0, DEFAULT_LINE_LIMIT) + content = result.content + if (result.wasTruncated) { + content += `\n\n[File truncated: showing ${result.returnedLines} of ${result.totalLines} total lines]` + } + } - // Track file in context - await task.fileContextTracker.trackFileContext(relPath, "read_tool") - } catch (error) { - const errorMsg = error instanceof Error ? error.message : String(error) - results.push(`File: ${relPath}\nError: ${errorMsg}`) - await task.say("error", `Error reading file ${relPath}: ${errorMsg}`) - // Mirror the native path: a failed read marks the tool turn as failed. - task.didToolFailInCurrentTurn = true + // Track file in context + await cancellation.run(() => task.fileContextTracker.trackFileContext(relPath, "read_tool")) + results.push(`File: ${relPath}\n${content}`) + }) + } catch (error) { + if (cancellation.isCancelled()) throw error + const errorMsg = error instanceof Error ? error.message : String(error) + results.push(`File: ${relPath}\nError: ${errorMsg}`) + await cancellation.run(() => task.say("error", `Error reading file ${relPath}: ${errorMsg}`)) + // Mirror the native path: a failed read marks the tool turn as failed. + task.didToolFailInCurrentTurn = true + } } - } - // Push combined results - pushToolResult(results.join("\n\n---\n\n")) + // Push combined results + cancellation.throwIfCancelled() + pushToolResult(results.join("\n\n---\n\n")) + } catch (error) { + if (cancellation.isCancelled()) { + this.buildAndPushResult(task, approvalResults, pushToolResult, cancellation) + return + } + throw error + } } } diff --git a/src/core/tools/__tests__/readFileTool.spec.ts b/src/core/tools/__tests__/readFileTool.spec.ts index 5f9033eb7c..fb79f1b819 100644 --- a/src/core/tools/__tests__/readFileTool.spec.ts +++ b/src/core/tools/__tests__/readFileTool.spec.ts @@ -27,7 +27,11 @@ import { isSupportedImageFormat, ImageMemoryTracker, } from "../helpers/imageHelpers" -import { extractTextFromFile, addLineNumbers, getSupportedBinaryFormats } from "../../../integrations/misc/extract-text" +import { + extractTextFromBuffer, + addLineNumbers, + getSupportedBinaryFormats, +} from "../../../integrations/misc/extract-text" import { readWithIndentation, readWithSlice } from "../../../integrations/misc/indentation-reader" // ─── Mocks ──────────────────────────────────────────────────────────────────── @@ -46,12 +50,23 @@ vi.mock("path", async () => { vi.mock("fs/promises", () => ({ readFile: vi.fn(), stat: vi.fn(), + realpath: vi.fn(async (file: string) => file), + lstat: vi.fn(async () => ({ dev: 1n, ino: 1n, isSymbolicLink: () => false })), + open: vi.fn(async (file: string) => { + const mocked = await import("fs/promises") + return { + stat: async () => ({ ...(await mocked.stat(file)), dev: 1n, ino: 1n }), + read: async () => ({ bytesRead: 0 }), + readFile: async (encoding?: "utf8") => (encoding ? mocked.readFile(file, encoding) : mocked.readFile(file)), + close: vi.fn().mockResolvedValue(undefined), + } + }), })) vi.mock("isbinaryfile") vi.mock("../../../integrations/misc/extract-text", () => ({ - extractTextFromFile: vi.fn(), + extractTextFromBuffer: vi.fn(), addLineNumbers: vi.fn().mockImplementation((text: string, startLine = 1) => { if (!text) return "" const lines = text.split("\n") @@ -125,7 +140,7 @@ const mockedFsReadFile = vi.mocked(fsPromises.readFile) const mockedFsStat = vi.mocked(fsPromises.stat) const mockedIsBinaryFile = vi.mocked(isBinaryFile) -const mockedExtractTextFromFile = vi.mocked(extractTextFromFile) +const mockedExtractTextFromBuffer = vi.mocked(extractTextFromBuffer) const mockedReadWithSlice = vi.mocked(readWithSlice) const mockedReadWithIndentation = vi.mocked(readWithIndentation) const mockedIsSupportedImageFormat = vi.mocked(isSupportedImageFormat) @@ -426,11 +441,11 @@ describe("ReadFileTool", () => { const mockTask = createMockTask() const callbacks = createMockCallbacks() - mockedExtractTextFromFile.mockResolvedValue("PDF content here") + mockedExtractTextFromBuffer.mockResolvedValue("PDF content here") await readFileTool.execute({ path: "document.pdf" }, mockTask as any, callbacks) - expect(mockedExtractTextFromFile).toHaveBeenCalled() + expect(mockedExtractTextFromBuffer).toHaveBeenCalled() expect(callbacks.pushToolResult).toHaveBeenCalledWith(expect.stringContaining("PDF content here")) }) @@ -438,11 +453,11 @@ describe("ReadFileTool", () => { const mockTask = createMockTask() const callbacks = createMockCallbacks() - mockedExtractTextFromFile.mockResolvedValue("DOCX content here") + mockedExtractTextFromBuffer.mockResolvedValue("DOCX content here") await readFileTool.execute({ path: "document.docx" }, mockTask as any, callbacks) - expect(mockedExtractTextFromFile).toHaveBeenCalled() + expect(mockedExtractTextFromBuffer).toHaveBeenCalled() expect(callbacks.pushToolResult).toHaveBeenCalledWith(expect.stringContaining("DOCX content here")) }) @@ -462,7 +477,7 @@ describe("ReadFileTool", () => { const mockTask = createMockTask() const callbacks = createMockCallbacks() - mockedExtractTextFromFile.mockRejectedValue(new Error("Extraction failed")) + mockedExtractTextFromBuffer.mockRejectedValue(new Error("Extraction failed")) await readFileTool.execute({ path: "corrupt.pdf" }, mockTask as any, callbacks) @@ -1013,7 +1028,7 @@ describe("ReadFileTool", () => { ) expect(mockedFsReadFile).not.toHaveBeenCalled() expect(mockedFsStat).not.toHaveBeenCalled() - expect(mockedExtractTextFromFile).not.toHaveBeenCalled() + expect(mockedExtractTextFromBuffer).not.toHaveBeenCalled() }, ) @@ -1038,7 +1053,7 @@ describe("ReadFileTool", () => { ) expect(mockedFsReadFile).not.toHaveBeenCalled() expect(mockedFsStat).not.toHaveBeenCalled() - expect(mockedExtractTextFromFile).not.toHaveBeenCalled() + expect(mockedExtractTextFromBuffer).not.toHaveBeenCalled() }) it.each(["src/app.ts", "src/файл.ts", "src/e\u0301-📚.ts"])( diff --git a/src/core/tools/file-reading/ReadFileCancellation.ts b/src/core/tools/file-reading/ReadFileCancellation.ts new file mode 100644 index 0000000000..1f97b6a0e9 --- /dev/null +++ b/src/core/tools/file-reading/ReadFileCancellation.ts @@ -0,0 +1,33 @@ +import type { Task } from "../../task/Task" + +import { ReadFileCancelledError } from "./ReadFileCancelledError" + +/** Observe existing task cancellation flags; never change lifecycle state. */ +export class ReadFileCancellation { + constructor(private readonly task: Pick) {} + + isCancelled(): boolean { + return this.task.abort || this.task.abandoned + } + + throwIfCancelled(): void { + if (this.isCancelled()) throw new ReadFileCancelledError() + } + + /** Guard value-producing operations; resource acquisition needs its own cleanup scope. */ + async run(operation: () => Promise): Promise { + this.throwIfCancelled() + + let result: T + try { + result = await operation() + } catch (error) { + if (error instanceof ReadFileCancelledError) throw error + this.throwIfCancelled() + throw error + } + + this.throwIfCancelled() + return result + } +} diff --git a/src/core/tools/file-reading/ReadFileCancelledError.ts b/src/core/tools/file-reading/ReadFileCancelledError.ts new file mode 100644 index 0000000000..a71035060c --- /dev/null +++ b/src/core/tools/file-reading/ReadFileCancelledError.ts @@ -0,0 +1,6 @@ +export class ReadFileCancelledError extends Error { + constructor() { + super("File read cancelled") + this.name = "ReadFileCancelledError" + } +} diff --git a/src/core/tools/file-reading/ReadFileTarget.ts b/src/core/tools/file-reading/ReadFileTarget.ts new file mode 100644 index 0000000000..fad8136c32 --- /dev/null +++ b/src/core/tools/file-reading/ReadFileTarget.ts @@ -0,0 +1,69 @@ +import * as fs from "fs/promises" +import { constants, type BigIntStats } from "fs" +import path from "path" +import * as vscode from "vscode" + +import type { ReadFileCancellation } from "./ReadFileCancellation" +import { ReadFileCancelledError } from "./ReadFileCancelledError" + +/** Metadata only until approval; content consumers must never reopen fullPath. */ +export class ReadFileTarget { + private constructor( + readonly fullPath: string, + private readonly cancellation: ReadFileCancellation, + private readonly identity?: BigIntStats, + private readonly missingError?: unknown, + ) {} + + static async resolve(lexicalPath: string, cancellation: ReadFileCancellation): Promise { + try { + const fullPath = await cancellation.run(() => fs.realpath(lexicalPath)) + const identity = await cancellation.run(() => fs.lstat(fullPath, { bigint: true })) + if (identity.isSymbolicLink()) { + throw new Error(`File target changed before approval: ${lexicalPath}`) + } + const revalidatedPath = await cancellation.run(() => fs.realpath(fullPath)) + if (revalidatedPath !== fullPath) { + throw new Error(`File target changed before approval: ${lexicalPath}`) + } + return new ReadFileTarget(fullPath, cancellation, identity) + } catch (error) { + if (error instanceof ReadFileCancelledError) throw error + // Filesystem diagnostics, including inaccessible parents, require approval. + // Code-less identity races and programmer errors must still fail closed. + if (error instanceof Error && "code" in error && typeof error.code === "string") { + return new ReadFileTarget(lexicalPath, cancellation, undefined, error) + } + throw error + } + } + + static async isOutsideWorkspace(fullPath: string): Promise { + const roots = await Promise.all( + (vscode.workspace.workspaceFolders ?? []).map((folder) => + fs.realpath(folder.uri.fsPath).catch(() => undefined), + ), + ) + return !roots.some((root) => { + if (!root) return false + const relative = path.relative(root, fullPath) + return relative !== ".." && !relative.startsWith(`..${path.sep}`) && !path.isAbsolute(relative) + }) + } + + async withHandle(read: (file: fs.FileHandle, stats: BigIntStats) => Promise): Promise { + return this.cancellation.run(async () => { + if (!this.identity) throw this.missingError + const file = await fs.open(this.fullPath, constants.O_RDONLY | (constants.O_NOFOLLOW ?? 0)) + try { + const stats = await this.cancellation.run(() => file.stat({ bigint: true })) + if (stats.dev !== this.identity.dev || stats.ino !== this.identity.ino) { + throw new Error(`File target changed after approval: ${this.fullPath}`) + } + return await read(file, stats) + } finally { + await file.close() + } + }) + } +} diff --git a/src/core/tools/file-reading/__tests__/ReadFileCancellation.spec.ts b/src/core/tools/file-reading/__tests__/ReadFileCancellation.spec.ts new file mode 100644 index 0000000000..e8353dfe09 --- /dev/null +++ b/src/core/tools/file-reading/__tests__/ReadFileCancellation.spec.ts @@ -0,0 +1,132 @@ +import { ReadFileCancellation } from "../ReadFileCancellation" +import { ReadFileCancelledError } from "../ReadFileCancelledError" + +describe("ReadFileCancellation", () => { + it.each([ + { abort: false, abandoned: false, cancelled: false }, + { abort: true, abandoned: false, cancelled: true }, + { abort: false, abandoned: true, cancelled: true }, + { abort: true, abandoned: true, cancelled: true }, + ])("observes abort=$abort and abandoned=$abandoned without changing them", ({ abort, abandoned, cancelled }) => { + const task = { abort, abandoned } + const cancellation = new ReadFileCancellation(task) + + expect(cancellation.isCancelled()).toBe(cancelled) + if (cancelled) { + expect(() => cancellation.throwIfCancelled()).toThrow(ReadFileCancelledError) + } else { + expect(() => cancellation.throwIfCancelled()).not.toThrow() + } + expect(task).toEqual({ abort, abandoned }) + }) + + it.each(["abort", "abandoned"] as const)( + "observes later changes to %s rather than capturing a snapshot", + (flag) => { + const task = { abort: false, abandoned: false } + const cancellation = new ReadFileCancellation(task) + + expect(cancellation.isCancelled()).toBe(false) + task[flag] = true + expect(cancellation.isCancelled()).toBe(true) + expect(() => cancellation.throwIfCancelled()).toThrow("File read cancelled") + task[flag] = false + expect(cancellation.isCancelled()).toBe(false) + expect(() => cancellation.throwIfCancelled()).not.toThrow() + }, + ) + + describe("checked async operations", () => { + it("returns a successful result without changing task flags", async () => { + const task = { abort: false, abandoned: false } + const cancellation = new ReadFileCancellation(task) + const result = { fullPath: "/workspace/file.txt" } + const operation = vi.fn(async () => result) + + await expect(cancellation.run(operation)).resolves.toBe(result) + expect(operation).toHaveBeenCalledOnce() + expect(task).toEqual({ abort: false, abandoned: false }) + }) + + it.each(["abort", "abandoned"] as const)("does not start an operation when %s is already set", async (flag) => { + const task = { abort: false, abandoned: false } + task[flag] = true + const operation = vi.fn(async () => "unexpected content") + + await expect(new ReadFileCancellation(task).run(operation)).rejects.toBeInstanceOf(ReadFileCancelledError) + expect(operation).not.toHaveBeenCalled() + }) + + it.each(["abort", "abandoned"] as const)( + "discards a result when %s changes during the operation", + async (flag) => { + const task = { abort: false, abandoned: false } + const cancellation = new ReadFileCancellation(task) + + await expect( + cancellation.run(async () => { + task[flag] = true + return "late content" + }), + ).rejects.toBeInstanceOf(ReadFileCancelledError) + }, + ) + + it.each(["abort", "abandoned"] as const)( + "prioritizes cancellation over a late rejection after %s", + async (flag) => { + const task = { abort: false, abandoned: false } + const cancellation = new ReadFileCancellation(task) + const filesystemError = Object.assign(new Error("late filesystem failure"), { code: "ENOENT" }) + + await expect( + cancellation.run(async () => { + task[flag] = true + throw filesystemError + }), + ).rejects.toBeInstanceOf(ReadFileCancelledError) + }, + ) + + it("preserves the original cancellation error and stack without checking cancellation again", async () => { + const task = { abort: false, abandoned: false } + const cancellation = new ReadFileCancellation(task) + const error = new ReadFileCancelledError() + const originalStack = error.stack + const check = vi.spyOn(cancellation, "throwIfCancelled") + + await expect( + cancellation.run(async () => { + task.abort = true + throw error + }), + ).rejects.toBe(error) + expect(error.stack).toBe(originalStack) + expect(check).toHaveBeenCalledOnce() + }) + + it.each([new Error("filesystem failure"), "non-Error failure"])( + "preserves an unrelated rejection when the task is active: %s", + async (error) => { + const cancellation = new ReadFileCancellation({ abort: false, abandoned: false }) + + await expect( + cancellation.run(async () => { + throw error + }), + ).rejects.toBe(error) + }, + ) + + it("handles synchronous operation failures without replacing the original error", async () => { + const cancellation = new ReadFileCancellation({ abort: false, abandoned: false }) + const error = new Error("synchronous operation failure") + + await expect( + cancellation.run(() => { + throw error + }), + ).rejects.toBe(error) + }) + }) +}) diff --git a/src/core/tools/file-reading/__tests__/ReadFileTarget.spec.ts b/src/core/tools/file-reading/__tests__/ReadFileTarget.spec.ts new file mode 100644 index 0000000000..66572185a7 --- /dev/null +++ b/src/core/tools/file-reading/__tests__/ReadFileTarget.spec.ts @@ -0,0 +1,69 @@ +import * as fs from "fs/promises" + +import { ReadFileCancellation } from "../ReadFileCancellation" +import { ReadFileCancelledError } from "../ReadFileCancelledError" +import { ReadFileTarget } from "../ReadFileTarget" + +vi.mock("fs/promises", async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, realpath: vi.fn(), lstat: vi.fn(), open: vi.fn() } +}) + +describe("ReadFileTarget resolution errors", () => { + beforeEach(() => vi.clearAllMocks()) + + it("preserves an existing cancellation error without replacing it in the target's catch", async () => { + const task = { abort: false, abandoned: false } + const cancellation = new ReadFileCancellation(task) + const error = new ReadFileCancelledError() + const check = vi.spyOn(cancellation, "throwIfCancelled") + vi.mocked(fs.realpath).mockImplementationOnce(async () => { + task.abort = true + throw error + }) + + await expect(ReadFileTarget.resolve("/workspace/file.txt", cancellation)).rejects.toBe(error) + expect(check).toHaveBeenCalledOnce() + expect(fs.lstat).not.toHaveBeenCalled() + }) + + it("does not defer a cancellation error even if it carries a filesystem-like code", async () => { + const cancellation = new ReadFileCancellation({ abort: false, abandoned: false }) + const error = Object.assign(new ReadFileCancelledError(), { code: "ECANCELED" }) + vi.mocked(fs.realpath).mockRejectedValueOnce(error) + + await expect(ReadFileTarget.resolve("/workspace/file.txt", cancellation)).rejects.toBe(error) + expect(fs.lstat).not.toHaveBeenCalled() + }) + + it.each(["abort", "abandoned"] as const)( + "does not defer a late filesystem error when %s changes during resolution", + async (flag) => { + const task = { abort: false, abandoned: false } + const cancellation = new ReadFileCancellation(task) + vi.mocked(fs.realpath).mockImplementationOnce(async () => { + task[flag] = true + throw Object.assign(new Error("late missing file"), { code: "ENOENT" }) + }) + + await expect(ReadFileTarget.resolve("/workspace/file.txt", cancellation)).rejects.toBeInstanceOf( + ReadFileCancelledError, + ) + expect(fs.lstat).not.toHaveBeenCalled() + }, + ) + + it("keeps ordinary filesystem diagnostics deferred until approved handle access", async () => { + const cancellation = new ReadFileCancellation({ abort: false, abandoned: false }) + const error = Object.assign(new Error("missing file"), { code: "ENOENT" }) + vi.mocked(fs.realpath).mockRejectedValueOnce(error) + const read = vi.fn(async () => "unexpected content") + + const target = await ReadFileTarget.resolve("/workspace/file.txt", cancellation) + + expect(target.fullPath).toBe("/workspace/file.txt") + await expect(target.withHandle(read)).rejects.toBe(error) + expect(fs.open).not.toHaveBeenCalled() + expect(read).not.toHaveBeenCalled() + }) +}) diff --git a/src/core/tools/file-reading/__tests__/readFileSafety.integration.spec.ts b/src/core/tools/file-reading/__tests__/readFileSafety.integration.spec.ts new file mode 100644 index 0000000000..ac592432fd --- /dev/null +++ b/src/core/tools/file-reading/__tests__/readFileSafety.integration.spec.ts @@ -0,0 +1,1508 @@ +import * as fs from "fs/promises" +import os from "os" +import path from "path" +import * as vscode from "vscode" +import ExcelJS from "exceljs" +import { isBinaryFile } from "isbinaryfile" +import mammoth from "mammoth" +import type { ClineSayTool, ReadFileToolParams } from "@roo-code/types" +import type { Task } from "../../../task/Task" +import { checkAutoApproval } from "../../../auto-approval" +import { readFileTool } from "../../ReadFileTool" +import * as imageHelpers from "../../helpers/imageHelpers" +import { RooIgnoreController } from "../../../ignore/RooIgnoreController" +import { ReadFileCancellation } from "../ReadFileCancellation" +import { ReadFileCancelledError } from "../ReadFileCancelledError" +import { ReadFileTarget } from "../ReadFileTarget" + +// Instrument host I/O without replacing real filesystem behavior. +vi.mock("fs/promises", async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + open: vi.fn(actual.open), + readFile: vi.fn(actual.readFile), + realpath: vi.fn(actual.realpath), + lstat: vi.fn(actual.lstat), + } +}) + +vi.mock("vscode", async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + RelativePattern: class { + constructor( + readonly base: string, + readonly pattern: string, + ) {} + }, + } +}) + +vi.mock("isbinaryfile", async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, isBinaryFile: vi.fn(actual.isBinaryFile) } +}) + +// Keep decoder protocols at the library boundary; approval and descriptor I/O remain real. +vi.mock("pdf-parse/lib/pdf-parse", () => ({ default: async (source: Buffer) => ({ text: source.toString() }) })) +vi.mock("mammoth", () => ({ + default: { + extractRawText: async (options: { buffer?: Buffer; path?: string }) => ({ + value: options.buffer?.toString() ?? (await fs.readFile(options.path!, "utf8")), + }), + }, +})) + +function deferred() { + let resolve!: (value: T) => void + let reject!: (reason: Error) => void + const promise = new Promise((accept, fail) => { + resolve = accept + reject = fail + }) + return { promise, resolve, reject } +} + +describe("public file reader safety", () => { + let directory: string + let workspace: string + let external: string + let task: ReturnType + let approvalMessages: ClineSayTool[] + let manualApproval: ReturnType> + let openedFiles: fs.FileHandle[] + const callbacks = { pushToolResult: vi.fn(), askApproval: vi.fn(), handleError: vi.fn() } + + function createTask(settings: Partial[0]["state"]>> = {}) { + return { + cwd: workspace, + abort: false, + abandoned: false, + didToolFailInCurrentTurn: false, + didRejectTool: false, + rooIgnoreController: { validateAccess: vi.fn<(file: string) => boolean>().mockReturnValue(true) }, + say: vi.fn().mockResolvedValue(undefined), + fileContextTracker: { trackFileContext: vi.fn().mockResolvedValue(undefined) }, + api: { getModel: () => ({ info: { supportsImages: true } }) }, + providerRef: { deref: () => ({ getState: async () => ({}) }) }, + ask: vi.fn(async (ask, text) => { + approvalMessages.push(JSON.parse(text ?? "{}")) + const policy = await checkAutoApproval({ + cwd: task.cwd, + ask, + text, + state: { + autoApprovalEnabled: true, + alwaysAllowReadOnly: true, + alwaysAllowReadOnlyOutsideWorkspace: false, + allowedReadFiles: [], + alwaysAllowWrite: false, + alwaysAllowWriteOutsideWorkspace: false, + alwaysAllowWriteProtected: false, + alwaysAllowMcp: false, + alwaysAllowModeSwitch: false, + alwaysAllowSubtasks: false, + alwaysAllowExecute: false, + alwaysAllowFollowupQuestions: false, + ...settings, + }, + }) + return policy.decision === "approve" ? { response: "yesButtonClicked" } : manualApproval(ask, text) + }), + } + } + + async function read(params: ReadFileToolParams) { + // This host double intentionally omits Task members unrelated to file reading. + await readFileTool.execute(params, task as unknown as Task, callbacks) + return JSON.stringify(callbacks.pushToolResult.mock.calls) + } + + async function replaceAfterOpening(filePath: string, replacement: string) { + const actual = await vi.importActual("fs/promises") + vi.mocked(fs.open) + .mockReset() + .mockImplementation(async (...args) => { + const file = await actual.open(...args) + openedFiles.push(file) + if (args[0] === filePath) { + await fs.rename(filePath, `${filePath}.original`) + await fs.symlink(replacement, filePath) + } + return file + }) + } + + async function inspectOpenedFile(inspect: (file: fs.FileHandle) => void) { + const actual = await vi.importActual("fs/promises") + vi.mocked(fs.open).mockImplementationOnce(async (...args) => { + const file = await actual.open(...args) + openedFiles.push(file) + inspect(file) + return file + }) + } + + beforeEach(async () => { + vi.clearAllMocks() + const actual = await vi.importActual("fs/promises") + openedFiles = [] + vi.mocked(fs.open) + .mockReset() + .mockImplementation(async (...args) => { + const file = await actual.open(...args) + openedFiles.push(file) + return file + }) + vi.mocked(fs.readFile).mockReset().mockImplementation(actual.readFile) + vi.mocked(fs.realpath).mockReset().mockImplementation(actual.realpath) + vi.mocked(fs.lstat).mockReset().mockImplementation(actual.lstat) + const binary = await vi.importActual("isbinaryfile") + vi.mocked(isBinaryFile).mockReset().mockImplementation(binary.isBinaryFile) + directory = await fs.realpath(await fs.mkdtemp(path.join(os.tmpdir(), "zoo-file-safety-"))) + workspace = path.join(directory, "workspace") + external = path.join(directory, "external.txt") + await fs.mkdir(workspace) + await fs.writeFile(external, "synthetic external secret") + vi.spyOn(vscode.workspace, "workspaceFolders", "get").mockReturnValue([ + { uri: vscode.Uri.file(workspace), name: "workspace", index: 0 }, + ]) + approvalMessages = [] + manualApproval = vi.fn().mockResolvedValue({ response: "noButtonClicked" }) + task = createTask() + }) + + afterEach(async () => { + expect(openedFiles.every((file) => file.fd === -1)).toBe(true) + vi.restoreAllMocks() + await fs.rm(directory, { recursive: true, force: true }) + }) + + it("requires manual external approval for an internal symlink under the real policy", async () => { + await fs.symlink(external, path.join(workspace, "linked.txt")) + + const result = await read({ path: "linked.txt" }) + + expect(manualApproval).toHaveBeenCalledOnce() + expect(approvalMessages[0]).toMatchObject({ isOutsideWorkspace: true, content: external }) + expect(result).toContain("Denied by user") + expect(result).not.toContain("synthetic external secret") + expect(fs.open).not.toHaveBeenCalled() + expect(fs.readFile).not.toHaveBeenCalled() + expect(task.fileContextTracker.trackFileContext).not.toHaveBeenCalled() + }) + + it("blocks an ignored canonical target before approval or content I/O", async () => { + const ignored = path.join(workspace, "ignored.txt") + await fs.writeFile(ignored, "ignored secret") + await fs.symlink(ignored, path.join(workspace, "alias.txt")) + task.rooIgnoreController.validateAccess.mockImplementation((file) => file !== ignored) + + const result = await read({ path: "alias.txt" }) + + expect(result).not.toContain("ignored secret") + expect(task.ask).not.toHaveBeenCalled() + expect(task.say).toHaveBeenCalledWith("rooignore_error", "alias.txt") + expect(fs.open).not.toHaveBeenCalled() + expect(fs.readFile).not.toHaveBeenCalled() + }) + + it("rejects a regular-file replacement during approval without disclosing replacement bytes", async () => { + manualApproval.mockImplementation(async () => { + await fs.rename(external, `${external}.original`) + await fs.writeFile(external, "unapproved replacement secret") + return { response: "yesButtonClicked" } + }) + + const result = await read({ path: external }) + + expect(result).not.toContain("unapproved replacement secret") + expect(result).toContain("File target changed after approval") + expect(task.didToolFailInCurrentTurn).toBe(true) + expect(task.fileContextTracker.trackFileContext).not.toHaveBeenCalled() + }) + + it("rejects a different device even when the approved inode is unchanged", async () => { + const actual = await vi.importActual("fs/promises") + const otherDeviceStats = await actual.lstat(external, { bigint: true }) + otherDeviceStats.dev += 1n + manualApproval.mockResolvedValue({ response: "yesButtonClicked" }) + await inspectOpenedFile((file) => { + vi.spyOn(file, "stat").mockResolvedValueOnce(otherDeviceStats) + vi.spyOn(file, "read") + vi.spyOn(file, "readFile") + }) + + const result = await read({ path: external }) + + expect(manualApproval).toHaveBeenCalledOnce() + expect(result).toContain("File target changed after approval") + expect(result).not.toContain("synthetic external secret") + expect(openedFiles[0].read).not.toHaveBeenCalled() + expect(openedFiles[0].readFile).not.toHaveBeenCalled() + expect(task.fileContextTracker.trackFileContext).not.toHaveBeenCalled() + }) + + it("legacy requires external target approval and retains its string result", async () => { + await fs.symlink(external, path.join(workspace, "linked.txt")) + + const result = await read({ files: [{ path: "linked.txt" }], _legacyFormat: true }) + + expect(manualApproval).toHaveBeenCalledOnce() + expect(approvalMessages[0]).toMatchObject({ isOutsideWorkspace: true, content: external }) + expect(callbacks.pushToolResult).toHaveBeenCalledWith("File: linked.txt\nStatus: Denied by user") + expect(result).not.toContain("synthetic external secret") + }) + + it.each((["abort", "abandoned"] as const).flatMap((flag) => [false, true].map((approved) => ({ flag, approved }))))( + "legacy stops when $flag changes during approval without treating the response as rejection (approved=$approved)", + async ({ flag, approved }) => { + manualApproval.mockImplementation(async () => { + task[flag] = true + return { response: approved ? "yesButtonClicked" : "noButtonClicked" } + }) + + const result = await read({ files: [{ path: external }], _legacyFormat: true }) + + expect(manualApproval).toHaveBeenCalledOnce() + expect(result).not.toContain("synthetic external secret") + expect(fs.open).not.toHaveBeenCalled() + expect(task.fileContextTracker.trackFileContext).not.toHaveBeenCalled() + expect(callbacks.pushToolResult).not.toHaveBeenCalled() + expect(task.say).not.toHaveBeenCalled() + expect(task.didToolFailInCurrentTurn).toBe(false) + expect(task.didRejectTool).toBe(false) + }, + ) + + it.each(["abort", "abandoned"] as const)( + "legacy closes a descriptor returned after %s without starting metadata or content reads", + async (flag) => { + task.ask.mockResolvedValueOnce({ response: "yesButtonClicked" }) + await inspectOpenedFile((file) => { + vi.spyOn(file, "stat") + vi.spyOn(file, "read") + vi.spyOn(file, "readFile") + task[flag] = true + }) + + await read({ files: [{ path: external }], _legacyFormat: true }) + + expect(openedFiles).toHaveLength(1) + expect(openedFiles[0].fd).toBe(-1) + expect(openedFiles[0].stat).not.toHaveBeenCalled() + expect(openedFiles[0].read).not.toHaveBeenCalled() + expect(openedFiles[0].readFile).not.toHaveBeenCalled() + expect(callbacks.pushToolResult).not.toHaveBeenCalled() + expect(task.say).not.toHaveBeenCalled() + expect(task.didToolFailInCurrentTurn).toBe(false) + }, + ) + + it("legacy discards completed content and does not approve another file after cancellation during cleanup", async () => { + task.ask.mockResolvedValue({ response: "yesButtonClicked" }) + await inspectOpenedFile((file) => { + const close = file.close.bind(file) + vi.spyOn(file, "close").mockImplementationOnce(async () => { + await close() + task.abort = true + }) + }) + + const result = await read({ files: [{ path: external }, { path: external }], _legacyFormat: true }) + + expect(result).not.toContain("synthetic external secret") + expect(task.ask).toHaveBeenCalledOnce() + expect(fs.open).toHaveBeenCalledOnce() + expect(callbacks.pushToolResult).not.toHaveBeenCalled() + expect(task.didToolFailInCurrentTurn).toBe(false) + }) + + it.each( + (["abort", "abandoned"] as const).flatMap((flag) => + ["reading", "closing"].map((boundary) => ({ flag, boundary })), + ), + )( + "verified handle rejects a late result after $flag during $boundary and closes the descriptor", + async ({ flag, boundary }) => { + const target = await ReadFileTarget.resolve(external, new ReadFileCancellation(task)) + if (boundary === "closing") { + await inspectOpenedFile((file) => { + const close = file.close.bind(file) + vi.spyOn(file, "close").mockImplementationOnce(async () => { + await close() + task[flag] = true + }) + }) + } + const readContent = vi.fn(async () => { + if (boundary === "reading") task[flag] = true + return "late content" + }) + + await expect(target.withHandle(readContent)).rejects.toBeInstanceOf(ReadFileCancelledError) + + expect(readContent).toHaveBeenCalledOnce() + expect(openedFiles).toHaveLength(1) + expect(openedFiles[0].fd).toBe(-1) + }, + ) + + it("verified handle preserves a read error and still closes the descriptor", async () => { + const target = await ReadFileTarget.resolve(external, new ReadFileCancellation(task)) + const error = new Error("read failure") + + await expect( + target.withHandle(async () => { + throw error + }), + ).rejects.toBe(error) + + expect(openedFiles).toHaveLength(1) + expect(openedFiles[0].fd).toBe(-1) + }) + + it("legacy rejects canonical symlink substitution during approval", async () => { + const replacement = path.join(directory, "unapproved.txt") + await fs.writeFile(replacement, "legacy replacement secret") + manualApproval.mockImplementation(async () => { + await fs.rename(external, `${external}.original`) + await fs.symlink(replacement, external) + return { response: "yesButtonClicked" } + }) + + const result = await read({ files: [{ path: external }], _legacyFormat: true }) + + expect(result).not.toContain("legacy replacement secret") + expect(result).toContain("Error:") + expect(task.didToolFailInCurrentTurn).toBe(true) + expect(task.fileContextTracker.trackFileContext).not.toHaveBeenCalled() + }) + + it("images use only approved descriptor bytes after filename replacement", async () => { + const image = path.join(workspace, "image.png") + const replacement = path.join(directory, "other.png") + const approved = Buffer.from([137, 80, 78, 71, 0, 1, 2, 3]) + const unapproved = Buffer.from([137, 80, 78, 71, 0, 9, 8, 7]) + await fs.writeFile(image, approved) + await fs.writeFile(replacement, unapproved) + await replaceAfterOpening(image, replacement) + + const result = await read({ path: "image.png" }) + + expect(result).toContain(approved.toString("base64")) + expect(result).not.toContain(unapproved.toString("base64")) + expect(fs.readFile).not.toHaveBeenCalled() + }) + + it("XLSX extraction preserves formatting and consumes approved bytes after filename replacement", async () => { + const spreadsheet = path.join(workspace, "book.xlsx") + const replacement = path.join(directory, "other.xlsx") + for (const [file, value] of [ + [spreadsheet, "approved cell"], + [replacement, "unapproved cell"], + ]) { + const workbook = new ExcelJS.Workbook() + workbook.addWorksheet("Data").getCell("A1").value = value + await workbook.xlsx.writeFile(file) + } + await replaceAfterOpening(spreadsheet, replacement) + + const result = await read({ path: "book.xlsx" }) + + expect(result).toContain("1 | --- Sheet: Data ---") + expect(result).toContain("2 | approved cell") + expect(result).not.toContain("unapproved cell") + }) + + it("notebook extraction uses approved bytes without changing historical line numbering", async () => { + const notebook = path.join(workspace, "book.ipynb") + const replacement = path.join(directory, "other.ipynb") + for (const [file, value] of [ + [notebook, "approved code"], + [replacement, "unapproved code"], + ]) { + await fs.writeFile(file, JSON.stringify({ cells: [{ cell_type: "code", source: [value] }] })) + } + await replaceAfterOpening(notebook, replacement) + // The old reader extracts notebooks only when binary detection chooses that branch. + vi.mocked(isBinaryFile).mockResolvedValue(true) + + const result = await read({ path: "book.ipynb" }) + + expect(result).toContain("1 | 1 | approved code") + expect(result).not.toContain("unapproved code") + expect(fs.readFile).not.toHaveBeenCalled() + }) + + it("PDF decoding consumes approved bytes after filename replacement", async () => { + const document = path.join(workspace, "document.pdf") + const replacement = path.join(directory, "other.pdf") + await fs.writeFile(document, "approved PDF text") + await fs.writeFile(replacement, "unapproved PDF text") + await replaceAfterOpening(document, replacement) + vi.mocked(isBinaryFile).mockResolvedValue(true) + + const result = await read({ path: "document.pdf" }) + + expect(result).toContain("1 | 1 | approved PDF text") + expect(result).not.toContain("unapproved PDF text") + expect(fs.readFile).not.toHaveBeenCalled() + }) + + it("DOCX decoding consumes approved bytes after filename replacement", async () => { + const document = path.join(workspace, "document.docx") + const replacement = path.join(directory, "other.docx") + await fs.writeFile(document, "approved DOCX text") + await fs.writeFile(replacement, "unapproved DOCX text") + await replaceAfterOpening(document, replacement) + vi.mocked(isBinaryFile).mockResolvedValue(true) + + const result = await read({ path: "document.docx" }) + + expect(result).toContain("1 | 1 | approved DOCX text") + expect(result).not.toContain("unapproved DOCX text") + expect(fs.readFile).not.toHaveBeenCalled() + }) + + it("reads the original canonical target when the requested alias changes during approval", async () => { + const alias = path.join(workspace, "alias.txt") + const replacement = path.join(directory, "replacement.txt") + await fs.writeFile(replacement, "unapproved alias bytes") + await fs.symlink(external, alias) + manualApproval.mockImplementation(async () => { + await fs.unlink(alias) + await fs.symlink(replacement, alias) + return { response: "yesButtonClicked" } + }) + + const result = await read({ path: "alias.txt" }) + + expect(result).toContain("1 | synthetic external secret") + expect(result).not.toContain("unapproved alias bytes") + expect(fs.readFile).not.toHaveBeenCalled() + }) + + it.each(["final symlink", "parent symlink", "legacy regular file"])( + "rejects approved identity replacement: %s", + async (kind) => { + const parent = path.join(directory, "approved") + const other = path.join(directory, "other") + await fs.mkdir(parent) + await fs.mkdir(other) + const approved = path.join(parent, "file.txt") + await fs.writeFile(approved, "approved bytes") + await fs.writeFile(path.join(other, "file.txt"), "unapproved bytes") + manualApproval.mockImplementation(async () => { + if (kind === "parent symlink") { + await fs.rename(parent, `${parent}.original`) + await fs.symlink(other, parent) + } else { + await fs.rename(approved, `${approved}.original`) + if (kind === "final symlink") await fs.symlink(path.join(other, "file.txt"), approved) + else await fs.writeFile(approved, "unapproved bytes") + } + return { response: "yesButtonClicked" } + }) + + const result = await read( + kind === "legacy regular file" + ? { files: [{ path: approved }], _legacyFormat: true } + : { path: approved }, + ) + + expect(result).not.toContain("unapproved bytes") + expect(result).toContain("Error:") + expect(task.didToolFailInCurrentTurn).toBe(true) + expect(task.fileContextTracker.trackFileContext).not.toHaveBeenCalled() + }, + ) + + it("a symlinked workspace root remains internal under the real approval policy", async () => { + const rootAlias = path.join(directory, "root-alias") + await fs.symlink(workspace, rootAlias) + await fs.writeFile(path.join(workspace, "internal.txt"), "first line\nsecond line") + vi.spyOn(vscode.workspace, "workspaceFolders", "get").mockReturnValue([ + { uri: vscode.Uri.file(rootAlias), name: "alias", index: 0 }, + ]) + task.cwd = rootAlias + + await read({ path: "internal.txt" }) + + expect(manualApproval).not.toHaveBeenCalled() + expect(approvalMessages[0]).toMatchObject({ isOutsideWorkspace: false }) + expect(callbacks.pushToolResult).toHaveBeenCalledWith("File: internal.txt\n1 | first line\n2 | second line") + expect(fs.readFile).not.toHaveBeenCalled() + }) + + it.each( + (["relative", "alias-absolute", "canonical-absolute"] as const).flatMap((spelling) => + [false, true].map((legacy) => ({ spelling, legacy })), + ), + )( + "auto-approves a canonical internal target with $spelling allowlist (legacy=$legacy)", + async ({ spelling, legacy }) => { + const rootAlias = path.join(directory, "root-alias") + await fs.symlink(workspace, rootAlias) + await fs.mkdir(path.join(workspace, "docs")) + const canonicalFile = path.join(workspace, "docs", "notes.md") + await fs.writeFile(canonicalFile, "allowed internal bytes") + const pattern = + spelling === "relative" ? "docs/**" : `${spelling === "alias-absolute" ? rootAlias : workspace}/docs/**` + task = createTask({ alwaysAllowReadOnly: false, allowedReadFiles: [pattern] }) + task.cwd = rootAlias + + const result = await read( + legacy ? { files: [{ path: "docs/notes.md" }], _legacyFormat: true } : { path: "docs/notes.md" }, + ) + + expect(manualApproval).not.toHaveBeenCalled() + expect(approvalMessages[0]).toMatchObject({ + path: canonicalFile, + content: canonicalFile, + isOutsideWorkspace: false, + }) + expect(result).toContain("1 | allowed internal bytes") + expect(fs.readFile).not.toHaveBeenCalled() + }, + ) + + it.each([false, true])( + "an external file symlink needs an explicit external pattern (explicit=%s)", + async (explicit) => { + const rootAlias = path.join(directory, "root-alias") + await fs.symlink(workspace, rootAlias) + await fs.mkdir(path.join(workspace, "docs")) + await fs.symlink(external, path.join(workspace, "docs", "alias.md")) + task = createTask({ + alwaysAllowReadOnly: false, + allowedReadFiles: explicit ? ["docs/**", external] : ["docs/**"], + }) + task.cwd = rootAlias + + const result = await read({ path: "docs/alias.md" }) + + expect(approvalMessages[0]).toMatchObject({ path: external, content: external, isOutsideWorkspace: true }) + if (explicit) { + expect(manualApproval).not.toHaveBeenCalled() + expect(result).toContain("1 | synthetic external secret") + } else { + expect(manualApproval).toHaveBeenCalledOnce() + expect(result).toContain("Denied by user") + expect(result).not.toContain("synthetic external secret") + expect(fs.open).not.toHaveBeenCalled() + } + expect(fs.readFile).not.toHaveBeenCalled() + }, + ) + + it("uses the task root rather than the first workspace folder for multi-root read matching", async () => { + const focusedRoot = path.join(directory, "focused-workspace") + const rootAlias = path.join(directory, "task-alias") + await fs.mkdir(focusedRoot) + await fs.symlink(workspace, rootAlias) + await fs.writeFile(path.join(workspace, "notes.md"), "task-local bytes") + vi.spyOn(vscode.workspace, "workspaceFolders", "get").mockReturnValue([ + { uri: vscode.Uri.file(focusedRoot), name: "focused", index: 0 }, + { uri: vscode.Uri.file(rootAlias), name: "task", index: 1 }, + ]) + task = createTask({ alwaysAllowReadOnly: false, allowedReadFiles: ["notes.md"] }) + task.cwd = rootAlias + + const result = await read({ path: "notes.md" }) + + expect(manualApproval).not.toHaveBeenCalled() + expect(approvalMessages[0]).toMatchObject({ + isOutsideWorkspace: false, + content: path.join(workspace, "notes.md"), + }) + expect(result).toContain("1 | task-local bytes") + }) + + it("does not apply a task-relative allowlist to another open workspace root", async () => { + const otherRoot = path.join(directory, "other-workspace") + const rootAlias = path.join(directory, "task-alias") + await fs.mkdir(otherRoot) + await fs.symlink(workspace, rootAlias) + const otherFile = path.join(otherRoot, "notes.md") + await fs.writeFile(otherFile, "unapproved other-root bytes") + vi.spyOn(vscode.workspace, "workspaceFolders", "get").mockReturnValue([ + { uri: vscode.Uri.file(otherRoot), name: "other", index: 0 }, + { uri: vscode.Uri.file(rootAlias), name: "task", index: 1 }, + ]) + task = createTask({ alwaysAllowReadOnly: false, allowedReadFiles: ["notes.md"] }) + task.cwd = rootAlias + + const result = await read({ path: otherFile }) + + expect(manualApproval).toHaveBeenCalledOnce() + expect(approvalMessages[0]).toMatchObject({ path: otherFile, content: otherFile, isOutsideWorkspace: false }) + expect(result).toContain("Denied by user") + expect(result).not.toContain("unapproved other-root bytes") + expect(fs.open).not.toHaveBeenCalled() + expect(fs.readFile).not.toHaveBeenCalled() + }) + + it.each(["absent", "unresolvable", "sibling"])( + "does not grant blanket internal permission with %s workspace containment", + async (kind) => { + const sibling = `${workspace}-sibling` + await fs.mkdir(sibling) + const filePath = path.join(kind === "sibling" ? sibling : workspace, "notes.md") + await fs.writeFile(filePath, "unapproved containment bytes") + vi.spyOn(vscode.workspace, "workspaceFolders", "get").mockReturnValue( + kind === "absent" + ? undefined + : [ + { + uri: vscode.Uri.file( + kind === "unresolvable" ? path.join(directory, "missing-root") : workspace, + ), + name: "root", + index: 0, + }, + ], + ) + + const result = await read({ path: filePath }) + + expect(manualApproval).toHaveBeenCalledOnce() + expect(approvalMessages[0]).toMatchObject({ isOutsideWorkspace: true, content: filePath }) + expect(result).toContain("Denied by user") + expect(result).not.toContain("unapproved containment bytes") + expect(fs.open).not.toHaveBeenCalled() + expect(fs.readFile).not.toHaveBeenCalled() + }, + ) + + it("does not disclose an existing external file through ENOTDIR when approval is denied", async () => { + const invalidChild = path.join(external, "child.txt") + manualApproval.mockImplementation(async () => { + expect(task.say).not.toHaveBeenCalled() + expect(callbacks.pushToolResult).not.toHaveBeenCalled() + expect(fs.open).not.toHaveBeenCalled() + expect(fs.readFile).not.toHaveBeenCalled() + return { response: "noButtonClicked" } + }) + + const result = await read({ path: invalidChild }) + + expect(manualApproval).toHaveBeenCalledOnce() + expect(approvalMessages[0]).toMatchObject({ isOutsideWorkspace: true, content: invalidChild }) + expect(result).toContain("Denied by user") + expect(result).not.toContain("ENOTDIR") + expect(fs.open).not.toHaveBeenCalled() + expect(fs.readFile).not.toHaveBeenCalled() + expect(task.didToolFailInCurrentTurn).toBe(false) + }) + + it.each( + ["ENOTDIR", "EACCES", "ELOOP"].flatMap((code) => + (["realpath", "lstat"] as const).flatMap((operation) => + [false, true].flatMap((legacy) => + [false, true].map((approved) => ({ code, operation, legacy, approved })), + ), + ), + ), + )( + "defers $operation $code diagnostics (legacy=$legacy, approved=$approved)", + async ({ code, operation, legacy, approved }) => { + const lexicalPath = path.join(directory, "outside-alias.txt") + await fs.symlink(external, lexicalPath) + vi.mocked(fs[operation]).mockRejectedValueOnce( + Object.assign(new Error(`${code}: resolution inaccessible`), { code }), + ) + manualApproval.mockImplementation(async () => { + expect(task.say).not.toHaveBeenCalled() + expect(callbacks.pushToolResult).not.toHaveBeenCalled() + expect(fs.open).not.toHaveBeenCalled() + expect(fs.readFile).not.toHaveBeenCalled() + return { response: approved ? "yesButtonClicked" : "noButtonClicked" } + }) + + const result = await read( + legacy ? { files: [{ path: lexicalPath }], _legacyFormat: true } : { path: lexicalPath }, + ) + + expect(manualApproval).toHaveBeenCalledOnce() + expect(approvalMessages[0]).toMatchObject({ isOutsideWorkspace: true, content: lexicalPath }) + if (approved) expect(result).toContain(`${code}: resolution inaccessible`) + else { + expect(result).toContain("Denied by user") + expect(result).not.toContain(code) + } + expect(task.didToolFailInCurrentTurn).toBe(approved) + expect(fs.open).not.toHaveBeenCalled() + expect(fs.readFile).not.toHaveBeenCalled() + expect(task.fileContextTracker.trackFileContext).not.toHaveBeenCalled() + }, + ) + + it.each([undefined, 123])( + "does not defer unexpected resolution errors with a non-string errno (%s)", + async (code) => { + vi.mocked(fs.realpath).mockRejectedValueOnce( + Object.assign(new TypeError("unexpected resolution failure"), { code }), + ) + + const result = await read({ path: external }) + + expect(result).toContain("unexpected resolution failure") + expect(task.ask).not.toHaveBeenCalled() + expect(task.didToolFailInCurrentTurn).toBe(true) + expect(fs.open).not.toHaveBeenCalled() + expect(fs.readFile).not.toHaveBeenCalled() + }, + ) + + it.each([false, true])("missing-file diagnostics stay after approval (legacy=%s)", async (legacy) => { + const missing = path.join(directory, "missing.txt") + manualApproval.mockImplementation(async () => { + expect(task.say).not.toHaveBeenCalled() + expect(fs.open).not.toHaveBeenCalled() + return { response: "yesButtonClicked" } + }) + + const result = await read(legacy ? { files: [{ path: missing }], _legacyFormat: true } : { path: missing }) + + expect(manualApproval).toHaveBeenCalledOnce() + expect(result).toContain("ENOENT") + expect(task.didToolFailInCurrentTurn).toBe(true) + expect(fs.open).not.toHaveBeenCalled() + }) + + it.each((["abort", "abandoned"] as const).flatMap((flag) => [false, true].map((legacy) => ({ flag, legacy }))))( + "pre-cancelled reads start no access UI, approval or filesystem work ($flag, legacy=$legacy)", + async ({ flag, legacy }) => { + task[flag] = true + + const result = await read( + legacy ? { files: [{ path: external }], _legacyFormat: true } : { path: external }, + ) + + expect(task.rooIgnoreController.validateAccess).not.toHaveBeenCalled() + expect(task.ask).not.toHaveBeenCalled() + expect(task.say).not.toHaveBeenCalled() + expect(fs.open).not.toHaveBeenCalled() + expect(fs.readFile).not.toHaveBeenCalled() + expect(result).not.toContain("synthetic external secret") + expect(task.didRejectTool).toBe(false) + expect(task.didToolFailInCurrentTurn).toBe(false) + }, + ) + + it("abandonment while realpath is pending stops identity capture without error UI", async () => { + const started = deferred() + const resolved = deferred() + vi.mocked(fs.realpath).mockImplementationOnce(() => { + started.resolve() + return resolved.promise + }) + const pending = read({ path: external }) + await started.promise + task.abandoned = true + resolved.resolve(external) + + await pending + + expect(fs.lstat).not.toHaveBeenCalled() + expect(task.ask).not.toHaveBeenCalled() + expect(task.say).not.toHaveBeenCalled() + expect(task.didToolFailInCurrentTurn).toBe(false) + }) + + it("abort during identity capture prevents canonical revalidation and target access", async () => { + const actual = await vi.importActual("fs/promises") + const identity = await actual.lstat(external, { bigint: true }) + const started = deferred() + const release = deferred() + vi.mocked(fs.realpath).mockClear() + vi.mocked(fs.lstat).mockImplementationOnce(async () => { + started.resolve() + await release.promise + return identity + }) + const pending = read({ path: external }) + await started.promise + task.abort = true + release.resolve() + await pending + + expect(fs.realpath).toHaveBeenCalledOnce() + expect(task.rooIgnoreController.validateAccess).toHaveBeenCalledOnce() + expect(task.ask).not.toHaveBeenCalled() + expect(task.say).not.toHaveBeenCalled() + }) + + it("abort during final canonical revalidation prevents resolved-target access", async () => { + const actual = await vi.importActual("fs/promises") + const started = deferred() + const release = deferred() + vi.mocked(fs.realpath) + .mockImplementationOnce(actual.realpath) + .mockImplementationOnce(async () => { + started.resolve() + await release.promise + return external + }) + const pending = read({ path: external }) + await started.promise + task.abort = true + release.resolve() + await pending + + expect(task.rooIgnoreController.validateAccess).toHaveBeenCalledOnce() + expect(task.ask).not.toHaveBeenCalled() + expect(task.say).not.toHaveBeenCalled() + }) + + it("abort during workspace classification does not start approval", async () => { + const actual = await vi.importActual("fs/promises") + const started = deferred() + const release = deferred() + vi.mocked(fs.realpath).mockImplementation(async (...args) => { + if (args[0] === workspace) { + started.resolve() + await release.promise + } + return actual.realpath(...args) + }) + const pending = read({ path: external }) + await started.promise + task.abort = true + release.resolve() + await pending + + expect(task.ask).not.toHaveBeenCalled() + expect(task.say).not.toHaveBeenCalled() + expect(task.didRejectTool).toBe(false) + }) + + it("late denied approval retains received feedback but does not become a rejection after cancellation", async () => { + const started = deferred() + const approval = deferred>>() + task.ask.mockImplementationOnce(() => { + started.resolve() + return approval.promise + }) + const pending = read({ path: external }) + await started.promise + task.abandoned = true + approval.resolve({ + response: "noButtonClicked", + text: "Inspect only", + images: ["data:image/png;base64,aW1hZ2U="], + }) + const result = await pending + + expect(task.didRejectTool).toBe(false) + expect(task.didToolFailInCurrentTurn).toBe(false) + expect(task.say).not.toHaveBeenCalled() + expect(result).toContain("Inspect only") + expect(result).toContain("aW1hZ2U=") + expect(result).not.toContain("Denied by user") + expect(fs.open).not.toHaveBeenCalled() + }) + + it("a descriptor opened after abort closes without starting metadata or content I/O", async () => { + manualApproval.mockResolvedValue({ response: "yesButtonClicked" }) + const actual = await vi.importActual("fs/promises") + const started = deferred() + const release = deferred() + vi.mocked(fs.open).mockImplementationOnce(async (...args) => { + const file = await actual.open(...args) + openedFiles.push(file) + vi.spyOn(file, "stat") + vi.spyOn(file, "read") + vi.spyOn(file, "readFile") + started.resolve(file) + await release.promise + return file + }) + const pending = read({ path: external }) + const file = await started.promise + task.abort = true + release.resolve() + await pending + + expect(file.stat).not.toHaveBeenCalled() + expect(file.read).not.toHaveBeenCalled() + expect(file.readFile).not.toHaveBeenCalled() + expect(file.fd).toBe(-1) + expect(task.say).not.toHaveBeenCalled() + expect(task.didToolFailInCurrentTurn).toBe(false) + }) + + it("abandonment during fstat stops the descriptor probe and closes the handle", async () => { + manualApproval.mockResolvedValue({ response: "yesButtonClicked" }) + const started = deferred() + const release = deferred() + await inspectOpenedFile((file) => { + const stat = file.stat.bind(file) + vi.spyOn(file, "stat").mockImplementationOnce(async (options) => { + started.resolve(file) + await release.promise + return stat(options) + }) + vi.spyOn(file, "read") + }) + const pending = read({ path: external }) + const file = await started.promise + task.abandoned = true + release.resolve() + await pending + + expect(file.read).not.toHaveBeenCalled() + expect(file.fd).toBe(-1) + expect(task.say).not.toHaveBeenCalled() + expect(task.didToolFailInCurrentTurn).toBe(false) + }) + + it("abort during the descriptor probe prevents binary detection", async () => { + manualApproval.mockResolvedValue({ response: "yesButtonClicked" }) + const started = deferred() + const release = deferred() + await inspectOpenedFile((file) => { + vi.spyOn(file, "read").mockImplementationOnce(async () => { + started.resolve() + await release.promise + return { bytesRead: 0, buffer: Buffer.alloc(0) } + }) + }) + const pending = read({ path: external }) + await started.promise + task.abort = true + release.resolve() + await pending + + expect(isBinaryFile).not.toHaveBeenCalled() + expect(task.fileContextTracker.trackFileContext).not.toHaveBeenCalled() + expect(task.say).not.toHaveBeenCalled() + }) + + it("abandonment during binary detection prevents whole-file I/O", async () => { + manualApproval.mockResolvedValue({ response: "yesButtonClicked" }) + const started = deferred() + const detected = deferred() + await inspectOpenedFile((file) => { + vi.spyOn(file, "readFile") + }) + vi.mocked(isBinaryFile).mockImplementationOnce(() => { + started.resolve() + return detected.promise + }) + const pending = read({ path: external }) + await started.promise + task.abandoned = true + detected.resolve(false) + await pending + + expect(openedFiles[0].readFile).not.toHaveBeenCalled() + expect(task.fileContextTracker.trackFileContext).not.toHaveBeenCalled() + }) + + it("late ordinary text bytes are not decoded or tracked after abandonment", async () => { + manualApproval.mockResolvedValue({ response: "yesButtonClicked" }) + const started = deferred() + const buffer = Buffer.from("late text secret") + const bytes = deferred() + const decode = vi.spyOn(buffer, "toString") + await inspectOpenedFile((file) => { + vi.spyOn(file, "readFile").mockImplementationOnce(() => { + started.resolve() + return bytes.promise + }) + }) + const pending = read({ path: external }) + await started.promise + task.abandoned = true + bytes.resolve(buffer) + const result = await pending + + expect(decode).not.toHaveBeenCalled() + expect(task.fileContextTracker.trackFileContext).not.toHaveBeenCalled() + expect(result).not.toContain("late text secret") + }) + + it("document bytes arriving after abort do not start extraction", async () => { + const document = path.join(workspace, "late.docx") + await fs.writeFile(document, "approved document") + vi.mocked(isBinaryFile).mockResolvedValue(true) + const extract = vi.spyOn(mammoth, "extractRawText") + const started = deferred() + const buffer = Buffer.from("late document secret") + const bytes = deferred() + await inspectOpenedFile((file) => { + vi.spyOn(file, "readFile").mockImplementationOnce(() => { + started.resolve() + return bytes.promise + }) + }) + const pending = read({ path: "late.docx" }) + await started.promise + task.abort = true + bytes.resolve(buffer) + const result = await pending + + expect(extract).not.toHaveBeenCalled() + expect(task.say).not.toHaveBeenCalled() + expect(task.fileContextTracker.trackFileContext).not.toHaveBeenCalled() + expect(result).not.toContain("late document secret") + }) + + it("late extraction output is discarded without starting context tracking", async () => { + await fs.writeFile(path.join(workspace, "late.docx"), "approved document") + vi.mocked(isBinaryFile).mockResolvedValue(true) + const started = deferred() + const extraction = deferred>>() + vi.spyOn(mammoth, "extractRawText").mockImplementationOnce(() => { + started.resolve() + return extraction.promise + }) + const pending = read({ path: "late.docx" }) + await started.promise + task.abandoned = true + extraction.resolve({ value: "late extraction secret", messages: [] }) + const result = await pending + + expect(task.fileContextTracker.trackFileContext).not.toHaveBeenCalled() + expect(task.say).not.toHaveBeenCalled() + expect(result).not.toContain("late extraction secret") + }) + + it.each([2, 3])("abort during image metadata step %s prevents image byte reads", async (step) => { + await fs.writeFile(path.join(workspace, "image.png"), Buffer.from([137, 80, 78, 71, 0])) + const started = deferred() + const release = deferred() + await inspectOpenedFile((file) => { + const stat = file.stat.bind(file) + let calls = 0 + vi.spyOn(file, "stat").mockImplementation(async (options) => { + if (++calls === step) { + started.resolve() + await release.promise + } + return stat(options) + }) + vi.spyOn(file, "readFile") + }) + const pending = read({ path: "image.png" }) + await started.promise + task.abort = true + release.resolve() + await pending + + expect(openedFiles[0].readFile).not.toHaveBeenCalled() + expect(openedFiles[0].stat).toHaveBeenCalledTimes(step) + expect(task.fileContextTracker.trackFileContext).not.toHaveBeenCalled() + expect(task.say).not.toHaveBeenCalled() + expect(task.didToolFailInCurrentTurn).toBe(false) + }) + + it("image bytes arriving after abort are not base64 encoded or tracked", async () => { + await fs.writeFile(path.join(workspace, "image.png"), Buffer.from([137, 80, 78, 71, 0])) + const started = deferred() + const buffer = Buffer.from([137, 80, 78, 71, 0, 9]) + const bytes = deferred() + const encode = vi.spyOn(buffer, "toString") + await inspectOpenedFile((file) => { + vi.spyOn(file, "readFile").mockImplementationOnce(() => { + started.resolve() + return bytes.promise + }) + }) + const pending = read({ path: "image.png" }) + await started.promise + task.abort = true + bytes.resolve(buffer) + await pending + + expect(encode).not.toHaveBeenCalled() + expect(task.fileContextTracker.trackFileContext).not.toHaveBeenCalled() + expect(task.say).not.toHaveBeenCalled() + }) + + it("processed images arriving after abandonment do not start context tracking", async () => { + await fs.writeFile(path.join(workspace, "image.png"), Buffer.from([137, 80, 78, 71, 0])) + const started = deferred() + const image = deferred() + vi.spyOn(imageHelpers, "processImageFile").mockImplementationOnce(() => { + started.resolve() + return image.promise + }) + const pending = read({ path: "image.png" }) + await started.promise + task.abandoned = true + image.resolve({ + dataUrl: "data:image/png;base64,bGF0ZQ==", + buffer: Buffer.from("late"), + sizeInKB: 1, + sizeInMB: 0.001, + notice: "late image", + }) + const result = await pending + + expect(task.fileContextTracker.trackFileContext).not.toHaveBeenCalled() + expect(result).not.toContain("bGF0ZQ==") + expect(task.say).not.toHaveBeenCalled() + }) + + it("cancellation during an already-started diagnostic retains approval feedback without marking failure", async () => { + task.ask.mockResolvedValueOnce({ response: "yesButtonClicked", text: "Inspect only" }) + vi.mocked(fs.open).mockRejectedValueOnce(new Error("read diagnostic")) + const started = deferred() + const release = deferred() + task.say.mockImplementation(async (type) => { + if (type === "error") { + started.resolve() + await release.promise + } + }) + const pending = read({ path: external }) + await started.promise + task.abort = true + release.resolve() + const result = await pending + + expect(task.didToolFailInCurrentTurn).toBe(false) + expect(task.didRejectTool).toBe(false) + expect(result).toContain("Inspect only") + expect(result).not.toContain("read diagnostic") + }) + + it.each([false, true])("abort during the outer diagnostic does not publish failure (reject=%s)", async (reject) => { + task.ask.mockRejectedValueOnce(new Error("approval technical failure")) + const started = deferred() + const release = deferred() + task.say.mockImplementationOnce(async () => { + started.resolve() + await release.promise + }) + const pending = read({ path: external }) + await started.promise + task.abort = true + if (reject) release.reject(new Error("late diagnostic rejection")) + else release.resolve() + await pending + + expect(task.didToolFailInCurrentTurn).toBe(false) + expect(callbacks.pushToolResult).not.toHaveBeenCalled() + }) + + it.each([false, true])( + "handle reads pinned text after opening and preserves ranges (legacy=%s)", + async (legacy) => { + const file = path.join(workspace, "pinned.txt") + const replacement = path.join(directory, "other.txt") + await fs.writeFile(file, "first\nsecond\nthird") + await fs.writeFile(replacement, "unapproved replacement text") + await replaceAfterOpening(file, replacement) + // This host double intentionally omits Task members unrelated to file reading. + await readFileTool.handle( + task as unknown as Task, + { + type: "tool_use", + name: "read_file", + params: {}, + partial: false, + nativeArgs: legacy + ? { files: [{ path: "pinned.txt", lineRanges: [{ start: 2, end: 3 }] }], _legacyFormat: true } + : { path: "pinned.txt", offset: 2, limit: 2 }, + }, + callbacks, + ) + + expect(callbacks.pushToolResult).toHaveBeenCalledWith("File: pinned.txt\n2 | second\n3 | third") + expect(fs.readFile).not.toHaveBeenCalled() + expect(task.didToolFailInCurrentTurn).toBe(false) + }, + ) + + it.each( + [ + "approval", + "realpath", + "fstat", + "probe", + "binary", + "provider", + "text", + "image", + "document", + "tracking", + "close", + ].flatMap((boundary) => + (boundary === "document" ? [false] : [false, true]).map((legacy) => ({ boundary, legacy })), + ), + )( + "late rejection at $boundary is suppressed after cancellation and descriptors are released (legacy=$legacy)", + async ({ boundary, legacy }) => { + const started = deferred() + const failure = deferred() + const fail = () => { + started.resolve() + return failure.promise + } + const filePath = + boundary === "image" || boundary === "provider" + ? "late.png" + : boundary === "document" + ? "late.docx" + : external + if (filePath !== external) await fs.writeFile(path.join(workspace, filePath), "approved bytes") + task.ask.mockResolvedValueOnce({ response: "yesButtonClicked", text: "Inspect only" }) + if (boundary === "approval") task.ask.mockReset().mockImplementationOnce(fail) + if (boundary === "realpath") vi.mocked(fs.realpath).mockImplementationOnce(fail) + if (boundary === "binary") vi.mocked(isBinaryFile).mockImplementationOnce(fail) + if (boundary === "provider") { + vi.mocked(isBinaryFile).mockResolvedValue(true) + vi.spyOn(task.providerRef, "deref").mockReturnValue({ getState: fail }) + } + if (boundary === "image") { + vi.mocked(isBinaryFile).mockResolvedValue(true) + vi.spyOn(imageHelpers, "processImageFile").mockImplementationOnce(fail) + } + if (boundary === "document") { + vi.mocked(isBinaryFile).mockResolvedValue(true) + vi.spyOn(mammoth, "extractRawText").mockImplementationOnce(fail) + } + if (boundary === "tracking") task.fileContextTracker.trackFileContext.mockImplementationOnce(fail) + await inspectOpenedFile((file) => { + if (boundary === "fstat") vi.spyOn(file, "stat").mockImplementationOnce(fail) + if (boundary === "probe") vi.spyOn(file, "read").mockImplementationOnce(fail) + if (boundary === "text") vi.spyOn(file, "readFile").mockImplementationOnce(fail) + if (boundary === "close") { + const close = file.close.bind(file) + vi.spyOn(file, "close").mockImplementationOnce(async () => { + await close() + return fail() + }) + } + }) + const pending = read(legacy ? { files: [{ path: filePath }], _legacyFormat: true } : { path: filePath }) + await started.promise + task.abort = true + failure.reject(new Error("late technical rejection")) + const result = await pending + + expect(task.didToolFailInCurrentTurn).toBe(false) + expect(task.didRejectTool).toBe(false) + expect(task.say.mock.calls.some(([type]) => type === "error")).toBe(false) + expect(result).not.toContain("late technical rejection") + if (boundary !== "approval" && boundary !== "realpath") expect(result).toContain("Inspect only") + }, + ) + + it.each( + ["ENOENT", "ENOTDIR", "EACCES", "ELOOP"].flatMap((code) => [false, true].map((legacy) => ({ code, legacy }))), + )("cancelled $code resolution does not start access or approval (legacy=$legacy)", async ({ code, legacy }) => { + const started = deferred() + const resolution = deferred() + vi.mocked(fs.realpath).mockImplementationOnce(() => { + started.resolve() + return resolution.promise + }) + const pending = read(legacy ? { files: [{ path: external }], _legacyFormat: true } : { path: external }) + await started.promise + task.abort = true + resolution.reject(Object.assign(new Error("resolution failed after cancellation"), { code })) + await pending + + expect(fs.lstat).not.toHaveBeenCalled() + expect(task.ask).not.toHaveBeenCalled() + expect(task.say).not.toHaveBeenCalled() + expect(fs.open).not.toHaveBeenCalled() + expect(fs.readFile).not.toHaveBeenCalled() + expect(task.didToolFailInCurrentTurn).toBe(false) + }) + + it.each( + ["approved feedback", "denied feedback", "rejected feedback", "tracking", "closing"].flatMap((boundary) => + [false, true].map((legacy) => ({ boundary, legacy })), + ), + )( + "cancellation during $boundary discards content while retaining received approval feedback (legacy=$legacy)", + async ({ boundary, legacy }) => { + task.ask.mockResolvedValueOnce({ + response: boundary === "denied feedback" ? "noButtonClicked" : "yesButtonClicked", + text: "Inspect only", + }) + const started = deferred() + const release = deferred() + const wait = () => { + started.resolve() + return release.promise + } + if (boundary.includes("feedback")) + task.say.mockImplementationOnce(async () => { + await wait() + return undefined + }) + if (boundary === "tracking") task.fileContextTracker.trackFileContext.mockImplementationOnce(wait) + if (boundary === "closing") { + await inspectOpenedFile((file) => { + const close = file.close.bind(file) + vi.spyOn(file, "close").mockImplementationOnce(async () => { + await wait() + await close() + }) + }) + } + const pending = read(legacy ? { files: [{ path: external }], _legacyFormat: true } : { path: external }) + await started.promise + task.abort = true + if (boundary === "rejected feedback") release.reject(new Error("late feedback rejection")) + else release.resolve() + const result = await pending + + expect(result).toContain("Inspect only") + expect(result).not.toContain("synthetic external secret") + expect(result).not.toContain("late feedback rejection") + expect(task.didToolFailInCurrentTurn).toBe(false) + expect(task.didRejectTool).toBe(false) + if (boundary.includes("feedback")) expect(fs.open).not.toHaveBeenCalled() + }, + ) + + it.each([false, true])( + "reports a parent retarget during identity capture before approval (legacy=%s)", + async (legacy) => { + const parent = path.join(directory, "parent") + const replacement = path.join(directory, "replacement") + await fs.mkdir(parent) + await fs.mkdir(replacement) + const file = path.join(parent, "file.txt") + await fs.writeFile(file, "approved bytes") + await fs.writeFile(path.join(replacement, "file.txt"), "unapproved parent bytes") + const actual = await vi.importActual("fs/promises") + vi.mocked(fs.lstat).mockImplementationOnce(async (...args) => { + await fs.rename(parent, `${parent}.original`) + await fs.symlink(replacement, parent) + return actual.lstat(...args) + }) + + const result = await read(legacy ? { files: [{ path: file }], _legacyFormat: true } : { path: file }) + + expect(result).toContain("File target changed before approval") + expect(result).not.toContain("unapproved parent bytes") + expect(task.didToolFailInCurrentTurn).toBe(true) + expect(task.ask).not.toHaveBeenCalled() + expect(fs.open).not.toHaveBeenCalled() + expect(fs.readFile).not.toHaveBeenCalled() + }, + ) + + it("the real ignore controller blocks aliases to ignored internal files", async () => { + await fs.writeFile(path.join(workspace, ".rooignore"), "ignored.txt\n") + await fs.writeFile(path.join(workspace, "ignored.txt"), "ignored real secret") + await fs.symlink(path.join(workspace, "ignored.txt"), path.join(workspace, "alias.txt")) + const controller = new RooIgnoreController(workspace) + try { + await controller.initialize() + vi.mocked(fs.readFile).mockClear() + task.rooIgnoreController.validateAccess.mockImplementation((file) => controller.validateAccess(file)) + + const result = await read({ path: "alias.txt" }) + + expect(result).not.toContain("ignored real secret") + expect(task.ask).not.toHaveBeenCalled() + expect(fs.open).not.toHaveBeenCalled() + expect(fs.readFile).not.toHaveBeenCalled() + } finally { + controller.dispose() + } + }) + + it("failed real notebook extraction closes the descriptor and keeps uncancelled errors visible", async () => { + await fs.writeFile(path.join(workspace, "invalid.ipynb"), "invalid notebook JSON") + vi.mocked(isBinaryFile).mockResolvedValue(true) + + const result = await read({ path: "invalid.ipynb" }) + + expect(result).toContain("Error:") + expect(task.say.mock.calls.some(([type]) => type === "error")).toBe(true) + expect(task.didToolFailInCurrentTurn).toBe(true) + expect(task.fileContextTracker.trackFileContext).not.toHaveBeenCalled() + expect(openedFiles).toHaveLength(1) + expect(openedFiles[0].fd).toBe(-1) + expect(fs.readFile).not.toHaveBeenCalled() + }) + + it("legacy binary probe failures retain the historical pinned text fallback", async () => { + await fs.writeFile(path.join(workspace, "fallback.txt"), "fallback content") + await inspectOpenedFile((file) => { + vi.spyOn(file, "read").mockRejectedValueOnce(new Error("transient probe failure")) + }) + + await read({ files: [{ path: "fallback.txt" }], _legacyFormat: true }) + + expect(callbacks.pushToolResult).toHaveBeenCalledWith("File: fallback.txt\n1 | fallback content") + expect(task.didToolFailInCurrentTurn).toBe(false) + expect(fs.readFile).not.toHaveBeenCalled() + }) + + it("cancelled approval retains received image-only feedback", async () => { + const started = deferred() + const approval = deferred>>() + task.ask.mockImplementationOnce(() => { + started.resolve() + return approval.promise + }) + const pending = read({ path: external }) + await started.promise + task.abort = true + approval.resolve({ response: "yesButtonClicked", images: ["data:image/png;base64,dXNlciBmZWVkYmFjaw=="] }) + const result = await pending + + expect(result).toContain("dXNlciBmZWVkYmFjaw==") + expect(result).not.toContain("synthetic external secret") + expect(task.say).not.toHaveBeenCalled() + expect(fs.open).not.toHaveBeenCalled() + }) + + it("external approval remains pending without content I/O and then returns the approved bytes", async () => { + const started = deferred() + const approval = deferred>>() + manualApproval.mockImplementationOnce(() => { + started.resolve() + return approval.promise + }) + const pending = read({ path: external }) + await started.promise + + expect(fs.open).not.toHaveBeenCalled() + expect(fs.readFile).not.toHaveBeenCalled() + expect(callbacks.pushToolResult).not.toHaveBeenCalled() + approval.resolve({ response: "yesButtonClicked" }) + const result = await pending + + expect(result).toContain("1 | synthetic external secret") + expect(task.didToolFailInCurrentTurn).toBe(false) + }) + + it("provider state arriving after abort does not start verified I/O or image processing", async () => { + await fs.writeFile(path.join(workspace, "image.png"), Buffer.from([137, 80, 78, 71, 0])) + const started = deferred() + const release = deferred() + task.providerRef.deref = () => ({ + getState: async () => { + started.resolve() + await release.promise + return {} + }, + }) + const validation = vi.spyOn(imageHelpers, "validateImageForProcessing") + const pending = read({ path: "image.png" }) + await started.promise + task.abort = true + release.resolve() + await pending + + expect(fs.open).not.toHaveBeenCalled() + expect(validation).not.toHaveBeenCalled() + expect(task.say).not.toHaveBeenCalled() + expect(task.didToolFailInCurrentTurn).toBe(false) + }) +}) diff --git a/src/core/tools/helpers/imageHelpers.ts b/src/core/tools/helpers/imageHelpers.ts index a1adb078e6..9dd4163d4e 100644 --- a/src/core/tools/helpers/imageHelpers.ts +++ b/src/core/tools/helpers/imageHelpers.ts @@ -3,6 +3,8 @@ import * as fs from "fs/promises" import { t } from "../../../i18n" import prettyBytes from "pretty-bytes" +import type { ReadFileCancellation } from "../file-reading/ReadFileCancellation" + /** * Default maximum allowed image file size in bytes (5MB) */ @@ -72,8 +74,13 @@ export interface ImageProcessingResult { /** * Reads an image file and returns both the data URL and buffer */ -export async function readImageAsDataUrlWithBuffer(filePath: string): Promise<{ dataUrl: string; buffer: Buffer }> { - const fileBuffer = await fs.readFile(filePath) +export async function readImageAsDataUrlWithBuffer( + filePath: string, + file?: fs.FileHandle, + cancellation?: ReadFileCancellation, +): Promise<{ dataUrl: string; buffer: Buffer }> { + const fileBuffer = file ? await file.readFile() : await fs.readFile(filePath) + cancellation?.throwIfCancelled() const base64 = fileBuffer.toString("base64") const ext = path.extname(filePath).toLowerCase() @@ -99,6 +106,7 @@ export async function validateImageForProcessing( maxImageFileSize: number, maxTotalImageSize: number, currentTotalMemoryUsed: number, + file?: fs.FileHandle, ): Promise { // Check if model supports images if (!supportsImages) { @@ -109,7 +117,7 @@ export async function validateImageForProcessing( } } - const imageStats = await fs.stat(fullPath) + const imageStats = file ? await file.stat() : await fs.stat(fullPath) const imageSizeInMB = imageStats.size / (1024 * 1024) // Check individual file size limit @@ -147,9 +155,14 @@ export async function validateImageForProcessing( /** * Processes an image file and returns the result */ -export async function processImageFile(fullPath: string): Promise { - const imageStats = await fs.stat(fullPath) - const { dataUrl, buffer } = await readImageAsDataUrlWithBuffer(fullPath) +export async function processImageFile( + fullPath: string, + file?: fs.FileHandle, + cancellation?: ReadFileCancellation, +): Promise { + const imageStats = file ? await file.stat() : await fs.stat(fullPath) + cancellation?.throwIfCancelled() + const { dataUrl, buffer } = await readImageAsDataUrlWithBuffer(fullPath, file, cancellation) const imageSizeInKB = Math.round(imageStats.size / 1024) const imageSizeInMB = imageStats.size / (1024 * 1024) const noticeText = t("tools:readFile.imageWithSize", { size: imageSizeInKB })