fix(check): scope signature-change detection to exported symbols

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 <n>', '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 <n>', 'Max BFS depth for blast radius (default: 3)'],
     ['-f, --file <path>', 'Scope to file (partial match, repeatable, manifesto mode)', collectFile],

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<string, DiffRange[]>,
-  oldRanges: Map<string, DiffRange[]>,
-): 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<string, DiffRange[]>,
+    changedRanges: Map<string, DiffRange[]>,
+  ): 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<string, DiffRange[]>): 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<string, DiffRange[]>): 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<string, DiffRange[]>,
+    changedRanges: Map<string, DiffRange[]>,
+  ): void {
+    this.flushRemoved(file, oldRanges);
+    this.flushAdded(file, changedRanges);
   }
 }
 
@@ -67,6 +143,7 @@ export function parseDiffOutput(diffOutput: string): ParsedDiff {
   const newFiles = new Set<string>();
   let currentFile: string | null = null;
   let prevIsDevNull = false;
+  const tracker = new DiffLineTracker();
 
   for (const line of diffOutput.split('\n')) {
     if (isDevNullSourceLine(line)) {
@@ -79,15 +156,40 @@ 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, []);
       if (prevIsDevNull) newFiles.add(currentFile);
       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[];
 

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 ────────────────────────────────────────