mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
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
This commit is contained in:
parent
137d3f4fd8
commit
65d432d90a
10 changed files with 136 additions and 45 deletions
|
|
@ -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<typeof experimentsSchema>
|
||||
|
|
|
|||
|
|
@ -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("<actual_tool_name>")
|
||||
})
|
||||
|
||||
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")
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -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.`
|
||||
}
|
||||
|
|
|
|||
|
|
@ -88,7 +88,7 @@ ${markdownFormattingSection()}
|
|||
|
||||
${getSharedToolUseSection()}${toolsCatalog}
|
||||
|
||||
${getToolUseGuidelinesSection()}
|
||||
${getToolUseGuidelinesSection(experiments)}
|
||||
|
||||
${getCapabilitiesSection(cwd, shouldIncludeMcp ? mcpHub : undefined)}
|
||||
|
||||
|
|
|
|||
|
|
@ -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<void> {
|
||||
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<void> => {
|
||||
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<error_details>\nThe specified file could not be found. Please verify the file path and try again.\n</error_details>`
|
||||
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</error_details>`
|
||||
}
|
||||
|
||||
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),
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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))
|
||||
})
|
||||
})
|
||||
|
||||
|
|
|
|||
|
|
@ -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<ExperimentId, boolean> = {
|
||||
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<ExperimentId, boolean> = {
|
||||
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<ExperimentId, boolean> = {
|
||||
|
|
@ -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)
|
||||
})
|
||||
|
|
|
|||
|
|
@ -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<string, ExperimentId>
|
||||
|
||||
type _AssertExperimentIds = AssertEqual<Equals<ExperimentId, Values<typeof EXPERIMENT_IDS>>>
|
||||
|
|
@ -20,6 +21,7 @@ export const experimentConfigsMap: Record<ExperimentKey, ExperimentConfig> = {
|
|||
IMAGE_GENERATION: { enabled: false },
|
||||
RUN_SLASH_COMMAND: { enabled: false },
|
||||
CUSTOM_TOOLS: { enabled: false },
|
||||
RE_ANCHOR_BEFORE_EDIT: { enabled: false },
|
||||
}
|
||||
|
||||
export const experimentDefault = Object.fromEntries(
|
||||
|
|
|
|||
|
|
@ -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": {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue