feat(coding-agent/patch): enhanced hashline edit validation and error recovery with deduplication and format checking
- Added noopEdits array to applyHashlineEdits return type to track edits that produce no changes. - Added validation to reject edits with wrong-format fields (old_text/new_text from replace mode, diff from patch mode) that indicate model confusion. - Added additionalProperties tolerance to hashline edit schemas to allow flexible field handling. - Added deduplication logic to remove duplicate edits targeting the same line(s) with identical destination content. - Improved error handling for missing end fields in edit ranges by returning single-line specs instead of requiring both start and end. - Enhanced no-op error recovery guidance in prompts with detailed instructions to re-read file and function context after consecutive no-op errors.
This commit is contained in:
can1357
@@ -28,16 +28,23 @@ function parseHashlineEdit(edit: HashlineEdit): { spec: ParsedRefs; dst: string
|
||||
};
|
||||
}
|
||||
if ("range" in edit) {
|
||||
const start = parseLineRef(edit.range.start);
|
||||
const end = parseLineRef(edit.range.end);
|
||||
const r = edit.range as Record<string, string>;
|
||||
const start = parseLineRef(r.start);
|
||||
if (!r.end) {
|
||||
return {
|
||||
spec: { kind: "single", ref: start },
|
||||
dst: r.replacement ?? "",
|
||||
};
|
||||
}
|
||||
const end = parseLineRef(r.end);
|
||||
return {
|
||||
spec: start.line === end.line ? { kind: "single", ref: start } : { kind: "range", start, end },
|
||||
dst: edit.range.replacement,
|
||||
dst: r.replacement ?? "",
|
||||
};
|
||||
}
|
||||
return {
|
||||
spec: { kind: "insertAfter", after: parseLineRef(edit.insertAfter.loc) },
|
||||
dst: edit.insertAfter.content,
|
||||
dst: edit.insertAfter.content ?? (edit.insertAfter as Record<string, string>).replacement ?? "",
|
||||
};
|
||||
}
|
||||
/** Split dst into lines; empty string means delete (no lines). */
|
||||
@@ -634,7 +641,12 @@ export function validateLineRef(ref: { line: number; hash: string }, fileLines:
|
||||
export function applyHashlineEdits(
|
||||
content: string,
|
||||
edits: HashlineEdit[],
|
||||
): { content: string; firstChangedLine: number | undefined; warnings?: string[] } {
|
||||
): {
|
||||
content: string;
|
||||
firstChangedLine: number | undefined;
|
||||
warnings?: string[];
|
||||
noopEdits?: Array<{ editIndex: number; loc: string; currentContent: string }>;
|
||||
} {
|
||||
if (edits.length === 0) {
|
||||
return { content, firstChangedLine: undefined };
|
||||
}
|
||||
@@ -642,6 +654,7 @@ export function applyHashlineEdits(
|
||||
const fileLines = content.split("\n");
|
||||
const originalFileLines = [...fileLines];
|
||||
let firstChangedLine: number | undefined;
|
||||
const noopEdits: Array<{ editIndex: number; loc: string; currentContent: string }> = [];
|
||||
|
||||
// Parse src specs and dst lines up front
|
||||
const parsed = edits.map(edit => {
|
||||
@@ -769,6 +782,36 @@ export function applyHashlineEdits(
|
||||
// adjacent lines as safe merge candidates.
|
||||
explicitlyTouchedLines = collectExplicitlyTouchedLines();
|
||||
|
||||
// Deduplicate identical edits targeting the same line(s)
|
||||
const seenEditKeys = new Map<string, number>();
|
||||
const dedupIndices = new Set<number>();
|
||||
for (let i = 0; i < parsed.length; i++) {
|
||||
const p = parsed[i];
|
||||
let lineKey: string;
|
||||
switch (p.spec.kind) {
|
||||
case "single":
|
||||
lineKey = `s:${p.spec.ref.line}`;
|
||||
break;
|
||||
case "range":
|
||||
lineKey = `r:${p.spec.start.line}:${p.spec.end.line}`;
|
||||
break;
|
||||
case "insertAfter":
|
||||
lineKey = `i:${p.spec.after.line}`;
|
||||
break;
|
||||
}
|
||||
const dstKey = `${lineKey}|${p.dstLines.join("\n")}`;
|
||||
if (seenEditKeys.has(dstKey)) {
|
||||
dedupIndices.add(i);
|
||||
} else {
|
||||
seenEditKeys.set(dstKey, i);
|
||||
}
|
||||
}
|
||||
if (dedupIndices.size > 0) {
|
||||
for (let i = parsed.length - 1; i >= 0; i--) {
|
||||
if (dedupIndices.has(i)) parsed.splice(i, 1);
|
||||
}
|
||||
}
|
||||
|
||||
// Compute sort key (descending) — bottom-up application
|
||||
const annotated = parsed.map((p, idx) => {
|
||||
let sortLine: number;
|
||||
@@ -793,7 +836,7 @@ export function applyHashlineEdits(
|
||||
annotated.sort((a, b) => b.sortLine - a.sortLine || a.precedence - b.precedence || a.idx - b.idx);
|
||||
|
||||
// Apply edits bottom-up
|
||||
for (const { spec, dstLines } of annotated) {
|
||||
for (const { spec, dstLines, idx } of annotated) {
|
||||
switch (spec.kind) {
|
||||
case "single": {
|
||||
const merged = maybeExpandSingleLineMerge(spec.ref.line, dstLines);
|
||||
@@ -810,6 +853,14 @@ export function applyHashlineEdits(
|
||||
) {
|
||||
nextLines = normalizeConfusableHyphensInLines(nextLines);
|
||||
}
|
||||
if (origLines.join("\n") === nextLines.join("\n")) {
|
||||
noopEdits.push({
|
||||
editIndex: idx,
|
||||
loc: `${spec.ref.line}:${spec.ref.hash}`,
|
||||
currentContent: origLines.join("\n"),
|
||||
});
|
||||
break;
|
||||
}
|
||||
fileLines.splice(merged.startLine - 1, merged.deleteCount, ...nextLines);
|
||||
trackFirstChanged(merged.startLine);
|
||||
break;
|
||||
@@ -823,6 +874,14 @@ export function applyHashlineEdits(
|
||||
if (origLines.join("\n") === newLines.join("\n") && origLines.some(l => CONFUSABLE_HYPHENS_RE.test(l))) {
|
||||
newLines = normalizeConfusableHyphensInLines(newLines);
|
||||
}
|
||||
if (origLines.join("\n") === newLines.join("\n")) {
|
||||
noopEdits.push({
|
||||
editIndex: idx,
|
||||
loc: `${spec.ref.line}:${spec.ref.hash}`,
|
||||
currentContent: origLines.join("\n"),
|
||||
});
|
||||
break;
|
||||
}
|
||||
fileLines.splice(spec.ref.line - 1, count, ...newLines);
|
||||
trackFirstChanged(spec.ref.line);
|
||||
break;
|
||||
@@ -836,6 +895,14 @@ export function applyHashlineEdits(
|
||||
if (origLines.join("\n") === newLines.join("\n") && origLines.some(l => CONFUSABLE_HYPHENS_RE.test(l))) {
|
||||
newLines = normalizeConfusableHyphensInLines(newLines);
|
||||
}
|
||||
if (origLines.join("\n") === newLines.join("\n")) {
|
||||
noopEdits.push({
|
||||
editIndex: idx,
|
||||
loc: `${spec.start.line}:${spec.start.hash}`,
|
||||
currentContent: origLines.join("\n"),
|
||||
});
|
||||
break;
|
||||
}
|
||||
fileLines.splice(spec.start.line - 1, count, ...newLines);
|
||||
trackFirstChanged(spec.start.line);
|
||||
break;
|
||||
@@ -843,6 +910,14 @@ export function applyHashlineEdits(
|
||||
case "insertAfter": {
|
||||
const anchorLine = originalFileLines[spec.after.line - 1];
|
||||
const inserted = stripInsertAnchorEchoAfter(anchorLine, dstLines);
|
||||
if (inserted.length === 0) {
|
||||
noopEdits.push({
|
||||
editIndex: idx,
|
||||
loc: `${spec.after.line}:${spec.after.hash}`,
|
||||
currentContent: originalFileLines[spec.after.line - 1],
|
||||
});
|
||||
break;
|
||||
}
|
||||
fileLines.splice(spec.after.line, 0, ...inserted);
|
||||
trackFirstChanged(spec.after.line + 1);
|
||||
break;
|
||||
@@ -864,6 +939,7 @@ export function applyHashlineEdits(
|
||||
content: fileLines.join("\n"),
|
||||
firstChangedLine,
|
||||
...(warnings.length > 0 ? { warnings } : {}),
|
||||
...(noopEdits.length > 0 ? { noopEdits } : {}),
|
||||
};
|
||||
|
||||
function trackFirstChanged(line: number): void {
|
||||
|
||||
@@ -124,31 +124,43 @@ const patchEditSchema = Type.Object({
|
||||
export type ReplaceParams = Static<typeof replaceEditSchema>;
|
||||
export type PatchParams = Static<typeof patchEditSchema>;
|
||||
|
||||
const hashlineSingleSchema = Type.Object({
|
||||
single: Type.Object({
|
||||
loc: Type.String({ description: 'Line reference "LINE:HASH"' }),
|
||||
replacement: Type.String({ description: 'Replacement content (\\n-separated) — "" for delete' }),
|
||||
}),
|
||||
});
|
||||
const hashlineSingleSchema = Type.Object(
|
||||
{
|
||||
single: Type.Object({
|
||||
loc: Type.String({ description: 'Line reference "LINE:HASH"' }),
|
||||
replacement: Type.String({ description: 'Replacement content (\\n-separated) — "" for delete' }),
|
||||
}),
|
||||
},
|
||||
{ additionalProperties: true },
|
||||
);
|
||||
|
||||
const hashlineRangeSchema = Type.Object({
|
||||
range: Type.Object({
|
||||
start: Type.String({ description: 'Start line ref "LINE:HASH"' }),
|
||||
end: Type.String({ description: 'End line ref "LINE:HASH"' }),
|
||||
replacement: Type.String({ description: 'Replacement content (\\n-separated) — "" for delete' }),
|
||||
}),
|
||||
});
|
||||
const hashlineInsertAfterSchema = Type.Object({
|
||||
insertAfter: Type.Object({
|
||||
loc: Type.String({ description: 'Insert after this line "LINE:HASH"' }),
|
||||
content: Type.String({ description: "Content to insert (\\n-separated); must be non-empty" }),
|
||||
}),
|
||||
});
|
||||
const hashlineRangeSchema = Type.Object(
|
||||
{
|
||||
range: Type.Object({
|
||||
start: Type.String({ description: 'Start line ref "LINE:HASH"' }),
|
||||
end: Type.String({ description: 'End line ref "LINE:HASH"' }),
|
||||
replacement: Type.String({ description: 'Replacement content (\\n-separated) — "" for delete' }),
|
||||
}),
|
||||
},
|
||||
{ additionalProperties: true },
|
||||
);
|
||||
const hashlineInsertAfterSchema = Type.Object(
|
||||
{
|
||||
insertAfter: Type.Object({
|
||||
loc: Type.String({ description: 'Insert after this line "LINE:HASH"' }),
|
||||
content: Type.String({ description: "Content to insert (\\n-separated); must be non-empty" }),
|
||||
}),
|
||||
},
|
||||
{ additionalProperties: true },
|
||||
);
|
||||
const hashlineEditItemSchema = Type.Union([hashlineSingleSchema, hashlineRangeSchema, hashlineInsertAfterSchema]);
|
||||
const hashlineEditSchema = Type.Object({
|
||||
path: Type.String({ description: "File path (relative or absolute)" }),
|
||||
edits: Type.Array(hashlineEditItemSchema, { description: "Array of edit operations" }),
|
||||
});
|
||||
const hashlineEditSchema = Type.Object(
|
||||
{
|
||||
path: Type.String({ description: "File path (relative or absolute)" }),
|
||||
edits: Type.Array(hashlineEditItemSchema, { description: "Array of edit operations" }),
|
||||
},
|
||||
{ additionalProperties: true },
|
||||
);
|
||||
|
||||
export type HashlineEdit = Static<typeof hashlineEditItemSchema>;
|
||||
export type HashlineParams = Static<typeof hashlineEditSchema>;
|
||||
@@ -388,6 +400,28 @@ export class EditTool implements AgentTool<TInput> {
|
||||
throw new Error("Cannot edit Jupyter notebooks with the Edit tool. Use the NotebookEdit tool instead.");
|
||||
}
|
||||
|
||||
// Detect wrong-format fields from models confusing edit modes
|
||||
for (let i = 0; i < edits.length; i++) {
|
||||
const edit = edits[i] as Record<string, unknown>;
|
||||
if ("old_text" in edit || "new_text" in edit) {
|
||||
throw new Error(
|
||||
`edits[${i}] contains 'old_text'/'new_text' fields from replace mode. ` +
|
||||
`Hashline edits use: {single: {loc, replacement}}, {range: {start, end, replacement}}, or {insertAfter: {loc, content}}.`,
|
||||
);
|
||||
}
|
||||
if ("diff" in edit) {
|
||||
throw new Error(
|
||||
`edits[${i}] contains 'diff' field from patch mode. ` +
|
||||
`Hashline edits use: {single: {loc, replacement}}, {range: {start, end, replacement}}, or {insertAfter: {loc, content}}.`,
|
||||
);
|
||||
}
|
||||
if (!("single" in edit) && !("range" in edit) && !("insertAfter" in edit)) {
|
||||
throw new Error(
|
||||
`edits[${i}] must contain exactly one of: 'single', 'range', or 'insertAfter'. Got keys: [${Object.keys(edit).join(", ")}].`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
const absolutePath = resolvePlanPath(this.session, path);
|
||||
const file = Bun.file(absolutePath);
|
||||
|
||||
@@ -402,62 +436,42 @@ export class EditTool implements AgentTool<TInput> {
|
||||
const result = applyHashlineEdits(normalizedContent, edits);
|
||||
if (normalizedContent === result.content) {
|
||||
let diagnostic = `No changes made to ${path}. The edits produced identical content.`;
|
||||
try {
|
||||
if (result.noopEdits && result.noopEdits.length > 0) {
|
||||
const details = result.noopEdits
|
||||
.map(
|
||||
e =>
|
||||
`Edit ${e.editIndex}: replacement for ${e.loc} is identical to current content:\n ${e.loc}| ${e.currentContent}`,
|
||||
)
|
||||
.join("\n");
|
||||
diagnostic += `\n${details}`;
|
||||
diagnostic +=
|
||||
"\nYour content must differ from what the file already contains. Re-read the file to see the current state.";
|
||||
} else {
|
||||
// Edits were not literally identical but heuristics normalized them back
|
||||
const lines = normalizedContent.split("\n");
|
||||
const noopDetails: string[] = [];
|
||||
const targetLines: string[] = [];
|
||||
for (const edit of edits) {
|
||||
if ("single" in edit) {
|
||||
const parsed = parseLineRef(edit.single.loc);
|
||||
if (parsed.line >= 1 && parsed.line <= lines.length) {
|
||||
const current = lines[parsed.line - 1];
|
||||
if (current === edit.single.replacement) {
|
||||
const hash = computeLineHash(parsed.line, current);
|
||||
noopDetails.push(
|
||||
`Line ${parsed.line} \u2014 your replacement is identical to the current content:\n ${parsed.line}:${hash}| ${current}`,
|
||||
);
|
||||
}
|
||||
}
|
||||
} else if ("range" in edit) {
|
||||
const start = parseLineRef(edit.range.start);
|
||||
const end = parseLineRef(edit.range.end);
|
||||
if (start.line >= 1 && end.line <= lines.length) {
|
||||
const current = lines.slice(start.line - 1, end.line).join("\n");
|
||||
if (current === edit.range.replacement) {
|
||||
noopDetails.push(
|
||||
`Lines ${start.line}-${end.line} \u2014 your replacement is identical to the current content.`,
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
if (noopDetails.length > 0) {
|
||||
diagnostic += `\n${noopDetails.join("\n")}`;
|
||||
diagnostic +=
|
||||
"\nYour content must differ from what the file already contains. Re-read the file to see the current state.";
|
||||
} else {
|
||||
// Edits were not literally identical but heuristics normalized them back.
|
||||
const targetLines: string[] = [];
|
||||
for (const edit of edits) {
|
||||
const refs: string[] = [];
|
||||
if ("single" in edit) refs.push(edit.single.loc);
|
||||
else if ("range" in edit) refs.push(edit.range.start, edit.range.end);
|
||||
else if ("insertAfter" in edit) refs.push(edit.insertAfter.loc);
|
||||
for (const ref of refs) {
|
||||
const refs: string[] = [];
|
||||
if ("single" in edit) refs.push(edit.single.loc);
|
||||
else if ("range" in edit) refs.push(edit.range.start, edit.range.end);
|
||||
else if ("insertAfter" in edit) refs.push(edit.insertAfter.loc);
|
||||
for (const ref of refs) {
|
||||
try {
|
||||
const parsed = parseLineRef(ref);
|
||||
if (parsed.line >= 1 && parsed.line <= lines.length) {
|
||||
const lineContent = lines[parsed.line - 1];
|
||||
const hash = computeLineHash(parsed.line, lineContent);
|
||||
targetLines.push(`${parsed.line}:${hash}| ${lineContent}`);
|
||||
}
|
||||
} catch {
|
||||
/* skip malformed refs */
|
||||
}
|
||||
}
|
||||
if (targetLines.length > 0) {
|
||||
const preview = [...new Set(targetLines)].slice(0, 5).join("\n");
|
||||
diagnostic += `\nThe file currently contains these lines:\n${preview}\nYour edits were normalized back to the original content (whitespace-only differences are preserved as-is). Ensure your replacement changes actual code, not just formatting.`;
|
||||
}
|
||||
}
|
||||
} catch {
|
||||
// Best-effort diagnostic \u2014 don't crash on malformed refs
|
||||
if (targetLines.length > 0) {
|
||||
const preview = [...new Set(targetLines)].slice(0, 5).join("\n");
|
||||
diagnostic += `\nThe file currently contains these lines:\n${preview}\nYour edits were normalized back to the original content (whitespace-only differences are preserved as-is). Ensure your replacement changes actual code, not just formatting.`;
|
||||
}
|
||||
}
|
||||
throw new Error(diagnostic);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user