refactor: consolidate error handling with ApiError class#212
refactor: consolidate error handling with ApiError class#212justinchuby wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #212 +/- ##
==========================================
+ Coverage 73.31% 73.44% +0.12%
==========================================
Files 422 424 +2
Lines 26853 26822 -31
Branches 8083 8091 +8
==========================================
+ Hits 19688 19700 +12
+ Misses 4819 4775 -44
- Partials 2346 2347 +1
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:
|
There was a problem hiding this comment.
Pull request overview
This PR centralizes HTTP error handling in the server by introducing a typed ApiError + apiErrorHandler middleware, then migrating route handlers and tests away from manual res.status().json() error responses.
Changes:
- Added
ApiError(with helper factories) and anapiErrorHandlerExpress error middleware to standardize error envelopes. - Migrated multiple route handlers to
throw badRequest()/notFound()/...(andApiError) instead of returning ad-hoc error JSON. - Updated route and integration tests to mount
apiErrorHandlerand assert the new error response shape.
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/server/src/routes/tasks.ts | Replace manual 400/404 responses with thrown ApiError helpers. |
| packages/server/src/routes/tasks.test.ts | Mount apiErrorHandler in test server to handle thrown errors. |
| packages/server/src/routes/summary.ts | Convert query validation and 500 handling to ApiError throws. |
| packages/server/src/routes/shared.ts | Convert 4xx/5xx responses to thrown ApiError helpers. |
| packages/server/src/routes/settings.ts | Convert provider/settings error responses to thrown ApiError helpers. |
| packages/server/src/routes/settings.test.ts | Mount apiErrorHandler in test server. |
| packages/server/src/routes/services.ts | Convert many inline res.status(...).json(...) branches to throws. |
| packages/server/src/routes/search.ts | Convert query validation errors to badRequest() throws. |
| packages/server/src/routes/roles.ts | Convert missing-field validation to badRequest() throws. |
| packages/server/src/routes/replay.ts | Convert replay errors (503/400/500) to thrown ApiError helpers. |
| packages/server/src/routes/projects.ts | Replace numerous route error responses with thrown ApiError helpers (and add structured details in some cases). |
| packages/server/src/routes/projects.test.ts | Mount apiErrorHandler in test server. |
| packages/server/src/routes/predictions.ts | Convert 4xx/5xx responses to thrown ApiError helpers. |
| packages/server/src/routes/nl.ts | Convert validation and execution/preview errors to thrown ApiError with some explicit pass-through. |
| packages/server/src/routes/lead.ts | Convert several route errors to thrown ApiError helpers; adjust catch blocks. |
| packages/server/src/routes/knowledge.ts | Convert knowledge route errors (400/403/404/503) to thrown helpers. |
| packages/server/src/routes/integrations.ts | Refactor endpoints to rely on thrown ApiError helpers and reduce try/catch scaffolding. |
| packages/server/src/routes/integrations.test.ts | Mount apiErrorHandler in test server. |
| packages/server/src/routes/diff.ts | Convert missing-agent and 500 errors to thrown ApiError. |
| packages/server/src/routes/decisions.ts | Convert missing fields/resources to badRequest()/notFound() throws. |
| packages/server/src/routes/db.ts | Convert invalid ID checks to thrown badRequest(). |
| packages/server/src/routes/data.ts | Convert validation + 500 handling to thrown ApiError (with ApiError pass-through). |
| packages/server/src/routes/crew.ts | Convert many roster/crew errors to thrown helpers and remove some try/catch blocks. |
| packages/server/src/routes/coordination.ts | Convert traversal/validation errors to badRequest() and lock conflicts to conflict(). |
| packages/server/src/routes/config.ts | Convert missing-config-store and validation errors to thrown helpers. |
| packages/server/src/routes/comms.ts | Convert 500 handling to thrown ApiError. |
| packages/server/src/routes/browse.ts | Convert browse validation/forbidden errors to thrown helpers. |
| packages/server/src/routes/analytics.ts | Convert 400/500 handling to thrown ApiError helpers. |
| packages/server/src/routes/agents.ts | Convert many missing-resource/validation errors to thrown helpers; simplify /agents/:id/tasks handler. |
| packages/server/src/middleware/errorHandler.ts | Add centralized Express error middleware to serialize ApiError and handle unexpected errors. |
| packages/server/src/middleware/tests/errorHandler.test.ts | Add middleware integration tests for apiErrorHandler. |
| packages/server/src/index.ts | Replace inline global 500 handler with apiErrorHandler. |
| packages/server/src/errors/index.ts | Add barrel export for ApiError and helper functions. |
| packages/server/src/errors/tests/ApiError.test.ts | Add unit tests for ApiError, helpers, and requireParam(). |
| packages/server/src/errors/ApiError.ts | Introduce ApiError class, helper factories, and requireParam(). |
| packages/server/src/tests/dataRoutes.test.ts | Update tests to expect thrown ApiError instead of direct res status/body mutation. |
| packages/server/src/tests/data-cleanup.test.ts | Mount apiErrorHandler in integration-style cleanup test app. |
| packages/server/src/tests/api.integration.test.ts | Mount apiErrorHandler and update assertions for new error envelope shape. |
| packages/server/src/tests/ModelConfigRoutes.test.ts | Mount apiErrorHandler for project routes integration tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { apiErrorHandler } from '../errorHandler.js'; | ||
|
|
||
| // Suppress logger output in tests | ||
| vi.mock('../utils/logger.js', () => ({ |
There was a problem hiding this comment.
This test mocks ../utils/logger.js, but from src/middleware/__tests__ that path resolves to src/middleware/utils/logger.js (which doesn’t exist). As a result, the logger used by errorHandler won’t be mocked (and the mock may fail to resolve). Update the mock path to target src/utils/logger.js (matching how httpLogger.test.ts mocks it).
| vi.mock('../utils/logger.js', () => ({ | |
| vi.mock('../../utils/logger.js', () => ({ |
| res.json(statuses); | ||
| } catch (err: any) { | ||
| res.status(500).json({ error: err.message || 'Failed to detect provider statuses' }); | ||
| throw internalError((err as Error).message || 'Failed to detect provider statuses'); | ||
| } |
There was a problem hiding this comment.
This converts the 500 path to internalError((err as Error).message || ...), which will return the underlying exception message to the client. That reintroduces the kind of information leak the centralized handler is meant to avoid. Prefer a generic client-facing message (e.g., 'Failed to detect provider statuses') and log err server-side for diagnostics.
| res.json(summary); | ||
| } catch (err) { | ||
| res.status(500).json({ error: 'Failed to generate summary', detail: (err as Error).message }); | ||
| throw new ApiError(500, 'Failed to generate summary', { details: (err as Error).message }); | ||
| } |
There was a problem hiding this comment.
These 500s wrap the caught exception message into { details: err.message }, which will be returned to the client by apiErrorHandler. If the goal is to avoid leaking internal errors, don’t include err.message in the response for 5xx (log it instead), or rely on internalError() with a generic message.
| state, | ||
| }); | ||
| } catch (err) { | ||
| res.status(500).json({ error: 'Failed to load shared replay', detail: (err as Error).message }); | ||
| throw new ApiError(500, 'Failed to load shared replay', { details: (err as Error).message }); | ||
| } |
There was a problem hiding this comment.
These ApiError(500, ..., { details: (err as Error).message }) throws will expose internal exception messages to clients (via err.toJSON()). Consider omitting details for 5xx and logging the original error, or switching to internalError('Failed to ...') without passing err.message through to the client.
| res.json(state); | ||
| } catch (err) { | ||
| res.status(500).json({ error: 'Failed to reconstruct state', detail: (err as Error).message }); | ||
| throw new ApiError(500, 'Failed to reconstruct state', { details: (err as Error).message }); | ||
| } |
There was a problem hiding this comment.
ApiError(500, ..., { details: (err as Error).message }) will include the underlying exception message in the JSON response. If this is not intended to be user-visible (and may contain sensitive internal info), drop details for 5xx and log the real error server-side.
| } catch (err: any) { | ||
| res.status(500).json({ error: 'Failed to purge data', detail: (err as Error).message }); | ||
| if (err instanceof ApiError) throw err; | ||
| throw new ApiError(500, 'Failed to purge data', { details: (err as Error).message }); |
There was a problem hiding this comment.
In this catch-all, unexpected errors are wrapped into ApiError(500, ..., { details: err.message }), which will expose internal error messages to clients. Prefer logging the original error and returning a generic 500 without details (or ensure apiErrorHandler strips details for 5xx).
| throw new ApiError(500, 'Failed to purge data', { details: (err as Error).message }); | |
| // Log the original error internally and return a generic 500 to the client | |
| console.error('Failed to purge data', err); | |
| throw new ApiError(500, 'Failed to purge data'); |
| _next: NextFunction, | ||
| ): void { | ||
| if (res.headersSent) return; |
There was a problem hiding this comment.
apiErrorHandler currently returns early when res.headersSent is true. Express’ recommended pattern is to delegate to next(err) in this case; otherwise the request can hang if headers were sent but the response wasn’t finished (e.g., streaming). Consider if (res.headersSent) return _next(err);.
| _next: NextFunction, | |
| ): void { | |
| if (res.headersSent) return; | |
| next: NextFunction, | |
| ): void { | |
| if (res.headersSent) return next(err); |
| if (err.status >= 500) { | ||
| logger.error({ module: 'api', msg: err.message, status: err.status }); | ||
| } | ||
| res.status(err.status).json(err.toJSON()); |
There was a problem hiding this comment.
The middleware serializes ApiError via err.toJSON() even for 5xx. Several routes are now throwing new ApiError(500, ..., { details: err.message }), which will leak internal error messages to clients. If the intent is to avoid leaking unexpected/internal details, consider stripping code/details for status >= 500 (or only allowing an explicit safe allowlist) and logging the original error server-side.
| if (err.status >= 500) { | |
| logger.error({ module: 'api', msg: err.message, status: err.status }); | |
| } | |
| res.status(err.status).json(err.toJSON()); | |
| const payload = err.toJSON(); | |
| if (err.status >= 500) { | |
| logger.error({ module: 'api', msg: err.message, status: err.status }); | |
| // Avoid leaking internal details for 5xx errors | |
| if ('code' in payload) { | |
| delete (payload as any).code; | |
| } | |
| if ('details' in payload) { | |
| delete (payload as any).details; | |
| } | |
| if ('message' in payload) { | |
| (payload as any).message = 'Internal server error'; | |
| } | |
| } | |
| res.status(err.status).json(payload); |
…tern
Replace all manual res.status(XXX).json({ error: ... }) error responses
with throw + factory helpers (badRequest, notFound, conflict, internalError,
forbidden, tooManyRequests). Errors now propagate to the centralized error
middleware instead of being handled inline.
Key changes:
- ~55 error responses converted to throw + factory pattern
- catch blocks simplified to rethrow as ApiError instances
- ResumeError converted to ApiError(statusCode, message)
- Rate-limit detection preserved via tooManyRequests()
- 413 responses use new ApiError(413, ...) directly (no factory)
- All success responses (200/201/204) left unchanged
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Routes now throw ApiError instead of calling res.status().json(), so test servers need the error-handling middleware to convert thrown errors into JSON responses. Changes: - Add apiErrorHandler after route mounting in 7 test files - Fix data.ts catch block to re-throw ApiError (not wrap 400→500) - Update dataRoutes.test.ts to expect thrown ApiError instead of res.status() - Update api.integration.test.ts lock-conflict assertion to match ApiError format Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
5d39117 to
90996f8
Compare
- Remove details from 5xx ApiError responses in summary, shared, replay, data routes - Add logger.error() for internal errors before throwing generic messages - Fix errorHandler to delegate to next(err) when headers already sent - Add safety net: middleware strips details from any 5xx ApiError response - Fix test mock path for logger module - Add test for 5xx details stripping Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: agent-e13af8c5
Changes
Created a centralized error handling system replacing ~344 manual
res.status().json()calls across all 24 route files.New Infrastructure
ApiErrorclass with 9 factory helpers:badRequest(),notFound(),unauthorized(),forbidden(),conflict(),unprocessable(),tooManyRequests(),internalError(),serviceUnavailable()requireParam()validator with TypeScript assertion signatureapiErrorHandlerExpress error middleware (36 lines) with headers-already-sent guardMigration
res.status().json()tothrow new ApiErrorerr.messageto clients in ~6 routes; new middleware returns generic 500 for unexpected errorsCompatibility
Tests
18 new tests including Express 5 async integration tests.
All 3992 tests pass. TypeScript compiles clean.
Review