Skip to content

refactor(sse): classify SSE critical-path empty catches + add CONTRIBUTING convention (#8142) - #8364

Merged
diegosouzapw merged 1 commit into
release/v3.8.49from
feat/8142-empty-catch-convention
Jul 24, 2026
Merged

diegosouzapw merged 1 commit into
release/v3.8.49from
feat/8142-empty-catch-convention

Conversation

@diegosouzapw

Copy link
Copy Markdown
Collaborator

Summary

  • Classifies the 7 remaining empty catch {} blocks in the SSE critical path (open-sse/utils/stream.ts x6, open-sse/utils/streamHandler.ts x1) into two buckets: intentional (our own best-effort cleanup/telemetry — gets a one-line rationale comment, no logging) vs should-log (the caller-supplied onFailure callback — a throw there previously vanished silently, changing control flow with zero trace).
  • Adds an "Error handling / empty catch blocks" subsection to CONTRIBUTING.md (near Code Style) codifying that rule for future waves.
  • Scope is exactly this SSE critical-path slice, per the plan-file split (executors/services follow in later PRs). Does not touch the other ~140 empty catches elsewhere in the repo; open-sse/executors/cursor.ts is already at zero from fix(stream): add logging to empty catch blocks in stream error handling #8143 and untouched here.

Sites classified

  • stream.ts L816 consumeToolFinishTime — intentional (comment)
  • stream.ts L1833, L2112 — telemetry/latency recording — intentional (comment)
  • stream.ts L1870, L1998 markToolFinish — intentional (comment)
  • stream.ts L1946 onFailure(failurePayload) — should log: kept the catch (never break the stream) but added console.debug so a buggy consumer callback is now discoverable
  • streamHandler.ts L586 controller.close() after disconnect — intentional (comment; the canonical example the issue cites)

How tested

TDD per Hard Rule #18 — new test file tests/unit/stream-onfailure-callback-logging.test.ts:

  • Wrote the failing test first: asserted a console.debug call referencing onFailure is emitted when the caller-supplied callback throws. Failed before the change (empty catch swallowed silently, no log).
  • Implemented the fix (kept catch, added console.debug), test now passes.
  • Regression guard: onFailure returning normally emits no log and behaves byte-identically — proves the happy path and the 6 comment-only sites are behavior-free.
node --import tsx/esm --test tests/unit/stream-onfailure-callback-logging.test.ts
# 2/2 passing

Also verified: tests/unit/stream-utils.test.ts + tests/unit/claude-empty-stream-error-3685.test.ts still 58/58 green (no regression in adjacent stream-path coverage).

Gates run locally: typecheck:core clean, eslint clean on changed files, check:cycles clean, check-file-size.mjs clean (tightened an unrelated 3-line JSDoc block to 1 line to stay under the frozen stream.ts baseline after adding the log), check-test-discovery.mjs clean. check:complexity-ratchets shows the same pre-existing base-red as the pristine tip (2167/956 vs baseline 2130/951) — unaffected by this change.

Closes #8142

@diegosouzapw
diegosouzapw merged commit 9994e00 into release/v3.8.49 Jul 24, 2026
10 checks passed
@diegosouzapw
diegosouzapw deleted the feat/8142-empty-catch-convention branch July 24, 2026 15:36
HouMinXi pushed a commit to HouMinXi/OmniRoute that referenced this pull request Aug 2, 2026
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(stream): document intentional empty catches vs ones needing logging

1 participant