Просмотр исходного кода

Move presentAssistantMessage into its own module (#3345)

Chris Estreich 7 месяцев назад
Родитель
Сommit
7cea2e8bc4

+ 0 - 1
jest.config.js

@@ -14,7 +14,6 @@ module.exports = {
 					allowJs: true,
 				},
 				diagnostics: false,
-				isolatedModules: true,
 			},
 		],
 	},

+ 1 - 1
src/core/CodeActionProvider.ts → src/activate/CodeActionProvider.ts

@@ -1,6 +1,6 @@
 import * as vscode from "vscode"
 
-import { EditorUtils } from "./EditorUtils"
+import { EditorUtils } from "../integrations/editor/EditorUtils"
 
 export type CodeActionName = "EXPLAIN" | "FIX" | "IMPROVE" | "ADD_TO_CONTEXT" | "NEW_TASK"
 

+ 5 - 13
src/core/__tests__/CodeActionProvider.test.ts → src/activate/__tests__/CodeActionProvider.test.ts

@@ -1,12 +1,11 @@
-// npx jest src/core/__tests__/CodeActionProvider.test.ts
+// npx jest src/activate/__tests__/CodeActionProvider.test.ts
 
 import * as vscode from "vscode"
 
-import { EditorUtils } from "../EditorUtils"
+import { EditorUtils } from "../../integrations/editor/EditorUtils"
 
 import { CodeActionProvider, ACTION_TITLES } from "../CodeActionProvider"
 
