Browse Source

fix: stabilize file paths during native tool call streaming (#10555)

Daniel 2 months ago
parent
commit
ef4b95060b

+ 8 - 1
src/core/tools/ApplyDiffTool.ts

@@ -259,6 +259,7 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
 			}
 
 			await task.diffViewProvider.reset()
+			this.resetPartialState()
 
 			// Process any queued messages after file edit completes
 			task.processQueuedMessages()
@@ -267,6 +268,7 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
 		} catch (error) {
 			await handleError("applying diff", error as Error)
 			await task.diffViewProvider.reset()
+			this.resetPartialState()
 			task.processQueuedMessages()
 			return
 		}
@@ -276,9 +278,14 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
 		const relPath: string | undefined = block.params.path
 		const diffContent: string | undefined = block.params.diff
 
+		// Wait for path to stabilize before showing UI (prevents truncated paths)
+		if (!this.hasPathStabilized(relPath)) {
+			return
+		}
+
 		const sharedMessageProps: ClineSayTool = {
 			tool: "appliedDiff",
-			path: getReadablePath(task.cwd, relPath || ""),
+			path: getReadablePath(task.cwd, relPath),
 			diff: diffContent,
 		}
 

+ 41 - 0
src/core/tools/BaseTool.ts

@@ -47,6 +47,12 @@ 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.
+	 */
+	protected lastSeenPartialPath: string | undefined = undefined
+
 	/**
 	 * Parse XML/legacy string-based parameters into typed parameters.
 	 *
@@ -120,6 +126,41 @@ export abstract class BaseTool<TName extends ToolName> {
 		return text.replace(tagRegex, "")
 	}
 
+	/**
+	 * Check if a path parameter has stabilized during streaming.
+	 *
+	 * 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).
+	 *
+	 * 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
+	 * ```
+	 *
+	 * @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
+		return pathHasStabilized && !!path
+	}
+
+	/**
+	 * Reset the partial state tracking.
+	 *
+	 * Should be called at the end of execute() (both success and error paths)
+	 * to ensure clean state for the next tool invocation.
+	 */
+	resetPartialState(): void {
+		this.lastSeenPartialPath = undefined
+	}
+
 	/**
 	 * Main entry point for tool execution.
 	 *

+ 13 - 6
src/core/tools/EditFileTool.ts

@@ -327,12 +327,14 @@ export class EditFileTool extends BaseTool<"edit_file"> {
 			// Record successful tool usage and cleanup
 			task.recordToolUsage("edit_file")
 			await task.diffViewProvider.reset()
+			this.resetPartialState()
 
 			// Process any queued messages after file edit completes
 			task.processQueuedMessages()
 		} catch (error) {
 			await handleError("edit_file", error as Error)
 			await task.diffViewProvider.reset()
+			this.resetPartialState()
 		}
 	}
 
@@ -340,6 +342,11 @@ export class EditFileTool extends BaseTool<"edit_file"> {
 		const filePath: string | undefined = block.params.file_path
 		const oldString: string | undefined = block.params.old_string
 
+		// Wait for path to stabilize before showing UI (prevents truncated paths)
+		if (!this.hasPathStabilized(filePath)) {
+			return
+		}
+
 		let operationPreview: string | undefined
 		if (oldString !== undefined) {
 			if (oldString === "") {
@@ -350,14 +357,14 @@ export class EditFileTool extends BaseTool<"edit_file"> {
 			}
 		}
 
-		// Determine relative path for display
-		let relPath = filePath || ""
-		if (filePath && path.isAbsolute(filePath)) {
-			relPath = path.relative(task.cwd, filePath)
+		// Determine relative path for display (filePath is guaranteed non-null after hasPathStabilized)
+		let relPath = filePath!
+		if (path.isAbsolute(relPath)) {
+			relPath = path.relative(task.cwd, relPath)
 		}
 
-		const absolutePath = relPath ? path.resolve(task.cwd, relPath) : ""
-		const isOutsideWorkspace = absolutePath ? isPathOutsideWorkspace(absolutePath) : false
+		const absolutePath = path.resolve(task.cwd, relPath)
+		const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
 
 		const sharedMessageProps: ClineSayTool = {
 			tool: "appliedDiff",

+ 12 - 3
src/core/tools/SearchAndReplaceTool.ts

@@ -259,17 +259,25 @@ 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()
 
 			// 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()
 		}
 	}
 
 	override async handlePartial(task: Task, block: ToolUse<"search_and_replace">): Promise<void> {
 		const relPath: string | undefined = block.params.path
+
+		// Wait for path to stabilize before showing UI (prevents truncated paths)
+		if (!this.hasPathStabilized(relPath)) {
+			return
+		}
+
 		const operationsStr: string | undefined = block.params.operations
 
 		let operationsPreview: string | undefined
@@ -284,12 +292,13 @@ export class SearchAndReplaceTool extends BaseTool<"search_and_replace"> {
 			}
 		}
 
-		const absolutePath = relPath ? path.resolve(task.cwd, relPath) : ""
-		const isOutsideWorkspace = absolutePath ? isPathOutsideWorkspace(absolutePath) : false
+		// relPath is guaranteed non-null after hasPathStabilized
+		const absolutePath = path.resolve(task.cwd, relPath!)
+		const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
 
 		const sharedMessageProps: ClineSayTool = {
 			tool: "appliedDiff",
-			path: getReadablePath(task.cwd, relPath || ""),
+			path: getReadablePath(task.cwd, relPath!),
 			diff: operationsPreview,
 			isOutsideWorkspace,
 		}

+ 13 - 6
src/core/tools/SearchReplaceTool.ts

@@ -240,12 +240,14 @@ export class SearchReplaceTool extends BaseTool<"search_replace"> {
 			// Record successful tool usage and cleanup
 			task.recordToolUsage("search_replace")
 			await task.diffViewProvider.reset()
+			this.resetPartialState()
 
 			// 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()
 		}
 	}
 
@@ -253,6 +255,11 @@ export class SearchReplaceTool extends BaseTool<"search_replace"> {
 		const filePath: string | undefined = block.params.file_path
 		const oldString: string | undefined = block.params.old_string
 
+		// Wait for path to stabilize before showing UI (prevents truncated paths)
+		if (!this.hasPathStabilized(filePath)) {
+			return
+		}
+
 		let operationPreview: string | undefined
 		if (oldString) {
 			// Show a preview of what will be replaced
@@ -260,14 +267,14 @@ export class SearchReplaceTool extends BaseTool<"search_replace"> {
 			operationPreview = `replacing: "${preview}"`
 		}
 
-		// Determine relative path for display
-		let relPath = filePath || ""
-		if (filePath && path.isAbsolute(filePath)) {
-			relPath = path.relative(task.cwd, filePath)
+		// Determine relative path for display (filePath is guaranteed non-null after hasPathStabilized)
+		let relPath = filePath!
+		if (path.isAbsolute(relPath)) {
+			relPath = path.relative(task.cwd, relPath)
 		}
 
-		const absolutePath = relPath ? path.resolve(task.cwd, relPath) : ""
-		const isOutsideWorkspace = absolutePath ? isPathOutsideWorkspace(absolutePath) : false
+		const absolutePath = path.resolve(task.cwd, relPath)
+		const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
 
 		const sharedMessageProps: ClineSayTool = {
 			tool: "appliedDiff",

+ 8 - 24
src/core/tools/WriteToFileTool.ts

@@ -200,21 +200,12 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
 		}
 	}
 
-	// Track the last seen path during streaming to detect when the path has stabilized
-	private lastSeenPartialPath: string | undefined = undefined
-
 	override async handlePartial(task: Task, block: ToolUse<"write_to_file">): Promise<void> {
 		const relPath: string | undefined = block.params.path
 		let newContent: string | undefined = block.params.content
 
-		// During streaming, the partial-json library may return truncated string values
-		// when chunk boundaries fall mid-value. To avoid creating files at incorrect paths,
-		// we wait until the path stops changing between consecutive partial blocks before
-		// creating the file. This ensures we have the complete, final path value.
-		const pathHasStabilized = this.lastSeenPartialPath !== undefined && this.lastSeenPartialPath === relPath
-		this.lastSeenPartialPath = relPath
-
-		if (!pathHasStabilized || !relPath || newContent === undefined) {
+		// Wait for path to stabilize before showing UI (prevents truncated paths)
+		if (!this.hasPathStabilized(relPath) || newContent === undefined) {
 			return
 		}
 
@@ -229,8 +220,9 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
 			return
 		}
 
+		// relPath is guaranteed non-null after hasPathStabilized
 		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"
@@ -245,13 +237,12 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
 			await createDirectoriesForFile(absolutePath)
 		}
 
-		const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath) || false
-		const fullPath = absolutePath
-		const isOutsideWorkspace = isPathOutsideWorkspace(fullPath)
+		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,
@@ -262,7 +253,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(
@@ -271,13 +262,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
 			)
 		}
 	}
-
-	/**
-	 * Reset state when the tool finishes (called from execute or on error)
-	 */
-	resetPartialState(): void {
-		this.lastSeenPartialPath = undefined
-	}
 }
 
 export const writeToFileTool = new WriteToFileTool()

