fix: use safe error coercion in debug catch blocks

[carlos-alm]

Summary

• Addresses Greptile review feedback on fix: add debug logging to empty catch blocks across infrastructure and domain #616 (merged): replaces (e as Error).message with e instanceof Error ? e.message : String(e) in all 14 catch blocks introduced by that PR
• Prevents undefined in debug output when non-Error values are thrown (e.g. from WASM layer)
• Affects config.ts (4), native.ts (2), parser.ts (4), resolve.ts (4)

Test plan

• All 2128 tests pass locally
• Lint clean on all 4 changed files
• CI green

[claude]

Claude finished @carlos-alm's task —— View job

---

PR Review Complete

Review Checklist

• Read all 4 changed files to understand modifications
• Phase 1: Root Cause Analysis - determine what problem this PR solves
• Phase 2: Configuration Change Detection - check for high-risk changes
• Phase 3: Test Integrity - watch for red flags in test modifications
• Phase 4: Code Quality - evaluate error handling implementation
• Phase 5: Backlog Compliance - rate against BACKLOG.md axes
• Provide final assessment and recommendation

---

Root Cause Analysis

Problem Being Solved: Fixes unsafe type assertions introduced in PR #616 that added debug logging to catch blocks. The original implementation used (e as Error).message which produces undefined when non-Error values are thrown (common with WASM layer errors).

Why This Approach: The fix properly handles the TypeScript reality that catch (e) can receive any value, not just Error objects. Using e instanceof Error ? e.message : String(e) provides safe coercion that always produces meaningful debug output.

Risk Assessment: Very low risk. This is purely defensive programming in debug/logging paths with no behavioral changes to core functionality.

Configuration Changes

✅ No high-risk configuration changes detected

• No linting rules modified
• No test expectations lowered
• No build configuration changed
• No TypeScript strict flags disabled

Test Integrity

✅ No test files modified

• Changes only affect error handling in debug logging
• No test expectations were relaxed
• Author claims all 2128 tests pass locally

Code Quality Analysis

✅ Strengths

1. Consistent pattern: All 14 catch blocks updated with identical safe coercion
2. Comprehensive coverage: All instances from PR fix: add debug logging to empty catch blocks across infrastructure and domain #616 addressed in the affected files
3. Proper error handling: Handles edge cases where non-Error objects are thrown
4. No behavior change: Only improves debug output quality, no functional changes

Pattern Analysis

Before (unsafe):

} catch (e) {
  debug(`message: ${(e as Error).message}`);
}

After (safe):

} catch (e) {
  debug(`message: ${e instanceof Error ? e.message : String(e)}`);
}

📍 Locations Updated

• src/infrastructure/config.ts: 4 instances in workspace detection (lines 315, 351, 372, 390)
• src/infrastructure/native.ts: 2 instances in platform detection (lines 31, 100)
• src/domain/parser.ts: 4 instances in WASM handling (lines 447, 466, 583, 592)
• src/domain/graph/resolve.ts: 4 instances in import resolution (lines 70, 524, 546, 588)

Validation

✅ Confirmed all unsafe (e as Error).message patterns removed from the 4 target files
✅ Other files with the pattern are outside this PR's scope (pre-existing)

Backlog Compliance

• Zero-dep: ✅ No new dependencies added
• Foundation-aligned: ✅ Improves reliability without violating any principles
• Problem-fit: 3/5 - Useful reliability improvement, though tangential to core "AI navigation" problem
• Breaking: No - Pure internal improvement, no API changes
• Tier: 1 (zero-dep ✓ + foundation-aligned ✓)

Final Recommendation

Rating: ⭐⭐⭐⭐☆ (4/5)

Action: APPROVE

Reasoning: This is a well-executed defensive programming fix that addresses a real issue. The unsafe type assertions could produce undefined in debug output when non-Error objects are thrown (particularly relevant for WASM interactions). The solution is consistent, comprehensive, and safe.

Minor considerations:

• This addresses feedback for an already-merged PR rather than being caught in initial review
• While solid execution, the improvement is incremental rather than high-impact

The PR demonstrates good engineering hygiene by properly handling TypeScript's catch semantics and ensuring debug output remains meaningful regardless of what's thrown.

---

[greptile-apps]

Greptile Summary

This PR completes a follow-up to #616 by replacing all 14 instances of (e as Error).message (an unsafe TypeScript cast that silently produces undefined when a non-Error value is thrown) with the safe runtime guard e instanceof Error ? e.message : String(e). Changes are confined to debug(...) logging calls across four files — no business logic is affected.\n\n- The fix is correct, complete (all 14 sites addressed), and consistent across all changed files.\n- The change is especially valuable in WASM-adjacent catch blocks (parser.ts tree cleanup, resolve.ts native calls) where non-Error throws are a realistic path.\n- One non-blocking style suggestion: the repeated ternary could be extracted to a toErrorMessage(e: unknown) utility in shared/errors.ts to reduce future duplication.\n- All tests pass per the PR description; CI green is still pending but no code-level risk is apparent.

Confidence Score: 5/5

Safe to merge — all changes are in debug-only logging paths with no functional side effects.

Pure defensive fix in debug logging; the pattern used is the idiomatic TypeScript-safe approach for unknown catch values, all 14 sites from PR #616 are covered, and the changes carry zero runtime risk to any non-debug code path.

No files require special attention.

Important Files Changed

Filename	Overview
src/domain/graph/resolve.ts	4 catch blocks updated from unsafe (e as Error).message cast to safe e instanceof Error ? e.message : String(e) — all in debug-only paths, no functional logic changed.
src/domain/parser.ts	4 catch blocks updated with safe error coercion; notably the WASM tree cleanup blocks where non-Error throws are most likely, making this change particularly valuable here.
src/infrastructure/config.ts	4 catch blocks updated across workspace/lerna/pnpm detection paths — all debug-only, no behavioral change.
src/infrastructure/native.ts	2 catch blocks updated in native addon loader — straightforward fix, no issues.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[catch block triggered
e: unknown] --> B{e instanceof Error?}
    B -- Yes --> C[use e.message
structured Error object]
    B -- No --> D[use String e
handles WASM strings,
numbers, objects, etc.]
    C --> E[debug log message]
    D --> E

Loading

Comments Outside Diff (1)

1. src/shared/errors.ts, line 75 (link)

   Consider extracting a shared toErrorMessage helper

   The pattern e instanceof Error ? e.message : String(e) now appears 14 times across 4 files. A small utility in shared/errors.ts would make the intent explicit and simplify any future changes (e.g. if you ever want to include the error name, or handle AggregateError):

   /** Safely extract a string message from an unknown thrown value. */
   export function toErrorMessage(e: unknown): string {
     return e instanceof Error ? e.message : String(e);
   }

   Each call site then becomes the more readable:

   debug(`readPackageExports: failed to read package.json in ${packageDir}: ${toErrorMessage(e)}`);

   This is a non-blocking suggestion — the current repetition is fine and consistent, just worth considering if the pattern spreads further.

   _{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!}

_{Reviews (1): Last reviewed commit: "fix: use safe error coercion in debug ca..." | Re-trigger Greptile}