Skip to content

fix: add debug logging to empty catch blocks across infrastructure and domain#616

Merged
carlos-alm merged 5 commits intomainfrom
fix/titan-empty-catch-logging
Mar 26, 2026
Merged

fix: add debug logging to empty catch blocks across infrastructure and domain#616
carlos-alm merged 5 commits intomainfrom
fix/titan-empty-catch-logging

Conversation

@carlos-alm
Copy link
Contributor

Summary

  • Replaces empty catch blocks with debug() logging across 4 files that failed Titan audit
  • Addresses Pillar II (Data & Type Sovereignty) violations: silent error swallowing
  • All 4 files already import or now import logger.ts -- no new dependencies

Titan Audit Context

  • Phase: quality_fix (Forge Phase 1)
  • Domain: infrastructure + domain
  • Commits: 1
  • Depends on: none

Changes

  • src/infrastructure/config.ts -- 4 empty catches replaced with debug logging
  • src/infrastructure/native.ts -- 2 empty catches replaced with debug logging
  • src/domain/parser.ts -- empty catch replaced with debug logging
  • src/domain/graph/resolve.ts -- empty catch replaced with debug logging

Metrics Impact

  • Fixes 4 of 6 FAIL targets from Titan audit
  • No signature changes, no blast radius
  • All changes are additive (adding debug() calls)

Test plan

  • CI passes (lint + build + tests)
  • codegraph check --cycles --boundaries passes
  • No new functions above complexity thresholds

…d domain layers

Impact: 8 functions changed, 33 affected
@claude
Copy link

claude bot commented Mar 26, 2026

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


PR Review: fix: add debug logging to empty catch blocks across infrastructure and domain

Review Progress

  • Read and analyze all changed files
  • Phase 1: Root cause analysis
  • Phase 2: Configuration change detection
  • Phase 3: Test integrity check
  • Phase 4: Code quality assessment
  • Phase 5: Backlog compliance grading
  • Provide final structured assessment

Root Cause Analysis

Problem Being Solved: Empty catch blocks that silently swallow errors without any logging or handling, flagged by Titan audit as Pillar II violations (Data & Type Sovereignty)
Why This Approach: The PR replaces catch { } with catch (e) { debug(...) } to provide visibility into errors while maintaining non-blocking fallback behavior. This addresses legitimate observability gaps rather than hiding symptoms.
Risk Assessment: Low risk - purely additive logging that doesn't change control flow or business logic

Phase Assessment Results

Phase 2: Configuration Change Detection ✅

PASS - No configuration changes detected. No biome rules relaxed, no test timeouts increased, no build configuration modified.

Phase 3: Test Integrity ✅

PASS - No test files modified, no assertions weakened, no eslint-disable comments added.

Phase 4: Code Quality ✅

PASS - Changes are well-implemented:

  • All catch blocks now capture the error parameter: catch (e)
  • Consistent error message formatting with context
  • Proper typing: (e as Error).message
  • Debug-level logging (appropriate for fallback scenarios)
  • No performance impact - only adds logging when exceptions occur

Specific Changes Reviewed:

src/infrastructure/config.ts (4 locations):

  • Line 313: resolveWorkspaceEntry - package.json probe failure
  • Line 347: detectWorkspaces - pnpm-workspace.yaml parse failure
  • Line 366: detectWorkspaces - package.json workspaces parse failure
  • Line 382: detectWorkspaces - lerna.json parse failure

src/infrastructure/native.ts (2 locations):

  • Line 30: detectLibc - /lib directory read failure
  • Line 98: getNativePackageVersion - package.json read failure

src/domain/parser.ts (3 locations):

  • Line 445: backfillTypeMap - file read failure
  • Line 462: backfillTypeMap - WASM tree cleanup failure
  • Line 577+585: parseFilesAuto - typeMap backfill and WASM cleanup failures

src/domain/graph/resolve.ts (4 locations):

  • Line 68: getPackageExports - package.json read failure
  • Line 522: resolveImportPath - native resolution fallback
  • Line 544: computeConfidence - native computation fallback
  • Line 586: resolveImportsBatch - native batch resolution failure

All changes follow consistent patterns and maintain existing error handling behavior while adding visibility.

Backlog Compliance

  • Zero-dep: ✅ — No new runtime dependencies added; uses existing logger.js
  • Foundation-aligned: ✅ — Improves observability without violating any P1-P8 principles
  • Problem-fit: 3 — Improves debugging/observability but doesn't directly address AI token waste or hallucinated dependencies
  • Breaking: No — Purely additive debug logging with no API/behavior changes
  • Tier: 1 (zero-dep ✅ + foundation-aligned ✅)

Critical Concerns

None identified - This is a straightforward observability improvement addressing legitimate silent error swallowing.

