From 1c07a54cd4da03ed4a47548861ebf5b26c7f20dd Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Mon, 6 Jul 2026 06:02:46 -0600 Subject: [PATCH] fix: scope diff file-header detection to between-hunk positions parseDiffOutput tested every diff line against the file-header regexes unconditionally, including lines inside an active hunk body. A removed line whose original content starts with "-- " (e.g. a Markdown horizontal rule) becomes a "--- " line once diff-prefixed and was misdetected as a source-file header, silently dropped, and desynced the old-line cursor for the rest of the hunk. Symmetrically, an added line starting with "++ b/" becomes a "+++ b/..." line and was misdetected as a new-file header, flushing the in-progress run under a phantom file key and misattributing every later line in the hunk to it. DiffLineTracker now derives an insideHunk() state from the old/new line-count bounds declared by the most recent hunk header, and parseDiffOutput only attempts file-header matching while that is false: before a file's first hunk, or once the previous hunk's declared counts are fully consumed. Real diffs never have hunk-header counts that lie about their body length, so this makes header detection position-aware without weakening it for any real diff shape. No user-facing behavior, commands, or architecture changed. docs check acknowledged. Fixes #1761 Impact: 4 functions changed, 5 affected --- src/features/check.ts | 116 +++++++++++++++++++++----------- tests/integration/check.test.ts | 76 +++++++++++++++++++++ 2 files changed, 154 insertions(+), 38 deletions(-) diff --git a/src/features/check.ts b/src/features/check.ts index 95a172e1..ca12c29c 100644 --- a/src/features/check.ts +++ b/src/features/check.ts @@ -74,6 +74,9 @@ function isDevNullTargetLine(line: string): boolean { class DiffLineTracker { private oldLineCursor = 0; private newLineCursor = 0; + /** End-exclusive old/new bounds declared by the current hunk's `@@ -a,b +c,d @@` header — see `insideHunk`. */ + private oldHunkEnd = 0; + private newHunkEnd = 0; private removedRunStart: number | null = null; private removedRunEnd: number | null = null; private addedRunStart: number | null = null; @@ -89,19 +92,40 @@ class DiffLineTracker { */ private pendingRemovedText: string[] = []; - startHunk(oldStart: number, newStart: number): void { + startHunk(oldStart: number, oldCount: number, newStart: number, newCount: number): void { this.oldLineCursor = oldStart; this.newLineCursor = newStart; + this.oldHunkEnd = oldStart + oldCount; + this.newHunkEnd = newStart + newCount; this.pendingRemovedText = []; } + /** + * True while either cursor is still short of the bounds declared by the + * most recent hunk header — i.e. while a hunk body is still being walked. + * False before the first hunk header of a file is seen, and again once + * both sides' declared line counts have been fully consumed. + * + * `parseDiffOutput` only attempts `--- `/`+++ b/` file-header matching + * while this is false. A hunk body line whose own content starts with + * `-- `/`++ ` (e.g. removing a Markdown horizontal rule `-- foo`) becomes + * `--- foo`/`+++ foo` once diff-prefixed — indistinguishable from a real + * file header by content alone — so position, not pattern, is what must + * disambiguate it. See issue #1761. + */ + insideHunk(): boolean { + return this.oldLineCursor < this.oldHunkEnd || this.newLineCursor < this.newHunkEnd; + } + /** * Consumes one hunk body line, advancing the old- and new-side cursors as - * needed. Lines beginning with `--- `/`+++ ` (source-file headers) are - * already filtered out by the caller before a line ever reaches here, so a - * leading `-`/`+` unambiguously marks a removed/added line — even one - * whose own content starts with dashes or pluses (e.g. removing a line of - * literal text `-- foo`). + * needed. The caller only attempts `--- `/`+++ ` file-header matching while + * `insideHunk()` is false, so a body line that happens to start with one of + * those prefixes (the diff-prefixed form of a removed/added line whose own + * content starts with dashes or pluses, e.g. literal text `-- foo`) still + * reaches here as ordinary content instead of being misdetected as a + * header. A leading `-`/`+` therefore unambiguously marks a removed/added + * line regardless of what follows it. */ consume( line: string, @@ -197,44 +221,60 @@ export function parseDiffOutput(diffOutput: string): ParsedDiff { const tracker = new DiffLineTracker(); for (const line of diffOutput.split('\n')) { - if (isDevNullSourceLine(line)) { - prevIsDevNull = true; - continue; - } - if (isSourceFileHeaderLine(line)) { - prevIsDevNull = false; - continue; - } - const newFile = extractNewFileName(line); - if (newFile) { - if (currentFile) tracker.flush(currentFile, oldRanges, changedRanges, changedEdits); - currentFile = newFile; - if (!changedRanges.has(currentFile)) changedRanges.set(currentFile, []); - if (!oldRanges.has(currentFile)) oldRanges.set(currentFile, []); - if (!changedEdits.has(currentFile)) changedEdits.set(currentFile, []); - if (prevIsDevNull) newFiles.add(currentFile); - prevIsDevNull = false; - continue; - } - if (isDevNullTargetLine(line)) { - // `+++ /dev/null` (file deletion) is not `b/`-prefixed, so - // extractNewFileName returned null above and this line would otherwise - // fall through to tracker.consume and be misread as an added source - // line under whichever file preceded this one in the diff. Flush and - // clear the file context instead — the deleted file's hunk body that - // follows has no corresponding DB entry to check against anyway (its - // nodes are purged from the graph), so there is nothing to track here. - if (currentFile) tracker.flush(currentFile, oldRanges, changedRanges, changedEdits); - currentFile = null; - prevIsDevNull = false; - continue; + // File-header lines (`--- `/`+++ b/`/`+++ /dev/null`) only ever appear + // between hunks — before the first hunk of a file, or once the previous + // hunk's declared old/new line counts are fully consumed. Gating this + // whole block on `!insideHunk()` keeps a hunk-body line whose own + // content starts with `-- `/`++ ` (e.g. a Markdown horizontal rule) from + // being misdetected as a file header purely because of a text + // coincidence — position, not pattern, decides. See issue #1761. + if (!tracker.insideHunk()) { + if (isDevNullSourceLine(line)) { + prevIsDevNull = true; + continue; + } + if (isSourceFileHeaderLine(line)) { + prevIsDevNull = false; + continue; + } + const newFile = extractNewFileName(line); + if (newFile) { + if (currentFile) tracker.flush(currentFile, oldRanges, changedRanges, changedEdits); + currentFile = newFile; + if (!changedRanges.has(currentFile)) changedRanges.set(currentFile, []); + if (!oldRanges.has(currentFile)) oldRanges.set(currentFile, []); + if (!changedEdits.has(currentFile)) changedEdits.set(currentFile, []); + if (prevIsDevNull) newFiles.add(currentFile); + prevIsDevNull = false; + continue; + } + if (isDevNullTargetLine(line)) { + // `+++ /dev/null` (file deletion) is not `b/`-prefixed, so + // extractNewFileName returned null above and this line would otherwise + // fall through to tracker.consume and be misread as an added source + // line under whichever file preceded this one in the diff. Flush and + // clear the file context instead — the deleted file's hunk body that + // follows has no corresponding DB entry to check against anyway (its + // nodes are purged from the graph), so there is nothing to track here. + if (currentFile) tracker.flush(currentFile, oldRanges, changedRanges, changedEdits); + currentFile = null; + prevIsDevNull = false; + continue; + } } if (!currentFile) continue; const hunkMatch = line.match(HUNK_RE); if (hunkMatch) { tracker.flush(currentFile, oldRanges, changedRanges, changedEdits); - tracker.startHunk(parseInt(hunkMatch[1]!, 10), parseInt(hunkMatch[3]!, 10)); + const oldCount = hunkMatch[2] === undefined ? 1 : parseInt(hunkMatch[2], 10); + const newCount = hunkMatch[4] === undefined ? 1 : parseInt(hunkMatch[4], 10); + tracker.startHunk( + parseInt(hunkMatch[1]!, 10), + oldCount, + parseInt(hunkMatch[3]!, 10), + newCount, + ); continue; } diff --git a/tests/integration/check.test.ts b/tests/integration/check.test.ts index ccb750d3..5b9bd906 100644 --- a/tests/integration/check.test.ts +++ b/tests/integration/check.test.ts @@ -211,13 +211,26 @@ describe('parseDiffOutput', () => { }); test('handles multiple files', () => { + // Each hunk header's declared old/new line counts must be backed by a + // matching number of actual body lines — parseDiffOutput is + // position-aware (issue #1761) and treats a hunk as still "open" until + // its declared counts are consumed, so a header with no body would + // swallow the next file's `--- `/`+++ ` lines as hunk content instead of + // recognizing them. const diff = [ '--- a/src/a.js', '+++ b/src/a.js', '@@ -5,2 +5,3 @@', + ' context a', + '-old a', + '+new a', + '+extra a', '--- a/src/b.js', '+++ b/src/b.js', '@@ -10,1 +10,2 @@', + '-old b', + '+new b', + '+extra b', ].join('\n'); const { changedRanges } = parseDiffOutput(diff); @@ -295,6 +308,69 @@ describe('parseDiffOutput', () => { { start: 1, end: 2, addedText: ['line B', 'line B2'], removedText: ['line A', 'line A2'] }, ]); }); + + // ─── hunk-scoped file-header detection (issue #1761) ─────────────────── + + test('a removed line whose content starts with "-- " is treated as hunk content, not a file header', () => { + // A removed line whose original text is `-- horizontal rule` becomes the + // diff line `--- horizontal rule` once prefixed with the single-`-` + // removal marker — three dashes plus a space, indistinguishable from a + // `--- a/file` source header by content alone (confirmed against real + // `git diff` output on a fixture with this exact content). Without + // position-aware parsing this line is misdetected as a file header and + // silently dropped, desyncing the old-line cursor for the rest of the + // hunk. + const diff = [ + '--- a/src/oldfile.md', + '+++ b/src/oldfile.md', + '@@ -10,4 +10,2 @@', + '-line A', + '--- horizontal rule', + ' context unchanged', + '-line D', + '+replacement', + ].join('\n'); + + const { oldRanges, changedRanges } = parseDiffOutput(diff); + // Both removed lines (old lines 10-11) form one contiguous run, and the + // trailing removed line correctly lands at old line 13 — after the + // untouched context line at 12 — rather than shifted down by one. + expect(oldRanges.get('src/oldfile.md')).toEqual([ + { start: 10, end: 11 }, + { start: 13, end: 13 }, + ]); + expect(changedRanges.get('src/oldfile.md')).toEqual([{ start: 11, end: 11 }]); + }); + + test('an added line whose content starts with "++ b/" is treated as hunk content, not a new-file header', () => { + // Symmetric case: an added line whose original text is + // `++ b/some-file-path` becomes the diff line `+++ b/some-file-path` + // once prefixed with the single-`+` addition marker — indistinguishable + // from a `+++ b/` new-file header by content alone (confirmed + // against real `git diff` output on a fixture with this exact content). + // Without position-aware parsing this line is misdetected as the start + // of a new file section, flushing the in-progress run under a phantom + // file key derived from the line's own content and misattributing every + // line that follows it in the hunk. + const diff = [ + '--- a/src/real-file.md', + '+++ b/src/real-file.md', + '@@ -1,2 +1,4 @@', + ' context unchanged', + '-old line', + '+line A', + '+++ b/some-file-path', + '+line D', + ].join('\n'); + + const { changedRanges, oldRanges } = parseDiffOutput(diff); + // All three added lines (new lines 2-4) form one contiguous run under + // the real file, and no phantom "some-file-path" entry is created. + expect(changedRanges.get('src/real-file.md')).toEqual([{ start: 2, end: 4 }]); + expect(oldRanges.get('src/real-file.md')).toEqual([{ start: 2, end: 2 }]); + expect(changedRanges.has('some-file-path')).toBe(false); + expect(oldRanges.has('some-file-path')).toBe(false); + }); }); // ─── checkNoNewCycles ─────────────────────────────────────────────────