[codex] Refactor engine architecture helpers#209
Merged
Conversation
📝 WalkthroughWalkthroughThis PR centralizes HTTP response formatting, request parsing, and WebSocket authentication across engine routes and middleware. It adds shared parsing/response helpers, rewires WS upgrade handling, refactors route handlers to use the helpers, and expands conformance coverage for malformed JSON and validation behavior. ChangesHTTP Response/Auth Centralization and Route Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. 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 standardizes HTTP response formatting, error handling, and request/query parsing across the entire engine codebase. It introduces new helper utilities in httpResponse.ts and httpQuery.ts, extracts WebSocket authentication into wsAuth.ts, and refactors all route handlers to use these helpers, significantly reducing boilerplate. Feedback on the changes includes addressing a validation issue in positiveIntQueryParam when handling empty or whitespace-only strings, and removing a redundant mixed-case header fallback in engine.ts to comply with the repository style guide.
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.
Comment on lines
+18
to
+21
| return z.preprocess( | ||
| (value) => (value === undefined ? undefined : Number(value)), | ||
| valueSchema, | ||
| ); |
There was a problem hiding this comment.
The positiveIntQueryParam preprocess function currently converts empty or whitespace-only strings (e.g., ?limit=) to 0 via Number(value). Since 0 is not a positive integer, this causes Zod validation to fail with a 400 Bad Request instead of falling back to the parameter's default value or treating it as optional/undefined.
Updating the preprocess function to map empty or whitespace-only strings to undefined allows Zod's .optional() or .default() schemas to handle them gracefully.
Suggested change
| return z.preprocess( | |
| (value) => (value === undefined ? undefined : Number(value)), | |
| valueSchema, | |
| ); | |
| return z.preprocess( | |
| (value) => (value === undefined || (typeof value === 'string' && value.trim() === '') ? undefined : Number(value)), | |
| valueSchema, | |
| ); |
| ? authHeader.replace(/^bearer\s+/i, '').trim() | ||
| : undefined; | ||
| const token = c.req.query('token') ?? bearer; | ||
| const token = queryOrBearerToken(c.req.query('token'), c.req.header('Authorization') ?? c.req.header('authorization')); |
There was a problem hiding this comment.
This line introduces a mixed-case field fallback: c.req.header('Authorization') ?? c.req.header('authorization').
According to the Repository Style Guide (Rule 9): "Do not introduce mixed-case field fallbacks."
Since Hono's c.req.header() is case-insensitive under the hood (conforming to the Fetch API standard), calling c.req.header('Authorization') is sufficient and will automatically retrieve the header regardless of its casing.
Suggested change
| const token = queryOrBearerToken(c.req.query('token'), c.req.header('Authorization') ?? c.req.header('authorization')); | |
| const token = queryOrBearerToken(c.req.query('token'), c.req.header('Authorization')); |
References
- Do not introduce mixed-case field fallbacks. (link)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/engine/src/routes/workspace.ts (1)
45-57: 🎯 Functional Correctness | 🟡 MinorEnforce an exclusive override shape in both feature-flag routes.
/workspace/streamand/workspace/fleet-nodesstill accept payloads with bothenabledandmode, andmode: "inherit"takes precedence over a providedenabled. Make the Zod schemas reject mixed shapes so the API only accepts{ enabled: boolean }or{ mode: "inherit" }.
packages/engine/src/routes/workspace.ts:45-57packages/engine/src/routes/workspace.ts:337-349packages/engine/src/routes/workspace.ts:397-409🤖 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/routes/workspace.ts` around lines 45 - 57, The updateWorkspaceStreamSchema and updateFleetNodesSchema currently allow mixed payloads where both enabled and mode can be provided, and the route handlers then let mode: "inherit" override enabled. Tighten the Zod validation in packages/engine/src/routes/workspace.ts so the workspaceStream and fleetNodes routes only accept one exclusive shape: either { enabled: boolean } or { mode: "inherit" }, but never both together. Update the route parsing/handling in the corresponding workspace stream and fleet-nodes endpoints to rely on these stricter schemas and reject mixed payloads before applying any override logic.Source: Coding guidelines
🧹 Nitpick comments (2)
packages/engine/src/routes/search.ts (1)
14-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the required
qcheck into the Zod schema.This adds a post-parse manual guard for
q, which is the validation pattern the TS/JS guidelines ask us to avoid. Putting the trim/non-empty requirement in the schema keeps validation in one place and removes the extra branch.As per coding guidelines, `Prefer zod schemas for validation instead of ad-hoc manual checks`.Suggested fix
const searchQuerySchema = PaginationQuerySchema.extend({ - q: z.string().optional(), + q: z.string().trim().min(1), channel: z.string().optional(), from: z.string().optional(), }); @@ - const parsed = parseQueryParams(c, searchQuerySchema, 'Invalid search query'); + const parsed = parseQueryParams(c, searchQuerySchema, (failure) => + failure.error.issues.some((issue) => issue.path[0] === 'q') + ? 'q (search query) is required' + : 'Invalid search query', + ); if (!parsed.ok) { return parsed.response; } const { q, channel, from, limit, before, after } = parsed.data; - if (!q || !q.trim()) { - return jsonInvalidRequest(c, 'q (search query) is required'); - }Also applies to: 29-35
🤖 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/routes/search.ts` around lines 14 - 18, The search route currently validates q with a manual post-parse guard instead of the schema. Move the trim/non-empty requirement into searchQuerySchema by making q a Zod string refinement rather than an optional plain string, and then remove the ad-hoc q check in the search handler logic so validation stays centralized in the schema.Source: Coding guidelines
packages/engine/src/engine.ts (1)
167-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid the mixed-case header fallback.
Use one canonical header spelling here; the
Authorization/authorizationcompatibility fallback violates the project rule against mixed-case fallbacks.Use a single header name
- const token = queryOrBearerToken(c.req.query('token'), c.req.header('Authorization') ?? c.req.header('authorization')); + const token = queryOrBearerToken(c.req.query('token'), c.req.header('authorization'));As per coding guidelines, “Do not introduce mixed-case field fallbacks” and “Do not add mixed-case compatibility fallbacks”.
🤖 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/engine.ts` at line 167, The token extraction in engine.ts uses a mixed-case header fallback, which violates the header naming rule. Update the queryOrBearerToken call in the engine request handler to read from one canonical Authorization header spelling only, and remove the alternate authorization fallback while keeping the existing token query support intact.Source: Coding guidelines
🤖 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/engine.ts`:
- Around line 260-268: In engine.ts, narrow the malformed-body handling in the
error response path so only explicit bad-body errors return
jsonMalformedBody(c); remove the broad error.message?.includes('JSON') check and
instead gate on the coded/status-based malformed-body condition already used in
this flow. Keep the fallback jsonError(...) path for all other errors in the
error handling branch around the existing jsonMalformedBody and jsonError calls.
In `@packages/engine/src/engine/wsAuth.ts`:
- Around line 44-46: The queryOrBearerToken function currently lets an empty
query token override a valid Authorization Bearer token because it uses nullish
coalescing. Update queryOrBearerToken in wsAuth.ts to treat empty or
whitespace-only queryToken values as missing and fall back to
extractBearerToken(authHeader) before returning a token, so valid header auth is
still used when the query parameter is empty.
- Around line 24-28: The success branch of NodeWsAuthResult still exposes the
nullable return type from getNodeByTokenHash, even though authenticateNodeWs
already filters out missing nodes. Update the node property in NodeWsAuthResult
to use a non-nullable form such as NonNullable<Awaited<ReturnType<typeof
getNodeByTokenHash>>> so authResult.node is guaranteed to be present in the
ok:true case.
- Around line 78-95: The `authenticateRealtimeWs` flow currently gates only the
`rk_live_` workspace-key path, so agent-scoped `at_live_` tokens can still open
`/v1/ws` when workspace streams are disabled. Update `authenticateRealtimeWs` in
`wsAuth.ts` so both realtime scopes run through `isWorkspaceStreamEnabled`
before returning success, using the authenticated workspace from
`deps.auth.authenticate` in the `at_live_` branch as well as the workspace-key
branch. Keep the existing invalid-token handling, but add the same not-found
response when the workspace-stream gate is closed for agent tokens.
In `@packages/engine/src/lib/httpError.ts`:
- Around line 42-45: The HTTP error helper is leaking backend `cause` details
into client responses through `messageWithCause()` and the `directory` route
path that opts into it. Update `messageWithCause()` in `httpError.ts` to return
only `error.message` for responses, and keep any `error.cause` handling confined
to logging/telemetry paths; also review the related response-building logic
around the referenced `directory` route and the `httpError` helpers to ensure no
caller still serializes the raw cause text.
In `@packages/engine/src/lib/httpQuery.ts`:
- Around line 14-20: The default handling in httpQuery’s value schema bypasses
validation because numberSchema.default(...) short-circuits parsing, so update
the logic around valueSchema to ensure options.defaultValue is validated by the
full schema. In the preprocess/numberSchema flow, switch to a path that runs the
default through parsing (for example, prefault) or explicitly validate
options.defaultValue before attaching it, so the .int(), .positive(), and
.max(...) constraints still apply.
In `@packages/engine/src/routes/action.ts`:
- Around line 53-64: The handler requirement is only checked in the
parseJsonBody error mapping, but registerActionSchema still allows handler_agent
and handler_node to be omitted, so the request can pass validation without a
handler. Update registerActionSchema to enforce that at least one handler field
is present (or make the required/conditional constraint explicit in the schema)
so /actions fails early with the correct validation response; keep the
parseJsonBody callback in action.ts aligned with the schema by preserving the
handler_agent/handler_node error message path.
In `@packages/engine/src/routes/directory.ts`:
- Around line 84-86: The directory route’s error handler is leaking internal
failure details by passing includeCause: true into errorResponse. Update
handleError in directory.ts to return a client-safe response without including
the underlying cause, and keep any detailed cause information out of the HTTP
body while preserving normal error handling via errorResponse.
In `@packages/engine/src/routes/message.ts`:
- Line 48: The validation message for postMessageSchema parsing is too generic
and masks which field actually failed. Update the message handling in message.ts
around parseJsonBody so schema-specific failures for fields like mode,
attachments, blocks, data, and content_type return the correct validation text
instead of always using text is required. Use the parseJsonBody call in the
message route and the postMessageSchema validation path to identify the failing
field and map it to an appropriate error message.
In `@packages/engine/src/routes/thread.ts`:
- Line 37: The validation error handling in the reply route is too specific
because `postReplySchema` validates more than just text, so the fixed message
does not match failures from `blocks` or `data`. Update the `parseJsonBody` call
in the thread reply handler to use a generic reply-body validation message
instead of `text is required`, keeping the change scoped to the route logic that
uses `postReplySchema`.
In `@packages/engine/src/routes/workspace.ts`:
- Around line 50-52: The activity feed limit in activityQuerySchema is currently
unbounded, and getActivityFeed is using the parsed limit directly, so add a
maximum cap before querying. Update the limit definition in the workspace route
to match the bounded pattern used elsewhere (for example, the session events
route), and ensure the value passed from the activity feed handler into
getActivityFeed cannot exceed that cap.
---
Outside diff comments:
In `@packages/engine/src/routes/workspace.ts`:
- Around line 45-57: The updateWorkspaceStreamSchema and updateFleetNodesSchema
currently allow mixed payloads where both enabled and mode can be provided, and
the route handlers then let mode: "inherit" override enabled. Tighten the Zod
validation in packages/engine/src/routes/workspace.ts so the workspaceStream and
fleetNodes routes only accept one exclusive shape: either { enabled: boolean }
or { mode: "inherit" }, but never both together. Update the route
parsing/handling in the corresponding workspace stream and fleet-nodes endpoints
to rely on these stricter schemas and reject mixed payloads before applying any
override logic.
---
Nitpick comments:
In `@packages/engine/src/engine.ts`:
- Line 167: The token extraction in engine.ts uses a mixed-case header fallback,
which violates the header naming rule. Update the queryOrBearerToken call in the
engine request handler to read from one canonical Authorization header spelling
only, and remove the alternate authorization fallback while keeping the existing
token query support intact.
In `@packages/engine/src/routes/search.ts`:
- Around line 14-18: The search route currently validates q with a manual
post-parse guard instead of the schema. Move the trim/non-empty requirement into
searchQuerySchema by making q a Zod string refinement rather than an optional
plain string, and then remove the ad-hoc q check in the search handler logic so
validation stays centralized in the schema.
🪄 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: e32ea86f-5010-4945-95fe-e78d36130baf
📒 Files selected for processing (33)
packages/engine/src/__tests__/conformance/httpResponse.test.tspackages/engine/src/engine.tspackages/engine/src/engine/wsAuth.tspackages/engine/src/entrypoints/node.tspackages/engine/src/lib/__tests__/httpError.test.tspackages/engine/src/lib/httpError.tspackages/engine/src/lib/httpQuery.tspackages/engine/src/lib/httpResponse.tspackages/engine/src/middleware/auth.tspackages/engine/src/middleware/fleetNodes.tspackages/engine/src/middleware/idempotency.tspackages/engine/src/middleware/planLimits.tspackages/engine/src/middleware/rateLimit.tspackages/engine/src/routes/a2a.tspackages/engine/src/routes/action.tspackages/engine/src/routes/agent.tspackages/engine/src/routes/certify.tspackages/engine/src/routes/channel.tspackages/engine/src/routes/console.tspackages/engine/src/routes/delivery.tspackages/engine/src/routes/directory.tspackages/engine/src/routes/dm.tspackages/engine/src/routes/file.tspackages/engine/src/routes/groupDm.tspackages/engine/src/routes/inboundWebhook.tspackages/engine/src/routes/inbox.tspackages/engine/src/routes/message.tspackages/engine/src/routes/presence.tspackages/engine/src/routes/reaction.tspackages/engine/src/routes/routing.tspackages/engine/src/routes/search.tspackages/engine/src/routes/thread.tspackages/engine/src/routes/workspace.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/engine/src/__tests__/nodeUpgradeAuth.test.ts (1)
47-53: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winResolve failed socket errors in
tryUpgrade.Line 52 can leave the Promise pending when the WS client emits
errorwithoutunexpected-response, causing a hung test instead of a clear failure.Proposed fix
function tryUpgrade(headers: Record<string, string>, query = '', path = '/v1/node/ws'): Promise<number> { return new Promise((resolve) => { + let settled = false; + const finish = (status: number) => { + if (!settled) { + settled = true; + resolve(status); + } + }; const ws = new WebSocket(`${wsBase}${path}${query}`, { headers }); - ws.on('open', () => { ws.close(); resolve(101); }); - ws.on('unexpected-response', (_req, res) => { resolve(res.statusCode ?? 0); }); - ws.on('error', () => { /* handled by unexpected-response */ }); + ws.on('open', () => { ws.close(); finish(101); }); + ws.on('unexpected-response', (_req, res) => { finish(res.statusCode ?? 0); }); + ws.on('error', () => { finish(0); }); }); }🤖 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/__tests__/nodeUpgradeAuth.test.ts` around lines 47 - 53, The tryUpgrade helper can hang because it only resolves on open or unexpected-response, so a WebSocket error path may leave the Promise pending. Update tryUpgrade in nodeUpgradeAuth.test.ts to handle the WebSocket client's error event by resolving or rejecting the Promise there as well, and make sure the ws.on('error') branch does not just swallow the failure when unexpected-response is never emitted.
🤖 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.
Outside diff comments:
In `@packages/engine/src/__tests__/nodeUpgradeAuth.test.ts`:
- Around line 47-53: The tryUpgrade helper can hang because it only resolves on
open or unexpected-response, so a WebSocket error path may leave the Promise
pending. Update tryUpgrade in nodeUpgradeAuth.test.ts to handle the WebSocket
client's error event by resolving or rejecting the Promise there as well, and
make sure the ws.on('error') branch does not just swallow the failure when
unexpected-response is never emitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 50dec171-a372-4d82-8f60-b2a9d89a6161
📒 Files selected for processing (14)
packages/engine/src/__tests__/conformance/httpResponse.test.tspackages/engine/src/__tests__/nodeUpgradeAuth.test.tspackages/engine/src/engine.tspackages/engine/src/engine/wsAuth.tspackages/engine/src/lib/__tests__/httpError.test.tspackages/engine/src/lib/httpError.tspackages/engine/src/lib/httpQuery.tspackages/engine/src/lib/httpResponse.tspackages/engine/src/routes/action.tspackages/engine/src/routes/directory.tspackages/engine/src/routes/message.tspackages/engine/src/routes/search.tspackages/engine/src/routes/thread.tspackages/engine/src/routes/workspace.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/engine/src/lib/httpQuery.ts
- packages/engine/src/routes/search.ts
- packages/engine/src/engine.ts
- packages/engine/src/lib/httpResponse.ts
- packages/engine/src/routes/directory.ts
- packages/engine/src/routes/message.ts
- packages/engine/src/routes/action.ts
- packages/engine/src/routes/thread.ts
- packages/engine/src/routes/workspace.ts
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.
Summary
This continues the engine architecture refactor after the already-merged response-helper PR.
/healthraw JSON and A2A JSON-RPC request parsingImpact
The HTTP wire shape stays consistent with the existing success/error envelopes while reducing repeated route-local parsing and rendering code. WebSocket auth and idempotent replay behavior now have single implementation points, which should make future API and auth changes easier to audit.
Validation
git diff --checknpm --workspace @relaycast/engine run typechecknpm --workspace @relaycast/engine run lintnpm --workspace @relaycast/engine run buildnpm --workspace @relaycast/engine test/healthand A2A JSON-RPC protocol pathspackages/engine/src/lib/httpQuery.tsNotes
This branch was recreated from current
origin/mainbecause the prior local branch name belonged to merged PR #207 and was based before newer release/SDK commits. Recreating the branch avoids unrelated reversions in the PR diff.