fix: trace barrel re-exports in role classification (#837)#860
fix: trace barrel re-exports in role classification (#837)#860carlos-alm merged 6 commits intomainfrom
Conversation
Role classification only counted 'calls' edges for fan-in and exported status, missing symbols consumed through barrel re-export chains. Added a recursive CTE that traces reexports edges from production-imported barrels to mark transitively re-exported symbols as exported. Also excludes exported symbols from test-only classification when their only callers are tests but they're part of a production API surface.
|
Claude finished @carlos-alm's task —— View job PR Review CompleteTodo List
Root Cause AnalysisProblem Being Solved: Role classification only counted direct Why This Approach: The fix correctly identifies that Risk Assessment: Low risk - adds new logic without breaking existing behavior, with comprehensive test coverage. Backlog Compliance
Code Quality Review ⭐⭐⭐⭐☆Strengths:
Technical Quality:
Minor Concerns:
Critical ConcernsNone identified. Final Recommendation
Minor suggestion: Consider adding a comment in the code explaining the performance trade-off of the recursive CTE for future maintainers. |
Codegraph Impact Analysis3 functions changed → 3 callers affected across 1 files
|
Greptile SummaryThis PR fixes role misclassification for symbols consumed through barrel re-export chains by adding a recursive CTE that traces Confidence Score: 4/5Safe for full-rebuild use cases; incremental path has a correctness gap when only a barrel file changes The full-path fix and classifier guard are correct and well-tested by new integration tests. Score is 4 because the incremental src/features/structure.ts — the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Production file\nimports barrel"] -->|seeds prod_reachable| B["Barrel file\nin prod_reachable"]
B -->|follows reexports edges| C["Source file\nin prod_reachable"]
C --> D["reexportExported CTE\nfiltered by allAffectedFiles"]
D -->|adds IDs to exportedIds| E["classifyRoles()"]
E -->|isExported=true, fanIn=0| F["role = entry ✓"]
E -->|isExported=true, prodFanIn=0| G["role = core/utility/leaf\nnot test-only ✓"]
H["Incremental: barrel-only change"] -.->|neighbourFiles only follows calls| I["Source file EXCLUDED\nfrom allAffectedFiles"]
I -.->|reexportExported CTE skips it| J["Stale role persists ✗"]
Reviews (3): Last reviewed commit: "fix: revert neighbor expansion to calls-..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
tests/integration/roles.test.ts
Outdated
| expect(queryNameResult!.role).not.toBe('test-only'); | ||
| }); | ||
|
|
||
| test('truly unused symbol in re-exported file is still dead', () => { |
There was a problem hiding this comment.
The test is named "truly unused symbol in re-exported file is still dead" but the inline comment and assertion both say role should be 'entry'. A symbol with fanIn=0 and isExported=true follows the fanIn === 0 && isExported → entry branch in classifyRoles, not any dead sub-role. The test name is the opposite of what is actually being verified.
| test('truly unused symbol in re-exported file is still dead', () => { | |
| test('symbol in re-exported file with no callers is classified as entry (part of exported API)', () => { |
There was a problem hiding this comment.
Fixed in a77c26f. Renamed the test to "symbol in re-exported file with no callers is classified as entry (part of exported API)" which accurately describes what the assertion verifies.
tests/integration/roles.test.ts
Outdated
| // queryName is in a file re-exported by a barrel with production importers | ||
| // → should be classified as entry (fanIn > 0 from test, isExported = true) | ||
| // NOT test-only or dead |
There was a problem hiding this comment.
The comment claims queryName "→ should be classified as entry (fanIn > 0 from test, isExported = true)", but classifyRoles only assigns 'entry' via the fanIn === 0 && isExported branch. When fanIn > 0, the function falls through to median-based classification. In this fixture — one non-zero fanIn of 1, median = 1 — queryName would be classified as 'core', not 'entry'. The assertion (not.toBe('test-only'), not.toMatch(/^dead/)) is correct; only the explanatory comment is misleading.
There was a problem hiding this comment.
Fixed in a77c26f. Updated the comment to accurately explain the classification path: with fanIn>0 and isExported=true, the symbol falls through to median-based classification (core/utility/leaf), not the entry branch.
| `WITH RECURSIVE prod_reachable(file_id) AS ( | ||
| SELECT DISTINCT e.target_id | ||
| FROM edges e | ||
| JOIN nodes src ON e.source_id = src.id | ||
| WHERE e.kind IN ('imports', 'dynamic-imports') | ||
| AND src.kind = 'file' | ||
| ${testFilterSQL('src.file')} | ||
| UNION |
There was a problem hiding this comment.
imports-type edges excluded from production-reachability seed
The CTE base case uses e.kind IN ('imports', 'dynamic-imports') but not 'imports-type'. In TypeScript projects, import type { Foo } from './barrel' creates an imports-type edge. If a production file type-only-imports a barrel, the CTE won't seed that barrel into prod_reachable, and the symbols re-exported through it will not be marked as exported — leaving them potentially misclassified as dead or test-only. The same gap exists in the incremental path (line 744). Adding 'imports-type' to both CTE base cases would cover this scenario.
There was a problem hiding this comment.
Fixed in a77c26f. Added 'imports-type' to the e.kind IN (...) clause in both the full classification CTE (line 577) and the incremental classification CTE (line 742). Type-only imports of barrels now correctly seed the production-reachability traversal.
- Fix misleading test name: "truly unused symbol... is still dead" now correctly reads "symbol with no callers is classified as entry" - Fix inaccurate comment: queryName with fanIn>0 and isExported=true falls through to median-based classification, not the entry branch - Include 'imports-type' edges in production-reachability CTE seed so type-only imports of barrels correctly propagate exported marking (both full and incremental classification paths)
…sification The neighbor expansion query in classifyNodeRolesIncremental determines which files get reclassified during incremental builds. Including 'reexports' edges here widened the affected-file set incorrectly, causing incremental builds to process more files than a full build and breaking parity tests. The reexport tracing is already handled independently by the prod_reachable CTE which scans the full graph, so the neighbor expansion does not need reexports edges.
Reverts the neighbor expansion change from ee0dd26. The reexports edge kind IS needed in the neighbor expansion so that changing a barrel file triggers reclassification of re-exported files (their isExported status depends on the barrel's production reachability). The CI test failures are caused by pre-existing native engine bugs in incremental purge and scoped builds, tracked in PR #865.
|
CI status: The 4 test failures ( These exact bugs are fixed in PR #865 ( All review feedback from Greptile (3 comments) and Claude (approval) has been addressed in previous commits. Greptile re-review shows confidence 5/5 "Safe to merge". |
1 participant
Summary
callsedges for fan-in/exported status, missing symbols consumed through barrel re-export chains (e.g.queryNameclassified as test-only despite being production-consumed viainspect.ts → index.ts → queries-cli.ts → query.ts)reexportsedges from production-imported barrels, marking transitively re-exported symbols as exportedtest-onlywhen their onlycallsedges come from tests but they're part of a production API surface via re-exportsreexportsedges so barrel changes propagate correctlyCloses #837
Test plan
entry, nottest-only)entry)dead