diff --git a/src/core/prompts/responses.ts b/src/core/prompts/responses.ts index 60b5b4123a..a2f6c205fa 100644 --- a/src/core/prompts/responses.ts +++ b/src/core/prompts/responses.ts @@ -66,6 +66,21 @@ 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}` }, + applyDiffMissingDiffError: () => { + const instructions = getToolInstructionsReminder() + + return `Missing value for required parameter 'diff'. This most commonly happens when the diff content is too large and your response was truncated before the 'diff' parameter could be fully generated. + +To recover, try one of these approaches: +1. **Apply smaller diffs**: Break your changes into multiple smaller apply_diff calls, each targeting a specific section of the file. +2. **Use edit_file instead**: For simple search-and-replace changes, edit_file may be more efficient and less likely to be truncated. +3. **Reduce the scope**: Focus on the most critical changes first and apply them incrementally. + +Do NOT simply retry apply_diff with the same large diff — it will likely fail again for the same reason. + +${instructions}` + }, + invalidMcpToolArgumentError: (serverName: string, toolName: string) => JSON.stringify({ status: "error", diff --git a/src/core/tools/ApplyDiffTool.ts b/src/core/tools/ApplyDiffTool.ts index 3b664b3bd2..c77332433b 100644 --- a/src/core/tools/ApplyDiffTool.ts +++ b/src/core/tools/ApplyDiffTool.ts @@ -43,7 +43,13 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { if (!diffContent) { task.consecutiveMistakeCount++ task.recordToolError("apply_diff") - pushToolResult(await task.sayAndCreateMissingParamError("apply_diff", "diff")) + await task.say( + "error", + `Roo tried to use apply_diff${ + relPath ? ` for '${relPath.toPosix()}'` : "" + } without value for required parameter 'diff'. This is likely due to output token limits. Retrying...`, + ) + pushToolResult(formatResponse.toolError(formatResponse.applyDiffMissingDiffError())) return } diff --git a/src/core/tools/__tests__/applyDiffTool.spec.ts b/src/core/tools/__tests__/applyDiffTool.spec.ts new file mode 100644 index 0000000000..5805ce2c83 --- /dev/null +++ b/src/core/tools/__tests__/applyDiffTool.spec.ts @@ -0,0 +1,191 @@ +import * as path from "path" + +import { fileExistsAtPath } from "../../../utils/fs" +import { getReadablePath } from "../../../utils/path" +import { unescapeHtmlEntities } from "../../../utils/text-normalization" +import { ToolUse } from "../../../shared/tools" +import { applyDiffTool } from "../ApplyDiffTool" + +vi.mock("path", async () => { + const originalPath = await vi.importActual("path") + return { + ...originalPath, + resolve: vi.fn().mockImplementation((...args) => { + const separator = process.platform === "win32" ? "\\" : "/" + return args.join(separator) + }), + } +}) + +vi.mock("delay", () => ({ + default: vi.fn(), +})) + +vi.mock("../../../utils/fs", () => ({ + fileExistsAtPath: vi.fn().mockResolvedValue(true), + createDirectoriesForFile: vi.fn().mockResolvedValue([]), +})) + +vi.mock("../../prompts/responses", () => ({ + formatResponse: { + toolError: vi.fn((msg) => `Error: ${msg}`), + rooIgnoreError: vi.fn((path) => `Access denied: ${path}`), + createPrettyPatch: vi.fn(() => "mock-diff"), + applyDiffMissingDiffError: vi.fn( + () => + "Missing value for required parameter 'diff'. This most commonly happens when the diff content is too large and your response was truncated.", + ), + }, +})) + +vi.mock("../../../utils/pathUtils", () => ({ + isPathOutsideWorkspace: vi.fn().mockReturnValue(false), +})) + +vi.mock("../../../utils/path", () => ({ + getReadablePath: vi.fn().mockReturnValue("test/file.txt"), +})) + +vi.mock("../../../utils/text-normalization", () => ({ + unescapeHtmlEntities: vi.fn().mockImplementation((content) => content), +})) + +vi.mock("vscode", () => ({ + window: { + showWarningMessage: vi.fn().mockResolvedValue(undefined), + }, + env: { + openExternal: vi.fn(), + }, + Uri: { + parse: vi.fn(), + }, +})) + +vi.mock("../../ignore/RooIgnoreController", () => ({ + RooIgnoreController: class { + initialize() { + return Promise.resolve() + } + validateAccess() { + return true + } + }, +})) + +describe("applyDiffTool", () => { + const testFilePath = "test/file.txt" + const testDiffContent = `--- a/test/file.txt ++++ b/test/file.txt +@@ -1,3 +1,3 @@ + line 1 +-line 2 ++line 2 modified + line 3` + + const mockCline: any = {} + let mockAskApproval: ReturnType + let mockHandleError: ReturnType + let mockPushToolResult: ReturnType + + beforeEach(() => { + vi.clearAllMocks() + applyDiffTool.resetPartialState() + + mockCline.cwd = "/" + mockCline.consecutiveMistakeCount = 0 + mockCline.consecutiveMistakeCountForApplyDiff = new Map() + mockCline.didEditFile = false + mockCline.diffStrategy = undefined + mockCline.providerRef = { + deref: vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + }), + }), + } + mockCline.rooIgnoreController = { + validateAccess: vi.fn().mockReturnValue(true), + } + mockCline.diffViewProvider = { + editType: undefined, + isEditing: false, + originalContent: "", + open: vi.fn().mockResolvedValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + reset: vi.fn().mockResolvedValue(undefined), + revertChanges: vi.fn().mockResolvedValue(undefined), + saveChanges: vi.fn().mockResolvedValue({ + newProblemsMessage: "", + userEdits: null, + finalContent: "final content", + }), + scrollToFirstDiff: vi.fn(), + updateDiagnosticSettings: vi.fn(), + } + mockCline.api = { + getModel: vi.fn().mockReturnValue({ id: "claude-3" }), + } + mockCline.fileContextTracker = { + trackFileContext: vi.fn().mockResolvedValue(undefined), + } + mockCline.say = vi.fn().mockResolvedValue(undefined) + mockCline.ask = vi.fn().mockResolvedValue(undefined) + mockCline.recordToolError = vi.fn() + mockCline.sayAndCreateMissingParamError = vi.fn().mockResolvedValue("Missing param error") + + mockAskApproval = vi.fn().mockResolvedValue(true) + mockHandleError = vi.fn().mockResolvedValue(undefined) + mockPushToolResult = vi.fn() + }) + + describe("missing parameter handling", () => { + it("returns enhanced error when diff is missing, suggesting alternatives to large diffs", async () => { + const toolUse: ToolUse = { + type: "tool_use", + name: "apply_diff", + params: { path: testFilePath }, + nativeArgs: { path: testFilePath, diff: undefined } as any, + partial: false, + } + + await applyDiffTool.handle(mockCline, toolUse as ToolUse<"apply_diff">, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockCline.consecutiveMistakeCount).toBe(1) + expect(mockCline.recordToolError).toHaveBeenCalledWith("apply_diff") + expect(mockCline.say).toHaveBeenCalledWith( + "error", + expect.stringContaining("without value for required parameter 'diff'"), + ) + expect(mockCline.say).toHaveBeenCalledWith("error", expect.stringContaining("output token limits")) + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("truncated")) + // Should NOT call the generic sayAndCreateMissingParamError + expect(mockCline.sayAndCreateMissingParamError).not.toHaveBeenCalled() + }) + + it("returns generic error when path is missing", async () => { + const toolUse: ToolUse = { + type: "tool_use", + name: "apply_diff", + params: {}, + nativeArgs: { path: undefined, diff: testDiffContent } as any, + partial: false, + } + + await applyDiffTool.handle(mockCline, toolUse as ToolUse<"apply_diff">, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockCline.consecutiveMistakeCount).toBe(1) + expect(mockCline.recordToolError).toHaveBeenCalledWith("apply_diff") + expect(mockCline.sayAndCreateMissingParamError).toHaveBeenCalledWith("apply_diff", "path") + }) + }) +})