feat(api): a real global error boundary — JSON 500s, structured logs, and non-Error containment - #9756
Merged
Merged
Conversation
… and non-Error containment
The app had no app.onError(), so it ran on Hono's built-in default. That
was never a crash risk -- Hono always installs one, and compose() sets
context.error before dispatching, which is what the PostHog capture reads
-- but the default is wrong for this app in two ways, and leaves a real
hole in a third.
1. It answers `c.text("Internal Server Error", 500)`: a text/plain body
from a JSON API. Every other route here answers `{ error: ... }`, so a
client parsing JSON hit a parse failure on precisely the response it
most needed to read. Now a 500 is `{ error: "internal_error" }`.
2. It logs `console.error(err)`, a raw Error. This codebase's forwarder
keys on structured JSON, so an unhandled route error was not
classified, and carried no method or path -- a production 500 could not
be traced to the route that raised it. Now it is one structured line.
3. THE HOLE: compose() routes a caught throw to onError only when it is
`instanceof Error`, and RE-THROWS everything else. So `throw "string"`,
a rejected promise carrying a plain object, or a library throwing a
non-Error escaped the boundary entirely -- no response, no log, and no
context.error, so PostHog never saw it either. Verified empirically:
without the new nonErrorBoundary the request REJECTS rather than
responding. A try/catch around next() is the right shape for exactly
this case and only this case, since an Error thrown below is already a
response by the time next() resolves.
Deliberately preserved: an HTTPException keeps its own status. Swallowing
those into 500s is the single most damaging thing a global handler can do,
so every one of 400/401/403/404/413/422/429 is asserted. The check is
duck-typed on getResponse, as Hono's own default is, because two copies of
hono in one workspace module graph would break `instanceof` -- asserted
with a hand-rolled foreign exception.
Never leaked: the body is a fixed code and nothing else -- no message,
stack or cause. Several routes here are unauthenticated and an error
string can carry a token, a binding name or an upstream URL. The detail
goes to the operator-only log, bounded, and the log uses c.req.path (the
matched path, no query string) so a token in a query parameter cannot ride
along -- both asserted.
13 tests, all through real Hono dispatch rather than a faked Context,
including that c.error is still set so error capture is unaffected, that
a successful route is untouched, and that a circular/huge thrown value
still produces a bounded loggable line.
Contributor
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-29 07:11:26 UTC
Review summary Blockers
Nits — 5 non-blocking
Concerns raised — review before merging
📋 Copy for AI agents — paste into your coding agentDecision drivers
Context & advisory signals — never blocks the verdict
Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9756 +/- ##
==========================================
- Coverage 90.27% 89.46% -0.82%
==========================================
Files 904 905 +1
Lines 113177 113191 +14
Branches 26840 26842 +2
==========================================
- Hits 102171 101261 -910
- Misses 9676 10842 +1166
+ Partials 1330 1088 -242
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The app had no
app.onError(), so it ran on Hono's built-in default. That was never a crash risk — Hono always installs one, andcompose()setscontext.errorbefore dispatching, which is what the PostHog capture middleware reads. But the default is wrong for this app in two ways and leaves a real hole in a third.1. It answers
c.text("Internal Server Error", 500)— atext/plainbody from a JSON API. Every other route here answers{ error: ... }, so a client parsing JSON hit a parse failure on precisely the response it most needed to read.2. It logs
console.error(err)— a raw Error. This codebase's forwarder keys on structured JSON ({level, event, …}), so an unhandled route error wasn't classified, and carried no method or path. A production 500 couldn't be traced back to the route that raised it.3. The hole.
compose()routes a caught throw toonErroronly when it isinstanceof Error, and re-throws everything else. Sothrow "a string", a rejected promise carrying a plain object, or a library throwing a non-Error escaped the boundary entirely — no response, no log, and nocontext.error, so PostHog never saw it either.I verified that empirically rather than reasoning about it: my first test asserted a non-Error "still produces a response somehow", and it failed — the request rejects.
nonErrorBoundarycloses it. Atry/catcharoundnext()is the right shape for exactly this case and only this case, since anErrorthrown below has already become a response by the timenext()resolves (which is whyworker-posthog.tsdocuments that a try/catch there would never see one).Deliberately preserved
An
HTTPExceptionkeeps its own status. Collapsing those into 500s is the single most damaging thing a global handler can do, so 400/401/403/404/413/422/429 are each asserted. The check is duck-typed ongetResponse, as Hono's own default is, rather thaninstanceof HTTPException— two copies of hono in one workspace module graph would breakinstanceofand silently turn every intentional 4xx into a 500. Asserted with a hand-rolled foreign exception carrying its owngetResponse.Never leaked
The body is a fixed code and nothing else — no message, stack, or cause. Several routes here are unauthenticated, and an error string can carry a token, a binding name, or an upstream URL. The detail goes to the operator-only log, bounded; and the log uses
c.req.path(the matched path, no query string) so a token in a query parameter can't ride along. Both asserted, including a probe for the specific secret substrings.Tests (13, all through real Hono dispatch)
JSON shape and content-type; zero leakage; one structured line with method+path; query strings excluded from logs; every HTTPException status preserved and not logged as a fault; non-Error containment with its own event; the boundary not double-handling Errors (exactly one log line, from
onError); a circular/huge thrown value still producing a bounded line; a successful route untouched; andc.errorstill set so error capture is unaffected.Run through a real app rather than a faked
Context— status preservation and leakage are only meaningful end to end, and a hand-built context would let all of it pass while the wiring was wrong. Regression-checked against the existing route suites (auth, ingest/repo-scope security, index, route↔spec ratchet): 130 passing, no behavior change on the ~241 existing routes.