refactor: isolate reusable file-reading foundation (part of #1948)

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

@@ -5,6 +5,7 @@ import type { ModelInfo } from "@roo-code/types"
 import { BaseProvider } from "../base-provider"
 import type { ApiStream } from "../../transform/stream"
 import { expectNoLoneSurrogates } from "../../../test-utils/surrogates"
+import { createReadFileTool } from "../../../core/prompts/tools/native-tools/read_file"
 
 // Create a concrete implementation for testing
 class TestProvider extends BaseProvider {
@@ -270,6 +271,37 @@ describe("BaseProvider", () => {
 		expect(result.properties.only.type).toBe("null")
 	})
 	describe("convertToolsForOpenAI", () => {
+		it("preserves read_file integer minima while requiring properties for strict mode", () => {
+			const result = provider.testConvertToolsForOpenAI([createReadFileTool()])
+
+			expect(result?.[0]).toMatchObject({
+				function: {
+					strict: true,
+					parameters: {
+						required: ["path", "mode", "offset", "limit", "indentation"],
+						properties: {
+							offset: { type: "integer", minimum: 1 },
+							limit: { type: "integer", minimum: 1 },
+							indentation: {
+								required: [
+									"anchor_line",
+									"max_levels",
+									"include_siblings",
+									"include_header",
+									"max_lines",
+								],
+								properties: {
+									anchor_line: { type: "integer", minimum: 1 },
+									max_levels: { type: "integer", minimum: 0 },
+									max_lines: { type: "integer", minimum: 1 },
+								},
+							},
+						},
+					},
+				},
+			})
+		})
+
 		it("should return undefined for undefined input", () => {
 			const result = provider.testConvertToolsForOpenAI(undefined)
 			expect(result).toBeUndefined()

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

@@ -3,6 +3,38 @@ import { NativeToolCallParser, type ToolCallStreamEvent } from "../NativeToolCal
 describe("NativeToolCallParser", () => {
 	describe("parseToolCall", () => {
 		describe("read_file tool", () => {
+			it("coerces numeric strings before reader-owned validation", () => {
+				expect(
+					NativeToolCallParser.parseToolCall({
+						id: "numeric_strings",
+						name: "read_file",
+						arguments: JSON.stringify({
+							path: "test.ts",
+							offset: "2",
+							limit: "1",
+							indentation: { anchor_line: "3", max_levels: "0", max_lines: "4" },
+						}),
+					}),
+				).toMatchObject({
+					nativeArgs: {
+						path: "test.ts",
+						offset: 2,
+						limit: 1,
+						indentation: { anchor_line: 3, max_levels: 0, max_lines: 4 },
+					},
+				})
+			})
+
+			it.each([0, -1, 1.5])("retains numeric limit %s for precise execution diagnostics", (limit) => {
+				expect(
+					NativeToolCallParser.parseToolCall({
+						id: "reader_validation",
+						name: "read_file",
+						arguments: JSON.stringify({ path: "test.ts", limit }),
+					}),
+				).toMatchObject({ nativeArgs: { path: "test.ts", limit } })
+			})
+
 			it("should parse minimal single-file read_file args", () => {
 				const toolCall = {
 					id: "toolu_123",

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

@@ -3,7 +3,7 @@
 import { describe, it, expect, beforeEach, vi } from "vitest"
 import { presentAssistantMessage } from "../presentAssistantMessage"
 import { validateToolUse } from "../../tools/validateToolUse"
-import { readFileTool } from "../../tools/ReadFileTool"
+import { ReadFileTool } from "../../tools/file-reading/ReadFileTool"
 import type { RequestPolicySnapshot } from "../../prompts/tools/effective-tool-policy"
 
 // Mock dependencies
@@ -399,7 +399,7 @@ describe("presentAssistantMessage - Custom Tool Recording", () => {
 			// longer re-reads provider state before dispatch (issue #1642), so the
 			// dispatch-time throw this test pins is triggered by the dispatched tool
 			// handler itself.
-			const handleSpy = vi.spyOn(readFileTool, "handle").mockRejectedValue(new Error("dispatch failed"))
+			const handleSpy = vi.spyOn(ReadFileTool.prototype, "handle").mockRejectedValue(new Error("dispatch failed"))
 
 			try {
 				await expect(presentAssistantMessage(mockTask, baseSnapshot)).rejects.toThrow("dispatch failed")

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

@@ -65,10 +65,10 @@ const { readFileHandle, attemptCompletionHandle } = vi.hoisted(() => ({
 	attemptCompletionHandle: vi.fn(async () => {}),
 }))
 
-vi.mock("../../tools/ReadFileTool", () => ({
-	readFileTool: {
-		handle: readFileHandle,
-		getReadFileToolDescription: vi.fn(() => "[read_file]"),
+vi.mock("../../tools/file-reading/ReadFileTool", () => ({
+	ReadFileTool: class {
+		handle = readFileHandle
+		getReadFileToolDescription = vi.fn(() => "[read_file]")
 	},
 }))
 

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

@@ -5,6 +5,7 @@ import { TelemetryEventName } from "@roo-code/types"
 import { describe, it, expect, beforeEach, vi, type Mock } from "vitest"
 import { presentAssistantMessage } from "../presentAssistantMessage"
 import { validateToolUse } from "../../tools/validateToolUse"
+import { ReadFileTool } from "../../tools/file-reading/ReadFileTool"
 import { getModeBySlug } from "../../../shared/modes"
 import type { Task } from "../../task/Task"
 import type { RequestPolicySnapshot } from "../../prompts/tools/effective-tool-policy"
@@ -168,6 +169,127 @@ describe("presentAssistantMessage - tool usage attribution", () => {
 		expect(TelemetryService.instance.captureToolUsage).toHaveBeenCalledWith(mockTask.taskId, "read_file")
 	})
 
+	describe("shared read_file tool", () => {
+		it("reuses one instance for partial and complete calls across tasks", async () => {
+			const handle = vi.spyOn(ReadFileTool.prototype, "handle").mockResolvedValue(undefined)
+			const otherTask = { ...mockTask, taskId: "other-task-id", instanceId: "other-instance" }
+
+			try {
+				for (const task of [mockTask, otherTask]) {
+					for (const partial of [true, false]) {
+						task.assistantMessageContent = [
+							{
+								type: "tool_use",
+								id: `call_${task.taskId}`,
+								name: "read_file",
+								params: { path: "test.txt" },
+								nativeArgs: { path: "test.txt" },
+								partial,
+							},
+						]
+
+						// This dispatch double omits Task fields unrelated to tool presentation.
+						await presentAssistantMessage(task as unknown as Task, baseSnapshot)
+					}
+				}
+
+				expect(handle).toHaveBeenCalledTimes(4)
+				const sharedInstance = handle.mock.contexts[0]
+				expect(sharedInstance).toBeInstanceOf(ReadFileTool)
+				for (const instance of handle.mock.contexts) {
+					expect(instance).toBe(sharedInstance)
+				}
+			} finally {
+				handle.mockRestore()
+			}
+		})
+
+		it("preserves the injected reader when recursively processing tool calls", async () => {
+			const reader = {
+				handle: vi.fn<ReadFileTool["handle"]>().mockResolvedValue(undefined),
+				getReadFileToolDescription: vi.fn(() => ""),
+			}
+			mockTask.assistantMessageContent = ["first.txt", "second.txt"].map((path, index) => ({
+				type: "tool_use",
+				id: `call_read_${index}`,
+				name: "read_file",
+				params: { path },
+				nativeArgs: { path },
+				partial: false,
+			}))
+
+			// This dispatch double omits Task fields unrelated to tool presentation.
+			await presentAssistantMessage(mockTask as unknown as Task, baseSnapshot, reader)
+
+			expect(reader.handle).toHaveBeenCalledTimes(2)
+			for (const instance of reader.handle.mock.contexts) {
+				expect(instance).toBe(reader)
+			}
+		})
+
+		it("preserves the injected reader when processing pending streaming updates", async () => {
+			const reader = {
+				handle: vi
+					.fn<ReadFileTool["handle"]>()
+					.mockResolvedValue(undefined)
+					.mockImplementationOnce(async (task) => {
+						task.presentAssistantMessageHasPendingUpdates = true
+					}),
+				getReadFileToolDescription: vi.fn(() => ""),
+			}
+			mockTask.assistantMessageContent = [
+				{
+					type: "tool_use",
+					id: "call_streaming_read",
+					name: "read_file",
+					params: { path: "test.txt" },
+					nativeArgs: { path: "test.txt" },
+					partial: true,
+				},
+			]
+
+			// This dispatch double omits Task fields unrelated to tool presentation.
+			await presentAssistantMessage(mockTask as unknown as Task, baseSnapshot, reader)
+
+			expect(reader.handle).toHaveBeenCalledTimes(2)
+			for (const instance of reader.handle.mock.contexts) {
+				expect(instance).toBe(reader)
+			}
+		})
+
+		it.each([
+			{ nativeArgs: { path: "native.txt" }, expectedPath: "native.txt" },
+			{ nativeArgs: undefined, expectedPath: "legacy.txt" },
+		])("uses the injected reader to describe $expectedPath", async ({ nativeArgs, expectedPath }) => {
+			const reader = new ReadFileTool()
+			const describeTool = vi.spyOn(reader, "getReadFileToolDescription")
+			mockTask.didRejectTool = true
+			mockTask.assistantMessageContent = [
+				{
+					type: "tool_use",
+					id: "call_rejected_read",
+					name: "read_file",
+					params: { path: "legacy.txt" },
+					nativeArgs,
+					partial: false,
+				},
+			]
+
+			try {
+				// This dispatch double omits Task fields unrelated to tool presentation.
+				await presentAssistantMessage(mockTask as unknown as Task, baseSnapshot, reader)
+
+				expect(describeTool).toHaveBeenCalledExactlyOnceWith("read_file", { path: expectedPath })
+				expect(describeTool.mock.contexts[0]).toBe(reader)
+				expect(mockTask.pushToolResultToUserContent).toHaveBeenCalledWith(
+					expect.objectContaining({ content: expect.stringContaining(expectedPath) }),
+				)
+			} finally {
+				describeTool.mockRestore()
+			}
+		})
+	})
+
 	it("records a valid dynamic mcp_ tool name as use_mcp_tool", async () => {
 		mockTask.assistantMessageContent = [
 			{

src/core/assistant-message/presentAssistantMessage.ts

@@ -17,7 +17,7 @@ import { AskIgnoredError } from "../task/AskIgnoredError"
 import { Task } from "../task/Task"
 
 import { listFilesTool } from "../tools/ListFilesTool"
-import { readFileTool } from "../tools/ReadFileTool"
+import { ReadFileTool } from "../tools/file-reading/ReadFileTool"
 import { readCommandOutputTool } from "../tools/ReadCommandOutputTool"
 import { writeToFileTool } from "../tools/WriteToFileTool"
 import { editTool } from "../tools/EditTool"
@@ -44,6 +44,8 @@ import { codebaseSearchTool } from "../tools/CodebaseSearchTool"
 import { formatResponse } from "../prompts/responses"
 import { sanitizeToolUseId } from "../../utils/tool-id"
 
+const defaultReadFileTool = new ReadFileTool()
+
 /**
  * Maps a raw, potentially model-controlled tool name to a safe analytics key.
  * Never returns the raw name unless it is a known static tool, so an
@@ -86,7 +88,11 @@ export function toTelemetryToolName(
  * as it becomes available.
  */
 
-export async function presentAssistantMessage(cline: Task, policySnapshot: RequestPolicySnapshot) {
+export async function presentAssistantMessage(
+	cline: Task,
+	policySnapshot: RequestPolicySnapshot,
+	readFileTool: Pick<ReadFileTool, "handle" | "getReadFileToolDescription"> = defaultReadFileTool,
+) {
 	if (cline.abort) {
 		return
 	}
@@ -113,7 +119,7 @@ export async function presentAssistantMessage(cline: Task, policySnapshot: Reque
 		// The last pending check and the release below run in the same
 		// synchronous step, so no update can be stranded behind the lock.
 		do {
-			await presentAssistantMessageBlock(cline, policySnapshot)
+			await presentAssistantMessageBlock(cline, policySnapshot, readFileTool)
 		} while (!cline.abort && cline.presentAssistantMessageHasPendingUpdates)
 	} finally {
 		// Tool handlers and provider-state reads can reject. Never strand the
@@ -122,7 +128,11 @@ export async function presentAssistantMessage(cline: Task, policySnapshot: Reque
 	}
 }
 
-async function presentAssistantMessageBlock(cline: Task, policySnapshot: RequestPolicySnapshot): Promise<void> {
+async function presentAssistantMessageBlock(
+	cline: Task,
+	policySnapshot: RequestPolicySnapshot,
+	readFileTool: Pick<ReadFileTool, "handle" | "getReadFileToolDescription">,
+): Promise<void> {
 	if (cline.abort) {
 		return
 	}
@@ -1140,7 +1150,7 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request
 		if (cline.currentStreamingContentIndex < cline.assistantMessageContent.length) {
 			// There are already more content blocks to stream, so we'll call
 			// this function ourselves.
-			return await presentAssistantMessageBlock(cline, policySnapshot)
+			return await presentAssistantMessageBlock(cline, policySnapshot, readFileTool)
 		} else {
 			// CRITICAL FIX: If we're out of bounds and the stream is complete, set userMessageContentReady
 			// This handles the case where assistantMessageContent is empty or becomes empty after processing

src/core/auto-approval/WorkspaceRootCanonicalizer.ts

@@ -0,0 +1,44 @@
+import type { WorkspaceRootStrategy } from "./strategies/WorkspaceRootStrategy"
+
+/** 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)
+	}
+
+	canonicalize(value: string): string {
+		if (this.root === undefined || this.canonicalRoot === undefined) return value
+
+		return this.replaceWorkspaceRootPrefix(value, this.root, this.canonicalRoot)
+	}
+
+	/** Keep both verified spellings so globs spanning the root retain their meaning. */
+	equivalentPaths(value: string): string[] {
+		if (this.root === undefined || this.canonicalRoot === undefined) return [value]
+
+		const canonical = this.canonicalize(value)
+		const equivalent =
+			canonical !== value ? canonical : this.replaceWorkspaceRootPrefix(value, this.canonicalRoot, this.root)
+		return equivalent === value ? [value] : [value, equivalent]
+	}
+
+	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
+	}
+}