Skip to content

fix(automation): answer real HTTP status codes on the run-resume route - #9379

Merged
os-zhuang merged 3 commits into
mainfrom
claude/issue-8684-automation-resume-status-unification
Aug 17, 2026
Merged

fix(automation): answer real HTTP status codes on the run-resume route#9379
os-zhuang merged 3 commits into
mainfrom
claude/issue-8684-automation-resume-status-unification

Conversation

@claude

@claudeclaudeBot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Fixes#8684

The automation run-resume route answered HTTP 200 wrapping an inner {success: false}
for a run that resumed and then failed. #3962 already unified /actions on real status
codes; this route was left out, so a scripted or integration caller that branches on the
HTTP status alone read a failed run as a successful one.

Implemented per the maintainer ruling recorded on the card (comment 5302930352,
2026-08-15) and the cross-repo contract handoff from objectui#4784 / PR #4899.

What changes

Producer first — packages/services/service-automation/src/engine.ts. The two
never-dispatched exits on the resume path now carry a classification instead of being
code-less: the flow deregistered under a live pause, and the suspended node edited away.
Both mean the suspension is real but can never continue — nothing ran — so they are the
same terminal class as a missing suspension and answer 404.

Route stays a pure mapper — packages/runtime/src/domains/automation.ts. One new arm
under the existing six-code map: a success: false result that reached it consumed its
pause and ran, so it answers 400FLOW_FAILED. The route never inspects summary or
durationMs to tell the two classes apart — that sniffing is the tolerant-consumer shape
PD #12 forbids, and it is unnecessary because the engine now says which is which.

engine exitrealityanswer
flow not found for run · suspended node no longer existsnever dispatched404 (RUN_NOT_FOUND)
run failed · subflow run failedran, rejected400FLOW_FAILED
PERMISSION_DENIED · INVALID_SIGNAL · INVALID_SCREEN_INPUT · RUN_NOT_FOUND · STORE_UNAVAILABLE · RESUME_IN_PROGRESSrefused before the pause was consumedunchanged (403/400/400/404/503/409)

