Repository navigation
Conversation
computerRoutes()'s error handler mapped every thrown error to HTTP 400, including domain/service-state failures that are not the client's fault: an unconfigured computer service, an unknown Dot id, a disabled permission, or the computer supervisor being unreachable all came back as 400 Bad Request. That misclassifies retryable/server-side failures as client input errors and is inconsistent with workspace-routes.ts's existing convention, which reserves 400 for actual validation problems (malformed JSON, Zod failures) and uses 503 for everything else. Also added the malformed-JSON carve-out workspace-routes.ts already has (SyntaxError from a bad request body was previously falling through to the generic case, which is still correctly 400 here, but only by coincidence of being the blanket default -- now it's explicit and covered by a regression test, matching the actions route's /dots/:id/ computer/actions endpoint which is the only one that parses JSON input beyond the route param). Covered by a new tests/computer-routes.test.ts (computer-routes.ts had no direct test coverage before this), exercising: malformed JSON (400), a Zod validation failure (400), an unconfigured service (503), and an unknown Dot id (503).
NathanTarbert
left a comment
NathanTarbert
left a comment
There was a problem hiding this comment.
Thanks @Ashfaqbs, and thanks for adding the first tests for computer-routes.ts. Getting upstream outages off 400 is the right call, and the malformed-JSON and Zod cases read well.
One thing worth adding, because it changes the shape of the fix. Several errors from ComputerService describe the request rather than the service. With this change they also come back as 503. "Unknown computer action." comes from an action name the caller sent. "Computer permission is disabled." and "Dot not found." are the same. A rejected upstream request, like reading a file that doesn't exist, surfaces as "Computer service returned HTTP 400." and now becomes a 503 as well. Someone calling the API would read all of these as "try again later", and none of them will succeed on retry.
The rest of the server already has codes for these. app.ts answers "Research is disabled in Settings." with 403 and "Task not found." with 404. One option is a small mapping in onError, the same way workspace-routes.ts matches known messages. That could be 404 for "Dot not found.", 403 for the permission and owner-only errors, 409 for "Start this Dot's computer first." and "There is no active control request.", and 400 for "Unknown computer action.". Everything else would stay 503 as you have it. The unknown-Dot test would then expect 404.
Happy to go with a different split if you'd rather keep this PR narrow. Even just moving "Unknown computer action." and "Dot not found." would cover the cases most likely to trip someone up.
Several ComputerService errors describe the caller's request rather than a service outage (unknown action, missing Dot, disabled permission, owner-only control, no computer running, no active control request). Returning 503 for these made them look retryable when they are not. Map them explicitly in onError: 404 for Dot not found, 403 for permission and owner-only errors, 409 for computer-not-running and no-active-request, 400 for unknown action. Everything else stays 503.
|
Went with the full mapping: 404 for "Dot not found.", 403 for "Computer permission is disabled." and "Human controls are owner-only.", 409 for "Start this Dot's computer first." and "There is no active control request.", 400 for "Unknown computer action.". Everything else stays 503. Updated the unknown-Dot test to expect 404 and added tests for the 400/403 cases; full suite (162 tests) passes. |
jerelvelarde
left a comment
jerelvelarde
left a comment
There was a problem hiding this comment.
The change improves API behavior by distinguishing missing Dots (404), disabled permissions (403), inactive control state (409), malformed requests (400), and ordinary service failures (503). The inline finding identifies a reachable upstream-validation case that still returns 400. Repository fit and security boundaries otherwise look sound: no dependencies/configuration changes, and permissions, owner authentication, endpoint binding, audit, and secret sanitization are preserved. Please also update the description: it still says unknown Dots and permissions return 503 and lists four new tests; this head implements additional mappings and six route tests. Verification: exact head, 16 focused route/service tests passed; changed files pass Prettier. A clean prospective merge with main 1b2425d passes the same 16 tests and reproduces the inline finding. Full repository gates are being handled separately; no live provider calls were made.
| app.onError((error, c) => { | ||
| if (error instanceof SyntaxError) | ||
| return c.json({ error: 'Invalid JSON request.' }, 400); | ||
| if (error instanceof z.ZodError) |
There was a problem hiding this comment.
[P2] Distinguish upstream schema errors from invalid caller input
ComputerService also uses Zod to validate supervisor/computer responses (endpoint, existing, and control), so this branch incorrectly treats service failures as malformed requests. With enabled permissions and a valid POST /dots/:id/computer/start, a mocked supervisor returning HTTP 200 with {} makes stateSchema.parse throw, and this handler returns 400 Invalid computer request.; a valid read action with {computers:[{botId:id}]} has the same result. The caller cannot fix either request, and the service failure still gets reported as the client error this PR intends to correct. Separate input-validation errors from upstream-response validation (e.g. wrap upstream schema failures in a service-domain error) so those cases reach 503, while invalid request bodies keep 400; add route regressions for both sources.
ComputerService also uses Zod to validate the supervisor's own responses (stateSchema, controlSchema), so onError's generic ZodError check was catching those upstream failures too and reporting them as 400 Invalid computer request, the same as a bad request body. A mocked supervisor returning 200 with a malformed body made start/read/take all look like caller mistakes when they were service failures. Wrap every upstream-response parse in parseUpstream, which rethrows a schema mismatch as a plain Error instead of a ZodError, so onError's fallback (503) applies. Caller-input Zod parses (the actions body, permissions patch, per-action input schema) are untouched and still map to 400.
|
@NathanTarbert @jerelvelarde thanks both, pushed a7f64f1 addressing this. For the status-code mapping (@NathanTarbert): added explicit codes in For the upstream-schema finding (@jerelvelarde): you were right that Also fixed the PR description, which still described the old (503-only) version of this change and the old test count. 164 tests passing, lint/typecheck/build all clean. Re-requesting review on both of your threads. |
3 participants
Summary
computerRoutes()'sonErrorhandler mapped every thrown error to HTTP 400, including domain/service-state failures that aren't the client's fault: an unconfigured computer service, an unknown Dot id, a disabled permission, or the computer supervisor being unreachable all came back as400 Bad Request.This is inconsistent with
workspace-routes.ts's existing convention, which reserves 400 for actual validation problems (malformed JSON, Zod failures) and falls back to 503 for everything else (see itsonError, and the recent #11 fix for the same class of issue).computer-routes.tshad no direct test coverage before this PR, so the gap wasn't caught.Changes
src/server/computer-routes.ts: Zod validation errors and malformed-JSON (SyntaxError) still return 400. Request-describing domain errors now get their own codes instead of a blanket 503: 404 for an unknown Dot, 403 for a disabled permission or owner-only action, 409 for "computer not running" / "no active control request", 400 for an unknown action name. Everything else (service-not-configured, upstream supervisor failures) stays 503.src/server/computer-service.ts: the service also uses Zod to validate the supervisor's own responses (stateSchema,controlSchema). A newparseUpstreamhelper wraps those parses so a malformed upstream body throws a plainErrorinstead of aZodError— otherwiseonError's blanketZodErrorcheck misclassified a service failure as a caller input error (400 instead of 503). Caller-input Zod parses (the actions body, permissions patch, per-action input schema) are untouched.tests/computer-routes.test.ts: covers malformed JSON (400), an invalid action shape (400), an unconfigured service (503), an unknown Dot (404), an unknown action (400), a disabled permission (403), and two upstream-schema-failure cases (503, not 400) — 8 tests total, 164 passing in the full suite.Test plan
npm test— 164 tests passing, no regressions.npm run lint— clean.npm run typecheck— clean.npm run check-format— clean on changed files.npm run build— succeeds.Assisted by Claude Code (Anthropic) during investigation and implementation; reviewed and tested by me before opening this PR.