-// Mock VSCode API
 jest.mock("vscode", () => ({
 	CodeAction: jest.fn().mockImplementation((title, kind) => ({
 		title,
@@ -29,8 +28,7 @@ jest.mock("vscode", () => ({
 	},
 }))
 
-// Mock EditorUtils
-jest.mock("../EditorUtils", () => ({
+jest.mock("../../integrations/editor/EditorUtils", () => ({
 	EditorUtils: {
 		getEffectiveRange: jest.fn(),
 		getFilePath: jest.fn(),
@@ -48,7 +46,6 @@ describe("CodeActionProvider", () => {
 	beforeEach(() => {
 		provider = new CodeActionProvider()
 
-		// Mock document
 		mockDocument = {
 			getText: jest.fn(),
 			lineAt: jest.fn(),
@@ -56,15 +53,9 @@ describe("CodeActionProvider", () => {
 			uri: { fsPath: "/test/file.ts" },
 		}
 
-		// Mock range
 		mockRange = new vscode.Range(0, 0, 0, 10)
 
-		// Mock context
-		mockContext = {
-			diagnostics: [],
-		}
-
-		// Setup default EditorUtils mocks
+		mockContext = { diagnostics: [] }
 		;(EditorUtils.getEffectiveRange as jest.Mock).mockReturnValue({
 			range: mockRange,
 			text: "test code",
@@ -106,6 +97,7 @@ describe("CodeActionProvider", () => {
 
 		it("should handle errors gracefully", () => {
 			const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {})
+
 			;(EditorUtils.getEffectiveRange as jest.Mock).mockImplementation(() => {
 				throw new Error("Test error")
 			})

+ 3 - 1
src/activate/handleTask.ts

@@ -1,9 +1,11 @@
 import * as vscode from "vscode"
 
-import { COMMAND_IDS } from "../core/CodeActionProvider"
 import { ClineProvider } from "../core/webview/ClineProvider"
+
 import { t } from "../i18n"
 
+import { COMMAND_IDS } from "./CodeActionProvider"
+
 export const handleNewTask = async (params: { prompt?: string } | null | undefined) => {
 	let prompt = params?.prompt
 

+ 1 - 0
src/activate/index.ts

@@ -2,3 +2,4 @@ export { handleUri } from "./handleUri"
 export { registerCommands } from "./registerCommands"
 export { registerCodeActions } from "./registerCodeActions"
 export { registerTerminalActions } from "./registerTerminalActions"
+export { CodeActionProvider } from "./CodeActionProvider"

+ 3 - 2
src/activate/registerCodeActions.ts

@@ -1,9 +1,10 @@
 import * as vscode from "vscode"
 
-import { type CodeActionName, type CodeActionId, COMMAND_IDS } from "../core/CodeActionProvider"
-import { EditorUtils } from "../core/EditorUtils"
+import { EditorUtils } from "../integrations/editor/EditorUtils"
 import { ClineProvider } from "../core/webview/ClineProvider"
 
+import { type CodeActionName, type CodeActionId, COMMAND_IDS } from "./CodeActionProvider"
+
 export const registerCodeActions = (context: vscode.ExtensionContext) => {
 	registerCodeAction(context, COMMAND_IDS.EXPLAIN, "EXPLAIN")
 	registerCodeAction(context, COMMAND_IDS.FIX, "FIX")

+ 36 - 502
src/core/Cline.ts

@@ -4,7 +4,6 @@ import crypto from "crypto"
 import EventEmitter from "events"
 
 import { Anthropic } from "@anthropic-ai/sdk"
-import cloneDeep from "clone-deep"
 import delay from "delay"
 import pWaitFor from "p-wait-for"
 import { serializeError } from "serialize-error"
@@ -32,14 +31,13 @@ import {
 import { getApiMetrics } from "../shared/getApiMetrics"
 import { HistoryItem } from "../shared/HistoryItem"
 import { ClineAskResponse } from "../shared/WebviewMessage"
-import { defaultModeSlug, getModeBySlug } from "../shared/modes"
-import { ToolParamName, ToolResponse, DiffStrategy } from "../shared/tools"
+import { defaultModeSlug } from "../shared/modes"
+import { DiffStrategy } from "../shared/tools"
 
 // services
 import { UrlContentFetcher } from "../services/browser/UrlContentFetcher"
 import { BrowserSession } from "../services/browser/BrowserSession"
 import { McpHub } from "../services/mcp/McpHub"
-import { ToolRepetitionDetector } from "./ToolRepetitionDetector"
 import { McpServerManager } from "../services/mcp/McpServerManager"
 import { telemetryService } from "../services/telemetry/TelemetryService"
 import { RepoPerTaskCheckpointService } from "../services/checkpoints"
@@ -54,36 +52,17 @@ import { TerminalRegistry } from "../integrations/terminal/TerminalRegistry"
 import { calculateApiCostAnthropic } from "../utils/cost"
 import { getWorkspacePath } from "../utils/path"
 
-// tools
-import { fetchInstructionsTool } from "./tools/fetchInstructionsTool"
-import { listFilesTool } from "./tools/listFilesTool"
-import { readFileTool } from "./tools/readFileTool"
-import { writeToFileTool } from "./tools/writeToFileTool"
-import { applyDiffTool } from "./tools/applyDiffTool"
-import { insertContentTool } from "./tools/insertContentTool"
-import { searchAndReplaceTool } from "./tools/searchAndReplaceTool"
-import { listCodeDefinitionNamesTool } from "./tools/listCodeDefinitionNamesTool"
-import { searchFilesTool } from "./tools/searchFilesTool"
-import { browserActionTool } from "./tools/browserActionTool"
-import { executeCommandTool } from "./tools/executeCommandTool"
-import { useMcpToolTool } from "./tools/useMcpToolTool"
-import { accessMcpResourceTool } from "./tools/accessMcpResourceTool"
-import { askFollowupQuestionTool } from "./tools/askFollowupQuestionTool"
-import { switchModeTool } from "./tools/switchModeTool"
-import { attemptCompletionTool } from "./tools/attemptCompletionTool"
-import { newTaskTool } from "./tools/newTaskTool"
-
 // prompts
 import { formatResponse } from "./prompts/responses"
 import { SYSTEM_PROMPT } from "./prompts/system"
 
-// ... everything else
+// core modules
+import { ToolRepetitionDetector } from "./tools/ToolRepetitionDetector"
 import { FileContextTracker } from "./context-tracking/FileContextTracker"
 import { RooIgnoreController } from "./ignore/RooIgnoreController"
-import { type AssistantMessageContent, parseAssistantMessage } from "./assistant-message"
+import { type AssistantMessageContent, parseAssistantMessage, presentAssistantMessage } from "./assistant-message"
 import { truncateConversationIfNeeded } from "./sliding-window"
 import { ClineProvider } from "./webview/ClineProvider"
-import { validateToolUse } from "./mode-validator"
 import { MultiSearchReplaceDiffStrategy } from "./diff/strategies/multi-search-replace"
 import { readApiMessages, saveApiMessages, readTaskMessages, saveTaskMessages, taskMetadata } from "./task-persistence"
 import { getEnvironmentDetails } from "./environment/getEnvironmentDetails"
@@ -139,74 +118,72 @@ export class Cline extends EventEmitter<ClineEvents> {
 	readonly taskNumber: number
 	readonly workspacePath: string
 
+	providerRef: WeakRef<ClineProvider>
+	private readonly globalStoragePath: string
+	abort: boolean = false
+	didFinishAbortingStream = false
+	abandoned = false
+	isInitialized = false
 	isPaused: boolean = false
 	pausedModeSlug: string = defaultModeSlug
 	private pauseInterval: NodeJS.Timeout | undefined
+	customInstructions?: string
 
+	// API
 	readonly apiConfiguration: ApiConfiguration
 	api: ApiHandler
 	private promptCacheKey: string
+	private lastApiRequestTime?: number
 
+	toolRepetitionDetector: ToolRepetitionDetector
 	rooIgnoreController?: RooIgnoreController
 	fileContextTracker: FileContextTracker
 	urlContentFetcher: UrlContentFetcher
+	terminalProcess?: RooTerminalProcess
+
+	// Computer User
 	browserSession: BrowserSession
-	didEditFile: boolean = false
-	customInstructions?: string
 
+	// Editing
+	diffViewProvider: DiffViewProvider
 	diffStrategy?: DiffStrategy
 	diffEnabled: boolean = false
 	fuzzyMatchThreshold: number
+	didEditFile: boolean = false
 
+	// LLM Messages & Chat Messages
 	apiConversationHistory: (Anthropic.MessageParam & { ts?: number })[] = []
 	clineMessages: ClineMessage[] = []
 
+	// Ask
 	private askResponse?: ClineAskResponse
 	private askResponseText?: string
 	private askResponseImages?: string[]
 	public lastMessageTs?: number
 
-	// Not private since it needs to be accessible by tools.
+	// Tool Use
 	consecutiveMistakeCount: number = 0
 	consecutiveMistakeLimit: number
 	consecutiveMistakeCountForApplyDiff: Map<string, number> = new Map()
+	private toolUsage: ToolUsage = {}
 
-	// For tracking identical consecutive tool calls
-	private toolRepetitionDetector: ToolRepetitionDetector
-
-	// Not private since it needs to be accessible by tools.
-	providerRef: WeakRef<ClineProvider>
-	private readonly globalStoragePath: string
-	private abort: boolean = false
-	didFinishAbortingStream = false
-	abandoned = false
-	diffViewProvider: DiffViewProvider
-	private lastApiRequestTime?: number
-	isInitialized = false
-
-	// checkpoints
+	// Checkpoints
 	enableCheckpoints: boolean
 	checkpointService?: RepoPerTaskCheckpointService
 	checkpointServiceInitializing = false
 
-	// streaming
+	// Streaming
 	isWaitingForFirstChunk = false
 	isStreaming = false
-	private currentStreamingContentIndex = 0
-	private assistantMessageContent: AssistantMessageContent[] = []
-	private presentAssistantMessageLocked = false
-	private presentAssistantMessageHasPendingUpdates = false
+	currentStreamingContentIndex = 0
+	assistantMessageContent: AssistantMessageContent[] = []
+	presentAssistantMessageLocked = false
+	presentAssistantMessageHasPendingUpdates = false
 	userMessageContent: (Anthropic.TextBlockParam | Anthropic.ImageBlockParam)[] = []
-	private userMessageContentReady = false
+	userMessageContentReady = false
 	didRejectTool = false
-	private didAlreadyUseTool = false
-	private didCompleteReadingStream = false
-
-	// metrics
-	private toolUsage: ToolUsage = {}
-
-	// terminal
-	public terminalProcess?: RooTerminalProcess
+	didAlreadyUseTool = false
+	didCompleteReadingStream = false
 
 	constructor({
 		provider,
@@ -1189,7 +1166,7 @@ export class Cline extends EventEmitter<ClineEvents> {
 							}
 
 							// Present content to user.
-							this.presentAssistantMessage()
+							presentAssistantMessage(this)
 							break
 					}
 
@@ -1280,7 +1257,7 @@ export class Cline extends EventEmitter<ClineEvents> {
 				// `pWaitFor` before making the next request. All this is really
 				// doing is presenting the last partial message that we just set
 				// to complete.
-				this.presentAssistantMessage()
+				presentAssistantMessage(this)
 			}
 
 			updateApiReqMsg()
@@ -1612,449 +1589,6 @@ export class Cline extends EventEmitter<ClineEvents> {
 		yield* iterator
 	}
 
-	public async presentAssistantMessage() {
-		if (this.abort) {
-			throw new Error(`[Cline#presentAssistantMessage] task ${this.taskId}.${this.instanceId} aborted`)
-		}
-
-		if (this.presentAssistantMessageLocked) {
-			this.presentAssistantMessageHasPendingUpdates = true
-			return
-		}
-		this.presentAssistantMessageLocked = true
-		this.presentAssistantMessageHasPendingUpdates = false
-
-		if (this.currentStreamingContentIndex >= this.assistantMessageContent.length) {
-			// this may happen if the last content block was completed before streaming could finish. if streaming is finished, and we're out of bounds then this means we already presented/executed the last content block and are ready to continue to next request
-			if (this.didCompleteReadingStream) {
-				this.userMessageContentReady = true
-			}
-			// console.log("no more content blocks to stream! this shouldn't happen?")
-			this.presentAssistantMessageLocked = false
-			return
-			//throw new Error("No more content blocks to stream! This shouldn't happen...") // remove and just return after testing
-		}
-
-		const block = cloneDeep(this.assistantMessageContent[this.currentStreamingContentIndex]) // need to create copy bc while stream is updating the array, it could be updating the reference block properties too
-
-		switch (block.type) {
-			case "text": {
-				if (this.didRejectTool || this.didAlreadyUseTool) {
-					break
-				}
-				let content = block.content
-				if (content) {
-					// (have to do this for partial and complete since sending content in thinking tags to markdown renderer will automatically be removed)
-					// Remove end substrings of <thinking or </thinking (below xml parsing is only for opening tags)
-					// (this is done with the xml parsing below now, but keeping here for reference)
-					// content = content.replace(/<\/?t(?:h(?:i(?:n(?:k(?:i(?:n(?:g)?)?)?$/, "")
-					// Remove all instances of <thinking> (with optional line break after) and </thinking> (with optional line break before)
-					// - Needs to be separate since we dont want to remove the line break before the first tag
-					// - Needs to happen before the xml parsing below
-					content = content.replace(/<thinking>\s?/g, "")
-					content = content.replace(/\s?<\/thinking>/g, "")
-
-					// Remove partial XML tag at the very end of the content (for tool use and thinking tags)
-					// (prevents scrollview from jumping when tags are automatically removed)
-					const lastOpenBracketIndex = content.lastIndexOf("<")
-					if (lastOpenBracketIndex !== -1) {
-						const possibleTag = content.slice(lastOpenBracketIndex)
-						// Check if there's a '>' after the last '<' (i.e., if the tag is complete) (complete thinking and tool tags will have been removed by now)
-						const hasCloseBracket = possibleTag.includes(">")
-						if (!hasCloseBracket) {
-							// Extract the potential tag name
-							let tagContent: string
-							if (possibleTag.startsWith("</")) {
-								tagContent = possibleTag.slice(2).trim()
-							} else {
-								tagContent = possibleTag.slice(1).trim()
-							}
-							// Check if tagContent is likely an incomplete tag name (letters and underscores only)
-							const isLikelyTagName = /^[a-zA-Z_]+$/.test(tagContent)
-							// Preemptively remove < or </ to keep from these artifacts showing up in chat (also handles closing thinking tags)
-							const isOpeningOrClosing = possibleTag === "<" || possibleTag === "</"
-							// If the tag is incomplete and at the end, remove it from the content
-							if (isOpeningOrClosing || isLikelyTagName) {
-								content = content.slice(0, lastOpenBracketIndex).trim()
-							}
-						}
-					}
-				}
-				await this.say("text", content, undefined, block.partial)
-				break
-			}
-			case "tool_use":
-				const toolDescription = (): string => {
-					switch (block.name) {
-						case "execute_command":
-							return `[${block.name} for '${block.params.command}']`
-						case "read_file":
-							return `[${block.name} for '${block.params.path}']`
-						case "fetch_instructions":
-							return `[${block.name} for '${block.params.task}']`
-						case "write_to_file":
-							return `[${block.name} for '${block.params.path}']`
-						case "apply_diff":
-							return `[${block.name} for '${block.params.path}']`
-						case "search_files":
-							return `[${block.name} for '${block.params.regex}'${
-								block.params.file_pattern ? ` in '${block.params.file_pattern}'` : ""
-							}]`
-						case "insert_content":
-							return `[${block.name} for '${block.params.path}']`
-						case "search_and_replace":
-							return `[${block.name} for '${block.params.path}']`
-						case "list_files":
-							return `[${block.name} for '${block.params.path}']`
-						case "list_code_definition_names":
-							return `[${block.name} for '${block.params.path}']`
-						case "browser_action":
-							return `[${block.name} for '${block.params.action}']`
-						case "use_mcp_tool":
-							return `[${block.name} for '${block.params.server_name}']`
-						case "access_mcp_resource":
-							return `[${block.name} for '${block.params.server_name}']`
-						case "ask_followup_question":
-							return `[${block.name} for '${block.params.question}']`
-						case "attempt_completion":
-							return `[${block.name}]`
-						case "switch_mode":
-							return `[${block.name} to '${block.params.mode_slug}'${block.params.reason ? ` because: ${block.params.reason}` : ""}]`
-						case "new_task": {
-							const mode = block.params.mode ?? defaultModeSlug
-							const message = block.params.message ?? "(no message)"
-							const modeName = getModeBySlug(mode, customModes)?.name ?? mode
-							return `[${block.name} in ${modeName} mode: '${message}']`
-						}
-					}
-				}
-
-				if (this.didRejectTool) {
-					// ignore any tool content after user has rejected tool once
-					if (!block.partial) {
-						this.userMessageContent.push({
-							type: "text",
-							text: `Skipping tool ${toolDescription()} due to user rejecting a previous tool.`,
-						})
-					} else {
-						// partial tool after user rejected a previous tool
-						this.userMessageContent.push({
-							type: "text",
-							text: `Tool ${toolDescription()} was interrupted and not executed due to user rejecting a previous tool.`,
-						})
-					}
-					break
-				}
-
-				if (this.didAlreadyUseTool) {
-					// ignore any content after a tool has already been used
-					this.userMessageContent.push({
-						type: "text",
-						text: `Tool [${block.name}] was not executed because a tool has already been used in this message. Only one tool may be used per message. You must assess the first tool's result before proceeding to use the next tool.`,
-					})
-					break
-				}
-
-				const pushToolResult = (content: ToolResponse) => {
-					this.userMessageContent.push({
-						type: "text",
-						text: `${toolDescription()} Result:`,
-					})
-					if (typeof content === "string") {
-						this.userMessageContent.push({
-							type: "text",
-							text: content || "(tool did not return anything)",
-						})
-					} else {
-						this.userMessageContent.push(...content)
-					}
-					// once a tool result has been collected, ignore all other tool uses since we should only ever present one tool result per message
-					this.didAlreadyUseTool = true
-
-					// Flag a checkpoint as possible since we've used a tool
-					// which may have changed the file system.
-				}
-
-				const askApproval = async (
-					type: ClineAsk,
-					partialMessage?: string,
-					progressStatus?: ToolProgressStatus,
-				) => {
-					const { response, text, images } = await this.ask(type, partialMessage, false, progressStatus)
-					if (response !== "yesButtonClicked") {
-						// Handle both messageResponse and noButtonClicked with text
-						if (text) {
-							await this.say("user_feedback", text, images)
-							pushToolResult(
-								formatResponse.toolResult(formatResponse.toolDeniedWithFeedback(text), images),
-							)
-						} else {
-							pushToolResult(formatResponse.toolDenied())
-						}
-						this.didRejectTool = true
-						return false
-					}
-					// Handle yesButtonClicked with text
-					if (text) {
-						await this.say("user_feedback", text, images)
-						pushToolResult(formatResponse.toolResult(formatResponse.toolApprovedWithFeedback(text), images))
-					}
-					return true
-				}
-
-				const askFinishSubTaskApproval = async () => {
-					// ask the user to approve this task has completed, and he has reviewd it, and we can declare task is finished
-					// and return control to the parent task to continue running the rest of the sub-tasks
-					const toolMessage = JSON.stringify({
-						tool: "finishTask",
-					})
-
-					return await askApproval("tool", toolMessage)
-				}
-
-				const handleError = async (action: string, error: Error) => {
-					const errorString = `Error ${action}: ${JSON.stringify(serializeError(error))}`
-					await this.say(
-						"error",
-						`Error ${action}:\n${error.message ?? JSON.stringify(serializeError(error), null, 2)}`,
-					)
-					// this.toolResults.push({
-					// 	type: "tool_result",
-					// 	tool_use_id: toolUseId,
-					// 	content: await this.formatToolError(errorString),
-					// })
-					pushToolResult(formatResponse.toolError(errorString))
-				}
-
-				// If block is partial, remove partial closing tag so its not presented to user
-				const removeClosingTag = (tag: ToolParamName, text?: string): string => {
-					if (!block.partial) {
-						return text || ""
-					}
-					if (!text) {
-						return ""
-					}
-					// This regex dynamically constructs a pattern to match the closing tag:
-					// - Optionally matches whitespace before the tag
-					// - Matches '<' or '</' optionally followed by any subset of characters from the tag name
-					const tagRegex = new RegExp(
-						`\\s?<\/?${tag
-							.split("")
-							.map((char) => `(?:${char})?`)
-							.join("")}$`,
-						"g",
-					)
-					return text.replace(tagRegex, "")
-				}
-
-				if (block.name !== "browser_action") {
-					await this.browserSession.closeBrowser()
-				}
-
-				if (!block.partial) {
-					this.recordToolUsage(block.name)
-					telemetryService.captureToolUsage(this.taskId, block.name)
-				}
-
-				// Validate tool use before execution
-				const { mode, customModes } = (await this.providerRef.deref()?.getState()) ?? {}
-				try {
-					validateToolUse(
-						block.name as ToolName,
-						mode ?? defaultModeSlug,
-						customModes ?? [],
-						{
-							apply_diff: this.diffEnabled,
-						},
-						block.params,
-					)
-				} catch (error) {
-					this.consecutiveMistakeCount++
-					pushToolResult(formatResponse.toolError(error.message))
-					break
-				}
-
-				// Check for identical consecutive tool calls
-				if (!block.partial) {
-					// Use the detector to check for repetition, passing the ToolUse block directly
-					const repetitionCheck = this.toolRepetitionDetector.check(block)
-
-					// If execution is not allowed, notify user and break
-					if (!repetitionCheck.allowExecution && repetitionCheck.askUser) {
-						// Handle repetition similar to mistake_limit_reached pattern
-						const { response, text, images } = await this.ask(
-							repetitionCheck.askUser.messageKey as ClineAsk,
-							repetitionCheck.askUser.messageDetail.replace("{toolName}", block.name),
-						)
-
-						if (response === "messageResponse") {
-							// Add user feedback to userContent
-							this.userMessageContent.push(
-								{
-									type: "text" as const,
-									text: `Tool repetition limit reached. User feedback: ${text}`,
-								},
-								...formatResponse.imageBlocks(images),
-							)
-
-							// Add user feedback to chat
-							await this.say("user_feedback", text, images)
-
-							// Track tool repetition in telemetry
-							telemetryService.captureConsecutiveMistakeError(this.taskId) // Using existing telemetry method
-						}
-
-						// Return tool result message about the repetition
-						pushToolResult(
-							formatResponse.toolError(
-								`Tool call repetition limit reached for ${block.name}. Please try a different approach.`,
-							),
-						)
-						break
-					}
-				}
-
-				switch (block.name) {
-					case "write_to_file":
-						await writeToFileTool(this, block, askApproval, handleError, pushToolResult, removeClosingTag)
-						break
-					case "apply_diff":
-						await applyDiffTool(this, block, askApproval, handleError, pushToolResult, removeClosingTag)
-						break
-					case "insert_content":
-						await insertContentTool(this, block, askApproval, handleError, pushToolResult, removeClosingTag)
-						break
-					case "search_and_replace":
-						await searchAndReplaceTool(
-							this,
-							block,
-							askApproval,
-							handleError,
-							pushToolResult,
-							removeClosingTag,
-						)
-						break
-					case "read_file":
-						await readFileTool(this, block, askApproval, handleError, pushToolResult, removeClosingTag)
-
-						break
-					case "fetch_instructions":
-						await fetchInstructionsTool(this, block, askApproval, handleError, pushToolResult)
-						break
-					case "list_files":
-						await listFilesTool(this, block, askApproval, handleError, pushToolResult, removeClosingTag)
-						break
-					case "list_code_definition_names":
-						await listCodeDefinitionNamesTool(
-							this,
-							block,
-							askApproval,
-							handleError,
-							pushToolResult,
-							removeClosingTag,
-						)
-						break
-					case "search_files":
-						await searchFilesTool(this, block, askApproval, handleError, pushToolResult, removeClosingTag)
-						break
-					case "browser_action":
-						await browserActionTool(this, block, askApproval, handleError, pushToolResult, removeClosingTag)
-						break
-					case "execute_command":
-						await executeCommandTool(
-							this,
-							block,
-							askApproval,
-							handleError,
-							pushToolResult,
-							removeClosingTag,
-						)
-						break
-					case "use_mcp_tool":
-						await useMcpToolTool(this, block, askApproval, handleError, pushToolResult, removeClosingTag)
-						break
-					case "access_mcp_resource":
-						await accessMcpResourceTool(
-							this,
-							block,
-							askApproval,
-							handleError,
-							pushToolResult,
-							removeClosingTag,
-						)
-						break
-					case "ask_followup_question":
-						await askFollowupQuestionTool(
-							this,
-							block,
-							askApproval,
-							handleError,
-							pushToolResult,
-							removeClosingTag,
-						)
-						break
-					case "switch_mode":
-						await switchModeTool(this, block, askApproval, handleError, pushToolResult, removeClosingTag)
-						break
-					case "new_task":
-						await newTaskTool(this, block, askApproval, handleError, pushToolResult, removeClosingTag)
-						break
-					case "attempt_completion":
-						await attemptCompletionTool(
-							this,
-							block,
-							askApproval,
-							handleError,
-							pushToolResult,
-							removeClosingTag,
-							toolDescription,
-							askFinishSubTaskApproval,
-						)
-						break
-				}
-
-				break
-		}
-
-		const recentlyModifiedFiles = this.fileContextTracker.getAndClearCheckpointPossibleFile()
-
-		if (recentlyModifiedFiles.length > 0) {
-			// TODO: We can track what file changes were made and only
-			// checkpoint those files, this will be save storage.
-			await checkpointSave(this)
-		}
-
-		/*
-		Seeing out of bounds is fine, it means that the next too call is being built up and ready to add to assistantMessageContent to present.
-		When you see the UI inactive during this, it means that a tool is breaking without presenting any UI. For example the write_to_file tool was breaking when relpath was undefined, and for invalid relpath it never presented UI.
-		*/
-		this.presentAssistantMessageLocked = false // this needs to be placed here, if not then calling this.presentAssistantMessage below would fail (sometimes) since it's locked
-		// NOTE: when tool is rejected, iterator stream is interrupted and it waits for userMessageContentReady to be true. Future calls to present will skip execution since didRejectTool and iterate until contentIndex is set to message length and it sets userMessageContentReady to true itself (instead of preemptively doing it in iterator)
-		if (!block.partial || this.didRejectTool || this.didAlreadyUseTool) {
-			// block is finished streaming and executing
-			if (this.currentStreamingContentIndex === this.assistantMessageContent.length - 1) {
-				// its okay that we increment if !didCompleteReadingStream, it'll just return bc out of bounds and as streaming continues it will call presentAssitantMessage if a new block is ready. if streaming is finished then we set userMessageContentReady to true when out of bounds. This gracefully allows the stream to continue on and all potential content blocks be presented.
-				// last block is complete and it is finished executing
-				this.userMessageContentReady = true // will allow pwaitfor to continue
-			}
-
-			// call next block if it exists (if not then read stream will call it when its ready)
-			this.currentStreamingContentIndex++ // need to increment regardless, so when read stream calls this function again it will be streaming the next block
-
-			if (this.currentStreamingContentIndex < this.assistantMessageContent.length) {
-				// there are already more content blocks to stream, so we'll call this function ourselves
-				// await this.presentAssistantContent()
-
-				this.presentAssistantMessage()
-				return
-			}
-		}
-		// block is partial, but the read stream may have finished
-		if (this.presentAssistantMessageHasPendingUpdates) {
-			this.presentAssistantMessage()
-		}
-	}
-
 	// Checkpoints
 
 	public async checkpointSave() {

+ 0 - 408
src/core/__tests__/read-file-maxReadFileLine.test.ts

@@ -1,408 +0,0 @@
-// npx jest src/core/__tests__/read-file-maxReadFileLine.test.ts
-
-import * as path from "path"
-
-import { countFileLines } from "../../integrations/misc/line-counter"
-import { readLines } from "../../integrations/misc/read-lines"
-import { extractTextFromFile } from "../../integrations/misc/extract-text"
-import { parseSourceCodeDefinitionsForFile } from "../../services/tree-sitter"
-import { isBinaryFile } from "isbinaryfile"
-import { ReadFileToolUse } from "../../shared/tools"
-
-// Mock dependencies
-jest.mock("../../integrations/misc/line-counter")
-jest.mock("../../integrations/misc/read-lines")
-jest.mock("../../integrations/misc/extract-text", () => {
-	const actual = jest.requireActual("../../integrations/misc/extract-text")
-	// Create a spy on the actual addLineNumbers function
-	const addLineNumbersSpy = jest.spyOn(actual, "addLineNumbers")
-
-	return {
-		...actual,
-		// Expose the spy so tests can access it
-		__addLineNumbersSpy: addLineNumbersSpy,
-		extractTextFromFile: jest.fn(),
-	}
-})
-
-// Get a reference to the spy
-const addLineNumbersSpy = jest.requireMock("../../integrations/misc/extract-text").__addLineNumbersSpy
-
-jest.mock("../../services/tree-sitter")
-jest.mock("isbinaryfile")
-jest.mock("../ignore/RooIgnoreController", () => ({
-	RooIgnoreController: class {
-		initialize() {
-			return Promise.resolve()
-		}
-		validateAccess() {
-			return true
-		}
-	},
-}))
-jest.mock("fs/promises", () => ({
-	mkdir: jest.fn().mockResolvedValue(undefined),
-	writeFile: jest.fn().mockResolvedValue(undefined),
-	readFile: jest.fn().mockResolvedValue("{}"),
-}))
-jest.mock("../../utils/fs", () => ({
-	fileExistsAtPath: jest.fn().mockReturnValue(true),
-}))
-
-// Mock path
-jest.mock("path", () => {
-	const originalPath = jest.requireActual("path")
-	return {
-		...originalPath,
-		resolve: jest.fn().mockImplementation((...args) => args.join("/")),
-	}
-})
-
-describe("read_file tool with maxReadFileLine setting", () => {
-	// Test data
-	const testFilePath = "test/file.txt"
-	const absoluteFilePath = "/test/file.txt"
-	const fileContent = "Line 1\nLine 2\nLine 3\nLine 4\nLine 5"
-	const numberedFileContent = "1 | Line 1\n2 | Line 2\n3 | Line 3\n4 | Line 4\n5 | Line 5\n"
-	const sourceCodeDef = "\n\n# file.txt\n1--5 | Content"
-	const expectedFullFileXml = `<file><path>${testFilePath}</path>\n<content lines="1-5">\n${numberedFileContent}</content>\n</file>`
-
-	// Mocked functions with correct types
-	const mockedCountFileLines = countFileLines as jest.MockedFunction<typeof countFileLines>
-	const mockedReadLines = readLines as jest.MockedFunction<typeof readLines>
-	const mockedExtractTextFromFile = extractTextFromFile as jest.MockedFunction<typeof extractTextFromFile>
-	const mockedParseSourceCodeDefinitionsForFile = parseSourceCodeDefinitionsForFile as jest.MockedFunction<
-		typeof parseSourceCodeDefinitionsForFile
-	>
-
-	// Variable to control what content is used by the mock - set in beforeEach
-	let mockInputContent = ""
-
-	const mockedIsBinaryFile = isBinaryFile as jest.MockedFunction<typeof isBinaryFile>
-	const mockedPathResolve = path.resolve as jest.MockedFunction<typeof path.resolve>
-
-	// Mock instances
-	const mockCline: any = {}
-	let mockProvider: any
-	let toolResult: string | undefined
-
-	beforeEach(() => {
-		jest.clearAllMocks()
-
-		// Setup path resolution
-		mockedPathResolve.mockReturnValue(absoluteFilePath)
-
-		// Setup mocks for file operations
-		mockedIsBinaryFile.mockResolvedValue(false)
-
-		// Set the default content for the mock
-		mockInputContent = fileContent
-
-		// Setup the extractTextFromFile mock implementation with the current mockInputContent
-		mockedExtractTextFromFile.mockImplementation((_filePath) => {
-			const actual = jest.requireActual("../../integrations/misc/extract-text")
-			return Promise.resolve(actual.addLineNumbers(mockInputContent))
-		})
-
-		// No need to setup the extractTextFromFile mock implementation here
-		// as it's already defined at the module level
-
-		// Setup mock provider
-		mockProvider = {
-			getState: jest.fn(),
-			deref: jest.fn().mockReturnThis(),
-		}
-
-		// Setup Cline instance with mock methods
-		mockCline.cwd = "/"
-		mockCline.task = "Test"
-		mockCline.providerRef = mockProvider
-		mockCline.rooIgnoreController = {
-			validateAccess: jest.fn().mockReturnValue(true),
-		}
-		mockCline.say = jest.fn().mockResolvedValue(undefined)
-		mockCline.ask = jest.fn().mockResolvedValue(true)
-		mockCline.presentAssistantMessage = jest.fn()
-		mockCline.getFileContextTracker = jest.fn().mockReturnValue({
-			trackFileContext: jest.fn().mockResolvedValue(undefined),
-		})
-		mockCline.recordToolUsage = jest.fn().mockReturnValue(undefined)
-		mockCline.recordToolError = jest.fn().mockReturnValue(undefined)
-		// Reset tool result
-		toolResult = undefined
-	})
-
-	/**
-	 * Helper function to execute the read file tool with different maxReadFileLine settings
-	 */
-	async function executeReadFileTool(
-		params: Partial<ReadFileToolUse["params"]> = {},
-		options: {
-			maxReadFileLine?: number
-			totalLines?: number
-			skipAddLineNumbersCheck?: boolean // Flag to skip addLineNumbers check
-		} = {},
-	): Promise<string | undefined> {
-		// Configure mocks based on test scenario
-		const maxReadFileLine = options.maxReadFileLine ?? 500
-		const totalLines = options.totalLines ?? 5
-
-		mockProvider.getState.mockResolvedValue({ maxReadFileLine })
-		mockedCountFileLines.mockResolvedValue(totalLines)
-
-		// Reset the spy before each test
-		addLineNumbersSpy.mockClear()
-
-		// Create a tool use object
-		const toolUse: ReadFileToolUse = {
-			type: "tool_use",
-			name: "read_file",
-			params: {
-				path: testFilePath,
-				...params,
-			},
-			partial: false,
-		}
-
-		// Import the tool implementation dynamically to avoid hoisting issues
-		const { readFileTool } = require("../tools/readFileTool")
-
-		// Execute the tool
-		await readFileTool(
-			mockCline,
-			toolUse,
-			mockCline.ask,
-			jest.fn(),
-			(result: string) => {
-				toolResult = result
-			},
-			(param: string, value: string) => value,
-		)
-
-		// Verify addLineNumbers was called appropriately
-		if (!options.skipAddLineNumbersCheck) {
-			expect(addLineNumbersSpy).toHaveBeenCalled()
-		} else {
-			expect(addLineNumbersSpy).not.toHaveBeenCalled()
-		}
-
-		return toolResult
-	}
-	describe("when maxReadFileLine is negative", () => {
-		it("should read the entire file using extractTextFromFile", async () => {
-			// Setup - use default mockInputContent
-			mockInputContent = fileContent
-
-			// Execute
-			const result = await executeReadFileTool({}, { maxReadFileLine: -1 })
-
-			// Verify
-			expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath)
-			expect(mockedReadLines).not.toHaveBeenCalled()
-			expect(mockedParseSourceCodeDefinitionsForFile).not.toHaveBeenCalled()
-			expect(result).toBe(expectedFullFileXml)
-		})
-
-		it("should ignore range parameters and read entire file when maxReadFileLine is -1", async () => {
-			// Setup - use default mockInputContent
-			mockInputContent = fileContent
-
-			// Execute with range parameters
-			const result = await executeReadFileTool(
-				{
-					start_line: "2",
-					end_line: "4",
-				},
-				{ maxReadFileLine: -1 },
-			)
-
-			// Verify that extractTextFromFile is still used (not readLines)
-			expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath)
-			expect(mockedReadLines).not.toHaveBeenCalled()
-			expect(mockedParseSourceCodeDefinitionsForFile).not.toHaveBeenCalled()
-			expect(result).toBe(expectedFullFileXml)
-		})
-
-		it("should not show line snippet in approval message when maxReadFileLine is -1", async () => {
-			// This test verifies the line snippet behavior for the approval message
-			// Setup - use default mockInputContent
-			mockInputContent = fileContent
-
-			// Execute - we'll reuse executeReadFileTool to run the tool
-			await executeReadFileTool({}, { maxReadFileLine: -1 })
-
-			// Verify the empty line snippet for full read was passed to the approval message
-			// Look at the parameters passed to the 'ask' method in the approval message
-			const askCall = mockCline.ask.mock.calls[0]
-			const completeMessage = JSON.parse(askCall[1])
-
-			// Verify the reason (lineSnippet) is empty or undefined for full read
-			expect(completeMessage.reason).toBeFalsy()
-		})
-	})
-
-	describe("when maxReadFileLine is 0", () => {
-		it("should return an empty content with source code definitions", async () => {
-			// Setup - for maxReadFileLine = 0, the implementation won't call readLines
-			mockedParseSourceCodeDefinitionsForFile.mockResolvedValue(sourceCodeDef)
-
-			// Execute - skip addLineNumbers check as it's not called for maxReadFileLine=0
-			const result = await executeReadFileTool(
-				{},
-				{
-					maxReadFileLine: 0,
-					totalLines: 5,
-					skipAddLineNumbersCheck: true,
-				},
-			)
-
-			// Verify
-			expect(mockedExtractTextFromFile).not.toHaveBeenCalled()
-			expect(mockedReadLines).not.toHaveBeenCalled() // Per implementation line 141
-			expect(mockedParseSourceCodeDefinitionsForFile).toHaveBeenCalledWith(
-				absoluteFilePath,
-				mockCline.rooIgnoreController,
-			)
-
-			// Verify XML structure
-			expect(result).toContain(`<file><path>${testFilePath}</path>`)
-			expect(result).toContain("<notice>Showing only 0 of 5 total lines")
-			expect(result).toContain("</notice>")
-			expect(result).toContain("<list_code_definition_names>")
-			expect(result).toContain(sourceCodeDef.trim())
-			expect(result).toContain("</list_code_definition_names>")
-			expect(result).not.toContain("<content") // No content when maxReadFileLine is 0
-		})
-	})
-
-	describe("when maxReadFileLine is less than file length", () => {
-		it("should read only maxReadFileLine lines and add source code definitions", async () => {
-			// Setup
-			const content = "Line 1\nLine 2\nLine 3"
-			mockedReadLines.mockResolvedValue(content)
-			mockedParseSourceCodeDefinitionsForFile.mockResolvedValue(sourceCodeDef)
-
-			// Execute
-			const result = await executeReadFileTool({}, { maxReadFileLine: 3 })
-
-			// Verify - check behavior but not specific implementation details
-			expect(mockedExtractTextFromFile).not.toHaveBeenCalled()
-			expect(mockedReadLines).toHaveBeenCalled()
-			expect(mockedParseSourceCodeDefinitionsForFile).toHaveBeenCalledWith(
-				absoluteFilePath,
-				mockCline.rooIgnoreController,
-			)
-
-			// Verify XML structure
-			expect(result).toContain(`<file><path>${testFilePath}</path>`)
-			expect(result).toContain('<content lines="1-3">')
-			expect(result).toContain("1 | Line 1")
-			expect(result).toContain("2 | Line 2")
-			expect(result).toContain("3 | Line 3")
-			expect(result).toContain("</content>")
-			expect(result).toContain("<notice>Showing only 3 of 5 total lines")
-			expect(result).toContain("</notice>")
-			expect(result).toContain("<list_code_definition_names>")
-			expect(result).toContain(sourceCodeDef.trim())
-			expect(result).toContain("</list_code_definition_names>")
-			expect(result).toContain("<list_code_definition_names>")
-			expect(result).toContain(sourceCodeDef.trim())
-		})
-	})
-
-	describe("when maxReadFileLine equals or exceeds file length", () => {
-		it("should use extractTextFromFile when maxReadFileLine > totalLines", async () => {
-			// Setup
-			mockedCountFileLines.mockResolvedValue(5) // File shorter than maxReadFileLine
-			mockInputContent = fileContent
-
-			// Execute
-			const result = await executeReadFileTool({}, { maxReadFileLine: 10, totalLines: 5 })
-
-			// Verify
-			expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath)
-			expect(result).toBe(expectedFullFileXml)
-		})
-
-		it("should read with extractTextFromFile when file has few lines", async () => {
-			// Setup
-			mockedCountFileLines.mockResolvedValue(3) // File shorter than maxReadFileLine
-			mockInputContent = fileContent
-
-			// Execute
-			const result = await executeReadFileTool({}, { maxReadFileLine: 5, totalLines: 3 })
-
-			// Verify
-			expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath)
-			expect(mockedReadLines).not.toHaveBeenCalled()
-			// Create a custom expected XML with lines="1-3" since totalLines is 3
-			const expectedXml = `<file><path>${testFilePath}</path>\n<content lines="1-3">\n${numberedFileContent}</content>\n</file>`
-			expect(result).toBe(expectedXml)
-		})
-	})
-
-	describe("when file is binary", () => {
-		it("should always use extractTextFromFile regardless of maxReadFileLine", async () => {
-			// Setup
-			mockedIsBinaryFile.mockResolvedValue(true)
-			// For binary files, we're using a maxReadFileLine of 3 and totalLines is assumed to be 3
-			mockedCountFileLines.mockResolvedValue(3)
-
-			// For binary files, we need a special mock implementation that doesn't use addLineNumbers
-			// Save the original mock implementation
-			const originalMockImplementation = mockedExtractTextFromFile.getMockImplementation()
-			// Create a special mock implementation that doesn't call addLineNumbers
-			mockedExtractTextFromFile.mockImplementation(() => {
-				return Promise.resolve(numberedFileContent)
-			})
-
-			// Reset the spy to clear any previous calls
-			addLineNumbersSpy.mockClear()
-
-			// Execute - skip addLineNumbers check as we're directly providing the numbered content
-			const result = await executeReadFileTool(
-				{},
-				{
-					maxReadFileLine: 3,
-					totalLines: 3,
-					skipAddLineNumbersCheck: true,
-				},
-			)
-
-			// Restore the original mock implementation after the test
-			mockedExtractTextFromFile.mockImplementation(originalMockImplementation)
-
-			// Verify
-			expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath)
-			expect(mockedReadLines).not.toHaveBeenCalled()
-			// Create a custom expected XML with lines="1-3" for binary files
-			const expectedXml = `<file><path>${testFilePath}</path>\n<content lines="1-3">\n${numberedFileContent}</content>\n</file>`
-			expect(result).toBe(expectedXml)
-		})
-	})
-
-	describe("with range parameters", () => {
-		it("should honor start_line and end_line when provided", async () => {
-			// Setup
-			mockedReadLines.mockResolvedValue("Line 2\nLine 3\nLine 4")
-
-			// Execute using executeReadFileTool with range parameters
-			const rangeResult = await executeReadFileTool({
-				start_line: "2",
-				end_line: "4",
-			})
-
-			// Verify
-			expect(mockedReadLines).toHaveBeenCalledWith(absoluteFilePath, 3, 1) // end_line - 1, start_line - 1
-			expect(addLineNumbersSpy).toHaveBeenCalledWith(expect.any(String), 2) // start with proper line numbers
-
-			// Verify XML structure with lines attribute
-			expect(rangeResult).toContain(`<file><path>${testFilePath}</path>`)
-			expect(rangeResult).toContain(`<content lines="2-4">`)
-			expect(rangeResult).toContain("2 | Line 2")
-			expect(rangeResult).toContain("3 | Line 3")
-			expect(rangeResult).toContain("4 | Line 4")
-			expect(rangeResult).toContain("</content>")
-		})
-	})
-})

+ 2 - 1
src/core/assistant-message/index.ts

@@ -1 +1,2 @@
-export { type AssistantMessageContent, parseAssistantMessage } from "./parse-assistant-message"
+export { type AssistantMessageContent, parseAssistantMessage } from "./parseAssistantMessage"
+export { presentAssistantMessage } from "./presentAssistantMessage"

+ 0 - 0
src/core/assistant-message/parse-assistant-message.ts → src/core/assistant-message/parseAssistantMessage.ts


+ 525 - 0
src/core/assistant-message/presentAssistantMessage.ts

@@ -0,0 +1,525 @@
+import cloneDeep from "clone-deep"
+import { serializeError } from "serialize-error"
+
+import type { ToolName } from "../../schemas"
+
+import { defaultModeSlug, getModeBySlug } from "../../shared/modes"
+import type { ToolParamName, ToolResponse } from "../../shared/tools"
+import type { ClineAsk, ToolProgressStatus } from "../../shared/ExtensionMessage"
+
+import { telemetryService } from "../../services/telemetry/TelemetryService"
+
+import { fetchInstructionsTool } from "../tools/fetchInstructionsTool"
+import { listFilesTool } from "../tools/listFilesTool"
+import { readFileTool } from "../tools/readFileTool"
+import { writeToFileTool } from "../tools/writeToFileTool"
+import { applyDiffTool } from "../tools/applyDiffTool"
+import { insertContentTool } from "../tools/insertContentTool"
+import { searchAndReplaceTool } from "../tools/searchAndReplaceTool"
+import { listCodeDefinitionNamesTool } from "../tools/listCodeDefinitionNamesTool"
+import { searchFilesTool } from "../tools/searchFilesTool"
+import { browserActionTool } from "../tools/browserActionTool"
+import { executeCommandTool } from "../tools/executeCommandTool"
+import { useMcpToolTool } from "../tools/useMcpToolTool"
+import { accessMcpResourceTool } from "../tools/accessMcpResourceTool"
+import { askFollowupQuestionTool } from "../tools/askFollowupQuestionTool"
+import { switchModeTool } from "../tools/switchModeTool"
+import { attemptCompletionTool } from "../tools/attemptCompletionTool"
+import { newTaskTool } from "../tools/newTaskTool"
+
+import { checkpointSave } from "../checkpoints"
+
+import { formatResponse } from "../prompts/responses"
+import { validateToolUse } from "../tools/validateToolUse"
+import { Cline } from "../Cline"
+
+/**
+ * Processes and presents assistant message content to the user interface.
+ *
+ * This function is the core message handling system that:
+ * - Sequentially processes content blocks from the assistant's response.
+ * - Displays text content to the user.
+ * - Executes tool use requests with appropriate user approval.
+ * - Manages the flow of conversation by determining when to proceed to the next content block.
+ * - Coordinates file system checkpointing for modified files.
+ * - Controls the conversation state to determine when to continue to the next request.
+ *
+ * The function uses a locking mechanism to prevent concurrent execution and handles
+ * partial content blocks during streaming. It's designed to work with the streaming
+ * API response pattern, where content arrives incrementally and needs to be processed
+ * as it becomes available.
+ */
+
+export async function presentAssistantMessage(cline: Cline) {
+	if (cline.abort) {
+		throw new Error(`[Cline#presentAssistantMessage] task ${cline.taskId}.${cline.instanceId} aborted`)
+	}
+
+	if (cline.presentAssistantMessageLocked) {
+		cline.presentAssistantMessageHasPendingUpdates = true
+		return
+	}
+
+	cline.presentAssistantMessageLocked = true
+	cline.presentAssistantMessageHasPendingUpdates = false
+
+	if (cline.currentStreamingContentIndex >= cline.assistantMessageContent.length) {
+		// This may happen if the last content block was completed before
+		// streaming could finish. If streaming is finished, and we're out of
+		// bounds then this means we already  presented/executed the last
+		// content block and are ready to continue to next request.
+		if (cline.didCompleteReadingStream) {
+			cline.userMessageContentReady = true
+		}
+
+		cline.presentAssistantMessageLocked = false
+		return
+	}
+
+	const block = cloneDeep(cline.assistantMessageContent[cline.currentStreamingContentIndex]) // need to create copy bc while stream is updating the array, it could be updating the reference block properties too
+
+	switch (block.type) {
+		case "text": {
+			if (cline.didRejectTool || cline.didAlreadyUseTool) {
+				break
+			}
+
+			let content = block.content
+
+			if (content) {
+				// Have to do this for partial and complete since sending
+				// content in thinking tags to markdown renderer will
+				// automatically be removed.
+				// Remove end substrings of <thinking or </thinking (below xml
+				// parsing is only for opening tags).
+				// Tthis is done with the xml parsing below now, but keeping
+				// here for reference.
+				// content = content.replace(/<\/?t(?:h(?:i(?:n(?:k(?:i(?:n(?:g)?)?)?$/, "")
+				//
+				// Remove all instances of <thinking> (with optional line break
+				// after) and </thinking> (with optional line break before).
+				// - Needs to be separate since we dont want to remove the line
+				//   break before the first tag.
+				// - Needs to happen before the xml parsing below.
+				content = content.replace(/<thinking>\s?/g, "")
+				content = content.replace(/\s?<\/thinking>/g, "")
+
+				// Remove partial XML tag at the very end of the content (for
+				// tool use and thinking tags), Prevents scrollview from
+				// jumping when tags are automatically removed.
+				const lastOpenBracketIndex = content.lastIndexOf("<")
+
+				if (lastOpenBracketIndex !== -1) {
+					const possibleTag = content.slice(lastOpenBracketIndex)
+
+					// Check if there's a '>' after the last '<' (i.e., if the
+					// tag is complete) (complete thinking and tool tags will
+					// have been removed by now.)
+					const hasCloseBracket = possibleTag.includes(">")
+
+					if (!hasCloseBracket) {
+						// Extract the potential tag name.
+						let tagContent: string
+
+						if (possibleTag.startsWith("</")) {
+							tagContent = possibleTag.slice(2).trim()
+						} else {
+							tagContent = possibleTag.slice(1).trim()
+						}
+
+						// Check if tagContent is likely an incomplete tag name
+						// (letters and underscores only).
+						const isLikelyTagName = /^[a-zA-Z_]+$/.test(tagContent)
+
+						// Preemptively remove < or </ to keep from these
+						// artifacts showing up in chat (also handles closing
+						// thinking tags).
+						const isOpeningOrClosing = possibleTag === "<" || possibleTag === "</"
+
+						// If the tag is incomplete and at the end, remove it
+						// from the content.
+						if (isOpeningOrClosing || isLikelyTagName) {
+							content = content.slice(0, lastOpenBracketIndex).trim()
+						}
+					}
+				}
+			}
+
+			await cline.say("text", content, undefined, block.partial)
+			break
+		}
+		case "tool_use":
+			const toolDescription = (): string => {
+				switch (block.name) {
+					case "execute_command":
+						return `[${block.name} for '${block.params.command}']`
+					case "read_file":
+						return `[${block.name} for '${block.params.path}']`
+					case "fetch_instructions":
+						return `[${block.name} for '${block.params.task}']`
+					case "write_to_file":
+						return `[${block.name} for '${block.params.path}']`
+					case "apply_diff":
+						return `[${block.name} for '${block.params.path}']`
+					case "search_files":
+						return `[${block.name} for '${block.params.regex}'${
+							block.params.file_pattern ? ` in '${block.params.file_pattern}'` : ""
+						}]`
+					case "insert_content":
+						return `[${block.name} for '${block.params.path}']`
+					case "search_and_replace":
+						return `[${block.name} for '${block.params.path}']`
+					case "list_files":
+						return `[${block.name} for '${block.params.path}']`
+					case "list_code_definition_names":
+						return `[${block.name} for '${block.params.path}']`
+					case "browser_action":
+						return `[${block.name} for '${block.params.action}']`
+					case "use_mcp_tool":
+						return `[${block.name} for '${block.params.server_name}']`
+					case "access_mcp_resource":
+						return `[${block.name} for '${block.params.server_name}']`
+					case "ask_followup_question":
+						return `[${block.name} for '${block.params.question}']`
+					case "attempt_completion":
+						return `[${block.name}]`
+					case "switch_mode":
+						return `[${block.name} to '${block.params.mode_slug}'${block.params.reason ? ` because: ${block.params.reason}` : ""}]`
+					case "new_task": {
+						const mode = block.params.mode ?? defaultModeSlug
+						const message = block.params.message ?? "(no message)"
+						const modeName = getModeBySlug(mode, customModes)?.name ?? mode
+						return `[${block.name} in ${modeName} mode: '${message}']`
+					}
+				}
+			}
+
+			if (cline.didRejectTool) {
+				// Ignore any tool content after user has rejected tool once.
+				if (!block.partial) {
+					cline.userMessageContent.push({
+						type: "text",
+						text: `Skipping tool ${toolDescription()} due to user rejecting a previous tool.`,
+					})
+				} else {
+					// Partial tool after user rejected a previous tool.
+					cline.userMessageContent.push({
+						type: "text",
+						text: `Tool ${toolDescription()} was interrupted and not executed due to user rejecting a previous tool.`,
+					})
+				}
+
+				break
+			}
+
+			if (cline.didAlreadyUseTool) {
+				// Ignore any content after a tool has already been used.
+				cline.userMessageContent.push({
+					type: "text",
+					text: `Tool [${block.name}] was not executed because a tool has already been used in this message. Only one tool may be used per message. You must assess the first tool's result before proceeding to use the next tool.`,
+				})
+
+				break
+			}
+
+			const pushToolResult = (content: ToolResponse) => {
+				cline.userMessageContent.push({ type: "text", text: `${toolDescription()} Result:` })
+
+				if (typeof content === "string") {
+					cline.userMessageContent.push({ type: "text", text: content || "(tool did not return anything)" })
+				} else {
+					cline.userMessageContent.push(...content)
+				}
+
+				// Once a tool result has been collected, ignore all other tool
+				// uses since we should only ever present one tool result per
+				// message.
+				cline.didAlreadyUseTool = true
+			}
+
+			const askApproval = async (
+				type: ClineAsk,
+				partialMessage?: string,
+				progressStatus?: ToolProgressStatus,
+			) => {
+				const { response, text, images } = await cline.ask(type, partialMessage, false, progressStatus)
+
+				if (response !== "yesButtonClicked") {
+					// Handle both messageResponse and noButtonClicked with text.
+					if (text) {
+						await cline.say("user_feedback", text, images)
+						pushToolResult(formatResponse.toolResult(formatResponse.toolDeniedWithFeedback(text), images))
+					} else {
+						pushToolResult(formatResponse.toolDenied())
+					}
+					cline.didRejectTool = true
+					return false
+				}
+
+				// Handle yesButtonClicked with text.
+				if (text) {
+					await cline.say("user_feedback", text, images)
+					pushToolResult(formatResponse.toolResult(formatResponse.toolApprovedWithFeedback(text), images))
+				}
+
+				return true
+			}
+
+			const askFinishSubTaskApproval = async () => {
+				// Ask the user to approve this task has completed, and he has
+				// reviewed it, and we can declare task is finished and return
+				// control to the parent task to continue running the rest of
+				// the sub-tasks.
+				const toolMessage = JSON.stringify({ tool: "finishTask" })
+				return await askApproval("tool", toolMessage)
+			}
+
+			const handleError = async (action: string, error: Error) => {
+				const errorString = `Error ${action}: ${JSON.stringify(serializeError(error))}`
+
+				await cline.say(
+					"error",
+					`Error ${action}:\n${error.message ?? JSON.stringify(serializeError(error), null, 2)}`,
+				)
+
+				pushToolResult(formatResponse.toolError(errorString))
+			}
+
+			// If block is partial, remove partial closing tag so its not
+			// presented to user.
+			const removeClosingTag = (tag: ToolParamName, text?: string): string => {
+				if (!block.partial) {
+					return text || ""
+				}
+
+				if (!text) {
+					return ""
+				}
+
+				// This regex dynamically constructs a pattern to match the
+				// closing tag:
+				// - Optionally matches whitespace before the tag.
+				// - Matches '<' or '</' optionally followed by any subset of
+				//   characters from the tag name.
+				const tagRegex = new RegExp(
+					`\\s?<\/?${tag
+						.split("")
+						.map((char) => `(?:${char})?`)
+						.join("")}$`,
+					"g",
+				)
+
+				return text.replace(tagRegex, "")
+			}
+
+			if (block.name !== "browser_action") {
+				await cline.browserSession.closeBrowser()
+			}
+
+			if (!block.partial) {
+				cline.recordToolUsage(block.name)
+				telemetryService.captureToolUsage(cline.taskId, block.name)
+			}
+
+			// Validate tool use before execution.
+			const { mode, customModes } = (await cline.providerRef.deref()?.getState()) ?? {}
+
+			try {
+				validateToolUse(
+					block.name as ToolName,
+					mode ?? defaultModeSlug,
+					customModes ?? [],
+					{ apply_diff: cline.diffEnabled },
+					block.params,
+				)
+			} catch (error) {
+				cline.consecutiveMistakeCount++
+				pushToolResult(formatResponse.toolError(error.message))
+				break
+			}
+
+			// Check for identical consecutive tool calls.
+			if (!block.partial) {
+				// Use the detector to check for repetition, passing the ToolUse
+				// block directly.
+				const repetitionCheck = cline.toolRepetitionDetector.check(block)
+
+				// If execution is not allowed, notify user and break.
+				if (!repetitionCheck.allowExecution && repetitionCheck.askUser) {
+					// Handle repetition similar to mistake_limit_reached pattern.
+					const { response, text, images } = await cline.ask(
+						repetitionCheck.askUser.messageKey as ClineAsk,
+						repetitionCheck.askUser.messageDetail.replace("{toolName}", block.name),
+					)
+
+					if (response === "messageResponse") {
+						// Add user feedback to userContent.
+						cline.userMessageContent.push(
+							{
+								type: "text" as const,
+								text: `Tool repetition limit reached. User feedback: ${text}`,
+							},
+							...formatResponse.imageBlocks(images),
+						)
+
+						// Add user feedback to chat.
+						await cline.say("user_feedback", text, images)
+
+						// Track tool repetition in telemetry.
+						telemetryService.captureConsecutiveMistakeError(cline.taskId)
+					}
+
+					// Return tool result message about the repetition
+					pushToolResult(
+						formatResponse.toolError(
+							`Tool call repetition limit reached for ${block.name}. Please try a different approach.`,
+						),
+					)
+					break
+				}
+			}
+
+			switch (block.name) {
+				case "write_to_file":
+					await writeToFileTool(cline, block, askApproval, handleError, pushToolResult, removeClosingTag)
+					break
+				case "apply_diff":
+					await applyDiffTool(cline, block, askApproval, handleError, pushToolResult, removeClosingTag)
+					break
+				case "insert_content":
+					await insertContentTool(cline, block, askApproval, handleError, pushToolResult, removeClosingTag)
+					break
+				case "search_and_replace":
+					await searchAndReplaceTool(cline, block, askApproval, handleError, pushToolResult, removeClosingTag)
+					break
+				case "read_file":
+					await readFileTool(cline, block, askApproval, handleError, pushToolResult, removeClosingTag)
+
+					break
+				case "fetch_instructions":
+					await fetchInstructionsTool(cline, block, askApproval, handleError, pushToolResult)
+					break
+				case "list_files":
+					await listFilesTool(cline, block, askApproval, handleError, pushToolResult, removeClosingTag)
+					break
+				case "list_code_definition_names":
+					await listCodeDefinitionNamesTool(
+						cline,
+						block,
+						askApproval,
+						handleError,
+						pushToolResult,
+						removeClosingTag,
+					)
+					break
+				case "search_files":
+					await searchFilesTool(cline, block, askApproval, handleError, pushToolResult, removeClosingTag)
+					break
+				case "browser_action":
+					await browserActionTool(cline, block, askApproval, handleError, pushToolResult, removeClosingTag)
+					break
+				case "execute_command":
+					await executeCommandTool(cline, block, askApproval, handleError, pushToolResult, removeClosingTag)
+					break
+				case "use_mcp_tool":
+					await useMcpToolTool(cline, block, askApproval, handleError, pushToolResult, removeClosingTag)
+					break
+				case "access_mcp_resource":
+					await accessMcpResourceTool(
+						cline,
+						block,
+						askApproval,
+						handleError,
+						pushToolResult,
+						removeClosingTag,
+					)
+					break
+				case "ask_followup_question":
+					await askFollowupQuestionTool(
+						cline,
+						block,
+						askApproval,
+						handleError,
+						pushToolResult,
+						removeClosingTag,
+					)
+					break
+				case "switch_mode":
+					await switchModeTool(cline, block, askApproval, handleError, pushToolResult, removeClosingTag)
+					break
+				case "new_task":
+					await newTaskTool(cline, block, askApproval, handleError, pushToolResult, removeClosingTag)
+					break
+				case "attempt_completion":
+					await attemptCompletionTool(
+						cline,
+						block,
+						askApproval,
+						handleError,
+						pushToolResult,
+						removeClosingTag,
+						toolDescription,
+						askFinishSubTaskApproval,
+					)
+					break
+			}
+
+			break
+	}
+
+	const recentlyModifiedFiles = cline.fileContextTracker.getAndClearCheckpointPossibleFile()
+
+	if (recentlyModifiedFiles.length > 0) {
+		// TODO: We can track what file changes were made and only
+		// checkpoint those files, this will be save storage.
+		await checkpointSave(cline)
+	}
+
+	// Seeing out of bounds is fine, it means that the next too call is being
+	// built up and ready to add to assistantMessageContent to present.
+	// When you see the UI inactive during this, it means that a tool is
+	// breaking without presenting any UI. For example the write_to_file tool
+	// was breaking when relpath was undefined, and for invalid relpath it never
+	// presented UI.
+	// This needs to be placed here, if not then calling
+	// cline.presentAssistantMessage below would fail (sometimes) since it's
+	// locked.
+	cline.presentAssistantMessageLocked = false
+
+	// NOTE: When tool is rejected, iterator stream is interrupted and it waits
+	// for `userMessageContentReady` to be true. Future calls to present will
+	// skip execution since `didRejectTool` and iterate until `contentIndex` is
+	// set to message length and it sets userMessageContentReady to true itself
+	// (instead of preemptively doing it in iterator).
+	if (!block.partial || cline.didRejectTool || cline.didAlreadyUseTool) {
+		// Block is finished streaming and executing.
+		if (cline.currentStreamingContentIndex === cline.assistantMessageContent.length - 1) {
+			// It's okay that we increment if !didCompleteReadingStream, it'll
+			// just return because out of bounds and as streaming continues it
+			// will call `presentAssitantMessage` if a new block is ready. If
+			// streaming is finished then we set `userMessageContentReady` to
+			// true when out of bounds. This gracefully allows the stream to
+			// continue on and all potential content blocks be presented.
+			// Last block is complete and it is finished executing
+			cline.userMessageContentReady = true // Will allow `pWaitFor` to continue.
+		}
+
+		// Call next block if it exists (if not then read stream will call it
+		// when it's ready).
+		// Need to increment regardless, so when read stream calls this function
+		// again it will be streaming the next block.
+		cline.currentStreamingContentIndex++
+
+		if (cline.currentStreamingContentIndex < cline.assistantMessageContent.length) {
+			// There are already more content blocks to stream, so we'll call
+			// this function ourselves.
+			presentAssistantMessage(cline)
+			return
+		}
+	}
+
+	// Block is partial, but the read stream may have finished.
+	if (cline.presentAssistantMessageHasPendingUpdates) {
+		presentAssistantMessage(cline)
+	}
+}

+ 2 - 2
src/core/ToolRepetitionDetector.ts → src/core/tools/ToolRepetitionDetector.ts

@@ -1,5 +1,5 @@
-import { ToolUse } from "../shared/tools"
-import { t } from "../i18n"
+import { ToolUse } from "../../shared/tools"
+import { t } from "../../i18n"
 
 /**
  * Class for detecting consecutive identical tool calls

+ 7 - 6
src/core/__tests__/ToolRepetitionDetector.test.ts → src/core/tools/__tests__/ToolRepetitionDetector.test.ts

@@ -1,11 +1,13 @@
+// npx jest src/core/tools/__tests__/ToolRepetitionDetector.test.ts
+
+import type { ToolName } from "../../../schemas"
+import type { ToolUse } from "../../../shared/tools"
+
 import { ToolRepetitionDetector } from "../ToolRepetitionDetector"
-import { ToolUse } from "../../shared/tools"
-import { ToolName } from "../../schemas"
 
-// Mock the i18n system
-jest.mock("../../i18n", () => ({
+jest.mock("../../../i18n", () => ({
 	t: jest.fn((key, options) => {
-		// For toolRepetitionLimitReached key, return a message with the tool name
+		// For toolRepetitionLimitReached key, return a message with the tool name.
 		if (key === "tools:toolRepetitionLimitReached" && options?.toolName) {
 			return `Roo appears to be stuck in a loop, attempting the same action (${options.toolName}) repeatedly. This might indicate a problem with its current strategy.`
 		}
@@ -13,7 +15,6 @@ jest.mock("../../i18n", () => ({
 	}),
 }))
 
-// Helper function to create a mock ToolUse
 function createToolUse(name: string, displayName?: string, params: Record<string, string> = {}): ToolUse {
 	return {
 		type: "tool_use",

+ 383 - 43
src/core/__tests__/read-file-xml.test.ts → src/core/tools/__tests__/readFileTool.test.ts

@@ -1,19 +1,38 @@
-// npx jest src/core/__tests__/read-file-xml.test.ts
+// npx jest src/core/tools/__tests__/readFileTool.test.ts
 
 import * as path from "path"
 
-import { countFileLines } from "../../integrations/misc/line-counter"
-import { readLines } from "../../integrations/misc/read-lines"
-import { extractTextFromFile } from "../../integrations/misc/extract-text"
-import { parseSourceCodeDefinitionsForFile } from "../../services/tree-sitter"
+import { countFileLines } from "../../../integrations/misc/line-counter"
+import { readLines } from "../../../integrations/misc/read-lines"
+import { extractTextFromFile } from "../../../integrations/misc/extract-text"
+import { parseSourceCodeDefinitionsForFile } from "../../../services/tree-sitter"
 import { isBinaryFile } from "isbinaryfile"
-import { ReadFileToolUse } from "../../shared/tools"
+import { ReadFileToolUse, ToolParamName, ToolResponse } from "../../../shared/tools"
+import { readFileTool } from "../readFileTool"
 
-// Mock dependencies
-jest.mock("../../integrations/misc/line-counter")
-jest.mock("../../integrations/misc/read-lines")
-jest.mock("../../integrations/misc/extract-text", () => {
-	const actual = jest.requireActual("../../integrations/misc/extract-text")
+jest.mock("path", () => {
+	const originalPath = jest.requireActual("path")
+	return {
+		...originalPath,
+		resolve: jest.fn().mockImplementation((...args) => args.join("/")),
+	}
+})
+
+jest.mock("fs/promises", () => ({
+	mkdir: jest.fn().mockResolvedValue(undefined),
+	writeFile: jest.fn().mockResolvedValue(undefined),
+	readFile: jest.fn().mockResolvedValue("{}"),
+}))
+
+jest.mock("isbinaryfile")
+
+jest.mock("../../../integrations/misc/line-counter")
+jest.mock("../../../integrations/misc/read-lines")
+
+let mockInputContent = ""
+
+jest.mock("../../../integrations/misc/extract-text", () => {
+	const actual = jest.requireActual("../../../integrations/misc/extract-text")
 	// Create a spy on the actual addLineNumbers function
 	const addLineNumbersSpy = jest.spyOn(actual, "addLineNumbers")
 
@@ -29,14 +48,11 @@ jest.mock("../../integrations/misc/extract-text", () => {
 	}
 })
 
-// Get a reference to the spy
-const addLineNumbersSpy = jest.requireMock("../../integrations/misc/extract-text").__addLineNumbersSpy
+const addLineNumbersSpy = jest.requireMock("../../../integrations/misc/extract-text").__addLineNumbersSpy
 
-// Variable to control what content is used by the mock
-let mockInputContent = ""
-jest.mock("../../services/tree-sitter")
-jest.mock("isbinaryfile")
-jest.mock("../ignore/RooIgnoreController", () => ({
+jest.mock("../../../services/tree-sitter")
+
+jest.mock("../../ignore/RooIgnoreController", () => ({
 	RooIgnoreController: class {
 		initialize() {
 			return Promise.resolve()
@@ -46,22 +62,349 @@ jest.mock("../ignore/RooIgnoreController", () => ({
 		}
 	},
 }))
-jest.mock("fs/promises", () => ({
-	mkdir: jest.fn().mockResolvedValue(undefined),
-	writeFile: jest.fn().mockResolvedValue(undefined),
-	readFile: jest.fn().mockResolvedValue("{}"),
-}))
-jest.mock("../../utils/fs", () => ({
+
+jest.mock("../../../utils/fs", () => ({
 	fileExistsAtPath: jest.fn().mockReturnValue(true),
 }))
 
-// Mock path
-jest.mock("path", () => {
-	const originalPath = jest.requireActual("path")
-	return {
-		...originalPath,
-		resolve: jest.fn().mockImplementation((...args) => args.join("/")),
+describe("read_file tool with maxReadFileLine setting", () => {
+	// Test data
+	const testFilePath = "test/file.txt"
+	const absoluteFilePath = "/test/file.txt"
+	const fileContent = "Line 1\nLine 2\nLine 3\nLine 4\nLine 5"
+	const numberedFileContent = "1 | Line 1\n2 | Line 2\n3 | Line 3\n4 | Line 4\n5 | Line 5\n"
+	const sourceCodeDef = "\n\n# file.txt\n1--5 | Content"
+	const expectedFullFileXml = `<file><path>${testFilePath}</path>\n<content lines="1-5">\n${numberedFileContent}</content>\n</file>`
+
+	// Mocked functions with correct types
+	const mockedCountFileLines = countFileLines as jest.MockedFunction<typeof countFileLines>
+	const mockedReadLines = readLines as jest.MockedFunction<typeof readLines>
+	const mockedExtractTextFromFile = extractTextFromFile as jest.MockedFunction<typeof extractTextFromFile>
+	const mockedParseSourceCodeDefinitionsForFile = parseSourceCodeDefinitionsForFile as jest.MockedFunction<
+		typeof parseSourceCodeDefinitionsForFile
+	>
+
+	const mockedIsBinaryFile = isBinaryFile as jest.MockedFunction<typeof isBinaryFile>
+	const mockedPathResolve = path.resolve as jest.MockedFunction<typeof path.resolve>
+
+	// Mock instances
+	const mockCline: any = {}
+	let mockProvider: any
+	let toolResult: ToolResponse | undefined
+
+	beforeEach(() => {
+		jest.clearAllMocks()
+
+		// Setup path resolution
+		mockedPathResolve.mockReturnValue(absoluteFilePath)
+
+		// Setup mocks for file operations
+		mockedIsBinaryFile.mockResolvedValue(false)
+
+		// Set the default content for the mock
+		mockInputContent = fileContent
+
+		// Setup the extractTextFromFile mock implementation with the current mockInputContent
+		mockedExtractTextFromFile.mockImplementation((_filePath) => {
+			const actual = jest.requireActual("../../../integrations/misc/extract-text")
+			return Promise.resolve(actual.addLineNumbers(mockInputContent))
+		})
+
+		// No need to setup the extractTextFromFile mock implementation here
+		// as it's already defined at the module level
+
+		// Setup mock provider
+		mockProvider = {
+			getState: jest.fn(),
+			deref: jest.fn().mockReturnThis(),
+		}
+
+		// Setup Cline instance with mock methods
+		mockCline.cwd = "/"
+		mockCline.task = "Test"
+		mockCline.providerRef = mockProvider
+		mockCline.rooIgnoreController = {
+			validateAccess: jest.fn().mockReturnValue(true),
+		}
+		mockCline.say = jest.fn().mockResolvedValue(undefined)
+		mockCline.ask = jest.fn().mockResolvedValue(true)
+		mockCline.presentAssistantMessage = jest.fn()
+		mockCline.getFileContextTracker = jest.fn().mockReturnValue({
+			trackFileContext: jest.fn().mockResolvedValue(undefined),
+		})
+		mockCline.recordToolUsage = jest.fn().mockReturnValue(undefined)
+		mockCline.recordToolError = jest.fn().mockReturnValue(undefined)
+		// Reset tool result
+		toolResult = undefined
+	})
+
+	/**
+	 * Helper function to execute the read file tool with different maxReadFileLine settings
+	 */
+	async function executeReadFileTool(
+		params: Partial<ReadFileToolUse["params"]> = {},
+		options: {
+			maxReadFileLine?: number
+			totalLines?: number
+			skipAddLineNumbersCheck?: boolean // Flag to skip addLineNumbers check
+		} = {},
+	): Promise<ToolResponse | undefined> {
+		// Configure mocks based on test scenario
+		const maxReadFileLine = options.maxReadFileLine ?? 500
+		const totalLines = options.totalLines ?? 5
+
+		mockProvider.getState.mockResolvedValue({ maxReadFileLine })
+		mockedCountFileLines.mockResolvedValue(totalLines)
+
+		// Reset the spy before each test
+		addLineNumbersSpy.mockClear()
+
+		// Create a tool use object
+		const toolUse: ReadFileToolUse = {
+			type: "tool_use",
+			name: "read_file",
+			params: { path: testFilePath, ...params },
+			partial: false,
+		}
+
+		await readFileTool(
+			mockCline,
+			toolUse,
+			mockCline.ask,
+			jest.fn(),
+			(result: ToolResponse) => {
+				toolResult = result
+			},
+			(_: ToolParamName, content?: string) => content ?? "",
+		)
+
+		// Verify addLineNumbers was called appropriately
+		if (!options.skipAddLineNumbersCheck) {
+			expect(addLineNumbersSpy).toHaveBeenCalled()
+		} else {
+			expect(addLineNumbersSpy).not.toHaveBeenCalled()
+		}
+
+		return toolResult
 	}
+
+	describe("when maxReadFileLine is negative", () => {
+		it("should read the entire file using extractTextFromFile", async () => {
+			// Setup - use default mockInputContent
+			mockInputContent = fileContent
+
+			// Execute
+			const result = await executeReadFileTool({}, { maxReadFileLine: -1 })
+
+			// Verify
+			expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath)
+			expect(mockedReadLines).not.toHaveBeenCalled()
+			expect(mockedParseSourceCodeDefinitionsForFile).not.toHaveBeenCalled()
+			expect(result).toBe(expectedFullFileXml)
+		})
+
+		it("should ignore range parameters and read entire file when maxReadFileLine is -1", async () => {
+			// Setup - use default mockInputContent
+			mockInputContent = fileContent
+
+			// Execute with range parameters
+			const result = await executeReadFileTool(
+				{
+					start_line: "2",
+					end_line: "4",
+				},
+				{ maxReadFileLine: -1 },
+			)
+
+			// Verify that extractTextFromFile is still used (not readLines)
+			expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath)
+			expect(mockedReadLines).not.toHaveBeenCalled()
+			expect(mockedParseSourceCodeDefinitionsForFile).not.toHaveBeenCalled()
+			expect(result).toBe(expectedFullFileXml)
+		})
+
+		it("should not show line snippet in approval message when maxReadFileLine is -1", async () => {
+			// This test verifies the line snippet behavior for the approval message
+			// Setup - use default mockInputContent
+			mockInputContent = fileContent
+
+			// Execute - we'll reuse executeReadFileTool to run the tool
+			await executeReadFileTool({}, { maxReadFileLine: -1 })
+
+			// Verify the empty line snippet for full read was passed to the approval message
+			// Look at the parameters passed to the 'ask' method in the approval message
+			const askCall = mockCline.ask.mock.calls[0]
+			const completeMessage = JSON.parse(askCall[1])
+
+			// Verify the reason (lineSnippet) is empty or undefined for full read
+			expect(completeMessage.reason).toBeFalsy()
+		})
+	})
+
+	describe("when maxReadFileLine is 0", () => {
+		it("should return an empty content with source code definitions", async () => {
+			// Setup - for maxReadFileLine = 0, the implementation won't call readLines
+			mockedParseSourceCodeDefinitionsForFile.mockResolvedValue(sourceCodeDef)
+
+			// Execute - skip addLineNumbers check as it's not called for maxReadFileLine=0
+			const result = await executeReadFileTool(
+				{},
+				{
+					maxReadFileLine: 0,
+					totalLines: 5,
+					skipAddLineNumbersCheck: true,
+				},
+			)
+
+			// Verify
+			expect(mockedExtractTextFromFile).not.toHaveBeenCalled()
+			expect(mockedReadLines).not.toHaveBeenCalled() // Per implementation line 141
+			expect(mockedParseSourceCodeDefinitionsForFile).toHaveBeenCalledWith(
+				absoluteFilePath,
+				mockCline.rooIgnoreController,
+			)
+
+			// Verify XML structure
+			expect(result).toContain(`<file><path>${testFilePath}</path>`)
+			expect(result).toContain("<notice>Showing only 0 of 5 total lines")
+			expect(result).toContain("</notice>")
+			expect(result).toContain("<list_code_definition_names>")
+			expect(result).toContain(sourceCodeDef.trim())
+			expect(result).toContain("</list_code_definition_names>")
+			expect(result).not.toContain("<content") // No content when maxReadFileLine is 0
+		})
+	})
+
+	describe("when maxReadFileLine is less than file length", () => {
+		it("should read only maxReadFileLine lines and add source code definitions", async () => {
+			// Setup
+			const content = "Line 1\nLine 2\nLine 3"
+			mockedReadLines.mockResolvedValue(content)
+			mockedParseSourceCodeDefinitionsForFile.mockResolvedValue(sourceCodeDef)
+
+			// Execute
+			const result = await executeReadFileTool({}, { maxReadFileLine: 3 })
+
+			// Verify - check behavior but not specific implementation details
+			expect(mockedExtractTextFromFile).not.toHaveBeenCalled()
+			expect(mockedReadLines).toHaveBeenCalled()
+			expect(mockedParseSourceCodeDefinitionsForFile).toHaveBeenCalledWith(
+				absoluteFilePath,
+				mockCline.rooIgnoreController,
+			)
+
+			// Verify XML structure
+			expect(result).toContain(`<file><path>${testFilePath}</path>`)
+			expect(result).toContain('<content lines="1-3">')
+			expect(result).toContain("1 | Line 1")
+			expect(result).toContain("2 | Line 2")
+			expect(result).toContain("3 | Line 3")
+			expect(result).toContain("</content>")
+			expect(result).toContain("<notice>Showing only 3 of 5 total lines")
+			expect(result).toContain("</notice>")
+			expect(result).toContain("<list_code_definition_names>")
+			expect(result).toContain(sourceCodeDef.trim())
+			expect(result).toContain("</list_code_definition_names>")
+			expect(result).toContain("<list_code_definition_names>")
+			expect(result).toContain(sourceCodeDef.trim())
+		})
+	})
+
+	describe("when maxReadFileLine equals or exceeds file length", () => {
+		it("should use extractTextFromFile when maxReadFileLine > totalLines", async () => {
+			// Setup
+			mockedCountFileLines.mockResolvedValue(5) // File shorter than maxReadFileLine
+			mockInputContent = fileContent
+
+			// Execute
+			const result = await executeReadFileTool({}, { maxReadFileLine: 10, totalLines: 5 })
+
+			// Verify
+			expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath)
+			expect(result).toBe(expectedFullFileXml)
+		})
+
+		it("should read with extractTextFromFile when file has few lines", async () => {
+			// Setup
+			mockedCountFileLines.mockResolvedValue(3) // File shorter than maxReadFileLine
+			mockInputContent = fileContent
+
+			// Execute
+			const result = await executeReadFileTool({}, { maxReadFileLine: 5, totalLines: 3 })
+
+			// Verify
+			expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath)
+			expect(mockedReadLines).not.toHaveBeenCalled()
+			// Create a custom expected XML with lines="1-3" since totalLines is 3
+			const expectedXml = `<file><path>${testFilePath}</path>\n<content lines="1-3">\n${numberedFileContent}</content>\n</file>`
+			expect(result).toBe(expectedXml)
+		})
+	})
+
+	describe("when file is binary", () => {
+		it("should always use extractTextFromFile regardless of maxReadFileLine", async () => {
+			// Setup
+			mockedIsBinaryFile.mockResolvedValue(true)
+			// For binary files, we're using a maxReadFileLine of 3 and totalLines is assumed to be 3
+			mockedCountFileLines.mockResolvedValue(3)
+
+			// For binary files, we need a special mock implementation that doesn't use addLineNumbers
+			// Save the original mock implementation
+			const originalMockImplementation = mockedExtractTextFromFile.getMockImplementation()
+			// Create a special mock implementation that doesn't call addLineNumbers
+			mockedExtractTextFromFile.mockImplementation(() => {
+				return Promise.resolve(numberedFileContent)
+			})
+
+			// Reset the spy to clear any previous calls
+			addLineNumbersSpy.mockClear()
+
+			// Execute - skip addLineNumbers check as we're directly providing the numbered content
+			const result = await executeReadFileTool(
+				{},
+				{
+					maxReadFileLine: 3,
+					totalLines: 3,
+					skipAddLineNumbersCheck: true,
+				},
+			)
+
+			// Restore the original mock implementation after the test
+			mockedExtractTextFromFile.mockImplementation(originalMockImplementation)
+
+			// Verify
+			expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath)
+			expect(mockedReadLines).not.toHaveBeenCalled()
+			// Create a custom expected XML with lines="1-3" for binary files
+			const expectedXml = `<file><path>${testFilePath}</path>\n<content lines="1-3">\n${numberedFileContent}</content>\n</file>`
+			expect(result).toBe(expectedXml)
+		})
+	})
+
+	describe("with range parameters", () => {
+		it("should honor start_line and end_line when provided", async () => {
+			// Setup
+			mockedReadLines.mockResolvedValue("Line 2\nLine 3\nLine 4")
+
+			// Execute using executeReadFileTool with range parameters
+			const rangeResult = await executeReadFileTool({
+				start_line: "2",
+				end_line: "4",
+			})
+
+			// Verify
+			expect(mockedReadLines).toHaveBeenCalledWith(absoluteFilePath, 3, 1) // end_line - 1, start_line - 1
+			expect(addLineNumbersSpy).toHaveBeenCalledWith(expect.any(String), 2) // start with proper line numbers
+
+			// Verify XML structure with lines attribute
+			expect(rangeResult).toContain(`<file><path>${testFilePath}</path>`)
+			expect(rangeResult).toContain(`<content lines="2-4">`)
+			expect(rangeResult).toContain("2 | Line 2")
+			expect(rangeResult).toContain("3 | Line 3")
+			expect(rangeResult).toContain("4 | Line 4")
+			expect(rangeResult).toContain("</content>")
+		})
+	})
 })
 
 describe("read_file tool XML output structure", () => {
@@ -85,7 +428,7 @@ describe("read_file tool XML output structure", () => {
 	// Mock instances
 	const mockCline: any = {}
 	let mockProvider: any
-	let toolResult: string | undefined
+	let toolResult: ToolResponse | undefined
 
 	beforeEach(() => {
 		jest.clearAllMocks()
@@ -139,7 +482,7 @@ describe("read_file tool XML output structure", () => {
 			validateAccess?: boolean
 			skipAddLineNumbersCheck?: boolean // Flag to skip addLineNumbers check
 		} = {},
-	): Promise<string | undefined> {
+	): Promise<ToolResponse | undefined> {
 		// Configure mocks based on test scenario
 		const totalLines = options.totalLines ?? 5
 		const maxReadFileLine = options.maxReadFileLine ?? 500
@@ -162,9 +505,6 @@ describe("read_file tool XML output structure", () => {
 			partial: false,
 		}
 
-		// Import the tool implementation dynamically to avoid hoisting issues
-		const { readFileTool } = require("../tools/readFileTool")
-
 		// Reset the spy's call history before each test
 		addLineNumbersSpy.mockClear()
 
@@ -174,10 +514,10 @@ describe("read_file tool XML output structure", () => {
 			toolUse,
 			mockCline.ask,
 			jest.fn(),
-			(result: string) => {
+			(result: ToolResponse) => {
 				toolResult = result
 			},
-			(param: string, value: string) => value,
+			(param: ToolParamName, content?: string) => content ?? "",
 		)
 		// Verify addLineNumbers was called (unless explicitly skipped)
 		if (!options.skipAddLineNumbersCheck) {
@@ -410,7 +750,9 @@ describe("read_file tool XML output structure", () => {
 
 			// Should contain all the requested lines, not just maxReadFileLine lines
 			expect(result).toBeDefined()
-			if (result) {
+			expect(typeof result).toBe("string")
+
+			if (typeof result === "string") {
 				expect(result.split("\n").length).toBeGreaterThan(maxReadFileLine)
 			}
 		})
@@ -507,19 +849,16 @@ describe("read_file tool XML output structure", () => {
 				partial: false,
 			}
 
-			// Import the tool implementation dynamically
-			const { readFileTool } = require("../tools/readFileTool")
-
 			// Execute the tool
 			await readFileTool(
 				mockCline,
 				toolUse,
 				mockCline.ask,
 				jest.fn(),
-				(result: string) => {
+				(result: ToolResponse) => {
 					toolResult = result
 				},
-				(param: string, value: string) => value,
+				(param: ToolParamName, content?: string) => content ?? "",
 			)
 
 			// Verify
@@ -565,6 +904,7 @@ describe("read_file tool XML output structure", () => {
 
 			// Execute
 			const result = await executeReadFileTool({}, { maxReadFileLine, totalLines })
+			console.log(result)
 
 			// Verify
 			// Empty files should include a content tag and notice

+ 4 - 4
src/core/__tests__/mode-validator.test.ts → src/core/tools/__tests__/validateToolUse.test.ts

@@ -1,8 +1,8 @@
-// npx jest src/core/__tests__/mode-validator.test.ts
+// npx jest src/core/tools/__tests__/validateToolUse.test.ts
 
-import { isToolAllowedForMode, modes, ModeConfig } from "../../shared/modes"
-import { TOOL_GROUPS } from "../../shared/tools"
-import { validateToolUse } from "../mode-validator"
+import { isToolAllowedForMode, modes, ModeConfig } from "../../../shared/modes"
+import { TOOL_GROUPS } from "../../../shared/tools"
+import { validateToolUse } from "../validateToolUse"
 
 const [codeMode, architectMode, askMode] = modes.map((mode) => mode.slug)
 

+ 2 - 2
src/core/mode-validator.ts → src/core/tools/validateToolUse.ts

@@ -1,5 +1,5 @@
-import { ToolName } from "../schemas"
-import { Mode, isToolAllowedForMode, ModeConfig } from "../shared/modes"
+import { ToolName } from "../../schemas"
+import { Mode, isToolAllowedForMode, ModeConfig } from "../../shared/modes"
 
 export function validateToolUse(
 	toolName: ToolName,

+ 1 - 1
src/core/webview/ClineProvider.ts

@@ -41,7 +41,7 @@ import { ContextProxy } from "../config/ContextProxy"
 import { ProviderSettingsManager } from "../config/ProviderSettingsManager"
 import { CustomModesManager } from "../config/CustomModesManager"
 import { buildApiHandler } from "../../api"
-import { CodeActionName } from "../CodeActionProvider"
+import { CodeActionName } from "../../activate/CodeActionProvider"
 import { Cline, ClineOptions } from "../Cline"
 import { getNonce } from "./getNonce"
 import { getUri } from "./getUri"

+ 10 - 5
src/extension.ts

@@ -14,20 +14,25 @@ try {
 
 import "./utils/path" // Necessary to have access to String.prototype.toPosix.
 
-import { initializeI18n } from "./i18n"
 import { ContextProxy } from "./core/config/ContextProxy"
 import { ClineProvider } from "./core/webview/ClineProvider"
-import { CodeActionProvider } from "./core/CodeActionProvider"
 import { DIFF_VIEW_URI_SCHEME } from "./integrations/editor/DiffViewProvider"
+import { TerminalRegistry } from "./integrations/terminal/TerminalRegistry"
 import { McpServerManager } from "./services/mcp/McpServerManager"
 import { telemetryService } from "./services/telemetry/TelemetryService"
-import { TerminalRegistry } from "./integrations/terminal/TerminalRegistry"
 import { API } from "./exports/api"
 import { migrateSettings } from "./utils/migrateSettings"
-
-import { handleUri, registerCommands, registerCodeActions, registerTerminalActions } from "./activate"
 import { formatLanguage } from "./shared/language"
 
+import {
+	handleUri,
+	registerCommands,
+	registerCodeActions,
+	registerTerminalActions,
+	CodeActionProvider,
+} from "./activate"
+import { initializeI18n } from "./i18n"
+
 /**
  * Built using https://github.com/microsoft/vscode-webview-ui-toolkit
  *

+ 0 - 0
src/core/EditorUtils.ts → src/integrations/editor/EditorUtils.ts


+ 1 - 1
src/core/__tests__/EditorUtils.test.ts → src/integrations/editor/__tests__/EditorUtils.test.ts

@@ -1,4 +1,4 @@
-// npx jest src/core/__tests__/EditorUtils.test.ts
+// npx jest src/integrations/editor/__tests__/EditorUtils.test.ts
 
 import * as vscode from "vscode"
 

+ 11 - 11
src/core/__tests__/read-file-tool.test.ts → src/integrations/misc/__tests__/read-file-tool.test.ts

@@ -1,21 +1,21 @@
-// npx jest src/core/__tests__/read-file-tool.test.ts
+// npx jest src/integrations/misc/__tests__/read-file-tool.test.ts
 
 import * as path from "path"
-import { countFileLines } from "../../integrations/misc/line-counter"
-import { readLines } from "../../integrations/misc/read-lines"
-import { extractTextFromFile, addLineNumbers } from "../../integrations/misc/extract-text"
+import { countFileLines } from "../line-counter"
+import { readLines } from "../read-lines"
+import { extractTextFromFile, addLineNumbers } from "../extract-text"
 
 // Mock the required functions
-jest.mock("../../integrations/misc/line-counter")
-jest.mock("../../integrations/misc/read-lines")
-jest.mock("../../integrations/misc/extract-text")
+jest.mock("../line-counter")
+jest.mock("../read-lines")
+jest.mock("../extract-text")
 
 describe("read_file tool with maxReadFileLine setting", () => {
 	// Mock original implementation first to use in tests
-	const originalCountFileLines = jest.requireActual("../../integrations/misc/line-counter").countFileLines
-	const originalReadLines = jest.requireActual("../../integrations/misc/read-lines").readLines
-	const originalExtractTextFromFile = jest.requireActual("../../integrations/misc/extract-text").extractTextFromFile
-	const originalAddLineNumbers = jest.requireActual("../../integrations/misc/extract-text").addLineNumbers
+	const originalCountFileLines = jest.requireActual("../line-counter").countFileLines
+	const originalReadLines = jest.requireActual("../read-lines").readLines
+	const originalExtractTextFromFile = jest.requireActual("../extract-text").extractTextFromFile
+	const originalAddLineNumbers = jest.requireActual("../extract-text").addLineNumbers
 
 	beforeEach(() => {
 		jest.resetAllMocks()