feat(service-automation): the flow end node honours outcome: 'refused' — a terminal refused run, distinct from failed - #18109
Conversation
…ting it Reproduction first: these fail against the current engine, where `executeNode` opens with `if (node.type === 'end') return;`. Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj Co-authored-by: Claude <noreply@anthropic.com>
The engine terminates the run with the `refused` outcome, renders the authored `message` through the SAME interpolator a screen `description` uses, persists both on the run record, and never resumes it. `successMessage` and the paused-run `silent` contract are untouched. Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj Co-authored-by: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj Co-authored-by: Claude <noreply@anthropic.com>
…ow-end-refused-outcome
📓 Docs Drift CheckThis PR changes 2 package(s): 1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 2 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 7 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin d51aeefdce8bc737bb79e5fe926342c4267812c7 && git checkout d51aeefdce8bc737bb79e5fe926342c4267812c7
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 66aa2d98d6d1ea1b12d179e3ec82e9f53697e6b8 60ca35a2f2ea33de5b256900f57f37471a4a8643 && git checkout -B drift-repro 66aa2d98d6d1ea1b12d179e3ec82e9f53697e6b8 && git merge --no-ff 60ca35a2f2ea33de5b256900f57f37471a4a8643
node scripts/docs-audit/affected-docs.mjs --json 66aa2d98d6d1ea1b12d179e3ec82e9f53697e6b8
|
Contract review
Independent clause-② review of PR #18109 (card #15788, lane 2 of the #14945 maintainer ruling 2′, comment 5548735593). Reviewed head: ① Derived judgments — acceptance set and public surfaceDeclaration under test:
Acceptance set, verified against the code and the pins:
Runs in this session (fresh container, dependency closure built first): ② Semver grading
③ Boundary flags
Non-blocking notes, for the record: Independence
VerdictPASS. The Generated by Claude Code |
|
Record of governing verdict: comment 5658765796 on this PR, judging head
⭐ The review corrected a real error in this PR's own evidence table, and it is worth reading. The body claimed that for
The review also independently confirmed the two escalations already filed by this seat — #18110 (a Generated by Claude Code |
Fixes #15788
Lane (2) of the #14945 maintainer ruling 2′: the flow
endnode honoursoutcome: 'refused'.The contract this is built against (lane 1, already on
main)Re-measured on
origin/mainatb5cbfef9c— a card split out of a sequenced ruling says "already landed" about the delivering branch, not aboutmain:ExecutionStatusmemberrefusedpackages/spec/src/automation/execution.zod.ts:38outcome: z.enum(['completed','refused']).default('completed')packages/spec/src/automation/builtin-node-config.zod.ts:658refusedREQUIRES amessage; amessageon acompletedend is refused as a silent no-op:671-690ExecutionLog.refusalMessageexecution.zod.ts:369AutomationResult.statusgainsrefused, plusAutomationResult.refusalMessagepackages/spec/src/contracts/automation-service.ts:405,430TriggerFlowResponseSchema.data.status/.refusalMessagepackages/spec/src/api/automation-api.zod.ts:356,382⛔
packages/specis untouched by this PR. Nothing in the implementation needed a spec change — the lane-1 contract was complete, including therefusalMessagerun-row key and the wire members, so this lane is purely the producer half.Two places in the tree stated, in words, that this was the missing half, and both are updated here rather than worked around:
TERMINAL_RUN_STATUSES(engine.ts): "refusedis declared by the spec but no engine path produces it today, so adding it here would enumerate a value nothing can write."sys_automation_run.status(the object): "refusedis deliberately ABSENT:ExecutionStatusdeclares it (A flow cannot REFUSE with per-record text: the only channel that interpolates is a screendescription, and a message-only screen still renders Submit and toasts "completed" #14945) but no engine path produces it, and an option nothing can write is a declared-but-inert value (ADR-0078)."The engine now produces it, so the writer, the reader's row gate and the stored column widen in this one change — which is the condition those notes set, not an exception to it.
What changed
executeNode'sendbranch. It opened withif (node.type === 'end') return;— the whole defect: a structural node with no executor and no descriptor, so this was the only place the outcome could be read and nothing read it. It now reads the PARSED config (lane 1'sparseEndNodeConfigruns insideFlowNodeSchema, regions included, sooutcomeis defaulted and arefusedwith nomessagewas already refused at the flow parse — no second door, no??, no re-parse) and throws aFlowRefusalSignal, the twin of the existingFlowSuspendSignal.finishRefusedRunis called fromexecute(),resumeInternalandexecuteWithoutRetry. That chokepoint is not stylistic: this exact file lostsuccessMessage(bug(service-automation):execute()never carries the flow author'ssuccessMessage/errorMessage— onlyresume()does, so a triggered run's friendly text is silently dropped #9414) and the durable pause (bug(service-automation): a retry attempt that PAUSES is recorded as failed and its suspension is never persisted —executeWithoutRetryhas noisSuspendSignalarm #9510) by implementing them at one exit and not the others, which made a run's outcome a function of its ROUTE. A triggered run, a resumed screen flow and an attempt understrategy: 'retry'now answer identically.success: true(a refusal is a successful evaluation that says no),status: 'refused',refusalMessage, noerror, noerrorMessage, ⛔ nosuccessMessage, ⛔ norunId. The retry ladder stops on it without a new branch —retryExecutionalready readsresult.successas "this attempt did not fail, stop retrying", which is the true sentence here; ⛔ a refusal must never consume retry budget.RunRecord.refusalMessage+ a newsys_automation_run.refusal_messagecolumn, written always (NULL included —recordTerminalis an upsert, so a rewritten row must CLEAR a refusal it no longer carries) and read back throughloadTerminal/runRecordToLogEntry. ⛔ Not folded intoerror: text inerrortells every reader — an operator, the Runs surface, a sweep filteringerror IS NOT NULL— that the run broke. Same reason [finding] service-automation: the durable run-history row foldscancelled(andtimed_out) intofailed— after a restart the Runs surface cannot tell a cancelled run from a failed one #15223 stopped foldingcancelledintofailed.resume()answersRUN_NOT_FOUND.packages/plugin-approvals, comment only. ItsRUN_STATUS_LIVENESSdocblock said the service-automation vocabulary "excludesrefusedon purpose". This PR makes that false, so the paragraph is corrected in the same change. No behaviour moves: that map already classifiedrefusedas terminal, and it stays the authority for its own sweep.The region boundary, made loud
An
enddeclaringoutcome: 'refused'inside a structured region is refused with a named message, at the same line whererunRegionalready refuses a durable pause. Left to propagate, the signal would unwind into thetry_catchexecutor's owncatch (err), which reads every throw as the try region FAILING — so an author's refusal would run the error path and the run would still recordcompleted: the pre-#15788 silence with an extra step.⛔ Nothing an author had is narrowed: before this PR an
endin a region was a no-op whatever itsoutcome, so the shape being made loud has never once been honoured. Whether a refusal should instead PROPAGATE out of a region and terminate the run is a real question and ⛔ not one this lane rules on — the #14945 ruling says nothing about regions, and "prefer failing to falling back" decides the interim.Before / after
Reproduce first: the pins were written and committed (
f525fb87f) before any engine edit, and run against the unfixed tree.vitest run src/end-node-refused-outcome.test.tspnpm --filter @objectstack/service-automation testpnpm --filter @objectstack/plugin-approvals testThe BEFORE failures were the shape of the defect, not of a broken harness:
The 4 that passed BEFORE are the fences, green on both sides on purpose: a plain
endstill completes withsuccessMessage, an explicitoutcome: 'completed'is the same completion, a genuinely failed run still readsfailed(the discriminating control — without it, "the refusal path readsrefused" would be consistent with an engine that had started calling everythingrefused), and a refused run was already not resumable because it writes no continuation.The interpolation is the screen-
descriptionone, and that is pinned by mechanismThe ruling: the refusal message goes through the same interpolation a screen
descriptiongets — one implementation, ⛔ never a second template engine.The implementation is
interpolate()inpackages/services/service-automation/src/builtin/template.ts, reached through the four-line coercion thatscreen-nodes.tsheld in a localinterpclosure and read atscreen.description(screen-nodes.ts:195and:279). Those four lines are hoisted verbatim totemplate.tsasinterpolateText;interpnow delegates to it and the refusal path calls the same function. Same bytes in, same bytes out — the only change is where the lines live.Asserting "it substitutes
{record.name}" would be far too weak, so the pin drives both slots with six templates whose behaviour is specific to this interpolator and compares the two renderings for equality: dotted-path walk, numeric array indexing, the{$User.Id}context token, the CEL-mirrored numeric stdlib ({round(x)}, #11060), an unresolvable embedded token rendering as empty string, and an object-valued token JSON-serialized rather than[object Object](#3450).That pin's ability to FAIL is measured below, not assumed.
Reverse verification
Both legs mutate the committed tree, prove the mutation reached disk before reading any result, restore with
git checkout HEAD -- path, and prove byte identity by blob hash. Both scripts carrytrap … EXIT INT TERMwith absolute paths; the trap is the crash convenience, the hash compare is the proof.Leg 1 — remove the refusal branch (the fix itself).
Predicted direction before running: RED. The 8 that reddened are exactly the assertions about the new behaviour — the 6 defect pins, the interpolator-equality pin and the region pin. The set is the right one in both directions:
suspended-run-store.test.tscases stayed green, including the 4 newrefusal_messageones, and that is correct rather than a gap: they driveObjectStoreSuspendedRunStorewith aRunRecorddirectly and never enterexecuteNode. They pin the persistence layer; leg 1 mutated the producer.src/, and the test file imports./engine.jsfrom inside the same package, so vitest resolves it from source. The RED itself is the proof of that resolution path — a stale-distreading would have stayed green.Leg 2 — can the "one interpolator" pin actually fail? The refusal path's
interpolateText(…)call was replaced with a plausible second template engine (a naive{token}substitution with dotted-path support — the kind a reviewer waves through).The second engine passed the obvious
{record.name}probe and was caught at the context token — which is the whole reason the probe set is six templates and not one.Clause-② re-derivation, from the DELIVERED diff
Clause-②: yes— which is what the claim predicted, re-derived here from the built output rather than inherited.The test is reachability from the published entry (
index.tsre-exports plus the package'sexports/files) plus any new key on a published payload.@objectstack/service-automationpublishes["dist","README.md","CHANGELOG.md"]with one entry,./dist/index.d.ts. Afterpnpm --filter @objectstack/service-automation build:RunRecordgainsrefusalMessage?: stringdist/index.d.ts:743, andtype RunRecordis in the entry's export list — a NEW KEY on an already-published payload, the mandatoryyesTerminalRunStatuswidens 4 members to 5dist/index.d.ts:694-696,type TerminalRunStatusexported; the runtime value ships too (dist/index.js:TERMINAL_RUN_STATUSES = ["completed","failed","cancelled","timed_out","refused"])SysAutomationRungains therefusal_messagefield and therefusedoptiondist/index.d.ts:9339,SysAutomationRunexported as a value⛔ The terminal status is NOT what carries it:
refusedwas already a declaredExecutionStatusmember, sostatusis an existing key taking a newly-legal value. The carrier is the new key.Discriminating controls, so the probe is not just reporting "everything in my diff is published":
dist/index.d.tsinterpolateText— new in this diff, internal tobuiltin/isTerminalRunStatus— exported fromengine.ts, not re-exported from the barrelRunRecord— published before this diffzzzNotASymbolA probe that answered ">0" for
interpolateTextandisTerminalRunStatuswould have been measuring file text rather than the published surface. It did not.The changeset is graded
minor, as the ruling grades this lane — neverpatch.Gates
Derived mechanically from the real change set, not from the dispatch list, and reconciled with
--ran:Denominator: 65 derived / 65 run / 0 NOT MEASURED / 0 UNRUN. Three needed a prerequisite the first pass did not have and were re-run after clearing it, ⛔ not recorded as failures:
check:dual-build-cjs-loadsandcheck:i18nexited 3 — PREREQUISITE NOT MET ("nothing was measured") untilturbo run build --filter='./packages/*' --filter='./packages/*/*'; both exit 0 after.check:type-check-debtexited 3 twice: once for the same unbuilt closure, then on an OOM at the--max-old-space-size=4096this container prefixes onto heavy commands — the gate prints that ceiling itself ("the caller's NODE_OPTIONS, which is tighter"). Re-run without the tightened cap: exit 0 —76/80 workspace packages type-checked, 5 ledger entries re-measured, 55 raw tsc errors, none above its recorded number.Also run, beyond the derived set:
pnpm --filter @objectstack/service-automation typecheck— exit 0, and itscheck:test-typecheckleg compiles the test layer, so the new pins are type-checked rather than merely executed.--filter '...@objectstack/service-automation', the direction a contract widening lands in): 18 packages typecheck green —cli,client,client-react, the fourconnectors/*,plugin-approvals, bothtriggers/*,verify,qa/dogfood,qa/downstream-contract, the fourexamples/*, andservice-automationitself.pnpm lint(=eslint . --no-inline-config) over the whole repo: exit 0, no findings. Run at60ca35a2f, after the final commit — no narrowing to justify.origin/mainwas merged at3aaea3879throughscripts/pm/os-regen-merge.sh; it left no generated artifact to regenerate and noos-regen-pendingdeferral, andpnpm install --frozen-lockfilewas re-run afterwards per the stale-artefact rule.Acceptance notes
Out of scope, noted rather than filed — each names who would meet it:
subflowCHILD that refuses is rolled up as an ordinary success by its parent.subflow-node.tsbranches only onchild.status === 'paused', so a refused child returnssuccess: trueand the parent walks on. ⛔ Not a regression this PR introduces — the parent behaved identically when a refusing child simply completed — but the ruling does not say what a parent should do with a child's refusal, and it is a live question the moment authors start writing them. Carrier: the A flow cannot REFUSE with per-record text: the only channel that interpolates is a screendescription, and a message-only screen still renders Submit and toasts "completed" #14945 ruling seat, or whoever takes lane 3.plugin.tsdescribes the retention scope as{ status: { $in: ['completed', 'failed'] } }in two comments (:138,:787) while the object has declared four members since [finding] service-automation: the durable run-history row foldscancelled(andtimed_out) intofailed— after a restart the Runs surface cannot tell a cancelled run from a failed one #15223 and five as of this PR. Pre-existing staleness, ⛔ not made false by this change, and left alone to keep the diff at the vocabulary it is actually widening. Carrier: the next PR to touchsys_automation_run's retention.refusedin this package overwhelmingly means a GUARD refusal (the engine declining to execute — a failure), and this lane'srefusedmeans a successful evaluation that said no. The disambiguation is written at both definition sites (FlowRefusalSignal, and the pin file's header) so a futuregrepreads the sense at the site rather than from the word.Generated by Claude Code