From 65d432d90a560c55a7bd9be353a084dbeaf138b8 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Sun, 29 Mar 2026 12:17:05 +0000 Subject: [PATCH] fix: stop spinner on failed file edits and add re-anchor experiment (#12032) Part 1 - UI Bug Fix: - ApplyDiffTool: Add finalizePartialToolAskIfNeeded to close partial ask messages on error paths, preventing stuck spinners - ApplyDiffTool: Always show diff_error on first failure (was >= 2) - ApplyDiffTool: Set didToolFailInCurrentTurn on diff apply failure - EditFileTool: Always show diff_error on first failure (was >= 2) Part 2 - Re-anchor Before Edit Experiment: - Add reAnchorBeforeEdit experiment type to packages/types - Add RE_ANCHOR_BEFORE_EDIT to shared experiments config (default off) - When enabled, adds system prompt guideline instructing the model to re-read a file before every edit to reduce hallucination-based failures - Add English localization keys for settings UI - Settings UI auto-renders via existing ExperimentalSettings component --- packages/types/src/experiment.ts | 9 +++- .../__tests__/tool-use-guidelines.spec.ts | 47 ++++++++----------- .../prompts/sections/tool-use-guidelines.ts | 17 ++++++- src/core/prompts/system.ts | 2 +- src/core/tools/ApplyDiffTool.ts | 46 ++++++++++++++++-- src/core/tools/EditFileTool.ts | 5 +- src/core/tools/__tests__/editFileTool.spec.ts | 15 +++--- src/shared/__tests__/experiments.spec.ts | 34 ++++++++++++++ src/shared/experiments.ts | 2 + webview-ui/src/i18n/locales/en/settings.json | 4 ++ 10 files changed, 136 insertions(+), 45 deletions(-) diff --git a/packages/types/src/experiment.ts b/packages/types/src/experiment.ts index d7eb0b03d6c..6f223496d4d 100644 --- a/packages/types/src/experiment.ts +++ b/packages/types/src/experiment.ts @@ -6,7 +6,13 @@ import type { Keys, Equals, AssertEqual } from "./type-fu.js" * ExperimentId */ -export const experimentIds = ["preventFocusDisruption", "imageGeneration", "runSlashCommand", "customTools"] as const +export const experimentIds = [ + "preventFocusDisruption", + "imageGeneration", + "runSlashCommand", + "customTools", + "reAnchorBeforeEdit", +] as const export const experimentIdsSchema = z.enum(experimentIds) @@ -21,6 +27,7 @@ export const experimentsSchema = z.object({ imageGeneration: z.boolean().optional(), runSlashCommand: z.boolean().optional(), customTools: z.boolean().optional(), + reAnchorBeforeEdit: z.boolean().optional(), }) export type Experiments = z.infer diff --git a/src/core/prompts/sections/__tests__/tool-use-guidelines.spec.ts b/src/core/prompts/sections/__tests__/tool-use-guidelines.spec.ts index 6d1f4b3fbf0..d1dd7529f84 100644 --- a/src/core/prompts/sections/__tests__/tool-use-guidelines.spec.ts +++ b/src/core/prompts/sections/__tests__/tool-use-guidelines.spec.ts @@ -1,39 +1,30 @@ import { getToolUseGuidelinesSection } from "../tool-use-guidelines" describe("getToolUseGuidelinesSection", () => { - it("should include proper numbered guidelines", () => { - const guidelines = getToolUseGuidelinesSection() - - expect(guidelines).toContain("1. Assess what information") - expect(guidelines).toContain("2. Choose the most appropriate tool") - expect(guidelines).toContain("3. If multiple actions are needed") - }) - - it("should include multiple-tools-per-message guidance", () => { - const guidelines = getToolUseGuidelinesSection() - - expect(guidelines).toContain("you may use multiple tools in a single message") - expect(guidelines).not.toContain("use one tool at a time per message") + it("returns base guidelines without re-anchor text when no experiments provided", () => { + const result = getToolUseGuidelinesSection() + expect(result).toContain("Tool Use Guidelines") + expect(result).toContain("Assess what information you already have") + expect(result).not.toContain("Re-anchor before every file edit") }) - it("should use simplified footer without step-by-step language", () => { - const guidelines = getToolUseGuidelinesSection() - - expect(guidelines).toContain("carefully considering the user's response after tool executions") - expect(guidelines).not.toContain("It is crucial to proceed step-by-step") - expect(guidelines).not.toContain("ALWAYS wait for user confirmation after each tool use") + it("returns base guidelines without re-anchor text when experiment is disabled", () => { + const result = getToolUseGuidelinesSection({ reAnchorBeforeEdit: false }) + expect(result).toContain("Tool Use Guidelines") + expect(result).not.toContain("Re-anchor before every file edit") }) - it("should include common guidance", () => { - const guidelines = getToolUseGuidelinesSection() - expect(guidelines).toContain("Assess what information you already have") - expect(guidelines).toContain("Choose the most appropriate tool") - expect(guidelines).not.toContain("") + it("includes re-anchor guideline when experiment is enabled", () => { + const result = getToolUseGuidelinesSection({ reAnchorBeforeEdit: true }) + expect(result).toContain("Tool Use Guidelines") + expect(result).toContain("Re-anchor before every file edit") + expect(result).toContain("re-read the file") + expect(result).toContain("read_file") }) - it("should not include per-tool confirmation guidelines", () => { - const guidelines = getToolUseGuidelinesSection() - - expect(guidelines).not.toContain("After each tool use, the user will respond with the result") + it("returns base guidelines when empty experiments object provided", () => { + const result = getToolUseGuidelinesSection({}) + expect(result).toContain("Tool Use Guidelines") + expect(result).not.toContain("Re-anchor before every file edit") }) }) diff --git a/src/core/prompts/sections/tool-use-guidelines.ts b/src/core/prompts/sections/tool-use-guidelines.ts index 78193372cc8..cf38b3b21da 100644 --- a/src/core/prompts/sections/tool-use-guidelines.ts +++ b/src/core/prompts/sections/tool-use-guidelines.ts @@ -1,9 +1,22 @@ -export function getToolUseGuidelinesSection(): string { +import type { Experiments } from "@roo-code/types" + +import { EXPERIMENT_IDS, experiments } from "../../../shared/experiments" + +export function getToolUseGuidelinesSection(experimentsConfig?: Experiments): string { + const reAnchorEnabled = experimentsConfig + ? experiments.isEnabled(experimentsConfig, EXPERIMENT_IDS.RE_ANCHOR_BEFORE_EDIT) + : false + + const reAnchorGuideline = reAnchorEnabled + ? ` +4. **Re-anchor before every file edit:** Before modifying any existing file, you MUST re-read the file (or the relevant section) using the read_file tool to confirm its current contents. This re-anchoring step is critical to avoid edits based on stale or hallucinated content. The re-read must happen immediately before the edit tool call, even if you have read the file earlier in the conversation. This applies to all file editing tools (apply_diff, edit_file, edit, search_replace, write_to_file for existing files, apply_patch for modifications). When the file is large, use read_file with an appropriate offset/limit or indentation mode to read the specific section you plan to modify. Skipping this step is the most common cause of failed edits.` + : "" + return `# Tool Use Guidelines 1. Assess what information you already have and what information you need to proceed with the task. 2. Choose the most appropriate tool based on the task and the tool descriptions provided. Assess if you need additional information to proceed, and which of the available tools would be most effective for gathering this information. For example using the list_files tool is more effective than running a command like \`ls\` in the terminal. It's critical that you think about each available tool and use the one that best fits the current step in the task. -3. If multiple actions are needed, you may use multiple tools in a single message when appropriate, or use tools iteratively across messages. Each tool use should be informed by the results of previous tool uses. Do not assume the outcome of any tool use. Each step must be informed by the previous step's result. +3. If multiple actions are needed, you may use multiple tools in a single message when appropriate, or use tools iteratively across messages. Each tool use should be informed by the results of previous tool uses. Do not assume the outcome of any tool use. Each step must be informed by the previous step's result.${reAnchorGuideline} By carefully considering the user's response after tool executions, you can react accordingly and make informed decisions about how to proceed with the task. This iterative process helps ensure the overall success and accuracy of your work.` } diff --git a/src/core/prompts/system.ts b/src/core/prompts/system.ts index 0d6071644a9..fc14a4be519 100644 --- a/src/core/prompts/system.ts +++ b/src/core/prompts/system.ts @@ -88,7 +88,7 @@ ${markdownFormattingSection()} ${getSharedToolUseSection()}${toolsCatalog} - ${getToolUseGuidelinesSection()} + ${getToolUseGuidelinesSection(experiments)} ${getCapabilitiesSection(cwd, shouldIncludeMcp ? mcpHub : undefined)} diff --git a/src/core/tools/ApplyDiffTool.ts b/src/core/tools/ApplyDiffTool.ts index 3b664b3bd22..a0366cf6244 100644 --- a/src/core/tools/ApplyDiffTool.ts +++ b/src/core/tools/ApplyDiffTool.ts @@ -5,6 +5,7 @@ import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" import { TelemetryService } from "@roo-code/telemetry" import { getReadablePath } from "../../utils/path" +import { isPathOutsideWorkspace } from "../../utils/pathUtils" import { Task } from "../task/Task" import { formatResponse } from "../prompts/responses" import { fileExistsAtPath } from "../../utils/fs" @@ -24,6 +25,9 @@ interface ApplyDiffParams { export class ApplyDiffTool extends BaseTool<"apply_diff"> { readonly name = "apply_diff" as const + private didSendPartialToolAsk = false + private partialToolAskRelPath: string | undefined + async execute(params: ApplyDiffParams, task: Task, callbacks: ToolCallbacks): Promise { const { askApproval, handleError, pushToolResult } = callbacks let { path: relPath, diff: diffContent } = params @@ -32,10 +36,31 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { diffContent = unescapeHtmlEntities(diffContent) } + // Finalize any outstanding partial tool ask so the UI spinner doesn't get stuck. + const finalizePartialToolAskIfNeeded = async (currentRelPath?: string): Promise => { + if (!this.didSendPartialToolAsk) { + return + } + + if (this.partialToolAskRelPath && currentRelPath && this.partialToolAskRelPath !== currentRelPath) { + return + } + + const sharedMessageProps: ClineSayTool = { + tool: "appliedDiff", + path: getReadablePath(task.cwd, currentRelPath ?? relPath ?? ""), + diff: diffContent, + } + + // Finalize the existing partial tool ask row so the UI doesn't get stuck in a spinner state. + await task.ask("tool", JSON.stringify(sharedMessageProps), false).catch(() => {}) + } + try { if (!relPath) { task.consecutiveMistakeCount++ task.recordToolError("apply_diff") + await finalizePartialToolAskIfNeeded() pushToolResult(await task.sayAndCreateMissingParamError("apply_diff", "path")) return } @@ -43,6 +68,7 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { if (!diffContent) { task.consecutiveMistakeCount++ task.recordToolError("apply_diff") + await finalizePartialToolAskIfNeeded() pushToolResult(await task.sayAndCreateMissingParamError("apply_diff", "diff")) return } @@ -50,6 +76,7 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { const accessAllowed = task.rooIgnoreController?.validateAccess(relPath) if (!accessAllowed) { + await finalizePartialToolAskIfNeeded(relPath) await task.say("rooignore_error", relPath) pushToolResult(formatResponse.rooIgnoreError(relPath)) return @@ -61,6 +88,7 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { if (!fileExists) { task.consecutiveMistakeCount++ task.recordToolError("apply_diff") + await finalizePartialToolAskIfNeeded(relPath) const formattedError = `File does not exist at path: ${absolutePath}\n\n\nThe specified file could not be found. Please verify the file path and try again.\n` await task.say("error", formattedError) task.didToolFailInCurrentTurn = true @@ -107,11 +135,14 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { }${errorDetails ? `\n\nDetails:\n${errorDetails}` : ""}\n` } - if (currentCount >= 2) { - await task.say("diff_error", formattedError) - } + // Always finalize any pending partial ask to stop the spinner. + await finalizePartialToolAskIfNeeded(relPath) + + // Always show diff_error on every failure so the user gets immediate visual feedback. + await task.say("diff_error", formattedError) task.recordToolError("apply_diff", formattedError) + task.didToolFailInCurrentTurn = true pushToolResult(formattedError) return @@ -261,11 +292,15 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { return } catch (error) { + await finalizePartialToolAskIfNeeded(relPath) await handleError("applying diff", error as Error) await task.diffViewProvider.reset() - this.resetPartialState() task.processQueuedMessages() return + } finally { + this.didSendPartialToolAsk = false + this.partialToolAskRelPath = undefined + this.resetPartialState() } } @@ -278,6 +313,9 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { return } + this.didSendPartialToolAsk = true + this.partialToolAskRelPath = relPath + const sharedMessageProps: ClineSayTool = { tool: "appliedDiff", path: getReadablePath(task.cwd, relPath), diff --git a/src/core/tools/EditFileTool.ts b/src/core/tools/EditFileTool.ts index 2495a372bc5..261f429190d 100644 --- a/src/core/tools/EditFileTool.ts +++ b/src/core/tools/EditFileTool.ts @@ -174,9 +174,8 @@ export class EditFileTool extends BaseTool<"edit_file"> { const currentCount = (task.consecutiveMistakeCountForEditFile.get(relPath) || 0) + 1 task.consecutiveMistakeCountForEditFile.set(relPath, currentCount) - if (currentCount >= 2) { - await task.say("diff_error", formattedError) - } + // Always show diff_error on every failure so the user gets immediate visual feedback. + await task.say("diff_error", formattedError) } try { diff --git a/src/core/tools/__tests__/editFileTool.spec.ts b/src/core/tools/__tests__/editFileTool.spec.ts index 80d431edab2..39caf0208a4 100644 --- a/src/core/tools/__tests__/editFileTool.spec.ts +++ b/src/core/tools/__tests__/editFileTool.spec.ts @@ -434,11 +434,11 @@ describe("editFileTool", () => { }) describe("consecutive error display behavior", () => { - it("does NOT show diff_error to user on first no_match failure", async () => { + it("shows diff_error to user on first no_match failure", async () => { await executeEditFileTool({ old_string: "NonExistent" }, { fileContent: "Line 1\nLine 2\nLine 3" }) expect(mockTask.consecutiveMistakeCountForEditFile.get(testFilePath)).toBe(1) - expect(mockTask.say).not.toHaveBeenCalledWith("diff_error", expect.any(String)) + expect(mockTask.say).toHaveBeenCalledWith("diff_error", expect.stringContaining("No match found")) expect(mockTask.recordToolError).toHaveBeenCalledWith( "edit_file", expect.stringContaining("No match found"), @@ -456,14 +456,17 @@ describe("editFileTool", () => { expect(mockTask.say).toHaveBeenCalledWith("diff_error", expect.stringContaining("No match found")) }) - it("does NOT show diff_error to user on first occurrence_mismatch failure", async () => { + it("shows diff_error to user on first occurrence_mismatch failure", async () => { await executeEditFileTool( { old_string: "Line", expected_replacements: "1" }, { fileContent: "Line 1\nLine 2\nLine 3" }, ) expect(mockTask.consecutiveMistakeCountForEditFile.get(testFilePath)).toBe(1) - expect(mockTask.say).not.toHaveBeenCalledWith("diff_error", expect.any(String)) + expect(mockTask.say).toHaveBeenCalledWith( + "diff_error", + expect.stringContaining("Occurrence count mismatch"), + ) expect(mockTask.recordToolError).toHaveBeenCalledWith( "edit_file", expect.stringContaining("Occurrence count mismatch"), @@ -522,8 +525,8 @@ describe("editFileTool", () => { expect(mockTask.consecutiveMistakeCountForEditFile.get(testFilePath)).toBe(1) expect(mockTask.consecutiveMistakeCountForEditFile.get(otherFilePath)).toBe(1) - // Neither should have triggered diff_error display - expect(mockTask.say).not.toHaveBeenCalledWith("diff_error", expect.any(String)) + // Both should have triggered diff_error display (errors are shown on every failure now) + expect(mockTask.say).toHaveBeenCalledWith("diff_error", expect.any(String)) }) }) diff --git a/src/shared/__tests__/experiments.spec.ts b/src/shared/__tests__/experiments.spec.ts index 92a7d7604ff..f0be6411a6b 100644 --- a/src/shared/__tests__/experiments.spec.ts +++ b/src/shared/__tests__/experiments.spec.ts @@ -14,6 +14,37 @@ describe("experiments", () => { }) }) + describe("RE_ANCHOR_BEFORE_EDIT", () => { + it("is configured correctly", () => { + expect(EXPERIMENT_IDS.RE_ANCHOR_BEFORE_EDIT).toBe("reAnchorBeforeEdit") + expect(experimentConfigsMap.RE_ANCHOR_BEFORE_EDIT).toMatchObject({ + enabled: false, + }) + }) + + it("returns false when not enabled", () => { + const experiments: Record = { + preventFocusDisruption: false, + imageGeneration: false, + runSlashCommand: false, + customTools: false, + reAnchorBeforeEdit: false, + } + expect(Experiments.isEnabled(experiments, EXPERIMENT_IDS.RE_ANCHOR_BEFORE_EDIT)).toBe(false) + }) + + it("returns true when enabled", () => { + const experiments: Record = { + preventFocusDisruption: false, + imageGeneration: false, + runSlashCommand: false, + customTools: false, + reAnchorBeforeEdit: true, + } + expect(Experiments.isEnabled(experiments, EXPERIMENT_IDS.RE_ANCHOR_BEFORE_EDIT)).toBe(true) + }) + }) + describe("isEnabled", () => { it("returns false when experiment is not enabled", () => { const experiments: Record = { @@ -21,6 +52,7 @@ describe("experiments", () => { imageGeneration: false, runSlashCommand: false, customTools: false, + reAnchorBeforeEdit: false, } expect(Experiments.isEnabled(experiments, EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION)).toBe(false) }) @@ -31,6 +63,7 @@ describe("experiments", () => { imageGeneration: false, runSlashCommand: false, customTools: false, + reAnchorBeforeEdit: false, } expect(Experiments.isEnabled(experiments, EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION)).toBe(true) }) @@ -41,6 +74,7 @@ describe("experiments", () => { imageGeneration: false, runSlashCommand: false, customTools: false, + reAnchorBeforeEdit: false, } expect(Experiments.isEnabled(experiments, EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION)).toBe(false) }) diff --git a/src/shared/experiments.ts b/src/shared/experiments.ts index e189f99e23d..e61f1dde538 100644 --- a/src/shared/experiments.ts +++ b/src/shared/experiments.ts @@ -5,6 +5,7 @@ export const EXPERIMENT_IDS = { IMAGE_GENERATION: "imageGeneration", RUN_SLASH_COMMAND: "runSlashCommand", CUSTOM_TOOLS: "customTools", + RE_ANCHOR_BEFORE_EDIT: "reAnchorBeforeEdit", } as const satisfies Record type _AssertExperimentIds = AssertEqual>> @@ -20,6 +21,7 @@ export const experimentConfigsMap: Record = { IMAGE_GENERATION: { enabled: false }, RUN_SLASH_COMMAND: { enabled: false }, CUSTOM_TOOLS: { enabled: false }, + RE_ANCHOR_BEFORE_EDIT: { enabled: false }, } export const experimentDefault = Object.fromEntries( diff --git a/webview-ui/src/i18n/locales/en/settings.json b/webview-ui/src/i18n/locales/en/settings.json index 3b2497aaee7..b9756e4a59a 100644 --- a/webview-ui/src/i18n/locales/en/settings.json +++ b/webview-ui/src/i18n/locales/en/settings.json @@ -886,6 +886,10 @@ "refreshSuccess": "Tools refreshed successfully", "refreshError": "Failed to refresh tools", "toolParameters": "Parameters" + }, + "RE_ANCHOR_BEFORE_EDIT": { + "name": "Re-anchor before file edits", + "description": "When enabled, the system prompt instructs the model to always re-read a file before editing it. This reduces hallucination-based edit failures by ensuring the model works with up-to-date file contents. Uses more tokens but significantly improves edit reliability." } }, "promptCaching": {