diff --git a/src/cli/commands/check.ts b/src/cli/commands/check.ts index fed4d1fcb..a00cec970 100644 --- a/src/cli/commands/check.ts +++ b/src/cli/commands/check.ts @@ -30,7 +30,7 @@ export const command: CommandDefinition = { ['--rules', 'Also run manifesto rules alongside diff predicates'], ['--cycles', 'Assert no dependency cycles involve changed files'], ['--blast-radius ', 'Assert no function exceeds N transitive callers'], - ['--signatures', 'Assert no function declaration lines were modified'], + ['--signatures', 'Assert no exported function/method/class declaration lines were modified'], ['--boundaries', 'Assert no cross-owner boundary violations'], ['--depth ', 'Max BFS depth for blast radius (default: 3)'], ['-f, --file ', 'Scope to file (partial match, repeatable, manifesto mode)', collectFile], diff --git a/src/features/check.ts b/src/features/check.ts index 471e7492e..2e9fc9df6 100644 --- a/src/features/check.ts +++ b/src/features/check.ts @@ -41,23 +41,99 @@ function extractNewFileName(line: string): string | null { return m ? m[1]! : null; } -function pushHunkRanges( - line: string, - currentFile: string, - changedRanges: Map, - oldRanges: Map, -): void { - const hunkMatch = line.match(HUNK_RE); - if (!hunkMatch) return; - const oldStart = parseInt(hunkMatch[1]!, 10); - const oldCount = parseInt(hunkMatch[2] || '1', 10); - if (oldCount > 0) { - oldRanges.get(currentFile)!.push({ start: oldStart, end: oldStart + oldCount - 1 }); +/** Returns true if the diff line marks the new file as /dev/null (file deletion). */ +function isDevNullTargetLine(line: string): boolean { + return line.startsWith('+++ /dev/null'); +} + +/** + * Tracks the old-side and new-side line cursors and the current runs of + * contiguously removed/added lines while walking a hunk body, so + * `parseDiffOutput` can emit precise `oldRanges` and `changedRanges` (only + * lines actually deleted/replaced or added) instead of the raw hunk header + * span (which always includes unchanged context lines on each side per the + * unified diff format whenever the diff was produced with non-zero context). + * `getGitDiff` always passes `--unified=0`, so in practice the header span + * and the tracked lines coincide today — but `parseDiffOutput` is a public + * export and this keeps it correct for any diff input, not just this one + * caller's. + */ +class DiffLineTracker { + private oldLineCursor = 0; + private newLineCursor = 0; + private removedRunStart: number | null = null; + private removedRunEnd: number | null = null; + private addedRunStart: number | null = null; + private addedRunEnd: number | null = null; + + startHunk(oldStart: number, newStart: number): void { + this.oldLineCursor = oldStart; + this.newLineCursor = newStart; + } + + /** + * 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`). + */ + consume( + line: string, + file: string, + oldRanges: Map, + changedRanges: Map, + ): void { + if (line.startsWith('-')) { + this.flushAdded(file, changedRanges); + if (this.removedRunStart === null) this.removedRunStart = this.oldLineCursor; + this.removedRunEnd = this.oldLineCursor; + this.oldLineCursor++; + return; + } + if (line.startsWith('+')) { + this.flushRemoved(file, oldRanges); + if (this.addedRunStart === null) this.addedRunStart = this.newLineCursor; + this.addedRunEnd = this.newLineCursor; + this.newLineCursor++; + return; + } + // A context line or a "\ No newline" marker ends both runs. + this.flushRemoved(file, oldRanges); + this.flushAdded(file, changedRanges); + if (line.startsWith(' ')) { + this.oldLineCursor++; + this.newLineCursor++; + } + } + + /** Closes out the current removed-line run, if any, into `oldRanges`. */ + flushRemoved(file: string, oldRanges: Map): void { + if (this.removedRunStart !== null) { + oldRanges.get(file)!.push({ start: this.removedRunStart, end: this.removedRunEnd! }); + this.removedRunStart = null; + this.removedRunEnd = null; + } } - const newStart = parseInt(hunkMatch[3]!, 10); - const newCount = parseInt(hunkMatch[4] || '1', 10); - if (newCount > 0) { - changedRanges.get(currentFile)!.push({ start: newStart, end: newStart + newCount - 1 }); + + /** Closes out the current added-line run, if any, into `changedRanges`. */ + flushAdded(file: string, changedRanges: Map): void { + if (this.addedRunStart !== null) { + changedRanges.get(file)!.push({ start: this.addedRunStart, end: this.addedRunEnd! }); + this.addedRunStart = null; + this.addedRunEnd = null; + } + } + + /** Closes out both the removed- and added-line runs, if any. */ + flush( + file: string, + oldRanges: Map, + changedRanges: Map, + ): void { + this.flushRemoved(file, oldRanges); + this.flushAdded(file, changedRanges); } } @@ -67,6 +143,7 @@ export function parseDiffOutput(diffOutput: string): ParsedDiff { const newFiles = new Set(); let currentFile: string | null = null; let prevIsDevNull = false; + const tracker = new DiffLineTracker(); for (const line of diffOutput.split('\n')) { if (isDevNullSourceLine(line)) { @@ -79,6 +156,7 @@ export function parseDiffOutput(diffOutput: string): ParsedDiff { } const newFile = extractNewFileName(line); if (newFile) { + if (currentFile) tracker.flush(currentFile, oldRanges, changedRanges); currentFile = newFile; if (!changedRanges.has(currentFile)) changedRanges.set(currentFile, []); if (!oldRanges.has(currentFile)) oldRanges.set(currentFile, []); @@ -86,8 +164,32 @@ export function parseDiffOutput(diffOutput: string): ParsedDiff { prevIsDevNull = false; continue; } - if (currentFile) pushHunkRanges(line, currentFile, changedRanges, oldRanges); + 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); + currentFile = null; + prevIsDevNull = false; + continue; + } + if (!currentFile) continue; + + const hunkMatch = line.match(HUNK_RE); + if (hunkMatch) { + tracker.flush(currentFile, oldRanges, changedRanges); + tracker.startHunk(parseInt(hunkMatch[1]!, 10), parseInt(hunkMatch[3]!, 10)); + continue; + } + + tracker.consume(line, currentFile, oldRanges, changedRanges); } + if (currentFile) tracker.flush(currentFile, oldRanges, changedRanges); + return { changedRanges, oldRanges, newFiles }; } @@ -209,9 +311,16 @@ export function checkNoSignatureChanges( if (ranges.length === 0) continue; if (noTests && isTestFile(file)) continue; + // Scoped to `exported = 1`: only a symbol reachable from outside its own + // file can have callers this diff doesn't already account for. A + // private, file-local helper's declaration can be freely deleted, + // renamed, or reshaped — every call site lives in the same file and is + // necessarily part of the same diff. Without this filter, any adoption + // of a shared helper that removes a file-local duplicate (the exact + // pattern the grind workflow performs) would always trip this check. const defs = db .prepare( - `SELECT name, kind, file, line FROM nodes WHERE file = ? AND kind IN ('function', 'method', 'class') ORDER BY line`, + `SELECT name, kind, file, line FROM nodes WHERE file = ? AND kind IN ('function', 'method', 'class') AND exported = 1 ORDER BY line`, ) .all(file) as SignatureViolation[]; diff --git a/tests/integration/check.test.ts b/tests/integration/check.test.ts index 9866f20e0..3fc78fbbc 100644 --- a/tests/integration/check.test.ts +++ b/tests/integration/check.test.ts @@ -22,10 +22,12 @@ import { // ─── Helpers ─────────────────────────────────────────────────────────── -function insertNode(db, name, kind, file, line, endLine = null) { +function insertNode(db, name, kind, file, line, endLine = null, exported = 0) { return db - .prepare('INSERT INTO nodes (name, kind, file, line, end_line) VALUES (?, ?, ?, ?, ?)') - .run(name, kind, file, line, endLine).lastInsertRowid; + .prepare( + 'INSERT INTO nodes (name, kind, file, line, end_line, exported) VALUES (?, ?, ?, ?, ?, ?)', + ) + .run(name, kind, file, line, endLine, exported).lastInsertRowid; } function insertEdge(db, sourceId, targetId, kind) { @@ -48,9 +50,11 @@ beforeAll(() => { initSchema(db); // --- Functions --- - // src/math.js: add (line 1-5), multiply (line 7-12) - const add = insertNode(db, 'add', 'function', 'src/math.js', 1, 5); - const multiply = insertNode(db, 'multiply', 'function', 'src/math.js', 7, 12); + // src/math.js: add (line 1-5, exported), multiply (line 7-12, exported), + // roundHalfEven (line 14-16, private — not part of the module's public API) + const add = insertNode(db, 'add', 'function', 'src/math.js', 1, 5, 1); + const multiply = insertNode(db, 'multiply', 'function', 'src/math.js', 7, 12, 1); + insertNode(db, 'roundHalfEven', 'function', 'src/math.js', 14, 16, 0); // src/utils.js: formatResult (line 1-10), parseInput (line 12-20) const formatResult = insertNode(db, 'formatResult', 'function', 'src/utils.js', 1, 10); @@ -101,9 +105,13 @@ describe('parseDiffOutput', () => { '--- a/src/math.js', '+++ b/src/math.js', '@@ -1,3 +1,4 @@', - '-old line', + '-old line 1', + '-old line 2', + '-old line 3', '+new line 1', '+new line 2', + '+new line 3', + '+new line 4', ].join('\n'); const { changedRanges, oldRanges, newFiles } = parseDiffOutput(diff); @@ -113,6 +121,55 @@ describe('parseDiffOutput', () => { expect(newFiles.size).toBe(0); }); + test('old-side ranges exclude unchanged context lines around a removal', () => { + // Simulates deleting a block that sits between two untouched declarations — + // the hunk header span (2..9) must not swallow the untouched line 9. + const diff = [ + '--- a/src/math.js', + '+++ b/src/math.js', + '@@ -2,8 +2,2 @@', + ' function add() {}', // context, old line 2 — untouched + '-function removedHelper() {', + '- return 1;', + '-}', + '-function alsoRemoved() {', + '- return 2;', + '-}', + ' function multiply() {}', // context, old line 9 — untouched + ].join('\n'); + + const { oldRanges } = parseDiffOutput(diff); + // Only the 6 actually-removed lines (3-8) should appear, not the + // untouched context lines immediately before (2) and after (9). + expect(oldRanges.get('src/math.js')).toEqual([{ start: 3, end: 8 }]); + }); + + test('new-side ranges exclude unchanged context lines around an addition', () => { + // Symmetric counterpart of the old-side test above: `getGitDiff` always + // uses `--unified=0` so this never happens in practice, but + // `parseDiffOutput` is a public export and must stay correct for any + // unified diff, including ones with non-zero context. + const diff = [ + '--- a/src/math.js', + '+++ b/src/math.js', + '@@ -2,2 +2,8 @@', + ' function add() {}', // context, line 2 — untouched + '+function addedHelper() {', + '+ return 1;', + '+}', + '+function alsoAdded() {', + '+ return 2;', + '+}', + ' function multiply() {}', // context, line 9 — untouched + ].join('\n'); + + const { changedRanges } = parseDiffOutput(diff); + // Only the 6 actually-added lines (3-8) should appear, not the untouched + // context lines immediately before (2) and after (9) — even though the + // raw hunk header span for the new side is 2..9. + expect(changedRanges.get('src/math.js')).toEqual([{ start: 3, end: 8 }]); + }); + test('detects new files', () => { const diff = ['--- /dev/null', '+++ b/src/new-file.js', '@@ -0,0 +1,5 @@', '+line1'].join('\n'); @@ -122,6 +179,37 @@ describe('parseDiffOutput', () => { expect(oldRanges.get('src/new-file.js')).toEqual([]); }); + test('a deleted file does not corrupt the ranges of the preceding file', () => { + // Regression test: `+++ /dev/null` (file deletion) is not `b/`-prefixed, + // so it was previously missed by every guard and fell through to + // tracker.consume, which recorded it as an added line under whichever + // file preceded it in the diff — corrupting that file's changedRanges + // and then misattributing the deleted file's removed lines to it too. + const diff = [ + '--- a/src/kept.js', + '+++ b/src/kept.js', + '@@ -1,1 +1,1 @@', + '-old kept line', + '+new kept line', + '--- a/src/removed.js', + '+++ /dev/null', + '@@ -1,2 +0,0 @@', + '-deleted line 1', + '-deleted line 2', + ].join('\n'); + + const { changedRanges, oldRanges } = parseDiffOutput(diff); + // kept.js's ranges must reflect only its own hunk, not the deleted + // file's `+++ /dev/null` header or removed body lines. + expect(changedRanges.get('src/kept.js')).toEqual([{ start: 1, end: 1 }]); + expect(oldRanges.get('src/kept.js')).toEqual([{ start: 1, end: 1 }]); + // The deleted file itself is not tracked — its nodes are purged from the + // DB during the rebuild, so there is nothing for the check predicates to + // match against. + expect(changedRanges.has('src/removed.js')).toBe(false); + expect(oldRanges.has('src/removed.js')).toBe(false); + }); + test('handles multiple files', () => { const diff = [ '--- a/src/a.js', @@ -212,6 +300,60 @@ describe('checkNoSignatureChanges', () => { const result = checkNoSignatureChanges(db, oldRanges, true); expect(result.passed).toBe(true); }); + + test('does not flag a private (non-exported) symbol whose declaration was removed', () => { + // roundHalfEven is at line 14, exported=0. Deleting it entirely — the + // exact "adopt shared helper, drop the file-local duplicate" pattern + // grind performs — must not trip this check: every caller of a + // private helper lives in the same file and is already part of the diff. + const oldRanges = new Map([['src/math.js', [{ start: 14, end: 16 }]]]); + const result = checkNoSignatureChanges(db, oldRanges, false); + expect(result.passed).toBe(true); + }); + + test('still flags an exported symbol even when a private symbol shares the file', () => { + // Sanity check that the exported-only filter doesn't accidentally + // suppress real violations on exported declarations in the same file. + const oldRanges = new Map([ + [ + 'src/math.js', + [ + { start: 1, end: 2 }, // add's declaration (exported) + { start: 14, end: 16 }, // roundHalfEven's declaration (private) + ], + ], + ]); + const result = checkNoSignatureChanges(db, oldRanges, false); + expect(result.passed).toBe(false); + expect(result.violations.map((v) => v.name)).toEqual(['add']); + }); + + test('regression: deleting a line near a declaration does not flag it (issue #1760)', () => { + // Fixture DB: `add` spans lines 1-5, `multiply`'s declaration is at + // line 7. A real `git diff` carries 3 lines of context around a + // change, so deleting the single stale line 6 produces a hunk whose + // *header span* (3..9) includes multiply's declaration line (7) even + // though multiply's own text is untouched. parseDiffOutput must only + // record line 6 as removed, not the surrounding context. + const diff = [ + '--- a/src/math.js', + '+++ b/src/math.js', + '@@ -3,7 +3,6 @@', + ' return 1;', // context, old line 3 (inside add) + ' }', // context, old line 4 + ' ', // context, old line 5 + '-// stale comment', // removed, old line 6 + ' function multiply() {', // context, old line 7 — multiply's declaration + ' return 2;', // context, old line 8 + ' }', // context, old line 9 + ].join('\n'); + + const { oldRanges } = parseDiffOutput(diff); + expect(oldRanges.get('src/math.js')).toEqual([{ start: 6, end: 6 }]); + + const result = checkNoSignatureChanges(db, oldRanges, false); + expect(result.passed).toBe(true); + }); }); // ─── checkNoBoundaryViolations ────────────────────────────────────────