From 7e5e7e864d835a307eb608e6007dfdbcbfa5a1d5 Mon Sep 17 00:00:00 2001 From: can1357 Date: Sat, 11 Jul 2026 17:04:23 +0200 Subject: [PATCH] feat(coding-agent-tools): implemented auto-trimming for echo lines - Implemented logic to automatically detect and trim redundant lines duplicating adjacent file content within conflict markers. - Added delimiter balancing and boundary tracking to ensure accurate removal of echoed text while preserving EOL formatting. - Updated user feedback to report the number of trimmed echo lines during write and conflict resolution operations. - Expanded test coverage to include multi-line echo scenarios and integration validation of the repair process. --- packages/coding-agent/CHANGELOG.md | 1 + .../coding-agent/src/tools/conflict-detect.ts | 102 ++++++++++- packages/coding-agent/src/tools/write.ts | 17 +- .../test/tools/conflict-detect.test.ts | 166 +++++++++++++++++- .../test/tools/conflict-integration.test.ts | 34 ++++ 5 files changed, 308 insertions(+), 12 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 39777c975..23cf1d4d7 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -29,6 +29,7 @@ ### Fixed +- Fixed `write conflict://` duplicating code when the model pastes the "whole resolved function" including lines adjacent to the marker block: replacement lines that exactly echo the context directly above/below the recorded region are now dropped (multi-line echoes always; single-line echoes only when removal restores the recorded sides' delimiter balance), with a note in the tool result. The conflict footer now also states that writes replace only the marker block and to prefer the minimal merge of the recorded sides. - Fixed visible per-keystroke lag while searching in the `/resume` session picker. Literal matches now rank synchronously from a cached per-session haystack, fuzzy scoring runs in bounded background chunks that converge to the same ranking (large listings previously rebuilt a fuzzy index per token per session on every keystroke), and the prompt-history SQLite lookup — an FTS query plus a LIKE scan over every stored prompt — is debounced off the keystroke path. - Fixed compiled Linux binary extension loading when bundled web-search header generation cannot read `header-generator` data files from the build-time path. ([#5178](https://github.com/can1357/oh-my-pi/issues/5178)) - Fixed `job` `list`/empty-poll snapshots returning empty output: a no-job result now says so explicitly, and both snapshots list running subagents that have no backing job (agents woken via `irc`, spawns owned by another agent) with an `irc` coordination hint, so the tool's picture matches the UI's running-agent badge. Polling an agent id that has no job now explains the agent's registry state instead of a bare "no matching jobs". diff --git a/packages/coding-agent/src/tools/conflict-detect.ts b/packages/coding-agent/src/tools/conflict-detect.ts index 4953445d8..3b800972c 100644 --- a/packages/coding-agent/src/tools/conflict-detect.ts +++ b/packages/coding-agent/src/tools/conflict-detect.ts @@ -319,6 +319,15 @@ export function parseConflictUri(raw: string): ParsedConflictUri | null { return recoveredPrefix !== undefined ? { id, scope, recoveredPrefix } : { id, scope }; } +/** Result of {@link spliceConflict}: the new file text plus any boundary-echo repair applied. */ +export interface ConflictSplice { + text: string; + /** Replacement lines dropped because they duplicated the context directly above the region. */ + trimmedLeading: number; + /** Replacement lines dropped because they duplicated the context directly below the region. */ + trimmedTrailing: number; +} + /** * Splice the conflict region recorded in `entry` out of `originalText` * and replace it with `replacement` (markers and all sides included). @@ -328,8 +337,16 @@ export function parseConflictUri(raw: string): ParsedConflictUri | null { * match), so out-of-band edits earlier in the file that shift line * numbers don't break resolution. Throws clearly when the marker block * has actually been altered or removed. + * + * Boundary-echo repair (same philosophy as the edit tool's hashline + * keeper repair): models frequently paste the "whole resolved function" + * including the lines that live directly before/after the marker block, + * which the verbatim splice would duplicate. Replacement lines that + * exactly echo the adjacent context are dropped when the echo is + * unambiguous — two or more consecutive lines, or a single line whose + * removal fixes a delimiter-balance mismatch against the recorded sides. */ -export function spliceConflict(originalText: string, entry: ConflictEntry, replacement: string): string { +export function spliceConflict(originalText: string, entry: ConflictEntry, replacement: string): ConflictSplice { const lines = originalText.split("\n"); const expected = buildRecordedRegion(entry); const match = locateRegion(lines, expected, entry.startLine - 1); @@ -341,6 +358,8 @@ export function spliceConflict(originalText: string, entry: ConflictEntry, repla const trimmed = normalizeTrailingNewline(replacement); let replacementLines = trimmed.split("\n").map(stripTrailingCr); + const echo = trimBoundaryEcho(replacementLines, lines, match, entry); + replacementLines = echo.lines; // Round-trip fidelity for CRLF files: recorded sections are LF-normalized, // so re-apply \r to spliced lines when the matched region used CRLF. The // final replacement line only carries \r when another line follows it. @@ -351,7 +370,79 @@ export function spliceConflict(originalText: string, entry: ConflictEntry, repla ); } const next = [...lines.slice(0, match.startIdx), ...replacementLines, ...lines.slice(match.endIdx + 1)]; - return next.join("\n"); + return { text: next.join("\n"), trimmedLeading: echo.leading, trimmedTrailing: echo.trailing }; +} + +const MAX_ECHO_LINES = 12; + +/** + * Net `{}`/`()`/`[]` count over `lines`. Crude (string/comment-blind) — + * used only to corroborate single-line echo trims, never alone. + */ +function delimiterBalance(lines: readonly string[]): number { + let balance = 0; + for (const line of lines) { + for (let i = 0; i < line.length; i++) { + const ch = line.charCodeAt(i); + if (ch === 123 /* { */ || ch === 40 /* ( */ || ch === 91 /* [ */) balance++; + else if (ch === 125 /* } */ || ch === 41 /* ) */ || ch === 93 /* ] */) balance--; + } + } + return balance; +} + +/** + * Drop replacement lines that exactly echo the file lines adjacent to the + * located region. A multi-line echo is trimmed unconditionally (a correct + * resolution ending with the exact lines that already follow the region + * would mean intentionally duplicated code — vanishingly unlikely, and the + * untrimmed splice produces exactly that duplication). A single-line echo + * is trimmed only when the recorded sides agree on the region's delimiter + * balance and dropping the echo is what restores it. + */ +function trimBoundaryEcho( + replacement: string[], + fileLines: readonly string[], + match: { startIdx: number; endIdx: number }, + entry: ConflictBlock, +): { lines: string[]; leading: number; trailing: number } { + const oursBalance = delimiterBalance(entry.oursLines); + const expectedBalance = oursBalance === delimiterBalance(entry.theirsLines) ? oursBalance : null; + const singleEchoJustified = (lines: string[], without: string[]) => + expectedBalance !== null && + delimiterBalance(lines) !== expectedBalance && + delimiterBalance(without) === expectedBalance; + + let lines = replacement; + let trailing = 0; + const after: string[] = []; + for (let i = match.endIdx + 1; i < fileLines.length && after.length < MAX_ECHO_LINES; i++) { + after.push(stripTrailingCr(fileLines[i]!)); + } + for (let k = Math.min(after.length, lines.length - 1); k >= 1; k--) { + if (!after.slice(0, k).every((line, i) => lines[lines.length - k + i] === line)) continue; + if (k >= 2 || singleEchoJustified(lines, lines.slice(0, -1))) { + trailing = k; + lines = lines.slice(0, lines.length - k); + } + break; + } + + let leading = 0; + const before: string[] = []; + for (let i = match.startIdx - 1; i >= 0 && before.length < MAX_ECHO_LINES; i--) { + before.unshift(stripTrailingCr(fileLines[i]!)); + } + for (let k = Math.min(before.length, lines.length - 1); k >= 1; k--) { + if (!before.slice(before.length - k).every((line, i) => lines[i] === line)) continue; + if (k >= 2 || singleEchoJustified(lines, lines.slice(1))) { + leading = k; + lines = lines.slice(k); + } + break; + } + + return { lines, leading, trailing }; } /** Reconstruct the recorded marker block as it should appear in the file. */ @@ -608,11 +699,14 @@ export function formatConflictWarning( if (theirsLabel) out.push(`- theirs = ${theirsLabel}`); if (anyBase) out.push(`- base = ${baseLabel ?? "(no label)"}`); out.push( - 'NOTICE: Inspect a block by reading `conflict://` (add `/ours` / `/theirs` / `/base` to render a single side). Resolve with `write({ path: "conflict://", content })`, or bulk-resolve every registered conflict with `write({ path: "conflict://*", content })`. Writes replace the whole conflict region (markers + all sides).', + 'NOTICE: Inspect a block by reading `conflict://` (add `/ours` / `/theirs` / `/base` to render a single side). Resolve with `write({ path: "conflict://", content })`, or bulk-resolve every registered conflict with `write({ path: "conflict://*", content })`. Writes replace ONLY the marker block (markers + all sides) — never repeat the lines before/after it; they stay in place.', ); out.push( '`content` shorthand: a line that is exactly `@ours` / `@theirs` / `@base` / `@both` expands to that recorded section. `@both` is ours-then-theirs with no separator. Lines that are not a token pass through verbatim, so `"// keep both\\n@ours\\n@theirs"` literally writes the comment, then ours, then theirs.', ); + out.push( + "Resolve with the minimal merge of the recorded sides — do not introduce logic that neither side contains.", + ); for (const entry of entries) { const range = entry.startLine === entry.endLine ? `L${entry.startLine}` : `L${entry.startLine}-${entry.endLine}`; @@ -674,7 +768,7 @@ export function formatConflictSummary( 'NOTICE: Bulk-resolve with `write({ path: "conflict://*", content })`, or address a single block with `write({ path: "conflict://", content })`. Inspect a block by reading `conflict://` (add `/ours` / `/theirs` / `/base` for a single side).', ); lines.push( - "`content` shorthand: `@ours` / `@theirs` / `@base` / `@both` lines expand to the recorded sections; `@both` = ours-then-theirs. Non-token lines pass through verbatim.", + "`content` shorthand: `@ours` / `@theirs` / `@base` / `@both` lines expand to the recorded sections; `@both` = ours-then-theirs. Non-token lines pass through verbatim. Writes replace ONLY the marker block — never repeat the surrounding lines. Prefer the minimal merge of the recorded sides.", ); lines.push(""); const idWidth = String(entries[entries.length - 1]?.id ?? 1).length; diff --git a/packages/coding-agent/src/tools/write.ts b/packages/coding-agent/src/tools/write.ts index 348384eed..9181b0c2d 100644 --- a/packages/coding-agent/src/tools/write.ts +++ b/packages/coding-agent/src/tools/write.ts @@ -591,7 +591,8 @@ export class WriteTool implements AgentTool 0) { + resultText += `\nNote: dropped ${echoTrimmed} content line(s) that duplicated the code adjacent to the conflict region — writes replace only the marker block; surrounding lines stay in place.`; + } return { content: [{ type: "text", text: resultText }], @@ -690,6 +695,7 @@ export class WriteTool implements AgentTool 0) { + summaryLines.push( + `Note: dropped ${totalEchoTrimmed} content line(s) that duplicated code adjacent to conflict regions — writes replace only the marker block; surrounding lines stay in place.`, + ); + } if (failedFiles.length > 0) { summaryLines.push( `Failed to resolve ${failedFiles.length} ${fileWord(failedFiles.length)} — registered entries left intact for retry:`, diff --git a/packages/coding-agent/test/tools/conflict-detect.test.ts b/packages/coding-agent/test/tools/conflict-detect.test.ts index e91181d62..4e038c25a 100644 --- a/packages/coding-agent/test/tools/conflict-detect.test.ts +++ b/packages/coding-agent/test/tools/conflict-detect.test.ts @@ -273,23 +273,23 @@ describe("spliceConflict", () => { it("replaces the marker region with the chosen content", () => { const result = spliceConflict(file, entry, "resolved\n"); - expect(result).toBe("before\nresolved\nafter\n"); + expect(result.text).toBe("before\nresolved\nafter\n"); }); it("accepts multi-line replacement", () => { const result = spliceConflict(file, entry, "alpha\nbeta\n"); - expect(result).toBe("before\nalpha\nbeta\nafter\n"); + expect(result.text).toBe("before\nalpha\nbeta\nafter\n"); }); it("accepts empty replacement", () => { const result = spliceConflict(file, entry, ""); - expect(result).toBe("before\n\nafter\n"); + expect(result.text).toBe("before\n\nafter\n"); }); it("relocates the block when earlier lines have been added (line numbers shift)", () => { const shifted = ["// new comment 1", "// new comment 2", ...file.split("\n")].join("\n"); const result = spliceConflict(shifted, entry, "resolved\n"); - expect(result).toBe("// new comment 1\n// new comment 2\nbefore\nresolved\nafter\n"); + expect(result.text).toBe("// new comment 1\n// new comment 2\nbefore\nresolved\nafter\n"); }); it("rejects when the recorded marker block has been edited away", () => { @@ -306,13 +306,167 @@ describe("spliceConflict", () => { "\r\n", ); const result = spliceConflict(crlfFile, entry, "alpha\nbeta\n"); - expect(result).toBe("before\r\nalpha\r\nbeta\r\nafter\r\n"); + expect(result.text).toBe("before\r\nalpha\r\nbeta\r\nafter\r\n"); }); it("does not append \\r when the spliced region ends the file without a trailing newline", () => { const crlfNoEof = ["before", "<<<<<<< HEAD", "ours", "=======", "theirs", ">>>>>>> feat"].join("\r\n"); const result = spliceConflict(crlfNoEof, entry, "resolved"); - expect(result).toBe("before\r\nresolved"); + expect(result.text).toBe("before\r\nresolved"); + }); +}); + +describe("spliceConflict boundary-echo repair", () => { + // The 08-multi-file-rename shape: the two lines after the closer are the + // function tail models love to re-emit when they paste the "whole + // resolved function" as the replacement. + const fnLines = [ + "const queue = [];", + "<<<<<<< HEAD", + "export function scheduleTask(task, priority = 0) {", + "\tif (dupe(task)) {", + "\t\treturn;", + "\t}", + "=======", + "export function enqueueTask(task) {", + "\tif (queued.has(task.id)) {", + "\t\treturn;", + "\t}", + ">>>>>>> feature", + "\tqueue.push(task);", + "}", + "", + ]; + const fnEntry = makeEntry({ + startLine: 2, + separatorLine: 7, + endLine: 12, + oursLabel: "HEAD", + theirsLabel: "feature", + oursLines: fnLines.slice(2, 6), + theirsLines: fnLines.slice(7, 11), + }); + + it("drops a multi-line trailing echo of the context below the region", () => { + const replacement = [ + "export function scheduleTask(task, priority = 0) {", + "\tif (queued.has(task.id)) {", + "\t\treturn;", + "\t}", + "\tqueue.push(task);", + "}", + ].join("\n"); + const result = spliceConflict(fnLines.join("\n"), fnEntry, replacement); + expect(result.trimmedTrailing).toBe(2); + expect(result.trimmedLeading).toBe(0); + expect(result.text).toBe( + [ + "const queue = [];", + "export function scheduleTask(task, priority = 0) {", + "\tif (queued.has(task.id)) {", + "\t\treturn;", + "\t}", + "\tqueue.push(task);", + "}", + "", + ].join("\n"), + ); + }); + + // The 02-rename-vs-limits shape: a lone `}` echoed after a body-only region. + const bodyLines = [ + "function nextDelay(a) {", + "<<<<<<< HEAD", + "\tconst delay = BASE * 2 ** a;", + "\treturn Math.min(delay, 10_000);", + "=======", + "\tconst d = B * 2 ** a;", + "\treturn Math.min(d, 30_000);", + ">>>>>>> tune", + "}", + "", + ]; + const bodyEntry = makeEntry({ + startLine: 2, + separatorLine: 5, + endLine: 8, + oursLabel: "HEAD", + theirsLabel: "tune", + oursLines: bodyLines.slice(2, 4), + theirsLines: bodyLines.slice(5, 7), + }); + + it("drops a single-line echo when it fixes the region's delimiter balance", () => { + const replacement = ["\tconst delay = BASE * 2 ** a;", "\treturn Math.min(delay, 30_000);", "}"].join("\n"); + const result = spliceConflict(bodyLines.join("\n"), bodyEntry, replacement); + expect(result.trimmedTrailing).toBe(1); + expect(result.text).toBe( + [ + "function nextDelay(a) {", + "\tconst delay = BASE * 2 ** a;", + "\treturn Math.min(delay, 30_000);", + "}", + "", + ].join("\n"), + ); + }); + + it("keeps a single-line echo when the delimiter balance is already consistent", () => { + const file = ["start", "<<<<<<< HEAD", "a", "=======", "b", ">>>>>>> x", "done();", ""].join("\n"); + const entry = makeEntry({ + startLine: 2, + separatorLine: 4, + endLine: 6, + oursLabel: "HEAD", + theirsLabel: "x", + oursLines: ["a"], + theirsLines: ["b"], + }); + const result = spliceConflict(file, entry, "merged\ndone();"); + expect(result.trimmedTrailing).toBe(0); + expect(result.text).toBe("start\nmerged\ndone();\ndone();\n"); + }); + + it("drops a multi-line leading echo of the context above the region", () => { + const file = [ + "// header", + "const queue = [];", + "<<<<<<< HEAD", + "a", + "=======", + "b", + ">>>>>>> x", + "tail", + "", + ].join("\n"); + const entry = makeEntry({ + startLine: 3, + separatorLine: 5, + endLine: 7, + oursLabel: "HEAD", + theirsLabel: "x", + oursLines: ["a"], + theirsLines: ["b"], + }); + const result = spliceConflict(file, entry, "// header\nconst queue = [];\nmerged"); + expect(result.trimmedLeading).toBe(2); + expect(result.text).toBe("// header\nconst queue = [];\nmerged\ntail\n"); + }); + + it("repairs echoes in CRLF files without breaking EOL round-trip", () => { + const crlf = bodyLines.join("\r\n"); + const replacement = ["\tconst delay = BASE * 2 ** a;", "\treturn Math.min(delay, 30_000);", "}"].join("\n"); + const result = spliceConflict(crlf, bodyEntry, replacement); + expect(result.trimmedTrailing).toBe(1); + expect(result.text).toBe( + [ + "function nextDelay(a) {", + "\tconst delay = BASE * 2 ** a;", + "\treturn Math.min(delay, 30_000);", + "}", + "", + ].join("\r\n"), + ); }); }); diff --git a/packages/coding-agent/test/tools/conflict-integration.test.ts b/packages/coding-agent/test/tools/conflict-integration.test.ts index a47a90ff3..44fd51c8f 100644 --- a/packages/coding-agent/test/tools/conflict-integration.test.ts +++ b/packages/coding-agent/test/tools/conflict-integration.test.ts @@ -332,6 +332,40 @@ describe("write resolves conflicts via conflict://N", () => { expect(session.conflictHistory?.get(1)).toBeUndefined(); }); + it("drops trailing lines that echo the context below the region and notes the repair", async () => { + const filePath = path.join(tempDir, "echo.ts"); + const content = [ + "function f() {", + "<<<<<<< HEAD", + "\tours();", + "=======", + "\ttheirs();", + ">>>>>>> feature/x", + "\tdone();", + "}", + "", + ].join("\n"); + await Bun.write(filePath, content); + const session = createTestSession(tempDir); + const read = await getTool(session, "read"); + const write = await getTool(session, "write"); + + await read.execute("read-echo", { path: "echo.ts" }); + // The classic failure: the model pastes the whole resolved function, + // including the two lines that live below the marker block. + const result = await write.execute("write-echo", { + path: "conflict://1", + content: "\tours();\n\ttheirs();\n\tdone();\n}\n", + }); + + const text = getText(result); + expect(text).toContain("Resolved conflict #1"); + expect(text).toContain("dropped 2 content line(s)"); + expect(await Bun.file(filePath).text()).toBe( + ["function f() {", "\tours();", "\ttheirs();", "\tdone();", "}", ""].join("\n"), + ); + }); + it("auto-recovers a `:conflict://N` path and resolves the conflict", async () => { const filePath = path.join(tempDir, "prefix.ts"); await Bun.write(filePath, TWO_WAY);