Skip to content

Commit a840ef6

Browse files
bntvllntbntvllnt
authored
fix: eliminate dead export false positives (#10)
## Summary Fixes #6 — `find_dead_exports` had ~33% false positive rate (2/6). Two root causes: - **Duplicate imports dropped**: second import to same target silently skipped due to graphology's single-edge constraint, losing symbols (e.g., `import type {SearchIndex}` after `import {createSearchIndex, ...}`) - **Same-file calls invisible**: parser excluded intra-file calls, analyzer only checked import edges — so `registerTools()` called within `startMcpServer()` was flagged dead ### Changes | File | Change | |------|--------| | `src/graph/index.ts` | Merge duplicate edge symbols into existing edge (both graphology attrs + `edges[]` array) | | `src/parser/index.ts` | Remove `declRelPath !== callerFile` guard — include same-file calls in `callSites` | | `src/analyzer/index.ts` | Integrate call graph edges into consumed symbols check + class method normalization (`ClassName.method` → `ClassName`) | | `src/analyzer/index.test.ts` | 7 regression tests | ### Semantics change - **Old**: dead = "no external file imports this symbol" - **New**: dead = "no import edges AND no call edges reference this symbol (including same-file)" ### Verification Self-analysis after fix: - `SearchIndex` — NOT flagged dead (was false positive) - `registerTools` — NOT flagged dead (was false positive) - `tokenize` — NOT flagged dead (same-file call by `createSearchIndex`) - `detectEntryPoints` — NOT flagged dead (same-file call by `traceProcesses`) ## Test plan - [x] 7 new regression tests (truly dead, type-only imports, duplicate merge, same-file calls, class methods, mixed dead/alive, edge merge sync) - [x] All 185 tests pass (178 existing + 7 new) - [x] Lint, typecheck, build all green - [x] Self-analysis: 0 false positives on known cases
1 parent ca4dd57 commit a840ef6

6 files changed

Lines changed: 430 additions & 2 deletions

File tree

‎specs/history.log‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,3 @@
11
2026-02-18 | shipped | 3d-code-mapper-v1 | 10h→2h | 1d | 3D codebase visualizer with 6 views, 6 MCP tools, 75 tests
22
2026-03-02 | shipped | mcp-parity-readme-sync | 3h→2h | 1d | 100% MCP-REST parity: +2 tools, enhanced 3 tools, 15 tool descriptions, README sync, 21 new tests
3+
2026-03-11 | shipped | fix-dead-export-false-positives | 2h→1.5h | 1d | Fix 33% false positive rate: merge duplicate imports, include same-file calls, call graph consumption. 8 regression tests.

‎specs/shipped/2026-03-10-fix-dead-export-false-positives.md‎

Lines changed: 225 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,225 @@
1+
# Fix: Dead Export Detection False Positives
2+
3+
**Issue**: [#6](https://github.com/bntvllnt/codebase-intelligence/issues/6)
4+
**Branch**: `fix/dead-export-false-positives`
5+
**Date**: 2026-03-10
6+
**Spec review**: Applied 2026-03-10 (7 items from 4-perspective adversarial review)
7+
8+
## Problem
9+
10+
`find_dead_exports` has ~33% false positive rate (2/6). Two bugs:
11+
12+
1. **Duplicate imports dropped** — second import to same target silently skipped, losing symbols
13+
2. **Same-file calls invisible** — parser skips intra-file calls, analyzer only checks import edges
14+
15+
### False Positives
16+
17+
| Export | File | Why Not Dead |
18+
|--------|------|-------------|
19+
| `SearchIndex` | `search/index.ts` | `import type` in `mcp/index.ts:8` |
20+
| `registerTools` | `mcp/index.ts` | Called at line 812 within `startMcpServer()` |
21+
22+
### True Positives (4)
23+
24+
`tokenize`, `setGraph`, `getGraph`, `detectEntryPoints`
25+
26+
## Semantics Change
27+
28+
**Old definition**: dead = "no external file imports this symbol"
29+
**New definition**: dead = "no import edges AND no call edges reference this symbol (including same-file)"
30+
31+
This means exports like `tokenize` (used only within `search/index.ts`) will no longer be flagged dead. This is intentional — if an export is called anywhere, removing it requires code changes, so it's not safe to delete.
32+
33+
## Root Cause (Confirmed)
34+
35+
```
36+
BUG 1: Duplicate edge dropped BUG 2: Same-file calls skipped
37+
──────────────────────────────── ──────────────────────────────
38+
mcp/index.ts: mcp/index.ts:
39+
L7: import {A,B,C} from search ──▶ edge created, symbols:[A,B,C]
40+
L8: import type {X} from search ──▶ SKIPPED (edge exists) parser/index.ts:345
41+
X never enters consumed declRelPath !== callerFile → skip
42+
same-file calls never recorded
43+
44+
graph/index.ts:65 analyzer/index.ts:36-40
45+
if (!graph.hasEdge(src, tgt)) consumedSymbols from edges only
46+
→ only FIRST edge per pair → no call graph integration
47+
```
48+
49+
## Implementation
50+
51+
### Task 1: Merge duplicate edge symbols (Graph)
52+
53+
**File**: `src/graph/index.ts:59-81`
54+
55+
When edge already exists for source→target, merge new symbols into existing edge instead of skipping. **Must update BOTH the graphology edge attributes AND the `edges[]` array atomically** — dead export detection reads `edges[]`, PageRank reads graphology.
56+
57+
```
58+
Current (line 65):
59+
if (!graph.hasEdge(src, target)) { addEdge(...) }
60+
61+
Fix:
62+
if (!graph.hasEdge(src, target)) {
63+
addEdge(...)
64+
} else {
65+
// Find existing edge entry in edges[]
66+
const existing = edges.find(e => e.source === src && e.target === target)
67+
// Merge symbols (union, no duplicates)
68+
const merged = [...new Set([...existing.symbols, ...imp.symbols])]
69+
existing.symbols = merged
70+
existing.weight = merged.length || 1
71+
// isTypeOnly: false if EITHER import is value (not type-only)
72+
existing.isTypeOnly = existing.isTypeOnly && imp.isTypeOnly
73+
// Update graphology edge attributes to match
74+
graph.setEdgeAttribute(src, target, 'symbols', merged)
75+
graph.setEdgeAttribute(src, target, 'weight', merged.length || 1)
76+
graph.setEdgeAttribute(src, target, 'isTypeOnly', existing.isTypeOnly)
77+
}
78+
```
79+
80+
**Merge rules:**
81+
- `symbols`: union (deduplicated)
82+
- `weight`: `mergedSymbols.length || 1`
83+
- `isTypeOnly`: `existing && new` (only true if BOTH imports are type-only)
84+
85+
**Side effect**: PageRank scores shift slightly for files with previously-dropped duplicate imports. Weight increases → hub files get marginally higher PageRank. Acceptable — more accurate than before.
86+
87+
Fixes: `SearchIndex` false positive.
88+
89+
### Task 2: Include same-file calls in parser (Parser)
90+
91+
**File**: `src/parser/index.ts:345`
92+
93+
Remove the `declRelPath !== callerFile` guard so same-file calls are recorded in `callSites`.
94+
95+
```
96+
Current (line 345):
97+
if (declRelPath !== callerFile && !declRelPath.startsWith("..") && ...)
98+
99+
Fix:
100+
if (!declRelPath.startsWith("..") && !path.isAbsolute(declRelPath))
101+
```
102+
103+
**Side effects** (all beneficial or neutral):
104+
- `symbol_context` — shows intra-file callers/callees (more complete)
105+
- `impact_analysis` — blast radius includes same-file dependents (more accurate but noisier)
106+
- `get_processes` — `detectEntryPoints` may return fewer results (internal helpers gain inbound edges, losing "entry point" status). Verify existing tests still pass.
107+
- `callEdges` / `symbolNodes` arrays grow. Negligible perf impact for typical codebases.
108+
109+
### Task 3: Use call graph for dead export detection (Analyzer) — BLOCKED BY Task 2
110+
111+
**File**: `src/analyzer/index.ts:36-41`
112+
113+
After building `consumedSymbols` from import edges, also add symbols consumed via call graph edges. `callEdges` are already available in `built: BuiltGraph` (confirmed: `BuiltGraph.callEdges` at `graph/index.ts:10`).
114+
115+
```typescript
116+
// After existing consumedSymbols loop (line 41):
117+
// Also count symbols consumed via call graph (includes same-file calls from Task 2)
118+
for (const callEdge of built.callEdges) {
119+
// Extract file path from callEdge.target ("file::symbol" format)
120+
const sepIdx = callEdge.target.indexOf("::");
121+
if (sepIdx === -1) continue;
122+
const targetFile = callEdge.target.substring(0, sepIdx);
123+
124+
// Normalize: class method "AuthService.validate" → class name "AuthService"
125+
const rawSymbol = callEdge.calleeSymbol;
126+
const consumedName = rawSymbol.includes(".") ? rawSymbol.split(".")[0] : rawSymbol;
127+
128+
const existing = consumedSymbols.get(targetFile) ?? new Set<string>();
129+
existing.add(consumedName);
130+
consumedSymbols.set(targetFile, existing);
131+
}
132+
```
133+
134+
**Critical: class method normalization.** `calleeSymbol` for method calls is `"ClassName.methodName"` but exports only contain `"ClassName"`. Must strip method suffix or class exports remain false positives.
135+
136+
Fixes: `registerTools` false positive.
137+
138+
### Task 4: Regression tests
139+
140+
**File**: `src/analyzer/index.test.ts`
141+
142+
Real fixture files through real pipeline (no mocks):
143+
144+
| Test | Fixture | Assert |
145+
|------|---------|--------|
146+
| Type-only import consumed | A: `import type { X } from "./b"`, B: exports `X` | `X` NOT in deadExports |
147+
| Duplicate import merged | A: `import { Y }` + `import type { Z }` from B | both `Y`, `Z` NOT dead |
148+
| Same-file call consumed | A: exports `foo`, `bar`; `bar` calls `foo` | `foo` NOT dead |
149+
| Class method consumed | A: `new B().method()`, B: exports class `B` | `B` NOT dead |
150+
| Truly dead export | A: exports `baz`, nobody imports or calls it | `baz` IS dead |
151+
| Mixed dead/alive | File with some consumed, some dead exports | only dead ones reported |
152+
| Edge merge sync | After merge, graphology attrs === edges[] entry | symbols, weight, isTypeOnly match |
153+
154+
**Fixture dir**: `tests/fixtures/dead-exports/`
155+
156+
### Task 5: Self-analysis verification
157+
158+
Run `find_dead_exports` against this repo after fix:
159+
- Expected: `registerTools` and `SearchIndex` NOT flagged
160+
- Expected: `setGraph`, `getGraph` still flagged (truly dead)
161+
- `tokenize` and `detectEntryPoints` may no longer be dead (if called same-file) — correct per new semantics
162+
163+
## Expected True Dead Exports After Fix
164+
165+
| Export | File | Status |
166+
|--------|------|--------|
167+
| `setGraph` | `server/graph-store.ts` | DEAD (exported, never imported or called) |
168+
| `getGraph` | `server/graph-store.ts` | DEAD (exported, never imported or called) |
169+
| `tokenize` | `search/index.ts` | Likely NOT dead (called same-file by `createSearchIndex`) |
170+
| `detectEntryPoints` | `process/index.ts` | Likely NOT dead (called same-file by `traceProcesses`) |
171+
172+
## State Machine
173+
174+
N/A — stateless computation fix.
175+
176+
## Files to Change
177+
178+
| File | Change | Lines |
179+
|------|--------|-------|
180+
| `src/graph/index.ts` | Merge duplicate edge symbols (both stores) | ~65-81 |
181+
| `src/parser/index.ts` | Include same-file calls | ~345 |
182+
| `src/analyzer/index.ts` | Add call graph to consumed check + class normalization | ~36-41 |
183+
| `src/analyzer/index.test.ts` | Dead export regression tests | new |
184+
| `tests/fixtures/dead-exports/` | Fixture .ts files | new |
185+
186+
## Quality Gates
187+
188+
- [ ] Lint (changed files)
189+
- [ ] Typecheck (full project)
190+
- [ ] Build
191+
- [ ] Tests (all + new regression)
192+
- [ ] Self-analysis: 0 false positives on known cases
193+
194+
## Risks
195+
196+
| Risk | Mitigation |
197+
|------|-----------|
198+
| Dual store desync (graphology vs edges[]) | Task 4 regression test asserts both match after merge |
199+
| Class method `calleeSymbol` doesn't match export name | Task 3 normalizes: strip `.method` suffix |
200+
| `isTypeOnly` collision on merge | Merge rule: `false` if either import is value |
201+
| PageRank shift from weight changes | Acceptable — more accurate. Existing tests may need threshold adjustment |
202+
| `get_processes` returns fewer entry points | Verify existing tests. Internal helpers losing entry status is correct |
203+
| Same-file calls inflate `impact_analysis` blast radius | Accept — more accurate. Document in tool description if noisy |
204+
205+
## Known Gaps (Out of Scope)
206+
207+
These false positive categories are NOT addressed by this fix:
208+
209+
| Gap | Description | Tracking |
210+
|-----|------------|---------|
211+
| Barrel `export *` | `symbols: ['*']` never matches concrete export names | File as separate issue |
212+
| Interface dispatch | Polymorphic calls resolve to interface, not implementation | Inherent TS limitation |
213+
| Dynamic calls | `obj[method]()`, HOF parameters — unresolvable statically | Accept |
214+
| Destructured requires | `const { foo } = require(...)` — not tracked | Rare in ESM codebases |
215+
216+
## Task Dependencies
217+
218+
```
219+
Task 1 (graph merge) ──────────┐
220+
├──▶ Task 3 (analyzer) ──▶ Task 4 (tests) ──▶ Task 5 (verify)
221+
Task 2 (parser same-file) ─────┘
222+
BLOCKS Task 3
223+
```
224+
225+
Tasks 1 and 2 are independent. Task 3 BLOCKS on both (hard dependency). Task 4 validates all.

0 commit comments

Comments
 (0)