From 41b65e75d479abed871893b8fff7dd22949d33c2 Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Fri, 9 Jan 2026 16:21:12 -0500 Subject: [PATCH 1/3] fix: Gemini edit_file showing CWD basename instead of file path during streaming MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes an issue where Gemini models displayed the CWD basename (e.g., 'extracker') instead of the actual file path (e.g., 'pkg/tm_tag/log_store.go') during edit_file streaming. Root cause: Gemini sends complete function calls in one chunk, but the code emits two tool_call_partial events. The original hasPathStabilized() required seeing the same path twice, but Gemini's pattern (undefined → actual_path) never matched. Solution: Modified hasPathStabilized() to recognize both streaming patterns: 1. Same non-empty value seen twice (incremental streaming - Claude/OpenAI) 2. First non-empty value after undefined (Gemini-style complete args) ROO-437 --- src/core/tools/BaseTool.ts | 33 +++- src/core/tools/__tests__/BaseTool.spec.ts | 187 ++++++++++++++++++ .../tools/__tests__/writeToFileTool.spec.ts | 18 +- 3 files changed, 224 insertions(+), 14 deletions(-) create mode 100644 src/core/tools/__tests__/BaseTool.spec.ts diff --git a/src/core/tools/BaseTool.ts b/src/core/tools/BaseTool.ts index e18c3593e43..58241f89164 100644 --- a/src/core/tools/BaseTool.ts +++ b/src/core/tools/BaseTool.ts @@ -132,8 +132,18 @@ export abstract class BaseTool { * * During native tool call streaming, the partial-json library may return truncated * string values when chunk boundaries fall mid-value. This method tracks the path - * value between consecutive handlePartial() calls and returns true only when the - * path has stopped changing (stabilized). + * value between consecutive handlePartial() calls and returns true when the path + * is ready to display. + * + * A path is considered stable when: + * 1. The same non-empty value is seen twice consecutively (handles incremental streaming + * where paths grow char-by-char until stabilizing) + * 2. A non-empty value appears after undefined (handles providers like Gemini that + * send complete args in one chunk - the first valid path is the complete path) + * + * For incremental streaming providers, option 2 may briefly show truncated paths, + * but these quickly update as more chunks arrive. This is preferable to showing + * nothing or showing incorrect paths (like CWD basename). * * Usage in handlePartial(): * ```typescript @@ -144,12 +154,25 @@ export abstract class BaseTool { * ``` * * @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 + * @returns true if path has stabilized 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 + + // Must have a valid path to consider it stable + if (!path) { + return false + } + + // Case 1: Same non-empty value seen twice (incremental streaming stabilized) + const sameValueTwice = previousPath !== undefined && previousPath === path + + // Case 2: First valid value after undefined (Gemini-style complete args) + // This handles providers that send undefined first (name only), then complete args + const firstValidAfterUndefined = previousPath === undefined + + return sameValueTwice || firstValidAfterUndefined } /** diff --git a/src/core/tools/__tests__/BaseTool.spec.ts b/src/core/tools/__tests__/BaseTool.spec.ts new file mode 100644 index 00000000000..98e567dec77 --- /dev/null +++ b/src/core/tools/__tests__/BaseTool.spec.ts @@ -0,0 +1,187 @@ +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("with Gemini-style providers (complete args in one chunk)", () => { + it("should return true when transitioning from undefined to a valid path", () => { + // Gemini sends name first (no args), then all args at once + // Simulate: First partial has undefined path + const result1 = tool.testHasPathStabilized(undefined) + expect(result1).toBe(false) // No path yet + + // Second partial has the complete path + const result2 = tool.testHasPathStabilized("src/file.ts") + expect(result2).toBe(true) // First valid path after undefined = 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 + }) + }) + + describe("with incremental streaming providers (char-by-char)", () => { + it("should return true when the same path is seen twice", () => { + // Simulate incremental streaming where path grows + tool.testHasPathStabilized(undefined) // Initial state + tool.testHasPathStabilized("s") // First char + tool.testHasPathStabilized("sr") // Growing + tool.testHasPathStabilized("src") // Growing + tool.testHasPathStabilized("src/") // Growing + tool.testHasPathStabilized("src/file") // Growing + tool.testHasPathStabilized("src/file.ts") // Complete + + // 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 return true on first valid path after undefined (may show truncated)", () => { + // This is acceptable behavior - briefly showing truncated paths + // is better than showing nothing or wrong paths + tool.testHasPathStabilized(undefined) + + const result = tool.testHasPathStabilized("s") + expect(result).toBe(true) // First valid after undefined + + // Subsequent different values won't trigger until stable + const result2 = tool.testHasPathStabilized("sr") + expect(result2).toBe(false) // Different from previous + + const result3 = tool.testHasPathStabilized("src") + expect(result3).toBe(false) // Different from previous + }) + }) + + 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, transitioning to a path should work + const result1 = tool.testHasPathStabilized(undefined) + expect(result1).toBe(false) + + const result2 = tool.testHasPathStabilized("new/path.ts") + expect(result2).toBe(true) // First valid after undefined + }) + + it("should handle state bleeding between tool calls (stale state cleared by undefined)", () => { + // Simulate: Tool A completes with a path + tool.testHasPathStabilized("old/path.ts") + tool.testHasPathStabilized("old/path.ts") + + // Simulate: Tool A is rejected, resetPartialState never called + // State is now "old/path.ts" + + // Simulate: Tool B starts (Gemini-style - undefined first) + const result1 = tool.testHasPathStabilized(undefined) + expect(result1).toBe(false) // Clears stale state + + // Tool B's actual path + const result2 = tool.testHasPathStabilized("new/path.ts") + expect(result2).toBe(true) // Works because previous was undefined + }) + + it("should handle state bleeding with non-Gemini providers", () => { + // 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 + const result1 = tool.testHasPathStabilized("n") + expect(result1).toBe(true) // First valid after undefined + + // Grows but different from previous + const result2 = tool.testHasPathStabilized("ne") + expect(result2).toBe(false) + + // Eventually stabilizes + tool.testHasPathStabilized("new") + tool.testHasPathStabilized("new/") + tool.testHasPathStabilized("new/path") + tool.testHasPathStabilized("new/path.ts") + const result3 = tool.testHasPathStabilized("new/path.ts") + expect(result3).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 true when same valid path is provided on first call (fresh state)", () => { + // Fresh tool state means lastSeenPartialPath is undefined + // First valid path = first valid after undefined + const result = tool.testHasPathStabilized("src/file.ts") + expect(result).toBe(true) + }) + + 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 + const result = tool.testHasPathStabilized("path.ts") + expect(result).toBe(true) + }) + }) + }) +}) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index fd791729b4d..15b48586cf3 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -281,11 +281,11 @@ describe("writeToFileTool", () => { it.skipIf(process.platform === "win32")( "creates parent directories when path has stabilized (partial)", async () => { - // First call - path not yet stabilized - await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // First call with undefined path - no action yet + await executeWriteFileTool({ path: undefined }, { fileExists: false, isPartial: true }) expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() - // Second call with same path - path is now stabilized + // Second call with valid path - path is now considered stable (first valid after undefined) await executeWriteFileTool({}, { fileExists: false, isPartial: true }) expect(mockedCreateDirectoriesForFile).toHaveBeenCalledWith(absoluteFilePath) }, @@ -400,12 +400,12 @@ describe("writeToFileTool", () => { }) it("streams content updates during partial execution after path stabilizes", async () => { - // First call - path not yet stabilized, early return (no file operations) - await executeWriteFileTool({}, { isPartial: true }) + // 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 same path - path is now stabilized, file operations proceed + // Second call with valid path - path is considered stable (first valid after undefined) await executeWriteFileTool({}, { isPartial: true }) expect(mockCline.ask).toHaveBeenCalled() expect(mockCline.diffViewProvider.open).toHaveBeenCalledWith(testFilePath) @@ -455,11 +455,11 @@ 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 - await executeWriteFileTool({}, { isPartial: true }) + // 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 same path - path is now stabilized, error occurs + // Second call with valid path - path is considered stable, error occurs during file operations await executeWriteFileTool({}, { isPartial: true }) expect(mockHandleError).toHaveBeenCalledWith("handling partial write_to_file", expect.any(Error)) }) From 2afc0b33e4a01659575471dcb236f8d601f5be56 Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Fri, 9 Jan 2026 17:13:20 -0500 Subject: [PATCH 2/3] fix: getReadablePath returns empty string for undefined/empty paths Addresses PR feedback to differentiate between undefined and empty paths. Now getReadablePath(cwd, undefined) and getReadablePath(cwd, '') both return '' instead of the CWD basename, preventing incorrect UI display. --- src/utils/__tests__/path.spec.ts | 8 ++++++-- src/utils/path.ts | 5 ++++- 2 files changed, 10 insertions(+), 3 deletions(-) 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"))) { From 2b7426b8dbdb5ddf1c8be0c3717e7104ce7c83c2 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Sat, 10 Jan 2026 01:45:34 +0000 Subject: [PATCH 3/3] fix: require same path value twice for stabilization to prevent file operations on truncated paths Addresses PR feedback about hasPathStabilized() potentially causing file operations on truncated paths for incremental streaming providers. Changes: - BaseTool.ts: Remove Case 2 (first valid after undefined = stable) from hasPathStabilized(). Now requires same non-empty path value twice. - BaseTool.spec.ts: Update tests to verify safer stabilization behavior. - writeToFileTool.spec.ts: Update tests to call with path twice for stabilization. This ensures tools like WriteToFileTool do not perform file operations (createDirectoriesForFile, diffViewProvider.open) on truncated paths. For Gemini-style providers, the path will stabilize when it appears twice. The getReadablePath() fix from the previous commit handles UI display. --- src/core/tools/BaseTool.ts | 48 +++--- src/core/tools/__tests__/BaseTool.spec.ts | 143 ++++++++++++------ .../tools/__tests__/writeToFileTool.spec.ts | 19 ++- 3 files changed, 127 insertions(+), 83 deletions(-) diff --git a/src/core/tools/BaseTool.ts b/src/core/tools/BaseTool.ts index 58241f89164..5dcae1ecd59 100644 --- a/src/core/tools/BaseTool.ts +++ b/src/core/tools/BaseTool.ts @@ -132,47 +132,33 @@ export abstract class BaseTool { * * During native tool call streaming, the partial-json library may return truncated * string values when chunk boundaries fall mid-value. This method tracks the path - * value between consecutive handlePartial() calls and returns true when the path - * is ready to display. + * value between consecutive handlePartial() calls and returns true only when the + * path has stopped changing (stabilized). * - * A path is considered stable when: - * 1. The same non-empty value is seen twice consecutively (handles incremental streaming - * where paths grow char-by-char until stabilizing) - * 2. A non-empty value appears after undefined (handles providers like Gemini that - * send complete args in one chunk - the first valid path is the complete path) + * 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 incremental streaming providers, option 2 may briefly show truncated paths, - * but these quickly update as more chunks arrive. This is preferable to showing - * nothing or showing incorrect paths (like CWD basename). - * - * 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 - * ``` + * 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 and is non-empty, false otherwise + * @returns true if path has stabilized (same value seen twice) and is non-empty, false otherwise */ protected hasPathStabilized(path: string | undefined): boolean { const previousPath = this.lastSeenPartialPath this.lastSeenPartialPath = path - // Must have a valid path to consider it stable - if (!path) { - return false - } - - // Case 1: Same non-empty value seen twice (incremental streaming stabilized) - const sameValueTwice = previousPath !== undefined && previousPath === path - - // Case 2: First valid value after undefined (Gemini-style complete args) - // This handles providers that send undefined first (name only), then complete args - const firstValidAfterUndefined = previousPath === undefined + // 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 sameValueTwice || firstValidAfterUndefined + return pathHasStabilized } /** diff --git a/src/core/tools/__tests__/BaseTool.spec.ts b/src/core/tools/__tests__/BaseTool.spec.ts index 98e567dec77..7d21115854d 100644 --- a/src/core/tools/__tests__/BaseTool.spec.ts +++ b/src/core/tools/__tests__/BaseTool.spec.ts @@ -40,16 +40,24 @@ describe("BaseTool", () => { tool = new TestTool() }) - describe("with Gemini-style providers (complete args in one chunk)", () => { - it("should return true when transitioning from undefined to a valid path", () => { - // Gemini sends name first (no args), then all args at once - // Simulate: First partial has undefined path + 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 partial has the complete path + // Second call with valid path - NOT stable yet (need same value twice) const result2 = tool.testHasPathStabilized("src/file.ts") - expect(result2).toBe(true) // First valid path after undefined = stable + 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)", () => { @@ -59,38 +67,65 @@ describe("BaseTool", () => { 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 return true when the same path is seen twice", () => { + it("should only return true when path stops changing", () => { // Simulate incremental streaming where path grows tool.testHasPathStabilized(undefined) // Initial state - tool.testHasPathStabilized("s") // First char - tool.testHasPathStabilized("sr") // Growing - tool.testHasPathStabilized("src") // Growing - tool.testHasPathStabilized("src/") // Growing - tool.testHasPathStabilized("src/file") // Growing - tool.testHasPathStabilized("src/file.ts") // Complete + 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 return true on first valid path after undefined (may show truncated)", () => { - // This is acceptable behavior - briefly showing truncated paths - // is better than showing nothing or wrong paths + 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(true) // First valid after undefined + expect(result).toBe(false) // First valid after undefined - NOT stable (could be truncated) - // Subsequent different values won't trigger until stable - const result2 = tool.testHasPathStabilized("sr") - expect(result2).toBe(false) // Different from previous + // Still not stable as path keeps changing + expect(tool.testHasPathStabilized("sr")).toBe(false) + expect(tool.testHasPathStabilized("src")).toBe(false) - const result3 = tool.testHasPathStabilized("src") - expect(result3).toBe(false) // Different from previous + // 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 }) }) @@ -103,32 +138,38 @@ describe("BaseTool", () => { // Reset tool.testResetPartialState() - // After reset, transitioning to a path should work + // 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(true) // First valid after undefined + 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 (stale state cleared by undefined)", () => { + 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") + tool.testHasPathStabilized("old/path.ts") // Stable // Simulate: Tool A is rejected, resetPartialState never called // State is now "old/path.ts" - // Simulate: Tool B starts (Gemini-style - undefined first) + // Simulate: Tool B starts (undefined first) const result1 = tool.testHasPathStabilized(undefined) expect(result1).toBe(false) // Clears stale state - // Tool B's actual path + // Tool B's actual path - need to see twice const result2 = tool.testHasPathStabilized("new/path.ts") - expect(result2).toBe(true) // Works because previous was undefined + expect(result2).toBe(false) // First time + + const result3 = tool.testHasPathStabilized("new/path.ts") + expect(result3).toBe(true) // Same value twice }) - it("should handle state bleeding with non-Gemini providers", () => { + 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") @@ -138,21 +179,17 @@ describe("BaseTool", () => { // Simulate: Tool B starts (incremental - undefined first) tool.testHasPathStabilized(undefined) // Clears stale state - // Tool B's path grows incrementally - const result1 = tool.testHasPathStabilized("n") - expect(result1).toBe(true) // First valid after undefined - - // Grows but different from previous - const result2 = tool.testHasPathStabilized("ne") - expect(result2).toBe(false) + // 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 - tool.testHasPathStabilized("new") - tool.testHasPathStabilized("new/") - tool.testHasPathStabilized("new/path") - tool.testHasPathStabilized("new/path.ts") - const result3 = tool.testHasPathStabilized("new/path.ts") - expect(result3).toBe(true) // Same value twice + const result = tool.testHasPathStabilized("new/path.ts") + expect(result).toBe(true) // Same value twice }) }) @@ -166,11 +203,19 @@ describe("BaseTool", () => { expect(tool.testHasPathStabilized("")).toBe(false) }) - it("should return true when same valid path is provided on first call (fresh state)", () => { + it("should return false on first call with valid path (fresh state)", () => { // Fresh tool state means lastSeenPartialPath is undefined - // First valid path = first valid after undefined + // First valid path should NOT be stable (need same value twice) const result = tool.testHasPathStabilized("src/file.ts") - expect(result).toBe(true) + 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", () => { @@ -178,9 +223,9 @@ describe("BaseTool", () => { expect(tool.testHasPathStabilized(undefined)).toBe(false) expect(tool.testHasPathStabilized(undefined)).toBe(false) - // Then valid path - const result = tool.testHasPathStabilized("path.ts") - expect(result).toBe(true) + // 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 15b48586cf3..89f43af5aaf 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -285,7 +285,11 @@ describe("writeToFileTool", () => { await executeWriteFileTool({ path: undefined }, { fileExists: false, isPartial: true }) expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() - // Second call with valid path - path is now considered stable (first valid after undefined) + // Second call with valid path - not yet stable (need same path twice) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() + + // Third call with same valid path - NOW path is stable await executeWriteFileTool({}, { fileExists: false, isPartial: true }) expect(mockedCreateDirectoriesForFile).toHaveBeenCalledWith(absoluteFilePath) }, @@ -405,7 +409,12 @@ describe("writeToFileTool", () => { expect(mockCline.ask).not.toHaveBeenCalled() expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() - // Second call with valid path - path is considered stable (first valid after undefined) + // 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() + + // 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) @@ -459,7 +468,11 @@ describe("writeToFileTool", () => { await executeWriteFileTool({ path: undefined }, { isPartial: true }) expect(mockHandleError).not.toHaveBeenCalled() - // Second call with valid path - path is considered stable, error occurs during file operations + // Second call with valid path - not yet stable (no file operations attempted) + await executeWriteFileTool({}, { isPartial: true }) + expect(mockHandleError).not.toHaveBeenCalled() + + // 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)) })