diff --git a/src/features/check.ts b/src/features/check.ts index 3b1ae049d..ba619d7b1 100644 --- a/src/features/check.ts +++ b/src/features/check.ts @@ -584,6 +584,11 @@ interface SignatureResult { violations: SignatureViolation[]; } +/** Escape a symbol name for literal use inside a RegExp. */ +function escapeRegExp(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + /** * `db` reflects the current working-tree (post-change) file content, so * `nodes.line` values are in new-file coordinates. `changedRanges` must be @@ -591,10 +596,18 @@ interface SignatureResult { * `diff.oldRanges` (which is pre-change/old-file and would only line up * with `db` by coincidence once a hunk changes the file's total line * count). See issues #1732 and #1737. + * + * `changedEdits` (same new-file coordinate space, same start/end as + * `changedRanges`) is the base-ref evidence: a declaration is only a + * *modification* if a symbol of the same name appears in one of the file's + * removed lines. Additions, renames, and newly-exported names — none of + * which existed under that name at the base ref — are skipped. Without this, + * appending or renaming an export trips the predicate. See issue #2674. */ export function checkNoSignatureChanges( db: BetterSqlite3Database, changedRanges: Map, + changedEdits: Map, noTests: boolean, ): SignatureResult { const violations: SignatureViolation[] = []; @@ -615,8 +628,21 @@ export function checkNoSignatureChanges( `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[]; + const edits = changedEdits.get(file); for (const def of defs) { + // A declaration can only be *modified* if a symbol of the same name + // existed at the base ref. A name absent from every removed line is a + // new export — an append, a rename, or a promotion — not a signature + // change. With no edit runs for this file the diff carries no base-ref + // evidence, so fall back to flagging. See #2674. + if (edits && edits.length > 0) { + const nameRe = new RegExp(`(^|[^A-Za-z0-9_$])${escapeRegExp(def.name)}([^A-Za-z0-9_$]|$)`); + const existedAtBase = edits.some((edit) => + edit.removedText.some((line) => nameRe.test(line)), + ); + if (!existedAtBase) continue; + } for (const range of ranges) { if (def.line >= range.start && def.line <= range.end) { violations.push({ @@ -915,7 +941,12 @@ function runPredicates( // flag/config and lets existing consumers of the 'signatures' predicate // (the pre-commit hook, `codegraph check --json`) pick up the new // violations with no wiring changes. - const editedResult = checkNoSignatureChanges(db, diff.changedRanges, noTests); + const editedResult = checkNoSignatureChanges( + db, + diff.changedRanges, + diff.changedEdits, + noTests, + ); const deletedResult = checkNoDeletedExportsInUse(db, diff.deletedFiles, noTests); predicates.push({ name: 'signatures', diff --git a/tests/integration/check.test.ts b/tests/integration/check.test.ts index b2724f9cc..ddd9e04c4 100644 --- a/tests/integration/check.test.ts +++ b/tests/integration/check.test.ts @@ -714,14 +714,14 @@ describe('checkNoSignatureChanges', () => { test('passes for body-only changes', () => { // add is at line 1. Changing lines 3-5 (body only) should pass const changedRanges = new Map([['src/math.js', [{ start: 3, end: 5 }]]]); - const result = checkNoSignatureChanges(db, changedRanges, false); + const result = checkNoSignatureChanges(db, changedRanges, new Map(), false); expect(result.passed).toBe(true); }); test('fails when declaration line is in a changed hunk', () => { // add is at line 1. Changing lines 1-2 (includes declaration) should fail const changedRanges = new Map([['src/math.js', [{ start: 1, end: 2 }]]]); - const result = checkNoSignatureChanges(db, changedRanges, false); + const result = checkNoSignatureChanges(db, changedRanges, new Map(), false); expect(result.passed).toBe(false); expect(result.violations.length).toBeGreaterThanOrEqual(1); expect(result.violations[0].name).toBe('add'); @@ -729,7 +729,7 @@ describe('checkNoSignatureChanges', () => { test('skips test files when noTests is true', () => { const changedRanges = new Map([['tests/math.test.js', [{ start: 1, end: 5 }]]]); - const result = checkNoSignatureChanges(db, changedRanges, true); + const result = checkNoSignatureChanges(db, changedRanges, new Map(), true); expect(result.passed).toBe(true); }); @@ -739,7 +739,7 @@ describe('checkNoSignatureChanges', () => { // 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 changedRanges = new Map([['src/math.js', [{ start: 14, end: 16 }]]]); - const result = checkNoSignatureChanges(db, changedRanges, false); + const result = checkNoSignatureChanges(db, changedRanges, new Map(), false); expect(result.passed).toBe(true); }); @@ -755,7 +755,7 @@ describe('checkNoSignatureChanges', () => { ], ], ]); - const result = checkNoSignatureChanges(db, changedRanges, false); + const result = checkNoSignatureChanges(db, changedRanges, new Map(), false); expect(result.passed).toBe(false); expect(result.violations.map((v) => v.name)).toEqual(['add']); }); @@ -787,7 +787,7 @@ describe('checkNoSignatureChanges', () => { // ...but checkNoSignatureChanges itself is driven by changedRanges // (new-file coordinates). This hunk has no added lines, so there is // nothing to compare against and multiply is correctly left alone. - const result = checkNoSignatureChanges(db, changedRanges, false); + const result = checkNoSignatureChanges(db, changedRanges, new Map(), false); expect(result.passed).toBe(true); }); @@ -828,13 +828,18 @@ describe('checkNoSignatureChanges', () => { // Prove the regression is real: had the call site still passed // oldRanges, isPidAlive's post-change line (2) falls inside the old // range [2, 12] and would be wrongly flagged. - const buggyResult = checkNoSignatureChanges(db, parsed.oldRanges, false); + const buggyResult = checkNoSignatureChanges(db, parsed.oldRanges, parsed.changedEdits, false); expect(buggyResult.passed).toBe(false); expect(buggyResult.violations.map((v) => v.name)).toContain('isPidAlive'); // The fix: changedRanges is empty for a pure deletion, so there is // nothing to compare against and isPidAlive is correctly left alone. - const fixedResult = checkNoSignatureChanges(db, parsed.changedRanges, false); + const fixedResult = checkNoSignatureChanges( + db, + parsed.changedRanges, + parsed.changedEdits, + false, + ); expect(fixedResult.passed).toBe(true); }); @@ -852,13 +857,72 @@ describe('checkNoSignatureChanges', () => { '+function someExportedFn(extra) {', ].join('\n'); - const { changedRanges } = parseDiffOutput(diff); + const { changedRanges, changedEdits } = parseDiffOutput(diff); expect(changedRanges.get('src/coordshift2.js')).toEqual([{ start: 1, end: 1 }]); + // A replacement run carries the removed declaration text, so it is a + // genuine modification and must still flag (issue #2674). + expect(changedEdits.get('src/coordshift2.js')?.[0].removedText).toEqual([ + 'function someExportedFn() {', + ]); - const result = checkNoSignatureChanges(db, changedRanges, false); + const result = checkNoSignatureChanges(db, changedRanges, changedEdits, false); expect(result.passed).toBe(false); expect(result.violations.map((v) => v.name)).toContain('someExportedFn'); }); + + test('regression: a newly ADDED exported function is not a signature change (issue #2674)', () => { + // The db reflects the working tree AFTER the addition, so the new + // symbol is already present at its post-change line. + insertNode(db, 'brandNewThing', 'function', 'src/added-export.js', 25, 27, 1); + + // A pure insertion — the exact shape of appending a new export at EOF. + // No line is removed, so the declaration did not exist at the base ref. + const diff = [ + '--- a/src/added-export.js', + '+++ b/src/added-export.js', + '@@ -24,0 +25,3 @@', + '+export function brandNewThing(x) {', + '+ return x;', + '+}', + ].join('\n'); + + const { changedRanges, changedEdits } = parseDiffOutput(diff); + expect(changedRanges.get('src/added-export.js')).toEqual([{ start: 25, end: 27 }]); + expect(changedEdits.get('src/added-export.js')?.[0].removedText).toEqual([]); + + // Before the fix the changed range covering line 25 flagged the new + // export. `changedEdits` marks the run as a pure insertion (nothing + // removed), so the new symbol is not treated as a modification. + const result = checkNoSignatureChanges(db, changedRanges, changedEdits, false); + expect(result.passed).toBe(true); + expect(result.violations).toEqual([]); + }); + + test('regression: renamed/newly-exported names sharing a hunk with removals are not flagged (issue #2674)', () => { + // Base exported only `register`. The diff renames it to `registerPanel` + // and adds `subscribeComposition`. The added run is paired with removed + // text, so `removedText` is NOT empty — the pure-insertion hedge alone + // was insufficient. Neither new name existed at the base ref. + insertNode(db, 'registerPanel', 'function', 'src/rename.js', 1, 3, 1); + insertNode(db, 'subscribeComposition', 'function', 'src/rename.js', 2, 4, 1); + + const diff = [ + '--- a/src/rename.js', + '+++ b/src/rename.js', + '@@ -1,1 +1,2 @@', + '-function register(id) {', + '+export function registerPanel(id) {', + '+export function subscribeComposition() {}', + ].join('\n'); + + const { changedRanges, changedEdits } = parseDiffOutput(diff); + expect(changedRanges.get('src/rename.js')).toEqual([{ start: 1, end: 2 }]); + expect(changedEdits.get('src/rename.js')?.[0].removedText).toEqual(['function register(id) {']); + + const result = checkNoSignatureChanges(db, changedRanges, changedEdits, false); + expect(result.passed).toBe(true); + expect(result.violations).toEqual([]); + }); }); // ─── checkNoDeletedExportsInUse (issue #1806) ──────────────────────────