Skip to content

Commit 364e649

Browse files
bntvllntbntvllnt
authored
fix: consistent error handling for impact_analysis + LOC off-by-one (#11)
## Summary Fixes #8 - `impact_analysis` now returns `isError: true` for nonexistent symbols (matching `symbol_context` and `file_context` behavior) - Fixed LOC off-by-one: trailing newline no longer double-counted - Added empty file guard (0 LOC instead of crash) ## Test plan - [x] Regression tests for all 8 ACs - [x] All existing tests pass - [x] Quality gates: lint, typecheck, build, test
1 parent a840ef6 commit 364e649

13 files changed

Lines changed: 439 additions & 5 deletions

‎specs/history.log‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
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
33
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.
4+
2026-03-11 | shipped | fix-error-handling | 1h→0.5h | 1d | Consistent impact_analysis error handling, LOC off-by-one fix, empty file guard. 17 regression tests.

‎specs/shipped/2026-03-11-fix-error-handling.md‎

Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,179 @@
1+
---
2+
title: "Fix inconsistent error handling + LOC off-by-one"
3+
status: active
4+
created: 2026-03-11
5+
estimate: 1h
6+
tier: mini
7+
reviewed: 2026-03-11
8+
---
9+
10+
# Fix inconsistent error handling + LOC off-by-one
11+
12+
**Issue**: [#8](https://github.com/bntvllnt/codebase-intelligence/issues/8)
13+
14+
## Codebase Impact
15+
16+
| Area | Impact | Detail |
17+
|------|--------|--------|
18+
| `src/impact/index.ts` | MODIFY | Add `notFound` flag to `ImpactResult` return value when symbol not found (line 71-72) |
19+
| `src/mcp/index.ts` | MODIFY | Check `impactAnalysis` result for `notFound`, return `isError: true` (line 616-624) |
20+
| `src/parser/index.ts` | MODIFY | Fix LOC fencepost error (line 160): use `getEnd() === 0 ? 0 : getLineAndCharacterOfPosition(getEnd() - 1).line + 1` |
21+
| `tests/mcp-tools.test.ts` | MODIFY | Update existing `impact_analysis` unknown symbol test (line 264-267) to expect `isError: true` instead of `totalAffected: 0` |
22+
| `tests/` | CREATE | Regression tests for both bugs + LOC edge cases |
23+
24+
**Files:** 0 create | 3 modify | 1 test create
25+
**Reuse:** Error pattern from `symbol_context` handler (`src/mcp/index.ts:443-448`) and `file_context` handler (`src/mcp/index.ts:74-78`)
26+
**Breaking changes:** none (MCP response shape change only affects error case, which previously returned misleading empty data)
27+
**New dependencies:** none
28+
29+
## User Journey
30+
31+
ACTOR: LLM calling MCP tools
32+
GOAL: Get consistent error responses for nonexistent entities
33+
PRECONDITION: Codebase graph is loaded
34+
35+
1. User calls `impact_analysis` with nonexistent symbol
36+
-> System detects symbol not in graph
37+
-> User sees `{"error": "Symbol not found: <name>"}` with `isError: true`
38+
39+
2. User calls `file_context` on a file
40+
-> System reports correct LOC (no off-by-one)
41+
-> User sees accurate line count matching `wc -l`
42+
43+
Error: User calls `impact_analysis` with valid symbol that has zero callers
44+
-> System returns `{"symbol": "...", "levels": [], "totalAffected": 0}` (not an error)
45+
46+
POSTCONDITION: All three entity-lookup tools (`file_context`, `symbol_context`, `impact_analysis`) return consistent `isError` responses for missing entities
47+
48+
## Acceptance Criteria
49+
50+
### Must Have (BLOCKING)
51+
52+
- [ ] AC-1: GIVEN a loaded graph WHEN `impact_analysis` is called with a nonexistent symbol THEN response contains `{"error": "Symbol not found: <name>"}` with `isError: true`
53+
- [ ] AC-2: GIVEN a loaded graph WHEN `impact_analysis` is called with a valid symbol that has zero callers THEN response contains `{"levels": [], "totalAffected": 0}` (not an error)
54+
- [ ] AC-3: GIVEN a file ending with a trailing newline WHEN `file_context` reports LOC THEN LOC matches `wc -l` (no off-by-one)
55+
- [ ] AC-4: GIVEN a file WITHOUT a trailing newline WHEN parsed THEN LOC equals number of content lines (fix must not regress no-newline case)
56+
- [ ] AC-5: GIVEN an empty file (0 bytes) WHEN parsed THEN LOC equals 0 (no crash from `getEnd() - 1`)
57+
- [ ] AC-6: GIVEN a single-line file (with or without trailing newline) WHEN parsed THEN LOC equals 1
58+
- [ ] AC-7: GIVEN a file with only comments WHEN parsed THEN LOC equals number of lines (comments count as LOC in this tool)
59+
60+
### Error Criteria (BLOCKING)
61+
62+
- [ ] AC-E1: GIVEN a loaded graph WHEN any entity-lookup tool receives a nonexistent entity THEN it returns `isError: true` with a descriptive message (consistency across `file_context`, `symbol_context`, `impact_analysis`)
63+
64+
## Scope
65+
66+
- [ ] 1. Add `notFound` flag to `ImpactResult` in `impactAnalysis` return value (`src/impact/index.ts:71-72`) -- return `{ symbol: symbolQuery, levels: [], totalAffected: 0, notFound: true }` when targetIds empty -> AC-1, AC-2
67+
- [ ] 2. Add error check in `impact_analysis` MCP handler (`src/mcp/index.ts:616-624`): if `result.notFound`, return `{ error: "Symbol not found: ${symbol}" }` with `isError: true`, matching `symbol_context` pattern -> AC-1, AC-E1
68+
- [ ] 3. Fix LOC fencepost in `parseFile` (`src/parser/index.ts:160`): replace `sourceFile.getLineAndCharacterOfPosition(sourceFile.getEnd()).line + 1` with `sourceFile.getEnd() === 0 ? 0 : sourceFile.getLineAndCharacterOfPosition(sourceFile.getEnd() - 1).line + 1` -> AC-3, AC-4, AC-5, AC-6, AC-7
69+
- [ ] 4. Update existing test `tests/mcp-tools.test.ts:264-267` ("returns empty levels for unknown symbol") to assert `isError: true` instead of `totalAffected: 0` -> AC-1
70+
- [ ] 5. Regression tests for error handling and LOC edge cases -> AC-1, AC-2, AC-3, AC-4, AC-5, AC-6, AC-7, AC-E1
71+
72+
### Out of Scope
73+
74+
- `rename_symbol` error handling for nonexistent symbols (see Analysis -- flagged for future work, separate issue)
75+
- Auditing other MCP tools for error consistency beyond `impact_analysis`
76+
- Changing the error response format (keep existing `{"error": "..."}` + `isError` pattern)
77+
78+
## Quality Checklist
79+
80+
### Blocking
81+
82+
- [ ] All ACs passing
83+
- [ ] No regressions in existing tests (note: existing `impact_analysis` unknown symbol test MUST be updated -- see scope item 4)
84+
- [ ] Error states handled
85+
- [ ] LOC fix doesn't break export-level LOC counting (`extractExports` uses `endLine - startLine + 1`, separate logic)
86+
- [ ] Empty file edge case guarded (`getEnd() === 0 ? 0 : ...`)
87+
88+
### Advisory
89+
90+
- [ ] Error message format matches existing tools exactly (`"Symbol not found: ${name}"`)
91+
- [ ] `rename_symbol` error handling inconsistency tracked for future fix
92+
93+
## Test Strategy
94+
95+
Runner: vitest | E2E: none needed (logic-level fix) | TDD: RED -> GREEN per AC
96+
97+
| AC | Test Type | Test Intention |
98+
|----|-----------|----------------|
99+
| AC-1 | Integration (MCP) | Call `impact_analysis` via MCP client with nonexistent symbol, assert `isError: true` and `error` contains "Symbol not found" |
100+
| AC-1 | Integration (function) | Call `impactAnalysis(graph, "nonexistent")` directly, assert `notFound: true` |
101+
| AC-2 | Integration (MCP) | Call `impact_analysis` with valid symbol with zero callers, assert `isError` is absent/falsy, `levels: []`, `totalAffected: 0` |
102+
| AC-3 | Integration (parser) | Create fixture file with trailing newline, parse, assert `loc` equals `wc -l` count |
103+
| AC-4 | Integration (parser) | Create fixture file WITHOUT trailing newline, parse, assert `loc` equals content line count |
104+
| AC-5 | Integration (parser) | Create empty `.ts` file (0 bytes), parse, assert `loc === 0` -- no crash |
105+
| AC-6 | Integration (parser) | Create single-line file with trailing newline, parse, assert `loc === 1`; same without trailing newline |
106+
| AC-7 | Integration (parser) | Create comments-only file, parse, assert `loc` equals line count |
107+
| AC-E1 | Integration (MCP) | Call `file_context`, `symbol_context`, `impact_analysis` each with nonexistent input, assert all three return `isError: true` with `{"error": "...not found..."}` shape |
108+
109+
Mocks: none (real parser, real graph, real analyzer per project rules)
110+
111+
### Existing Test Updates
112+
113+
The test at `tests/mcp-tools.test.ts:264-267` currently asserts:
114+
```typescript
115+
it("returns empty levels for unknown symbol", async () => {
116+
const r = await callTool("impact_analysis", { symbol: "nonexistent_xyz_123" });
117+
expect(r).toHaveProperty("totalAffected", 0);
118+
});
119+
```
120+
This MUST be updated to assert `isError: true` instead, since the behavior is intentionally changing. This is NOT a regression -- it's correcting the test to match the new correct behavior.
121+
122+
## Analysis
123+
124+
### Assumptions Challenged
125+
126+
| # | Assumption | Evidence For | Evidence Against | Verdict | Action |
127+
|---|------------|-------------|-----------------|---------|--------|
128+
| 1 | `sourceFile.getEnd()` points past the final newline, causing `+1` to double-count | TS API docs: `getEnd()` returns position after last character. Empirically verified: 3-line file with trailing newline gives `loc=4` (should be 3) | None | VALID | -> no action |
129+
| 2 | `impactAnalysis` is the only tool with inconsistent error handling | `file_context`, `symbol_context`, `get_dependents` all check and return `isError` | **WRONG**: `rename_symbol` MCP handler (lines 636-644) does NOT check for empty results. Calling with nonexistent symbol returns `{ references: [], totalReferences: 0 }` without `isError`. The spec originally claimed `rename_symbol` checks and returns `isError` -- this is false. | WRONG | -> flagged for future fix, kept out of this spec's scope to avoid scope creep |
130+
| 3 | Export-level LOC in `extractExports` (line 201: `endLine - startLine + 1`) is unaffected by the fix | Different calculation using span delta, not file-level `getEnd()` | None | VALID | -> no action |
131+
| 4 | The LOC fix `getEnd() - 1` handles all edge cases | Works for trailing newline (3 lines: 4->3), no trailing newline (3->3), single line (1->1) | **Crashes on empty files**: `getEnd() === 0`, so `getEnd() - 1 === -1` causes invalid position. Empirically verified. | RISKY | -> update scope to include empty file guard (done) |
132+
| 5 | No existing tests will break | AC-1 changes behavior for unknown symbol case | **WRONG**: Existing test at `mcp-tools.test.ts:264-267` asserts `totalAffected: 0` for unknown symbol. This WILL break. | WRONG | -> update spec to include test update (done, scope item 4) |
133+
134+
### Blind Spots
135+
136+
| # | Category | Blind Spot | Impact If Ignored | Suggested Spec Change |
137+
|---|----------|-----------|-------------------|----------------------|
138+
| 1 | [data] | Files without trailing newlines may report LOC differently after fix | False -- empirically verified fix is correct for both cases (3 lines -> 3 either way) | Added AC-4 to test explicitly |
139+
| 2 | [integration] | Downstream consumers caching old LOC values (persistence layer) may show stale data until re-analyzed | Low -- users re-analyze on demand, stale data is expected behavior | N/A |
140+
| 3 | [testing] | Empty file, single-line file, comments-only file LOC edge cases not tested | Parser crash on empty file; silent incorrect counts on edge cases | Added AC-5, AC-6, AC-7 with dedicated tests |
141+
| 4 | [consistency] | `rename_symbol` has same missing error handling as `impact_analysis` | LLM callers get empty result instead of clear error for nonexistent symbols in rename | Flagged for future issue, not in this spec's scope |
142+
| 5 | [testing] | Spec had no test for existing test update | Build would fail on unchanged test asserting old behavior | Added scope item 4 to update existing test |
143+
144+
### Failure Hypotheses
145+
146+
| # | IF | THEN | BECAUSE | Severity | Mitigation Status |
147+
|---|-----|------|---------|----------|-------------------|
148+
| 1 | LOC fix uses `getEnd() - 1` without guard | Empty file (0 bytes) crashes with invalid position | `getEnd() === 0`, so `getEnd() - 1 === -1` | HIGH | Added -- guard in scope item 3 |
149+
| 2 | Existing test not updated | CI fails with test regression | Test at line 264-267 asserts old behavior (`totalAffected: 0`) | HIGH | Added -- scope item 4 |
150+
| 3 | `impactAnalysis` returns `notFound` but MCP handler doesn't check it | Unknown symbol silently returns empty result (current broken behavior persists) | Implementation misses the handler-level check | MED | Explicit in scope items 1+2 |
151+
152+
### The Real Question
153+
154+
Confirmed -- spec solves the right problems. Both are clear bugs with deterministic fixes. The LOC fix was empirically verified across 8 edge cases (trailing newline, no trailing newline, empty file, single newline, single line with/without newline, comments-only, multi-line). The `rename_symbol` inconsistency is real but correctly scoped out to avoid creep.
155+
156+
### Open Items
157+
158+
- [risk] `rename_symbol` has same missing error handling as `impact_analysis` -> file separate issue for future fix
159+
- ~~[gap] Empty file edge case for LOC fix -> update scope item 3 to include guard~~ RESOLVED: guard added to scope item 3
160+
- ~~[gap] Existing test will break -> update scope~~ RESOLVED: scope item 4 added
161+
162+
## Notes
163+
164+
Spec review applied: 2026-03-11
165+
166+
### Review Findings Applied
167+
168+
1. **WRONG assumption corrected**: `rename_symbol` does NOT have error handling -- the original spec falsely claimed it did. Flagged for future work.
169+
2. **Existing test breakage identified**: `tests/mcp-tools.test.ts:264-267` must be updated (scope item 4 added).
170+
3. **LOC fix empirically verified**: `getEnd() === 0 ? 0 : getLineAndCharacterOfPosition(getEnd() - 1).line + 1` passes all 8 edge cases tested.
171+
4. **Edge case ACs added**: AC-4 (no trailing newline), AC-5 (empty file), AC-6 (single-line), AC-7 (comments-only).
172+
5. **Test strategy expanded**: Every AC now has a specific test intention with real pipeline, no mocks.
173+
174+
## Timeline
175+
176+
| Action | Timestamp | Duration | Notes |
177+
|--------|-----------|----------|-------|
178+
| plan | 2026-03-11 | - | Created |
179+
| spec-review | 2026-03-11 | - | Adversarial review: 2 wrong assumptions found, 5 blind spots, 3 failure hypotheses, 4 ACs added, 1 scope item added |

‎src/impact/index.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ interface ImpactResult {
1616
symbol: string;
1717
levels: ImpactLevel[];
1818
totalAffected: number;
19+
notFound?: boolean;
1920
}
2021

2122
interface RenameReference {
@@ -69,7 +70,7 @@ export function impactAnalysis(graph: CodebaseGraph, symbolQuery: string): Impac
6970
}
7071

7172
if (targetIds.length === 0) {
72-
return { symbol: symbolQuery, levels: [], totalAffected: 0 };
73+
return { symbol: symbolQuery, levels: [], totalAffected: 0, notFound: true };
7374
}
7475

7576
const reverseEdges = new Map<string, Array<{ source: string; confidence: CallConfidence }>>();

‎src/mcp/index.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -615,6 +615,12 @@ export function registerTools(server: McpServer, graph: CodebaseGraph): void {
615615
},
616616
async ({ symbol }) => {
617617
const result = impactAnalysis(graph, symbol);
618+
if (result.notFound) {
619+
return {
620+
content: [{ type: "text" as const, text: JSON.stringify({ error: `Symbol not found: ${symbol}` }) }],
621+
isError: true,
622+
};
623+
}
618624
return {
619625
content: [{
620626
type: "text" as const,

‎src/parser/index.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ function walkDir(dir: string, rootDir: string, results: string[], visited: Set<s
157157
function parseFile(sourceFile: ts.SourceFile, checker: ts.TypeChecker, rootDir: string, aliases: PathAlias[]): ParsedFile {
158158
const filePath = sourceFile.fileName;
159159
const relativePath = path.relative(rootDir, filePath);
160-
const loc = sourceFile.getLineAndCharacterOfPosition(sourceFile.getEnd()).line + 1;
160+
const loc = sourceFile.getEnd() === 0 ? 0 : sourceFile.getLineAndCharacterOfPosition(sourceFile.getEnd() - 1).line + 1;
161161
const exports = extractExports(sourceFile, checker);
162162
const imports = extractImports(sourceFile, aliases);
163163
const callSites = extractCallSites(sourceFile, checker, rootDir);

0 commit comments

Comments
 (0)