refactor(workflow-executor): centralize HTTP error mapping into typed errors [PRD-477]#1657
Open
Scra3 wants to merge 6 commits into
Open
Conversation
… errors [PRD-477] Introduce a BaseHttpError > status class > precise-case hierarchy and a single toHttpError translator. The error-translation middleware now owns all error->HTTP mapping; handlers and hasRunAccessMiddleware just throw typed errors. Behaviour change: infra failures (WorkflowPortError/RunStorePortError) now map to 503 with their userMessage everywhere (previously 400 on trigger, 500 on GET). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 new issue
|
|
Coverage Impact This PR will not change total coverage. Modified Files with Diff Coverage (4)
🛟 Help
|
…d lock stack-from-cause [PRD-477] Post-review fixes for the centralized HTTP error mapping: - UserMismatchError now carries bearerUserId + ownerUserId in its message, so the centralized request log keeps the authz forensic signal (who tried to act on a run they don't own) that the per-handler log used to provide. Ids stay in the log only — the HTTP body remains 'Forbidden', no id leak. - Middleware falls back to the HTTP error's own stack when the cause has none, and a test now asserts the logged stack comes from the cause (the real fault site), not the synthetic wrapper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… HTTP errors [PRD-477] abstract already prevents direct instantiation of the status classes, so the protected modifier only forced no-arg leaves to re-declare a public constructor. Removing it lets MissingOrInvalidTokenHttpError/RunAccessDeniedHttpError be empty. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…in family [PRD-477] RunNotFoundError / UserMismatchError / RunAlreadyInFlightError now extend WorkflowExecutorError instead of plain Error, so every domain error shares one base. toHttpError keeps an explicit branch per status (404/403/400), but a forgotten mapping now degrades to a 400 with the userMessage instead of a silent 500. UserMismatchError keeps the bearer/owner ids in its technical message only; its userMessage stays id-free so the HTTP body never leaks ownership. No behaviour change: these are thrown solely at the Runner.triggerPoll level (verified), never inside an instanceof-WorkflowExecutorError guard scope. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… per-class [PRD-477]
Introduce transport-agnostic domain categories (NotFoundError, ConflictError,
AccessDeniedError, UnavailableError) under WorkflowExecutorError. toHttpError now
maps by category, so a new request-level error gets its status by extending the
right category — no HTTP binding to add. The HTTP side collapses to one concrete
class per status (no per-error leaf classes); handlers throw them directly.
Behaviour change: RunAlreadyInFlightError now maps to 409 Conflict (was 400),
the semantically correct status for "run already being processed". 403 bodies
stay opaque ('Forbidden').
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…sed Conflict category [PRD-477] Revert the 409 change: RunAlreadyInFlightError stays a 400. It's uncategorized (an explicit toHttpError branch flags it as expected churn → not logged), so the ConflictError category and ConflictHttpError, now memberless, are removed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
What
Centralizes the executor's error → HTTP-response mapping (PRD-477). Previously, translation was scattered across three places in
executor-http-server.ts(error middleware,hasRunAccessMiddleware,handleTriggerwith oneinstanceofbranch per error type), and was already inconsistent — an orchestrator failure surfaced as 400 onPOST /triggerbut 500 onGET /runs.Design
Domain error categories (transport-agnostic). A small set of abstract categories under
WorkflowExecutorErrorinerrors.ts:NotFoundErrorAccessDeniedErrorUnavailableErrorWorkflowExecutorError)Run lifecycle errors extend their category:
RunNotFoundError → NotFoundError,UserMismatchError → AccessDeniedError,RunStorePortError/WorkflowPortError → UnavailableError.RunAlreadyInFlightErrorstays a 400 (an explicit branch flags it as expected churn so it isn't logged).Single translator.
toHttpError(err)maps by category, not by concrete class — so a new request-level error (e.g.StepNotFoundError extends NotFoundError) gets the right status with zero change to the HTTP layer. Returnsnullfor untranslatable errors → middleware responds a generic 500 (no internals leaked).Error-translation middleware owns all mapping; handlers just
throw. The HTTP side is one concrete class per status (BadRequestHttpError,UnauthorizedHttpError,ForbiddenHttpError,NotFoundHttpError,ServiceUnavailableHttpError) — no per-error leaf classes.Logging. A
logflag on each HTTP error: genuine server/security faults (503, user mismatch) are logged with the underlying cause's stack; expected client churn (404/400) stays out of the logs.UserMismatchErrorcarries the bearer/owner ids in its technical message (logged) but never in the body — 403 responses stay opaque (Forbidden).Infra failures (
WorkflowPortError/RunStorePortError) → 503 everywhere (were 400 on trigger, 500 on GET). Semantically correct (server fault, retryable). Every other response is byte-for-byte identical.Tests
test/http/http-errors.test.ts: category mapping (404/403/503, the 400 default for an uncategorizedWorkflowExecutorErrorincl.RunAlreadyInFlightError,null→ 500), status-class invariants, koa-jwt 401.test/http/executor-http-server.test.ts: end-to-end statuses incl. the new 503, middleware logslog:trueerrors (asserting the stack comes from the cause) and stays quiet for expected churn.Refs PRD-477
Note
Centralize HTTP error mapping in workflow-executor using typed domain errors
NotFoundError,AccessDeniedError,UnavailableError) in errors.ts and re-parents existing domain errors under them.toHttpErrorutility that maps domain errors to HTTP responses.toHttpError, replacing per-handler try/catch cascades.RunStorePortErrorandWorkflowPortErrornow map to 503;UserMismatchErrorto logged 403;RunAlreadyInFlightErrorto non-logged 400; unmapped errors still return 500.Macroscope summarized 3f17687.