Move executeCommand out of Cline and add telemetry for shell integration errors (#2771)

Author: cte

.changeset/four-trainers-move.md

@@ -0,0 +1,5 @@
+---
+"roo-cline": patch
+---
+
+Move executeCommand out of Cline and add telemetry for shell integration errors

src/core/Cline.ts

@@ -39,7 +39,7 @@ import { GlobalFileNames } from "../shared/globalFileNames"
 import { defaultModeSlug, getModeBySlug, getFullModeDetails, isToolAllowedForMode } from "../shared/modes"
 import { EXPERIMENT_IDS, experiments as Experiments, ExperimentId } from "../shared/experiments"
 import { formatLanguage } from "../shared/language"
-import { ToolParamName, ToolResponse } from "../shared/tools"
+import { ToolParamName, ToolResponse, DiffStrategy } from "../shared/tools"
 
 // services
 import { UrlContentFetcher } from "../services/browser/UrlContentFetcher"
@@ -52,7 +52,6 @@ import { CheckpointServiceOptions, RepoPerTaskCheckpointService } from "../servi
 // integrations
 import { DIFF_VIEW_URI_SCHEME, DiffViewProvider } from "../integrations/editor/DiffViewProvider"
 import { findToolName, formatContentBlockToMarkdown } from "../integrations/misc/export-markdown"
-import { ExitCodeDetails, TerminalProcess } from "../integrations/terminal/TerminalProcess"
 import { Terminal } from "../integrations/terminal/Terminal"
 import { TerminalRegistry } from "../integrations/terminal/TerminalRegistry"
 
@@ -92,8 +91,8 @@ import { RooIgnoreController } from "./ignore/RooIgnoreController"
 import { type AssistantMessageContent, parseAssistantMessage } from "./assistant-message"
 import { truncateConversationIfNeeded } from "./sliding-window"
 import { ClineProvider } from "./webview/ClineProvider"
-import { DiffStrategy, getDiffStrategy } from "./diff/DiffStrategy"
 import { validateToolUse } from "./mode-validator"
+import { MultiSearchReplaceDiffStrategy } from "./diff/strategies/multi-search-replace"
 
 type UserContent = Array<Anthropic.Messages.ContentBlockParam>
 
@@ -247,8 +246,7 @@ export class Cline extends EventEmitter<ClineEvents> {
 			telemetryService.captureTaskCreated(this.taskId)
 		}
 
-		// Initialize diffStrategy based on current state.
-		this.updateDiffStrategy(experiments ?? {})
+		this.diffStrategy = new MultiSearchReplaceDiffStrategy(this.fuzzyMatchThreshold)
 
 		onCreated?.(this)
 
@@ -283,15 +281,6 @@ export class Cline extends EventEmitter<ClineEvents> {
 		return getWorkspacePath(path.join(os.homedir(), "Desktop"))
 	}
 
-	// Add method to update diffStrategy.
-	async updateDiffStrategy(experiments: Partial<Record<ExperimentId, boolean>>) {
-		this.diffStrategy = getDiffStrategy({
-			model: this.api.getModel().id,
-			experiments,
-			fuzzyMatchThreshold: this.fuzzyMatchThreshold,
-		})
-	}
-
 	// Storing task to disk for history
 
 	private async ensureTaskDirectoryExists(): Promise<string> {
@@ -308,9 +297,11 @@ export class Cline extends EventEmitter<ClineEvents> {
 	private async getSavedApiConversationHistory(): Promise<Anthropic.MessageParam[]> {
 		const filePath = path.join(await this.ensureTaskDirectoryExists(), GlobalFileNames.apiConversationHistory)
 		const fileExists = await fileExistsAtPath(filePath)
+
 		if (fileExists) {
 			return JSON.parse(await fs.readFile(filePath, "utf8"))
 		}
+
 		return []
 	}
 
@@ -378,7 +369,8 @@ export class Cline extends EventEmitter<ClineEvents> {
 			const tokenUsage = this.getTokenUsage()
 			this.emit("taskTokenUsageUpdated", this.taskId, tokenUsage)
 
-			const taskMessage = this.clineMessages[0] // first message is always the task say
+			const taskMessage = this.clineMessages[0] // First message is always the task say
+
 			const lastRelevantMessage =
 				this.clineMessages[
 					findLastIndex(
@@ -913,11 +905,6 @@ export class Cline extends EventEmitter<ClineEvents> {
 	}
 
 	async abortTask(isAbandoned = false) {
-		// if (this.abort) {
-		// 	console.log(`[subtasks] already aborted task ${this.taskId}.${this.instanceId}`)
-		// 	return
-		// }
-
 		console.log(`[subtasks] aborting task ${this.taskId}.${this.instanceId}`)
 
 		// Will stop any autonomously running promises.
@@ -951,159 +938,6 @@ export class Cline extends EventEmitter<ClineEvents> {
 
 	// Tools
 
-	async executeCommandTool(command: string, customCwd?: string): Promise<[boolean, ToolResponse]> {
-		let workingDir: string
-		if (!customCwd) {
-			workingDir = this.cwd
-		} else if (path.isAbsolute(customCwd)) {
-			workingDir = customCwd
-		} else {
-			workingDir = path.resolve(this.cwd, customCwd)
-		}
-
-		// Check if directory exists
-		try {
-			await fs.access(workingDir)
-		} catch (error) {
-			return [false, `Working directory '${workingDir}' does not exist.`]
-		}
-
-		const terminalInfo = await TerminalRegistry.getOrCreateTerminal(workingDir, !!customCwd, this.taskId)
-
-		// Update the working directory in case the terminal we asked for has
-		// a different working directory so that the model will know where the
-		// command actually executed:
-		workingDir = terminalInfo.getCurrentWorkingDirectory()
-
-		const workingDirInfo = workingDir ? ` from '${workingDir.toPosix()}'` : ""
-		terminalInfo.terminal.show() // weird visual bug when creating new terminals (even manually) where there's an empty space at the top.
-		let userFeedback: { text?: string; images?: string[] } | undefined
-		let didContinue = false
-		let completed = false
-		let result: string = ""
-		let exitDetails: ExitCodeDetails | undefined
-		const { terminalOutputLineLimit = 500 } = (await this.providerRef.deref()?.getState()) ?? {}
-
-		const sendCommandOutput = async (line: string, terminalProcess: TerminalProcess): Promise<void> => {
-			try {
-				const { response, text, images } = await this.ask("command_output", line)
-				if (response === "yesButtonClicked") {
-					// proceed while running
-				} else {
-					userFeedback = { text, images }
-				}
-				didContinue = true
-				terminalProcess.continue() // continue past the await
-			} catch {
-				// This can only happen if this ask promise was ignored, so ignore this error
-			}
-		}
-
-		const process = terminalInfo.runCommand(command, {
-			onLine: (line, process) => {
-				if (!didContinue) {
-					sendCommandOutput(Terminal.compressTerminalOutput(line, terminalOutputLineLimit), process)
-				} else {
-					this.say("command_output", Terminal.compressTerminalOutput(line, terminalOutputLineLimit))
-				}
-			},
-			onCompleted: (output) => {
-				result = output ?? ""
-				completed = true
-			},
-			onShellExecutionComplete: (details) => {
-				exitDetails = details
-			},
-			onNoShellIntegration: async (message) => {
-				await this.say("shell_integration_warning", message)
-			},
-		})
-
-		await process
-
-		// Wait for a short delay to ensure all messages are sent to the webview
-		// This delay allows time for non-awaited promises to be created and
-		// for their associated messages to be sent to the webview, maintaining
-		// the correct order of messages (although the webview is smart about
-		// grouping command_output messages despite any gaps anyways)
-		await delay(50)
-
-		result = Terminal.compressTerminalOutput(result, terminalOutputLineLimit)
-
-		// keep in case we need it to troubleshoot user issues, but this should be removed in the future
-		// if everything looks good:
-		console.debug(
-			"[execute_command status]",
-			JSON.stringify(
-				{
-					completed,
-					userFeedback,
-					hasResult: result.length > 0,
-					exitDetails,
-					terminalId: terminalInfo.id,
-					workingDir: workingDirInfo,
-					isTerminalBusy: terminalInfo.busy,
-				},
-				null,
-				2,
-			),
-		)
-
-		if (userFeedback) {
-			await this.say("user_feedback", userFeedback.text, userFeedback.images)
-			return [
-				true,
-				formatResponse.toolResult(
-					`Command is still running in terminal ${terminalInfo.id}${workingDirInfo}.${
-						result.length > 0 ? `\nHere's the output so far:\n${result}` : ""
-					}\n\nThe user provided the following feedback:\n<feedback>\n${userFeedback.text}\n</feedback>`,
-					userFeedback.images,
-				),
-			]
-		} else if (completed) {
-			let exitStatus: string = ""
-			if (exitDetails !== undefined) {
-				if (exitDetails.signal) {
-					exitStatus = `Process terminated by signal ${exitDetails.signal} (${exitDetails.signalName})`
-					if (exitDetails.coreDumpPossible) {
-						exitStatus += " - core dump possible"
-					}
-				} else if (exitDetails.exitCode === undefined) {
-					result += "<VSCE exit code is undefined: terminal output and command execution status is unknown.>"
-					exitStatus = `Exit code: <undefined, notify user>`
-				} else {
-					if (exitDetails.exitCode !== 0) {
-						exitStatus += "Command execution was not successful, inspect the cause and adjust as needed.\n"
-					}
-					exitStatus += `Exit code: ${exitDetails.exitCode}`
-				}
-			} else {
-				result += "<VSCE exitDetails == undefined: terminal output and command execution status is unknown.>"
-				exitStatus = `Exit code: <undefined, notify user>`
-			}
-
-			let workingDirInfo: string = workingDir ? ` within working directory '${workingDir.toPosix()}'` : ""
-			const newWorkingDir = terminalInfo.getCurrentWorkingDirectory()
-
-			if (newWorkingDir !== workingDir) {
-				workingDirInfo += `\nNOTICE: Your command changed the working directory for this terminal to '${newWorkingDir.toPosix()}' so you MUST adjust future commands accordingly because they will be executed in this directory`
-			}
-
-			const outputInfo = `\nOutput:\n${result}`
-			return [
-				false,
-				`Command executed in terminal ${terminalInfo.id}${workingDirInfo}. ${exitStatus}${outputInfo}`,
-			]
-		} else {
-			return [
-				false,
-				`Command is still running in terminal ${terminalInfo.id}${workingDirInfo}.${
-					result.length > 0 ? `\nHere's the output so far:\n${result}` : ""
-				}\n\nYou will be updated on the terminal status and new output in the future.`,
-			]
-		}
-	}
-
 	async *attemptApiRequest(previousApiReqIndex: number, retryAttempt: number = 0): ApiStream {
 		let mcpHub: McpHub | undefined
 
@@ -1566,6 +1400,7 @@ export class Cline extends EventEmitter<ClineEvents> {
 				}
 
 				if (!block.partial) {
+					this.recordToolUsage(block.name)
 					telemetryService.captureToolUsage(this.taskId, block.name)
 				}
 
@@ -2699,16 +2534,19 @@ export class Cline extends EventEmitter<ClineEvents> {
 		return getApiMetrics(combineApiRequests(combineCommandSequences(this.clineMessages.slice(1))))
 	}
 
-	public recordToolUsage({ toolName, success = true }: { toolName: ToolName; success?: boolean }) {
+	public recordToolUsage(toolName: ToolName) {
 		if (!this.toolUsage[toolName]) {
 			this.toolUsage[toolName] = { attempts: 0, failures: 0 }
 		}
 
 		this.toolUsage[toolName].attempts++
-
-		if (!success) {
-			this.toolUsage[toolName].failures++
+	}
+	public recordToolError(toolName: ToolName) {
+		if (!this.toolUsage[toolName]) {
+			this.toolUsage[toolName] = { attempts: 0, failures: 0 }
 		}
+
+		this.toolUsage[toolName].failures++
 	}
 
 	public getToolUsage() {

src/core/__tests__/Cline.test.ts

@@ -17,12 +17,12 @@ jest.mock("../ignore/RooIgnoreController")
 
 // Mock storagePathManager to prevent dynamic import issues
 jest.mock("../../shared/storagePathManager", () => ({
-	getTaskDirectoryPath: jest.fn().mockImplementation((globalStoragePath, taskId) => {
-		return Promise.resolve(`${globalStoragePath}/tasks/${taskId}`)
-	}),
-	getSettingsDirectoryPath: jest.fn().mockImplementation((globalStoragePath) => {
-		return Promise.resolve(`${globalStoragePath}/settings`)
-	}),
+	getTaskDirectoryPath: jest
+		.fn()
+		.mockImplementation((globalStoragePath, taskId) => Promise.resolve(`${globalStoragePath}/tasks/${taskId}`)),
+	getSettingsDirectoryPath: jest
+		.fn()
+		.mockImplementation((globalStoragePath) => Promise.resolve(`${globalStoragePath}/settings`)),
 }))
 
 // Mock fileExistsAtPath
@@ -298,50 +298,6 @@ describe("Cline", () => {
 			expect(cline.diffStrategy).toBeDefined()
 		})
 
-		it("should use provided fuzzy match threshold", async () => {
-			const getDiffStrategySpy = jest.spyOn(require("../diff/DiffStrategy"), "getDiffStrategy")
-
-			const cline = new Cline({
-				provider: mockProvider,
-				apiConfiguration: mockApiConfig,
-				customInstructions: "custom instructions",
-				enableDiff: true,
-				fuzzyMatchThreshold: 0.9,
-				task: "test task",
-				startTask: false,
-			})
-
-			expect(cline.diffEnabled).toBe(true)
-			expect(cline.diffStrategy).toBeDefined()
-
-			expect(getDiffStrategySpy).toHaveBeenCalledWith({
-				model: "claude-3-5-sonnet-20241022",
-				experiments: {},
-				fuzzyMatchThreshold: 0.9,
-			})
-		})
-
-		it("should pass default threshold to diff strategy when not provided", async () => {
-			const getDiffStrategySpy = jest.spyOn(require("../diff/DiffStrategy"), "getDiffStrategy")
-
-			const cline = new Cline({
-				provider: mockProvider,
-				apiConfiguration: mockApiConfig,
-				customInstructions: "custom instructions",
-				enableDiff: true,
-				task: "test task",
-				startTask: false,
-			})
-
-			expect(cline.diffEnabled).toBe(true)
-			expect(cline.diffStrategy).toBeDefined()
-			expect(getDiffStrategySpy).toHaveBeenCalledWith({
-				model: "claude-3-5-sonnet-20241022",
-				experiments: {},
-				fuzzyMatchThreshold: 1.0,
-			})
-		})
-
 		it("should require either task or historyItem", () => {
 			expect(() => {
 				new Cline({ provider: mockProvider, apiConfiguration: mockApiConfig })

src/core/__tests__/read-file-maxReadFileLine.test.ts

@@ -127,8 +127,8 @@ describe("read_file tool with maxReadFileLine setting", () => {
 		mockCline.getFileContextTracker = jest.fn().mockReturnValue({
 			trackFileContext: jest.fn().mockResolvedValue(undefined),
 		})
-		mockCline.recordToolUsage = jest.fn().mockReturnValue({} as ToolUsage)
-
+		mockCline.recordToolUsage = jest.fn().mockReturnValue(undefined)
+		mockCline.recordToolError = jest.fn().mockReturnValue(undefined)
 		// Reset tool result
 		toolResult = undefined
 	})

src/core/__tests__/read-file-xml.test.ts

@@ -121,7 +121,8 @@ describe("read_file tool XML output structure", () => {
 		mockCline.getFileContextTracker = jest.fn().mockReturnValue({
 			trackFileContext: jest.fn().mockResolvedValue(undefined),
 		})
-		mockCline.recordToolUsage = jest.fn().mockReturnValue({} as ToolUsage)
+		mockCline.recordToolUsage = jest.fn().mockReturnValue(undefined)
+		mockCline.recordToolError = jest.fn().mockReturnValue(undefined)
 
 		// Reset tool result
 		toolResult = undefined