+ 6 - 1
src/core/tools/__tests__/editFileTool.spec.ts

@@ -363,13 +363,18 @@ describe("editFileTool", () => {
 	})
 
 	describe("partial block handling", () => {
-		it("handles partial block without errors", async () => {
+		it("handles partial block without errors after path stabilizes", async () => {
+			// Path stabilization requires two consecutive calls with the same path
+			// First call sets lastSeenPartialPath, second call sees it has stabilized
+			await executeEditFileTool({}, { isPartial: true })
 			await executeEditFileTool({}, { isPartial: true })
 
 			expect(mockTask.ask).toHaveBeenCalled()
 		})
 
 		it("shows creating new file preview when old_string is empty", async () => {
+			// Path stabilization requires two consecutive calls with the same path
+			await executeEditFileTool({ old_string: "" }, { isPartial: true })
 			await executeEditFileTool({ old_string: "" }, { isPartial: true })
 
 			expect(mockTask.ask).toHaveBeenCalled()

+ 4 - 1
src/core/tools/__tests__/searchAndReplaceTool.spec.ts

@@ -346,7 +346,10 @@ describe("searchAndReplaceTool", () => {
 	})
 
 	describe("partial block handling", () => {
-		it("handles partial block without errors", async () => {
+		it("handles partial block without errors after path stabilizes", async () => {
+			// Path stabilization requires two consecutive calls with the same path
+			// First call sets lastSeenPartialPath, second call sees it has stabilized
+			await executeSearchAndReplaceTool({}, { isPartial: true })
 			await executeSearchAndReplaceTool({}, { isPartial: true })
 
 			expect(mockTask.ask).toHaveBeenCalled()

+ 4 - 1
src/core/tools/__tests__/searchReplaceTool.spec.ts

@@ -321,7 +321,10 @@ describe("searchReplaceTool", () => {
 	})
 
 	describe("partial block handling", () => {
-		it("handles partial block without errors", async () => {
+		it("handles partial block without errors after path stabilizes", async () => {
+			// Path stabilization requires two consecutive calls with the same path
+			// First call sets lastSeenPartialPath, second call sees it has stabilized
+			await executeSearchReplaceTool({}, { isPartial: true })
 			await executeSearchReplaceTool({}, { isPartial: true })
 
 			expect(mockCline.ask).toHaveBeenCalled()