mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-07 08:26:51 +00:00
fix(tools): prevent EACCES errors from absolute paths in tool calls (#11208)
This commit is contained in:
randomizedcoder dave.seddon.ca@gmail.com
parent
b0dc6ae918
commit
bdae65b27b
13 changed files with 808 additions and 79 deletions
|
|
@ -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": {
|
||||
|
|
|
|||
64
pnpm-lock.yaml
generated
64
pnpm-lock.yaml
generated
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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<TName extends ToolName> {
|
|||
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.
|
||||
*/
|
||||
protected lastSeenPartialPath: string | undefined = undefined
|
||||
private static partialStateByTask = new WeakMap<Task, PartialExecutionState>()
|
||||
|
||||
/**
|
||||
* Get partial state for a task, initializing if needed.
|
||||
*/
|
||||
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<TName extends ToolName> {
|
|||
*
|
||||
* 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(): void {
|
||||
this.lastSeenPartialPath = undefined
|
||||
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
|
||||
*/
|
||||
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<TName extends ToolName> {
|
|||
async handle(task: Task, block: ToolUse<TName>, callbacks: ToolCallbacks): Promise<void> {
|
||||
// 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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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<void> {
|
||||
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<void> {
|
||||
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(
|
||||
|
|
|
|||
308
src/core/tools/__tests__/BaseTool.spec.ts
Normal file
308
src/core/tools/__tests__/BaseTool.spec.ts
Normal file
|
|
@ -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<void> {
|
||||
this.executeCallCount++
|
||||
}
|
||||
|
||||
override async handlePartial(task: Task, block: ToolUse<"read_file">): Promise<void> {
|
||||
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<typeof vi.spyOn>
|
||||
|
||||
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<typeof vi.spyOn>
|
||||
|
||||
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)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
@ -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()
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -11,7 +11,7 @@
|
|||
},
|
||||
"engines": {
|
||||
"vscode": "^1.84.0",
|
||||
"node": "20.19.2"
|
||||
"node": "20.20.0"
|
||||
},
|
||||
"author": {
|
||||
"name": "Roo Code"
|
||||
|
|
|
|||
184
src/utils/__tests__/pathUtils.spec.ts
Normal file
184
src/utils/__tests__/pathUtils.spec.ts
Normal file
|
|
@ -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)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue