diff --git a/src/core/tools/BaseTool.ts b/src/core/tools/BaseTool.ts index e18c3593e43..5dcae1ecd59 100644 --- a/src/core/tools/BaseTool.ts +++ b/src/core/tools/BaseTool.ts @@ -135,21 +135,30 @@ export abstract class BaseTool { * value between consecutive handlePartial() calls and returns true only when the * path has stopped changing (stabilized). * - * Usage in handlePartial(): - * ```typescript - * if (!this.hasPathStabilized(block.params.path)) { - * return // Path still changing, wait for it to stabilize - * } - * // Path is stable, proceed with UI updates - * ``` + * A path is considered stable ONLY when the same non-empty value is seen twice + * consecutively. This is critical for safety because tools like WriteToFileTool + * perform file operations (createDirectoriesForFile, diffViewProvider.open) after + * the path stabilizes. Accepting the first non-empty value after undefined would + * cause file operations on truncated paths for incremental streaming providers. + * + * For Gemini-style providers that send complete args in one chunk, the path will + * stabilize when it appears twice (e.g., in subsequent partial events). The UI + * may briefly show nothing or empty state, but this is safer than file operations + * on truncated paths. The getReadablePath() function returns empty string for + * undefined/empty paths to prevent showing CWD basename during this brief period. * * @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 + const previousPath = this.lastSeenPartialPath this.lastSeenPartialPath = path - return pathHasStabilized && !!path + + // Path is stable only when the same non-empty value is seen twice consecutively. + // This prevents file operations on truncated paths from incremental streaming. + const pathHasStabilized = previousPath !== undefined && previousPath === path && !!path + + return pathHasStabilized } /** diff --git a/src/core/tools/__tests__/BaseTool.spec.ts b/src/core/tools/__tests__/BaseTool.spec.ts new file mode 100644 index 00000000000..7d21115854d --- /dev/null +++ b/src/core/tools/__tests__/BaseTool.spec.ts @@ -0,0 +1,232 @@ +import { describe, it, expect, beforeEach } from "vitest" +import { BaseTool, ToolCallbacks } from "../BaseTool" +import type { ToolUse } from "../../../shared/tools" +import type { Task } from "../../task/Task" + +/** + * Concrete implementation of BaseTool for testing purposes. + * Exposes protected methods for testing. + */ +class TestTool extends BaseTool<"edit_file"> { + readonly name = "edit_file" as const + + parseLegacy(params: Partial>) { + return { + file_path: params.file_path || "", + old_string: params.old_string || "", + new_string: params.new_string || "", + } + } + + async execute(params: any, task: Task, callbacks: ToolCallbacks): Promise { + // No-op for testing + } + + // Expose protected methods for testing + public testHasPathStabilized(path: string | undefined): boolean { + return this.hasPathStabilized(path) + } + + public testResetPartialState(): void { + this.resetPartialState() + } +} + +describe("BaseTool", () => { + describe("hasPathStabilized", () => { + let tool: TestTool + + beforeEach(() => { + tool = new TestTool() + }) + + describe("path stabilization requires same value twice (safe for file operations)", () => { + it("should return false on first valid path (not yet stable)", () => { + // First call with undefined + const result1 = tool.testHasPathStabilized(undefined) + expect(result1).toBe(false) // No path yet + + // Second call with valid path - NOT stable yet (need same value twice) + const result2 = tool.testHasPathStabilized("src/file.ts") + expect(result2).toBe(false) // First time seeing this path + }) + + it("should return true when same path is seen twice consecutively", () => { + tool.testHasPathStabilized(undefined) + tool.testHasPathStabilized("src/file.ts") // First time - not stable + + // Same path seen again - NOW stable + const result = tool.testHasPathStabilized("src/file.ts") + expect(result).toBe(true) // Same value twice = stable + }) + + it("should handle empty string as falsy (not a valid path)", () => { + const result1 = tool.testHasPathStabilized(undefined) + expect(result1).toBe(false) + + const result2 = tool.testHasPathStabilized("") + expect(result2).toBe(false) // Empty string is not a valid path + }) + + it("should return false when path changes (not stable)", () => { + tool.testHasPathStabilized("src/file.ts") + tool.testHasPathStabilized("src/file.ts") // Stable + + // Path changes + const result = tool.testHasPathStabilized("src/other.ts") + expect(result).toBe(false) // Different path = not stable + }) + }) + + describe("with incremental streaming providers (char-by-char)", () => { + it("should only return true when path stops changing", () => { + // Simulate incremental streaming where path grows + tool.testHasPathStabilized(undefined) // Initial state + expect(tool.testHasPathStabilized("s")).toBe(false) // First char - not stable + expect(tool.testHasPathStabilized("sr")).toBe(false) // Growing - not stable + expect(tool.testHasPathStabilized("src")).toBe(false) // Growing - not stable + expect(tool.testHasPathStabilized("src/")).toBe(false) // Growing - not stable + expect(tool.testHasPathStabilized("src/file")).toBe(false) // Growing - not stable + expect(tool.testHasPathStabilized("src/file.ts")).toBe(false) // Complete but first time + + // Path repeats when streaming moves past the path field + const result = tool.testHasPathStabilized("src/file.ts") + expect(result).toBe(true) // Same value twice = stable + }) + + it("should NOT return true on first valid path after undefined (prevents truncated paths)", () => { + // This is the critical safety behavior - we do NOT accept first valid after undefined + // because it could be a truncated path for incremental streaming providers + tool.testHasPathStabilized(undefined) + + const result = tool.testHasPathStabilized("s") + expect(result).toBe(false) // First valid after undefined - NOT stable (could be truncated) + + // Still not stable as path keeps changing + expect(tool.testHasPathStabilized("sr")).toBe(false) + expect(tool.testHasPathStabilized("src")).toBe(false) + + // Eventually stabilizes when same value seen twice + expect(tool.testHasPathStabilized("src/file.ts")).toBe(false) + expect(tool.testHasPathStabilized("src/file.ts")).toBe(true) // Now stable + }) + }) + + describe("with Gemini-style providers (complete args in one chunk)", () => { + it("should stabilize when path appears twice", () => { + // Gemini sends name first (no args), then all args at once + // First partial has undefined path + const result1 = tool.testHasPathStabilized(undefined) + expect(result1).toBe(false) // No path yet + + // Second partial has the complete path - but need to see it twice + const result2 = tool.testHasPathStabilized("src/file.ts") + expect(result2).toBe(false) // First time seeing path + + // Third call with same path - NOW stable + const result3 = tool.testHasPathStabilized("src/file.ts") + expect(result3).toBe(true) // Same value twice = stable + }) + }) + + describe("state management", () => { + it("should reset state with resetPartialState", () => { + // Build up some state + tool.testHasPathStabilized("src/file.ts") + tool.testHasPathStabilized("src/file.ts") + + // Reset + tool.testResetPartialState() + + // After reset, need to see path twice again + const result1 = tool.testHasPathStabilized(undefined) + expect(result1).toBe(false) + + const result2 = tool.testHasPathStabilized("new/path.ts") + expect(result2).toBe(false) // First time after reset + + const result3 = tool.testHasPathStabilized("new/path.ts") + expect(result3).toBe(true) // Same value twice + }) + + it("should handle state bleeding between tool calls (requires same value twice)", () => { + // Simulate: Tool A completes with a path + tool.testHasPathStabilized("old/path.ts") + tool.testHasPathStabilized("old/path.ts") // Stable + + // Simulate: Tool A is rejected, resetPartialState never called + // State is now "old/path.ts" + + // Simulate: Tool B starts (undefined first) + const result1 = tool.testHasPathStabilized(undefined) + expect(result1).toBe(false) // Clears stale state + + // Tool B's actual path - need to see twice + const result2 = tool.testHasPathStabilized("new/path.ts") + expect(result2).toBe(false) // First time + + const result3 = tool.testHasPathStabilized("new/path.ts") + expect(result3).toBe(true) // Same value twice + }) + + it("should handle incremental streaming after stale state", () => { + // Simulate: Tool A completes with a path + tool.testHasPathStabilized("old/path.ts") + tool.testHasPathStabilized("old/path.ts") + + // Simulate: Tool A is rejected, state is "old/path.ts" + + // Simulate: Tool B starts (incremental - undefined first) + tool.testHasPathStabilized(undefined) // Clears stale state + + // Tool B's path grows incrementally - none should be stable until same twice + expect(tool.testHasPathStabilized("n")).toBe(false) + expect(tool.testHasPathStabilized("ne")).toBe(false) + expect(tool.testHasPathStabilized("new")).toBe(false) + expect(tool.testHasPathStabilized("new/")).toBe(false) + expect(tool.testHasPathStabilized("new/path")).toBe(false) + expect(tool.testHasPathStabilized("new/path.ts")).toBe(false) + + // Eventually stabilizes + const result = tool.testHasPathStabilized("new/path.ts") + expect(result).toBe(true) // Same value twice + }) + }) + + describe("edge cases", () => { + it("should return false when path is undefined", () => { + expect(tool.testHasPathStabilized(undefined)).toBe(false) + }) + + it("should return false when path is empty string", () => { + tool.testHasPathStabilized(undefined) + expect(tool.testHasPathStabilized("")).toBe(false) + }) + + it("should return false on first call with valid path (fresh state)", () => { + // Fresh tool state means lastSeenPartialPath is undefined + // First valid path should NOT be stable (need same value twice) + const result = tool.testHasPathStabilized("src/file.ts") + expect(result).toBe(false) // Not stable - need same value twice + }) + + it("should return true when same valid path is provided twice starting from fresh state", () => { + // First call + tool.testHasPathStabilized("src/file.ts") + // Second call with same path + const result = tool.testHasPathStabilized("src/file.ts") + expect(result).toBe(true) // Same value twice = stable + }) + + it("should handle multiple sequential undefined values", () => { + expect(tool.testHasPathStabilized(undefined)).toBe(false) + expect(tool.testHasPathStabilized(undefined)).toBe(false) + expect(tool.testHasPathStabilized(undefined)).toBe(false) + + // Then valid path - still need twice + expect(tool.testHasPathStabilized("path.ts")).toBe(false) + expect(tool.testHasPathStabilized("path.ts")).toBe(true) + }) + }) + }) +}) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index fd791729b4d..89f43af5aaf 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -281,11 +281,15 @@ describe("writeToFileTool", () => { it.skipIf(process.platform === "win32")( "creates parent directories when path has stabilized (partial)", async () => { - // First call - path not yet stabilized + // First call with undefined path - no action yet + await executeWriteFileTool({ path: undefined }, { fileExists: false, isPartial: true }) + expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() + + // Second call with valid path - not yet stable (need same path twice) await executeWriteFileTool({}, { fileExists: false, isPartial: true }) expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() - // Second call with same path - path is now stabilized + // Third call with same valid path - NOW path is stable await executeWriteFileTool({}, { fileExists: false, isPartial: true }) expect(mockedCreateDirectoriesForFile).toHaveBeenCalledWith(absoluteFilePath) }, @@ -400,12 +404,17 @@ describe("writeToFileTool", () => { }) it("streams content updates during partial execution after path stabilizes", async () => { - // First call - path not yet stabilized, early return (no file operations) + // First call with undefined path - early return (no file operations) + await executeWriteFileTool({ path: undefined }, { isPartial: true }) + expect(mockCline.ask).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + + // Second call with valid path - not yet stable (need same path twice) await executeWriteFileTool({}, { isPartial: true }) expect(mockCline.ask).not.toHaveBeenCalled() expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() - // Second call with same path - path is now stabilized, file operations proceed + // Third call with same valid path - NOW path is stable, file operations proceed await executeWriteFileTool({}, { isPartial: true }) expect(mockCline.ask).toHaveBeenCalled() expect(mockCline.diffViewProvider.open).toHaveBeenCalledWith(testFilePath) @@ -455,11 +464,15 @@ describe("writeToFileTool", () => { it("handles partial streaming errors after path stabilizes", async () => { mockCline.diffViewProvider.open.mockRejectedValue(new Error("Open failed")) - // First call - path not yet stabilized, no error yet + // First call with undefined path - no error yet (no file operations attempted) + await executeWriteFileTool({ path: undefined }, { isPartial: true }) + expect(mockHandleError).not.toHaveBeenCalled() + + // Second call with valid path - not yet stable (no file operations attempted) await executeWriteFileTool({}, { isPartial: true }) expect(mockHandleError).not.toHaveBeenCalled() - // Second call with same path - path is now stabilized, error occurs + // Third call with same valid path - NOW path is stable, error occurs during file operations await executeWriteFileTool({}, { isPartial: true }) expect(mockHandleError).toHaveBeenCalledWith("handling partial write_to_file", expect.any(Error)) }) diff --git a/src/utils/__tests__/path.spec.ts b/src/utils/__tests__/path.spec.ts index a8cf84b68c9..d4efb79fe77 100644 --- a/src/utils/__tests__/path.spec.ts +++ b/src/utils/__tests__/path.spec.ts @@ -153,8 +153,12 @@ describe("Path Utilities", () => { expect(getReadablePath(desktop, filePath)).toBe(filePath.toPosix()) }) - it("should handle undefined relative path", () => { - expect(getReadablePath(cwd)).toBe("project") + it("should return empty string for undefined relative path", () => { + expect(getReadablePath(cwd)).toBe("") + }) + + it("should return empty string for empty string relative path", () => { + expect(getReadablePath(cwd, "")).toBe("") }) it("should handle parent directory traversal", () => { diff --git a/src/utils/path.ts b/src/utils/path.ts index 48e2ce66738..718921ba3f5 100644 --- a/src/utils/path.ts +++ b/src/utils/path.ts @@ -80,7 +80,10 @@ function normalizePath(p: string): string { } export function getReadablePath(cwd: string, relPath?: string): string { - relPath = relPath || "" + // Return empty string if relPath is undefined or empty (no path to show) + if (!relPath) { + return "" + } // path.resolve is flexible in that it will resolve relative paths like '../../' to the cwd and even ignore the cwd if the relPath is actually an absolute path const absolutePath = path.resolve(cwd, relPath) if (arePathsEqual(cwd, path.join(os.homedir(), "Desktop"))) {