diff --git a/.changeset/fix-write-to-file-progressive-error.md b/.changeset/fix-write-to-file-progressive-error.md new file mode 100644 index 00000000000..3d19d4d69bf --- /dev/null +++ b/.changeset/fix-write-to-file-progressive-error.md @@ -0,0 +1,5 @@ +--- +"roo-cline": patch +--- + +Fix infinite retry loop when write_to_file fails with missing content parameter by providing progressive, tiered error guidance with context window awareness diff --git a/src/core/prompts/__tests__/writeToFileMissingContentError.spec.ts b/src/core/prompts/__tests__/writeToFileMissingContentError.spec.ts new file mode 100644 index 00000000000..dd9223b9b4a --- /dev/null +++ b/src/core/prompts/__tests__/writeToFileMissingContentError.spec.ts @@ -0,0 +1,123 @@ +import { formatResponse } from "../responses" + +describe("formatResponse.writeToFileMissingContentError", () => { + describe("first failure (tier 1)", () => { + it("should include the file path in the error message", () => { + const result = formatResponse.writeToFileMissingContentError("src/index.ts", 1) + expect(result).toContain("src/index.ts") + }) + + it("should include the base error explanation", () => { + const result = formatResponse.writeToFileMissingContentError("src/index.ts", 1) + expect(result).toContain("'content' parameter was empty") + expect(result).toContain("output token limits") + }) + + it("should include helpful suggestions", () => { + const result = formatResponse.writeToFileMissingContentError("src/index.ts", 1) + expect(result).toContain("Suggestions") + expect(result).toContain("apply_diff") + }) + + it("should include the tool use instructions reminder", () => { + const result = formatResponse.writeToFileMissingContentError("src/index.ts", 1) + expect(result).toContain("Reminder: Instructions for Tool Use") + }) + + it("should mention breaking down the task into smaller steps", () => { + const result = formatResponse.writeToFileMissingContentError("src/index.ts", 1) + expect(result).toContain("breaking down the task into smaller steps") + }) + + it("should suggest using apply_diff or edit for existing files", () => { + const result = formatResponse.writeToFileMissingContentError("src/index.ts", 1) + expect(result).toContain("prefer apply_diff or edit to make targeted edits") + }) + + it("should not include CRITICAL language on first failure", () => { + const result = formatResponse.writeToFileMissingContentError("src/index.ts", 1) + expect(result).not.toContain("CRITICAL") + }) + + it("should work with different file paths", () => { + const result = formatResponse.writeToFileMissingContentError("components/MyComponent.tsx", 1) + expect(result).toContain("components/MyComponent.tsx") + }) + }) + + describe("second failure (tier 2)", () => { + it("should indicate this is the 2nd failed attempt", () => { + const result = formatResponse.writeToFileMissingContentError("src/index.ts", 2) + expect(result).toContain("2nd failed attempt") + }) + + it("should strongly suggest alternative approaches", () => { + const result = formatResponse.writeToFileMissingContentError("src/index.ts", 2) + expect(result).toContain("must use a different strategy") + expect(result).toContain("Recommended approaches") + }) + + it("should tell model not to retry full write again", () => { + const result = formatResponse.writeToFileMissingContentError("src/index.ts", 2) + expect(result).toContain("Do NOT attempt to write the full file content") + }) + + it("should not include CRITICAL language on second failure", () => { + const result = formatResponse.writeToFileMissingContentError("src/index.ts", 2) + expect(result).not.toContain("CRITICAL") + }) + }) + + describe("third+ failure (tier 3)", () => { + it("should include CRITICAL language on third failure", () => { + const result = formatResponse.writeToFileMissingContentError("src/index.ts", 3) + expect(result).toContain("CRITICAL") + expect(result).toContain("3 times in a row") + }) + + it("should tell model to NOT retry write_to_file", () => { + const result = formatResponse.writeToFileMissingContentError("src/index.ts", 3) + expect(result).toContain("do NOT retry write_to_file") + }) + + it("should include required action strategies", () => { + const result = formatResponse.writeToFileMissingContentError("src/index.ts", 3) + expect(result).toContain("Required action") + expect(result).toContain("apply_diff or edit") + expect(result).toContain("50-100 lines") + }) + + it("should show correct count for higher failure counts", () => { + const result = formatResponse.writeToFileMissingContentError("src/index.ts", 5) + expect(result).toContain("5 times in a row") + }) + }) + + describe("context window awareness", () => { + it("should include context warning when usage exceeds 50%", () => { + const result = formatResponse.writeToFileMissingContentError("src/index.ts", 1, 60) + expect(result).toContain("60% full") + expect(result).toContain("output capacity is reduced") + }) + + it("should not include context warning when usage is 50% or below", () => { + const result = formatResponse.writeToFileMissingContentError("src/index.ts", 1, 50) + expect(result).not.toContain("% full") + }) + + it("should not include context warning when usage is undefined", () => { + const result = formatResponse.writeToFileMissingContentError("src/index.ts", 1) + expect(result).not.toContain("% full") + }) + + it("should include context warning in tier 2 messages", () => { + const result = formatResponse.writeToFileMissingContentError("src/index.ts", 2, 75) + expect(result).toContain("75% full") + }) + + it("should include context warning in tier 3 messages", () => { + const result = formatResponse.writeToFileMissingContentError("src/index.ts", 3, 90) + expect(result).toContain("90% full") + }) + }) +}) diff --git a/src/core/prompts/responses.ts b/src/core/prompts/responses.ts index 60b5b4123ac..84734b2fdc7 100644 --- a/src/core/prompts/responses.ts +++ b/src/core/prompts/responses.ts @@ -66,6 +66,66 @@ Otherwise, if you have not completed the task and do not need additional informa return `Missing value for required parameter '${paramName}'. Please retry with complete response.\n\n${instructions}` }, + /** + * Progressive error for write_to_file when the 'content' parameter is missing. + * Uses tiered messaging to break infinite retry loops: + * - Tier 1 (first failure): Helpful suggestions + * - Tier 2 (second failure): Stronger guidance to use alternative tools + * - Tier 3+ (third+ failure): Critical stop with explicit instructions + * + * @param relPath - The file path that was being written to + * @param failureCount - How many consecutive times this has failed (1-based) + * @param contextUsagePercent - Optional context window usage percentage + */ + writeToFileMissingContentError: (relPath: string, failureCount: number, contextUsagePercent?: number): string => { + const toolUseInstructionsReminder = getToolInstructionsReminder() + + const baseError = + `[write_to_file for '${relPath}'] The 'content' parameter was empty. ` + + `This usually means the file content was too large for the model's output token limits, ` + + `causing it to be truncated or dropped entirely.` + + const contextWarning = + contextUsagePercent !== undefined && contextUsagePercent > 50 + ? `\n\nContext window is ${contextUsagePercent}% full — output capacity is reduced.` + : "" + + // Tier 3: Critical stop after 3+ failures + if (failureCount >= 3) { + return ( + `CRITICAL: ${baseError}${contextWarning}\n\n` + + `This has now failed ${failureCount} times in a row — do NOT retry write_to_file for this file.\n\n` + + `Required action:\n` + + `1. **Use apply_diff or edit** to make targeted changes to the file instead of rewriting it entirely\n` + + `2. **If creating a new file**, write a minimal skeleton first (just imports and function/class signatures, no implementations — keep it under 50-100 lines), then use apply_diff or edit to fill in each section incrementally\n` + + `3. **Break the task into smaller steps** — write one function or section at a time` + ) + } + + // Tier 2: Stronger guidance after 2nd failure + if (failureCount >= 2) { + return ( + `${baseError}${contextWarning}\n\n` + + `This is the 2nd failed attempt — you must use a different strategy.\n\n` + + `Recommended approaches:\n` + + `1. **Use write_to_file with a minimal skeleton** (just the structure — imports, class/function signatures, no implementations), then use apply_diff or edit to fill in each section incrementally\n` + + `2. **Use apply_diff or edit with smaller chunks** — if the file already exists, make targeted edits instead of rewriting the entire file\n` + + `3. **Break the task into smaller steps** — write one function or section at a time\n\n` + + `Do NOT attempt to write the full file content in a single write_to_file call again.` + ) + } + + // Tier 1: Helpful guidance on first failure + return ( + `${baseError}${contextWarning}\n\n` + + `Suggestions:\n` + + `- If the file is large, try breaking down the task into smaller steps. Write a skeleton first, then fill in sections using apply_diff or edit.\n` + + `- If the file already exists, prefer apply_diff or edit to make targeted edits instead of rewriting the entire file.\n` + + `- Ensure the 'content' parameter contains the complete file content before closing the tool tag.\n\n` + + toolUseInstructionsReminder + ) + }, + invalidMcpToolArgumentError: (serverName: string, toolName: string) => JSON.stringify({ status: "error", diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index c8455ef3d97..57386070397 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -6,6 +6,7 @@ import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" import { Task } from "../task/Task" import { formatResponse } from "../prompts/responses" +import { getApiMetrics } from "../../shared/getApiMetrics" import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { fileExistsAtPath, createDirectoriesForFile } from "../../utils/fs" import { stripLineNumbers, everyLineHasLineNumbers } from "../../integrations/misc/extract-text" @@ -42,7 +43,29 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { if (newContent === undefined) { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") - pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "content")) + + // Use progressive error with context window awareness to break retry loops + const contextWindow = task.api.getModel().info.contextWindow ?? 128_000 + const tokenUsage = getApiMetrics(task.combineMessages(task.clineMessages.slice(1))) + const totalTokens = (tokenUsage.totalTokensIn ?? 0) + (tokenUsage.totalTokensOut ?? 0) + const contextUsagePercent = contextWindow > 0 ? Math.round((totalTokens / contextWindow) * 100) : undefined + + const errorMessage = formatResponse.writeToFileMissingContentError( + relPath || "unknown", + task.consecutiveMistakeCount, + contextUsagePercent, + ) + + await task.say( + "error", + `Roo tried to use write_to_file for '${relPath || "unknown"}' without value for required parameter 'content'. ${ + task.consecutiveMistakeCount >= 2 + ? "This has happened multiple times -- Roo will try a different approach." + : "Retrying..." + }`, + ) + + pushToolResult(formatResponse.toolError(errorMessage)) await task.diffViewProvider.reset() return } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 6c63387ee10..587e3b83216 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -36,6 +36,10 @@ vi.mock("../../prompts/responses", () => ({ toolError: vi.fn((msg) => `Error: ${msg}`), rooIgnoreError: vi.fn((path) => `Access denied: ${path}`), createPrettyPatch: vi.fn(() => "mock-diff"), + writeToFileMissingContentError: vi.fn( + (relPath: string, failureCount: number, contextUsagePercent?: number) => + `Progressive error for ${relPath} (attempt ${failureCount})`, + ), }, })) @@ -62,6 +66,16 @@ vi.mock("../../../integrations/misc/extract-text", () => ({ ), })) +vi.mock("../../../shared/getApiMetrics", () => ({ + getApiMetrics: vi.fn().mockReturnValue({ + totalTokensIn: 50000, + totalTokensOut: 10000, + totalCacheWrites: 0, + totalCacheReads: 0, + totalCost: 0, + }), +})) + vi.mock("vscode", () => ({ window: { showWarningMessage: vi.fn().mockResolvedValue(undefined), @@ -171,8 +185,10 @@ describe("writeToFileTool", () => { }), } mockCline.api = { - getModel: vi.fn().mockReturnValue({ id: "claude-3" }), + getModel: vi.fn().mockReturnValue({ id: "claude-3", info: { contextWindow: 200_000 } }), } + mockCline.clineMessages = [] + mockCline.combineMessages = vi.fn().mockReturnValue([]) mockCline.fileContextTracker = { trackFileContext: vi.fn().mockResolvedValue(undefined), } @@ -328,7 +344,7 @@ describe("writeToFileTool", () => { }) it("unescapes HTML entities for non-Claude models", async () => { - mockCline.api.getModel.mockReturnValue({ id: "gpt-4" }) + mockCline.api.getModel.mockReturnValue({ id: "gpt-4", info: { contextWindow: 128_000 } }) await executeWriteFileTool({ content: "<test>" })