From c07c1421aa85f00eb583491820bb07702c9c9224 Mon Sep 17 00:00:00 2001 From: avengedsevenskull-ctrl <253869695+avengedsevenskull-ctrl@users.noreply.github.com> Date: Wed, 30 Sep 2026 08:15:19 -0300 Subject: [PATCH 1/2] fix(check): don't flag newly-added exported functions as signature changes `checkNoSignatureChanges` flagged any exported declaration whose line fell inside a changed range, so appending a new export tripped `check --signatures` even though the symbol did not exist at the base ref. That contradicts the predicate's own contract ("Assert no exported function/method/class declaration lines were modified"). Reproduced on v3.17.0; filed as #2674. Thread the already-parsed `ParsedDiff.changedEdits` into the predicate (same new-file coordinate space and start/end as `changedRanges`). A run whose `removedText` is empty is a pure insertion, so an exported symbol on it is new at the base ref and cannot be a signature change. - src/features/check.ts: add a `changedEdits` parameter, skip pure-insertion runs when matching a declaration, and pass `diff.changedEdits` from `runPredicates` - tests/integration/check.test.ts: add a regression test for the added-export case and assert the replaced-declaration case still flags Scope: B1 -> B2 = src/features/check.ts, tests/integration/check.test.ts (match) Verification: npx vitest run tests/integration/check.test.ts -> 61/61 passed; tsc build clean Refs #2674 Impact: 2 functions changed, 5 affected --- src/features/check.ts | 21 +++++++++++- tests/integration/check.test.ts | 58 +++++++++++++++++++++++++++------ 2 files changed, 68 insertions(+), 11 deletions(-) diff --git a/src/features/check.ts b/src/features/check.ts index 3b1ae049d..c155dc3ec 100644 --- a/src/features/check.ts +++ b/src/features/check.ts @@ -591,10 +591,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`) distinguishes a declaration that was *modified* from one + * that is merely *new*: a run whose `removedText` is empty is a pure + * insertion, so any exported symbol on it did not exist at the base ref and + * cannot be a signature change. Without this, appending a new export trips + * the predicate. See issue #2674. */ export function checkNoSignatureChanges( db: BetterSqlite3Database, changedRanges: Map, + changedEdits: Map, noTests: boolean, ): SignatureResult { const violations: SignatureViolation[] = []; @@ -615,10 +623,16 @@ 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) { for (const range of ranges) { if (def.line >= range.start && def.line <= range.end) { + // A pure insertion (`removedText` empty) means the declaration is + // new at this diff's base ref — an added export, not a modified + // one. Only a declaration that replaced existing text flags. #2674 + const edit = edits.find((e) => e.start === range.start && e.end === range.end); + if (edit && edit.removedText.length === 0) break; violations.push({ name: def.name, kind: def.kind, @@ -915,7 +929,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..d60c0ba91 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,46 @@ 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([]); + }); }); // ─── checkNoDeletedExportsInUse (issue #1806) ────────────────────────── From a8254a622bf3da9676e7cd62d6471c50df85c01d Mon Sep 17 00:00:00 2001 From: avengedsevenskull-ctrl <253869695+avengedsevenskull-ctrl@users.noreply.github.com> Date: Wed, 30 Sep 2026 08:20:03 -0300 Subject: [PATCH 2/2] fix(check): flag a modified export only when the name existed at base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first cut treated an added-only run as "new" but still flagged a current export whose name was new whenever its hunk also contained removed text — e.g. a rename (`register` -> `registerPanel`) or a newly exported helper added beside a deletion. Those names were never exported at the base ref, so they cannot break callers; it is the same false positive in its rename/promotion facet. Require base-ref evidence by name: a declaration is a violation only if the same name appears in one of the file's removed lines. Appends, renames and newly-exported names are skipped; genuine same-name signature edits still flag. With no edit runs for a file the diff carries no base-ref evidence, so the predicate falls back to flagging. - src/features/check.ts: replace the pure-insertion check with a name-exists-in-removed-lines check; add an escapeRegExp helper - tests/integration/check.test.ts: add the rename/new-export-in-shared-hunk regression test Scope: B1 -> B2 = src/features/check.ts, tests/integration/check.test.ts (match) Verification: npx vitest run tests/integration/check.test.ts -> 62/62 passed Refs #2674 Impact: 2 functions changed, 5 affected --- src/features/check.ts | 34 ++++++++++++++++++++++----------- tests/integration/check.test.ts | 26 +++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 11 deletions(-) diff --git a/src/features/check.ts b/src/features/check.ts index c155dc3ec..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 @@ -593,11 +598,11 @@ interface SignatureResult { * count). See issues #1732 and #1737. * * `changedEdits` (same new-file coordinate space, same start/end as - * `changedRanges`) distinguishes a declaration that was *modified* from one - * that is merely *new*: a run whose `removedText` is empty is a pure - * insertion, so any exported symbol on it did not exist at the base ref and - * cannot be a signature change. Without this, appending a new export trips - * the predicate. See issue #2674. + * `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, @@ -623,16 +628,23 @@ 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) ?? []; + 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) { - // A pure insertion (`removedText` empty) means the declaration is - // new at this diff's base ref — an added export, not a modified - // one. Only a declaration that replaced existing text flags. #2674 - const edit = edits.find((e) => e.start === range.start && e.end === range.end); - if (edit && edit.removedText.length === 0) break; violations.push({ name: def.name, kind: def.kind, diff --git a/tests/integration/check.test.ts b/tests/integration/check.test.ts index d60c0ba91..ddd9e04c4 100644 --- a/tests/integration/check.test.ts +++ b/tests/integration/check.test.ts @@ -897,6 +897,32 @@ describe('checkNoSignatureChanges', () => { 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) ──────────────────────────