The consumer contract is carried, not dropped. The ADR-0112 envelope has no data, so
the flow author's own errorMessage travels in error.details.errorMessage — the one
documented location the console reads (objectui flowResponse.ts, PR #4899) — and the raw
node failure stays error.message. The run's per-node summary rides in details for the
same reason it was on the 200 body: it is how a caller finds which node failed.
The named anti-pattern was re-verified and deliberately not reproduced: the live
FLOW_FAILED producer at packages/runtime/src/action-execution.ts:607-615 builds its
message from result.error and drops result.errorMessage entirely.

INVALID_SIGNAL / INVALID_SCREEN_INPUT keep their own codes and stay retryable — the
engine refuses those before consuming the suspension, and only FLOW_FAILED is terminal
consumer-side.

@objectstack/client needed no code change and this is a measured finding, not an
omission: _fetch already throws on every non-2xx before unwrapResponse runs, so
automation.resume() now rejects with err.code === 'FLOW_FAILED', err.httpStatus,
and err.details.errorMessage rather than resolving with an inner failure. Both resume
surfaces document the new contract and both are pinned by tests.

Two rulings collided on one file, and the collision is reported rather than resolved

The ruling says the four code-less exits get real codes in the engine, FLOW_FAILED among
them (sub-decision 1) — and it also says any fragment that drifts into packages/spec
belongs to the spec seat. Those cannot both hold here.
AutomationResult.code is a closed union declared in
packages/spec/src/contracts/automation-service.ts, so naming FLOW_FAILED in the engine
requires widening it. Measured, not assumed — with code: 'FLOW_FAILED' on the
ran-and-failed exit, tsc goes from 3 pre-existing errors to 4:

src/engine.ts(4085,21): error TS2322: Type '"FLOW_FAILED"' is not assignable to type
'"PERMISSION_DENIED" | "INVALID_SIGNAL" | "RUN_NOT_FOUND" | "STORE_UNAVAILABLE" |
"RESUME_IN_PROGRESS" | "INVALID_SCREEN_INPUT" | undefined'.

So this PR keeps packages/spec untouched and delivers the ruling's substance — the
producer classifies, the transport maps, no sniffing — by naming FLOW_FAILED at the
route. That is also where it belongs on the ledger's own terms: ADR-0112 registers
FLOW_FAILED to @objectstack/runtime, and the precedent producer this card inherits
(action-execution.ts) stamps it in runtime too. Stamping it inside
service-automation would have made a second package emit a runtime-owned wire code — a
second spec fragment (a ledger provenance entry) on top of the union widening.

For the spec seat, one line: adding 'FLOW_FAILED' to that union would let the engine
name the ran-and-failed class directly. It changes no generated artifact — the api-surface
snapshot records AutomationResult (interface) with no member detail.

The two open questions the dispatch left open, answered

The 404 code reuses RUN_NOT_FOUND rather than introducing a distinct one. The remedy
is identical (this pause is finished; start a new run), the consumer branches on the 404
status and not on the code, and the engine's message still names which of the three
causes it was, verbatim on the wire. A distinct code would also require widening the same
closed union — vocabulary nothing reads, at the cost of the spec fragment above. If a
caller ever needs to branch on the difference, add it then.

The engine's success: false exits reachable from resume() were enumerated in full
(the ruling named four; this confirms the set is complete): RESUME_IN_PROGRESS,
STORE_UNAVAILABLE, RUN_NOT_FOUND, PERMISSION_DENIED, INVALID_SIGNAL,
INVALID_SCREEN_INPUT, the two stale-suspension exits, the subflow-failed exit and the
ran-and-failed exit. Every one is now answered by a named arm; nothing rides HTTP 200.

Scope held

Verification

Union of gates run at 4af8ef9fc (the final commit): check:cross-package-test-inputs,
check:route-envelope, check:test-source-alias, check:type-source-resolution,
scripts/check-cross-package-test-inputs.mjs, scripts/docs-audit/check-affected-docs.mjs,
check:query-options-erasure, check:type-check-coverage, check:engine-double-contract,
check:where-matcher, check:nul-bytes, and check:type-check-debt on a fully built
workspace closure (--re-measure: 33 ledger entries, none above its recorded number),
plus the changeset family the re-derivation added
that the dispatch had not named — check:changeset-gate-self-tests,
check:objectui-changeset, check-changeset-no-major.mjs, check-empty-changeset.mjs,
check-adr-0087-registration.mjs. All green.

Tests: @objectstack/runtime 165 files / 2467 tests, @objectstack/service-automation
80 files / 969 tests, @objectstack/client 23 files / 303 tests — all passing.
typecheck green for runtime and client; service-automation has no typecheck
script and its 3 ledgered errors (TS2341 in a file this PR does not touch) are unchanged.

Two gate outcomes worth a reviewer's eye, both resolved by following the gate rather than
around it:

  • check-changeset-no-major refused the major bump. Every publishable package is in
    one Changesets fixed group, so a single major promotes the whole monorepo; the gate's
    own remedy during the launch window is to ship a breaking change as minor. The bump is
    minor, the body keeps the BREAKING framing. Adding the allow-major label instead
    would have been a release-versioning decision, which is not this PR's to take.
  • check-adr-0087-registration required a disposition in writing. Answered
    not-required (no-migration-prescription): this retires no metadata surface — no Zod
    schema, no authorable key, no stored row changes shape — so objectstack migrate meta
    has nothing to rewrite and no ledger entry can honestly be written. The consumer change
    is HTTP/SDK-level and this changeset is the channel that carries it. Overrule it if you
    read the category differently.

Out-of-scope findings filed


Generated by Claude Code

The resume route answered HTTP 200 wrapping an inner `{success:false}` for a
run that resumed and then failed, so a caller branching on the HTTP status
alone read a failed run as a successful one. Applies the `/actions`
status-code unification to this route.
Producer-first: the engine's two never-dispatched exits (the flow
deregistered, the suspended node edited away) now carry `RUN_NOT_FOUND`, so
the route answers them 404 instead of mislabelling them a business rejection.
What reaches the route's terminal-failure arm has consumed its pause and run,
and answers 400 `FLOW_FAILED` — carrying the flow author's `errorMessage` and
the run summary in `error.details`, the one location the console reads them
from. The route never sniffs the result shape to tell the two classes apart.
Co-Authored-By: Claude <noreply@anthropic.com>
… launch-window lockstep rule
Every publishable package is in one Changesets fixed group, so a single major
promotes the whole monorepo; check-changeset-no-major prescribes minor for a
breaking change during the launch window. The body keeps the BREAKING framing.
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

2 anchor(s) derived from 3 changed package(s); no hand-written page names any of them. ✅

What this run could not see
  • 1 name(s) were too generic to anchor anything (single lowercase words)

Coarse fallback — 31 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json origin/mainpackageMentionDocs.

@os-zhuangClaude

Copy link
Copy Markdown
Contributor

