From 53e43b56ba2ba1a6fc1989bb8990ad245e138cf8 Mon Sep 17 00:00:00 2001 From: Urjit Chakraborty <135136842+urjitc@users.noreply.github.com> Date: Thu, 30 Apr 2026 02:00:48 -0400 Subject: [PATCH 1/2] feat(item-edit): add lenient matching and best-effort multi-edit behavior Improve item_edit reliability by adding safe fallback matching strategies and partial-apply semantics for multi-edit requests. Persist successful document/quiz/flashcard edits even when some entries fail, and return applied/failed diagnostics so callers can recover without losing progress. Made-with: Cursor --- .../ai/tools/__tests__/edit-item-tool.test.ts | 17 + src/lib/ai/tools/edit-item-tool.ts | 7 +- src/lib/ai/workers/workspace-worker.ts | 93 +++- src/lib/utils/__tests__/edit-replace.test.ts | 36 ++ src/lib/utils/edit-replace.ts | 418 ++++++++++++++++-- 5 files changed, 518 insertions(+), 53 deletions(-) diff --git a/src/lib/ai/tools/__tests__/edit-item-tool.test.ts b/src/lib/ai/tools/__tests__/edit-item-tool.test.ts index 0afb00ac..ec6a8e39 100644 --- a/src/lib/ai/tools/__tests__/edit-item-tool.test.ts +++ b/src/lib/ai/tools/__tests__/edit-item-tool.test.ts @@ -194,6 +194,23 @@ describe("createEditItemTool", () => { expect(result.message).toMatch(/occurrences/i); }); + it("passes through partial-apply diagnostics when worker returns success=false", async () => { + mockWorkspaceWorker.mockResolvedValueOnce({ + success: false, + message: "Applied 1 edit(s), failed 1", + partialApplied: true, + appliedEditIndices: [0], + failedEdits: [{ index: 1, reason: "Could not find edits[1]." }], + }); + const tool: any = createEditItemTool(ctx); + const result = await tool.execute({ itemName: "My Document", edits: [{ oldText: "a", newText: "b" }] }); + + expect(result.success).toBe(false); + expect(result.partialApplied).toBe(true); + expect(result.appliedEditIndices).toEqual([0]); + expect(result.failedEdits).toEqual([{ index: 1, reason: "Could not find edits[1]." }]); + }); + it("rejects empty edits array without newName", async () => { const tool: any = createEditItemTool(ctx); const result = await tool.execute({ itemName: "My Document", edits: [] }); diff --git a/src/lib/ai/tools/edit-item-tool.ts b/src/lib/ai/tools/edit-item-tool.ts index 44e6950b..d5505e0c 100644 --- a/src/lib/ai/tools/edit-item-tool.ts +++ b/src/lib/ai/tools/edit-item-tool.ts @@ -19,7 +19,8 @@ export function createEditItemTool(ctx: WorkspaceToolContext) { return withSanitizedModelOutput(tool({ description: "Edit a document, flashcard deck, quiz, or PDF. You must use workspace_read at least once before editing. " + - "TARGETED EDIT: provide edits array with one or more {oldText, newText} pairs. Each oldText must match exactly and uniquely in the original content. Copy from workspace_read as-is. When changing multiple separate sections, use one call with multiple edits[] entries instead of multiple calls. " + + "TARGETED EDIT: provide edits array with one or more {oldText, newText} pairs. Matches must be unique; whitespace/context fallbacks are supported. " + + "MULTI-EDIT: successful edits may still apply even if some fail; in that case response includes applied/failed details with success=false. " + "FULL REWRITE: single edit with oldText='' and newText=entire new content (quizzes: only the JSON). " + "RENAME ONLY: empty edits array [] with newName='new name'. " + "PDFs: RENAME ONLY — empty edits array with newName. " + @@ -31,10 +32,10 @@ export function createEditItemTool(ctx: WorkspaceToolContext) { .string() .describe("Name of the item to edit (document, flashcard deck, quiz, or PDF; matched by fuzzy search)"), edits: z.array(z.object({ - oldText: z.string().describe("Exact text to find in the original content. Must be unique. Copy from workspace_read as-is."), + oldText: z.string().describe("Text to find in original content. Must be unique."), newText: z.string().describe("Replacement text for this edit."), })).describe( - "One or more targeted replacements. Each edit is matched against the original content, not incrementally. Do not include overlapping edits. If two changes touch the same block or nearby lines, merge them into one edit instead. For full rewrite: use a single edit with oldText='' and newText=entire new content." + "One or more targeted replacements. Prefer non-overlapping edits. For full rewrite: use one edit with oldText='' and full newText." ), newName: z.string().optional().describe("Rename the item to this. If not provided, the existing name is preserved."), sources: z diff --git a/src/lib/ai/workers/workspace-worker.ts b/src/lib/ai/workers/workspace-worker.ts index 558a8c24..4f133d78 100644 --- a/src/lib/ai/workers/workspace-worker.ts +++ b/src/lib/ai/workers/workspace-worker.ts @@ -21,7 +21,8 @@ import { checkDuplicateName, } from "@/lib/workspace/mutation-helpers"; import { - applyEdits, + applyEditsBestEffort, + type FailedEdit, trimDiff, normalizeLineEndings, } from "@/lib/utils/edit-replace"; @@ -223,6 +224,12 @@ async function updateWorkspaceItem( }); } +function formatFailedEdits(failedEdits: FailedEdit[]): string { + return failedEdits + .map((f) => `edits[${f.index}]: ${f.reason}`) + .join(" | "); +} + async function deleteWorkspaceItem(workspaceId: string, itemId: string) { await db.transaction(async (tx) => { await deleteWorkspaceItemById(tx, { workspaceId, itemId }); @@ -645,6 +652,13 @@ export async function workspaceWorker( if (existingItem.type === "flashcard") { const data = existingItem.data as FlashcardData; const cards: FlashcardItem[] = data.cards ?? []; + let editDiagnostics: + | { + appliedEditIndices: number[]; + failedEdits: FailedEdit[]; + hadFailures: boolean; + } + | undefined; const payload = { cards: cards.map((c) => ({ @@ -661,8 +675,14 @@ export async function workspaceWorker( ) { serialized = params.edits![0].newText; } else { - const { newContent } = applyEdits(serialized, params.edits!); + const { + newContent, + appliedEditIndices, + failedEdits, + hadFailures, + } = applyEditsBestEffort(serialized, params.edits!); serialized = newContent; + editDiagnostics = { appliedEditIndices, failedEdits, hadFailures }; } } @@ -720,10 +740,19 @@ export async function workspaceWorker( ); return { - success: true, + success: !editDiagnostics?.hadFailures, itemId: params.itemId, - message: `Updated flashcard deck (${newCards.length} cards)`, + message: editDiagnostics?.hadFailures + ? `Updated flashcard deck (${newCards.length} cards) with partial edits. Applied ${editDiagnostics.appliedEditIndices.length} edit(s), failed ${editDiagnostics.failedEdits.length}: ${formatFailedEdits(editDiagnostics.failedEdits)}` + : `Updated flashcard deck (${newCards.length} cards)`, cardCount: newCards.length, + ...(editDiagnostics + ? { + partialApplied: editDiagnostics.hadFailures, + appliedEditIndices: editDiagnostics.appliedEditIndices, + failedEdits: editDiagnostics.failedEdits, + } + : {}), }; } @@ -731,6 +760,13 @@ export async function workspaceWorker( const data = existingItem.data as QuizData; const questions = data.questions ?? []; const payload = { questions }; + let editDiagnostics: + | { + appliedEditIndices: number[]; + failedEdits: FailedEdit[]; + hadFailures: boolean; + } + | undefined; let serialized = JSON.stringify(payload, null, 2); if (!isRenameOnly) { if ( @@ -739,8 +775,14 @@ export async function workspaceWorker( ) { serialized = params.edits![0].newText; } else { - const { newContent } = applyEdits(serialized, params.edits!); + const { + newContent, + appliedEditIndices, + failedEdits, + hadFailures, + } = applyEditsBestEffort(serialized, params.edits!); serialized = newContent; + editDiagnostics = { appliedEditIndices, failedEdits, hadFailures }; } } @@ -838,16 +880,32 @@ export async function workspaceWorker( ); return { - success: true, + success: !editDiagnostics?.hadFailures, itemId: params.itemId, - message: `Updated quiz (${validatedQuestions.length} questions)`, + message: editDiagnostics?.hadFailures + ? `Updated quiz (${validatedQuestions.length} questions) with partial edits. Applied ${editDiagnostics.appliedEditIndices.length} edit(s), failed ${editDiagnostics.failedEdits.length}: ${formatFailedEdits(editDiagnostics.failedEdits)}` + : `Updated quiz (${validatedQuestions.length} questions)`, questionCount: validatedQuestions.length, + ...(editDiagnostics + ? { + partialApplied: editDiagnostics.hadFailures, + appliedEditIndices: editDiagnostics.appliedEditIndices, + failedEdits: editDiagnostics.failedEdits, + } + : {}), }; } if (existingItem.type === "document") { const changes: Partial = {}; const docData = existingItem.data as DocumentData; + let editDiagnostics: + | { + appliedEditIndices: number[]; + failedEdits: FailedEdit[]; + hadFailures: boolean; + } + | undefined; if (rename) { const dupError = checkDuplicateName( currentState, @@ -876,8 +934,14 @@ export async function workspaceWorker( contentNew = params.edits![0].newText; } else { contentOld = normalizeLineEndings(docData.markdown ?? ""); - const { newContent } = applyEdits(contentOld, params.edits!); + const { + newContent, + appliedEditIndices, + failedEdits, + hadFailures, + } = applyEditsBestEffort(contentOld, params.edits!); contentNew = newContent; + editDiagnostics = { appliedEditIndices, failedEdits, hadFailures }; } changes.data = { markdown: contentNew, @@ -929,14 +993,23 @@ export async function workspaceWorker( } return { - success: true, + success: !editDiagnostics?.hadFailures, itemId: params.itemId, - message: "Updated document successfully", + message: editDiagnostics?.hadFailures + ? `Updated document with partial edits. Applied ${editDiagnostics.appliedEditIndices.length} edit(s), failed ${editDiagnostics.failedEdits.length}: ${formatFailedEdits(editDiagnostics.failedEdits)}` + : "Updated document successfully", diff: diffOutput, filediff: { additions: filediffAdditions, deletions: filediffDeletions, }, + ...(editDiagnostics + ? { + partialApplied: editDiagnostics.hadFailures, + appliedEditIndices: editDiagnostics.appliedEditIndices, + failedEdits: editDiagnostics.failedEdits, + } + : {}), }; } diff --git a/src/lib/utils/__tests__/edit-replace.test.ts b/src/lib/utils/__tests__/edit-replace.test.ts index 401586f4..e7273f55 100644 --- a/src/lib/utils/__tests__/edit-replace.test.ts +++ b/src/lib/utils/__tests__/edit-replace.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect } from "vitest"; import { applyEdits, + applyEditsBestEffort, replace, normalizeLineEndings, trimDiff, @@ -206,6 +207,41 @@ describe("applyEdits", () => { const result = applyEdits(content, [{ oldText: "hello", newText: "goodbye" }]); expect(result.baseContent).toBe("hello\nworld"); }); + + it("matches trimmed boundary text when unique", () => { + const content = " \n\n\n- Satisfaction\n - Describe specific nonprofit *in detail*:\n ...\n\n \n\n\n"; + const result = applyEdits(content, [ + { + oldText: "- Satisfaction\n - Describe specific nonprofit *in detail*:\n ...", + newText: "- Satisfaction\n - Describe specific nonprofit *in detail*:\n - Updated details", + }, + ]); + expect(result.newContent).toContain("Updated details"); + }); + + it("keeps ambiguity errors under lenient matching", () => { + const block = " \n\n\n- Satisfaction\n ...\n\n \n\n\n"; + const content = `${block}middle\n${block}`; + expect(() => + applyEdits(content, [{ oldText: "- Satisfaction\n ...", newText: "- Satisfaction\n - updated" }]), + ).toThrow(/occurrences/i); + }); +}); + +describe("applyEditsBestEffort", () => { + it("applies successful edits and reports failed ones", () => { + const content = "alpha\nbeta\ngamma"; + const result = applyEditsBestEffort(content, [ + { oldText: "beta", newText: "BETA" }, + { oldText: "missing", newText: "x" }, + ]); + expect(result.newContent).toBe("alpha\nBETA\ngamma"); + expect(result.appliedEditIndices).toEqual([0]); + expect(result.failedEdits).toEqual([ + expect.objectContaining({ index: 1 }), + ]); + expect(result.hadFailures).toBe(true); + }); }); describe("replace (deprecated wrapper)", () => { diff --git a/src/lib/utils/edit-replace.ts b/src/lib/utils/edit-replace.ts index 0b796bd1..7f1ced6f 100644 --- a/src/lib/utils/edit-replace.ts +++ b/src/lib/utils/edit-replace.ts @@ -85,6 +85,17 @@ export interface AppliedEditsResult { newContent: string; } +export interface FailedEdit { + index: number; + reason: string; +} + +export interface BestEffortEditsResult extends AppliedEditsResult { + appliedEditIndices: number[]; + failedEdits: FailedEdit[]; + hadFailures: boolean; +} + function findText(content: string, oldText: string): { found: boolean; index: number; matchLength: number } { const index = content.indexOf(oldText); if (index !== -1) { @@ -97,6 +108,234 @@ function countOccurrences(content: string, oldText: string): number { return content.split(oldText).length - 1; } +type Replacer = (content: string, find: string) => Iterable; + +const simpleReplacer: Replacer = function* (_content, find) { + yield find; +}; + +const lineTrimmedReplacer: Replacer = function* (content, find) { + const originalLines = content.split("\n"); + const searchLines = find.split("\n"); + if (searchLines.length === 0) return; + if (searchLines[searchLines.length - 1] === "") searchLines.pop(); + if (searchLines.length === 0) return; + for (let i = 0; i <= originalLines.length - searchLines.length; i++) { + let matches = true; + for (let j = 0; j < searchLines.length; j++) { + if (originalLines[i + j].trim() !== searchLines[j].trim()) { + matches = false; + break; + } + } + if (!matches) continue; + const block = originalLines.slice(i, i + searchLines.length).join("\n"); + yield block; + } +}; + +const trimmedBoundaryReplacer: Replacer = function* (content, find) { + const trimmedFind = find.trim(); + if (!trimmedFind || trimmedFind === find) return; + if (content.includes(trimmedFind)) yield trimmedFind; +}; + +const whitespaceNormalizedReplacer: Replacer = function* (content, find) { + const normalizeWhitespace = (text: string) => text.replace(/\s+/g, " ").trim(); + const normalizedFind = normalizeWhitespace(find); + if (!normalizedFind) return; + + const lines = content.split("\n"); + for (const line of lines) { + if (normalizeWhitespace(line) === normalizedFind) { + yield line; + } + } + + const findLines = find.split("\n"); + if (findLines.length > 1) { + for (let i = 0; i <= lines.length - findLines.length; i++) { + const block = lines.slice(i, i + findLines.length).join("\n"); + if (normalizeWhitespace(block) === normalizedFind) { + yield block; + } + } + } +}; + +const indentationFlexibleReplacer: Replacer = function* (content, find) { + const removeIndentation = (text: string): string => { + const lines = text.split("\n"); + const nonEmptyLines = lines.filter((line) => line.trim().length > 0); + if (nonEmptyLines.length === 0) return text; + const minIndent = Math.min( + ...nonEmptyLines.map((line) => { + const match = line.match(/^(\s*)/); + return match ? match[1].length : 0; + }), + ); + return lines + .map((line) => (line.trim().length === 0 ? line : line.slice(minIndent))) + .join("\n"); + }; + + const normalizedFind = removeIndentation(find); + const contentLines = content.split("\n"); + const findLines = find.split("\n"); + if (findLines.length === 0) return; + for (let i = 0; i <= contentLines.length - findLines.length; i++) { + const block = contentLines.slice(i, i + findLines.length).join("\n"); + if (removeIndentation(block) === normalizedFind) { + yield block; + } + } +}; + +const escapeNormalizedReplacer: Replacer = function* (content, find) { + const unescapeString = (str: string): string => + str.replace(/\\(n|t|r|'|"|`|\\|\n|\$)/g, (match, capturedChar) => { + switch (capturedChar) { + case "n": + return "\n"; + case "t": + return "\t"; + case "r": + return "\r"; + case "'": + return "'"; + case '"': + return '"'; + case "`": + return "`"; + case "\\": + return "\\"; + case "\n": + return "\n"; + case "$": + return "$"; + default: + return match; + } + }); + + const unescapedFind = unescapeString(find); + if (unescapedFind && content.includes(unescapedFind)) { + yield unescapedFind; + } +}; + +const contextAwareReplacer: Replacer = function* (content, find) { + const findLines = find.split("\n"); + if (findLines.length < 3) return; + if (findLines[findLines.length - 1] === "") findLines.pop(); + if (findLines.length < 3) return; + + const contentLines = content.split("\n"); + const firstLine = findLines[0].trim(); + const lastLine = findLines[findLines.length - 1].trim(); + + for (let i = 0; i < contentLines.length; i++) { + if (contentLines[i].trim() !== firstLine) continue; + for (let j = i + 2; j < contentLines.length; j++) { + if (contentLines[j].trim() !== lastLine) continue; + const blockLines = contentLines.slice(i, j + 1); + if (blockLines.length !== findLines.length) break; + let matchingLines = 0; + let totalNonEmptyLines = 0; + for (let k = 1; k < blockLines.length - 1; k++) { + const blockLine = blockLines[k].trim(); + const findLine = findLines[k].trim(); + if (blockLine.length > 0 || findLine.length > 0) { + totalNonEmptyLines++; + if (blockLine === findLine) matchingLines++; + } + } + if (totalNonEmptyLines === 0 || matchingLines / totalNonEmptyLines >= 0.5) { + yield blockLines.join("\n"); + break; + } + break; + } + } +}; + +const replacers: Replacer[] = [ + simpleReplacer, + lineTrimmedReplacer, + trimmedBoundaryReplacer, + whitespaceNormalizedReplacer, + indentationFlexibleReplacer, + escapeNormalizedReplacer, + contextAwareReplacer, +]; + +function notFoundError(totalEdits: number, editIndex: number): Error { + return new Error( + totalEdits === 1 + ? `Could not find the exact text. The old text must match exactly including all whitespace and newlines.` + : `Could not find edits[${editIndex}]. The oldText must match exactly including all whitespace and newlines.`, + ); +} + +function occurrenceError(totalEdits: number, editIndex: number, occurrences: number): Error { + return new Error( + totalEdits === 1 + ? `Found ${occurrences} occurrences of the text. The text must be unique. Please provide more context to make it unique.` + : `Found ${occurrences} occurrences of edits[${editIndex}]. Each oldText must be unique. Please provide more context to make it unique.`, + ); +} + +function resolveUniqueMatch( + content: string, + oldText: string, + contentLooksJson: boolean, + totalEdits: number, + editIndex: number, +): { matchText: string; matchIndex: number; matchLength: number } { + const searchCandidates = buildSearchCandidates(oldText, contentLooksJson); + const yielded = new Set(); + let maxOccurrences = 0; + + for (const candidate of searchCandidates) { + for (const replacer of replacers) { + for (const search of replacer(content, candidate)) { + if (!search || yielded.has(search)) continue; + yielded.add(search); + const result = findText(content, search); + if (!result.found) continue; + const occurrences = countOccurrences(content, search); + maxOccurrences = Math.max(maxOccurrences, occurrences); + if (occurrences > 1) continue; + return { + matchText: search, + matchIndex: result.index, + matchLength: result.matchLength, + }; + } + } + } + + if (maxOccurrences > 1) { + throw occurrenceError(totalEdits, editIndex, maxOccurrences); + } + throw notFoundError(totalEdits, editIndex); +} + +function applyResolvedEdits( + content: string, + resolved: Array<{ editIndex: number; matchIndex: number; matchLength: number; newText: string }>, +): string { + let newContent = content; + for (let i = resolved.length - 1; i >= 0; i--) { + const edit = resolved[i]; + newContent = + newContent.substring(0, edit.matchIndex) + + edit.newText + + newContent.substring(edit.matchIndex + edit.matchLength); + } + return newContent; +} + export function applyEdits(content: string, edits: Edit[]): AppliedEditsResult { const normalizedContent = normalizeLineEndings(content); const contentLooksJson = /"questions"\s*:|"cards"\s*:|^\s*[\[{]/.test(normalizedContent); @@ -124,39 +363,20 @@ export function applyEdits(content: string, edits: Edit[]): AppliedEditsResult { for (let i = 0; i < normalizedEdits.length; i++) { const edit = normalizedEdits[i]; - const candidates = buildSearchCandidates(edit.oldText, contentLooksJson); const replacementText = normalizeReplacementText(normalizedContent, edit.newText); - let matched = false; - - for (const candidate of candidates) { - const result = findText(normalizedContent, candidate); - if (result.found) { - const occurrences = countOccurrences(normalizedContent, candidate); - if (occurrences > 1) { - throw new Error( - normalizedEdits.length === 1 - ? `Found ${occurrences} occurrences of the text. The text must be unique. Please provide more context to make it unique.` - : `Found ${occurrences} occurrences of edits[${i}]. Each oldText must be unique. Please provide more context to make it unique.` - ); - } - matchedEdits.push({ - editIndex: i, - matchIndex: result.index, - matchLength: result.matchLength, - newText: replacementText, - }); - matched = true; - break; - } - } - - if (!matched) { - throw new Error( - normalizedEdits.length === 1 - ? `Could not find the exact text. The old text must match exactly including all whitespace and newlines.` - : `Could not find edits[${i}]. The oldText must match exactly including all whitespace and newlines.` - ); - } + const match = resolveUniqueMatch( + normalizedContent, + edit.oldText, + contentLooksJson, + normalizedEdits.length, + i, + ); + matchedEdits.push({ + editIndex: i, + matchIndex: match.matchIndex, + matchLength: match.matchLength, + newText: replacementText, + }); } matchedEdits.sort((a, b) => a.matchIndex - b.matchIndex); @@ -170,14 +390,7 @@ export function applyEdits(content: string, edits: Edit[]): AppliedEditsResult { } } - let newContent = normalizedContent; - for (let i = matchedEdits.length - 1; i >= 0; i--) { - const edit = matchedEdits[i]; - newContent = - newContent.substring(0, edit.matchIndex) + - edit.newText + - newContent.substring(edit.matchIndex + edit.matchLength); - } + const newContent = applyResolvedEdits(normalizedContent, matchedEdits); if (normalizedContent === newContent) { throw new Error(`No changes made. The replacements produced identical content.`); @@ -186,6 +399,131 @@ export function applyEdits(content: string, edits: Edit[]): AppliedEditsResult { return { baseContent: normalizedContent, newContent }; } +export function applyEditsBestEffort(content: string, edits: Edit[]): BestEffortEditsResult { + const normalizedContent = normalizeLineEndings(content); + const contentLooksJson = /"questions"\s*:|"cards"\s*:|^\s*[\[{]/.test(normalizedContent); + const normalizedEdits = edits.map((e) => ({ + oldText: normalizeLineEndings(e.oldText), + newText: normalizeLineEndings(e.newText), + })); + + const failedEdits: FailedEdit[] = []; + const appliedEditIndices: number[] = []; + + const pending = new Set(); + for (let i = 0; i < normalizedEdits.length; i++) pending.add(i); + + const firstPassResolved: Array<{ + editIndex: number; + matchIndex: number; + matchLength: number; + newText: string; + }> = []; + + // Pass 1: resolve against original snapshot for independence. + for (let i = 0; i < normalizedEdits.length; i++) { + const edit = normalizedEdits[i]; + if (edit.oldText.length === 0) { + failedEdits.push({ + index: i, + reason: + normalizedEdits.length === 1 + ? `oldText must not be empty.` + : `edits[${i}].oldText must not be empty.`, + }); + pending.delete(i); + continue; + } + const replacementText = normalizeReplacementText(normalizedContent, edit.newText); + try { + const match = resolveUniqueMatch( + normalizedContent, + edit.oldText, + contentLooksJson, + normalizedEdits.length, + i, + ); + firstPassResolved.push({ + editIndex: i, + matchIndex: match.matchIndex, + matchLength: match.matchLength, + newText: replacementText, + }); + pending.delete(i); + } catch { + // Leave unresolved for pass 2. + } + } + + firstPassResolved.sort((a, b) => a.matchIndex - b.matchIndex); + const nonOverlapping: typeof firstPassResolved = []; + for (let i = 0; i < firstPassResolved.length; i++) { + const curr = firstPassResolved[i]; + if (nonOverlapping.length === 0) { + nonOverlapping.push(curr); + continue; + } + const prev = nonOverlapping[nonOverlapping.length - 1]!; + if (prev.matchIndex + prev.matchLength > curr.matchIndex) { + failedEdits.push({ + index: curr.editIndex, + reason: `edits[${prev.editIndex}] and edits[${curr.editIndex}] overlap. Merge them into one edit or target disjoint regions.`, + }); + continue; + } + nonOverlapping.push(curr); + } + + let currentContent = applyResolvedEdits(normalizedContent, nonOverlapping); + for (const resolved of nonOverlapping) appliedEditIndices.push(resolved.editIndex); + + // Pass 2: retry unresolved edits sequentially against latest content. + for (const editIndex of Array.from(pending).sort((a, b) => a - b)) { + const edit = normalizedEdits[editIndex]; + const replacementText = normalizeReplacementText(currentContent, edit.newText); + try { + const match = resolveUniqueMatch( + currentContent, + edit.oldText, + contentLooksJson, + normalizedEdits.length, + editIndex, + ); + currentContent = + currentContent.substring(0, match.matchIndex) + + replacementText + + currentContent.substring(match.matchIndex + match.matchLength); + appliedEditIndices.push(editIndex); + } catch (error) { + failedEdits.push({ + index: editIndex, + reason: error instanceof Error ? error.message : String(error), + }); + } + } + + appliedEditIndices.sort((a, b) => a - b); + failedEdits.sort((a, b) => a.index - b.index); + + if (appliedEditIndices.length === 0 && failedEdits.length === 0) { + throw new Error(`No changes made. The replacements produced identical content.`); + } + if (appliedEditIndices.length === 0 && failedEdits.length > 0) { + throw new Error(failedEdits[0]?.reason ?? `No changes made.`); + } + if (normalizedContent === currentContent) { + throw new Error(`No changes made. The replacements produced identical content.`); + } + + return { + baseContent: normalizedContent, + newContent: currentContent, + appliedEditIndices, + failedEdits, + hadFailures: failedEdits.length > 0, + }; +} + /** @deprecated Use applyEdits() for new code. Kept for any remaining callers. */ export function replace(content: string, oldString: string, newString: string): string { if (oldString === newString) { From 1e78b779482979200798062e8c6ebd3ea56d9126 Mon Sep 17 00:00:00 2001 From: Urjit Chakraborty <135136842+urjitc@users.noreply.github.com> Date: Thu, 30 Apr 2026 16:27:10 -0400 Subject: [PATCH 2/2] fix(item-edit): address PR review edge cases Tighten context-aware fallback matching and retry overlap-rejected edits in pass 2 so best-effort edits remain safe and complete. Also include itemName on partial-apply tool responses and expand test coverage for these paths. Made-with: Cursor --- .../ai/tools/__tests__/edit-item-tool.test.ts | 1 + src/lib/ai/tools/edit-item-tool.ts | 2 +- src/lib/utils/__tests__/edit-replace.test.ts | 45 +++++++++++++++++++ src/lib/utils/edit-replace.ts | 12 ++--- 4 files changed, 53 insertions(+), 7 deletions(-) diff --git a/src/lib/ai/tools/__tests__/edit-item-tool.test.ts b/src/lib/ai/tools/__tests__/edit-item-tool.test.ts index ec6a8e39..6cb403fe 100644 --- a/src/lib/ai/tools/__tests__/edit-item-tool.test.ts +++ b/src/lib/ai/tools/__tests__/edit-item-tool.test.ts @@ -209,6 +209,7 @@ describe("createEditItemTool", () => { expect(result.partialApplied).toBe(true); expect(result.appliedEditIndices).toEqual([0]); expect(result.failedEdits).toEqual([{ index: 1, reason: "Could not find edits[1]." }]); + expect(result.itemName).toBe("My Document"); }); it("rejects empty edits array without newName", async () => { diff --git a/src/lib/ai/tools/edit-item-tool.ts b/src/lib/ai/tools/edit-item-tool.ts index d5505e0c..7c2dad4d 100644 --- a/src/lib/ai/tools/edit-item-tool.ts +++ b/src/lib/ai/tools/edit-item-tool.ts @@ -153,7 +153,7 @@ export function createEditItemTool(ctx: WorkspaceToolContext) { sources: input.sources, }); - if (workerResult.success) { + if (workerResult.success || (workerResult as { partialApplied?: boolean }).partialApplied) { return { ...workerResult, itemName: newName ?? matchedItem.name, diff --git a/src/lib/utils/__tests__/edit-replace.test.ts b/src/lib/utils/__tests__/edit-replace.test.ts index e7273f55..a3598c7d 100644 --- a/src/lib/utils/__tests__/edit-replace.test.ts +++ b/src/lib/utils/__tests__/edit-replace.test.ts @@ -226,6 +226,18 @@ describe("applyEdits", () => { applyEdits(content, [{ oldText: "- Satisfaction\n ...", newText: "- Satisfaction\n - updated" }]), ).toThrow(/occurrences/i); }); + + it("does not accept weak context-aware 50% middle-line matches", () => { + const content = ["- Question", " - correct", " - wrongA", "- End"].join("\n"); + expect(() => + applyEdits(content, [ + { + oldText: ["- Question", " - correct", " - wrongB", "- End"].join("\n"), + newText: ["- Question", " - updated", " - wrongB", "- End"].join("\n"), + }, + ]), + ).toThrow(/Could not find/); + }); }); describe("applyEditsBestEffort", () => { @@ -242,6 +254,39 @@ describe("applyEditsBestEffort", () => { ]); expect(result.hadFailures).toBe(true); }); + + it("marks hadFailures false when all edits apply", () => { + const content = "alpha\nbeta\ngamma"; + const result = applyEditsBestEffort(content, [ + { oldText: "alpha", newText: "ALPHA" }, + { oldText: "gamma", newText: "GAMMA" }, + ]); + expect(result.newContent).toBe("ALPHA\nbeta\nGAMMA"); + expect(result.appliedEditIndices).toEqual([0, 1]); + expect(result.failedEdits).toEqual([]); + expect(result.hadFailures).toBe(false); + }); + + it("retries overlap-rejected edits in pass 2", () => { + const content = "abcde"; + const result = applyEditsBestEffort(content, [ + { oldText: "abc", newText: "xabc" }, + { oldText: "abcde", newText: "ABCDE" }, + ]); + expect(result.newContent).toBe("xABCDE"); + expect(result.appliedEditIndices).toEqual([0, 1]); + expect(result.failedEdits).toEqual([]); + expect(result.hadFailures).toBe(false); + }); + + it("throws when all edits fail", () => { + expect(() => + applyEditsBestEffort("alpha\nbeta", [ + { oldText: "missing one", newText: "x" }, + { oldText: "missing two", newText: "y" }, + ]), + ).toThrow(/Could not find/); + }); }); describe("replace (deprecated wrapper)", () => { diff --git a/src/lib/utils/edit-replace.ts b/src/lib/utils/edit-replace.ts index 7f1ced6f..c4637fd3 100644 --- a/src/lib/utils/edit-replace.ts +++ b/src/lib/utils/edit-replace.ts @@ -109,6 +109,7 @@ function countOccurrences(content: string, oldText: string): number { } type Replacer = (content: string, find: string) => Iterable; +const CONTEXT_AWARE_SIMILARITY_THRESHOLD = 0.8; const simpleReplacer: Replacer = function* (_content, find) { yield find; @@ -250,11 +251,13 @@ const contextAwareReplacer: Replacer = function* (content, find) { if (blockLine === findLine) matchingLines++; } } - if (totalNonEmptyLines === 0 || matchingLines / totalNonEmptyLines >= 0.5) { + if ( + totalNonEmptyLines === 0 || + matchingLines / totalNonEmptyLines >= CONTEXT_AWARE_SIMILARITY_THRESHOLD + ) { yield blockLines.join("\n"); break; } - break; } } }; @@ -465,10 +468,7 @@ export function applyEditsBestEffort(content: string, edits: Edit[]): BestEffort } const prev = nonOverlapping[nonOverlapping.length - 1]!; if (prev.matchIndex + prev.matchLength > curr.matchIndex) { - failedEdits.push({ - index: curr.editIndex, - reason: `edits[${prev.editIndex}] and edits[${curr.editIndex}] overlap. Merge them into one edit or target disjoint regions.`, - }); + pending.add(curr.editIndex); continue; } nonOverlapping.push(curr);