diff --git a/package.json b/package.json index de8dff751cb..e3cd114633a 100644 --- a/package.json +++ b/package.json @@ -2,7 +2,7 @@ "name": "roo-code", "packageManager": "pnpm@10.8.1", "engines": { - "node": "20.19.2" + "node": "20.20.0" }, "scripts": { "preinstall": "node scripts/bootstrap.mjs", @@ -46,7 +46,7 @@ "prettier": "^3.4.2", "rimraf": "^6.0.1", "tsx": "^4.19.3", - "turbo": "^2.5.6", + "turbo": "^2.8.3", "typescript": "5.8.3" }, "lint-staged": { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 0a47ae416ae..20ef6bb7ef2 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -75,8 +75,8 @@ importers: specifier: ^4.19.3 version: 4.19.4 turbo: - specifier: ^2.5.6 - version: 2.5.6 + specifier: ^2.8.3 + version: 2.8.3 typescript: specifier: 5.8.3 version: 5.8.3 @@ -540,7 +540,7 @@ importers: version: 5.2.0(eslint@9.27.0(jiti@2.4.2)) eslint-plugin-turbo: specifier: ^2.4.4 - version: 2.5.6(eslint@9.27.0(jiti@2.4.2))(turbo@2.5.6) + version: 2.5.6(eslint@9.27.0(jiti@2.4.2))(turbo@2.8.3) globals: specifier: ^16.0.0 version: 16.1.0 @@ -10332,38 +10332,38 @@ packages: resolution: {integrity: sha512-1h/Lnq9yajKY2PEbBadPXj3VxsDDu844OnaAo52UVmIzIvwwtBPIuNvkjuzBlTWpfJyUbG3ez0KSBibQkj4ojg==} engines: {node: '>=0.6.11 <=0.7.0 || >=0.7.3'} - turbo-darwin-64@2.5.6: - resolution: {integrity: sha512-3C1xEdo4aFwMJAPvtlPqz1Sw/+cddWIOmsalHFMrsqqydcptwBfu26WW2cDm3u93bUzMbBJ8k3zNKFqxJ9ei2A==} + turbo-darwin-64@2.8.3: + resolution: {integrity: sha512-4kXRLfcygLOeNcP6JquqRLmGB/ATjjfehiojL2dJkL7GFm3SPSXbq7oNj8UbD8XriYQ5hPaSuz59iF1ijPHkTw==} cpu: [x64] os: [darwin] - turbo-darwin-arm64@2.5.6: - resolution: {integrity: sha512-LyiG+rD7JhMfYwLqB6k3LZQtYn8CQQUePbpA8mF/hMLPAekXdJo1g0bUPw8RZLwQXUIU/3BU7tXENvhSGz5DPA==} + turbo-darwin-arm64@2.8.3: + resolution: {integrity: sha512-xF7uCeC0UY0Hrv/tqax0BMbFlVP1J/aRyeGQPZT4NjvIPj8gSPDgFhfkfz06DhUwDg5NgMo04uiSkAWE8WB/QQ==} cpu: [arm64] os: [darwin] - turbo-linux-64@2.5.6: - resolution: {integrity: sha512-GOcUTT0xiT/pSnHL4YD6Yr3HreUhU8pUcGqcI2ksIF9b2/r/kRHwGFcsHgpG3+vtZF/kwsP0MV8FTlTObxsYIA==} + turbo-linux-64@2.8.3: + resolution: {integrity: sha512-vxMDXwaOjweW/4etY7BxrXCSkvtwh0PbwVafyfT1Ww659SedUxd5rM3V2ZCmbwG8NiCfY7d6VtxyHx3Wh1GoZA==} cpu: [x64] os: [linux] - turbo-linux-arm64@2.5.6: - resolution: {integrity: sha512-10Tm15bruJEA3m0V7iZcnQBpObGBcOgUcO+sY7/2vk1bweW34LMhkWi8svjV9iDF68+KJDThnYDlYE/bc7/zzQ==} + turbo-linux-arm64@2.8.3: + resolution: {integrity: sha512-mQX7uYBZFkuPLLlKaNe9IjR1JIef4YvY8f21xFocvttXvdPebnq3PK1Zjzl9A1zun2BEuWNUwQIL8lgvN9Pm3Q==} cpu: [arm64] os: [linux] - turbo-windows-64@2.5.6: - resolution: {integrity: sha512-FyRsVpgaj76It0ludwZsNN40ytHN+17E4PFJyeliBEbxrGTc5BexlXVpufB7XlAaoaZVxbS6KT8RofLfDRyEPg==} + turbo-windows-64@2.8.3: + resolution: {integrity: sha512-YLGEfppGxZj3VWcNOVa08h6ISsVKiG85aCAWosOKNUjb6yErWEuydv6/qImRJUI+tDLvDvW7BxopAkujRnWCrw==} cpu: [x64] os: [win32] - turbo-windows-arm64@2.5.6: - resolution: {integrity: sha512-j/tWu8cMeQ7HPpKri6jvKtyXg9K1gRyhdK4tKrrchH8GNHscPX/F71zax58yYtLRWTiK04zNzPcUJuoS0+v/+Q==} + turbo-windows-arm64@2.8.3: + resolution: {integrity: sha512-afTUGKBRmOJU1smQSBnFGcbq0iabAPwh1uXu2BVk7BREg30/1gMnJh9DFEQTah+UD3n3ru8V55J83RQNFfqoyw==} cpu: [arm64] os: [win32] - turbo@2.5.6: - resolution: {integrity: sha512-gxToHmi9oTBNB05UjUsrWf0OyN5ZXtD0apOarC1KIx232Vp3WimRNy3810QzeNSgyD5rsaIDXlxlbnOzlouo+w==} + turbo@2.8.3: + resolution: {integrity: sha512-8Osxz5Tu/Dw2kb31EAY+nhq/YZ3wzmQSmYa1nIArqxgCAldxv9TPlrAiaBUDVnKA4aiPn0OFBD1ACcpc5VFOAQ==} hasBin: true turndown@7.2.0: @@ -16947,11 +16947,11 @@ snapshots: string.prototype.matchall: 4.0.12 string.prototype.repeat: 1.0.0 - eslint-plugin-turbo@2.5.6(eslint@9.27.0(jiti@2.4.2))(turbo@2.5.6): + eslint-plugin-turbo@2.5.6(eslint@9.27.0(jiti@2.4.2))(turbo@2.8.3): dependencies: dotenv: 16.0.3 eslint: 9.27.0(jiti@2.4.2) - turbo: 2.5.6 + turbo: 2.8.3 eslint-scope@8.3.0: dependencies: @@ -21605,32 +21605,32 @@ snapshots: tunnel@0.0.6: {} - turbo-darwin-64@2.5.6: + turbo-darwin-64@2.8.3: optional: true - turbo-darwin-arm64@2.5.6: + turbo-darwin-arm64@2.8.3: optional: true - turbo-linux-64@2.5.6: + turbo-linux-64@2.8.3: optional: true - turbo-linux-arm64@2.5.6: + turbo-linux-arm64@2.8.3: optional: true - turbo-windows-64@2.5.6: + turbo-windows-64@2.8.3: optional: true - turbo-windows-arm64@2.5.6: + turbo-windows-arm64@2.8.3: optional: true - turbo@2.5.6: + turbo@2.8.3: optionalDependencies: - turbo-darwin-64: 2.5.6 - turbo-darwin-arm64: 2.5.6 - turbo-linux-64: 2.5.6 - turbo-linux-arm64: 2.5.6 - turbo-windows-64: 2.5.6 - turbo-windows-arm64: 2.5.6 + turbo-darwin-64: 2.8.3 + turbo-darwin-arm64: 2.8.3 + turbo-linux-64: 2.8.3 + turbo-linux-arm64: 2.8.3 + turbo-windows-64: 2.8.3 + turbo-windows-arm64: 2.8.3 turndown@7.2.0: dependencies: diff --git a/src/core/tools/ApplyDiffTool.ts b/src/core/tools/ApplyDiffTool.ts index 5ca7002ff2d..bc88fc9d206 100644 --- a/src/core/tools/ApplyDiffTool.ts +++ b/src/core/tools/ApplyDiffTool.ts @@ -252,7 +252,7 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { } await task.diffViewProvider.reset() - this.resetPartialState() + this.resetPartialState(task) // Process any queued messages after file edit completes task.processQueuedMessages() @@ -261,7 +261,7 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { } catch (error) { await handleError("applying diff", error as Error) await task.diffViewProvider.reset() - this.resetPartialState() + this.resetPartialState(task) task.processQueuedMessages() return } @@ -272,7 +272,7 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { const diffContent: string | undefined = block.params.diff // Wait for path to stabilize before showing UI (prevents truncated paths) - if (!this.hasPathStabilized(relPath)) { + if (!this.hasPathStabilized(task, relPath)) { return } diff --git a/src/core/tools/BaseTool.ts b/src/core/tools/BaseTool.ts index 7d574068a97..893135f2ccf 100644 --- a/src/core/tools/BaseTool.ts +++ b/src/core/tools/BaseTool.ts @@ -3,6 +3,29 @@ import type { ToolName } from "@roo-code/types" import { Task } from "../task/Task" import type { ToolUse, HandleError, PushToolResult, AskApproval, NativeToolArgs } from "../../shared/tools" +/** + * State tracking for partial (streaming) tool execution. + * + * Uses a discriminated union to prevent impossible states: + * - Can't have errorCount without firstErrorTime + * - Can't be in "erroring" state without error metadata + * - Reset is trivial: `{ status: "idle" }` + */ +type PartialExecutionState = + | { status: "idle" } + | { + status: "streaming" + lastSeenPath?: string + } + | { + status: "erroring" + errorCount: number + lastErrorMessage: string + firstErrorTime: number + lastSeenPath?: string + isPathValidationError?: boolean + } + /** * Callbacks passed to tool execution */ @@ -33,10 +56,30 @@ export abstract class BaseTool { abstract readonly name: TName /** - * Track the last seen path during streaming to detect when the path has stabilized. - * Used by hasPathStabilized() to prevent displaying truncated paths from partial-json parsing. + * Per-task partial execution state. + * WeakMap ensures no memory leak - state is GC'd when Task is GC'd. + * Static so state is shared across tool instances but isolated per task. + */ + private static partialStateByTask = new WeakMap() + + /** + * Get partial state for a task, initializing if needed. */ - protected lastSeenPartialPath: string | undefined = undefined + protected getPartialState(task: Task): PartialExecutionState { + let state = BaseTool.partialStateByTask.get(task) + if (!state) { + state = { status: "idle" } + BaseTool.partialStateByTask.set(task, state) + } + return state + } + + /** + * Set partial state for a task. + */ + protected setPartialState(task: Task, state: PartialExecutionState): void { + BaseTool.partialStateByTask.set(task, state) + } /** * Execute the tool with typed parameters. @@ -73,38 +116,111 @@ export abstract class BaseTool { * * Usage in handlePartial(): * ```typescript - * if (!this.hasPathStabilized(block.params.path)) { + * if (!this.hasPathStabilized(task, block.params.path)) { * return // Path still changing, wait for it to stabilize * } * // Path is stable, proceed with UI updates * ``` * + * @param task - Task instance for per-task state isolation * @param path - The current path value from the partial block * @returns true if path has stabilized (same value seen twice) and is non-empty, false otherwise */ - protected hasPathStabilized(path: string | undefined): boolean { - const pathHasStabilized = this.lastSeenPartialPath !== undefined && this.lastSeenPartialPath === path - this.lastSeenPartialPath = path + protected hasPathStabilized(task: Task, path: string | undefined): boolean { + const state = this.getPartialState(task) + const lastSeenPath = state.status !== "idle" ? state.lastSeenPath : undefined + const pathHasStabilized = lastSeenPath !== undefined && lastSeenPath === path + + // Update state with new path, preserving other state fields + if (state.status === "idle") { + this.setPartialState(task, { status: "streaming", lastSeenPath: path }) + } else if (state.status === "streaming") { + this.setPartialState(task, { ...state, lastSeenPath: path }) + } else { + // erroring state - preserve error info + this.setPartialState(task, { ...state, lastSeenPath: path }) + } + return pathHasStabilized && !!path } /** - * Reset the partial state tracking. + * Reset the partial state tracking for a task. * * Should be called at the end of execute() (both success and error paths) * to ensure clean state for the next tool invocation. + * + * @param task - Task instance to reset state for + */ + resetPartialState(task: Task): void { + this.setPartialState(task, { status: "idle" }) + } + + /** + * Notify about a partial execution error with suppression for repeated errors. + * + * This method implements error suppression to prevent log spam during streaming + * when the same error (e.g., invalid path) repeats on every chunk. + * + * Behavior: + * - First error: logs and returns true (caller should handle) + * - Subsequent identical errors: suppressed, returns false + * - Different error message: logs and returns true + * + * @param task - Task instance for per-task state isolation + * @param errorMessage - The error message to report + * @param isPathValidationError - Whether this is a path validation error (enables fast-path guard) + * @returns true if the error should be handled (first occurrence), false if suppressed */ - resetPartialState(): void { - this.lastSeenPartialPath = undefined + protected notifyPartialError(task: Task, errorMessage: string, isPathValidationError = false): boolean { + const state = this.getPartialState(task) + + if (state.status === "erroring" && state.lastErrorMessage === errorMessage) { + // Suppress repeated identical error + this.setPartialState(task, { + ...state, + errorCount: state.errorCount + 1, + }) + return false + } + + // First error or different error message - log and track + const now = Date.now() + this.setPartialState(task, { + status: "erroring", + errorCount: 1, + lastErrorMessage: errorMessage, + firstErrorTime: now, + lastSeenPath: state.status !== "idle" ? state.lastSeenPath : undefined, + isPathValidationError, + }) + + console.error(`[${this.name}] Partial error: ${errorMessage}`) + return true + } + + /** + * Check if we should skip expensive work due to a known path validation error. + * + * This is a fast-path optimization: once we know the path is invalid, skip + * filesystem operations and other expensive work on subsequent streaming chunks. + * + * @param task - Task instance for per-task state isolation + * @returns true if we should skip (path validation error is active), false otherwise + */ + protected shouldSkipDueToPathError(task: Task): boolean { + const state = this.getPartialState(task) + return state.status === "erroring" && state.isPathValidationError === true } /** * Main entry point for tool execution. * * Handles the complete flow: - * 1. Partial message handling (if partial) - * 2. Parameter parsing (nativeArgs only) - * 3. Core execution (execute) + * 1. Fast-path guard for known path validation errors (skip expensive work) + * 2. Partial message handling with error suppression (if partial) + * 3. Parameter parsing (nativeArgs only) + * 4. Core execution (execute) * * @param task - Task instance * @param block - ToolUse block from assistant message @@ -113,14 +229,25 @@ export abstract class BaseTool { async handle(task: Task, block: ToolUse, callbacks: ToolCallbacks): Promise { // Handle partial messages if (block.partial) { + // Fast-path guard: skip expensive work if we already know the path is invalid. + // This prevents repeated filesystem operations on every streaming chunk. + if (this.shouldSkipDueToPathError(task)) { + return + } + try { await this.handlePartial(task, block) } catch (error) { - console.error(`Error in handlePartial:`, error) - await callbacks.handleError( - `handling partial ${this.name}`, - error instanceof Error ? error : new Error(String(error)), - ) + const errorMessage = error instanceof Error ? error.message : String(error) + const isPathError = errorMessage.includes("Invalid path:") + + // Use error suppression: only handle first occurrence of each error + if (this.notifyPartialError(task, errorMessage, isPathError)) { + // First error - let the caller know (but don't spam handleError) + // Note: We log in notifyPartialError, so just return silently here + // The error will be properly reported when execute() runs + } + // Suppressed errors: silently skip } return } diff --git a/src/core/tools/EditFileTool.ts b/src/core/tools/EditFileTool.ts index 2495a372bc5..55609ef7bf8 100644 --- a/src/core/tools/EditFileTool.ts +++ b/src/core/tools/EditFileTool.ts @@ -466,7 +466,7 @@ export class EditFileTool extends BaseTool<"edit_file"> { // Record successful tool usage and cleanup task.recordToolUsage("edit_file") await task.diffViewProvider.reset() - this.resetPartialState() + this.resetPartialState(task) // Process any queued messages after file edit completes task.processQueuedMessages() @@ -480,7 +480,7 @@ export class EditFileTool extends BaseTool<"edit_file"> { } finally { this.didSendPartialToolAsk = false this.partialToolAskRelPath = undefined - this.resetPartialState() + this.resetPartialState(task) } } @@ -489,7 +489,7 @@ export class EditFileTool extends BaseTool<"edit_file"> { const oldString: string | undefined = block.params.old_string // Wait for path to stabilize before showing UI (prevents truncated paths) - if (!this.hasPathStabilized(filePath)) { + if (!this.hasPathStabilized(task, filePath)) { return } diff --git a/src/core/tools/SearchAndReplaceTool.ts b/src/core/tools/SearchAndReplaceTool.ts index 93c3b4533b7..70b3f41f9e0 100644 --- a/src/core/tools/SearchAndReplaceTool.ts +++ b/src/core/tools/SearchAndReplaceTool.ts @@ -243,14 +243,14 @@ export class SearchAndReplaceTool extends BaseTool<"search_and_replace"> { // Record successful tool usage and cleanup task.recordToolUsage("search_and_replace") await task.diffViewProvider.reset() - this.resetPartialState() + this.resetPartialState(task) // Process any queued messages after file edit completes task.processQueuedMessages() } catch (error) { await handleError("search and replace", error as Error) await task.diffViewProvider.reset() - this.resetPartialState() + this.resetPartialState(task) } } @@ -258,7 +258,7 @@ export class SearchAndReplaceTool extends BaseTool<"search_and_replace"> { const relPath: string | undefined = block.params.path // Wait for path to stabilize before showing UI (prevents truncated paths) - if (!this.hasPathStabilized(relPath)) { + if (!this.hasPathStabilized(task, relPath)) { return } diff --git a/src/core/tools/SearchReplaceTool.ts b/src/core/tools/SearchReplaceTool.ts index 2d8817364ff..9254b37f93f 100644 --- a/src/core/tools/SearchReplaceTool.ts +++ b/src/core/tools/SearchReplaceTool.ts @@ -228,14 +228,14 @@ export class SearchReplaceTool extends BaseTool<"search_replace"> { // Record successful tool usage and cleanup task.recordToolUsage("search_replace") await task.diffViewProvider.reset() - this.resetPartialState() + this.resetPartialState(task) // Process any queued messages after file edit completes task.processQueuedMessages() } catch (error) { await handleError("search and replace", error as Error) await task.diffViewProvider.reset() - this.resetPartialState() + this.resetPartialState(task) } } @@ -244,7 +244,7 @@ export class SearchReplaceTool extends BaseTool<"search_replace"> { const oldString: string | undefined = block.params.old_string // Wait for path to stabilize before showing UI (prevents truncated paths) - if (!this.hasPathStabilized(filePath)) { + if (!this.hasPathStabilized(task, filePath)) { return } diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index c8455ef3d97..5fa091a83fa 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -10,7 +10,7 @@ import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { fileExistsAtPath, createDirectoriesForFile } from "../../utils/fs" import { stripLineNumbers, everyLineHasLineNumbers } from "../../integrations/misc/extract-text" import { getReadablePath } from "../../utils/path" -import { isPathOutsideWorkspace } from "../../utils/pathUtils" +import { isPathOutsideWorkspace, normalizeToolPath } from "../../utils/pathUtils" import { unescapeHtmlEntities } from "../../utils/text-normalization" import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" import { convertNewFileToUnifiedDiff, computeDiffStats, sanitizeUnifiedDiff } from "../diff/stats" @@ -28,10 +28,9 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { async execute(params: WriteToFileParams, task: Task, callbacks: ToolCallbacks): Promise { const { pushToolResult, handleError, askApproval } = callbacks - const relPath = params.path let newContent = params.content - if (!relPath) { + if (!params.path) { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "path")) @@ -39,6 +38,17 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { return } + // Normalize path using shared helper (fixes #11208) + const { relPath, isValid, error } = normalizeToolPath(params.path, task.cwd) + + if (!isValid) { + task.consecutiveMistakeCount++ + task.recordToolError("write_to_file") + pushToolResult(formatResponse.toolError(`Invalid path: ${error}`)) + await task.diffViewProvider.reset() + return + } + if (newContent === undefined) { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") @@ -180,7 +190,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { pushToolResult(message) await task.diffViewProvider.reset() - this.resetPartialState() + this.resetPartialState(task) task.processQueuedMessages() @@ -188,17 +198,17 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { } catch (error) { await handleError("writing file", error as Error) await task.diffViewProvider.reset() - this.resetPartialState() + this.resetPartialState(task) return } } override async handlePartial(task: Task, block: ToolUse<"write_to_file">): Promise { - const relPath: string | undefined = block.params.path + const rawPath: string | undefined = block.params.path let newContent: string | undefined = block.params.content // Wait for path to stabilize before showing UI (prevents truncated paths) - if (!this.hasPathStabilized(relPath) || newContent === undefined) { + if (!this.hasPathStabilized(task, rawPath) || newContent === undefined) { return } @@ -213,9 +223,16 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { return } - // relPath is guaranteed non-null after hasPathStabilized + // Normalize path using shared helper (fixes #11208) + const { relPath, isValid, error } = normalizeToolPath(rawPath!, task.cwd) + + if (!isValid) { + // Invalid paths during streaming throw (will be suppressed after first error) + throw new Error(`Invalid path: ${error}`) + } + let fileExists: boolean - const absolutePath = path.resolve(task.cwd, relPath!) + const absolutePath = path.resolve(task.cwd, relPath) if (task.diffViewProvider.editType !== undefined) { fileExists = task.diffViewProvider.editType === "modify" @@ -230,12 +247,12 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { await createDirectoriesForFile(absolutePath) } - const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath!) || false + const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath) || false const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) const sharedMessageProps: ClineSayTool = { tool: fileExists ? "editedExistingFile" : "newFileCreated", - path: getReadablePath(task.cwd, relPath!), + path: getReadablePath(task.cwd, relPath), content: newContent || "", isOutsideWorkspace, isProtected: isWriteProtected, @@ -246,7 +263,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { if (newContent) { if (!task.diffViewProvider.isEditing) { - await task.diffViewProvider.open(relPath!) + await task.diffViewProvider.open(relPath) } await task.diffViewProvider.update( diff --git a/src/core/tools/__tests__/BaseTool.spec.ts b/src/core/tools/__tests__/BaseTool.spec.ts new file mode 100644 index 00000000000..45063425cb0 --- /dev/null +++ b/src/core/tools/__tests__/BaseTool.spec.ts @@ -0,0 +1,308 @@ +// npx vitest core/tools/__tests__/BaseTool.spec.ts + +import type { ToolName } from "@roo-code/types" + +import { BaseTool, ToolCallbacks } from "../BaseTool" +import type { ToolUse } from "../../../shared/tools" +import { Task } from "../../task/Task" + +// Create a concrete implementation for testing +class TestTool extends BaseTool<"read_file"> { + readonly name = "read_file" as const + public executeCallCount = 0 + public handlePartialCallCount = 0 + public shouldThrowInHandlePartial = false + public partialErrorMessage = "Test error" + + async execute(params: any, task: Task, callbacks: ToolCallbacks): Promise { + this.executeCallCount++ + } + + override async handlePartial(task: Task, block: ToolUse<"read_file">): Promise { + this.handlePartialCallCount++ + if (this.shouldThrowInHandlePartial) { + throw new Error(this.partialErrorMessage) + } + } + + // Expose protected methods for testing + public testGetPartialState(task: Task) { + return this.getPartialState(task) + } + + public testSetPartialState(task: Task, state: any) { + this.setPartialState(task, state) + } + + public testHasPathStabilized(task: Task, path: string | undefined) { + return this.hasPathStabilized(task, path) + } + + public testNotifyPartialError(task: Task, errorMessage: string, isPathValidationError = false) { + return this.notifyPartialError(task, errorMessage, isPathValidationError) + } + + public testShouldSkipDueToPathError(task: Task) { + return this.shouldSkipDueToPathError(task) + } +} + +// Mock Task +const createMockTask = (): Task => { + return { + id: "test-task-" + Math.random(), + } as unknown as Task +} + +// Mock ToolUse block +const createMockToolUse = (partial: boolean): ToolUse<"read_file"> => ({ + type: "tool_use", + name: "read_file", + params: { path: "/test/file.ts" }, + nativeArgs: { path: "/test/file.ts" }, + partial, +}) + +// Mock callbacks +const createMockCallbacks = (): ToolCallbacks => ({ + askApproval: vi.fn().mockResolvedValue(true), + handleError: vi.fn().mockResolvedValue(undefined), + pushToolResult: vi.fn(), +}) + +describe("BaseTool", () => { + let testTool: TestTool + let mockTask: Task + let mockCallbacks: ToolCallbacks + + beforeEach(() => { + vi.clearAllMocks() + testTool = new TestTool() + mockTask = createMockTask() + mockCallbacks = createMockCallbacks() + }) + + describe("partial state management", () => { + it("should initialize with idle state", () => { + const state = testTool.testGetPartialState(mockTask) + expect(state.status).toBe("idle") + }) + + it("should isolate state per task", () => { + const task1 = createMockTask() + const task2 = createMockTask() + + testTool.testSetPartialState(task1, { status: "streaming", lastSeenPath: "/path1" }) + testTool.testSetPartialState(task2, { status: "streaming", lastSeenPath: "/path2" }) + + const state1 = testTool.testGetPartialState(task1) + const state2 = testTool.testGetPartialState(task2) + + expect(state1.status).toBe("streaming") + expect((state1 as any).lastSeenPath).toBe("/path1") + expect(state2.status).toBe("streaming") + expect((state2 as any).lastSeenPath).toBe("/path2") + }) + + it("should reset state to idle", () => { + testTool.testSetPartialState(mockTask, { + status: "erroring", + errorCount: 5, + lastErrorMessage: "test", + firstErrorTime: Date.now(), + }) + + testTool.resetPartialState(mockTask) + + const state = testTool.testGetPartialState(mockTask) + expect(state.status).toBe("idle") + }) + }) + + describe("hasPathStabilized", () => { + it("should return false on first call", () => { + const result = testTool.testHasPathStabilized(mockTask, "/test/path.ts") + expect(result).toBe(false) + }) + + it("should return true when path matches previous call", () => { + testTool.testHasPathStabilized(mockTask, "/test/path.ts") + const result = testTool.testHasPathStabilized(mockTask, "/test/path.ts") + expect(result).toBe(true) + }) + + it("should return false when path changes", () => { + testTool.testHasPathStabilized(mockTask, "/test/path1.ts") + testTool.testHasPathStabilized(mockTask, "/test/path1.ts") // stabilized + const result = testTool.testHasPathStabilized(mockTask, "/test/path2.ts") // changed + expect(result).toBe(false) + }) + + it("should return false for undefined path", () => { + const result = testTool.testHasPathStabilized(mockTask, undefined) + expect(result).toBe(false) + }) + + it("should return false for empty path", () => { + testTool.testHasPathStabilized(mockTask, "") + const result = testTool.testHasPathStabilized(mockTask, "") + expect(result).toBe(false) // empty string is falsy + }) + + it("should transition from idle to streaming", () => { + testTool.testHasPathStabilized(mockTask, "/test/path.ts") + const state = testTool.testGetPartialState(mockTask) + expect(state.status).toBe("streaming") + }) + }) + + describe("notifyPartialError", () => { + let consoleErrorSpy: ReturnType + + beforeEach(() => { + consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + }) + + afterEach(() => { + consoleErrorSpy.mockRestore() + }) + + it("should return true and log on first error", () => { + const result = testTool.testNotifyPartialError(mockTask, "First error") + expect(result).toBe(true) + expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("First error")) + }) + + it("should return false and not log on repeated identical error", () => { + testTool.testNotifyPartialError(mockTask, "Same error") + consoleErrorSpy.mockClear() + + const result = testTool.testNotifyPartialError(mockTask, "Same error") + expect(result).toBe(false) + expect(consoleErrorSpy).not.toHaveBeenCalled() + }) + + it("should return true and log when error message changes", () => { + testTool.testNotifyPartialError(mockTask, "Error 1") + consoleErrorSpy.mockClear() + + const result = testTool.testNotifyPartialError(mockTask, "Error 2") + expect(result).toBe(true) + expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("Error 2")) + }) + + it("should track error count for repeated errors", () => { + testTool.testNotifyPartialError(mockTask, "Repeated error") + testTool.testNotifyPartialError(mockTask, "Repeated error") + testTool.testNotifyPartialError(mockTask, "Repeated error") + + const state = testTool.testGetPartialState(mockTask) + expect(state.status).toBe("erroring") + expect((state as any).errorCount).toBe(3) + }) + + it("should set isPathValidationError flag when specified", () => { + testTool.testNotifyPartialError(mockTask, "Invalid path: /outside", true) + + const state = testTool.testGetPartialState(mockTask) + expect(state.status).toBe("erroring") + expect((state as any).isPathValidationError).toBe(true) + }) + }) + + describe("shouldSkipDueToPathError", () => { + it("should return false when in idle state", () => { + const result = testTool.testShouldSkipDueToPathError(mockTask) + expect(result).toBe(false) + }) + + it("should return false when in streaming state", () => { + testTool.testSetPartialState(mockTask, { status: "streaming" }) + const result = testTool.testShouldSkipDueToPathError(mockTask) + expect(result).toBe(false) + }) + + it("should return false for non-path errors", () => { + testTool.testNotifyPartialError(mockTask, "Some other error", false) + const result = testTool.testShouldSkipDueToPathError(mockTask) + expect(result).toBe(false) + }) + + it("should return true for path validation errors", () => { + testTool.testNotifyPartialError(mockTask, "Invalid path: /outside", true) + const result = testTool.testShouldSkipDueToPathError(mockTask) + expect(result).toBe(true) + }) + }) + + describe("handle() - error suppression integration", () => { + let consoleErrorSpy: ReturnType + + beforeEach(() => { + consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + }) + + afterEach(() => { + consoleErrorSpy.mockRestore() + }) + + it("should call handlePartial for partial blocks", async () => { + const block = createMockToolUse(true) + + await testTool.handle(mockTask, block, mockCallbacks) + + expect(testTool.handlePartialCallCount).toBe(1) + expect(testTool.executeCallCount).toBe(0) + }) + + it("should call execute for complete blocks", async () => { + const block = createMockToolUse(false) + + await testTool.handle(mockTask, block, mockCallbacks) + + expect(testTool.handlePartialCallCount).toBe(0) + expect(testTool.executeCallCount).toBe(1) + }) + + it("should suppress repeated partial errors", async () => { + testTool.shouldThrowInHandlePartial = true + testTool.partialErrorMessage = "Repeated error" + const block = createMockToolUse(true) + + // First error - logged + await testTool.handle(mockTask, block, mockCallbacks) + expect(consoleErrorSpy).toHaveBeenCalledTimes(1) + + // Second identical error - suppressed + consoleErrorSpy.mockClear() + await testTool.handle(mockTask, block, mockCallbacks) + expect(consoleErrorSpy).not.toHaveBeenCalled() + + // handleError should NOT be called (error suppression) + expect(mockCallbacks.handleError).not.toHaveBeenCalled() + }) + + it("should skip handlePartial when path error is active (fast-path guard)", async () => { + // Simulate a path validation error + testTool.testNotifyPartialError(mockTask, "Invalid path: /outside", true) + const block = createMockToolUse(true) + + await testTool.handle(mockTask, block, mockCallbacks) + + // handlePartial should be skipped due to fast-path guard + expect(testTool.handlePartialCallCount).toBe(0) + }) + + it("should recover from error state on reset", async () => { + // Set up error state + testTool.testNotifyPartialError(mockTask, "Invalid path: /outside", true) + expect(testTool.testShouldSkipDueToPathError(mockTask)).toBe(true) + + // Reset state (simulating end of execute) + testTool.resetPartialState(mockTask) + + // Should no longer skip + expect(testTool.testShouldSkipDueToPathError(mockTask)).toBe(false) + }) + }) +}) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 6c63387ee10..b43c7058bfb 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -41,6 +41,10 @@ vi.mock("../../prompts/responses", () => ({ vi.mock("../../../utils/pathUtils", () => ({ isPathOutsideWorkspace: vi.fn().mockReturnValue(false), + normalizeToolPath: vi.fn().mockImplementation((rawPath: string, _cwd: string) => ({ + relPath: rawPath, + isValid: true, + })), })) vi.mock("../../../utils/path", () => ({ @@ -110,7 +114,9 @@ describe("writeToFileTool", () => { beforeEach(() => { vi.clearAllMocks() - writeToFileTool.resetPartialState() + // Reset partial state for clean test isolation + // Since mockCline is defined outside, reset it after mocks are cleared + writeToFileTool.resetPartialState(mockCline) mockedPathResolve.mockReturnValue(absoluteFilePath) mockedFileExistsAtPath.mockResolvedValue(false) @@ -453,15 +459,28 @@ describe("writeToFileTool", () => { }) it("handles partial streaming errors after path stabilizes", async () => { + // With error suppression, partial errors are logged but not passed to handleError + // The error will be properly reported when execute() runs with the final params + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) mockCline.diffViewProvider.open.mockRejectedValue(new Error("Open failed")) // First call - path not yet stabilized, no error yet await executeWriteFileTool({}, { isPartial: true }) expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).not.toHaveBeenCalled() // Second call with same path - path is now stabilized, error occurs + // Error is logged via notifyPartialError but NOT passed to handleError await executeWriteFileTool({}, { isPartial: true }) - expect(mockHandleError).toHaveBeenCalledWith("handling partial write_to_file", expect.any(Error)) + expect(mockHandleError).not.toHaveBeenCalled() // Error suppression: no handleError call + expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("Partial error")) + + // Third call with same error - suppressed (not logged again) + consoleErrorSpy.mockClear() + await executeWriteFileTool({}, { isPartial: true }) + expect(consoleErrorSpy).not.toHaveBeenCalled() // Duplicate suppressed + + consoleErrorSpy.mockRestore() }) }) }) diff --git a/src/package.json b/src/package.json index 042119134ba..ae98a480662 100644 --- a/src/package.json +++ b/src/package.json @@ -11,7 +11,7 @@ }, "engines": { "vscode": "^1.84.0", - "node": "20.19.2" + "node": "20.20.0" }, "author": { "name": "Roo Code" diff --git a/src/utils/__tests__/pathUtils.spec.ts b/src/utils/__tests__/pathUtils.spec.ts new file mode 100644 index 00000000000..9da83098b89 --- /dev/null +++ b/src/utils/__tests__/pathUtils.spec.ts @@ -0,0 +1,184 @@ +// npx vitest utils/__tests__/pathUtils.spec.ts + +import * as path from "path" + +import { normalizeToolPath, isPathOutsideWorkspace } from "../pathUtils" + +// Mock vscode module +vi.mock("vscode", () => ({ + workspace: { + workspaceFolders: [ + { + uri: { fsPath: "/workspace/project" }, + name: "project", + index: 0, + }, + ], + asRelativePath: vi.fn().mockImplementation((pathOrUri: string, includeWorkspaceFolder?: boolean) => { + // Simulate VS Code's asRelativePath behavior + const wsPath = "/workspace/project" + if (pathOrUri.startsWith(wsPath + "/") || pathOrUri.startsWith(wsPath + path.sep)) { + return pathOrUri.slice(wsPath.length + 1) + } + // Return unchanged if outside workspace + return pathOrUri + }), + }, +})) + +describe("pathUtils", () => { + describe("normalizeToolPath", () => { + const cwd = "/workspace/project" + + describe("valid paths", () => { + it("should accept simple relative paths", () => { + const result = normalizeToolPath("src/file.ts", cwd) + expect(result.isValid).toBe(true) + expect(result.relPath).toBe("src/file.ts") + expect(result.error).toBeUndefined() + }) + + it("should accept nested relative paths", () => { + const result = normalizeToolPath("src/components/Button.tsx", cwd) + expect(result.isValid).toBe(true) + expect(result.relPath).toBe("src/components/Button.tsx") + }) + + it("should accept paths with ./", () => { + const result = normalizeToolPath("./src/file.ts", cwd) + expect(result.isValid).toBe(true) + expect(result.relPath).toBe("src/file.ts") + }) + + it("should normalize redundant path segments", () => { + const result = normalizeToolPath("src/../src/file.ts", cwd) + expect(result.isValid).toBe(true) + expect(result.relPath).toBe("src/file.ts") + }) + + it("should accept absolute paths within workspace", () => { + const result = normalizeToolPath("/workspace/project/src/file.ts", cwd) + expect(result.isValid).toBe(true) + expect(result.relPath).toBe("src/file.ts") + }) + + it("should accept file in root of workspace", () => { + const result = normalizeToolPath("README.md", cwd) + expect(result.isValid).toBe(true) + expect(result.relPath).toBe("README.md") + }) + }) + + describe("invalid paths - directory traversal attacks", () => { + it("should reject paths starting with ../", () => { + const result = normalizeToolPath("../outside/file.ts", cwd) + expect(result.isValid).toBe(false) + expect(result.error).toContain("resolves outside the workspace") + }) + + it("should reject paths with multiple ../", () => { + const result = normalizeToolPath("../../etc/passwd", cwd) + expect(result.isValid).toBe(false) + expect(result.error).toContain("resolves outside the workspace") + }) + + it("should reject paths that traverse then back in", () => { + // src/../../outside resolves to ../outside + const result = normalizeToolPath("src/../../outside/file.ts", cwd) + expect(result.isValid).toBe(false) + expect(result.error).toContain("resolves outside the workspace") + }) + }) + + describe("invalid paths - absolute paths outside workspace", () => { + it("should reject absolute paths to root", () => { + const result = normalizeToolPath("/plans", cwd) + expect(result.isValid).toBe(false) + expect(result.error).toContain("resolves outside the workspace") + }) + + it("should reject absolute paths to /etc", () => { + const result = normalizeToolPath("/etc/passwd", cwd) + expect(result.isValid).toBe(false) + expect(result.error).toContain("resolves outside the workspace") + }) + + it("should reject absolute paths to sibling directories", () => { + const result = normalizeToolPath("/workspace/other-project/file.ts", cwd) + expect(result.isValid).toBe(false) + expect(result.error).toContain("resolves outside the workspace") + }) + + it("should reject paths to home directory", () => { + const result = normalizeToolPath("/home/user/.ssh/id_rsa", cwd) + expect(result.isValid).toBe(false) + expect(result.error).toContain("resolves outside the workspace") + }) + }) + + describe("edge cases", () => { + it("should handle empty path segments", () => { + const result = normalizeToolPath("src//file.ts", cwd) + expect(result.isValid).toBe(true) + // path.normalize handles this + expect(result.relPath).toBe("src/file.ts") + }) + + it("should handle paths with only dots", () => { + const result = normalizeToolPath(".", cwd) + expect(result.isValid).toBe(true) + expect(result.relPath).toBe(".") + }) + + it("should handle paths with spaces", () => { + const result = normalizeToolPath("src/my file.ts", cwd) + expect(result.isValid).toBe(true) + expect(result.relPath).toBe("src/my file.ts") + }) + + it("should handle paths with special characters", () => { + const result = normalizeToolPath("src/file-name_v2.test.ts", cwd) + expect(result.isValid).toBe(true) + expect(result.relPath).toBe("src/file-name_v2.test.ts") + }) + }) + + describe("the original issue #11208", () => { + it("should reject /plans path that caused the EACCES error", () => { + // This is the exact path from issue #11208 that caused: + // EACCES: permission denied, mkdir '/plans' + const result = normalizeToolPath("/plans", cwd) + expect(result.isValid).toBe(false) + expect(result.error).toContain("resolves outside the workspace") + }) + + it("should reject /plans/implementation.md", () => { + const result = normalizeToolPath("/plans/implementation.md", cwd) + expect(result.isValid).toBe(false) + expect(result.error).toContain("resolves outside the workspace") + }) + }) + }) + + describe("isPathOutsideWorkspace", () => { + it("should return false for paths inside workspace", () => { + expect(isPathOutsideWorkspace("/workspace/project/src/file.ts")).toBe(false) + }) + + it("should return false for workspace root", () => { + expect(isPathOutsideWorkspace("/workspace/project")).toBe(false) + }) + + it("should return true for paths outside workspace", () => { + expect(isPathOutsideWorkspace("/other/path/file.ts")).toBe(true) + }) + + it("should return true for absolute paths to root", () => { + expect(isPathOutsideWorkspace("/plans")).toBe(true) + }) + + it("should return true for sibling directories", () => { + expect(isPathOutsideWorkspace("/workspace/other-project")).toBe(true) + }) + }) +}) diff --git a/src/utils/pathUtils.ts b/src/utils/pathUtils.ts index dae300f8f38..675f6985505 100644 --- a/src/utils/pathUtils.ts +++ b/src/utils/pathUtils.ts @@ -1,6 +1,80 @@ import * as vscode from "vscode" import * as path from "path" +/** + * Result of path normalization for tool operations. + */ +export interface PathNormalizationResult { + /** The normalized relative path (if valid) */ + relPath: string + /** Whether the path is valid for tool operations */ + isValid: boolean + /** Error message if invalid */ + error?: string +} + +/** + * Normalize a tool path to workspace-relative, with security validation. + * + * This function: + * 1. Converts absolute paths to workspace-relative using VS Code's API + * 2. Validates the result is safe (no traversal, stays within workspace) + * 3. Returns a consistent result for both execute() and handlePartial() + * + * @param rawPath - The path from the LLM (may be absolute or relative) + * @param cwd - The task's current working directory (workspace folder) + * @returns Normalization result with validity status + */ +export function normalizeToolPath(rawPath: string, cwd: string): PathNormalizationResult { + let relPath = rawPath + + // Step 1: Convert absolute paths to workspace-relative + if (path.isAbsolute(rawPath)) { + // Use VS Code's API for multi-root workspace support and separator handling + relPath = vscode.workspace.asRelativePath(rawPath, false) + + // asRelativePath may return the absolute path unchanged if it's outside + // all workspace folders. Fall back to path.relative in that case. + if (path.isAbsolute(relPath)) { + relPath = path.relative(cwd, rawPath) + } + } + + // Step 2: Normalize to remove redundant segments (./foo/../bar -> bar) + relPath = path.normalize(relPath) + + // Step 3: Security validation - reject paths that escape workspace via ../ + if (relPath.startsWith("..") || relPath.startsWith(path.sep + "..")) { + return { + relPath, + isValid: false, + error: `Path "${rawPath}" resolves outside the workspace`, + } + } + + // Step 4: Reject paths that are still absolute after normalization + if (path.isAbsolute(relPath)) { + return { + relPath, + isValid: false, + error: `Path "${rawPath}" could not be resolved relative to workspace`, + } + } + + // Step 5: Final check - resolve and verify it's within cwd + const absoluteResolved = path.resolve(cwd, relPath) + const cwdNormalized = path.normalize(cwd) + if (!absoluteResolved.startsWith(cwdNormalized + path.sep) && absoluteResolved !== cwdNormalized) { + return { + relPath, + isValid: false, + error: `Path "${rawPath}" resolves outside the workspace`, + } + } + + return { relPath, isValid: true } +} + /** * Checks if a file path is outside all workspace folders * @param filePath The file path to check