Do not enqueue this PR yet — and the reason is not the code.domain:cli PM seat (#6024), session session_012WKSnqAaoqtW3QX7SSf1Vk.

The implementation is complete and, on the readings I have taken so far, correct. It is held open deliberately as an objection window, under the standing rule that when a ruling's mandated action measures a contrary fact during implementation, the conflict becomes a decision card and the PR does not arm auto-merge until it is answered.

The conflict:#8684's ruling, sub-decision 1, says the engine names FLOW_FAILED in packages/services/service-automation. AutomationResult.code is a closed union in packages/spec/src/contracts/automation-service.ts:223 and does not contain it, so that sentence requires a packages/spec edit which the same ruling routes to the spec seat. The dev obeyed the spec-escape clause, delivered the ruling's substance without the spec edit, and reported the collision rather than resolving it in the worktree — the correct order of operations.

Filed as #9384 for the spec seat / maintainer. The deviation is where the string is stamped, not what goes on the wire: the ruling's own status table is reproduced exactly.

What I verified myself rather than taking on report (the dev's readings are good; these are the two that decide whether the shape is sound):

  1. The closed union is real. Read on origin/main, not inferred — FLOW_FAILED is genuinely absent from the union.
  2. The route does not sniff, and this is provable rather than argued. I enumerated every success: false return in resumeInternal on the branch: the only two still code-less are the ran-and-failed exit and the subflow-failed exit — both the ruling's "ran, rejected" row. So if (result?.success === false) ⇒ 400 FLOW_FAILED is a fallthrough over a provably single class, not a heuristic over a mixed one. That is the distinction PD Add comprehensive test suite for Zod schema validation #12 turns on, and it holds here. The execute / executeWithoutRetry code-less exits are a different path and are correctly filed separately as automation: both trigger routes still answer HTTP 200 wrapping an inner {success:false} — the same #3962 residue #8684 closed on resume #9378, not swept in.

Two things I am not deciding and have escalated instead:

  • The allow-major label. The changeset is minor because check-changeset-no-major.mjs refuses major (every publishable package sits in one lockstep group, so one major promotes the whole monorepo) and its own prescribed remedy during the launch window is to ship breaking changes as minor. The label is the only override, and moving the whole stack's major version is a release act — the maintainer's, not a PM's and not a dev's. The dev declined it correctly. Flagged in the round report.
  • The ADR-0087 answer (not-required (no-migration-prescription)) reads correct to me — this retires no metadata surface, so objectstack migrate meta has nothing to rewrite — but it is recorded in the changeset for a reviewer to overrule rather than buried.

CI at the time of writing: 8 of the 5 required check families complete and green (Build Core, ESLint, all three Dogfood Regression Gate shards, Test Core 1/3 and 2/3); Test Core (3/3) and TypeScript Type Check still running; nothing red, no non-required failures.


Generated by Claude Code

@os-zhuangClaude

Copy link
Copy Markdown
Contributor

Correction to my own landing criterion above — the required check set is SIX, not five. PM domain:cli seat (#6024).

My ACCEPT comment said "all five required check families are green" and named TypeScript Type Check, ESLint, Test Core, Dogfood Regression Gate, Build Core. Read from ruleset 12119582 just now rather than recalled, the required set also includes Temporal Conformance (live PG + MySQL).

The verdict on this PR is unchanged — I re-checked head 4af8ef9fc against the true six and Temporal Conformance (live PG + MySQL) is completed / success there too. This PR is green on all six. Nothing about the ACCEPT or the hold moves.

Correcting it anyway because the rule was wrong, not just the count: I had been treating that check as advisory in every landing decision this session, and an advisory check is one this seat is explicitly required not to treat as a gate. A red Temporal Conformance would have been read as "non-required, ignore" and enqueued over. It never came up, which is luck rather than method. The seat post now carries the six-name set, the provenance, and the same warning.

While reading the ruleset, two further facts worth having on the record for anyone landing in this lane: required_approving_review_count is 0 (so an ACCEPT is this seat's process discipline, not a mechanical gate — controlled by #9383, which merged with zero reviews), and the merge queue runs SQUASH with grouping_strategy: ALLGREEN, min_entries_to_merge_wait_minutes: 5, check_response_timeout_minutes: 60.

⛔ This PR remains held on #9384, unchanged.


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

Development

Successfully merging this pull request may close these issues.

automation: the run-resume route still answers HTTP 200 wrapping an inner {success:false} — the route #3962's status-code unification left behind

2 participants

@os-zhuang@claude