fix(check): discriminate file-level from symbol-level consumers (#1973) - #2188
Conversation
findExternalConsumers's query (backing checkNoDeletedExportsInUse) matched both 'calls' and 'imports-type' edges with no way to tell them apart. For an imports-type edge, e.source_id is the importing file's own node (not a real call-site), so caller.line is always a fabricated 0 and caller.name equals caller.file. presentation/check.ts rendered this as a misleading `<file>:0` in the CI violation message, looking like a genuine call-site line when there is none -- the same class of bug fixed for exports' consumer output in #1830. Add consumerKind ('file' | 'symbol'), derived from the source node's own kind, mirroring #1830's exact discriminator. Optional on ExternalConsumerRow since the persisted deleted-export-advisories snapshot (#1938) doesn't store this discriminator -- only findExternalConsumers's live-DB query populates it; advisory-derived consumers keep their existing file:line rendering unchanged, since retrofitting that snapshot's schema is out of scope here. presentation/check.ts now renders a file-level consumer as `<file> (type-only import)` instead of a fabricated line number. No native Rust mirror needed -- checkNoDeletedExportsInUse is a pure query-layer feature reading whatever DB either engine already built, not something the native engine implements separately. Closes #1973 docs check acknowledged: internal check-predicate fix, no README/CLAUDE.md/ROADMAP surface change Impact: 6 functions changed, 10 affected
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // for #1830 (see `domain/analysis/exports.ts`). Renderers must not treat | ||
| // `name`/`line` on a `'file'` entry as a caller symbol/call-site (#1973). | ||
| return rows.map(({ sourceKind, ...row }) => ({ | ||
| ...row, |
There was a problem hiding this comment.
When an external call occurs at file scope, the call resolver uses the file node as its source, so this mapping classifies the genuine call as a file consumer and the check output labels it as a type-only import instead of reporting its call site.
Knowledge Base Used:
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed — switched the discriminator to key off e.kind (the edge's own kind) instead of caller.kind (the source node's kind). findExternalConsumers now selects e.kind AS edgeKind, and consumerKind = edgeKind === 'imports-type' ? 'file' : 'symbol'. This correctly handles the top-level-call-sourced-from-file-node case you flagged: a genuine 'calls' edge always stays 'symbol' regardless of what its source node happens to be, since only imports-type edges are file-sourced by construction. Added a dedicated regression test (tests/integration/check.test.ts) with exactly this fixture shape (a real top-level call from a file node), and mirrored the same fix in the native Rust advisory-recording path (it runs its own independent query). Also found this same bug already shipped in exports.ts's #1830 fix — filed separately as #2189 since that file isn't part of this diff.
| const sample = v.consumers | ||
| .slice(0, 3) | ||
| .map((c) => `${c.file}:${c.line}`) | ||
| .map((c) => |
There was a problem hiding this comment.
When deleted nodes have already been purged, advisory-derived consumers omit consumerKind, so a type-only import takes the fallback file:line branch and the check continues to display a fabricated file-node line instead of identifying the type-only import.
Knowledge Base Used:
There was a problem hiding this comment.
Fixed — recordDeletedExportAdvisories already calls findExternalConsumers (which now computes consumerKind) at capture time, so persisting it through was a natural extension rather than out of scope. Added migration v22 (consumer_kind TEXT column on deleted_export_advisories), mirrored in both engines' migration lists, and threaded the field through record/read on both the TS and native Rust sides. getDeletedExportAdvisories probes for the column's existence the same way hasAdvisoryTable already probes for the table, so a read-only check invocation against a DB that has the table but hasn't run v22 yet degrades gracefully instead of erroring.
Greptile SummaryThe PR now distinguishes file-level type imports from symbol-level calls throughout live and persisted deleted-export checks.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; both previously reported discriminator issues are addressed in the current code. Important Files Changed
Reviews (3): Last reviewed commit: "fix: render legacy advisory consumers as..." | Re-trigger Greptile |
Codegraph Impact Analysis6 functions changed → 12 callers affected across 6 files
|
…sories Two issues Greptile caught in review of the initial #1973 fix: 1. Keying consumerKind off the source node's own kind (caller.kind === 'file') misclassifies a genuine top-level call as a type-only import: findCaller falls back to the file node as a call's source when there's no enclosing function/binding (a bare statement at module scope), so a real 'calls' edge can legitimately be sourced from a file-kind node too. The edge's own kind is the correct, unambiguous signal instead -- imports-type edges are always file-sourced by construction; calls edges are always genuine calls regardless of what happens to be at their source. Mirrored the fix in the native Rust advisory-recording path too (it runs its own independent query, not a shared function with the TS side). 2. The persisted deleted-export-advisories snapshot (#1938) discarded consumerKind entirely -- an advisory-derived consumer (the deleted file's nodes already purged before check ran) fell through to the old fabricated file:line rendering regardless of whether it was actually a type-only import. recordDeletedExportAdvisories already calls findExternalConsumers (which now computes consumerKind) at capture time, so persisting it through is a natural extension: added migration v22 (consumer_kind TEXT column, mirrored in both engines' migration lists) and threaded the field through record/ read on both the TS and native Rust sides. A read-only check invocation can still hit a DB that has the table (v21) but hasn't run v22 yet, so getDeletedExportAdvisories probes for the column the same way hasAdvisoryTable already probes for the table. Found the identical edge-vs-source-kind bug already shipped in exports.ts's own #1830 fix while investigating this -- filed separately as #2189 since that file isn't part of this diff. docs check acknowledged: internal check-predicate fix, no README/CLAUDE.md/ROADMAP surface change Impact: 5 functions changed, 9 affected
Greptile (round 2): a pre-existing deleted_export_advisories row persisted before this discriminator existed (or before migration v22 added the column) has consumerKind = undefined -- and its underlying nodes/edges are permanently gone by definition, since that's exactly why it fell back to the advisory snapshot. There's no way to retroactively re-derive which case it was, so defaulting undefined to the old file:line rendering would silently reintroduce the same "confidently wrong" fabricated-line risk for exactly the rows that can't be verified. Render that case explicitly as "(kind unknown -- pre-existing advisory)" instead of guessing. docs check acknowledged: internal check-predicate fix, no README/CLAUDE.md/ROADMAP surface change Impact: 2 functions changed, 3 affected
|
Addressed the round-2 finding: legacy advisory rows with |
1 participant
Summary
Verification
Closes #1973