Repository navigation
fix(check): only flag modified exports whose name existed at the base ref - #2677
Open
avengedsevenskull-ctrl wants to merge 2 commits into
Open
avengedsevenskull-ctrl wants to merge 2 commits into
avengedsevenskull-ctrl wants to merge 2 commits into
Conversation
…anges
`checkNoSignatureChanges` flagged any exported declaration whose line fell
inside a changed range, so appending a new export tripped `check <ref>
--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 optave#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 optave#2674
Impact: 2 functions changed, 5 affected
Contributor
Contributor
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
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 optave#2674 Impact: 2 functions changed, 5 affected
avengedsevenskull-ctrl
changed the title
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
check <ref> --signaturesflags an exported declaration only when a symbol of the same name existed at the base ref and its declaration line changed. Appends, renames, and newly-exported names are no longer treated as signature modifications.Why
checkNoSignatureChangesflagged any exported declaration whose line fell inside a changed range. That produced two false-positive facets, both blocking additive work:register→registerPanel, plus a newsubscribeComposition), trips it because the hunk has removed text.In both cases no existing exported name changed, so no caller can break. Repro on v3.17.0: append
export function brandNewThing(x) { return x; },codegraph build . --no-incremental,codegraph check HEAD→[FAIL] signatures: brandNewThing, exit 1. Filed as #2674.Scope snapshot
src/features/check.ts,tests/integration/check.test.ts— matchavengedsevenskull-ctrl:fix/2674-signatures-added-export@ a8254a6What changed
src/features/check.ts:checkNoSignatureChangestakeschangedEdits(same new-file coordinate space aschangedRanges) and only flags a declaration when the same name appears in one of the file's removed lines. With no edit runs for a file (no base-ref evidence) it falls back to the previous behaviour. Adds anescapeRegExphelper;runPredicatespassesdiff.changedEdits.tests/integration/check.test.ts: regression tests for the pure-append case and the rename/new-export-in-shared-hunk case; the replaced-declaration case still flags.Verification
Local test suite and build:
npx vitest run tests/integration/check.test.ts→ 62/62 passedtscbuild clean;biome check src/ tests/clean for the changed files (one pre-existing, unrelated warning atsrc/graph/algorithms/louvain.ts:135); commitlint passedEnd-to-end with
codegraph check HEAD --signatureson a local build of this branch:[PASS] signatures(exit 0) — wasFAIL[FAIL] signatures(exit 1) — real change still caught[PASS] signatures(exit 0) — previously flaggedfunction foo→export function foo[FAIL] signatures— documented residual (see Limitation)Case C is the reported facet:
register→registerPanelplus a newsubscribeComposition, in a hunk that also carries removed text.Repository state
Risk & rollback
Low: predicate-only, no runtime path. Genuine same-name signature edits still flag (covered by the retained test). Rollback = revert these two commits.
Limitation: base-ref evidence is derived from the diff's removed lines rather than a full parse of the base file. A same-name private→public promotion (
function foo→export function foo, case D) still flags. Truly matching "exported at base" by name would require parsing the base revision — happy to follow up on that if you prefer it over keeping the predicate synchronous.Tracker
Refs #2674 (this PR does not close it)