Skip to content

Commit b4e9d4b

Browse files
committed
refactor(read-file): layer reusable foundation on finalized safety
Retain the exact approval and safety prerequisite commits from PR #1999. Reuse verified target/cancellation and image I/O helpers, preserve request policy snapshots and presenter ownership, and keep the residual constructor-composed reader strategies, block formatting, raw-document bounds and numeric validation. Port all safety regressions through the new boundary; add exact decoder, ignored-target display and late-diagnostic tests without new lint suppressions.
1 parent c10dece commit b4e9d4b

42 files changed

Lines changed: 6766 additions & 2589 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎src/api/providers/__tests__/base-provider.spec.ts‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import type { ModelInfo } from "@roo-code/types"
55
import { BaseProvider } from "../base-provider"
66
import type { ApiStream } from "../../transform/stream"
77
import { expectNoLoneSurrogates } from "../../../test-utils/surrogates"
8+
import { createReadFileTool } from "../../../core/prompts/tools/native-tools/read_file"
89

910
// Create a concrete implementation for testing
1011
class TestProvider extends BaseProvider {
@@ -270,6 +271,37 @@ describe("BaseProvider", () => {
270271
expect(result.properties.only.type).toBe("null")
271272
})
272273
describe("convertToolsForOpenAI", () => {
274+
it("preserves read_file integer minima while requiring properties for strict mode", () => {
275+
const result = provider.testConvertToolsForOpenAI([createReadFileTool()])
276+
277+
expect(result?.[0]).toMatchObject({
278+
function: {
279+
strict: true,
280+
parameters: {
281+
required: ["path", "mode", "offset", "limit", "indentation"],
282+
properties: {
283+
offset: { type: "integer", minimum: 1 },
284+
limit: { type: "integer", minimum: 1 },
285+
indentation: {
286+
required: [
287+
"anchor_line",
288+
"max_levels",
289+
"include_siblings",
290+
"include_header",
291+
"max_lines",
292+
],
293+
properties: {
294+
anchor_line: { type: "integer", minimum: 1 },
295+
max_levels: { type: "integer", minimum: 0 },
296+
max_lines: { type: "integer", minimum: 1 },
297+
},
298+
},
299+
},
300+
},
301+
},
302+
})
303+
})
304+
273305
it("should return undefined for undefined input", () => {
274306
const result = provider.testConvertToolsForOpenAI(undefined)
275307
expect(result).toBeUndefined()

‎src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,38 @@ import { NativeToolCallParser, type ToolCallStreamEvent } from "../NativeToolCal
33
describe("NativeToolCallParser", () => {
44
describe("parseToolCall", () => {
55
describe("read_file tool", () => {
6+
it("coerces numeric strings before reader-owned validation", () => {
7+
expect(
8+
NativeToolCallParser.parseToolCall({
9+
id: "numeric_strings",
10+
name: "read_file",
11+
arguments: JSON.stringify({
12+
path: "test.ts",
13+
offset: "2",
14+
limit: "1",
15+
indentation: { anchor_line: "3", max_levels: "0", max_lines: "4" },
16+
}),
17+
}),
18+
).toMatchObject({
19+
nativeArgs: {
20+
path: "test.ts",
21+
offset: 2,
22+
limit: 1,
23+
indentation: { anchor_line: 3, max_levels: 0, max_lines: 4 },
24+
},
25+
})
26+
})
27+
28+
it.each([0, -1, 1.5])("retains numeric limit %s for precise execution diagnostics", (limit) => {
29+
expect(
30+
NativeToolCallParser.parseToolCall({
31+
id: "reader_validation",
32+
name: "read_file",
33+
arguments: JSON.stringify({ path: "test.ts", limit }),
34+
}),
35+
).toMatchObject({ nativeArgs: { path: "test.ts", limit } })
36+
})
37+
638
it("should parse minimal single-file read_file args", () => {
739
const toolCall = {
840
id: "toolu_123",

‎src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
import { describe, it, expect, beforeEach, vi } from "vitest"
44
import { presentAssistantMessage } from "../presentAssistantMessage"
55
import { validateToolUse } from "../../tools/validateToolUse"
6-
import { readFileTool } from "../../tools/ReadFileTool"
6+
import { ReadFileTool } from "../../tools/file-reading/ReadFileTool"
77
import type { RequestPolicySnapshot } from "../../prompts/tools/effective-tool-policy"
88

99
// Mock dependencies
@@ -399,7 +399,7 @@ describe("presentAssistantMessage - Custom Tool Recording", () => {
399399
// longer re-reads provider state before dispatch (issue #1642), so the
400400
// dispatch-time throw this test pins is triggered by the dispatched tool
401401
// handler itself.
402-
const handleSpy = vi.spyOn(readFileTool, "handle").mockRejectedValue(new Error("dispatch failed"))
402+
const handleSpy = vi.spyOn(ReadFileTool.prototype, "handle").mockRejectedValue(new Error("dispatch failed"))
403403

404404
try {
405405
await expect(presentAssistantMessage(mockTask, baseSnapshot)).rejects.toThrow("dispatch failed")

‎src/core/assistant-message/__tests__/presentAssistantMessage-tool-repetition.spec.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -65,10 +65,10 @@ const { readFileHandle, attemptCompletionHandle } = vi.hoisted(() => ({
6565
attemptCompletionHandle: vi.fn(async () => {}),
6666
}))
6767

68-
vi.mock("../../tools/ReadFileTool", () => ({
69-
readFileTool: {
70-
handle: readFileHandle,
71-
getReadFileToolDescription: vi.fn(() => "[read_file]"),
68+
vi.mock("../../tools/file-reading/ReadFileTool", () => ({
69+
ReadFileTool: class {
70+
handle = readFileHandle
71+
getReadFileToolDescription = vi.fn(() => "[read_file]")
7272
},
7373
}))
7474

‎src/core/assistant-message/__tests__/presentAssistantMessage-tool-usage-attribution.spec.ts‎

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { TelemetryEventName } from "@roo-code/types"
55
import { describe, it, expect, beforeEach, vi, type Mock } from "vitest"
66
import { presentAssistantMessage } from "../presentAssistantMessage"
77
import { validateToolUse } from "../../tools/validateToolUse"
8+
import { ReadFileTool } from "../../tools/file-reading/ReadFileTool"
89
import { getModeBySlug } from "../../../shared/modes"
910
import type { Task } from "../../task/Task"
1011
import type { RequestPolicySnapshot } from "../../prompts/tools/effective-tool-policy"
@@ -168,6 +169,127 @@ describe("presentAssistantMessage - tool usage attribution", () => {
168169
expect(TelemetryService.instance.captureToolUsage).toHaveBeenCalledWith(mockTask.taskId, "read_file")
169170
})
170171

172+
describe("shared read_file tool", () => {
173+
it("reuses one instance for partial and complete calls across tasks", async () => {
174+
const handle = vi.spyOn(ReadFileTool.prototype, "handle").mockResolvedValue(undefined)
175+
const otherTask = { ...mockTask, taskId: "other-task-id", instanceId: "other-instance" }
176+
177+
try {
178+
for (const task of [mockTask, otherTask]) {
179+
for (const partial of [true, false]) {
180+
task.assistantMessageContent = [
181+
{
182+
type: "tool_use",
183+
id: `call_${task.taskId}`,
184+
name: "read_file",
185+
params: { path: "test.txt" },
186+
nativeArgs: { path: "test.txt" },
187+
partial,
188+
},
189+
]
190+
191+
// This dispatch double omits Task fields unrelated to tool presentation.
192+
await presentAssistantMessage(task as unknown as Task, baseSnapshot)
193+
}
194+
}
195+
196+
expect(handle).toHaveBeenCalledTimes(4)
197+
const sharedInstance = handle.mock.contexts[0]
198+
expect(sharedInstance).toBeInstanceOf(ReadFileTool)
199+
for (const instance of handle.mock.contexts) {
200+
expect(instance).toBe(sharedInstance)
201+
}
202+
} finally {
203+
handle.mockRestore()
204+
}
205+
})
206+
207+
it("preserves the injected reader when recursively processing tool calls", async () => {
208+
const reader = {
209+
handle: vi.fn<ReadFileTool["handle"]>().mockResolvedValue(undefined),
210+
getReadFileToolDescription: vi.fn(() => ""),
211+
}
212+
mockTask.assistantMessageContent = ["first.txt", "second.txt"].map((path, index) => ({
213+
type: "tool_use",
214+
id: `call_read_${index}`,
215+
name: "read_file",
216+
params: { path },
217+
nativeArgs: { path },
218+
partial: false,
219+
}))
220+
221+
// This dispatch double omits Task fields unrelated to tool presentation.
222+
await presentAssistantMessage(mockTask as unknown as Task, baseSnapshot, reader)
223+
224+
expect(reader.handle).toHaveBeenCalledTimes(2)
225+
for (const instance of reader.handle.mock.contexts) {
226+
expect(instance).toBe(reader)
227+
}
228+
})
229+
230+
it("preserves the injected reader when processing pending streaming updates", async () => {
231+
const reader = {
232+
handle: vi
233+
.fn<ReadFileTool["handle"]>()
234+
.mockResolvedValue(undefined)
235+
.mockImplementationOnce(async (task) => {
236+
task.presentAssistantMessageHasPendingUpdates = true
237+
}),
238+
getReadFileToolDescription: vi.fn(() => ""),
239+
}
240+
mockTask.assistantMessageContent = [
241+
{
242+
type: "tool_use",
243+
id: "call_streaming_read",
244+
name: "read_file",
245+
params: { path: "test.txt" },
246+
nativeArgs: { path: "test.txt" },
247+
partial: true,
248+
},
249+
]
250+
251+
// This dispatch double omits Task fields unrelated to tool presentation.
252+
await presentAssistantMessage(mockTask as unknown as Task, baseSnapshot, reader)
253+
254+
expect(reader.handle).toHaveBeenCalledTimes(2)
255+
for (const instance of reader.handle.mock.contexts) {
256+
expect(instance).toBe(reader)
257+
}
258+
})
259+
260+
it.each([
261+
{ nativeArgs: { path: "native.txt" }, expectedPath: "native.txt" },
262+
{ nativeArgs: undefined, expectedPath: "legacy.txt" },
263+
])("uses the injected reader to describe $expectedPath", async ({ nativeArgs, expectedPath }) => {
264+
const reader = new ReadFileTool()
265+
const describeTool = vi.spyOn(reader, "getReadFileToolDescription")
266+
mockTask.didRejectTool = true
267+
mockTask.assistantMessageContent = [
268+
{
269+
type: "tool_use",
270+
id: "call_rejected_read",
271+
name: "read_file",
272+
params: { path: "legacy.txt" },
273+
nativeArgs,
274+
partial: false,
275+
},
276+
]
277+
278+
try {
279+
// This dispatch double omits Task fields unrelated to tool presentation.
280+
await presentAssistantMessage(mockTask as unknown as Task, baseSnapshot, reader)
281+
282+
expect(describeTool).toHaveBeenCalledExactlyOnceWith("read_file", { path: expectedPath })
283+
expect(describeTool.mock.contexts[0]).toBe(reader)
284+
expect(mockTask.pushToolResultToUserContent).toHaveBeenCalledWith(
285+
expect.objectContaining({ content: expect.stringContaining(expectedPath) }),
286+
)
287+
} finally {
288+
describeTool.mockRestore()
289+
}
290+
})
291+
})
292+
171293
it("records a valid dynamic mcp_ tool name as use_mcp_tool", async () => {
172294
mockTask.assistantMessageContent = [
173295
{

‎src/core/assistant-message/presentAssistantMessage.ts‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import { AskIgnoredError } from "../task/AskIgnoredError"
1717
import { Task } from "../task/Task"
1818

1919
import { listFilesTool } from "../tools/ListFilesTool"
20-
import { readFileTool } from "../tools/ReadFileTool"
20+
import { ReadFileTool } from "../tools/file-reading/ReadFileTool"
2121
import { readCommandOutputTool } from "../tools/ReadCommandOutputTool"
2222
import { writeToFileTool } from "../tools/WriteToFileTool"
2323
import { editTool } from "../tools/EditTool"
@@ -44,6 +44,8 @@ import { codebaseSearchTool } from "../tools/CodebaseSearchTool"
4444
import { formatResponse } from "../prompts/responses"
4545
import { sanitizeToolUseId } from "../../utils/tool-id"
4646

47+
const defaultReadFileTool = new ReadFileTool()
48+
4749
/**
4850
* Maps a raw, potentially model-controlled tool name to a safe analytics key.
4951
* Never returns the raw name unless it is a known static tool, so an
@@ -86,7 +88,11 @@ export function toTelemetryToolName(
8688
* as it becomes available.
8789
*/
8890

89-
export async function presentAssistantMessage(cline: Task, policySnapshot: RequestPolicySnapshot) {
91+
export async function presentAssistantMessage(
92+
cline: Task,
93+
policySnapshot: RequestPolicySnapshot,
94+
readFileTool: Pick<ReadFileTool, "handle" | "getReadFileToolDescription"> = defaultReadFileTool,
95+
) {
9096
if (cline.abort) {
9197
return
9298
}
@@ -113,7 +119,7 @@ export async function presentAssistantMessage(cline: Task, policySnapshot: Reque
113119
// The last pending check and the release below run in the same
114120
// synchronous step, so no update can be stranded behind the lock.
115121
do {
116-
await presentAssistantMessageBlock(cline, policySnapshot)
122+
await presentAssistantMessageBlock(cline, policySnapshot, readFileTool)
117123
} while (!cline.abort && cline.presentAssistantMessageHasPendingUpdates)
118124
} finally {
119125
// Tool handlers and provider-state reads can reject. Never strand the
@@ -122,7 +128,11 @@ export async function presentAssistantMessage(cline: Task, policySnapshot: Reque
122128
}
123129
}
124130

125-
async function presentAssistantMessageBlock(cline: Task, policySnapshot: RequestPolicySnapshot): Promise<void> {
131+
async function presentAssistantMessageBlock(
132+
cline: Task,
133+
policySnapshot: RequestPolicySnapshot,
134+
readFileTool: Pick<ReadFileTool, "handle" | "getReadFileToolDescription">,
135+
): Promise<void> {
126136
if (cline.abort) {
127137
return
128138
}
@@ -1140,7 +1150,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request
11401150
if (cline.currentStreamingContentIndex < cline.assistantMessageContent.length) {
11411151
// There are already more content blocks to stream, so we'll call
11421152
// this function ourselves.
1143-
return await presentAssistantMessageBlock(cline, policySnapshot)
1153+
return await presentAssistantMessageBlock(cline, policySnapshot, readFileTool)
11441154
} else {
11451155
// CRITICAL FIX: If we're out of bounds and the stream is complete, set userMessageContentReady
11461156
// This handles the case where assistantMessageContent is empty or becomes empty after processing

‎src/core/mentions/index.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import { getCommitInfo, getWorkingState } from "../../utils/git"
1111
import { openFile } from "../../integrations/misc/open-file"
1212
import { extractTextFromFileWithMetadata, type ExtractTextResult } from "../../integrations/misc/extract-text"
1313
import { diagnosticsToProblemsString } from "../../integrations/diagnostics"
14-
import { DEFAULT_LINE_LIMIT } from "../prompts/tools/native-tools/read_file"
14+
import { DEFAULT_LINE_LIMIT } from "../tools/file-reading/readFileConstants"
1515

1616
import { FileContextTracker } from "../context-tracking/FileContextTracker"
1717

0 commit comments

Comments
 (0)