fix: persist deleted-export advisories so check survives purge ordering#2068
Open
carlos-alm wants to merge 1 commit into
Conversation
checkNoDeletedExportsInUse (#1806) only ever queried the live nodes/edges tables, so once any codegraph build purged a deleted file's rows - detectChanges does this unconditionally, independent of whether codegraph check has run yet - the violation silently went undetected. Capture a durable snapshot of each deleted file's exported defs and their external consumers at the exact point detectChanges/the native orchestrator computes the removed-file set, BEFORE purging. check now falls back to this deleted_export_advisories table whenever the live nodes for a deleted file are already gone. Advisory rows are cleared whenever a file reappears under the same path, so a resolved deletion never misattributes a stale violation. Mirrored in the native Rust orchestrator (record/clear_deleted_export_ advisories in detect_changes.rs, wired into pipeline.rs's save_and_purge_changed), since the native incremental fast path bypasses the JS detectChanges stage entirely. Impact: 28 functions changed, 13 affected
Contributor
Greptile SummaryThis PR persists deleted-export evidence so signature checks can survive graph purges. The main changes are:
Confidence Score: 4/5The advisory persistence path can still drop evidence after repeated builds of the same deleted file.
src/db/repository/deleted-export-advisories.ts and crates/codegraph-core/src/domain/graph/builder/stages/detect_changes.rs Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant B1 as Build N
participant DB as graph.db
participant B2 as Build N+1
participant C as codegraph check
B1->>DB: Capture advisory while deleted file nodes exist
B1->>DB: Purge deleted file nodes and edges
B2->>DB: Detect same removed file again
B2->>DB: Delete existing advisory
B2->>DB: Find no live nodes, insert no advisory
C->>DB: Live query returns empty
C->>DB: Advisory fallback returns empty
C-->>C: Deleted export violation is missed
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant B1 as Build N
participant DB as graph.db
participant B2 as Build N+1
participant C as codegraph check
B1->>DB: Capture advisory while deleted file nodes exist
B1->>DB: Purge deleted file nodes and edges
B2->>DB: Detect same removed file again
B2->>DB: Delete existing advisory
B2->>DB: Find no live nodes, insert no advisory
C->>DB: Live query returns empty
C->>DB: Advisory fallback returns empty
C-->>C: Deleted export violation is missed
Reviews (1): Last reviewed commit: "fix: persist deleted-export advisories s..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const tx = db.transaction(() => { | ||
| const now = Date.now(); | ||
| for (const file of removedFiles) { | ||
| deleteStmt.run(file); |
Contributor
There was a problem hiding this comment.
When the same deleted file is seen by a later incremental build, the prior advisory is deleted before a replacement is known to exist. Because the first build already purged that file's live nodes, the second capture finds no exported definitions and inserts nothing, so a later codegraph check has neither live rows nor an advisory to report the deleted export.
Contributor
Codegraph Impact Analysis28 functions changed → 13 callers affected across 6 files
|
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
checkNoDeletedExportsInUse(#1806) detects an exported function/method/class lost when a file is deleted in its entirety, but only by querying the current DB for that file's rows. Once anycodegraph buildpurges the deleted file'snodes/edgesrows —detectChangesdoes this unconditionally for any file no longer found on disk, regardless of whethercodegraph checkhas run yet — the predicate has nothing left to query and the violation silently goes undetected. This is purely a function of external invocation ordering (e.g. this repo's ownupdate-graph.shhook firing a plaincodegraph buildbeforepre-commit.sh'scheckruns), not something the predicate itself can control.Closes #1938
Fix
Capture a durable, purge-order-independent snapshot instead of relying on live rows surviving until
checkruns:deleted_export_advisoriestable (migration v21, mirrored in bothsrc/db/migrations.tsandcrates/codegraph-core/src/db/connection.rs) stores, per deleted file, each lost export's cross-file consumers.detectChanges(domain/graph/builder/stages/detect-changes.ts) captures this snapshot at the exact point it computes the removed-file set — before purging — via newrecordDeletedExportAdvisories/clearDeletedExportAdvisoriesindb/repository/deleted-export-advisories.ts. A file that reappears under the same path has its stale advisory cleared before its nodes are (re)inserted, so a resolved deletion never misattributes a stale violation.checkNoDeletedExportsInUsenow queries livenodesfirst (unchanged behavior when nothing has purged yet) and falls back to the persisted advisory table only when a deleted file's live rows are already gone.db/repository/nodes.ts/edges.ts(findExportedDefinitions,findExternalConsumers) so both the live-DB path and the advisory-capture path use identical semantics.Native engine parity: the native Rust orchestrator's incremental fast path (
crates/codegraph-core/.../pipeline.rs'ssave_and_purge_changed) bypasses the JSdetectChangesstage entirely for a plain incremental build — this is the default code path when the native engine is available. Mirrored the capture/clear logic there (record_deleted_export_advisories/clear_deleted_export_advisoriesindetect_changes.rs) so both engines produce identical results. Verified via a realbuildGraph()end-to-end regression test that runs against both engines.While implementing, found and fixed a real bug this surfaced: the Rust
table_queryableexistence-probe usedquery_row(...).is_ok(), which returnsErr(QueryReturnedNoRows)— notOk— for a table that exists but is still empty, causing the whole advisory capture to silently no-op on a freshly-migrated table. Caught by the new Rust unit tests; fixed by treatingQueryReturnedNoRowsas "exists".Test plan
cargo test --lib(crates/codegraph-core) — 649 passed, including 8 new tests forrecord_deleted_export_advisories/clear_deleted_export_advisoriesnpm test— 4128 tests, 1 pre-existing unrelated failure (node-order non-determinism incheckNoNewCycles, filed as test: checkNoNewCycles reports the same cycle with non-deterministic node order across calls #2067), everything else green including 30 new tests acrosstests/unit/deleted-export-advisories.test.ts, the newcheckNoDeletedExportsInUseadvisory-fallback describe block intests/integration/check.test.ts, and a new end-to-end regression test (tests/integration/issue-1938-deleted-export-advisory-persistence.test.ts) exercising both WASM and native engines via a realbuildGraph()→ delete → rebuild →checkDataflownpm run lint— clean (only a pre-existing unrelated warning in an untouched fixture file)codegraph diff-impact --staged -T— 28 symbols changed, 13 callers affected across 6 files, no cycles, no dead exportsOut-of-scope findings filed separately
init_schema()never mirrors TS migration v20 (edges.dynamic_kind), even thoughdo_insert_edgesalways references that columncheckNoNewCyclesreports the same cycle with non-deterministic node order across two calls against the same DB stateStacked on #1937/#1929 (base branch
fix/issue-1929-bare-super-constructor-calls-are-never) — only the check/detect-changes/migrations/test diff described above is this issue's change.