Skip to content

Commit c10dece

Browse files
WebMadWebMad
committed
fix(read-file): bind approved reads and honor cancellation
Keep the symlinked-workspace approval prerequisite unchanged. Pin approved targets to verified descriptors, consume descriptor-bound text/document/image bytes, close resources on all paths, and honor cancellation in both native and legacy readers while preserving received approval feedback.
1 parent 2f5f7d1 commit c10dece

10 files changed

Lines changed: 2243 additions & 188 deletions

‎src/core/tools/ReadFileTool.ts‎

Lines changed: 293 additions & 172 deletions
Large diffs are not rendered by default.

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

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ import { isBinaryFile } from "isbinaryfile"
2222
import { readFileTool } from "../ReadFileTool"
2323
import { formatResponse } from "../../prompts/responses"
2424
import { validateImageForProcessing, processImageFile, isSupportedImageFormat } from "../helpers/imageHelpers"
25-
import { extractTextFromFile, getSupportedBinaryFormats } from "../../../integrations/misc/extract-text"
25+
import { extractTextFromBuffer, getSupportedBinaryFormats } from "../../../integrations/misc/extract-text"
2626
import { readWithIndentation, readWithSlice } from "../../../integrations/misc/indentation-reader"
2727

2828
// ─── Mocks ────────────────────────────────────────────────────────────────────
@@ -41,12 +41,23 @@ vi.mock("path", async () => {
4141
vi.mock("fs/promises", () => ({
4242
readFile: vi.fn(),
4343
stat: vi.fn(),
44+
realpath: vi.fn(async (file: string) => file),
45+
lstat: vi.fn(async () => ({ dev: 1n, ino: 1n, isSymbolicLink: () => false })),
46+
open: vi.fn(async (file: string) => {
47+
const mocked = await import("fs/promises")
48+
return {
49+
stat: async () => ({ ...(await mocked.stat(file)), dev: 1n, ino: 1n }),
50+
read: async () => ({ bytesRead: 0 }),
51+
readFile: async (encoding?: "utf8") => (encoding ? mocked.readFile(file, encoding) : mocked.readFile(file)),
52+
close: vi.fn().mockResolvedValue(undefined),
53+
}
54+
}),
4455
}))
4556

4657
vi.mock("isbinaryfile")
4758

4859
vi.mock("../../../integrations/misc/extract-text", () => ({
49-
extractTextFromFile: vi.fn(),
60+
extractTextFromBuffer: vi.fn(),
5061
addLineNumbers: vi.fn().mockImplementation((text: string, startLine = 1) => {
5162
if (!text) return ""
5263
const lines = text.split("\n")
@@ -120,7 +131,7 @@ const mockedFsReadFile = vi.mocked(fsPromises.readFile)
120131
const mockedFsStat = vi.mocked(fsPromises.stat)
121132

122133
const mockedIsBinaryFile = vi.mocked(isBinaryFile)
123-
const mockedExtractTextFromFile = vi.mocked(extractTextFromFile)
134+
const mockedExtractTextFromBuffer = vi.mocked(extractTextFromBuffer)
124135
const mockedReadWithSlice = vi.mocked(readWithSlice)
125136
const mockedReadWithIndentation = vi.mocked(readWithIndentation)
126137
const mockedIsSupportedImageFormat = vi.mocked(isSupportedImageFormat)
@@ -420,24 +431,34 @@ describe("ReadFileTool", () => {
420431
it("should extract text from PDF files", async () => {
421432
const mockTask = createMockTask()
422433
const callbacks = createMockCallbacks()
434+
const source = Buffer.from("%PDF-1.7\nsynthetic PDF bytes")
423435

424-
mockedExtractTextFromFile.mockResolvedValue("PDF content here")
436+
mockedFsReadFile.mockResolvedValue(source)
437+
mockedExtractTextFromBuffer.mockResolvedValue("PDF content here")
425438

426439
await readFileTool.execute({ path: "document.pdf" }, mockTask as any, callbacks)
427440

428-
expect(mockedExtractTextFromFile).toHaveBeenCalled()
441+
expect(mockedExtractTextFromBuffer).toHaveBeenCalledExactlyOnceWith(
442+
source,
443+
path.resolve(mockTask.cwd, "document.pdf"),
444+
)
429445
expect(callbacks.pushToolResult).toHaveBeenCalledWith(expect.stringContaining("PDF content here"))
430446
})
431447

432448
it("should extract text from DOCX files", async () => {
433449
const mockTask = createMockTask()
434450
const callbacks = createMockCallbacks()
451+
const source = Buffer.from("PK\u0003\u0004synthetic DOCX bytes")
435452

436-
mockedExtractTextFromFile.mockResolvedValue("DOCX content here")
453+
mockedFsReadFile.mockResolvedValue(source)
454+
mockedExtractTextFromBuffer.mockResolvedValue("DOCX content here")
437455

438456
await readFileTool.execute({ path: "document.docx" }, mockTask as any, callbacks)
439457

440-
expect(mockedExtractTextFromFile).toHaveBeenCalled()
458+
expect(mockedExtractTextFromBuffer).toHaveBeenCalledExactlyOnceWith(
459+
source,
460+
path.resolve(mockTask.cwd, "document.docx"),
461+
)
441462
expect(callbacks.pushToolResult).toHaveBeenCalledWith(expect.stringContaining("DOCX content here"))
442463
})
443464

@@ -457,7 +478,7 @@ describe("ReadFileTool", () => {
457478
const mockTask = createMockTask()
458479
const callbacks = createMockCallbacks()
459480

460-
mockedExtractTextFromFile.mockRejectedValue(new Error("Extraction failed"))
481+
mockedExtractTextFromBuffer.mockRejectedValue(new Error("Extraction failed"))
461482

462483
await readFileTool.execute({ path: "corrupt.pdf" }, mockTask as any, callbacks)
463484

@@ -1008,7 +1029,7 @@ describe("ReadFileTool", () => {
10081029
)
10091030
expect(mockedFsReadFile).not.toHaveBeenCalled()
10101031
expect(mockedFsStat).not.toHaveBeenCalled()
1011-
expect(mockedExtractTextFromFile).not.toHaveBeenCalled()
1032+
expect(mockedExtractTextFromBuffer).not.toHaveBeenCalled()
10121033
},
10131034
)
10141035

@@ -1033,7 +1054,7 @@ describe("ReadFileTool", () => {
10331054
)
10341055
expect(mockedFsReadFile).not.toHaveBeenCalled()
10351056
expect(mockedFsStat).not.toHaveBeenCalled()
1036-
expect(mockedExtractTextFromFile).not.toHaveBeenCalled()
1057+
expect(mockedExtractTextFromBuffer).not.toHaveBeenCalled()
10371058
})
10381059

10391060
it.each(["src/app.ts", "src/файл.ts", "src/e\u0301-📚.ts"])(

‎src/core/tools/file-reading/ReadFileCancellation.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
import type { Task } from "../../task/Task"
2+
3+
import { ReadFileCancelledError } from "./ReadFileCancelledError"
4+
5+
/** Observe existing task cancellation flags; never change lifecycle state. */
6+
export class ReadFileCancellation {
7+
constructor(private readonly task: Pick<Task, "abort" | "abandoned">) {}
8+
9+
isCancelled(): boolean {
10+
return this.task.abort || this.task.abandoned
11+
}
12+
13+
throwIfCancelled(): void {
14+
if (this.isCancelled()) throw new ReadFileCancelledError()
15+
}
16+
17+
/** Guard value-producing operations; resource acquisition needs its own cleanup scope. */
18+
async run<T>(operation: () => Promise<T>): Promise<T> {
19+
this.throwIfCancelled()
20+
21+
let result: T
22+
try {
23+
result = await operation()
24+
} catch (error) {
25+
if (error instanceof ReadFileCancelledError) throw error
26+
this.throwIfCancelled()
27+
throw error
28+
}
29+
30+
this.throwIfCancelled()
31+
return result
32+
}
33+
}

‎src/core/tools/file-reading/ReadFileCancelledError.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
export class ReadFileCancelledError extends Error {
2+
constructor() {
3+
super("File read cancelled")
4+
this.name = "ReadFileCancelledError"
5+
}
6+
}

‎src/core/tools/file-reading/ReadFileTarget.ts‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
import * as fs from "fs/promises"
2+
import { constants, type BigIntStats } from "fs"
3+
import path from "path"
4+
import * as vscode from "vscode"
5+
6+
import type { ReadFileCancellation } from "./ReadFileCancellation"
7+
import { ReadFileCancelledError } from "./ReadFileCancelledError"
8+
9+
/** Metadata only until approval; content consumers must never reopen fullPath. */
10+
export class ReadFileTarget {
11+
private constructor(
12+
readonly fullPath: string,
13+
private readonly cancellation: ReadFileCancellation,
14+
private readonly identity?: BigIntStats,
15+
private readonly missingError?: unknown,
16+
) {}
17+
18+
static async resolve(lexicalPath: string, cancellation: ReadFileCancellation): Promise<ReadFileTarget> {
19+
try {
20+
const fullPath = await cancellation.run(() => fs.realpath(lexicalPath))
21+
const identity = await cancellation.run(() => fs.lstat(fullPath, { bigint: true }))
22+
if (identity.isSymbolicLink()) {
23+
throw new Error(`File target changed before approval: ${lexicalPath}`)
24+
}
25+
const revalidatedPath = await cancellation.run(() => fs.realpath(fullPath))
26+
if (revalidatedPath !== fullPath) {
27+
throw new Error(`File target changed before approval: ${lexicalPath}`)
28+
}
29+
return new ReadFileTarget(fullPath, cancellation, identity)
30+
} catch (error) {
31+
if (error instanceof ReadFileCancelledError) throw error
32+
// Filesystem diagnostics, including inaccessible parents, require approval.
33+
// Code-less identity races and programmer errors must still fail closed.
34+
if (error instanceof Error && "code" in error && typeof error.code === "string") {
35+
return new ReadFileTarget(lexicalPath, cancellation, undefined, error)
36+
}
37+
throw error
38+
}
39+
}
40+
41+
static async isOutsideWorkspace(fullPath: string): Promise<boolean> {
42+
const roots = await Promise.all(
43+
(vscode.workspace.workspaceFolders ?? []).map((folder) =>
44+
fs.realpath(folder.uri.fsPath).catch(() => undefined),
45+
),
46+
)
47+
return !roots.some((root) => {
48+
if (!root) return false
49+
const relative = path.relative(root, fullPath)
50+
return relative !== ".." && !relative.startsWith(`..${path.sep}`) && !path.isAbsolute(relative)
51+
})
52+
}
53+
54+
async withHandle<T>(read: (file: fs.FileHandle, stats: BigIntStats) => Promise<T>): Promise<T> {
55+
return this.cancellation.run(async () => {
56+
if (!this.identity) throw this.missingError
57+
const file = await fs.open(this.fullPath, constants.O_RDONLY | (constants.O_NOFOLLOW ?? 0))
58+
try {
59+
const stats = await this.cancellation.run(() => file.stat({ bigint: true }))
60+
if (stats.dev !== this.identity.dev || stats.ino !== this.identity.ino) {
61+
throw new Error(`File target changed after approval: ${this.fullPath}`)
62+
}
63+
return await read(file, stats)
64+
} finally {
65+
await file.close()
66+
}
67+
})
68+
}
69+
}

‎src/core/tools/file-reading/__tests__/ReadFileCancellation.spec.ts‎

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
import { ReadFileCancellation } from "../ReadFileCancellation"
2+
import { ReadFileCancelledError } from "../ReadFileCancelledError"
3+
4+
describe("ReadFileCancellation", () => {
5+
it.each([
6+
{ abort: false, abandoned: false, cancelled: false },
7+
{ abort: true, abandoned: false, cancelled: true },
8+
{ abort: false, abandoned: true, cancelled: true },
9+
{ abort: true, abandoned: true, cancelled: true },
10+
])("observes abort=$abort and abandoned=$abandoned without changing them", ({ abort, abandoned, cancelled }) => {
11+
const task = { abort, abandoned }
12+
const cancellation = new ReadFileCancellation(task)
13+
14+
expect(cancellation.isCancelled()).toBe(cancelled)
15+
if (cancelled) {
16+
expect(() => cancellation.throwIfCancelled()).toThrow(ReadFileCancelledError)
17+
} else {
18+
expect(() => cancellation.throwIfCancelled()).not.toThrow()
19+
}
20+
expect(task).toEqual({ abort, abandoned })
21+
})
22+
23+
it.each(["abort", "abandoned"] as const)(
24+
"observes later changes to %s rather than capturing a snapshot",
25+
(flag) => {
26+
const task = { abort: false, abandoned: false }
27+
const cancellation = new ReadFileCancellation(task)
28+
29+
expect(cancellation.isCancelled()).toBe(false)
30+
task[flag] = true
31+
expect(cancellation.isCancelled()).toBe(true)
32+
expect(() => cancellation.throwIfCancelled()).toThrow("File read cancelled")
33+
task[flag] = false
34+
expect(cancellation.isCancelled()).toBe(false)
35+
expect(() => cancellation.throwIfCancelled()).not.toThrow()
36+
},
37+
)
38+
39+
describe("checked async operations", () => {
40+
it("returns a successful result without changing task flags", async () => {
41+
const task = { abort: false, abandoned: false }
42+
const cancellation = new ReadFileCancellation(task)
43+
const result = { fullPath: "/workspace/file.txt" }
44+
const operation = vi.fn(async () => result)
45+
46+
await expect(cancellation.run(operation)).resolves.toBe(result)
47+
expect(operation).toHaveBeenCalledOnce()
48+
expect(task).toEqual({ abort: false, abandoned: false })
49+
})
50+
51+
it.each(["abort", "abandoned"] as const)("does not start an operation when %s is already set", async (flag) => {
52+
const task = { abort: false, abandoned: false }
53+
task[flag] = true
54+
const operation = vi.fn(async () => "unexpected content")
55+
56+
await expect(new ReadFileCancellation(task).run(operation)).rejects.toBeInstanceOf(ReadFileCancelledError)
57+
expect(operation).not.toHaveBeenCalled()
58+
})
59+
60+
it.each(["abort", "abandoned"] as const)(
61+
"discards a result when %s changes during the operation",
62+
async (flag) => {
63+
const task = { abort: false, abandoned: false }
64+
const cancellation = new ReadFileCancellation(task)
65+
66+
await expect(
67+
cancellation.run(async () => {
68+
task[flag] = true
69+
return "late content"
70+
}),
71+
).rejects.toBeInstanceOf(ReadFileCancelledError)
72+
},
73+
)
74+
75+
it.each(["abort", "abandoned"] as const)(
76+
"prioritizes cancellation over a late rejection after %s",
77+
async (flag) => {
78+
const task = { abort: false, abandoned: false }
79+
const cancellation = new ReadFileCancellation(task)
80+
const filesystemError = Object.assign(new Error("late filesystem failure"), { code: "ENOENT" })
81+
82+
await expect(
83+
cancellation.run(async () => {
84+
task[flag] = true
85+
throw filesystemError
86+
}),
87+
).rejects.toBeInstanceOf(ReadFileCancelledError)
88+
},
89+
)
90+
91+
it("preserves the original cancellation error and stack without checking cancellation again", async () => {
92+
const task = { abort: false, abandoned: false }
93+
const cancellation = new ReadFileCancellation(task)
94+
const error = new ReadFileCancelledError()
95+
const originalStack = error.stack
96+
const check = vi.spyOn(cancellation, "throwIfCancelled")
97+
98+
await expect(
99+
cancellation.run(async () => {
100+
task.abort = true
101+
throw error
102+
}),
103+
).rejects.toBe(error)
104+
expect(error.stack).toBe(originalStack)
105+
expect(check).toHaveBeenCalledOnce()
106+
})
107+
108+
it.each([new Error("filesystem failure"), "non-Error failure"])(
109+
"preserves an unrelated rejection when the task is active: %s",
110+
async (error) => {
111+
const cancellation = new ReadFileCancellation({ abort: false, abandoned: false })
112+
113+
await expect(
114+
cancellation.run(async () => {
115+
throw error
116+
}),
117+
).rejects.toBe(error)
118+
},
119+
)
120+
121+
it("handles synchronous operation failures without replacing the original error", async () => {
122+
const cancellation = new ReadFileCancellation({ abort: false, abandoned: false })
123+
const error = new Error("synchronous operation failure")
124+
125+
await expect(
126+
cancellation.run(() => {
127+
throw error
128+
}),
129+
).rejects.toBe(error)
130+
})
131+
})
132+
})

0 commit comments

Comments
 (0)