diff --git a/apps/vscode-e2e/src/fixtures/apply-diff.ts b/apps/vscode-e2e/src/fixtures/apply-diff.ts index adc604391b..35991d851a 100644 --- a/apps/vscode-e2e/src/fixtures/apply-diff.ts +++ b/apps/vscode-e2e/src/fixtures/apply-diff.ts @@ -31,7 +31,7 @@ export function addApplyDiffResultFixtures(mock: InstanceType) { }, { toolCallId: "call_apply_diff_error_001", - expected: ["No sufficiently similar match found at line: 1", "This content does not exist"], + expected: ["No sufficiently similar match found at line: 1", "- Best Match Location: "], result: "The apply_diff operation on `apply-diff-tool-fixture/error-handling.txt` was rejected - the search content did not match any content in the file, so it was not modified.", id: "call_apply_diff_error_002", }, diff --git a/src/core/diff/strategies/__tests__/multi-search-replace-marker-hint.spec.ts b/src/core/diff/strategies/__tests__/multi-search-replace-marker-hint.spec.ts new file mode 100644 index 0000000000..c38b2e24f2 --- /dev/null +++ b/src/core/diff/strategies/__tests__/multi-search-replace-marker-hint.spec.ts @@ -0,0 +1,183 @@ +import { MultiSearchReplaceDiffStrategy } from "../multi-search-replace" + +// Guards the POST-FAILURE "LIKELY CAUSE" hint for mistyped start-line markers. +// The heuristic must never act as an upfront rejection gate. +describe("MultiSearchReplaceDiffStrategy malformed start-line marker hint", () => { + let strategy: MultiSearchReplaceDiffStrategy + + beforeEach(() => { + strategy = new MultiSearchReplaceDiffStrategy() + }) + + const nonMatchingFile = ["alpha", "beta", "gamma", "delta", "epsilon"].join("\n") + + function buildDiff(searchBody: string, replaceBody: string): string { + return `<<<<<<< SEARCH\n${searchBody}\n=======\n${replaceBody}\n>>>>>>> REPLACE` + } + + function firstFailureError(result: Awaited>): string { + expect(result.success).toBe(false) + const failures = result.failParts ?? [] + expect(failures.length).toBeGreaterThan(0) + const failure = failures[0] + if (failure.success || !failure.error) { + throw new Error("Expected a failed block with error details") + } + return failure.error + } + + it("adds LIKELY CAUSE when the first search line is a bare ':3' marker and the block fails", async () => { + const diffContent = buildDiff(":3\nnot-in-the-file", "replacement") + const result = await strategy.applyDiff(nonMatchingFile, diffContent) + + const errorText = firstFailureError(result) + expect(errorText).toContain("LIKELY CAUSE") + expect(errorText).toContain(":start_line:") + }) + + const markerVariants = ["start_line:3", ":start-line 3", ": start_line : 3", ":START_LINE:3", "Start_Line: 3"] + + it.each(markerVariants)("adds LIKELY CAUSE for the malformed marker variant %j", async (markerVariant) => { + const diffContent = buildDiff(`${markerVariant}\nnot-in-the-file`, "replacement") + const result = await strategy.applyDiff(nonMatchingFile, diffContent) + + expect(firstFailureError(result)).toContain("LIKELY CAUSE") + }) + + it.each([": 3", ":42", "startline:3", "start_line:42"])( + "adds LIKELY CAUSE for the spacing, multi-digit or separator-less marker %j", + async (markerVariant) => { + const diffContent = buildDiff(`${markerVariant}\nnot-in-the-file`, "replacement") + const result = await strategy.applyDiff(nonMatchingFile, diffContent) + + const errorText = firstFailureError(result) + expect(errorText).toContain("first line of your search content looks like a malformed start-line marker") + expect(errorText).not.toContain(markerVariant) + }, + ) + + it("skips the marker hint for an overlong whitespace-padded first line", async () => { + const paddedLine = `startline${" ".repeat(40_000)}3` + const diffContent = buildDiff(`${paddedLine}\nnot-in-the-file`, "replacement") + + const startedAt = performance.now() + const errorText = firstFailureError(await strategy.applyDiff(nonMatchingFile, diffContent)) + + expect(errorText).not.toContain("LIKELY CAUSE") + expect(performance.now() - startedAt).toBeLessThan(2_000) + }) + + it.each([ + { length: 64, hinted: true }, + { length: 65, hinted: false }, + ])("hints only while the first line fits the length guard ($length chars)", async ({ length, hinted }) => { + const firstLine = `startline${" ".repeat(length - 10)}3` + expect(firstLine).toHaveLength(length) + const diffContent = buildDiff(`${firstLine}\nnot-in-the-file`, "replacement") + const errorText = firstFailureError(await strategy.applyDiff(nonMatchingFile, diffContent)) + + expect(errorText.includes("LIKELY CAUSE")).toBe(hinted) + }) + + it.each(["value:3", ":3 trailing", "x start_line:3"])( + "omits LIKELY CAUSE when %j only contains a marker-like fragment", + async (firstLine) => { + const diffContent = buildDiff(`${firstLine}\nnot-in-the-file`, "replacement") + const result = await strategy.applyDiff(nonMatchingFile, diffContent) + + expect(firstFailureError(result)).not.toContain("LIKELY CAUSE") + }, + ) + + it("does not claim a separator was swallowed when none follows the marker", async () => { + const diffContent = buildDiff(":3\nnot-in-the-file", "replacement") + const result = await strategy.applyDiff(nonMatchingFile, diffContent) + + const errorText = firstFailureError(result) + expect(errorText).toContain( + "looks like a malformed start-line marker and was therefore searched for as literal file text. The only recognized form", + ) + expect(errorText).not.toContain("together with") + }) + + it("detects a swallowed separator even when it is indented", async () => { + const diffContent = buildDiff(" :start_line:3\n -------\nnot-in-the-file", "replacement") + const result = await strategy.applyDiff(nonMatchingFile, diffContent) + + expect(firstFailureError(result)).toContain('together with the "-------" line after it') + }) + + it("reports the real similarity, threshold and distance for a near miss", async () => { + const diffContent = buildDiff("alpha\nbetx", "replacement") + const result = await strategy.applyDiff(nonMatchingFile, diffContent) + + const errorText = firstFailureError(result) + const similarity = /\((\d+)% similar, needs 100%\)\n\nDebug Info:\n- Similarity Score: (\d+)%\n/.exec(errorText) + expect(similarity).not.toBeNull() + expect(Number(similarity?.[1])).toBeGreaterThan(50) + expect(similarity?.[2]).toBe(similarity?.[1]) + expect(errorText).toMatch(/- Levenshtein Distance: [1-9]\d* characters\n/) + }) + + it("still applies cleanly when the file genuinely contains a ':3' line (hint is not a gate)", async () => { + const fileWithMarkerLikeLine = ["alpha", ":3", "beta", "gamma"].join("\n") + const diffContent = buildDiff(":3\nbeta", ":3\nBETA") + + const result = await strategy.applyDiff(fileWithMarkerLikeLine, diffContent) + + expect(result.success).toBe(true) + if (!result.success) { + throw new Error("Expected marker-like file content to match") + } + expect(result.content).toBe(["alpha", ":3", "BETA", "gamma"].join("\n")) + expect(result.content).not.toContain("LIKELY CAUSE") + expect(result.failParts ?? []).toHaveLength(0) + }) + + it("reports the parsed block count when repair splits an inline SEARCH marker", async () => { + const original = ["alpha", "beta"].join("\n") + const diffContent = "<<<<<<< SEARCH\nalpha\n=======\nALPHA x <<<<<<< SEARCH\nmissing\n>>>>>>> REPLACE" + + const result = await strategy.applyDiff(original, diffContent) + + expect(result.success).toBe(true) + if (!result.success) { + throw new Error("Expected the first repaired block to apply") + } + expect(result.content).toBe(["ALPHA x", "beta"].join("\n")) + expect(result.blockCount).toBe(2) + expect(result.failParts?.filter((part) => !part.success)).toHaveLength(1) + }) + + it("omits the hint for ordinary non-matching content but keeps the file-changed tip", async () => { + const diffContent = buildDiff("completely unrelated content\nsecond line", "replacement") + const result = await strategy.applyDiff(nonMatchingFile, diffContent) + + const errorText = firstFailureError(result) + expect(errorText).not.toContain("LIKELY CAUSE") + expect(errorText).toContain( + "- Tried both standard and aggressive line number stripping\n" + + "- Tip: Use the read_file tool to get the latest content of the file before attempting " + + "to use the apply_diff tool again, as the file content may have changed\n" + + "- Tip: The SEARCH block must contain complete, whole lines — partial-line (substring) matching is not supported. " + + "If the target line is very long, copy the entire line into the SEARCH block", + ) + }) + + it("does not flag the correct ':start_line:3' marker form", async () => { + const diffContent = `<<<<<<< SEARCH\n:start_line:3\n-------\nnot-in-the-file\n=======\nreplacement\n>>>>>>> REPLACE` + const result = await strategy.applyDiff(nonMatchingFile, diffContent) + + expect(firstFailureError(result)).not.toContain("LIKELY CAUSE") + }) + + it("adds LIKELY CAUSE when a well-formed ':start_line:N' line goes unparsed and swallows its separator", async () => { + // Indenting the marker makes the parser miss it, so both it and the "-------" become search text. + const diffContent = buildDiff(" :start_line:3\n-------\nnot-in-the-file", "replacement") + const result = await strategy.applyDiff(nonMatchingFile, diffContent) + + const errorText = firstFailureError(result) + expect(errorText).toContain("LIKELY CAUSE") + expect(errorText).toContain('together with the "-------" line after it') + }) +}) diff --git a/src/core/diff/strategies/__tests__/multi-search-replace.spec.ts b/src/core/diff/strategies/__tests__/multi-search-replace.spec.ts index 07ffc419a4..81adddb9c9 100644 --- a/src/core/diff/strategies/__tests__/multi-search-replace.spec.ts +++ b/src/core/diff/strategies/__tests__/multi-search-replace.spec.ts @@ -900,7 +900,7 @@ replacement content here expect(error).toContain("No sufficiently similar match found") expect(error).toContain("at line: 999") expect(error).toContain("Search Range: starting at line 999") - expect(error).toContain("Best Match Found:\n(no match)") + expect(error).toContain("Best Match Location: none") expect(error).toContain("Levenshtein Distance: N/A") expect(error).toContain("Best Match Length: 0 characters") }) @@ -928,7 +928,7 @@ replacement content here expect(error).toContain("Tip: The SEARCH block must contain complete, whole lines") }) - it("should include scoped original content when search fails with start_line that has a low-score match", async () => { + it("reports only numeric diagnostics when search fails with start_line that has a low-score match", async () => { const originalContent = "function existing() {\n return 42;\n}\n" const diffContent = `test.ts <<<<<<< SEARCH @@ -954,8 +954,9 @@ function newVersion() { expect(error).toContain("No sufficiently similar match found") expect(error).toContain("at line: 1") expect(error).toContain("Search Range: starting at line 1") - expect(error).toContain("Best Match Found:") - expect(error).toContain("Original Content:\n1 | function existing()") + expect(error).toContain("Best Match Location: lines 1-") + expect(error).not.toContain("function existing()") + expect(error).not.toContain("function different()") }) it("should include best-match debug info when unscoped search is below threshold", async () => { @@ -984,7 +985,9 @@ function processUsers(data) { expect(error).toContain("Levenshtein Distance:") expect(error).toContain("characters") expect(error).toContain("Best Match Length:") - expect(error).toContain("Best Match Found:\n1 | function processUsers(data)") + expect(error).toContain("Best Match Location: lines 1-3") + expect(error).not.toContain("user.username") + expect(error).not.toContain("user.name") }) it("should include zero-match info when unscoped search finds no similarity at all", async () => { @@ -1007,11 +1010,24 @@ aaaaaa : "" expect(error).toContain("No sufficiently similar match found") expect(error).toContain("Search Range: start to end") - expect(error).toContain("Best Match Found:\n(no match)") + expect(error).toContain("Best Match Location: none") expect(error).toContain("Levenshtein Distance: N/A") expect(error).toContain("Best Match Length: 0 characters") - expect(error).toContain("Original Content:") - expect(error).toContain("1 | xxxxxx") + expect(error).not.toContain("xxxxxx") + expect(error).not.toContain("!!!!!!") + }) + + it("never echoes search, best-match or surrounding file text in failure diagnostics", async () => { + const originalContent = "const a = 1\nAPI_KEY=FILE-SECRET-1\nconst b = 2\nAPI_KEY=FILE-SECRET-2\n" + const diffContent = + "<<<<<<< SEARCH\n:start_line:2\n-------\nAPI_KEY=SEARCH-SECRET-0\n=======\nAPI_KEY=x\n>>>>>>> REPLACE" + + const result = await strategy.applyDiff(originalContent, diffContent) + const serialized = JSON.stringify(result) + + expect(result.success).toBe(false) + expect(serialized).toContain("No sufficiently similar match found") + expect(serialized).not.toMatch(/FILE-SECRET|SEARCH-SECRET/) }) it("should match content with extra whitespace", async () => { @@ -1610,7 +1626,7 @@ function sum(a, b) { expect(failedPart).toHaveProperty("error") const errorMsg = (failedPart as { error: string }).error expect(errorMsg).toContain("No sufficiently similar match found") - expect(errorMsg).toContain("Best Match Found:") + expect(errorMsg).toContain("Best Match Location:") expect(errorMsg).toContain("Levenshtein Distance:") expect(errorMsg).toContain("Search Range: start to end") }) diff --git a/src/core/diff/strategies/multi-search-replace.ts b/src/core/diff/strategies/multi-search-replace.ts index cac52c5486..babe60938d 100644 --- a/src/core/diff/strategies/multi-search-replace.ts +++ b/src/core/diff/strategies/multi-search-replace.ts @@ -2,11 +2,13 @@ import { distance } from "fastest-levenshtein" import { ToolProgressStatus, DEFAULT_DIFF_FUZZY_THRESHOLD } from "@roo-code/types" -import { addLineNumbers, everyLineHasLineNumbers, stripLineNumbers } from "../../../integrations/misc/extract-text" +import { everyLineHasLineNumbers, stripLineNumbers } from "../../../integrations/misc/extract-text" import { ToolUse, DiffStrategy, DiffResult } from "../../../shared/tools" import { normalizeString } from "../../../utils/text-normalization" const BUFFER_LINES = 40 // Number of extra context lines to show before and after matches +// Real malformed markers are short; longer lines skip the hint regex entirely. +const MAX_MARKER_HINT_LINE_LENGTH = 64 function getSimilarity(original: string, search: string): number { // Empty searches are no longer supported @@ -468,8 +470,6 @@ export class MultiSearchReplaceDiffStrategy implements DiffStrategy { continue } - const endLine = replacement.startLine + searchLines.length - 1 - // Initialize search variables let matchIndex = -1 let bestMatchScore = 0 @@ -538,46 +538,47 @@ export class MultiSearchReplaceDiffStrategy implements DiffStrategy { searchLines = aggressiveSearchLines replaceLines = replaceContent ? replaceContent.split(/\r?\n/) : [] } else { - // No match found with either method - const originalContentSection = - startLine && endLine - ? `\n\nOriginal Content:\n${addLineNumbers( - resultLines - .slice( - Math.max(0, startLine - 1 - this.bufferLines), - Math.min(resultLines.length, endLine + this.bufferLines), - ) - .join("\n"), - Math.max(1, startLine - this.bufferLines), - )}` - : `\n\nOriginal Content:\n${addLineNumbers( - resultLines - .slice( - Math.max(0, (matchIndex >= 0 ? matchIndex : 0) - this.bufferLines), - Math.min( - resultLines.length, - (matchIndex >= 0 ? matchIndex : 0) + - searchLines.length + - this.bufferLines, - ), - ) - .join("\n"), - Math.max(1, (matchIndex >= 0 ? matchIndex : 0) - this.bufferLines + 1), - )}` - - const bestMatchSection = bestMatchContent - ? `\n\nBest Match Found:\n${addLineNumbers(bestMatchContent, matchIndex + 1)}` - : `\n\nBest Match Found:\n(no match)` - + // File and search text are never echoed: they may hold secrets. Report only numbers and constants. const lineRange = startLine ? ` at line: ${startLine}` : "" + const bestMatchLocation = + bestMatchContent && matchIndex >= 0 + ? `lines ${matchIndex + 1}-${matchIndex + bestMatchContent.split("\n").length}` + : "none" const levenDist = bestMatchContent ? distance(normalizeString(searchChunk), normalizeString(bestMatchContent)) : -1 + // Only diagnose markers after matching fails: marker-like text can be valid file content. + const hintLines = searchChunk.split("\n") + const firstSearchLine = hintLines[0]?.trim() ?? "" + const separatorSwallowed = hintLines[1]?.trim() === "-------" + // Length guard: adjacent `\s*` groups backtrack quadratically on long whitespace runs. + const looksLikeMalformedMarker = + firstSearchLine.length <= MAX_MARKER_HINT_LINE_LENGTH && + /^(?::\s*\d{1,9}:?|:?\s*start[_-]?line\s*:?\s*\d{1,9}:?)$/i.test(firstSearchLine) + const malformedMarkerHint = looksLikeMalformedMarker + ? `\n- LIKELY CAUSE: the first line of your search content looks like a malformed start-line marker and was therefore searched for as literal file text${separatorSwallowed ? ', together with the "-------" line after it' : ""}. The only recognized form is ":start_line:" on the line immediately after "<<<<<<< SEARCH", optionally followed by a "-------" line. Remove or correct that line and retry.` + : "" + diffResults.push({ success: false, - error: `No sufficiently similar match found${lineRange} (${Math.floor(bestMatchScore * 100)}% similar, needs ${Math.floor(this.fuzzyThreshold * 100)}%)\n\nDebug Info:\n- Similarity Score: ${Math.floor(bestMatchScore * 100)}%\n- Required Threshold: ${Math.floor(this.fuzzyThreshold * 100)}%\n- Search Range: ${startLine ? `starting at line ${startLine}` : "start to end"}\n- Levenshtein Distance: ${levenDist >= 0 ? `${levenDist} characters` : "N/A"}\n- Search Length: ${searchChunk.length} characters\n- Best Match Length: ${bestMatchContent ? bestMatchContent.length : 0} characters\n- Tried both standard and aggressive line number stripping\n- Tip: Use the read_file tool to get the latest content of the file before attempting to use the apply_diff tool again, as the file content may have changed\n- Tip: The SEARCH block must contain complete, whole lines — partial-line (substring) matching is not supported. If the target line is very long, copy the entire line into the SEARCH block\n\nSearch Content:\n${searchChunk}${bestMatchSection}${originalContentSection}`, + error: + `No sufficiently similar match found${lineRange} ` + + `(${Math.floor(bestMatchScore * 100)}% similar, needs ${Math.floor(this.fuzzyThreshold * 100)}%)\n\n` + + `Debug Info:\n` + + `- Similarity Score: ${Math.floor(bestMatchScore * 100)}%\n` + + `- Required Threshold: ${Math.floor(this.fuzzyThreshold * 100)}%\n` + + `- Search Range: ${startLine ? `starting at line ${startLine}` : "start to end"}\n` + + `- Levenshtein Distance: ${levenDist >= 0 ? `${levenDist} characters` : "N/A"}\n` + + `- Search Length: ${searchChunk.length} characters\n` + + `- Best Match Location: ${bestMatchLocation}\n` + + `- Best Match Length: ${bestMatchContent ? bestMatchContent.length : 0} characters\n` + + `- Tried both standard and aggressive line number stripping${malformedMarkerHint}\n` + + `- Tip: Use the read_file tool to get the latest content of the file before attempting ` + + `to use the apply_diff tool again, as the file content may have changed\n` + + `- Tip: The SEARCH block must contain complete, whole lines — partial-line (substring) matching is not supported. ` + + `If the target line is very long, copy the entire line into the SEARCH block`, }) continue } @@ -641,6 +642,7 @@ export class MultiSearchReplaceDiffStrategy implements DiffStrategy { success: true, content: finalContent, failParts: diffResults, + blockCount: matches.length, } } diff --git a/src/core/tools/ApplyDiffTool.ts b/src/core/tools/ApplyDiffTool.ts index 3b664b3bd2..4011f802e9 100644 --- a/src/core/tools/ApplyDiffTool.ts +++ b/src/core/tools/ApplyDiffTool.ts @@ -12,7 +12,7 @@ import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { unescapeHtmlEntities } from "../../utils/text-normalization" import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" import { computeDiffStats, sanitizeUnifiedDiff } from "../diff/stats" -import type { ToolUse } from "../../shared/tools" +import type { DiffResult, ToolUse } from "../../shared/tools" import { BaseTool, ToolCallbacks } from "./BaseTool" @@ -21,6 +21,11 @@ interface ApplyDiffParams { diff: string } +const DETAILED_APPLY_DIFF_FAILURES = 5 +const MAX_DETAILED_FAILURE_CHARACTERS = 4000 +const MAX_SUMMARY_FAILURE_CHARACTERS = 200 +const MAX_ABBREVIATED_FAILURES = 50 + export class ApplyDiffTool extends BaseTool<"apply_diff"> { readonly name = "apply_diff" as const @@ -88,23 +93,19 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { TelemetryService.instance.captureDiffApplicationError(task.taskId, currentCount) if (diffResult.failParts && diffResult.failParts.length > 0) { - for (const failPart of diffResult.failParts) { - if (failPart.success) { - continue - } - - const errorDetails = failPart.details ? JSON.stringify(failPart.details, null, 2) : "" + const failedParts = diffResult.failParts.filter((part) => !part.success) + const summary = + failedParts.length > 1 + ? `${failedParts.length} diff blocks failed to apply. Fix the failures below before retrying.\n\n` + : "" - formattedError = `\n${ - failPart.error - }${errorDetails ? `\n\nDetails:\n${errorDetails}` : ""}\n` - } + formattedError = `Unable to apply diff to file: ${absolutePath}\n\n\n${summary}${this.formatFailedParts(failedParts)}\n` } else { - const errorDetails = diffResult.details ? JSON.stringify(diffResult.details, null, 2) : "" - - formattedError = `Unable to apply diff to file: ${absolutePath}\n\n\n${ - diffResult.error - }${errorDetails ? `\n\nDetails:\n${errorDetails}` : ""}\n` + // `details` may carry search/best-match file text, so it is never forwarded. + formattedError = `Unable to apply diff to file: ${absolutePath}\n\n\n${this.truncate( + diffResult.error ?? "Unknown apply_diff failure", + MAX_DETAILED_FAILURE_CHARACTERS, + )}\n` } if (currentCount >= 2) { @@ -231,17 +232,27 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { // Used to determine if we should wait for busy terminal to update before sending api request task.didEditFile = true + + // Prefer the parser's count: truncation repair can split inline markers into extra blocks. + const searchBlocks = + diffResult.blockCount ?? (diffContent.match(/^<<<<<<< SEARCH>?[^\S\r\n]*\r?$/gm) || []).length + const unappliedParts = diffResult.failParts?.filter((part) => !part.success) ?? [] let partFailHint = "" - if (diffResult.failParts && diffResult.failParts.length > 0) { - partFailHint = `But unable to apply all diff parts to file: ${absolutePath}. Use the read_file tool to check the newest file version and re-apply diffs.\n` + if (unappliedParts.length > 0) { + const appliedBlocks = Math.max(0, searchBlocks - unappliedParts.length) + partFailHint = + `Applied ${appliedBlocks} of ${searchBlocks} diff blocks to ${absolutePath}. ` + + `The other ${unappliedParts.length} did NOT apply. ` + + `Inspect the saved file before retrying, since an earlier block may already contain the intended change. ` + + `Then resend ONLY the failed blocks that are still needed; the blocks that already applied will no longer match.\n\n` + + `\n${this.formatFailedParts(unappliedParts)}\n\n\n` } // Get the formatted response message const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, !fileExists) // Check for single SEARCH/REPLACE block warning - const searchBlocks = (diffContent.match(/<<<<<<< SEARCH/g) || []).length const singleBlockNotice = searchBlocks === 1 ? "\nMaking multiple related changes in a single apply_diff is more efficient. If other changes are needed in this file, please include them as additional SEARCH/REPLACE blocks." @@ -269,6 +280,49 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { } } + /** Strategy errors are file-content-free diagnostics; `details` may hold file text and is never rendered. */ + private formatFailedParts(failedParts: Extract[]): string { + const renderedCount = Math.min(failedParts.length, MAX_ABBREVIATED_FAILURES) + const sections = failedParts.slice(0, renderedCount).map((failPart, index) => { + const label = failedParts.length > 1 ? `--- Failure ${index + 1} of ${failedParts.length} ---\n` : "" + const error = failPart.error ?? "Unknown apply_diff failure" + + if (index >= DETAILED_APPLY_DIFF_FAILURES) { + const summaryLine = error.split("\n")[0] + return `${label}${this.truncate(summaryLine, MAX_SUMMARY_FAILURE_CHARACTERS)}` + } + + return this.truncate(`${label}${error}`, MAX_DETAILED_FAILURE_CHARACTERS) + }) + + const trailers: string[] = [] + const abbreviatedRangeStart = DETAILED_APPLY_DIFF_FAILURES + 1 + + if (renderedCount > DETAILED_APPLY_DIFF_FAILURES) { + trailers.push( + `(Failures ${abbreviatedRangeStart}-${renderedCount} are shown as a single summary line each; retry those blocks individually for full detail.)`, + ) + } + + const omitted = failedParts.length - renderedCount + + if (omitted > 0) { + trailers.push(`(+${omitted} more failures not shown; retry those blocks individually for full detail.)`) + } + + return [sections.join("\n\n"), ...trailers].join("\n\n") + } + + private truncate(text: string, maxCharacters: number): string { + if (text.length <= maxCharacters) { + return text + } + + const omitted = text.length - maxCharacters + + return `${text.slice(0, maxCharacters)}\n... [truncated; ${omitted} characters omitted — retry this block alone for full detail]` + } + override async handlePartial(task: Task, block: ToolUse<"apply_diff">): Promise { const relPath: string | undefined = block.params.path const diffContent: string | undefined = block.params.diff diff --git a/src/core/tools/__tests__/applyDiffTool.failureReporting.spec.ts b/src/core/tools/__tests__/applyDiffTool.failureReporting.spec.ts new file mode 100644 index 0000000000..bf7b3f5351 --- /dev/null +++ b/src/core/tools/__tests__/applyDiffTool.failureReporting.spec.ts @@ -0,0 +1,517 @@ +import fs from "fs/promises" +import path from "path" + +import type { DiffResult, ToolResponse } from "../../../shared/tools" +import type { Task } from "../../task/Task" +import { fileExistsAtPath } from "../../../utils/fs" +import { MultiSearchReplaceDiffStrategy } from "../../diff/strategies/multi-search-replace" +import type { ToolCallbacks } from "../BaseTool" +import { applyDiffTool } from "../ApplyDiffTool" + +vi.mock("@roo-code/telemetry", () => ({ + TelemetryService: { instance: { captureDiffApplicationError: vi.fn() } }, +})) + +vi.mock("fs/promises", () => ({ + default: { + readFile: vi.fn().mockResolvedValue(""), + }, +})) + +vi.mock("../../../utils/fs", () => ({ + fileExistsAtPath: vi.fn().mockResolvedValue(true), +})) + +vi.mock("../../../utils/path", () => ({ + getReadablePath: vi.fn().mockReturnValue("test/file.txt"), +})) + +vi.mock("../../prompts/responses", () => ({ + formatResponse: { + toolError: vi.fn((message: string) => `Error: ${message}`), + rooIgnoreError: vi.fn((filePath: string) => `Access denied: ${filePath}`), + createPrettyPatch: vi.fn(() => "mock-diff"), + }, +})) + +vi.mock("../../diff/stats", () => ({ + sanitizeUnifiedDiff: vi.fn((diff: string) => diff), + computeDiffStats: vi.fn(() => ({ additions: 1, deletions: 1 })), +})) + +vi.mock("vscode", () => ({ + window: { showWarningMessage: vi.fn() }, + env: { openExternal: vi.fn() }, + Uri: { parse: vi.fn() }, +})) + +type FailPart = Extract + +describe("ApplyDiffTool failure reporting", () => { + const testFilePath = "test/file.txt" + + let mockTask: Task + let mockAskApproval: ReturnType> + let mockHandleError: ReturnType> + let pushedResult: ToolResponse | undefined + + beforeEach(() => { + vi.clearAllMocks() + pushedResult = undefined + vi.mocked(fileExistsAtPath).mockResolvedValue(true) + vi.mocked(fs.readFile).mockResolvedValue("line one\nline two\nline three\n") + + mockAskApproval = vi.fn().mockResolvedValue(true) + mockHandleError = vi.fn().mockResolvedValue(undefined) + + // Only the provider, edit tracking, and diff-view members used by execute are needed. + mockTask = { + cwd: "/workspace", + consecutiveMistakeCount: 0, + consecutiveMistakeCountForApplyDiff: new Map(), + didEditFile: false, + api: { getModel: () => ({ id: "claude-test" }) }, + rooIgnoreController: { validateAccess: vi.fn().mockReturnValue(true) }, + rooProtectedController: { isWriteProtected: vi.fn().mockReturnValue(false) }, + providerRef: { + deref: vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 0, + experiments: {}, + }), + }), + }, + diffViewProvider: { + editType: undefined, + 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(undefined), + saveDirectly: vi.fn().mockResolvedValue(undefined), + scrollToFirstDiff: vi.fn(), + pushToolWriteResult: vi.fn().mockResolvedValue("Changes successfully applied."), + }, + fileContextTracker: { trackFileContext: vi.fn().mockResolvedValue(undefined) }, + say: vi.fn().mockResolvedValue(undefined), + ask: vi.fn().mockResolvedValue(undefined), + recordToolError: vi.fn(), + recordToolUsage: vi.fn(), + processQueuedMessages: vi.fn(), + sayAndCreateMissingParamError: vi.fn().mockResolvedValue("Missing param error"), + diffStrategy: undefined, + } as unknown as Task + }) + + function makeFailure(errorText: string | undefined, details?: FailPart["details"]): FailPart { + return { success: false, error: errorText, details, failParts: [] } + } + + function buildDiffContent(blockCount: number): string { + const blocks: string[] = [] + for (let blockIndex = 0; blockIndex < blockCount; blockIndex++) { + blocks.push(`<<<<<<< SEARCH\nsearch-${blockIndex}\n=======\nreplace-${blockIndex}\n>>>>>>> REPLACE`) + } + return blocks.join("\n") + } + + async function runApplyDiff(diffResult: DiffResult, blockCount: number, diffOverride?: string): Promise { + mockTask.diffStrategy = { + getName: () => "test", + applyDiff: vi.fn().mockResolvedValue(diffResult), + getProgressStatus: vi.fn().mockReturnValue({ icon: "diff-multiple", text: "x" }), + } + + const diff = diffOverride ?? buildDiffContent(blockCount) + await applyDiffTool.execute({ path: testFilePath, diff }, mockTask, { + askApproval: mockAskApproval, + handleError: mockHandleError, + toolCallId: "tool-call-1", + pushToolResult: vi.fn((result: ToolResponse) => { + pushedResult = result + }), + }) + + expect(mockHandleError).not.toHaveBeenCalled() + return String(pushedResult ?? "") + } + + function makeFailureSet(count: number): FailPart[] { + const failures: FailPart[] = [] + for (let failureIndex = 1; failureIndex <= count; failureIndex++) { + failures.push(makeFailure(`FAILURE-MARKER-${failureIndex}\nsecond line of failure ${failureIndex}`)) + } + return failures + } + + describe("failure accumulation and rendering", () => { + it("reports every failed block, not just the last one", async () => { + const output = await runApplyDiff({ success: false, failParts: makeFailureSet(2) }, 2) + + expect(output).toContain("FAILURE-MARKER-1") + expect(output).toContain("FAILURE-MARKER-2") + expect(output).toContain("--- Failure 1 of 2 ---") + expect(output).toContain("--- Failure 2 of 2 ---") + expect(output).not.toContain("Diff block") + }) + + it("names the target file before the multi-block failure details", async () => { + const output = await runApplyDiff({ success: false, failParts: makeFailureSet(2) }, 2) + + const expectedPrefix = `Unable to apply diff to file: ${path.resolve("/workspace", testFilePath)}\n\n\n` + expect(output.startsWith(expectedPrefix)).toBe(true) + }) + + it("records every failure and shows the same details on the second consecutive error", async () => { + const result: DiffResult = { success: false, failParts: makeFailureSet(2) } + const firstOutput = await runApplyDiff(result, 2) + expect(mockTask.say).not.toHaveBeenCalled() + expect(mockTask.recordToolError).toHaveBeenLastCalledWith("apply_diff", firstOutput) + + const secondOutput = await runApplyDiff(result, 2) + expect(mockTask.say).toHaveBeenCalledWith("diff_error", secondOutput) + expect(secondOutput).toContain("FAILURE-MARKER-1") + expect(secondOutput).toContain("FAILURE-MARKER-2") + }) + + it("renders exactly five failures in full detail with no abbreviation trailer", async () => { + const output = await runApplyDiff({ success: false, failParts: makeFailureSet(5) }, 5) + + for (let failureIndex = 1; failureIndex <= 5; failureIndex++) { + expect(output).toContain(`FAILURE-MARKER-${failureIndex}`) + expect(output).toContain(`second line of failure ${failureIndex}`) + } + expect(output).not.toContain("are shown as a single summary line each") + }) + + it("abbreviates the sixth failure and emits a trailer naming the range", async () => { + const output = await runApplyDiff({ success: false, failParts: makeFailureSet(6) }, 6) + + expect(output).toContain("FAILURE-MARKER-6") + expect(output).not.toContain("second line of failure 6") + expect(output).toContain("second line of failure 5") + expect(output).toContain("(Failures 6-6 are shown as a single summary line each") + }) + + it("omits the diff block label when there is a single failure", async () => { + const output = await runApplyDiff({ success: false, failParts: makeFailureSet(1) }, 1) + + expect(output).toContain("FAILURE-MARKER-1") + expect(output).not.toContain("--- Diff block") + }) + + it("produces no stray separators for an empty failures array", async () => { + const output = await runApplyDiff({ success: false, failParts: [], error: "nothing applied" }, 1) + + expect(output).not.toContain("--- Diff block") + expect(output).not.toContain("are shown as a single summary line each") + }) + + it("substitutes a placeholder when a failure has no error text", async () => { + const output = await runApplyDiff({ success: false, failParts: [makeFailure(undefined)] }, 1) + + expect(output).toContain("Unknown apply_diff failure") + }) + + it("never renders the details object of any failure", async () => { + const failures = makeFailureSet(6) + failures[0] = makeFailure("FAILURE-MARKER-1\nsecond line of failure 1", { bestMatch: "DETAIL-FIRST" }) + failures[5] = makeFailure("FAILURE-MARKER-6\nsecond line of failure 6", { bestMatch: "DETAIL-SIXTH" }) + + const output = await runApplyDiff({ success: false, failParts: failures }, 6) + + expect(output).not.toContain("Details:") + expect(output).not.toContain("DETAIL-FIRST") + expect(output).not.toContain("DETAIL-SIXTH") + }) + + it("truncates a detailed failure that exceeds the character cap", async () => { + const oversizedError = `FAILURE-MARKER-1\n${"x".repeat(6000)}` + const output = await runApplyDiff({ success: false, failParts: [makeFailure(oversizedError)] }, 1) + + expect(output).toContain("characters omitted — retry this block alone for full detail") + expect(output).not.toContain("x".repeat(5000)) + }) + + it("leaves a detailed failure just under the character cap untruncated", async () => { + const nearCapError = `FAILURE-MARKER-1\n${"x".repeat(3900)}` + const output = await runApplyDiff({ success: false, failParts: [makeFailure(nearCapError)] }, 1) + + expect(output).not.toContain("[truncated") + expect(output).toContain("x".repeat(3900)) + }) + + it("truncates an abbreviated one-line summary that exceeds the summary cap", async () => { + const failures = makeFailureSet(6) + failures[5] = makeFailure(`FAILURE-MARKER-6 ${"y".repeat(500)}\nsecond line of failure 6`) + + const output = await runApplyDiff({ success: false, failParts: failures }, 6) + + expect(output).not.toContain("y".repeat(400)) + expect(output).toContain("characters omitted — retry this block alone for full detail") + }) + + it("emits a real trailer range when eight failures are reported", async () => { + const output = await runApplyDiff({ success: false, failParts: makeFailureSet(8) }, 8) + + expect(output).toContain("(Failures 6-8 are shown as a single summary line each") + expect(output).toContain("FAILURE-MARKER-8") + expect(output).not.toContain("second line of failure 8") + }) + + it("prefixes multiple failures with a count summary that ignores successful parts", async () => { + const mixedParts: DiffResult[] = [{ success: true, content: "applied" }, ...makeFailureSet(2)] + const output = await runApplyDiff({ success: false, failParts: mixedParts }, 3) + + expect(output).toContain( + "\n2 diff blocks failed to apply. Fix the failures below before retrying.\n\n--- Failure 1 of 2 ---\n", + ) + expect(output).toContain("second line of failure 1\n\n--- Failure 2 of 2 ---\nFAILURE-MARKER-2") + }) + + it("renders a single failure verbatim with no summary, label, details or trailer", async () => { + const output = await runApplyDiff({ success: false, failParts: makeFailureSet(1) }, 1) + + expect(output).toContain("\nFAILURE-MARKER-1\nsecond line of failure 1\n") + }) + + it("separates the abbreviated trailer from the last summary and omits the remainder trailer", async () => { + const output = await runApplyDiff({ success: false, failParts: makeFailureSet(6) }, 6) + + expect(output).toContain("FAILURE-MARKER-6\n\n(Failures 6-6 are shown") + expect(output).not.toContain("more failures not shown") + }) + + it("does not truncate a detailed failure of exactly the character cap", async () => { + const exactCapError = "z".repeat(4000) + const output = await runApplyDiff({ success: false, failParts: [makeFailure(exactCapError)] }, 1) + + expect(output).not.toContain("[truncated") + expect(output).toContain(`${exactCapError}\n`) + }) + + it("reports the exact number of omitted characters when truncating", async () => { + const oversizedError = `FAILURE-MARKER-1\n${"x".repeat(6000)}` + const output = await runApplyDiff({ success: false, failParts: [makeFailure(oversizedError)] }, 1) + + expect(output).toContain(`[truncated; ${oversizedError.length - 4000} characters omitted`) + }) + + it("caps the number of abbreviated failures and reports the remainder as a count", async () => { + const output = await runApplyDiff({ success: false, failParts: makeFailureSet(60) }, 60) + + expect(output).toContain("(Failures 6-50 are shown as a single summary line each") + expect(output).toContain("(+10 more failures not shown") + expect(output).not.toContain("FAILURE-MARKER-51") + }) + }) + + describe("file content exposure (real strategy)", () => { + const FILE_SECRETS = ["FILE-SECRET-ORIGINAL", "FILE-SECRET-BESTMATCH"] + const SEARCH_SECRET = "SEARCH-SECRET" + const DETAIL_SECRET = "DETAIL-SECRET" + const fileText = [ + "const header = 1", + `token = "${FILE_SECRETS[0]}"`, + `apiKey = "${FILE_SECRETS[1]}"`, + "const footer = 2", + ].join("\n") + + function failingBlock(index: number): string { + return `<<<<<<< SEARCH\n:start_line:2\n-------\napiKey = "${SEARCH_SECRET}-${index}"\n=======\nx${index}\n>>>>>>> REPLACE` + } + + const applyingBlock = "<<<<<<< SEARCH\nconst header = 1\n=======\nconst header = 3\n>>>>>>> REPLACE" + + async function runRealStrategy(diff: string): Promise { + vi.mocked(fs.readFile).mockResolvedValue(fileText) + const strategy = new MultiSearchReplaceDiffStrategy() + const realResult = await strategy.applyDiff(fileText, diff) + // Synthetic details exercise the tool's own guard against forwarding strategy detail objects. + const withDetails = (part: DiffResult): DiffResult => + part.success ? part : { ...part, details: { searchContent: DETAIL_SECRET, bestMatch: DETAIL_SECRET } } + const result: DiffResult = realResult.success + ? { ...realResult, failParts: realResult.failParts?.map(withDetails) } + : { + ...realResult, + failParts: realResult.failParts?.map(withDetails) ?? [], + details: { bestMatch: DETAIL_SECRET }, + } + return runApplyDiff(result, 0, diff) + } + + function collectUiAndModelOutputs(output: string): string { + const sayText = vi.mocked(mockTask.say).mock.calls.map((call) => String(call[1] ?? "")) + const recordedErrors = vi.mocked(mockTask.recordToolError).mock.calls.map((call) => String(call[1] ?? "")) + return [output, ...sayText, ...recordedErrors].join("\n") + } + + function expectNoSecrets(text: string): void { + for (const secret of [...FILE_SECRETS, SEARCH_SECRET, DETAIL_SECRET]) { + expect(text).not.toContain(secret) + } + expect(text).not.toMatch(/Search Content:|Best Match Found:|Original Content:|Details:/) + } + + it("excludes file, search and detail text from a single failure in model and UI output", async () => { + await runRealStrategy(failingBlock(1)) + const output = await runRealStrategy(failingBlock(1)) + + expect(output).toContain("No sufficiently similar match found at line: 2") + expect(output).toContain("- Similarity Score:") + expect(mockTask.say).toHaveBeenCalledWith("diff_error", output) + expectNoSecrets(collectUiAndModelOutputs(output)) + }) + + it("excludes sensitive text from every block when multiple blocks fail", async () => { + const diff = [1, 2, 3].map(failingBlock).join("\n") + await runRealStrategy(diff) + const output = await runRealStrategy(diff) + + expect(output).toContain("3 diff blocks failed to apply") + expect(output).toContain("--- Failure 3 of 3 ---") + expectNoSecrets(collectUiAndModelOutputs(output)) + }) + + it("excludes sensitive text from the partial-apply result", async () => { + const output = await runRealStrategy(`${applyingBlock}\n${failingBlock(1)}`) + + expect(output).toContain("Applied 1 of 2 diff blocks") + expect(output).toContain("No sufficiently similar match found") + expectNoSecrets(collectUiAndModelOutputs(output)) + // The approval payload intentionally echoes the model's own diff, so only detail text is checked. + for (const call of vi.mocked(mockAskApproval).mock.calls) { + expect(String(call[1])).not.toContain(DETAIL_SECRET) + } + }) + + it("drops top-level details on the fallback path without failParts", async () => { + const output = await runApplyDiff( + { success: false, error: "Invalid diff format", details: { bestMatch: DETAIL_SECRET } }, + 1, + ) + + expect(output).toContain("Invalid diff format") + expectNoSecrets(collectUiAndModelOutputs(output)) + }) + }) + + describe("partial apply path", () => { + function partialResult(failures: FailPart[]): DiffResult { + return { success: true, content: "updated content", failParts: failures } + } + + it("reports how many blocks applied and asks for only the failed blocks to be resent", async () => { + const output = await runApplyDiff(partialResult(makeFailureSet(1)), 2) + + expect(output).toContain("Applied 1 of 2") + expect(output).toContain("FAILURE-MARKER-1") + expect(output).toContain("resend ONLY the failed blocks that are still needed") + }) + + it("does not tell the model to use read_file to re-apply the diffs", async () => { + const output = await runApplyDiff(partialResult(makeFailureSet(1)), 2) + + expect(output).not.toContain("re-apply") + }) + + it.each(["\n", "\r\n"])( + "counts only unescaped line-level SEARCH markers with %j endings", + async (lineEnding) => { + const diff = [ + "<<<<<<< SEARCH> ", + "\\<<<<<<< SEARCH", + "inline <<<<<<< SEARCH", + "<<<<<<< SEARCHING", + "=======", + "replacement", + ">>>>>>> REPLACE", + buildDiffContent(1), + ] + .join("\n") + .replace(/\n/g, lineEnding) + const output = await runApplyDiff(partialResult(makeFailureSet(1)), 2, diff) + + expect(output).toContain("Applied 1 of 2 diff blocks") + }, + ) + + it("prefers the parser-reported block count over the raw marker count", async () => { + const output = await runApplyDiff( + { success: true, content: "updated content", failParts: makeFailureSet(1), blockCount: 2 }, + 1, + buildDiffContent(1), + ) + + expect(output).toContain("Applied 1 of 2 diff blocks") + expect(output).toContain("The other 1 did NOT apply.") + }) + + it("states how many blocks did not apply", async () => { + const output = await runApplyDiff(partialResult(makeFailureSet(2)), 3) + + expect(output).toContain(`Applied 1 of 3 diff blocks to ${path.resolve("/workspace", testFilePath)}. `) + expect(output).toContain( + "The other 2 did NOT apply. Inspect the saved file before retrying, since an earlier block may already contain the intended change. ", + ) + expect(output).not.toContain("NOT in the file") + }) + + it.each([ + { name: "every block fails", failures: 2, blockCount: 2 }, + { name: "failures outnumber parsed blocks", failures: 3, blockCount: 2 }, + ])("reports zero applied blocks when $name", async ({ failures, blockCount }) => { + const output = await runApplyDiff( + { success: true, content: "updated content", failParts: makeFailureSet(failures), blockCount }, + blockCount, + ) + + expect(output).toContain(`Applied 0 of ${blockCount} diff blocks`) + expect(output).toContain(`The other ${failures} did NOT apply.`) + expect(output).not.toMatch(/Applied -\d/) + }) + + it("tolerates a successful result without failParts or SEARCH markers", async () => { + const output = await runApplyDiff( + { success: true, content: "updated content" }, + 0, + "diff text without block markers", + ) + + expect(output).not.toContain("Applied") + expect(output).not.toContain("Making multiple related changes") + expect(output).toContain("Changes successfully applied.") + }) + + it("applies the abbreviation cap on the partial path as well", async () => { + const output = await runApplyDiff(partialResult(makeFailureSet(6)), 8) + + expect(output).toContain("Applied 2 of 8") + expect(output).toContain("(Failures 6-6 are shown as a single summary line each") + expect(output).not.toContain("second line of failure 6") + }) + + it("emits no partial-apply hint when every block applies", async () => { + const output = await runApplyDiff({ success: true, content: "updated content", failParts: [] }, 1) + + expect(output).not.toContain("Applied") + expect(output).toContain("Changes successfully applied.") + expect(output).toContain("Making multiple related changes in a single apply_diff is more efficient") + }) + + it("counts and renders only the unsuccessful entries in failParts", async () => { + const mixedParts: DiffResult[] = [ + { success: true, content: "first change" }, + makeFailure("FAILURE-MARKER-1\nsecond line of failure 1"), + { success: true, content: "third change" }, + ] + const output = await runApplyDiff({ success: true, content: "updated content", failParts: mixedParts }, 3) + + expect(output).toContain("Applied 2 of 3") + expect(output).toContain("FAILURE-MARKER-1") + expect(output).not.toContain("--- Diff block") + }) + }) +}) diff --git a/src/shared/tools.ts b/src/shared/tools.ts index b9a56d7d3d..8730fd4446 100644 --- a/src/shared/tools.ts +++ b/src/shared/tools.ts @@ -251,7 +251,7 @@ export const TOOL_ALIASES: Record = { } as const export type DiffResult = - | { success: true; content: string; failParts?: DiffResult[] } + | { success: true; content: string; failParts?: DiffResult[]; blockCount?: number } | ({ success: false error?: string