fix(coding-agent/edit): added atom range-repair and multi-section preflight checks
- Allowed bare `LidA..LidB` to recover a missing-range-delete typo and accepted `|` as a legacy range replacement separator while validating ranges and replacement text. - Enabled indented hashline statements to parse as replacement edits and added a preflight pass that validates all atom sections before any file write occurs. - Updated hash-mismatch messaging, prompt wording, and tests to reflect hash-only rebase candidates and the new range/section behaviors.
This commit is contained in:
can1357
@@ -148,39 +148,46 @@ function parseLidStmt(body: string, lineNum: number): ParsedStmt[] | null {
|
||||
|
||||
// Range replace: `LidA..LidB=TEXT` deletes the inclusive range LidA..LidB
|
||||
// and inserts TEXT in its place. Following insert statements append more
|
||||
// replacement lines through the normal hunk reorder path.
|
||||
// replacement lines through the normal hunk reorder path. Legacy `|` is
|
||||
// accepted as a set separator for parity with single-line `Lid|TEXT`.
|
||||
// Bare `LidA..LidB` recovers the common missing-`-` typo for range delete.
|
||||
if (rest.startsWith("..")) {
|
||||
const m2 = LID_RE.exec(rest.slice(2));
|
||||
if (m2) {
|
||||
const endLn = Number.parseInt(m2[1], 10);
|
||||
const endHash = m2[2];
|
||||
const after = rest.slice(2 + m2[0].length);
|
||||
const eq = /^[ \t]*=(.*)$/.exec(after);
|
||||
if (eq) {
|
||||
if (endLn < ln) {
|
||||
throw new Error(
|
||||
`Diff line ${lineNum}: range \`${ln}${hash}..${endLn}${endHash}\` ends before it starts. Use \`LidA..LidB=TEXT\` with LidA's line number ≤ LidB's.`,
|
||||
);
|
||||
}
|
||||
if (endLn === ln && endHash !== hash) {
|
||||
throw new Error(
|
||||
`Diff line ${lineNum}: range \`${ln}${hash}..${endLn}${endHash}\` uses two different hashes for the same line. Copy the same Lid at both endpoints or use \`${ln}${hash}=TEXT\` for a single-line replacement.`,
|
||||
);
|
||||
}
|
||||
if (eq[1].includes("\r")) {
|
||||
const range = `${ln}${hash}..${endLn}${endHash}`;
|
||||
if (endLn < ln) {
|
||||
throw new Error(
|
||||
`Diff line ${lineNum}: range \`${range}\` ends before it starts. Use \`LidA..LidB=TEXT\` with LidA's line number ≤ LidB's.`,
|
||||
);
|
||||
}
|
||||
if (endLn === ln && endHash !== hash) {
|
||||
throw new Error(
|
||||
`Diff line ${lineNum}: range \`${range}\` uses two different hashes for the same line. Copy the same Lid at both endpoints or use \`${ln}${hash}=TEXT\` for a single-line replacement.`,
|
||||
);
|
||||
}
|
||||
|
||||
const stmts: ParsedStmt[] = [];
|
||||
for (let l = ln; l <= endLn; l++) {
|
||||
const h = l === ln ? hash : l === endLn ? endHash : RANGE_INTERIOR_HASH;
|
||||
stmts.push({
|
||||
kind: "anchor_op",
|
||||
anchor: { line: l, hash: h },
|
||||
op: { op: "delete" },
|
||||
lineNum,
|
||||
});
|
||||
}
|
||||
|
||||
if (after.trim().length === 0) return stmts;
|
||||
|
||||
const replacement = /^[ \t]*([=|])(.*)$/.exec(after);
|
||||
if (replacement) {
|
||||
if (replacement[2].includes("\r")) {
|
||||
throw new Error(`Diff line ${lineNum}: set value contains a carriage return; use a single-line value.`);
|
||||
}
|
||||
const stmts: ParsedStmt[] = [];
|
||||
for (let l = ln; l <= endLn; l++) {
|
||||
const h = l === ln ? hash : l === endLn ? endHash : RANGE_INTERIOR_HASH;
|
||||
stmts.push({
|
||||
kind: "anchor_op",
|
||||
anchor: { line: l, hash: h },
|
||||
op: { op: "delete" },
|
||||
lineNum,
|
||||
});
|
||||
}
|
||||
stmts.push({ kind: "insert", text: eq[1], lineNum });
|
||||
stmts.push({ kind: "insert", text: replacement[2], lineNum });
|
||||
return stmts;
|
||||
}
|
||||
}
|
||||
@@ -289,6 +296,17 @@ function parseDeleteStmt(body: string, lineNum: number): ParsedStmt[] | null {
|
||||
return null;
|
||||
}
|
||||
|
||||
function parseIndentedHashlineStmt(line: string, lineNum: number): ParsedStmt[] | null {
|
||||
const trimmed = line.trimStart();
|
||||
if (trimmed === line) return null;
|
||||
const stmts = parseLidStmt(trimmed, lineNum);
|
||||
if (!stmts) return null;
|
||||
const safeHashlineEcho = stmts.every(
|
||||
stmt => stmt.kind === "bare_anchor" || (stmt.kind === "anchor_op" && stmt.op.op === "set"),
|
||||
);
|
||||
return safeHashlineEcho ? stmts : null;
|
||||
}
|
||||
|
||||
function throwMalformedLidDiagnostic(line: string, lineNum: number, raw: string): never {
|
||||
const text = line.trimStart();
|
||||
const withoutLegacyMove = text.startsWith("@@ ") ? text.slice(3).trimStart() : text;
|
||||
@@ -323,6 +341,9 @@ function parseDiffLine(raw: string, lineNum: number): ParsedStmt[] {
|
||||
// inserts corrupts files, and the canonical syntax has no comment op.
|
||||
if (line[0] === "#") return [];
|
||||
|
||||
const indentedHashline = parseIndentedHashlineStmt(line, lineNum);
|
||||
if (indentedHashline) return indentedHashline;
|
||||
|
||||
// `+TEXT` inserts at the cursor. Everything after `+` is content. A
|
||||
// `+Lid|TEXT` or `+Lid=TEXT` line is a diff-ish add (unified-diff trap):
|
||||
// emit a tagged stmt so the normalizer can fuse it with a preceding `-Lid`.
|
||||
@@ -476,13 +497,13 @@ function parseDiffLine(raw: string, lineNum: number): ParsedStmt[] {
|
||||
}
|
||||
|
||||
// Lines that look like recognized atom ops. Used to delimit range-replace
|
||||
// recovery continuation: after `LidA..LidB=TEXT`, an unprefixed non-op line
|
||||
// recovery continuation: after `LidA..LidB=TEXT` (or legacy `|` separator),
|
||||
// is treated as literal replacement text for backward compatibility.
|
||||
const OP_LINE_HEAD_RE = /^([+\-@$^!]|[1-9]\d*[a-z]{2}|[ \t]*$)/;
|
||||
const RANGE_CONTINUATION_SENTINEL = "\u0000";
|
||||
|
||||
function isRangeReplaceStart(line: string): boolean {
|
||||
return /^[1-9]\d*[a-z]{2}\.\.[1-9]\d*[a-z]{2}[ \t]*=/.test(line);
|
||||
return /^[1-9]\d*[a-z]{2}\.\.[1-9]\d*[a-z]{2}[ \t]*[=|]/.test(line);
|
||||
}
|
||||
|
||||
// A single-line `Lid=TEXT` (or legacy `Lid|TEXT`, with optional leading `@`)
|
||||
@@ -1534,6 +1555,60 @@ async function executeAtomWholeFileOperation(
|
||||
};
|
||||
}
|
||||
|
||||
async function preflightAtomSection(options: ExecuteAtomSingleOptions & AtomInputSection): Promise<void> {
|
||||
const { session, path: sectionPath, diff } = options;
|
||||
if (options.wholeFileOperation) {
|
||||
const { wholeFileOperation } = options;
|
||||
const absolutePath = resolvePlanPath(session, sectionPath);
|
||||
if (sectionPath.endsWith(".ipynb")) {
|
||||
throw new Error("Cannot edit Jupyter notebooks with the Edit tool. Use the NotebookEdit tool instead.");
|
||||
}
|
||||
if (wholeFileOperation.kind === "delete") {
|
||||
enforcePlanModeWrite(session, sectionPath, { op: "delete" });
|
||||
await assertEditableFile(absolutePath, sectionPath);
|
||||
return;
|
||||
}
|
||||
|
||||
const destinationPath = wholeFileOperation.destination;
|
||||
if (destinationPath.endsWith(".ipynb")) {
|
||||
throw new Error("Cannot edit Jupyter notebooks with the Edit tool. Use the NotebookEdit tool instead.");
|
||||
}
|
||||
enforcePlanModeWrite(session, sectionPath, { op: "update", move: destinationPath });
|
||||
const absoluteDestinationPath = resolvePlanPath(session, destinationPath);
|
||||
if (absoluteDestinationPath === absolutePath) {
|
||||
throw new Error("rename path is the same as source path");
|
||||
}
|
||||
await assertEditableFile(absolutePath, sectionPath);
|
||||
return;
|
||||
}
|
||||
|
||||
const { edits } = parseAtomWithWarnings(diff);
|
||||
if (edits.length === 0 && diff.trim().length > 0) {
|
||||
throw new Error(formatNoAtomEditDiagnostic(sectionPath, diff));
|
||||
}
|
||||
|
||||
enforcePlanModeWrite(session, sectionPath, { op: "update" });
|
||||
if (sectionPath.endsWith(".ipynb") && edits.length > 0) {
|
||||
throw new Error("Cannot edit Jupyter notebooks with the Edit tool. Use the NotebookEdit tool instead.");
|
||||
}
|
||||
|
||||
const absolutePath = resolvePlanPath(session, sectionPath);
|
||||
const source = await readAtomFile(absolutePath);
|
||||
if (!source.exists && hasAnchorScopedEdit(edits)) {
|
||||
throw new Error(`File not found: ${sectionPath}`);
|
||||
}
|
||||
if (source.exists) {
|
||||
assertEditableFileContent(source.rawContent, sectionPath);
|
||||
}
|
||||
|
||||
const { text } = stripBom(source.rawContent);
|
||||
const originalNormalized = normalizeToLF(text);
|
||||
const result = applyAtomEdits(originalNormalized, edits);
|
||||
if (originalNormalized === result.lines && (result.noopEdits?.length ?? 0) === 0) {
|
||||
throw new Error(formatNoChangeDiagnostic(sectionPath, result));
|
||||
}
|
||||
}
|
||||
|
||||
async function executeAtomSection(
|
||||
options: ExecuteAtomSingleOptions & AtomInputSection,
|
||||
): Promise<AgentToolResult<EditToolDetails, typeof atomEditParamsSchema>> {
|
||||
@@ -1629,6 +1704,10 @@ export async function executeAtomSingle(
|
||||
return executeAtomSection({ ...options, ...section });
|
||||
}
|
||||
|
||||
for (const section of sections) {
|
||||
await preflightAtomSection({ ...options, ...section });
|
||||
}
|
||||
|
||||
const results = [];
|
||||
for (const section of sections) {
|
||||
results.push({
|
||||
|
||||
@@ -606,9 +606,9 @@ export class HashlineMismatchError extends Error {
|
||||
"The edit was NOT applied, please use the updated file content shown below, and issue another edit tool-call.",
|
||||
);
|
||||
|
||||
// Content-based recovery hint: if the original hash uniquely matches a
|
||||
// line elsewhere in the file (outside the auto-rebase window), suggest
|
||||
// the new Lid so the model can retry without a full re-read.
|
||||
// Content-based recovery hint: the two-letter hash is weak, so a
|
||||
// unique match elsewhere is only a candidate. Keep this advisory; never
|
||||
// silently retarget stale edits based on a whole-file hash-only match.
|
||||
const hints: string[] = [];
|
||||
for (const m of mismatches) {
|
||||
const matches: number[] = [];
|
||||
@@ -621,7 +621,7 @@ export class HashlineMismatchError extends Error {
|
||||
}
|
||||
}
|
||||
if (hints.length > 0) {
|
||||
lines.push("Likely shifted (hash matched a unique line elsewhere):");
|
||||
lines.push("Hash-only shifted candidate; verify content/context before using:");
|
||||
lines.push(...hints);
|
||||
}
|
||||
|
||||
|
||||
@@ -35,7 +35,7 @@ Lid= blank the anchored line's content but KEEP the line (results in an em
|
||||
- To insert ABOVE a line, you **MUST** use `^Lid` then `+TEXT`. To insert above line 1, you **MUST** use `^` (BOF) then `+TEXT`. To insert below a line, you **MUST** use `@Lid` then `+TEXT`.
|
||||
- Multiple `---PATH` sections **MAY** appear in one input; each section is applied in order.
|
||||
- `!rm` / `!mv DEST` **MUST NOT** be combined with line edits in the same section.
|
||||
- Lids contain a content hash. If a line has changed since you read it, the tool rejects the edit and shows the current content; you **MUST** re-read and retry with fresh Lids. Small drift (≤5 lines) where the original hash still matches a nearby line auto-rebases with a warning; larger shifts require a re-read.
|
||||
- Lids contain a content hash. If a line has changed since you read it, the tool rejects the edit and shows the current content; you **MUST** re-read and retry with fresh Lids. Small drift (≤5 lines) where the original hash still matches a nearby line auto-rebases with a warning. Larger shifts may show a hash-only candidate, but two-letter hashes collide; verify surrounding content or re-read before using it.
|
||||
- After `+TEXT` (or `+`) the cursor advances past the inserted line, so consecutive `+TEXT` ops stack in order. After `Lid=TEXT` the cursor sits on the modified anchor; after `-Lid` it sits on the slot the deleted line vacated. You **MUST** use a fresh `@Lid` / `^Lid` / `^` / `$` to reposition.
|
||||
- The tool is syntax-blind: it will not check brackets, indentation, table column counts, or fence integrity. You **MUST** verify indentation-sensitive or structured files after editing (Python, Markdown tables/fences).
|
||||
- A section whose PATH does not yet exist creates the file from your `+TEXT` lines (use `^` or `$` then `+TEXT…`). No separate "create file" op is needed.
|
||||
|
||||
@@ -112,6 +112,12 @@ describe("atom parser — basic forms", () => {
|
||||
expect(applyDiff(longer, diff)).toBe("aaa\nREPLACED\neee");
|
||||
});
|
||||
|
||||
it("bare `LidA..LidB` recovers a missing `-` range delete typo", () => {
|
||||
const longer = "aaa\nbbb\nccc\nddd\neee";
|
||||
const diff = `${tag(2, "bbb")}..${tag(4, "ddd")}`;
|
||||
expect(applyDiff(longer, diff)).toBe("aaa\neee");
|
||||
});
|
||||
|
||||
it("`-LidA..LidB` rejects a reversed range", () => {
|
||||
const longer = "aaa\nbbb\nccc\nddd";
|
||||
const diff = `-${tag(3, "ccc")}..${tag(2, "bbb")}`;
|
||||
@@ -180,6 +186,12 @@ describe("atom parser — basic forms", () => {
|
||||
expect(applyDiff(longer, diff)).toBe("aaa\nexport function label() {\n return 1;\n}\neee");
|
||||
});
|
||||
|
||||
it("`LidA..LidB|FIRST` accepts legacy pipe as range replacement separator", () => {
|
||||
const longer = "aaa\nbbb\nccc\nddd\neee";
|
||||
const diff = [`${tag(2, "bbb")}..${tag(4, "ddd")}|ONE`, "\\TWO"].join("\n");
|
||||
expect(applyDiff(longer, diff)).toBe("aaa\nONE\nTWO\neee");
|
||||
});
|
||||
|
||||
it("bare backslash continuation inserts a blank replacement line", () => {
|
||||
const longer = "aaa\nbbb\nccc\nddd\neee";
|
||||
const diff = [`${tag(2, "bbb")}..${tag(4, "ddd")}=ONE`, "\\", "\\THREE"].join("\n");
|
||||
@@ -521,6 +533,15 @@ describe("atom parser — edge cases", () => {
|
||||
expect(applyDiff(content, diff)).toBe("aaa\nBBB\nccc");
|
||||
});
|
||||
|
||||
it("repairs indented read-output lines as hashline replacements", () => {
|
||||
const content = '{\n "mode": "demo",\n "strict": true';
|
||||
const t1 = tag(1, "{");
|
||||
const t2 = tag(2, ' "mode": "demo",');
|
||||
const t3 = tag(3, ' "strict": true');
|
||||
const diff = `${t1}|{\n ${t2}| "mode": "demo2",\n ${t3}| "strict": true`;
|
||||
expect(applyDiff(content, diff)).toBe('{\n "mode": "demo2",\n "strict": true');
|
||||
});
|
||||
|
||||
it("same-line OLD|NEW repair works through `@` prefix slip", () => {
|
||||
const t = tag(2, "bbb");
|
||||
const diff = `@${t}|bbb|BBB`;
|
||||
@@ -965,6 +986,22 @@ describe("atom executor — whole-file operations", () => {
|
||||
expect(await Bun.file(path.join(tempDir, "file.ts")).text()).toBe(content);
|
||||
});
|
||||
});
|
||||
|
||||
it("preflights all sections before writing a multi-file edit", async () => {
|
||||
await withTempDir(async tempDir => {
|
||||
const aPath = path.join(tempDir, "a.ts");
|
||||
const bPath = path.join(tempDir, "b.ts");
|
||||
await Bun.write(aPath, "aaa\n");
|
||||
await Bun.write(bPath, "bbb\n");
|
||||
|
||||
const input = [`---a.ts`, `${tag(1, "aaa")}=AAA`, `---b.ts`, `${mistag(1, "bbb")}=BBB`].join("\n");
|
||||
await expect(executeAtomSingle(atomExecuteOptions(tempDir, input))).rejects.toThrow(
|
||||
/changed since the last read/,
|
||||
);
|
||||
expect(await Bun.file(aPath).text()).toBe("aaa\n");
|
||||
expect(await Bun.file(bPath).text()).toBe("bbb\n");
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// ───────────────────────────────────────────────────────────────────────────
|
||||
|
||||
@@ -696,7 +696,7 @@ describe("applyHashlineEdits — errors", () => {
|
||||
}
|
||||
});
|
||||
|
||||
it("stale hash error suggests new Lid when content moved", () => {
|
||||
it("stale hash error suggests a hash-only candidate when content moved", () => {
|
||||
// Original line: "moved" was at line 2. Now it's at line 8 (way beyond ±5 rebase).
|
||||
// Caller still references it via the old `2<hash>` lid.
|
||||
const content = "aaa\nbbb\nccc\nddd\neee\nfff\nggg\nmoved\niii";
|
||||
@@ -708,7 +708,7 @@ describe("applyHashlineEdits — errors", () => {
|
||||
} catch (err) {
|
||||
expect(err).toBeInstanceOf(HashlineMismatchError);
|
||||
const msg = (err as HashlineMismatchError).message;
|
||||
expect(msg).toContain("Likely shifted");
|
||||
expect(msg).toContain("Hash-only shifted candidate");
|
||||
expect(msg).toContain(`2${movedHash} → 8${movedHash}`);
|
||||
}
|
||||
});
|
||||
|
||||
@@ -3,7 +3,7 @@
|
||||
*/
|
||||
|
||||
import { formatDuration, formatPercent, truncate } from "@oh-my-pi/pi-utils";
|
||||
import { EDIT_FAILURE_CATEGORIES, type BenchmarkResult, type TaskResult } from "./runner";
|
||||
import { type BenchmarkResult, EDIT_FAILURE_CATEGORIES, type TaskResult } from "./runner";
|
||||
|
||||
function getStatusEmoji(successRate: number, runsPerTask: number): string {
|
||||
const passing = Math.round(successRate * runsPerTask);
|
||||
|
||||
Reference in New Issue
Block a user