diff --git a/packages/types/src/experiment.ts b/packages/types/src/experiment.ts index d7eb0b03d6..6f223496d4 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 6d1f4b3fbf..d1dd7529f8 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("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 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 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 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("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 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("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 78193372cc..cf38b3b21d 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 0d6071644a..fc14a4be51 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 3b664b3bd2..a0366cf624 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 2495a372bc..261f429190 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 80d431edab..39caf0208a 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 92a7d7604f..f0be6411a6 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 e189f99e23..e61f1dde53 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 3b2497aaee..b9756e4a59 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": {