diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index fdddbb211..2bbc597a4 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Restored automatic repair of `edit` range hunks that break bracket balance — the failure class that previously left a duplicated closing line (a `` / `);` / `}` echoed just below the range) or dropped one (the range swallowed a `});` the payload never restated), leaving the file syntactically broken until a follow-up edit. The hashline applier now normalizes each replacement so its payload preserves the deleted region's delimiter balance, dropping a duplicated bordering closer or sparing a deleted one, and surfaces a warning on the tool result. Always on and balance-validated (no `edit.hashlineAutoDropPureInsertDuplicates` setting); see `@oh-my-pi/hashline` for the contract. + ## [15.5.12] - 2026-05-29 ### Added diff --git a/packages/coding-agent/test/core/hashline.test.ts b/packages/coding-agent/test/core/hashline.test.ts index 36f5ee304..e36ec1a85 100644 --- a/packages/coding-agent/test/core/hashline.test.ts +++ b/packages/coding-agent/test/core/hashline.test.ts @@ -313,14 +313,20 @@ describe("hashline parser — range-anchor syntax", () => { expect(applyDiff(suffixSource, suffixDiff)).toBe(["new();", "// one", "// two", "// one", "// two"].join("\n")); }); - it("keeps duplicated structural replacement boundaries literal", () => { + it("de-duplicates structural replacement boundaries (balance-validated)", () => { + // `1 1` replaces `old();` but the payload also restates the `};` that + // survives at line 2 — a duplicate close that would unbalance braces. const suffixSource = ["old();", "};"].join("\n"); const suffixDiff = [`${sameLineRange(tag(1, "old();"))}`, repl("new();"), repl("};")].join("\n"); - expect(applyDiff(suffixSource, suffixDiff)).toBe(["new();", "};", "};"].join("\n")); + expect(applyDiff(suffixSource, suffixDiff)).toBe(["new();", "};"].join("\n")); + // Mirror case at the leading edge. const prefixSource = ["};", "old();"].join("\n"); const prefixDiff = [`${sameLineRange(tag(2, "old();"))}`, repl("};"), repl("new();")].join("\n"); - expect(applyDiff(prefixSource, prefixDiff)).toBe(["};", "};", "new();"].join("\n")); + expect(applyDiff(prefixSource, prefixDiff)).toBe(["};", "new();"].join("\n")); + + const result = applyHashlineEdits(suffixSource, parseHashline(suffixDiff).edits); + expect(result.warnings?.some(w => /delimiter-balance/.test(w))).toBe(true); }); it("keeps duplicated single non-structural replacement boundaries literal", () => { diff --git a/packages/hashline/CHANGELOG.md b/packages/hashline/CHANGELOG.md index 063df05e9..55162339a 100644 --- a/packages/hashline/CHANGELOG.md +++ b/packages/hashline/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Added + +- Re-introduced balance-validated boundary repair in `applyEdits`. A replacement hunk (`A B` + body) is normalized so its payload preserves the deleted region's delimiter balance: when the body restates a closing delimiter that survives just outside the range (duplicate `}` / `);` / `]`) the echo is dropped, and when the range deletes a structural closer the body never restates (missing closer) the closer is spared instead of deleted. A repair fires only when one boundary operation drives the per-channel `()` / `[]` / `{}` imbalance to exactly zero while leaving surrounding text byte-identical (single-line ops are limited to pure structural-closer lines), so balance-preserving edits and intentional balanced duplicates are never touched. Bracket counting skips strings, template literals, and comments. Each repair surfaces a `delimiter-balance` warning through `ApplyResult.warnings`. + ## [15.5.12] - 2026-05-29 ### Changed diff --git a/packages/hashline/src/apply.ts b/packages/hashline/src/apply.ts index e2ba809bc..14bf88b4b 100644 --- a/packages/hashline/src/apply.ts +++ b/packages/hashline/src/apply.ts @@ -1,6 +1,11 @@ /** * Apply a parsed list of {@link Edit}s to a text body and return the - * post-edit lines. Pure function: no FS, no mutation of the input. + * post-edit lines plus any diagnostic warnings. Pure function: no FS, no + * mutation of the input. + * + * Replacement groups are first normalized by {@link repairBoundaryBalance}, + * which fixes the common model mistake of a payload that duplicates or drops + * the closing delimiter bordering the range (balance-validated; see below). */ import { cloneCursor } from "./tokenizer"; import type { Anchor, ApplyResult, Cursor, Edit } from "./types"; @@ -132,6 +137,311 @@ function bucketAnchorEditsByLine(edits: IndexedEdit[]): Map= 1; k--) { + let matches = true; + for (let t = 0; t < k; t++) { + if (payload[payload.length - k + t] !== fileLines[endLine + t]) { + matches = false; + break; + } + } + if (!matches) continue; + if (k === 1 && !STRUCTURAL_CLOSER_RE.test(payload[payload.length - 1])) continue; + if (balanceEqual(computeDelimiterBalance(payload.slice(payload.length - k)), delta)) return k; + } + return 0; +} + +/** + * Largest `j` such that the payload's first `j` lines exactly equal the `j` + * surviving file lines just above the range AND dropping them zeroes `delta`. + */ +function findDuplicatePrefix(group: ReplacementGroup, fileLines: readonly string[], delta: DelimiterBalance): number { + const { payload, startLine } = group; + const maxJ = Math.min(payload.length, startLine - 1); + for (let j = maxJ; j >= 1; j--) { + let matches = true; + for (let t = 0; t < j; t++) { + if (payload[t] !== fileLines[startLine - 1 - j + t]) { + matches = false; + break; + } + } + if (!matches) continue; + if (j === 1 && !STRUCTURAL_CLOSER_RE.test(payload[0])) continue; + if (balanceEqual(computeDelimiterBalance(payload.slice(0, j)), delta)) return j; + } + return 0; +} + +/** + * Smallest `m` such that the range's last `m` deleted lines are all pure + * structural closers and sparing them (keeping instead of deleting) zeroes + * `delta`. The mirror mistake: a range that swallows a closing delimiter the + * payload never restates. + */ +function findDroppedSuffixClosers( + group: ReplacementGroup, + fileLines: readonly string[], + delta: DelimiterBalance, +): number { + const wanted = balanceNegate(delta); + const maxM = group.deleteIndices.length; + for (let m = 1; m <= maxM; m++) { + if (!STRUCTURAL_CLOSER_RE.test(fileLines[group.endLine - m] ?? "")) break; + if (balanceEqual(computeDelimiterBalance(fileLines.slice(group.endLine - m, group.endLine)), wanted)) return m; + } + return 0; +} + +function describeBoundaryRepair(group: ReplacementGroup, action: string): string { + return ( + `Auto-repaired a delimiter-balance mismatch in the replacement at line ${group.startLine}: ${action}. ` + + `Issue the payload as the final desired content only — never restate or omit a closing bracket bordering the range.` + ); +} + +/** + * Normalize each replacement group so its payload preserves the deleted + * region's delimiter balance. See the section header for the contract. Returns + * the (possibly trimmed) edit list plus one warning per repaired group. + */ +function repairBoundaryBalance( + edits: readonly AppliedEdit[], + fileLines: readonly string[], +): { + edits: AppliedEdit[]; + warnings: string[]; +} { + const out: AppliedEdit[] = []; + const warnings: string[] = []; + let i = 0; + while (i < edits.length) { + const group = findReplacementGroup(edits, i); + if (!group) { + out.push(edits[i]); + i++; + continue; + } + const inserts = group.insertIndices.map(idx => edits[idx]); + const deletes = group.deleteIndices.map(idx => edits[idx]); + i = group.deleteIndices[group.deleteIndices.length - 1] + 1; + + const delta = balanceDelta( + computeDelimiterBalance(group.payload), + computeDelimiterBalance(fileLines.slice(group.startLine - 1, group.endLine)), + ); + if (balanceIsZero(delta)) { + out.push(...inserts, ...deletes); + continue; + } + + const dupSuffix = findDuplicateSuffix(group, fileLines, delta); + if (dupSuffix > 0) { + warnings.push( + describeBoundaryRepair( + group, + `dropped ${dupSuffix} duplicated trailing payload line(s) already present below the range`, + ), + ); + out.push(...inserts.slice(0, inserts.length - dupSuffix), ...deletes); + continue; + } + const dupPrefix = findDuplicatePrefix(group, fileLines, delta); + if (dupPrefix > 0) { + warnings.push( + describeBoundaryRepair( + group, + `dropped ${dupPrefix} duplicated leading payload line(s) already present above the range`, + ), + ); + out.push(...inserts.slice(dupPrefix), ...deletes); + continue; + } + const droppedClosers = findDroppedSuffixClosers(group, fileLines, delta); + if (droppedClosers > 0) { + warnings.push( + describeBoundaryRepair( + group, + `kept ${droppedClosers} structural closing line(s) the range deleted without restating`, + ), + ); + out.push(...inserts, ...deletes.slice(0, deletes.length - droppedClosers)); + continue; + } + out.push(...inserts, ...deletes); + } + return { edits: out, warnings }; +} + /** * Apply a parsed list of edits to a text body. Pure function — no I/O. * @@ -151,12 +461,13 @@ export function applyEdits(text: string, edits: Edit[]): ApplyResult { const targetEdits = expandRepeatEdits(edits, fileLines); validateLineBounds(targetEdits, fileLines); + const { edits: repaired, warnings } = repairBoundaryBalance(targetEdits, fileLines); // Partition edits into BOF, EOF, and anchor-targeted buckets. const bofLines: string[] = []; const eofLines: string[] = []; const anchorEdits: IndexedEdit[] = []; - targetEdits.forEach((edit, idx) => { + repaired.forEach((edit, idx) => { if (edit.kind === "insert" && edit.cursor.kind === "bof") { bofLines.push(edit.text); } else if (edit.kind === "insert" && edit.cursor.kind === "eof") { @@ -213,5 +524,6 @@ export function applyEdits(text: string, edits: Edit[]): ApplyResult { return { text: fileLines.join("\n"), firstChangedLine, + ...(warnings.length > 0 ? { warnings } : {}), }; } diff --git a/packages/hashline/src/prompt.md b/packages/hashline/src/prompt.md index 4db0121a4..59a6c50e9 100644 --- a/packages/hashline/src/prompt.md +++ b/packages/hashline/src/prompt.md @@ -30,6 +30,7 @@ Every file section starts with `¶PATH#HASH`. `HASH` is the snapshot tag from yo - Line numbers refer to the ORIGINAL file and stay valid for the whole patch — they do not shift as your hunks land. - An empty body **deletes** the selected range entirely. To replace lines A..B with completely new content, list the new content under the hunk header (do not write `&A..B` for the lines you are replacing). - `@@` is NOT a hashline construct. Do not wrap headers in `@@ ... @@` — write the anchor bare. +- Keep `A B` aligned to your body: select exactly the lines the body replaces. Never restate a bordering closer (`}`, `);`, `]`) that survives just outside the range, and never let `A B` swallow a closer your body omits — either unbalances the file. (An obvious off-by-one closer is auto-repaired with a warning; still aim to get the range right.) diff --git a/packages/hashline/test/boundary-repair.test.ts b/packages/hashline/test/boundary-repair.test.ts new file mode 100644 index 000000000..756b814cb --- /dev/null +++ b/packages/hashline/test/boundary-repair.test.ts @@ -0,0 +1,166 @@ +import { describe, expect, it } from "bun:test"; +import { applyEdits, InMemorySnapshotStore, parsePatch, Recovery } from "@oh-my-pi/hashline"; + +function apply(text: string, diff: string): { text: string; warnings: string[] } { + const result = applyEdits(text, parsePatch(diff).edits); + return { text: result.text, warnings: result.warnings ?? [] }; +} + +describe("boundary-balance repair", () => { + // The canonical incident: a range-replace whose payload restates the + // fragment + paren close that still live just below the range, doubling + // `` and `);`. `11 31` covers `const …` through the second `/>`. + it("drops a duplicated multi-line closing block (the Root.tsx incident)", () => { + const file = [ + 'import type React from "react";', + 'import { Composition } from "remotion";', + 'import { Sizzle, type SizzleProps } from "./compositions/Sizzle";', + 'import { FPS, totalDurationInFrames } from "./lib/scenes";', + "", + "export const RemotionRoot: React.FC = () => {", + "\tconst durationInFrames = totalDurationInFrames();", + "\treturn (", + "\t\t<>", + "\t\t\t", + "\t\t", + "\t);", + "};", + ].join("\n"); + // Range 7..16 = `const …` through the first `/>`; payload restates the + // `` + `);` that survive at lines 17-18. + const diff = [ + "7 16", + "+\treturn (", + "+\t\t<>", + "+\t\t\t", + "+\t\t", + "+\t);", + ].join("\n"); + const { text, warnings } = apply(file, diff); + // Exactly one `` and one `);` survive — no doubling. + expect(text.split("\n").filter(l => l.trim() === "")).toHaveLength(1); + expect(text.split("\n").filter(l => l.trim() === ");")).toHaveLength(1); + expect(text.endsWith("\t\t\n\t);\n};")).toBe(true); + expect(warnings.some(w => /delimiter-balance/.test(w))).toBe(true); + }); + + // Single structural-closer duplication: the range ends one line short and + // the payload restates the `});` that survives just below it. + it("drops a single duplicated structural closer (`});`)", () => { + const file = ["it('a', () => {", "\tsetup();", "\trun();", "});", "after();"].join("\n"); + // `2 3` replaces the two body lines but the payload also restates the + // `});` at line 4, which survives — a duplicate close. + const diff = ["2 3", "+\tsetup2();", "+\trun2();", "+});"].join("\n"); + const { text, warnings } = apply(file, diff); + expect(text).toBe(["it('a', () => {", "\tsetup2();", "\trun2();", "});", "after();"].join("\n")); + expect(warnings.some(w => /delimiter-balance/.test(w))).toBe(true); + }); + + // Genuine missing-closer: payload omits the trailing `});`. + it("spares the deleted closing line when the payload omits it", () => { + const file = ["const handlers = {", "\ta() {", "\t\treturn 1;", "\t},", "};"].join("\n"); + // `5 5` is the final `};`. Model inserts a new method but forgets to + // restate `};`; sparing it keeps the object literal balanced. + const diff = ["5 5", "+\tb() {", "+\t\treturn 2;", "+\t},"].join("\n"); + const { text, warnings } = apply(file, diff); + expect(text).toBe( + ["const handlers = {", "\ta() {", "\t\treturn 1;", "\t},", "\tb() {", "\t\treturn 2;", "\t},", "};"].join( + "\n", + ), + ); + expect(warnings.some(w => /delimiter-balance/.test(w))).toBe(true); + }); + + // Balance-preserving edits are never touched, even when the payload's last + // line coincidentally equals the line just below the range. + it("leaves a balance-preserving replacement alone (no false positive)", () => { + const file = ["foo();", "bar();", "bar();", "baz();"].join("\n"); + // Replace line 2 with two balanced statements; the tail `bar();` equals + // the surviving line 3 but the payload is balanced — must NOT be dropped. + const diff = ["2 2", "+qux();", "+bar();"].join("\n"); + const { text, warnings } = apply(file, diff); + expect(text).toBe(["foo();", "qux();", "bar();", "bar();", "baz();"].join("\n")); + expect(warnings).toHaveLength(0); + }); + + // A duplicated full statement (balance-neutral) is left intact: dropping it + // could discard intended content, and it does not break syntax. + it("does not drop a balance-neutral duplicated statement", () => { + const file = ["a = 1;", "b = 2;", "c = 3;"].join("\n"); + const diff = ["1 1", "+a = 1;", "+b = 2;"].join("\n"); + const { text, warnings } = apply(file, diff); + expect(text).toBe(["a = 1;", "b = 2;", "b = 2;", "c = 3;"].join("\n")); + expect(warnings).toHaveLength(0); + }); + + // Brackets inside strings must not trigger a spurious balance mismatch. + it("ignores brackets inside string literals", () => { + const file = ['const a = "}";', 'const b = "x";', 'const c = "y";'].join("\n"); + const diff = ["2 2", '+const b = "}}}";'].join("\n"); + const { text, warnings } = apply(file, diff); + expect(text).toBe(['const a = "}";', 'const b = "}}}";', 'const c = "y";'].join("\n")); + expect(warnings).toHaveLength(0); + }); +}); + +describe("boundary-balance repair through stale-snapshot recovery", () => { + const PATH = "/tmp/__hashline-boundary-recovery__.ts"; + + // Recovery composes `applyEdits` to compute the intended change, so the + // boundary repair runs there too. The snapshot (what the model read) + // carries the structure; the live file has drifted far from the edit + // region, so the stale-hash 3-way merge succeeds and the repaired + // (de-duplicated) hunk lands without doubling the closer. + it("de-duplicates a closer while recovering from a drifted file", () => { + const snapshotLines = [ + 'import { x } from "y";', + "", + "it('a', () => {", + "\tsetup();", + "\trun();", + "});", + "", + "function filler1() { return 1; }", + "function filler2() { return 2; }", + "function filler3() { return 3; }", + "function filler4() { return 4; }", + "function filler5() { return 5; }", + "const tail = 0;", + "export { tail };", + ]; + const snapshotText = `${snapshotLines.join("\n")}\n`; + // Live file drifted only at the tail (line 13) — far outside the edit + // region (lines 4-6), so the 3-way merge applies cleanly. + const currentText = snapshotText.replace("const tail = 0;", "const tail = 99;"); + + const store = new InMemorySnapshotStore(); + const fileHash = store.recordContiguous(PATH, 1, snapshotText.split("\n"), { fullText: snapshotText }); + + // `4 5` replaces the body lines but the payload also restates the `});` + // that survives at line 6 — the duplicate-closer mistake. + const { edits } = parsePatch(["4 5", "+\tsetup2();", "+\trun2();", "+});"].join("\n")); + const recovered = new Recovery(store).tryRecover({ path: PATH, currentText, fileHash, edits }); + + expect(recovered).not.toBeNull(); + // Exactly one `});` — the duplicate was absorbed during recovery. + expect(recovered?.text.split("\n").filter(l => l === "});")).toHaveLength(1); + expect(recovered?.text).toContain("setup2();"); + expect(recovered?.text).toContain("run2();"); + // The unrelated drift on the live file survives the merge. + expect(recovered?.text).toContain("const tail = 99;"); + // The repair warning propagates out through the recovery result. + expect(recovered?.warnings.some(w => /delimiter-balance/.test(w))).toBe(true); + }); +});