Final Recommendation

  • Rating: ⭐⭐⭐⭐☆ (4/5)
  • Action: APPROVE
  • Reasoning: Well-executed fix that improves error visibility without introducing risk. Clean implementation following consistent patterns across the codebase. The changes address legitimate observability gaps identified by static analysis tooling rather than papering over real issues.

Minor suggestions for future PRs:

  • Consider including a sample of the new debug output in the PR description to demonstrate the improvement
  • The 8 empty catch blocks → debug logging conversion rate shows good coverage

This is exactly the type of low-risk, high-value observability improvement that helps maintain a healthy codebase.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 26, 2026

Greptile Summary

This PR addresses a Titan audit quality finding (Pillar II — silent error swallowing) by replacing empty catch blocks with debug() logging across 4 files. The change is purely additive: no signatures change, no new dependencies are introduced (two files add a debug import from the already-present logger.ts), and all calls are gated behind the existing verbose flag so there is zero production noise impact.\n\nThe messages are well-named, consistently prefixed by function name, and cover the right error sites — fs reads, JSON parses, WASM tree cleanup, and native addon fallbacks.\n\nKey observations:\n- All 4 files use (e as Error).message consistently; for Node.js core APIs this is safe in practice, but it's an unsafe TypeScript pattern that would silently produce undefined for non-Error throws (see inline comment).\n- Import paths added to native.ts (./logger.js) and resolve.ts (../../infrastructure/logger.js) are correct.\n- No circular dependency risk — logger.ts depends only on process.stderr.\n- The debug() guard in logger.ts (if (verbose)) ensures these calls are no-ops unless --verbose is passed.

Confidence Score: 5/5

Safe to merge — changes are purely additive debug logging with no behavior changes.

All changes are mechanical replacements of empty catch blocks with no-op-by-default debug() calls. No logic, signatures, or public API is altered. The only finding is a minor style concern with (e as Error).message that has no practical impact given the callers all use Node.js standard library functions.

No files require special attention.

Important Files Changed

Filename Overview
src/infrastructure/config.ts Replaces 3 silent /* ignore */ catch blocks in workspace detection functions with debug() calls; debug was already imported — purely additive change.
src/infrastructure/native.ts Adds debug import and replaces 2 empty catch blocks in detectLibc and getNativePackageVersion with informative debug messages.
src/domain/parser.ts Replaces 4 empty/comment-only catch blocks (file read failure, WASM tree cleanup ×2, typeMap backfill) with debug() calls; debug was already imported.
src/domain/graph/resolve.ts Adds debug import and replaces 4 empty catch blocks across package-exports lookup and native/batch resolution fallback paths with contextual debug messages.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Error thrown in catch block] --> B{Before this PR}
    A --> C{After this PR}
    B --> D[Silent: empty catch]
    C --> E{verbose mode?}
    E -->|no| F[No-op: debug gated off]
    E -->|yes| G["stderr: [codegraph DEBUG] <fn>: ...message"]
    G --> H[Execution continues with fallback value]
    F --> H
Loading

Reviews (2): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile

…-logging

Impact: 1 functions changed, 2 affected
@carlos-alm
Copy link
Contributor Author

@greptileai

/* ignore */
} catch (e) {
debug(
`resolveWorkspaceEntry: package.json probe failed for ${pkgDir}: ${(e as Error).message}`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 (e as Error).message unsafe for non-Error throws

The pattern (e as Error).message is used consistently across all 4 changed files. In TypeScript strict mode, e is unknown, and if anything other than an Error instance is thrown (e.g. a plain string, null, or a non-standard exception from the WASM layer), .message will be undefined, producing unhelpful log output like readPackageExports: failed to read package.json in /path: undefined.

A safer, idiomatic alternative:

Suggested change
`resolveWorkspaceEntry: package.json probe failed for ${pkgDir}: ${(e as Error).message}`,
`resolveWorkspaceEntry: package.json probe failed for ${pkgDir}: ${e instanceof Error ? e.message : String(e)}`,

The same pattern appears in all new debug() calls across these files:

  • src/infrastructure/config.ts:315, 349, 367
  • src/infrastructure/native.ts:31, 98
  • src/domain/graph/resolve.ts:70, 522, 544, 585
  • src/domain/parser.ts:443, 462, 576, 582

For the Node.js built-ins (fs, JSON.parse, require) these always throw Error instances, so the risk is low in practice — but the WASM tree.delete() calls could theoretically throw a non-Error value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in follow-up PR #630 — all 14 occurrences of \ in this PR's catch blocks have been replaced with \ for safe handling of non-Error throws.

@carlos-alm carlos-alm merged commit 714fc0e into main Mar 26, 2026
12 checks passed
@carlos-alm carlos-alm deleted the fix/titan-empty-catch-logging branch March 26, 2026 07:17
@github-actions github-actions bot locked and limited conversation to collaborators Mar 26, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant