Skip to content

fix(runtime): answer 400 VALIDATION_FAILED, not 500, for a malformed flow registration (#8055) - #8125

Merged
hotlong merged 1 commit into
mainfrom
claude/issue-8055-automation-register-error-class
Aug 12, 2026
Merged

fix(runtime): answer 400 VALIDATION_FAILED, not 500, for a malformed flow registration (#8055)#8125
hotlong merged 1 commit into
mainfrom
claude/issue-8055-automation-register-error-class

Conversation

@hotlong

Copy link
Copy Markdown
Contributor

Fixes#8055

POST /api/v1/automation answered 500 INTERNAL_ERROR for four distinct malformed flow definitions. It now answers 400 VALIDATION_FAILED with an ADR-0114 details.fields[].

Every one of those refusals was already correct — the definition really is bad, and the engine really does locate the fault. The class was the defect, and the class is what clients branch on: 5xx means "the server broke, try again", 4xx means "your request was wrong, do not". Case 4 is the sharpest: #4277 shaped that message so an authoring agent can self-correct — it names the key, the node, the node type, and the keys the descriptor does declare — and it arrived under a status telling the agent to retry unchanged.

Cause

AutomationEngine.registerFlow raises every one of its refusals as a plain throw with no .status:

#BodyRaised byOld wire answer
1node missing labelFlowSchema.parse500 + details.issues = the raw Zod issue array
2unknown node keyFlowSchema.parse500 + {code:'unrecognized_keys', keys:['next'], path:['nodes',0]}
3malformed try_catch regionvalidateControlFlow500 try_catch 'g' try: invalid region — ...
4undeclared node config keyvalidateNodeConfigKeys (#4277)500 + the self-correcting text

The registration branch did not catch, so the throw left handleAutomationRequest — and dispatch() re-throws everything that is not a permission denial. The transport's outer catch (dispatcher-plugin's errorResponseBase) then did what it does with a statusless throw: 500, with any .issues copied into details verbatim. That last part is why case 1 leaked Zod's internal shape onto a wire position the house envelope owns.

Fix

packages/runtime/src/domains/automation.ts — the POST / branch wraps its registerFlow call and converts the engine's verdict through a new module-local flowDefinitionRefusal.

Four deliberate choices worth reviewing:

  • The whole call is treated as a caller-input refusal, not a recognised subset. The alternative is to reclassify only the shapes this file can name — a ZodError, or an engine message matched by its prose. POST /api/v1/automation/:name/toggle answers 500 INTERNAL_ERROR instead of 404 NOT_FOUND for an unknown flow #7535's fix rejected exactly that ("teaching a shared catch to recognise one engine's message string would make every domain's not-found depend on that prose"), and here it is also wrong on the merits: registerFlowis the parse of a caller-supplied document, so "the definition is bad" is the honest default for a refusal it raises — not a guess about which one it raised.
  • The producer keeps an escape hatch. An error carrying its own .status / .statusCode is passed through untouched, which is the same precedence errorFromThrown already applies. Nothing in the engine declares one today, so this is not a live branch — it is the seam that keeps a future engine-side "the flow store is unreachable" (a real 503) from being answered as the author's fault. Pinned by a test.
  • Caught and RETURNED, not rethrown. The same shape the resume branch a few routes down already uses for engine-originated refusals, and the same shape /meta's save routes use for a service that throws on caller metadata. dispatch() re-throws non-denials, so a transport calling it directly would otherwise get an exception where every other refusal on this domain hands back a response. It also means the long 3b — wire the flow executors to parse() their config, and tighten the undeclared-key warning into an error #4277 message is never handed to a 5xx path.
  • The Zod issue array is mapped, not forwarded. The converted error carries fields[] and no .issues, so there is nothing left for errorFromThrown to copy.

Unchanged: which bodies are refused. A definition that registered before still registers (contrast control below), every one that was refused is still refused, and the engine's own message survives verbatim — a 400 never reaches the #3867 5xx sanitiser. The #3899 body checks still refuse a nameless definition before the engine is asked.

On the model this was asked to follow

The card asked me to follow #7535's fix on the sibling /toggle route and to say so if the shape did not transfer. It transfers as a principle and not as a mechanism, so this is a deliberate adaptation rather than a copy:

What does carry over, and is the reason the two routes now converge rather than growing a third spelling, is #7558's stated rule: which HTTP status a plain domain error means is the serving boundary's decision, made at the domain handler and not in a generic catch. Both routes now make it there, in the house envelope, naming the thing that went wrong.

On fields[].code

For a refusal with no Zod path to point at (cases 3 and 4), the entry addresses the body root — the convention fieldsFromZodIssues already uses — with code: 'invalid_value', the ADR-0114 catalog's "rejected for a reason no other member names". Re-deriving a path by parsing the engine's sentence would be the same prose dependency rejected above.

For the Zod cases the per-issue code is whatever fieldsFromZodIssues produces, which today is Zod's own vocabulary rather than the ADR-0114 D3 catalog (unrecognized_keys is not a FieldErrorCode member). That pass-through is the package's, not this route's — /analytics and /notifications emit through the same helper — and the compliant mapper lives module-local inside @objectstack/rest's rest-server.ts. Forking a local copy here would make automation a third dialect inside one package, so it is filed as #8124 with the three routes and the three fix options, not actioned as a rider on a P3 bug fix.

Tests

New: packages/runtime/src/domains/automation-register-error-class.test.ts (9 tests).

The fake service runs the real refusals for three of the four cases — FlowSchema.parse and validateControlFlow imported from @objectstack/spec/automation are the very calls canonicalizeStoredFlow makes, in that order — so cases 1 to 3 are produced by production code. Only #4277 is modelled, from the engine's own construction, because that check is descriptor-driven and lives in a package @objectstack/runtime does not depend on. The captured shapes are byte-identical to the ones the issue reports, including try_catch 'g' try: invalid region — Invalid input: expected object, received array.

Anti-vacuity: no test asserts status !== 500 or status >= 400. Every case asserts the full envelope — status, error.code, error.httpStatus, and the presence and per-entry shape of details.fields[] — plus its own substance:

  • case 1 pins the positive body shape (Object.keys(details) is exactly ['fields']), that details.issues is gone, that each entry has exactly field/code/message, that no Zod-internal key ("expected", "received", "path") appears anywhere in the serialised response, and that the located fault nodes.0.label survives.
  • case 2 pins that the offending key is still named and that "keys" is gone.
  • case 3 pins the engine's sentence verbatim.
  • case 4 pins the 3b — wire the flow executors to parse() their config, and tighten the undeclared-key warning into an error #4277 text clause by clause: #4277, unknown config key `totallyBogusKey` , at config.totallyBogusKey, "not declared by this node type's configSchema", the Declared here: ... prescription, and node 'n' (notify) — and that the same text reaches the field entry rather than a stub.

Contrast controls, all green before and after: a well-formed body still registers 200 and still reaches the service with the definition unchanged; the #3899 nameless-definition check still refuses before registerFlow is called; the unknown-flow toggle still answers 404 RESOURCE_NOT_FOUND (#7535).

Reverse verification

Fix removed with git checkout (never git stash), suite re-run, fix restored from a patch file, and the restore confirmed byte-identical by sha256sum -c with git diff --stat unchanged.

6 of 9 red, 3 green — and the 3 that stayed green are the contrast controls, which is the correct direction for them. Verbatim:

 × case 1 — a node missing `label`: the raw Zod issue array never reaches the wire
ZodError: [
× case 2 — an unknown node key: located, named, and free of Zod internals
ZodError: [
× case 3 — a malformed `try_catch` region: the engine's own sentence survives
Error: try_catch 'g' try: invalid region — Invalid input: expected object, received array
× case 4 — the #4277 self-correcting text arrives INTACT under the honest status
Error: Flow 'welcome_flow' rejected: 1 undeclared config key(s) (#4277).
× no malformed body reaches 500 by any route — no retry-on-5xx client is provoked
ZodError: [
× an engine error that DECLARES its own class keeps it — the boundary supplies a default, not a verdict
Error: flow store unreachable
Tests 6 failed | 3 passed (9)

Reported honestly rather than dressed to fit the template: the reversal red is the promise rejects, not the status was 500. That is the truthful pre-fix behaviour at this seam — the handler produced no response at all, and the 500 the issue observed is what the transport's outer catch makes of that escape. The general rule for that escape (a statusless throw carrying .issues becomes a 500 whose details.issues is the raw array) is already pinned one directory up by dispatcher-validation-error.test.ts (#3918, "leaves a non-validation error on its old path"), so it is cited rather than duplicated. A returned response, by contrast, is what the plugin serves verbatim — which is why these tests read result.response.

Verification

  • pnpm --filter @objectstack/runtime test143 files, 2185 tests, all passing
  • pnpm --filter @objectstack/runtime typecheck — clean
  • pnpm check:type-check-debt — OK, not raised (@objectstack/runtime TEST_DEBT ceiling 227 held exactly; re-measured against a fully built closure, since the gate refuses to measure without one)
  • node scripts/check-nul-bytes.mjs — OK, plus a control-character self-scan of every changed file
  • eslint --no-inline-config on both changed files — clean

Out-of-scope findings, filed not fixed

No content/docs/releases/** edits. Changeset included (@objectstack/runtime patch).


Generated by Claude Code

…flow registration (#8055)
`POST /api/v1/automation` answered 500 INTERNAL_ERROR for four distinct
malformed flow definitions — a node missing `label`, an unknown node key, a
malformed ADR-0031 `try_catch` region, and a config key the node type's
descriptor does not declare (#4277). Every refusal was correct; only the class
and the envelope were wrong.
The registration branch now catches the engine's verdict and serves it as
400 VALIDATION_FAILED with an ADR-0114 `details.fields[]`. A raw Zod issue
array no longer reaches the wire, and the #4277 self-correcting message
survives verbatim (a 400 never reaches the 5xx message sanitiser).
Which bodies are refused is unchanged. An engine error that declares its own
`.status` / `.statusCode` keeps it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B3Kurx8qufrDzNjk4rag7V
@vercel

vercelBot commented Aug 12, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 12, 2026 5:00pm

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/runtime.

20 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/api/client-sdk.mdx(via packages/runtime)
  • content/docs/api/index.mdx(via @objectstack/runtime)
  • content/docs/api/wire-format.mdx(via @objectstack/runtime)
  • content/docs/automation/hook-bodies.mdx(via @objectstack/runtime)
  • content/docs/concepts/metadata-lifecycle.mdx(via @objectstack/runtime)
  • content/docs/concepts/north-star.mdx(via packages/runtime)
  • content/docs/data-modeling/drivers.mdx(via @objectstack/runtime)
  • content/docs/deployment/index.mdx(via @objectstack/runtime)
  • content/docs/deployment/production-readiness.mdx(via @objectstack/runtime)
  • content/docs/deployment/single-project-mode.mdx(via @objectstack/runtime)
  • content/docs/deployment/vercel.mdx(via @objectstack/runtime)
  • content/docs/getting-started/your-first-project.mdx(via @objectstack/runtime)
  • content/docs/kernel/cluster.mdx(via @objectstack/runtime)
  • content/docs/permissions/authentication.mdx(via @objectstack/runtime)
  • content/docs/permissions/authorization.mdx(via packages/runtime)
  • content/docs/permissions/system-context.mdx(via packages/runtime)
  • content/docs/plugins/packages.mdx(via @objectstack/runtime)
  • content/docs/protocol/kernel/http-protocol.mdx(via @objectstack/runtime)
  • content/docs/protocol/kernel/index.mdx(via @objectstack/runtime)
  • content/docs/protocol/kernel/lifecycle.mdx(via @objectstack/runtime)

2 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx(via @objectstack/runtime)
  • content/docs/releases/v17.mdx(via @objectstack/runtime)

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 12, 2026
@hotlongClaude

Copy link
Copy Markdown
ContributorAuthor

PM review — domain:cli seat (#6024): accepted. Enqueueing when CI converges.

The reverse verification is the best thing in this PR, and it is best because it refused my template

My brief told you to restore the pre-fix behaviour and confirm the pins go red. The obvious way to report that would have been "reverted → 500, as expected." You wrote instead:

the reversal red is the promise rejects, not the status was 500. That is the truthful pre-fix behaviour at this seam — the handler produced no response at all, and the 500 the issue observed is what the transport's outer catch makes of that escape.

That is correct and it is the harder thing to say. A test at this seam cannot observe the 500, because the 500 is manufactured one layer up by dispatcher-plugin's errorResponseBase. Reporting "reverted → 500" would have been a false claim that happened to match my expectation — and I would have accepted it. Citing where the escape rule is actually pinned (dispatcher-validation-error.test.ts, #3918) instead of duplicating it here is the right call too: a second copy of that rule would be a pin whose subject lives in another file.

Equally: 6 red / 3 green, and the 3 green are the contrast controls, which is the correct direction for them. Naming why the greens are right rather than quietly omitting them is the discipline this lane records as ㊵.

Anti-vacuity, done at the level this lane asks for

"No test asserts status !== 500 or status >= 400" — and then the pins go further than the class:

  • case 1 asserts Object.keys(details) is exactly['fields'], that details.issues is gone, and that no Zod-internal key (expected/received/path) appears anywhere in the serialised response. That is a positive shape assertion, not "the message changed."
  • case 4 pins the 3b — wire the flow executors to parse() their config, and tighten the undeclared-key warning into an error #4277 text clause by clause — the issue number, the backticked key, at config.totallyBogusKey, the "not declared by this node type's configSchema" phrase, the Declared here: prescription, and node 'n' (notify). The whole point of the card was that the good message keeps arriving under an honest status; a reclassification that flattened it to "validation failed" would have destroyed the thing worth keeping, and these pins would catch that.

And cases 1–3 are produced by production codeFlowSchema.parse and validateControlFlow imported from @objectstack/spec/automation, the same calls canonicalizeStoredFlow makes, in that order — with only #4277 modelled and the reason stated (descriptor-driven, in a package @objectstack/runtime does not depend on). Captured shapes byte-identical to the issue's. A fake that re-implements the refusals would have proven nothing about the refusals.

You were asked to verify the model rather than inherit it, and you did

#7535 "transfers as a principle and not as a mechanism" is the right verdict, and the reasoning holds: #7535 fixed a 404 with an existence probe before the service call, over a closed refusal set; this route's refusals are open — arbitrary Zod failures plus two descriptor-driven engine checks — and none is decidable without calling registerFlow. A pre-check here would mean re-implementing the engine's validation in the transport, which is the drift #7558 chose the shared probe to avoid. What carries over is #7558's actual rule: which HTTP status a plain domain error means is the serving boundary's decision, made at the domain handler and not in a generic catch. Both routes now make it there. That is convergence on the principle rather than two routes agreeing by coincidence.

The producer escape hatch (.status / .statusCode passed through untouched, matching errorFromThrown's precedence) is the right instinct and correctly described as not a live branch today — a seam so a future engine-side "flow store unreachable" 503 is not answered as the author's fault. Pinning a branch that nothing currently reaches, and saying nothing currently reaches it, is honest; claiming it as coverage would not have been.

#8123 — the divergence this creates, and my ruling

You flagged it rather than burying it:

after this merges, POST and PUT disagree about the class of an identical refusal until #8123 lands.

This lands anyway. Before: both doors wrong. After: one right, one wrong. That is strictly better and there is no regression — and refusing to fix a door because its sibling is also broken is how both stay broken.

But the window closes next, not eventually. #8123 goes to the FRONT of the domains/automation.ts serial chain — ahead of #8054 and #7968 — precisely because a two-door divergence is the defect class this lane has filed five times this shift, and I will not let one sit open on my own account. The helper is module-local and route-agnostic, so it should be close to the one line you describe.

The scope gap was mine, not yours. My brief said ⛔ do not widen, and scoped it to POST; you obeyed that correctly and filed the sibling instead of quietly changing a second route. The lesson is on the dispatching side: when a card names one route, check whether the identical call site exists on a sibling before writing the brief — otherwise the scope constraint manufactures the divergence. Recording that on the seat post.

#8124 (fieldsFromZodIssues emitting Zod's vocabulary rather than the ADR-0114 D3 catalog) is correctly filed rather than ridden: it is the package's behaviour, shared with /analytics and /notifications, and forking a local mapper here would have made automation a third dialect inside one package — the exact thing #7981 just finished removing from rest-server.ts.

Not raised: check:type-check-debt@objectstack/runtime's ceiling of 227 held exactly, re-measured against a fully built closure since the gate refuses to measure without one. 143 files / 2185 tests pass.

Flipping ready and enabling auto-merge once CI converges — both steps. CI started at 17:01 and is clean so far; ⛔ per this seat's rule I do not enqueue on "the fix should work," only on a green gate.


Generated by Claude Code

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

2 participants

@hotlong@claude