refactor(engine): tighten types & validation, remove cast tech debt#150
Conversation
Engine review pass focused on `any`/`as` casts and input validation.
Net -280 LOC; casts down 250 -> ~50 (all remaining are framework
status casts or guarded `unknown` narrowing).
Error handling
- Add src/lib/httpError.ts: errorResponse(c, err), asCodedError(err),
codedError(msg, code, status). Narrows thrown values via `instanceof`
instead of the unchecked `err as Error & {...}` assertion that was
copy-pasted into 93 catch blocks across 23 files.
- Replace those catch blocks with errorResponse(c, err); blocks that also
log/emit telemetry keep their logic but use asCodedError.
- Convert coded-error throw sites (action.ts, inboundWebhook.ts) to the
cast-free codedError()/Object.assign form.
Schema typing flows through
- Add .$type<Record<string, unknown>>() to the metadata/capabilities JSON
columns. Lets ~30 `row.metadata as Record<string, unknown>` casts become
clean `?? {}`. `.default('{}')` -> `.default({})` serializes to the same
DEFAULT '{}' SQL (no migration change).
Validation at parse boundaries (zod)
- files.ts: HMAC-verified download-token payload now safeParse'd, so a
malformed exp can't yield a non-expiring token.
- certify.ts: the three probe tests that walked external A2A responses via
chained `as Record<string, unknown>` casts now use focused zod schemas.
EngineDb is generic, future-proofed per adapter
- EngineDb<TRunResult = unknown>: core stays driver-agnostic; Node adapter
specializes EngineDb<RunResult> (better-sqlite3), so run-result types flow
through on that side without coupling core to a platform. Removes the last
real `any` and its eslint-disable.
Verified: turbo lint (14/14), turbo test (19/19), local:parity (103
endpoints), engine build — all pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 49 minutes and 13 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. 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 ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR refactors error handling and type safety across the engine. It introduces centralized HTTP error utilities, generalizes the database type system to support driver-specific types, adds runtime validation for external data via Zod schemas, and consolidates scattered error responses across 20+ route handlers into a single standardized pattern. ChangesCore Platform Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 refactors error handling across the engine by introducing a centralized CodedError interface and helper functions (asCodedError, codedError, errorResponse) in a new httpError.ts utility, replacing repetitive try-catch error response formatting in various route files. It also improves type safety by generifying the EngineDb port, using Zod schemas to defensively parse untrusted external A2A responses and token payloads, and refining metadata and capability field types in the database schema. Feedback was provided to make the asCodedError helper more robust when handling plain objects thrown as errors, ensuring custom properties like code or status are preserved.
| export function asCodedError(err: unknown): CodedError { | ||
| if (err instanceof Error) return err as CodedError; | ||
| return new Error(typeof err === 'string' ? err : 'Unknown error'); | ||
| } |
There was a problem hiding this comment.
When err is a plain object (e.g., from serialized error payloads or third-party libraries that do not throw real Error instances), the current implementation of asCodedError discards all custom properties (such as code, status, cause, or data) and returns a generic new Error('Unknown error'). We can make this much more robust by checking if err is a non-null object and using Object.assign to preserve all of its custom properties on the newly created Error instance, while ensuring the message property is safely coerced to a string.
| export function asCodedError(err: unknown): CodedError { | |
| if (err instanceof Error) return err as CodedError; | |
| return new Error(typeof err === 'string' ? err : 'Unknown error'); | |
| } | |
| export function asCodedError(err: unknown): CodedError { | |
| if (err instanceof Error) return err as CodedError; | |
| if (err && typeof err === 'object' && !Array.isArray(err)) { | |
| const message = 'message' in err ? String(err.message) : 'Unknown error'; | |
| const error = Object.assign(new Error(message), err); | |
| error.message = message; | |
| return error; | |
| } | |
| return new Error(typeof err === 'string' ? err : 'Unknown error'); | |
| } |
There was a problem hiding this comment.
Good catch — fixed in e1d9296. asCodedError now wraps non-Error plain objects with Object.assign, preserving code/status/cause (and coercing message to a string), so a thrown { code, status, message } still surfaces those fields exactly as the old err as Error & {...} reads did. Excluded arrays so their indices are not copied onto the Error.
|
Preview deployed!
This preview shares the staging database and will be cleaned up when the PR is merged or closed. Run E2E testsnpm run e2e -- https://pr150-api.relaycast.dev --ciOpen observer dashboard |
![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
🧹 Nitpick comments (2)
packages/engine/src/engine/inboundWebhook.ts (1)
170-173: 💤 Low valueConsider using the
codedError()helper for these thrown engine errors.
codedError(message, code, status)is exported and matches the manualObject.assign(new Error(...), { code, status })pattern used ininboundWebhook.ts(both thewebhook_no_agentandwebhook_agent_provision_failedcases). Swap those tothrow codedError(...)for consistency (and add the import if missing).🤖 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/inboundWebhook.ts` around lines 170 - 173, Replace the manual Object.assign(new Error(...), { code, status }) patterns in inboundWebhook.ts (the errors for 'webhook_no_agent' and 'webhook_agent_provision_failed') with the exported helper codedError(message, code, status); specifically, remove the Object.assign constructions and throw codedError(...) instead, and add the codedError import at the top if it's not already present to keep error creation consistent.packages/engine/src/routes/a2a.ts (1)
148-154: ⚡ Quick winReuse
errorResponse(c, err)for the plain HTTP-JSON catch blocks inpackages/engine/src/routes/a2a.ts.
- The five plain HTTP-JSON catches at lines 148-154, 169-175, 183-189, 204-210, and 263-269 are byte-identical to the
{ ok:false, error:{ code: error.code || 'internal_error', message: error.message } }+(error.status || 500)envelope thaterrorResponse(c, err)already builds.- Update those catch blocks to
return errorResponse(c, err);and adderrorResponseto the existing import (../lib/httpError.js) alongsideasCodedError.- Keep the JSON-RPC catches (lines 321 and 428) on the
asCodedError(...) as CodedError & { data?: unknown }path since they need the JSON-RPCjsonRpcErrorenvelope anderror.data.♻️ Example for one route
- } catch (err: unknown) { - const error = asCodedError(err); - return jsonResponse(c, { - ok: false, - error: { code: error.code || 'internal_error', message: error.message }, - }, error.status || 500); - } + } catch (err: unknown) { + return errorResponse(c, err); + }🤖 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/a2a.ts` around lines 148 - 154, Replace the five duplicate plain HTTP-JSON catch blocks in a2a.ts with the helper call errorResponse(c, err): import errorResponse alongside asCodedError from ../lib/httpError.js, then change each of the catch blocks at the locations noted to simply return errorResponse(c, err); do not modify the JSON-RPC handlers that use asCodedError(...) as CodedError & { data?: unknown } because they still need the jsonRpcError envelope and error.data.
🤖 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/certify.ts`:
- Around line 328-333: The current check in the certify logic (where
jsonRpcEnvelopeSchema.safeParse(response.body) is assigned to envelope and
hasPayload is computed) incorrectly treats envelopes with both result and error
as valid; change the payload test to require exactly one branch present (result
XOR error) — i.e., set hasPayload to envelope.success && ((envelope.data.result
!== undefined) !== (envelope.data.error !== undefined)) — and update the
returned message to reflect whether a single JSON-RPC payload branch was found
while keeping details: { body: response.body, parse_error: response.parse_error
} unchanged.
---
Nitpick comments:
In `@packages/engine/src/engine/inboundWebhook.ts`:
- Around line 170-173: Replace the manual Object.assign(new Error(...), { code,
status }) patterns in inboundWebhook.ts (the errors for 'webhook_no_agent' and
'webhook_agent_provision_failed') with the exported helper codedError(message,
code, status); specifically, remove the Object.assign constructions and throw
codedError(...) instead, and add the codedError import at the top if it's not
already present to keep error creation consistent.
In `@packages/engine/src/routes/a2a.ts`:
- Around line 148-154: Replace the five duplicate plain HTTP-JSON catch blocks
in a2a.ts with the helper call errorResponse(c, err): import errorResponse
alongside asCodedError from ../lib/httpError.js, then change each of the catch
blocks at the locations noted to simply return errorResponse(c, err); do not
modify the JSON-RPC handlers that use asCodedError(...) as CodedError & { data?:
unknown } because they still need the jsonRpcError envelope and error.data.
🪄 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: 24e012ad-7441-4f1c-b70b-4b75de2bfb82
📒 Files selected for processing (38)
packages/engine/src/adapters/node/database.tspackages/engine/src/adapters/node/files.tspackages/engine/src/db/schema.tspackages/engine/src/engine.tspackages/engine/src/engine/action.tspackages/engine/src/engine/agent.tspackages/engine/src/engine/certify.tspackages/engine/src/engine/channel.tspackages/engine/src/engine/console.tspackages/engine/src/engine/directory.tspackages/engine/src/engine/dm.tspackages/engine/src/engine/inboundWebhook.tspackages/engine/src/engine/message.tspackages/engine/src/engine/thread.tspackages/engine/src/lib/httpError.tspackages/engine/src/ports/database.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/directory.tspackages/engine/src/routes/dm.tspackages/engine/src/routes/eventSubscription.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/receipt.tspackages/engine/src/routes/routing.tspackages/engine/src/routes/search.tspackages/engine/src/routes/systemPrompt.tspackages/engine/src/routes/thread.tspackages/engine/src/routes/workspace.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 38 files
You’re at about 90% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- httpError: asCodedError now preserves properties of thrown plain
objects (code/status/message), matching the prior `err as Error & {...}`
reads it replaces, instead of flattening them to a generic Error.
(gemini-code-assist)
- certify: jsonrpc_shape test now requires exactly one of result/error
(XOR) per JSON-RPC 2.0, rather than accepting both. (coderabbitai)
- engine/inboundWebhook: use the codedError() helper for both throw
sites for consistency. (coderabbitai)
Declined: routing a2a.ts's plain-HTTP catch blocks through errorResponse
— they use the file's `jsonResponse` helper (raw Response,
`application/json`), not Hono `c.json` (`application/json; charset=UTF-8`),
so they are not equivalent and the file uses jsonResponse uniformly.
Verified: engine typecheck, eslint --max-warnings 0, vitest (15/15).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review feedback addressed —
|
| # | Source | Item | Action |
|---|---|---|---|
| 1 | gemini, cubic | asCodedError dropped code/status on plain-object throws |
Fixed — preserves own props via Object.assign, coerces message |
| 2 | coderabbit | jsonrpc_shape accepted both result+error |
Fixed — now requires exactly one (XOR) per JSON-RPC 2.0 |
| 3 | coderabbit (nitpick) | engine/inboundWebhook.ts should use codedError() |
Applied — both throw sites now use the helper |
| 4 | coderabbit (nitpick) | route a2a catches through errorResponse |
Declined (see below) |
On #4: the five plain-HTTP catches in routes/a2a.ts use the file's local jsonResponse helper — a raw new Response(JSON.stringify(...), { headers: { 'Content-Type': 'application/json' } }) — not Hono's c.json, which sets application/json; charset=UTF-8. They're therefore not byte-identical to errorResponse, and the a2a routes use jsonResponse uniformly for every response. Routing only the error path through c.json would change the Content-Type and break that consistency, so I kept them on asCodedError + jsonResponse.
All checks green locally: engine tsc --noEmit, eslint --max-warnings 0, vitest (15/15).
1 participant
Summary
A review pass over
packages/enginefocused on type-safety tech debt —any/ascasts — and input validation. The codebase was already disciplined (every body-reading route uses zodsafeParse; no: any/as any), so this is mostly about removing duplicated unsafe casts and tightening the few real parse boundaries.Net −280 LOC. Casts: 250 → ~50 (all remaining are framework status casts or guarded
unknownnarrowing). Zeroanytypes left in the package.Changes
Error handling — one helper instead of 93 copies
src/lib/httpError.ts:errorResponse(c, err),asCodedError(err),codedError(msg, code, status). These narrow thrown values with a realinstanceof Errorcheck instead of the uncheckederr as Error & { code?: string; status?: number }assertion that was copy-pasted into 93 catch blocks across 23 files.return errorResponse(c, err);. Blocks that also log/emit telemetry (workspace stream config, globalonError, a2a JSON-RPC envelopes) keep their logic but useasCodedError.action.ts,inboundWebhook.ts) now use the cast-freecodedError()/Object.assignform.Schema types flow through
.$type<Record<string, unknown>>()to the metadata/capabilities JSON columns indb/schema.ts, so ~30row.metadata as Record<string, unknown>casts become clean?? {}..default('{}')→.default({})serializes to the identicalDEFAULT '{}'SQL (same patterninputSchema/outputSchemaalready used) — no migration change.Validation at parse boundaries (zod)
adapters/node/files.ts: the HMAC-verified download-token payload is nowsafeParse'd — a malformedexpcan no longer yield a non-expiring token.engine/certify.ts: the three probe tests that walked external A2A responses via chainedas Record<string, unknown>casts now use focused zod schemas, preserving the original lenient semantics.EngineDbis generic, future-proofed per adapterEngineDb<TRunResult = unknown>: engine core stays driver-agnostic (EngineDb<unknown>, never calls.run()), while the Node adapter specializesEngineDb<RunResult>(better-sqlite3) so run-result types flow through there without coupling core to a platform. A future Cloudflare adapter would declareEngineDb<D1Result>in its own package. Removes the last realanyand itseslint-disable.Deliberately left as-is
idempotency.tsJSON.parse as T(×3) — the app's own cache, genericT, no fixed schema to validate against.a2a.tsparam-walk casts anddirectory.tsskill/capability casts — narrowingunknownaftertypeof === 'object'guards on request/derived data, not DB rows.as ContentfulStatusCode— genuine success-path Hono status casts, plus the one centralized in the helper.Verification
npm run local:parity— passes (103 endpoints)npx turbo lint --force— 14/14npx turbo test --force— 19/19 (DO_NOT_TRACK=1)npx turbo build— all packages🤖 Generated with Claude Code