Skip to content

Commit ab966c2

Browse files
zw-xyskvincentkoc
zw-xysk
and
vincentkoc
authored
fix(tools): treat no-op writes and edits as terminal tool-loop failures (fixes #96983) (#97044)
* fix(tools): treat no-op writes and edits as terminal tool-loop failures Fixes #96983 * fix(tools): treat no-op writes and edits as terminal tool-loop failures Fixes #96983 * fix(tools): preserve valid sibling edits in mixed no-op batches Fixes #96983 * fix(tools): terminate apply_patch no-ops safely * fix(tools): validate no-op edits independently * fix(tools): preserve no-op edit overlap checks * fix(tools): preserve no-op patch file formatting * fix(tools): preserve move no-op formatting * fix(tools): narrow same-path move no-op typing * fix(tools): distinguish edit no-op errors * fix(tools): keep edit previews aligned with execution * fix(tools): align no-op validation and formatting * fix(tools): preserve empty patch and preview no-ops * fix(tools): preview fuzzy edit no-ops cleanly * fix(tools): isolate fuzzy-equivalent edit no-ops --------- Co-authored-by: Vincent Koc <[email protected]>
1 parent 41c00a6 commit ab966c2

7 files changed

Lines changed: 667 additions & 38 deletions

File tree

‎src/agents/apply-patch.test.ts‎

Lines changed: 86 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import {
1111
createRebindableDirectoryAlias,
1212
withRealpathSymlinkRebindRace,
1313
} from "../test-utils/symlink-rebind-race.js";
14-
import { applyPatch } from "./apply-patch.js";
14+
import { applyPatch, createApplyPatchTool } from "./apply-patch.js";
1515
import type { SandboxFsBridge } from "./sandbox/fs-bridge.js";
1616

1717
async function withTempDir<T>(fn: (dir: string) => Promise<T>) {
@@ -43,15 +43,16 @@ function createMemoryPatchSandbox(initialFiles: Record<string, string> = {}) {
4343
const files = new Map<string, string>(
4444
Object.entries(initialFiles).map(([filePath, contents]) => [`/sandbox/${filePath}`, contents]),
4545
);
46+
const writeFile = vi.fn(async ({ filePath, data }) => {
47+
files.set(filePath, Buffer.isBuffer(data) ? data.toString("utf8") : data);
48+
});
4649
const bridge: SandboxFsBridge = {
4750
resolvePath: ({ filePath }) => ({
4851
relativePath: filePath,
4952
containerPath: `/sandbox/${filePath}`,
5053
}),
5154
readFile: async ({ filePath }) => Buffer.from(files.get(filePath) ?? "", "utf8"),
52-
writeFile: async ({ filePath, data }) => {
53-
files.set(filePath, Buffer.isBuffer(data) ? data.toString("utf8") : data);
54-
},
55+
writeFile,
5556
remove: async ({ filePath }) => {
5657
files.delete(filePath);
5758
},
@@ -72,6 +73,8 @@ function createMemoryPatchSandbox(initialFiles: Record<string, string> = {}) {
7273
};
7374
return {
7475
files,
76+
bridge,
77+
writeFile,
7578
options: {
7679
cwd: "/local/workspace",
7780
sandbox: {
@@ -155,6 +158,85 @@ describe("applyPatch", () => {
155158
expect(result.summary.modified).toEqual(["source.txt"]);
156159
});
157160

161+
it("returns a terminal no-op without rewriting unchanged update hunks", async () => {
162+
const memory = createMemoryPatchSandbox({
163+
"source.txt": "foo\nbar\n",
164+
});
165+
const patch = `*** Begin Patch
166+
*** Update File: source.txt
167+
@@
168+
foo
169+
-bar
170+
+bar
171+
*** End Patch`;
172+
173+
const result = await applyPatch(patch, memory.options);
174+
175+
expect(result.noOp).toBe(true);
176+
expect(result.text).toBe("No changes made to source.txt.");
177+
expect(result.summary).toEqual({ added: [], modified: [], deleted: [] });
178+
expect(memory.files.get("/sandbox/source.txt")).toBe("foo\nbar\n");
179+
expect(memory.writeFile.mock.calls).toHaveLength(0);
180+
181+
const tool = createApplyPatchTool(memory.options);
182+
const toolResult = await tool.execute("call-no-op", { input: patch }, undefined);
183+
expect(toolResult.terminate).toBe(true);
184+
});
185+
186+
it("preserves line endings and EOF state for no-op update hunks", async () => {
187+
const patch = `*** Begin Patch
188+
*** Update File: source.txt
189+
@@
190+
foo
191+
-bar
192+
+bar
193+
*** End Patch`;
194+
for (const initial of ["foo\r\nbar\r\n", "foo\nbar"]) {
195+
const memory = createMemoryPatchSandbox({ "source.txt": initial });
196+
197+
const result = await applyPatch(patch, memory.options);
198+
199+
expect(result.noOp).toBe(true);
200+
expect(memory.files.get("/sandbox/source.txt")).toBe(initial);
201+
expect(memory.writeFile.mock.calls).toHaveLength(0);
202+
}
203+
});
204+
205+
it("applies a real deletion of the sole blank line", async () => {
206+
const memory = createMemoryPatchSandbox({ "source.txt": "\n" });
207+
const patch = `*** Begin Patch
208+
*** Update File: source.txt
209+
@@
210+
-
211+
*** End Patch`;
212+
213+
const result = await applyPatch(patch, memory.options);
214+
215+
expect(result.noOp).toBeUndefined();
216+
expect(memory.files.get("/sandbox/source.txt")).toBe("");
217+
expect(memory.writeFile.mock.calls).toHaveLength(1);
218+
});
219+
220+
it("preserves formatting for same-path move no-op hunks", async () => {
221+
const patch = `*** Begin Patch
222+
*** Update File: source.txt
223+
*** Move to: ./source.txt
224+
@@
225+
foo
226+
-bar
227+
+bar
228+
*** End Patch`;
229+
for (const initial of ["foo\r\nbar\r\n", "foo\nbar"]) {
230+
const memory = createMemoryPatchSandbox({ "source.txt": initial });
231+
232+
const result = await applyPatch(patch, memory.options);
233+
234+
expect(result.noOp).toBe(true);
235+
expect(memory.files.get("/sandbox/source.txt")).toBe(initial);
236+
expect(memory.writeFile.mock.calls).toHaveLength(0);
237+
}
238+
});
239+
158240
it("applies context-only insertions at the requested context", async () => {
159241
const memory = createMemoryPatchSandbox({
160242
"source.txt": "alpha\nanchor\nomega\n",

‎src/agents/apply-patch.ts‎

Lines changed: 43 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -62,12 +62,21 @@ export type ApplyPatchSummary = {
6262
type ApplyPatchResult = {
6363
summary: ApplyPatchSummary;
6464
text: string;
65+
noOp?: boolean;
6566
};
6667

6768
type ApplyPatchToolDetails = {
6869
summary: ApplyPatchSummary;
6970
};
7071

72+
function normalizeUpdateComparison(content: string): string {
73+
const normalized = content.replace(/\r\n/g, "\n").replace(/\r/g, "\n");
74+
if (normalized.length === 0 || normalized.endsWith("\n")) {
75+
return normalized;
76+
}
77+
return `${normalized}\n`;
78+
}
79+
7180
type SandboxApplyPatchConfig = {
7281
root: string;
7382
bridge: SandboxFsBridge;
@@ -123,6 +132,7 @@ export function createApplyPatchTool(
123132
return {
124133
content: [{ type: "text", text: result.text }],
125134
details: { summary: result.summary },
135+
...(result.noOp ? { terminate: true } : {}),
126136
};
127137
},
128138
};
@@ -148,6 +158,7 @@ export async function applyPatch(
148158
modified: new Set<string>(),
149159
deleted: new Set<string>(),
150160
};
161+
const noOpPaths = new Set<string>();
151162
const fileOps = resolvePatchFileOps(options);
152163

153164
for (const hunk of parsed.hunks) {
@@ -184,28 +195,47 @@ export async function applyPatch(
184195
await ensureDir(moveTarget.resolved, fileOps);
185196
const moveResolvesToSource =
186197
path.resolve(moveTarget.resolved) === path.resolve(target.resolved);
187-
await fileOps.writeFile(
188-
moveResolvesToSource ? target.resolved : moveTarget.resolved,
189-
applied,
190-
);
198+
const destination = moveResolvesToSource ? target.resolved : moveTarget.resolved;
199+
if (moveResolvesToSource) {
200+
const existing = await fileOps.readFile(target.resolved);
201+
if (normalizeUpdateComparison(existing) === normalizeUpdateComparison(applied)) {
202+
noOpPaths.add(target.display);
203+
} else {
204+
noOpPaths.delete(target.display);
205+
await fileOps.writeFile(destination, applied);
206+
}
207+
} else {
208+
noOpPaths.delete(target.display);
209+
await fileOps.writeFile(destination, applied);
210+
}
191211
if (!moveResolvesToSource) {
192212
await fileOps.remove(target.resolved);
193213
}
194-
recordSummary(
195-
summary,
196-
seen,
197-
"modified",
198-
moveResolvesToSource ? target.display : moveTarget.display,
199-
);
214+
if (!noOpPaths.has(target.display)) {
215+
recordSummary(
216+
summary,
217+
seen,
218+
"modified",
219+
moveResolvesToSource ? target.display : moveTarget.display,
220+
);
221+
}
200222
} else {
201-
await fileOps.writeFile(target.resolved, applied);
202-
recordSummary(summary, seen, "modified", target.display);
223+
const existing = await fileOps.readFile(target.resolved);
224+
if (normalizeUpdateComparison(existing) === normalizeUpdateComparison(applied)) {
225+
noOpPaths.add(target.display);
226+
} else {
227+
noOpPaths.delete(target.display);
228+
await fileOps.writeFile(target.resolved, applied);
229+
recordSummary(summary, seen, "modified", target.display);
230+
}
203231
}
204232
}
205233

234+
const noOp = noOpPaths.size > 0 && Object.values(summary).every((paths) => paths.length === 0);
206235
return {
207236
summary,
208-
text: formatSummary(summary),
237+
text: noOp ? `No changes made to ${Array.from(noOpPaths).join(", ")}.` : formatSummary(summary),
238+
...(noOp ? { noOp: true } : {}),
209239
};
210240
}
211241

‎src/agents/sessions/tools/edit-diff.ts‎

Lines changed: 72 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,13 @@ export interface Edit {
7979
newText: string;
8080
}
8181

82+
export class EditNoChangeError extends Error {
83+
constructor(message: string) {
84+
super(message);
85+
this.name = "EditNoChangeError";
86+
}
87+
}
88+
8289
interface MatchedEdit {
8390
editIndex: number;
8491
matchIndex: number;
@@ -179,13 +186,15 @@ function getEmptyOldTextError(path: string, editIndex: number, totalEdits: numbe
179186
return new Error(`edits[${editIndex}].oldText must not be empty in ${path}.`);
180187
}
181188

182-
function getNoChangeError(path: string, totalEdits: number): Error {
189+
function getNoChangeError(path: string, totalEdits: number): EditNoChangeError {
183190
if (totalEdits === 1) {
184-
return new Error(
191+
return new EditNoChangeError(
185192
`No changes made to ${path}. The replacement produced identical content. This might indicate an issue with special characters or the text not existing as expected.`,
186193
);
187194
}
188-
return new Error(`No changes made to ${path}. The replacements produced identical content.`);
195+
return new EditNoChangeError(
196+
`No changes made to ${path}. The replacements produced identical content.`,
197+
);
189198
}
190199

191200
/**
@@ -418,6 +427,57 @@ export interface EditDiffError {
418427
error: string;
419428
}
420429

430+
export function validateNoOpEditTargets(
431+
normalizedContent: string,
432+
noOpEdits: Edit[],
433+
realEdits: Edit[],
434+
path: string,
435+
): void {
436+
if (noOpEdits.length > 0) {
437+
applyEditsToNormalizedContent(
438+
normalizedContent,
439+
noOpEdits.map((edit) => ({ oldText: edit.oldText, newText: "" })),
440+
path,
441+
);
442+
}
443+
const exactNoOpEdits = noOpEdits.filter((edit) =>
444+
normalizedContent.includes(normalizeToLF(edit.oldText)),
445+
);
446+
if (exactNoOpEdits.length > 0 && realEdits.length > 0) {
447+
applyEditsToNormalizedContent(
448+
normalizedContent,
449+
[...exactNoOpEdits, ...realEdits].map((edit) => ({
450+
oldText: edit.oldText,
451+
newText: "",
452+
})),
453+
path,
454+
);
455+
}
456+
}
457+
458+
export function splitNoOpEdits(
459+
normalizedContent: string,
460+
edits: Edit[],
461+
path: string,
462+
): { noOpEdits: Edit[]; realEdits: Edit[] } {
463+
const noOpEdits: Edit[] = [];
464+
const realEdits: Edit[] = [];
465+
for (const edit of edits) {
466+
const fuzzyNoOp = normalizeForFuzzyMatch(edit.oldText) === normalizeForFuzzyMatch(edit.newText);
467+
if (edit.oldText === edit.newText || fuzzyNoOp) {
468+
applyEditsToNormalizedContent(
469+
normalizedContent,
470+
[{ oldText: edit.oldText, newText: "" }],
471+
path,
472+
);
473+
noOpEdits.push(edit);
474+
} else {
475+
realEdits.push(edit);
476+
}
477+
}
478+
return { noOpEdits, realEdits };
479+
}
480+
421481
/**
422482
* Compute the diff for one or more edit operations without applying them.
423483
* Used for preview rendering in the TUI before the tool executes.
@@ -459,15 +519,23 @@ export async function computeEditsDiff(
459519
// Strip BOM before matching (LLM won't include invisible BOM in oldText)
460520
const { text: content } = stripBom(rawContent);
461521
const normalizedContent = normalizeToLF(content);
522+
const { noOpEdits, realEdits } = splitNoOpEdits(normalizedContent, edits, path);
523+
validateNoOpEditTargets(normalizedContent, noOpEdits, realEdits, path);
524+
if (realEdits.length === 0) {
525+
return { diff: "", firstChangedLine: undefined };
526+
}
462527
const { baseContent, newContent } = applyEditsToNormalizedContent(
463528
normalizedContent,
464-
edits,
529+
realEdits,
465530
path,
466531
);
467532

468533
// Generate the diff
469534
return generateDiffString(baseContent, newContent);
470535
} catch (err) {
536+
if (err instanceof EditNoChangeError) {
537+
return { diff: "", firstChangedLine: undefined };
538+
}
471539
return { error: err instanceof Error ? err.message : String(err) };
472540
}
473541
}

0 commit comments

Comments
 (0)