From a4769439ddf63d95f171c49ba4832a22cf59c1c0 Mon Sep 17 00:00:00 2001 From: auyua9 Date: Fri, 11 Sep 2026 01:27:00 +0800 Subject: [PATCH] fix(parse): retain metadata-only diff files (#3251) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(parse): retain metadata-only diff files * Address PR review feedback (#3251) Keep detect_changes honest on C-quoted, TAB-terminated, and ambiguous diff --git dests, and fail closed when a header cannot be parsed. Co-authored-by: Cursor * Simplify parseDiffHunks dest-prefix helper (#3251) Drop the unused a/ prefix arm and the redundant empty-parse unparsed-header check. Co-authored-by: Cursor * fix(parse): ignore +++ inside hunks and accept mixed-quoted git headers Stop treating hunk-body lines as file headers, and parse independently C-quoted src/dest tokens on diff --git. Co-authored-by: Cursor --------- Co-authored-by: ming Co-authored-by: Gergő Magyar Co-authored-by: Gergo Magyar Co-authored-by: Cursor --- gitnexus/src/mcp/local/local-backend.ts | 19 +- gitnexus/src/storage/git.ts | 226 +++++++++++++++++- gitnexus/test/unit/detect-changes-eol.test.ts | 4 +- .../unit/detect-changes-hunk-scale.test.ts | 5 +- gitnexus/test/unit/parse-diff-hunks.test.ts | 165 ++++++++++++- 5 files changed, 405 insertions(+), 14 deletions(-) diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index 8b34a3716..bcb2cf1b4 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -36,13 +36,12 @@ import { isWalCorruptionError, WAL_RECOVERY_SUGGESTION } from '../../core/lbug/l // git utilities available if needed // import { isGitRepo, getCurrentCommit, getGitRoot } from '../../storage/git.js'; import { - parseDiffHunks, + parseDiffHunksResult, coalesceHunksByPath, hunksOverlapRange, findGitRootByDotGit, getCanonicalRepoRoot, getGitRoot, - type FileDiff, } from '../../storage/git.js'; import { realpathSync } from 'fs'; import { @@ -1178,6 +1177,12 @@ export function buildDetectChangesDiffArgs(scope: string, baseRef?: string): str // not `--default-prefix`, which needs git >= 2.42. `--no-ext-diff` stops a // configured external diff driver from replacing the unified output we parse. const args = [ + // Before the subcommand: default core.quotePath C-quotes non-ASCII so + // `diff --git` / `+++` tokens no longer match the unquoted `a/` `b/` forms + // the parser also accepts. The parser still decodes quoted tokens; this + // pin keeps production git from emitting them. + '-c', + 'core.quotePath=false', 'diff', '--ignore-cr-at-eol', '--no-ext-diff', @@ -5606,11 +5611,11 @@ export class LocalBackend { return { error: `Git diff failed: ${err.message}` }; } - const fileDiffs: FileDiff[] = parseDiffHunks(diffOutput); + const { files: fileDiffs, unparsedGitHeaders } = parseDiffHunksResult(diffOutput); if (fileDiffs.length === 0) { - // Git printed a diff but none of it parsed: the `+++ b/` headers were not - // where `parseDiffHunks` looks. That is a PARSE failure, not a clean tree, + // Git printed a diff but none of it parsed: no `diff --git` / `+++` + // file header was recognised. That is a PARSE failure, not a clean tree, // and the clean branch below would report it to the pre-commit gate as // `risk_level:'none'`, no `partial`, exit 0 — a false all-clear (#2915). const parseFailed = diffOutput.trim().length > 0; @@ -5643,7 +5648,9 @@ export class LocalBackend { const changedSymbols = new Map(); // Set if a swallowed graph query fails below — surfaces `partial:true` so a // degraded run cannot report a false-clean `risk_level:'low'` (#2283). - let queryDegraded = false; + // An unparsed `diff --git` is the same class: later hunks must not make + // the gate look complete. + let queryDegraded = unparsedGitHeaders > 0; // Hunks arrive grouped per path and already in the graph's 0-based line // space, so every comparison below is base-neutral (#2377). diff --git a/gitnexus/src/storage/git.ts b/gitnexus/src/storage/git.ts index 792c1010b..1e06b0477 100644 --- a/gitnexus/src/storage/git.ts +++ b/gitnexus/src/storage/git.ts @@ -750,10 +750,189 @@ export interface FileDiff { hunks: DiffHunk[]; } +/** `parseDiffHunks` plus how many `diff --git` headers could not be decoded. */ +export interface DiffHunkParseResult { + files: FileDiff[]; + unparsedGitHeaders: number; +} + +const DIFF_GIT_PREFIX = 'diff --git '; + +/** + * Decode one Git C-quoted token (`"a/\\344\\270\\255.png"`). Returns + * `undefined` when the quotes are unbalanced or a trailing escape is bare. + */ +function unquoteCStyleGitToken(quoted: string): string | undefined { + if (quoted.length < 2 || quoted[0] !== '"' || quoted[quoted.length - 1] !== '"') { + return undefined; + } + // Git C-quotes are byte-oriented: non-ASCII is `\nnn` octal of the UTF-8 + // code units, not JS UTF-16 characters. + const bytes: number[] = []; + const pushChar = (ch: string): void => { + const code = ch.charCodeAt(0); + if (code < 0x80) bytes.push(code); + else bytes.push(...new TextEncoder().encode(ch)); + }; + for (let i = 1; i < quoted.length - 1; i++) { + const ch = quoted[i]; + if (ch !== '\\') { + pushChar(ch); + continue; + } + const next = quoted[++i]; + if (next === undefined) return undefined; + switch (next) { + case '\\': + case '"': + pushChar(next); + break; + case 'n': + bytes.push(0x0a); + break; + case 't': + bytes.push(0x09); + break; + case 'r': + bytes.push(0x0d); + break; + case 'a': + bytes.push(0x07); + break; + case 'b': + bytes.push(0x08); + break; + case 'v': + bytes.push(0x0b); + break; + case 'f': + bytes.push(0x0c); + break; + default: { + if (next < '0' || next > '7') { + pushChar(next); + break; + } + let oct = next; + while (oct.length < 3 && i + 1 < quoted.length - 1) { + const digit = quoted[i + 1]; + if (digit < '0' || digit > '7') break; + oct += digit; + i++; + } + bytes.push(parseInt(oct, 8)); + } + } + } + return new TextDecoder('utf-8').decode(Uint8Array.from(bytes)); +} + +function takeCQuotedToken( + source: string, + start: number, +): { token: string; end: number } | undefined { + if (source[start] !== '"') return undefined; + for (let i = start + 1; i < source.length; i++) { + if (source[i] === '\\') { + i++; + continue; + } + if (source[i] === '"') return { token: source.slice(start, i + 1), end: i + 1 }; + } + return undefined; +} + +function stripGitDstPrefix(raw: string): string | undefined { + return raw.startsWith('b/') ? raw.slice(2) : undefined; +} + +/** Unified-diff paths end at the first TAB (timestamp / empty terminator). */ +function stripUnifiedDiffTab(pathWithOptionalTab: string): string { + const tab = pathWithOptionalTab.indexOf('\t'); + return tab === -1 ? pathWithOptionalTab : pathWithOptionalTab.slice(0, tab); +} + +function decodeGitPathToken(raw: string): string | undefined { + const trimmed = stripUnifiedDiffTab(raw); + if (trimmed.startsWith('"')) return unquoteCStyleGitToken(trimmed); + return trimmed; +} + +/** + * Destination path from `diff --git a/ b/`. + * + * Same-path headers recover `name` from `a/${name} b/${name}` so a dest that + * itself contains ` b/` is not split at the last occurrence. C-quoted tokens + * (default `core.quotePath`) are decoded. Renames that the greedy split would + * mis-parse stay a best-effort dest; `rename to` / `+++` correct them. + */ +function takeGitHeaderPathToken( + source: string, + start: number, +): { path: string; end: number } | undefined { + if (start >= source.length) return undefined; + if (source[start] === '"') { + const tok = takeCQuotedToken(source, start); + if (!tok) return undefined; + const path = unquoteCStyleGitToken(tok.token); + if (path === undefined) return undefined; + return { path, end: tok.end }; + } + if (source.startsWith('b/', start)) { + return { path: stripUnifiedDiffTab(source.slice(start)), end: source.length }; + } + if (source.startsWith('a/', start)) { + for (let i = start + 2; i < source.length; i++) { + if (source.startsWith(' b/', i) || source.startsWith(' "', i)) { + return { path: source.slice(start, i), end: i }; + } + } + } + return undefined; +} + +function filePathFromGitHeader(line: string): string | undefined { + if (!line.startsWith(DIFF_GIT_PREFIX)) return undefined; + const rest = line.slice(DIFF_GIT_PREFIX.length); + + if (rest.startsWith('a/')) { + for (let i = 2; i < rest.length; i++) { + if (!rest.startsWith(' b/', i)) continue; + const nameA = rest.slice(2, i); + const nameB = rest.slice(i + 3); + if (nameA.length > 0 && nameA === nameB) return nameA; + } + } + + const src = takeGitHeaderPathToken(rest, 0); + if (!src) return undefined; + let i = src.end; + while (rest[i] === ' ') i++; + const dest = takeGitHeaderPathToken(rest, i); + return dest ? stripGitDstPrefix(dest.path) : undefined; +} + +function pathFromPlusPlusPlus(line: string): string | undefined { + if (!line.startsWith('+++ ')) return undefined; + const raw = decodeGitPathToken(line.slice(4)); + if (!raw || raw === '/dev/null') return undefined; + return stripGitDstPrefix(raw); +} + +function pathFromRenameTo(line: string): string | undefined { + if (!line.startsWith('rename to ')) return undefined; + return decodeGitPathToken(line.slice('rename to '.length)); +} + /** * Parse unified diff output (with -U0) into per-file hunk ranges. * Extracts the new-file line ranges from @@ hunk headers. * + * The `diff --git` header is also retained as a file entry. This matters for + * binary, rename-only, and mode-only changes, which have no `+++ b/` header. + * Such entries intentionally have no hunks: callers can count the changed + * path without pretending that a symbol line range was touched. + * * A pure deletion adds no new lines, and unified diff spells that empty range * as the line BEFORE it: `@@ -4,2 +3,0 @@` removed old lines 4–5 from between * new lines 3 and 4 (git emits `+0,0` when the deletion is at the head of the @@ -769,13 +948,52 @@ export interface FileDiff { * gap — the widening {@link coalesceHunks} is careful never to do. */ export function parseDiffHunks(diffOutput: string): FileDiff[] { + return parseDiffHunksResult(diffOutput).files; +} + +/** + * Same as {@link parseDiffHunks}, plus a count of `diff --git` lines that + * could not be decoded. `detect_changes` uses the count to fail closed + * (`partial` + `risk_level:'unknown'`) instead of attaching later hunks to a + * previous file. + */ +export function parseDiffHunksResult(diffOutput: string): DiffHunkParseResult { const files: FileDiff[] = []; let current: FileDiff | null = null; + let unparsedGitHeaders = 0; + // `+++` after the first `@@` of a file is hunk body (`+` plus source text + // that itself starts `++ …`), not another file header. + let inHunk = false; for (const line of diffOutput.split('\n')) { - if (line.startsWith('+++ b/')) { - current = { filePath: line.slice(6), hunks: [] }; - files.push(current); + if (line.startsWith(DIFF_GIT_PREFIX)) { + // Drop the previous file first: an unparsed header must not leave + // `current` live for a later `@@` / quoted `+++` to steal. + current = null; + inHunk = false; + const filePath = filePathFromGitHeader(line); + if (filePath) { + current = { filePath, hunks: [] }; + files.push(current); + } else { + unparsedGitHeaders++; + } + } else if (line.startsWith('rename to ')) { + const filePath = pathFromRenameTo(line); + if (!filePath) continue; + if (current) current.filePath = filePath; + else { + current = { filePath, hunks: [] }; + files.push(current); + } + } else if (!inHunk && line.startsWith('+++ ')) { + const filePath = pathFromPlusPlusPlus(line); + if (!filePath) continue; + if (!current || current.filePath !== filePath) { + current = { filePath, hunks: [] }; + files.push(current); + } } else if (line.startsWith('@@') && current) { + inHunk = true; const match = line.match(/@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/); if (match) { const start = parseInt(match[1], 10); @@ -791,7 +1009,7 @@ export function parseDiffHunks(diffOutput: string): FileDiff[] { } } } - return files; + return { files, unparsedGitHeaders }; } /** diff --git a/gitnexus/test/unit/detect-changes-eol.test.ts b/gitnexus/test/unit/detect-changes-eol.test.ts index bb3da5328..89384a049 100644 --- a/gitnexus/test/unit/detect-changes-eol.test.ts +++ b/gitnexus/test/unit/detect-changes-eol.test.ts @@ -8,8 +8,10 @@ import { parseDiffHunks } from '../../src/storage/git.js'; import { diffArgsFor } from '../helpers/detect-changes-diff-args.js'; import { commitAll, initGitRepo } from '../helpers/temp-git-repo.js'; -/** The six flags every scope carries, ahead of its own ref/staging arguments. */ +/** Flags every scope carries, ahead of its own ref/staging arguments. */ const GUARD_FLAGS = [ + '-c', + 'core.quotePath=false', 'diff', '--ignore-cr-at-eol', '--no-ext-diff', diff --git a/gitnexus/test/unit/detect-changes-hunk-scale.test.ts b/gitnexus/test/unit/detect-changes-hunk-scale.test.ts index 77e72c681..a9e1035fc 100644 --- a/gitnexus/test/unit/detect-changes-hunk-scale.test.ts +++ b/gitnexus/test/unit/detect-changes-hunk-scale.test.ts @@ -361,11 +361,12 @@ describe('#2915 detect_changes hunk scaling', () => { ); registerRepo(repoDir); - // Non-vacuous: the diff really does parse to two entries for one path. + // A content line that is itself `+++ b/code.py` must not open a second + // FileDiff — the same-path skip is the intended shape (#3251). const parsed = parseDiffHunks( execFileSync('git', diffArgsFor('unstaged'), { cwd: repoDir, encoding: 'utf-8' }), ); - expect(parsed.map((fileDiff) => fileDiff.filePath)).toEqual(['code.py', 'code.py']); + expect(parsed.map((fileDiff) => fileDiff.filePath)).toEqual(['code.py']); const result = await runDetectChanges(); diff --git a/gitnexus/test/unit/parse-diff-hunks.test.ts b/gitnexus/test/unit/parse-diff-hunks.test.ts index e354eca66..363d6b49f 100644 --- a/gitnexus/test/unit/parse-diff-hunks.test.ts +++ b/gitnexus/test/unit/parse-diff-hunks.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest'; -import { parseDiffHunks } from '../../src/storage/git.js'; +import { parseDiffHunks, parseDiffHunksResult } from '../../src/storage/git.js'; describe('parseDiffHunks', () => { it('parses a single file with one hunk', () => { @@ -133,4 +133,167 @@ describe('parseDiffHunks', () => { expect(result[1].hunks[0]).toEqual({ startLine: 51, endLine: 51 }); expect(result[1].hunks[1]).toEqual({ startLine: 82, endLine: 84 }); }); + + it('retains binary-only files from the git header', () => { + const diff = [ + 'diff --git a/assets/logo.png b/assets/logo.png', + 'index 1111111..2222222 100644', + 'Binary files a/assets/logo.png and b/assets/logo.png differ', + ].join('\n'); + expect(parseDiffHunks(diff)).toEqual([{ filePath: 'assets/logo.png', hunks: [] }]); + }); + + it('retains the destination of a rename-only diff', () => { + const diff = [ + 'diff --git a/src/old.ts b/src/new.ts', + 'similarity index 100%', + 'rename from src/old.ts', + 'rename to src/new.ts', + ].join('\n'); + expect(parseDiffHunks(diff)).toEqual([{ filePath: 'src/new.ts', hunks: [] }]); + }); + + it('keeps line ranges for whitespace-only hunks', () => { + const diff = [ + 'diff --git a/src/format.ts b/src/format.ts', + '--- a/src/format.ts', + '+++ b/src/format.ts', + '@@ -4,2 +4,2 @@ function format() {', + '- return value;', + '+return value;', + ].join('\n'); + expect(parseDiffHunks(diff)).toEqual([ + { filePath: 'src/format.ts', hunks: [{ startLine: 4, endLine: 5 }] }, + ]); + }); + + it('retains a mode-only change from the git header', () => { + const diff = ['diff --git a/script.sh b/script.sh', 'old mode 100644', 'new mode 100755'].join( + '\n', + ); + expect(parseDiffHunks(diff)).toEqual([{ filePath: 'script.sh', hunks: [] }]); + }); + + it('decodes a C-quoted metadata-only header', () => { + const diff = [ + 'diff --git "a/assets/\\344\\270\\255.png" "b/assets/\\344\\270\\255.png"', + 'Binary files differ', + ].join('\n'); + expect(parseDiffHunks(diff)).toEqual([{ filePath: 'assets/中.png', hunks: [] }]); + }); + + it('does not attach a later C-quoted hunk to a previous metadata-only file', () => { + const diff = [ + 'diff --git a/script.sh b/script.sh', + 'old mode 100644', + 'new mode 100755', + 'diff --git "a/src/\\344\\275\\240\\345\\245\\275.ts" "b/src/\\344\\275\\240\\345\\245\\275.ts"', + '--- "a/src/\\344\\275\\240\\345\\245\\275.ts"', + '+++ "b/src/\\344\\275\\240\\345\\245\\275.ts"', + '@@ -1,0 +1,1 @@', + '+export const ok = 1;', + ].join('\n'); + expect(parseDiffHunks(diff)).toEqual([ + { filePath: 'script.sh', hunks: [] }, + { filePath: 'src/你好.ts', hunks: [{ startLine: 1, endLine: 1 }] }, + ]); + }); + + it('strips the unified-diff TAB on +++ so a spaced path is one FileDiff', () => { + const diff = [ + 'diff --git a/My Documents/file.ts b/My Documents/file.ts', + '--- a/My Documents/file.ts', + '+++ b/My Documents/file.ts\t', + '@@ -1,0 +1,1 @@', + '+x', + ].join('\n'); + expect(parseDiffHunks(diff)).toEqual([ + { filePath: 'My Documents/file.ts', hunks: [{ startLine: 1, endLine: 1 }] }, + ]); + }); + + it('recovers a same-path dest that itself contains " b/"', () => { + const diff = [ + 'diff --git a/foo b/bar.png b/foo b/bar.png', + 'Binary files a/foo b/bar.png and b/foo b/bar.png differ', + ].join('\n'); + expect(parseDiffHunks(diff)).toEqual([{ filePath: 'foo b/bar.png', hunks: [] }]); + }); + + it('prefers rename to over a greedy b/ split', () => { + const diff = [ + 'diff --git a/plain.ts b/foo b/plain.ts', + 'similarity index 100%', + 'rename from plain.ts', + 'rename to foo b/plain.ts', + ].join('\n'); + expect(parseDiffHunks(diff)).toEqual([{ filePath: 'foo b/plain.ts', hunks: [] }]); + }); + + it('keeps one FileDiff when a content line repeats +++ b/', () => { + const diff = [ + 'diff --git a/code.py b/code.py', + '--- a/code.py', + '+++ b/code.py', + '@@ -5,0 +5,1 @@', + '+++ b/code.py', + ].join('\n'); + expect(parseDiffHunks(diff)).toEqual([ + { filePath: 'code.py', hunks: [{ startLine: 5, endLine: 5 }] }, + ]); + }); + + it('does not treat a quoted +++ content line as a second file', () => { + const diff = [ + 'diff --git a/src/foo.ts b/src/foo.ts', + '--- a/src/foo.ts', + '+++ b/src/foo.ts', + '@@ -1,0 +1,1 @@', + '+++ "b/generated.ts"', + ].join('\n'); + expect(parseDiffHunks(diff)).toEqual([ + { filePath: 'src/foo.ts', hunks: [{ startLine: 1, endLine: 1 }] }, + ]); + }); + + it('parses a quoted source and unquoted dest on the same git header', () => { + const diff = [ + 'diff --git "a/old name.ts" b/new.ts', + 'similarity index 100%', + 'rename from old name.ts', + 'rename to new.ts', + ].join('\n'); + expect(parseDiffHunksResult(diff)).toEqual({ + files: [{ filePath: 'new.ts', hunks: [] }], + unparsedGitHeaders: 0, + }); + }); + + it('parses an unquoted source and quoted dest on the same git header', () => { + const diff = [ + 'diff --git a/old.ts "b/new name.ts"', + 'similarity index 100%', + 'rename from old.ts', + 'rename to new name.ts', + ].join('\n'); + expect(parseDiffHunksResult(diff)).toEqual({ + files: [{ filePath: 'new name.ts', hunks: [] }], + unparsedGitHeaders: 0, + }); + }); + + it('counts an unparsed git header and does not keep current live', () => { + const diff = [ + 'diff --git a/script.sh b/script.sh', + 'old mode 100644', + 'new mode 100755', + 'diff --git not-a-valid-header', + '@@ -1,0 +1,1 @@', + '+stolen', + ].join('\n'); + expect(parseDiffHunksResult(diff)).toEqual({ + files: [{ filePath: 'script.sh', hunks: [] }], + unparsedGitHeaders: 1, + }); + }); });