[codex] Refactor engine route response helpers#207
Conversation
|
Warning Review limit reached
More reviews will be available in 27 minutes and 55 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughA new ChangesHTTP Response Standardization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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 introduces a set of standardized HTTP response and request body parsing helpers in httpResponse.ts and refactors multiple route handlers to use them, improving consistency and reducing boilerplate. It also adds conformance tests for these helpers. The feedback points out that the broad check for 'JSON' in the error handler in httpError.ts could lead to false positives for non-syntax errors (such as database errors involving JSON columns) and suggests restricting the check to SyntaxError instances.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (error.message?.includes('JSON')) { | ||
| return jsonMalformedBody(c); | ||
| } |
There was a problem hiding this comment.
Checking error.message?.includes('JSON') is too broad and can lead to false positives. For example, if a database query fails on a JSON column or an internal library throws an error containing the word "JSON", it will be incorrectly treated as a 400 Bad Request with a "Malformed JSON in request body" message instead of a 500 Internal Server Error.
To prevent this, restrict the check to SyntaxError instances (which are thrown by JSON.parse).
| if (error.message?.includes('JSON')) { | |
| return jsonMalformedBody(c); | |
| } | |
| if (error instanceof SyntaxError && error.message?.includes('JSON')) { | |
| return jsonMalformedBody(c); | |
| } |
There was a problem hiding this comment.
Fixed in 2a98936 by restricting malformed-body mapping to SyntaxError instances. Added packages/engine/src/lib/tests/httpError.test.ts to prove SyntaxError still maps to invalid_json and coded errors mentioning JSON keep their original route_not_found/404 response.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28f111cab2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (error.message?.includes('JSON')) { | ||
| return jsonMalformedBody(c); |
There was a problem hiding this comment.
This string check treats any thrown error whose message contains uppercase JSON as malformed request JSON, regardless of where it came from. For example, a valid POST /v1/route body with a missing skill named JSON reaches routingEngine.routeBySkill, which throws route_not_found/404 with No active agents found for skill "JSON"; routes/routing.ts then forwards it here and these lines rewrite it to invalid_json/400. Please restrict malformed-body handling to the request parser or a more specific error type so normal coded errors keep their status and code.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2a98936. errorResponse now only treats SyntaxError messages containing JSON as malformed request bodies, and the new httpError regression test covers the skill "JSON" coded-error case so it remains route_not_found/404.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/engine/src/lib/httpResponse.ts (1)
8-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidationIssue interface is minimal compared to Zod v4's full issue structure.
The
ValidationIssueinterface only defines thepathproperty, but Zod v4 issues include additional properties likecode,message, and type-specific fields (e.g.,minimumfortoo_smallissues). Routes using the callback form ofinvalidMessagethat try to access these properties won't have type safety.Consider expanding this interface to match Zod v4's core issue types, or adding a comment documenting that routes should only rely on the
pathproperty for maximum compatibility.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/lib/httpResponse.ts` around lines 8 - 10, The ValidationIssue interface in the httpResponse.ts file is incomplete and only includes the path property, but Zod v4 provides additional issue properties like code, message, and type-specific fields (e.g., minimum for too_small issues). To fix this, expand the ValidationIssue interface to include all the core properties that Zod v4 provides (at minimum: code and message), or add a JSDoc comment above the interface documenting that consumers should only rely on the path property for maximum compatibility and should cast to any type if they need to access other Zod issue properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/engine/src/lib/httpError.ts`:
- Around line 47-49: The condition checking `error.message?.includes('JSON')` is
unreliable for detecting malformed JSON because it relies on string matching
which can produce false positives with unrelated errors. Replace this
string-based check with a proper type check using `instanceof SyntaxError`,
since Hono's `c.req.json()` throws a `SyntaxError` when encountering malformed
JSON in the request body. Update the if statement that currently checks
`error.message?.includes('JSON')` to instead check `error instanceof
SyntaxError` to ensure only actual syntax errors from JSON parsing are handled
by the `jsonMalformedBody(c)` response.
In `@packages/engine/src/lib/httpResponse.ts`:
- Around line 12-15: Update the SafeParseFailure interface to correctly
represent Zod v4's contract by removing the optional markers from the error
field and its nested issues property, making both required since they are always
present when success is false. Additionally, update the ValidationIssue
interface to include the code and message fields that Zod always provides
alongside the path field, ensuring the type accurately reflects the structure of
validation issues returned by Zod's safeParse method.
---
Nitpick comments:
In `@packages/engine/src/lib/httpResponse.ts`:
- Around line 8-10: The ValidationIssue interface in the httpResponse.ts file is
incomplete and only includes the path property, but Zod v4 provides additional
issue properties like code, message, and type-specific fields (e.g., minimum for
too_small issues). To fix this, expand the ValidationIssue interface to include
all the core properties that Zod v4 provides (at minimum: code and message), or
add a JSDoc comment above the interface documenting that consumers should only
rely on the path property for maximum compatibility and should cast to any type
if they need to access other Zod issue properties.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ded5bbab-63e4-4db3-8ef1-b86497237774
📒 Files selected for processing (8)
packages/engine/src/__tests__/conformance/httpResponse.test.tspackages/engine/src/lib/httpError.tspackages/engine/src/lib/httpResponse.tspackages/engine/src/routes/eventSubscription.tspackages/engine/src/routes/node.tspackages/engine/src/routes/receipt.tspackages/engine/src/routes/systemPrompt.tspackages/engine/src/routes/trigger.ts
1 participant
What changed
This refactors a slice of the engine route layer around HTTP response envelopes and JSON body parsing.
Why
Route modules were repeatedly hand-rendering the same
{ ok: true, data }and{ ok: false, error }envelopes and eachtry/catchcould decide malformed JSON behavior independently. Centralizing the common response paths gives the engine one pattern for new routes while preserving the existing snake_case wire contract and route-specific validation semantics.Validation
git diff --checknpm --workspace @relaycast/engine run typechecknpm --workspace @relaycast/engine run lintnpm --workspace @relaycast/engine test -- src/__tests__/conformance/httpResponse.test.tsnpm --workspace @relaycast/engine test -- src/__tests__/conformance/httpResponse.test.ts src/__tests__/conformance/sdk-contract.test.tsnpm --workspace @relaycast/engine testnpm --workspace @relaycast/engine run build127.0.0.1:18788for workspace/agent creation, malformed JSON, trigger create/list.127.0.0.1:18789for system prompt, subscription validation/create/list/get/delete, and read receipt flows.External
codex review --uncommittedwas attempted after user approval, but tenant policy still rejected uploading uncommitted private repository contents. I used the local automated review gate above as the safer substitute and recorded that in/tmp/refactor-relaycast.md.