fix: replace empty catch blocks with structured error handling#764
fix: replace empty catch blocks with structured error handling#764carlos-alm merged 10 commits intomainfrom
Conversation
Replace catch blocks that silently swallow errors with proper error variable capture and debug() logging across 10 files. This addresses Pillar II (Rule 10: emptyCatch) violations identified by the gauntlet audit. Files that already had proper error handling were verified and left unchanged.
|
Claude finished @carlos-alm's task —— View job Reviewing PR: Empty Catch Blocks FixI'll conduct a comprehensive review of this PR following the 5-phase analysis framework. Todo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved: The PR addresses empty catch blocks across the codebase that were likely flagged by linting rules (Biome Rule 10: emptyCatch). Empty catch blocks silently swallow errors without logging or handling them appropriately. Why This Approach: The PR takes a structured approach by:
Risk Assessment: ✅ LOW RISK - This is genuine error handling improvement, not rule suppression. Phase 2: Configuration Change Detection ✅No configuration changes detected:
Phase 3: Test Integrity ✅No test files modified:
Phase 4: Code Quality Review ✅Excellent implementation quality:
Specific code quality observations:
Phase 5: Backlog ComplianceRoot Cause AnalysisProblem Being Solved: Eliminates silent error swallowing via empty catch blocks, replacing with structured logging for debugging and monitoring. Backlog Compliance
Critical ConcernsNone identified. The implementation follows best practices and improves code quality without introducing risks. Final Recommendation
Additional notes:
|
Greptile SummaryThis PR completes the structured error-handling cleanup by replacing every remaining
Confidence Score: 5/5All prior P1 concerns resolved; no new issues found — safe to merge. The two P1 findings from the previous review round (unsafe non-Error casts and the unused helper API) are both fully addressed. The changes are purely mechanical substitutions of a well-tested utility, with no logic regressions. The one deliberate divergence in mcp/server.ts (stack trace on fatal exit) is an improvement, not a gap. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[catch receives unknown] --> B{e instanceof Error?}
B -- Yes --> C[e.message]
B -- No --> D["String(e)"]
C --> E[toErrorMessage result]
D --> E
E --> F{Call site}
F -- debug log --> G[debug context + message]
F -- warn log --> H[warn context + message]
F -- fatal exit --> I["err.stack ?? err.message"]
F -- errno code --> J["ErrnoException.code or toErrorMessage"]
F -- return value --> K["return error string"]
subgraph suppress["suppress.ts helpers"]
L["suppressError / suppressErrorAsync"] --> M[try fn]
M -- throws --> N[debug + return fallback]
M -- ok --> O[return result]
end
Reviews (3): Last reviewed commit: "fix: replace remaining unsafe (err as Er..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/shared/errors.ts
Outdated
| export function suppressError<T>(fn: () => T, context: string, fallback: T): T { | ||
| try { | ||
| return fn(); | ||
| } catch (e: unknown) { | ||
| debug(`${context}: ${toErrorMessage(e)}`); | ||
| return fallback; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Async variant of {@link suppressError}. Awaits `fn()` and returns `fallback` | ||
| * on rejection, logging the error via `debug()`. | ||
| * | ||
| * @example | ||
| * const data = await suppressErrorAsync(() => fetchOptionalData(), 'fetch data', null); | ||
| */ | ||
| export async function suppressErrorAsync<T>( | ||
| fn: () => Promise<T>, | ||
| context: string, | ||
| fallback: T, | ||
| ): Promise<T> { | ||
| try { | ||
| return await fn(); | ||
| } catch (e: unknown) { | ||
| debug(`${context}: ${toErrorMessage(e)}`); | ||
| return fallback; | ||
| } | ||
| } |
There was a problem hiding this comment.
suppressError helpers added but never used
The PR adds suppressError and suppressErrorAsync as reusable helpers, and the JSDoc + design are solid. However, none of the 10 changed files actually call them — every catch block is replaced with the inline catch (e) { debug(...) } pattern instead. This means two competing idioms now coexist:
suppressError(() => fn(), 'ctx', fallback)— usestoErrorMessage(e)(safe for non-Error throws)catch (e) { debug(\ctx: ${(e as Error).message}`) }— unsafe cast, returnsundefined` message if a non-Error is thrown
Either apply the helper uniformly (replacing the inline patterns), or don't ship the helper until it's actually adopted. Shipping an abstraction that isn't used creates maintenance confusion about which pattern is canonical.
There was a problem hiding this comment.
Addressed the unsafe cast issue (all 10 files now use toErrorMessage(e) consistently). Regarding the unused helpers: suppressError/suppressErrorAsync are intentionally shipped as available utilities for future adoption — they provide a higher-level API over the inline pattern. The codebase now has one canonical safety pattern (toErrorMessage) used everywhere, and the helpers build on it. Removing them would mean re-adding them later when callers want the wrap-and-fallback pattern.
| ); | ||
| } |
There was a problem hiding this comment.
(e as Error).message cast throughout PR
Every changed catch block uses (e as Error).message. If the thrown value is not an Error instance (e.g. a rejected promise with a plain string, null, or a plain object), .message evaluates to undefined, producing an uninformative debug line like "Version mismatch check skipped (build_meta may not exist): undefined".
The codebase already has toErrorMessage(e) in shared/errors.ts and the new suppressError helper uses it correctly. The inline catch blocks should do the same:
| ); | |
| } | |
| debug(`Version mismatch check skipped (build_meta may not exist): ${toErrorMessage(e)}`); |
The same applies in every other catch block added in this PR (src/ast-analysis/engine.ts, src/domain/graph/builder/pipeline.ts, src/domain/graph/builder/stages/insert-nodes.ts, src/features/audit.ts, src/features/branch-compare.ts, src/infrastructure/config.ts, src/mcp/server.ts, src/presentation/queries-cli/overview.ts, src/presentation/viewer.ts).
There was a problem hiding this comment.
Fixed. All 10 files now use toErrorMessage(e) from shared/errors.ts instead of the unsafe (e as Error).message cast. Each file that didn't already import it now does. The special case in connection.ts (line 107) preserves the ErrnoException.code check while using toErrorMessage(e) as the fallback: (e as NodeJS.ErrnoException).code || toErrorMessage(e).
* fix: address remaining quality issues across domains Extract sub-functions to reduce halsteadEffort and cognitive complexity in 11 FAIL targets: dependencies, communities, server, parser, context, overview, impact, exports, path, viewer, dataflow. All functions now pass gate thresholds (cognitive <= 30, halstead.bugs <= 1.0, MI >= 20). * fix: correct hc parameter type in buildNodeDepsResult (#774) Change Map<unknown, unknown> to Map<string, string | null> to match normalizeSymbol's hashCache parameter type, fixing a TS2345 build error.
) * refactor: extract sub-functions from features domain god-functions * refactor: split buildAstNodes and fix console.log usage * fix: resolve TS2345 type error in buildNativeDataflowResult parameter (#772) Use NativeDatabase type directly instead of inline type literal, with non-null assertion on getDataflowEdges since the call site guards with an existence check. * fix: remove unused _baseSymbols parameter from attachImpactToSymbols (#772) The parameter was accepted but never read. Drop it from the signature and call site as flagged by Greptile review. * fix: add clarifying comment for handleBranchNode partial-handling return (#772) Document that returning false after incrementing accumulators is intentional — the caller continues with remaining checks (pattern-C else, case nodes, walkChildren).
|
Addressed the 4 remaining unsafe casts identified in the re-review:
Merged origin/main to resolve conflicts. All 2275 core tests pass, TypeScript compiles cleanly. |
Extract repeated patterns into focused helper functions: - pipeline.ts: closeNativeDb, reopenNativeDb, suspendNativeDb, refreshJsDb - insert-nodes.ts: marshalSymbolBatches, buildFileHashes - resolve-imports.ts: buildReexportMap, findBarrelCandidates, reparseBarrelFiles - watcher.ts: prepareWatcherStatements, processPendingFiles, writeJournalAndChangeEvents
1 participant
Summary
suppressExpectederror-handling helper inshared/errors.tsfor intentional catch suppressionTitan Audit Context
Changes
src/ast-analysis/engine.ts- Replace empty catches with logger.debugsrc/db/connection.ts- Replace empty catches with logger.debugsrc/domain/graph/builder/pipeline.ts- Replace empty catches with logger.debugsrc/domain/graph/builder/stages/insert-nodes.ts- Replace empty catches with logger.debugsrc/features/audit.ts- Replace empty catches with logger.debugsrc/features/branch-compare.ts- Replace empty catches with logger.debugsrc/infrastructure/config.ts- Replace empty catches with logger.debugsrc/mcp/server.ts- Replace empty catches with logger.debugsrc/presentation/queries-cli/overview.ts- Replace empty catches with logger.debugsrc/presentation/viewer.ts- Replace empty catches with logger.debugsrc/shared/errors.ts- Add suppressExpected helperMetrics Impact
Test plan