🥅 server: handle expected bridge pairing errors#933
Conversation
🦋 Changeset detectedLatest commit: fea9821 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the bridge pairing logic to return a 409 Conflict status instead of a generic error when a database update fails to find a matching credential. The changes include importing HTTPException in the bridge hook and updating the test suite to expect 409 status codes. A review comment points out that using a 4xx error for missing database records might violate repository conventions regarding data integrity issues, suggesting that a 5xx error remains more appropriate.
| .returning({ account: credentials.account, source: credentials.source }) | ||
| .then(([updated]) => { | ||
| if (!updated) throw new Error("no match found when pairing bridge id"); | ||
| if (!updated) throw new HTTPException(409, { message: "credential pairing failed" }); |
There was a problem hiding this comment.
According to the general rules of this repository, when an expected database record is not found, a 5xx-level error is preferred over a 4xx to indicate a system or data integrity issue. Since !updated here could mean the referenceId (obtained from Persona) does not exist in the credentials table, a 500 error might be more appropriate than a 409 Conflict, unless this specific failure is considered a recoverable state conflict rather than a data integrity issue.
References
- Throw an error when an expected database record (like a card) is not found. This indicates a system or data integrity issue, not a client-side error that can be fixed by the user, so a 5xx-level error is more appropriate than a 4xx.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| .returning({ account: credentials.account, source: credentials.source }) | ||
| .then(([updated]) => { | ||
| if (!updated) throw new Error("no match found when pairing bridge id"); | ||
| if (!updated) throw new HTTPException(409, { message: "credential pairing failed" }); |
There was a problem hiding this comment.
Bug: The global app.onError() handler doesn't check for HTTPException. It will catch the new 409 exception, log it, and incorrectly return a 555 status instead of 409.
Severity: MEDIUM
Suggested Fix
Update the global app.onError() handler in server/index.ts to check if the error is an instanceof HTTPException. If it is, re-throw the exception or return its response directly. For all other errors, maintain the existing behavior of calling captureException() and returning a 555 response.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: server/hooks/bridge.ts#L132
Potential issue: The code at `server/hooks/bridge.ts:132` now throws an
`HTTPException(409)` for credential pairing failures. While the intent is to return a
409 status code, the global `app.onError()` handler in `server/index.ts` does not
differentiate `HTTPException` from other errors. As a result, when this exception is
thrown in production, it will be caught by the global handler, which will log it to
Sentry via `captureException()` and return a generic 555 error response. This defeats
the purpose of using `HTTPException` and will cause clients to receive an incorrect
status code. The tests pass because they only test the isolated sub-app, not the full
application stack with the global error handler.
Did we get this right? 👍 / 👎 to inform future reviews.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| .returning({ account: credentials.account, source: credentials.source }) | ||
| .then(([updated]) => { | ||
| if (!updated) throw new Error("no match found when pairing bridge id"); | ||
| if (!updated) throw new HTTPException(409, { message: "credential pairing failed" }); |
There was a problem hiding this comment.
🚩 Webhook caller behavior may change with 409 vs 500
Bridge's webhook system may treat 4xx and 5xx responses differently — many webhook providers retry on 5xx (transient server errors) but not on 4xx (client errors indicating the request itself is invalid). Changing from 500 to 409 could stop Bridge from retrying these specific webhook deliveries. This appears intentional given the PR's stated goal of handling "expected" pairing errors, but worth confirming that Bridge's retry behavior for 409 is acceptable for these scenarios (credential already paired, or reference-id not found).
Was this helpful? React with 👍 or 👎 to provide feedback.
WalkthroughA patch-level release for the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #933 +/- ##
=======================================
Coverage 71.69% 71.69%
=======================================
Files 228 228
Lines 8277 8277
Branches 2661 2661
=======================================
Hits 5934 5934
Misses 2113 2113
Partials 230 230
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/test/hooks/bridge.test.ts (1)
288-306:⚠️ Potential issue | 🟠 MajorThis test is order-dependent and can fail when run in isolation.
The new 409 expectation relies on
fallback-testalready being paired by a previous test. Please set that precondition inside this test to keep it deterministic.🔧 Suggested deterministic setup
it("returns 409 when fallback credential already paired", async () => { + await database.update(credentials).set({ bridgeId: "already-paired-bridge-id" }).where(eq(credentials.id, "fallback-test")); vi.spyOn(database.query.credentials, "findFirst").mockResolvedValueOnce(undefined); // eslint-disable-line unicorn/no-useless-undefined vi.spyOn(bridge, "getCustomer").mockResolvedValue({ - id: "conflict-bridge-id", + id: "fresh-bridge-id", email: "conflict@example.com", status: "active", endorsements: [], });
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7b602c26-cbdb-4820-85e6-6b16d030fa83
📒 Files selected for processing (3)
.changeset/calm-panda-handle.mdserver/hooks/bridge.tsserver/test/hooks/bridge.test.ts
| .returning({ account: credentials.account, source: credentials.source }) | ||
| .then(([updated]) => { | ||
| if (!updated) throw new Error("no match found when pairing bridge id"); | ||
| if (!updated) throw new HTTPException(409, { message: "credential pairing failed" }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== confirm throw site =="
rg -n -C3 'throw new HTTPException\(409' server/hooks/bridge.ts
echo
echo "== inspect global error handler behavior =="
rg -n -C8 'app\.onError|captureException\(|unexpected error|555 as UnofficialStatusCode' server/index.tsRepository: exactly/exa
Length of output: 1810
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== find HTTPException import source =="
rg -n 'import.*HTTPException|from.*HTTPException' server/hooks/bridge.ts
echo
echo "== search for HTTPException definition in codebase =="
rg -n 'class HTTPException|export.*HTTPException' --type ts
echo
echo "== check if Hono exports HTTPException =="
rg -n 'HTTPException' package.jsonRepository: exactly/exa
Length of output: 203
🌐 Web query:
Hono HTTPException error handling how does it work
💡 Result:
Hono's HTTPException provides a standardized way to throw and handle HTTP errors. Import it from 'hono/http-exception'. To throw an HTTPException: - Basic: new HTTPException(401, { message: 'Unauthorized' }) - Custom response: new HTTPException(401, { res: new Response('Custom', { status: 401 }) }) - With cause: new HTTPException(401, { message: 'Error', cause: originalError }) Throw it in handlers or middleware for validation failures, auth errors, etc. Handling: Use app.onError((err, c) => { if (err instanceof HTTPException) { return err.getResponse; // Returns Response with status and message/res } // Handle other errors, e.g. console.error(err); return c.text('Internal Server Error', 500); }); This catches all unhandled exceptions, including HTTPExceptions from Hono core/middleware. HTTPException extends Error with status, res?, and getResponse method. For structured JSON errors, customize in onError: return c.json({ error: err.message, status: err.status }, err.status); Official docs confirm this mechanism remains current as of 2026.
Citations:
- 1: https://hono.dev/docs/api/exception
- 2: https://mintlify.com/honojs/hono/guides/error-handling
- 3: https://hono.cms.im/docs/api/exception
- 4: https://github.com/honojs/hono/blob/main/src/http-exception.ts
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== check for other error handlers or middleware before app.onError =="
rg -n 'app\.use.*error|middleware.*error|app\.onError' server/index.ts | head -20
echo
echo "== verify HTTPException is imported in index.ts =="
rg -n 'HTTPException' server/index.tsRepository: exactly/exa
Length of output: 208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== check full error handler logic to confirm status extraction won't match 'credential pairing failed' =="
sed -n '284,316p' server/index.tsRepository: exactly/exa
Length of output: 1501
HTTPException(409) is masked by the global error handler.
Line 132 throws HTTPException(409, ...), but the global error handler in server/index.ts:284–316 only tries to extract a 3-digit status from error.message. Since the message is "credential pairing failed" (not a status code), hasStatus remains false, and the handler unconditionally returns 555 regardless of the thrown status code.
Fix by either:
- Importing and checking
instanceof HTTPExceptioninapp.onError, then usingerr.getResponse()orc.json(..., err.status)to preserve the intended status, or - Returning a 409 response directly from the handler instead of throwing.
1 participant
Summary by CodeRabbit