From 2efd667916faed29644ac54a3966ee1db791c072 Mon Sep 17 00:00:00 2001 From: can1357 Date: Mon, 8 Jun 2026 13:34:41 +0200 Subject: [PATCH] refactor(coding-agent): removed mid-skip placeholder from diff output - Removed the mid-skip placeholder line that was output when skipping lines in the middle of a diff, relying instead on the line number jump to convey the gap. Removed the corresponding placeholder-filtering logic from the hashline diff preview that was handling these placeholders. --- packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/src/edit/diff.ts | 4 ++- .../coding-agent/test/core/hashline.test.ts | 21 ++---------- .../coding-agent/test/tools/edit-diff.test.ts | 5 ++- packages/hashline/CHANGELOG.md | 1 - packages/hashline/src/diff-preview.ts | 6 ---- packages/hashline/test/core-contracts.test.ts | 32 +++++++++++++------ packages/hashline/test/diff-preview.test.ts | 9 ------ 8 files changed, 33 insertions(+), 46 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index d605c963a..eddb0ce52 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -18,6 +18,7 @@ - Fixed follow-up handling to clear consumed clipboard image state after submission so pasted images are not silently carried into later messages - Fixed clipboard-pasted images being rejected when steering or following up during compaction. Instead of bailing with "Retry after it completes to send images", the message and its images are now queued via `queueCompactionMessage` and forwarded to the session (steer/follow-up/prompt) when the compaction queue flushes. - Fixed edit tool result previews to show only current-file lines and collapse long inserted blocks instead of echoing removed content. +- Fixed `generateDiffString` to omit the mid-skip `...` placeholder between two nearby edits, conveying the elided gap via the jump in line numbers instead (consistent with how leading/trailing context skips already render). The placeholder row was indistinguishable from a genuine `...` context line and wasted a row in compact previews. ## [15.10.4] - 2026-06-08 diff --git a/packages/coding-agent/src/edit/diff.ts b/packages/coding-agent/src/edit/diff.ts index 143b73795..de38be329 100644 --- a/packages/coding-agent/src/edit/diff.ts +++ b/packages/coding-agent/src/edit/diff.ts @@ -133,8 +133,10 @@ export function generateDiffString(oldContent: string, newContent: string, conte newLineNum++; } + // Mid-skip placeholder is omitted too: the jump between the trailing + // number of the leading context and the leading number of the + // trailing context conveys the gap, just like leading/trailing skips. if (middleSkip > 0) { - output.push(formatNumberedDiffLine(" ", oldLineNum, "...")); oldLineNum += middleSkip; newLineNum += middleSkip; for (const line of linesToShow.slice(firstChunkLength)) { diff --git a/packages/coding-agent/test/core/hashline.test.ts b/packages/coding-agent/test/core/hashline.test.ts index eff611f4e..24c3036d7 100644 --- a/packages/coding-agent/test/core/hashline.test.ts +++ b/packages/coding-agent/test/core/hashline.test.ts @@ -3,8 +3,8 @@ import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; import { + type InMemorySnapshotStore as FileReadCache, formatHashlineHeader, - InMemorySnapshotStore as FileReadCache, MismatchError as HashlineMismatchError, } from "@oh-my-pi/hashline"; import { resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; @@ -15,9 +15,8 @@ import { getFileSnapshotStore as getFileReadCache, hashlineEditParamsSchema, } from "@oh-my-pi/pi-coding-agent/edit"; -import * as z from "zod/v4"; - import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; +import * as z from "zod/v4"; beforeAll(async () => { resetSettingsForTest(); @@ -49,7 +48,6 @@ function sameLineRange(anchor: string): string { return `replace ${anchor}..${anchor}:`; } - async function withTempDir(fn: (tempDir: string) => Promise): Promise { const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "hashline-edit-")); try { @@ -84,12 +82,6 @@ function hashlineExecuteOptions( }; } - - - - - - describe("hashline executor", () => { it("rejects file creation and directs to the write tool", async () => { await withTempDir(async tempDir => { @@ -116,7 +108,6 @@ describe("hashline executor", () => { }); }); - it("emits an actionable no-op diagnostic when the payload matches the file byte-for-byte", async () => { await withTempDir(async tempDir => { const filePath = path.join(tempDir, "a.ts"); @@ -274,7 +265,6 @@ describe("hashlineEditParamsSchema — payload shape", () => { }); }); - describe("hashline — anchor-stale recovery via read snapshot cache", () => { it("recovers when the file was modified out-of-band after a read", async () => { await withTempDir(async tempDir => { @@ -338,8 +328,6 @@ describe("hashline — anchor-stale recovery via read snapshot cache", () => { }); }); - - it("captures the post-edit result so the next edit can recover from anchors against it", async () => { await withTempDir(async tempDir => { const filePath = path.join(tempDir, "a.ts"); @@ -411,9 +399,4 @@ describe("hashline — anchor-stale recovery via read snapshot cache", () => { expect(await Bun.file(filePath).text()).toBe(`${v1Lines.join("\n")}\n`); }); }); - - }); - - - diff --git a/packages/coding-agent/test/tools/edit-diff.test.ts b/packages/coding-agent/test/tools/edit-diff.test.ts index 8445e37c1..330d6c9eb 100644 --- a/packages/coding-agent/test/tools/edit-diff.test.ts +++ b/packages/coding-agent/test/tools/edit-diff.test.ts @@ -11,7 +11,10 @@ describe("generateDiffString", () => { const result = generateDiffString(oldLines.join("\n"), newLines.join("\n"), 2); const diffLines = result.diff.split("\n"); - expect(diffLines).toContain(" 5|..."); + // The mid-skip emits no placeholder row; the jump from the leading + // context (line 4) to the trailing context (line 16) conveys the gap. + expect(diffLines.some(line => line.endsWith("|...") || line.endsWith("|…"))).toBe(false); + expect(diffLines[diffLines.indexOf(" 4|line 4") + 1]).toBe(" 16|line 16"); expect(diffLines).toContain("-2|line 2"); expect(diffLines).toContain("+2|line 2 changed"); expect(diffLines).toContain("-18|line 18"); diff --git a/packages/hashline/CHANGELOG.md b/packages/hashline/CHANGELOG.md index 401815e80..6a39b08ed 100644 --- a/packages/hashline/CHANGELOG.md +++ b/packages/hashline/CHANGELOG.md @@ -14,7 +14,6 @@ ### Fixed - Fixed compact edit previews to omit deleted content, keep visible lines anchored to the current file, and collapse long inserted runs with a `+N…` elision marker. -- Fixed `buildCompactDiffPreview` to drop context-gap placeholder rows (`...`/`…`) from the model-facing preview, relying on the jump in emitted line numbers to convey the elision. The placeholder was byte-identical to a genuine context line whose text is `...`, and wasted a row; the diff producer still emits the marker for the human-facing TUI diff. ## [15.10.3] - 2026-06-08 diff --git a/packages/hashline/src/diff-preview.ts b/packages/hashline/src/diff-preview.ts index e604c4cf6..be62c8f8b 100644 --- a/packages/hashline/src/diff-preview.ts +++ b/packages/hashline/src/diff-preview.ts @@ -96,12 +96,6 @@ export function buildCompactDiffPreview(diff: string, options: CompactDiffOption break; default: { flushAddedRun(); - // Context-gap placeholders (`...`/`…`) carry no real content and are - // byte-identical to a genuine context line whose text is "...". Drop - // them from the model-facing preview; the jump in emitted line - // numbers conveys the elision (matching how the diff producer already - // omits leading/trailing skips). - if (parsed.content === "..." || parsed.content === "…") break; const newLineNumber = parsed.lineNumber + addedLines - removedLines; formatted.push(` ${newLineNumber}:${parsed.content}`); break; diff --git a/packages/hashline/test/core-contracts.test.ts b/packages/hashline/test/core-contracts.test.ts index 5cefba09b..bbd66d4ba 100644 --- a/packages/hashline/test/core-contracts.test.ts +++ b/packages/hashline/test/core-contracts.test.ts @@ -2,15 +2,15 @@ import { describe, expect, it } from "bun:test"; import { applyEdits, detectLineEnding, + type Edit, formatHashlineHeader, InMemoryFilesystem, InMemorySnapshotStore, Patch, Patcher, + type PatchSection, parsePatch, Recovery, - type Edit, - type PatchSection, type SplitOptions, } from "@oh-my-pi/hashline"; @@ -117,9 +117,9 @@ describe("hashline parser — range-anchor contracts", () => { it("preserves whitespace-bearing and sigil-leading payload exactly", () => { const payload = "\tconst streamKeepaliveMs = opts.streamKeepaliveMs;"; expect(applyDiff(content, `insert after ${tag(2)}:\n${repl(payload)}`)).toBe(`aaa\nbbb\n${payload}\nccc`); - expect(applyDiff(content, `${sameLineRange(tag(2))}\n${repl("|literal")}\n${repl("^literal")}\n${repl("↓literal")}`)).toBe( - "aaa\n|literal\n^literal\n↓literal\nccc", - ); + expect( + applyDiff(content, `${sameLineRange(tag(2))}\n${repl("|literal")}\n${repl("^literal")}\n${repl("↓literal")}`), + ).toBe("aaa\n|literal\n^literal\n↓literal\nccc"); }); it("strips copied read-output prefixes only inside pasted bare body rows", () => { @@ -259,7 +259,9 @@ describe("hashline abort sentinel", () => { const sentinel = "*** Abort"; it("terminates parsing without surfacing a warning", () => { - const diff = [`insert after ${tag(1)}:`, repl("HELLO"), sentinel, `insert after ${tag(99)}:`, repl("never")].join("\n"); + const diff = [`insert after ${tag(1)}:`, repl("HELLO"), sentinel, `insert after ${tag(99)}:`, repl("never")].join( + "\n", + ); const { edits, warnings } = parsePatch(diff); expect(edits).toHaveLength(1); expect(edits[0]).toMatchObject({ kind: "insert", text: "HELLO" }); @@ -267,7 +269,15 @@ describe("hashline abort sentinel", () => { }); it("stops the input splitter before later sections", () => { - const input = ["[a.ts]", `insert after ${tag(1)}:`, repl("a-payload"), sentinel, "[b.ts]", `insert after ${tag(1)}:`, repl("never")].join("\n"); + const input = [ + "[a.ts]", + `insert after ${tag(1)}:`, + repl("a-payload"), + sentinel, + "[b.ts]", + `insert after ${tag(1)}:`, + repl("never"), + ].join("\n"); const sections = splitHashlineInputs(input); expect(sections).toHaveLength(1); expect(sections[0].path).toBe("a.ts"); @@ -278,8 +288,12 @@ describe("hashline abort sentinel", () => { describe("hashline parser — delete and blank payload semantics", () => { it("applies inline delete and empty replace operations", () => { expect(applyDiff("line1\nline2\nline3\n", splitHashlineInput("[a.ts]\ndelete 2\n").diff)).toBe("line1\nline3\n"); - expect(applyDiff("line1\nline2\nline3\nline4\n", splitHashlineInput("[a.ts]\ndelete 2..3\n").diff)).toBe("line1\nline4\n"); - expect(applyDiff("line1\nline2\nline3\n", splitHashlineInput("[a.ts]\nreplace 2..2:\n").diff)).toBe("line1\nline3\n"); + expect(applyDiff("line1\nline2\nline3\nline4\n", splitHashlineInput("[a.ts]\ndelete 2..3\n").diff)).toBe( + "line1\nline4\n", + ); + expect(applyDiff("line1\nline2\nline3\n", splitHashlineInput("[a.ts]\nreplace 2..2:\n").diff)).toBe( + "line1\nline3\n", + ); }); it("treats old inline replacement syntax as orphan body", () => { diff --git a/packages/hashline/test/diff-preview.test.ts b/packages/hashline/test/diff-preview.test.ts index 7c6544914..babe6de24 100644 --- a/packages/hashline/test/diff-preview.test.ts +++ b/packages/hashline/test/diff-preview.test.ts @@ -46,13 +46,4 @@ describe("buildCompactDiffPreview", () => { expect(preview.addedLines).toBe(7); expect(preview.removedLines).toBe(0); }); - - it("drops context-gap placeholders and conveys elision via the line-number jump", () => { - const preview = buildCompactDiffPreview( - [" 2|line 2", " 3|line 3", " 4|...", " 49|line 49", "+50|inserted"].join("\n"), - ); - - expect(preview.preview).toBe([" 2:line 2", " 3:line 3", " 49:line 49", "+50:inserted"].join("\n")); - expect(preview.addedLines).toBe(1); - }); });