fix(impact): return structured error + partial results instead of crashing (#321)

[ShunsukeHayashi]

Problem

The impact command crashes with SIGSEGV (exit 139) on certain repositories (#321). When the underlying graph query fails (e.g., due to corrupted index, large traversal, or native binding issues), the entire Node.js process terminates instead of returning a useful error.

This makes impact unreliable in production workflows — both for CLI users and for MCP-connected agents that depend on gitnexus_impact() returning structured results.

Root Cause

1. impactCommand (CLI) — No try-catch around backend.callTool('impact', ...). Any thrown error kills the process.
2. impact() (LocalBackend) — The depth traversal catch block (catch (e) { logQueryError(...) }) silently swallows errors and continues, which means:
   • Partial results from successful depths are mixed with incomplete data
   • The caller has no way to know the analysis was interrupted
   • If the error is severe enough (native crash), the entire process dies anyway

Changes

1. Structured error boundary in impact() → _impactImpl()

• Wrapped the entire impact logic in a try-catch that returns structured error JSON:

  {
    "error": "Impact analysis failed",
    "target": "symbolName",
    "direction": "upstream",
    "impactedCount": 0,
    "risk": "UNKNOWN",
    "partial": true,
    "suggestion": "Try gitnexus context <symbol> as a fallback"
  }

• This prevents process crashes from unhandled exceptions in the impact pipeline.

2. Partial results on depth traversal failure

• When a Cypher query fails mid-traversal, we now break out of the depth loop instead of silently continuing.
• Results collected from successful depths are still returned.
• A partial: true flag is added to the response so callers know the analysis was interrupted.

3. CLI error handling

• impactCommand now catches errors and outputs structured JSON instead of crashing.
• Exit code 1 is used (instead of SIGSEGV/139).

4. Formatter improvements

• formatImpactResult now displays:
  • Suggestion text when provided in error responses
  • ⚠️ Partial results warning when traversal was interrupted

Files changed

File	Change
src/mcp/local/local-backend.ts	Extract _impactImpl(), add error boundary, partial flag, break on traversal error
src/cli/tool.ts	Add try-catch in impactCommand with structured error output
src/cli/eval-server.ts	Improve formatImpactResult for suggestion + partial warning
test/unit/eval-formatters.test.ts	3 new tests: error+suggestion, partial results, existing test adjusted

Testing

• 3 new unit tests added
• Existing formatImpactResult tests updated for compatibility
• Backward compatible — successful impact calls return the same structure as before

Context

We've been running GitNexus across 25+ repositories in production and built gitnexus-stable-ops as an operational toolkit. The gitnexus-safe-impact.sh script in that toolkit was our workaround for this issue — this PR brings the fix into the core.

Related: #344

[vercel]

@ShunsukeHayashi is attempting to deploy a commit to the NexusCore Team on Vercel.

A member of the Team first needs to authorize it.

[magyargergo]

What's the most common error you tend to see from exceptions?

[ShunsukeHayashi]

From running impact across 25 repos in production, the most common exceptions:

1. Symbol not found (~40%) — user typos or stale index. The new code returns:

   {"error": "Symbol 'FooBar' not found", "suggestion": "Try: gitnexus query 'FooBar' to search"}

2. SIGSEGV / native crash (~30%) — the one this PR is primarily about. LadybugDB's native layer segfaults during complex depth traversals (especially depth 3+ with many edges). Before this PR: process dies silently. After: structured error JSON + partial results if any depth levels completed.

3. Query timeout (~15%) — large repos with 5000+ symbols. The existing withTimeout() handles this, but the error wasn't surfaced cleanly to CLI users.

4. Empty graph / no edges (~10%) — freshly analyzed repos where edge extraction failed (e.g., Python repos produce 0 CALLS edges — call extraction works but resolveCallTarget fails #337 Python CALLS). Returns impactedCount: 0 with a meaningful message.

5. Concurrent query crash (~5%) — addressed separately in PR fix(impact): serialize enrichment queries to prevent concurrent query crash (#316) #347.

The _impactImpl() wrapper catches all of these and returns structured JSON instead of crashing, so downstream tools (MCP, CLI, eval-server) always get a parseable response.

[magyargergo]

@claude ultra-think while you are reviewing this pr and you are tasked to find bugs, code quality issues, architectural fit and performance issues

[claude]

Claude finished @magyargergo's task in 2m 43s —— View job

---

PR Review: fix(impact) — structured error + partial results

Tasks

• Gather context & read changed files
• Review local-backend.ts — core logic changes
• Review tool.ts — CLI error handling
• Review eval-server.ts — formatter improvements
• Review eval-formatters.test.ts — new test coverage
• Synthesize findings: bugs, code quality, architecture, performance

---

Summary

The intent is sound — turning crashes into structured errors is correct. The _impactImpl() extraction is clean and the traversalComplete + break pattern is the right fix for the depth loop. However there are several bugs and design issues that should be addressed before merging.

---

Bugs

1. Inconsistent target field shape in error responses

local-backend.ts:1339 — When the outer impact() catch fires, target is set to params.target — a raw string. But in successful responses, target is an object {id, name, type, filePath}. MCP clients and any code that does result.target.name will throw a TypeError on error responses.

// ERROR response — target is a string
{ error: "...", target: "MySymbol", ... }

// SUCCESS response — target is an object
{ target: { id: "...", name: "MySymbol", type: "Function", filePath: "..." }, ... }

The formatter handles it safely because it returns early on result.error, but downstream API consumers will see two incompatible shapes for the same field. Fix this →

---

2. partial: true is set on total-failure error responses — misleading and invisible

local-backend.ts:1341-1343 — The outer impact() catch returns partial: true with impactedCount: 0. partial was designed to mean "we got some results but the traversal was cut short." A complete failure with zero results is not a partial result.

More concretely: this partial: true is also invisible in the formatter. formatImpactResult checks result.error first and returns the error string early — the partial warning branch at eval-server.ts:162 is never reached. The flag exists in the JSON payload but is never shown to users, which wastes space and confuses API consumers who may branch on it.

// outer catch — this partial flag is dead in the formatter
return {
  error: err?.message || 'Impact analysis failed',
  partial: true,  // <-- never displayed; misleading for zero-result failures
  ...
};

Fix this →

---

3. getBackend() is outside the try-catch in impactCommand

tool.ts:95 — getBackend() can throw (e.g., config not found, backend init failure) but it's called one line before the try block, so it's still uncaught and will crash the CLI. The PR partially fixes crash handling but misses this call.

const backend = await getBackend();  // <-- uncaught
try {
  const result = await backend.callTool('impact', { ... });

Fix this →

---

4. err?.message is unsafe when the thrown value is a string

local-backend.ts:1338 and tool.ts:108 — Some libraries and older code throw "some string" rather than throw new Error("..."). When err is a string, err?.message is undefined, so the fallback fires and swallows the original message. The pattern err?.message || 'fallback' should be (err instanceof Error ? err.message : String(err)) || 'fallback'.

Fix this →

---

Code Quality

5. Duplicate parameter interface between impact() and _impactImpl()

local-backend.ts:1325-1331 and 1349-1355 — Both methods declare identical inline parameter objects. Since impact() is just an error wrapper that delegates to _impactImpl(), this duplication is unnecessary. Extract a shared ImpactParams interface.

// Both functions currently repeat this verbatim:
params: {
  target: string;
  direction: 'upstream' | 'downstream';
  maxDepth?: number;
  relationTypes?: string[];
  includeTests?: boolean;
  minConfidence?: number;
}

6. Error response schema inconsistency between the two catch layers

The outer impact() catch returns { error, target, direction, impactedCount: 0, risk: 'UNKNOWN', partial: true, suggestion }. The CLI impactCommand catch returns { error, target, direction, suggestion } — missing impactedCount, risk, partial. Callers expecting a consistent shape will need special-casing for each path.

---

Architecture

7. Two error boundaries doing overlapping work

Since impact() now catches all exceptions and returns structured JSON (never throws), the catch block in impactCommand is effectively dead for the impact case. This creates a confusing dual-layer that's hard to reason about. The PR should either:

• Document clearly that the CLI catch is belt-and-suspenders for infrastructure failures (not impact logic failures), or
• Move the error handling fully into the backend and remove the CLI catch

Currently neither layer has a comment explaining the intended separation.

---

Tests

8. Weakened existing test without justification

eval-formatters.test.ts:150 — The original test used toBe (strict equality); this PR changes it to toContain without explanation:

// Before (strict):
expect(formatImpactResult({ error: 'bad request' })).toBe('Error: bad request');

// After (relaxed):
expect(formatImpactResult({ error: 'bad request' })).toContain('Error: bad request');

toContain is appropriate when extra content is expected, but the comment says nothing about why the old exact match broke. If the behavior changed (e.g., a newline was added), the test should explain what the new output looks like.

9. No test for the case where impactedCount: 0 + partial: true + no error

The partial warning test (eval-formatters.test.ts:162) only covers impactedCount > 0. There's no coverage for what the formatter does when partial: true but impactedCount === 0 (the formatter hits the zero-impact early return at line 155 and never displays the partial warning).

---

Minor

• parseInt(options.depth) at tool.ts:100 is missing the radix argument — should be parseInt(options.depth, 10) (pre-existing, but touched by this diff).
• The ⚠️ emoji in the formatter output (eval-server.ts:163) will render differently in terminals depending on locale/font support. Consider falling back to [!] or WARNING: for environments where Unicode isn't guaranteed.

---

Verdict

The core fix — traversalComplete flag + break on depth failure — is correct and the test coverage for partial results is good. The extraction to _impactImpl() is clean. The issues above (especially #1 inconsistent target shape, #3 uncaught getBackend(), and #2 misleading partial flag on failures) are worth fixing before merge.

[magyargergo]

Could you please look at this report? 🙏

[ShunsukeHayashi]

Addressed all 4 bugs from @claude's review:

Fixed:

• 🔴 Inconsistent target shape — error responses now return {name: string} instead of raw string, matching success schema
• 🔴 Misleading partial: true — removed from total-failure responses (only meaningful when some depth levels succeeded)
• 🔴 getBackend() outside try-catch — moved inside so backend init failures return structured JSON
• 🔴 err?.message on thrown strings — now uses instanceof Error check with String(err) fallback
• 🟢 parseInt radix — added 10 argument

Acknowledged (scope):

• Duplicate parameter interface — agree, but refactoring to a shared ImpactParams type touches other callers; better as a follow-up
• Two error boundaries — the CLI catch is now documented as belt-and-suspenders for infrastructure failures (transport, backend init), not graph logic
• Weakened test assertion — toContain is correct since the formatter now adds suggestion text after the error message

[magyargergo]

@claude recommend integration test for this PR

[claude]

Claude finished @magyargergo's task in 2m 14s —— View job

---

Tasks

• Gather context
• Read changed files
• Identify integration test gaps
• Write integration test recommendations

---

Integration Test Recommendations

The existing unit tests in eval-formatters.test.ts cover the formatter layer well. The integration gap is in the callTool dispatch layer and the CLI e2e layer — neither tests the new error boundary paths introduced by this PR. Here are 6 targeted integration tests.

---

1. callTool('impact', …) with nonexistent symbol returns structured error (not a throw)

File: test/integration/local-backend-calltool.test.ts — add to "tool parameter edge cases" block

This is the most critical gap. The existing impact tool returns upstream dependents test only covers the happy path. There's no integration test verifying that a missing symbol returns {error: ..., target: {name: ...}} rather than throwing.

it('impact tool returns structured error for nonexistent symbol', async () => {
  const result = await backend.callTool('impact', {
    target: 'nonexistent_xyz_symbol_999',
    direction: 'upstream',
  });
  expect(result).toHaveProperty('error');
  expect(result.error).toMatch(/not found/i);
  // After the #321 fix: target must be an object, not a raw string
  expect(result.target).toHaveProperty('name');
  expect(result.target.name).toBe('nonexistent_xyz_symbol_999');
  expect(result.impactedCount).toBe(0);
  expect(result.risk).toBe('UNKNOWN');
});

---

2. callTool('impact', …) outer error boundary — no partial: true on total failure

File: test/integration/local-backend-calltool.test.ts — add to "tool parameter edge cases" block

Verifies the bug fix from review comment #2: that partial is absent (or false) when the call fails completely (zero results), distinguishing it from a mid-traversal interruption.

it('impact tool error response does not set partial:true on total failure', async () => {
  const result = await backend.callTool('impact', {
    target: 'nonexistent_xyz_symbol_999',
    direction: 'upstream',
  });
  expect(result).toHaveProperty('error');
  // partial:true is only meaningful when some depth levels succeeded
  expect(result.partial).toBeFalsy();
});

---

3. callTool('impact', …) partial results when depth traversal fails mid-way

File: test/integration/local-backend-calltool.test.ts — add as a separate describe block since it requires mocking

This is the core logic introduced by this PR (traversalComplete = false; break;) and has zero integration test coverage — only a unit formatter test. The mock below forces a failure on the depth-2 query while letting depth-1 complete.

import * as lbugAdapter from '../../src/mcp/core/lbug-adapter.js';

describe('impact partial results on traversal failure', () => {
  let backend: LocalBackend;

  beforeAll(async () => {
    const ext = handle as any;
    backend = ext._backend;
  });

  afterEach(() => {
    vi.restoreAllMocks();
  });

  it('returns partial:true and depth-1 results when depth-2 query throws', async () => {
    let callCount = 0;
    const original = lbugAdapter.executeQuery;
    vi.spyOn(lbugAdapter, 'executeQuery').mockImplementation(async (...args) => {
      callCount++;
      // Let depth-1 traversal through; fail on the second executeQuery call
      if (callCount >= 2) throw new Error('Simulated native crash at depth 2');
      return original(...args as Parameters<typeof original>);
    });

    const result = await backend.callTool('impact', {
      target: 'validate',
      direction: 'upstream',
      maxDepth: 3,
    });

    // Should NOT have an error field — partial results are still returned
    expect(result).not.toHaveProperty('error');
    // partial flag must be set so callers know the analysis was cut short
    expect(result.partial).toBe(true);
    // impactedCount reflects only the depths that completed
    expect(result.impactedCount).toBeGreaterThanOrEqual(0);
    // byDepth should exist even if sparse
    expect(result.byDepth).toBeDefined();
  });
});

---

4. CLI impact command — nonexistent symbol exits cleanly (not SIGSEGV)

File: test/integration/cli-e2e.test.ts — add to describe('CLI error handling')

The entire motivation for this PR was exit code 139 (SIGSEGV). This e2e test verifies the CLI never exits with that code, and that stdout is parseable JSON even on failure. Requires the mini-repo to have been analyzed first (run after the analyze test).

it('impact command on nonexistent symbol exits with code 1, not 139', () => {
  const result = runCliRaw(['impact', 'nonexistent_xyz_999'], MINI_REPO);
  if (result.status === null) return; // CI timeout

  // SIGSEGV is 139 — this is the primary regression guard for #321
  expect(result.status).not.toBe(139);
  expect(result.status).toBe(1);

  // stdout should be parseable structured JSON, not a crash dump
  let parsed: any;
  expect(() => { parsed = JSON.parse(result.stdout); }).not.toThrow();
  expect(parsed).toHaveProperty('error');
  expect(parsed.target).toHaveProperty('name');
  expect(parsed.suggestion).toBeTruthy();
});

---

5. CLI impact command — valid symbol returns parseable JSON with correct shape

File: test/integration/cli-e2e.test.ts

Complements test #4 by confirming the happy path after analyze runs. Ensures the structured response shape is stable end-to-end, not just in unit tests.

it('impact command on valid symbol returns structured JSON with byDepth', () => {
  // Depends on 'analyze command runs pipeline on mini-repo' having run first
  const result = runCliRaw(['impact', 'someExportedFunction'], MINI_REPO);
  if (result.status === null) return;

  // If the symbol doesn't exist in mini-repo, verify structured error — not a crash
  expect(result.status).not.toBe(139);

  let parsed: any;
  expect(() => { parsed = JSON.parse(result.stdout); }).not.toThrow();

  if (parsed.error) {
    // nonexistent — still structured
    expect(parsed.target).toHaveProperty('name');
  } else {
    // found — must have full result shape
    expect(parsed).toHaveProperty('impactedCount');
    expect(parsed).toHaveProperty('byDepth');
    expect(parsed).toHaveProperty('risk');
    expect(parsed.target).toHaveProperty('name');
  }
});

---

6. target field shape is always {name: string} in both error and success responses

File: test/integration/local-backend-calltool.test.ts — add to "tool parameter edge cases" block

Guards against the schema inconsistency bug fixed in this PR (review bug #1). Checks that no code path returns target as a raw string.

it('impact tool target field is always an object with .name in both error and success', async () => {
  const errorResult = await backend.callTool('impact', {
    target: 'nonexistent_xyz_999',
    direction: 'upstream',
  });
  expect(typeof errorResult.target).toBe('object');
  expect(typeof errorResult.target.name).toBe('string');

  const successResult = await backend.callTool('impact', {
    target: 'validate',
    direction: 'upstream',
  });
  expect(typeof successResult.target).toBe('object');
  expect(typeof successResult.target.name).toBe('string');
});

---

Priority order

#	Test	Why critical
1	Nonexistent symbol → structured error	Covers the "symbol not found" (~40%) failure mode, the most common production error
4	CLI exit code ≠ 139	Direct regression guard for the SIGSEGV bug that motivated the whole PR
3	Partial results on traversal failure	The traversalComplete + break logic has no integration coverage at all
2	No partial on total failure	Guards the misleading-flag fix from the review
6	target shape consistency	Guards the schema fix from review bug #1
5	CLI happy path shape	Baseline e2e confidence in the structured output format

Tests 1, 2, and 6 slot cleanly into the existing withTestLbugDB block in local-backend-calltool.test.ts with no new dependencies. Test 3 requires vi.spyOn on executeQuery. Tests 4 and 5 extend the existing cli-e2e.test.ts and depend on analyze having run — run them in the same describe block after the analyze test.

[github-actions]

CI Report

✅ All checks passed

Pipeline Status

Stage	Status	Details
✅ Typecheck	success	tsc --noEmit
✅ Unit Tests	success	3 platforms
✅ Integration	success	3 OS x 4 groups = 12 jobs

Test Results

Suite	Tests	Passed	Failed	Skipped	Duration
Unit	1565	1564	0	1	8s
Integration	1167	1147	0	19	52s
Total	2732	2711	0	20	60s

✅ All 2711 tests passed

20 test(s) skipped — expand for details

Integration:

• Python match/case as-pattern type binding > resolves u.save() to User#save via match/case as-pattern binding
• Python match/case as-pattern type binding > does NOT resolve u.save() to Repo#save (negative disambiguation)
• Swift constructor-inferred type resolution > detects User and Repo classes, both with save methods
• Swift constructor-inferred type resolution > resolves user.save() to Models/User.swift via constructor-inferred type
• Swift constructor-inferred type resolution > resolves repo.save() to Models/Repo.swift via constructor-inferred type
• Swift constructor-inferred type resolution > emits exactly 2 save() CALLS edges (one per receiver type)
• Swift self resolution > detects User and Repo classes, each with a save function
• Swift self resolution > resolves self.save() inside User.process to User.save, not Repo.save
• Swift parent resolution > detects BaseModel and User classes plus Serializable protocol
• Swift parent resolution > emits EXTENDS edge: User → BaseModel
• Swift parent resolution > emits IMPLEMENTS edge: User → Serializable (protocol conformance)
• Swift cross-file User.init() inference > resolves user.save() via User.init(name:) inference
• Swift cross-file User.init() inference > resolves user.greet() via User.init(name:) inference
• Swift return type inference > detects User class and getUser function
• Swift return type inference > detects save function on User (Swift class methods are Function nodes)
• Swift return type inference > resolves user.save() to User#save via return type of getUser() -> User
• Swift return-type inference via function return type > resolves user.save() to User#save via return type of getUser()
• Swift return-type inference via function return type > user.save() does NOT resolve to Repo#save
• Swift return-type inference via function return type > resolves repo.save() to Repo#save via return type of getRepo()

Unit:

• buildTypeEnv > known limitations (documented skip tests) > Ruby block parameter: users.each { |user| } — closure param inference, different feature

Code Coverage

Combined (Unit + Integration)

Metric	Coverage	Covered	Base	Delta	Status
Statements	61.47%	6847/11137	46.42%	📈 +15.0	🟢 ████████████░░░░░░░░
Branches	55.67%	4677/8401	41.41%	📈 +14.3	🟢 ███████████░░░░░░░░░
Functions	60.71%	592/975	45.99%	📈 +14.7	🟢 ████████████░░░░░░░░
Lines	63.41%	6125/9658	47.96%	📈 +15.4	🟢 ████████████░░░░░░░░

Coverage breakdown by test suite

Unit Tests

Metric	Coverage	Covered	Base	Delta	Status
Statements	46.88%	5222/11137	46.42%	📈 +0.5	🟢 █████████░░░░░░░░░░░
Branches	42.11%	3538/8401	41.41%	📈 +0.7	🟢 ████████░░░░░░░░░░░░
Functions	45.84%	447/975	45.99%	📉 -0.1	🔴 █████████░░░░░░░░░░░
Lines	48.43%	4678/9658	47.96%	📈 +0.5	🟢 █████████░░░░░░░░░░░

Integration Tests

Metric	Coverage	Covered	Base	Delta	Status
Statements	46.5%	5179/11137	46.42%	📈 +0.1	🟢 █████████░░░░░░░░░░░
Branches	43.87%	3686/8401	41.41%	📈 +2.5	🟢 ████████░░░░░░░░░░░░
Functions	44.1%	430/975	45.99%	📉 -1.9	🔴 ████████░░░░░░░░░░░░
Lines	47.58%	4596/9658	47.96%	📉 -0.4	🔴 █████████░░░░░░░░░░░

---

_{📋 View full run · Generated by CI}

[magyargergo]

Can you please also resolve the merge conflicts? 🙏

[ShunsukeHayashi]

Done — resolved merge conflicts and added integration tests per @claude's recommendation.

Merge conflicts resolved:

• src/cli/tool.ts: kept getBackend() inside try-catch + instanceof Error check
• src/mcp/local/local-backend.ts: kept {name: string} target shape + removed misleading partial: true

Integration tests added (test/integration/local-backend-calltool.test.ts):

1. Unknown symbol → structured error — verifies no crash, returns structured JSON
2. Consistent target shape — when error is returned, target is an object (not raw string)
3. Partial results flag — partial: true only set when results were actually collected

Uses the existing withTestLbugDB fixture + seeded graph data, consistent with existing test patterns.