Skip to content

Commit cca6991

Browse files
claude[bot]claude
andauthored
feat(service-automation): the flow end node honours outcome: 'refused' — a terminal refused run, distinct from failed (#18109)
Fixes #15788 Lane (2) of the #14945 maintainer ruling 2′: the flow `end` node honours `outcome: 'refused'`. ## The contract this is built against (lane 1, already on `main`) Re-measured on `origin/main` at `b5cbfef9c` — a card split out of a sequenced ruling says "already landed" about the delivering branch, not about `main`: | declaration | where | |---|---| | `ExecutionStatus` member `refused` | `packages/spec/src/automation/execution.zod.ts:38` | | `outcome: z.enum(['completed','refused']).default('completed')` | `packages/spec/src/automation/builtin-node-config.zod.ts:658` | | refinement — `refused` REQUIRES a `message`; a `message` on a `completed` end is refused as a silent no-op | same file, `:671-690` | | `ExecutionLog.refusalMessage` | `execution.zod.ts:369` | | `AutomationResult.status` gains `refused`, plus `AutomationResult.refusalMessage` | `packages/spec/src/contracts/automation-service.ts:405,430` | | `TriggerFlowResponseSchema.data.status` / `.refusalMessage` | `packages/spec/src/api/automation-api.zod.ts:356,382` | ⛔ `packages/spec` is untouched by this PR. Nothing in the implementation needed a spec change — the lane-1 contract was complete, including the `refusalMessage` run-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): *"`refused` is 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): *"`refused` is deliberately ABSENT: `ExecutionStatus` declares it (#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`'s `end` branch.** It opened with `if (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's `parseEndNodeConfig` runs inside `FlowNodeSchema`, regions included, so `outcome` is defaulted and a `refused` with no `message` was already refused at the flow parse — no second door, no `??`, no re-parse) and throws a `FlowRefusalSignal`, the twin of the existing `FlowSuspendSignal`. - **One terminal shape, three producers.** `finishRefusedRun` is called from `execute()`, `resumeInternal` and `executeWithoutRetry`. That chokepoint is not stylistic: this exact file lost `successMessage` (#9414) and the durable pause (#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 under `strategy: 'retry'` now answer identically. - **The envelope.** `success: true` (a refusal is a successful evaluation that says no), `status: 'refused'`, `refusalMessage`, no `error`, no `errorMessage`, ⛔ no `successMessage`, ⛔ no `runId`. The retry ladder stops on it without a new branch — `retryExecution` already reads `result.success` as "this attempt did not fail, stop retrying", which is the true sentence here; ⛔ a refusal must never consume retry budget. - **Persistence.** `RunRecord.refusalMessage` + a new `sys_automation_run.refusal_message` column, written always (NULL included — `recordTerminal` is an upsert, so a rewritten row must CLEAR a refusal it no longer carries) and read back through `loadTerminal` / `runRecordToLogEntry`. ⛔ Not folded into `error`: text in `error` tells every reader — an operator, the Runs surface, a sweep filtering `error IS NOT NULL` — that the run broke. Same reason #15223 stopped folding `cancelled` into `failed`. - **Never resumed.** A refusal writes no continuation, so `resume()` answers `RUN_NOT_FOUND`. - **`packages/plugin-approvals`, comment only.** Its `RUN_STATUS_LIVENESS` docblock said the service-automation vocabulary *"excludes `refused` on purpose"*. This PR makes that false, so the paragraph is corrected in the same change. No behaviour moves: that map already classified `refused` as terminal, and it stays the authority for its own sweep. ### The region boundary, made loud An `end` declaring `outcome: 'refused'` **inside a structured region** is refused with a named message, at the same line where `runRegion` already refuses a durable pause. Left to propagate, the signal would unwind into the `try_catch` executor's own `catch (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 record `completed`: the pre-#15788 silence with an extra step. ⛔ Nothing an author had is narrowed: before this PR an `end` in a region was a no-op whatever its `outcome`, 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. | run | command | result | |---|---|---| | BEFORE (unfixed engine) | `vitest run src/end-node-refused-outcome.test.ts` | **7 failed, 4 passed (11)** | | AFTER | same file, + the region pin | **12 passed (12)** | | AFTER | `pnpm --filter @objectstack/service-automation test` | **135 files, 1597 tests, all passing** | | AFTER | `pnpm --filter @objectstack/plugin-approvals test` | **45 files, 738 tests, all passing** | The BEFORE failures were the shape of the defect, not of a broken harness: ``` AssertionError: expected undefined to be 'refused' AssertionError: expected undefined to be 'Refused: Acme Corp is a confirmed dup…' AssertionError: expected 'completed' to be 'refused' ``` The 4 that passed BEFORE are the fences, green on both sides on purpose: a plain `end` still completes with `successMessage`, an explicit `outcome: 'completed'` is the same completion, a genuinely failed run still reads `failed` (the discriminating control — without it, "the refusal path reads `refused`" would be consistent with an engine that had started calling everything `refused`), and a refused run was already not resumable because it writes no continuation. ## The interpolation is the screen-`description` one, and that is pinned by mechanism The ruling: the refusal message goes through *the same interpolation a screen `description` gets* — one implementation, ⛔ never a second template engine. The implementation is `interpolate()` in `packages/services/service-automation/src/builtin/template.ts`, reached through the four-line coercion that `screen-nodes.ts` held in a local `interp` closure and read at `screen.description` (`screen-nodes.ts:195` and `:279`). Those four lines are hoisted verbatim to `template.ts` as `interpolateText`; `interp` now 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 carry `trap … EXIT INT TERM` with absolute paths; the trap is the crash convenience, the hash compare is the proof. **Leg 1 — remove the refusal branch (the fix itself).** ``` HEAD blob : 86fede4 anchor count before : 1 inject count before : 0 anchor count after : 0 inject count after : 1 on-disk mutated : 1d64fe18affedec59afaa230a4b7d92cf4c21951 (differs — not a no-op) ABLATED TEST EXIT : 1 Tests 8 failed | 56 passed (64) on-disk restored : 86fede4 RESTORE: byte-identical to HEAD git diff HEAD / git status --porcelain : empty ``` 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: - The 4 fences stayed green, because none of them depends on the branch that was removed. A fence that reddened here would mean the change had reached something the ruling names ⛔ do not touch. - All 52 `suspended-run-store.test.ts` cases stayed green, **including the 4 new `refusal_message` ones**, and that is correct rather than a gap: they drive `ObjectStoreSuspendedRunStore` with a `RunRecord` directly and never enter `executeNode`. They pin the persistence layer; leg 1 mutated the producer. - The mutation was on `src/`, and the test file imports `./engine.js` from inside the same package, so vitest resolves it from source. The RED itself is the proof of that resolution path — a stale-`dist` reading 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). ``` anchor count after : 0 inject count after : 2 on-disk mutated : 79dd7a425bce3252d526de885b1c8826451e089c (differs) SECOND-ENGINE TEST EXIT : 1 × renders a refusal `message` byte-identically to a screen `description` AssertionError: refusal message for by {$User.Id}: expected 'by ' to be 'by usr_7' Tests 1 failed | 11 passed (12) on-disk restored : 86fede4 RESTORE: byte-identical to HEAD ``` 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.ts` re-exports plus the package's `exports`/`files`) plus any new key on a published payload. `@objectstack/service-automation` publishes `["dist","README.md","CHANGELOG.md"]` with one entry, `./dist/index.d.ts`. After `pnpm --filter @objectstack/service-automation build`: | carrier | evidence | |---|---| | `RunRecord` gains `refusalMessage?: string` | `dist/index.d.ts:743`, and `type RunRecord` is in the entry's export list — a NEW KEY on an already-published payload, the mandatory `yes` | | `TerminalRunStatus` widens 4 members to 5 | `dist/index.d.ts:694-696`, `type TerminalRunStatus` exported; the runtime value ships too (`dist/index.js`: `TERMINAL_RUN_STATUSES = ["completed","failed","cancelled","timed_out","refused"]`) | | `SysAutomationRun` gains the `refusal_message` field and the `refused` option | `dist/index.d.ts:9339`, `SysAutomationRun` exported as a value | ⛔ The terminal status is NOT what carries it: `refused` was already a declared `ExecutionStatus` member, so `status` is 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": | probe | hits in `dist/index.d.ts` | reads as | |---|---|---| | `interpolateText` — new in this diff, internal to `builtin/` | **0** | in the diff, NOT published | | `isTerminalRunStatus` — exported from `engine.ts`, not re-exported from the barrel | **0** | exists in source, NOT on the published surface | | `RunRecord` — published before this diff | 18 | positive control: the probe can see published symbols | | `zzzNotASymbol` | 0 | negative control | A probe that answered ">0" for `interpolateText` and `isTerminalRunStatus` would have been measuring file text rather than the published surface. It did not. The changeset is graded **`minor`**, as the ruling grades this lane — never `patch`. ## Gates Derived mechanically from the real change set, not from the dispatch list, and reconciled with `--ran`: ``` node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack → 9 path(s) vs merge base 3aaea38 · 65 command(s) derived node scripts/pm/dispatch-gates.mjs --ran ran.txt --repo objectstack-ai/objectstack → Run reconciliation — 65 derived, 65 run, 0 NOT-MEASURED, 0 UNRUN. EXIT CODES — all 65 accounted famil(ies) carry one, so the NOT-MEASURED count above is DERIVED from them. ``` **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-loads` and `check:i18n` exited **3 — PREREQUISITE NOT MET** ("nothing was measured") until `turbo run build --filter='./packages/*' --filter='./packages/*/*'`; both **exit 0** after. - `check:type-check-debt` exited 3 twice: once for the same unbuilt closure, then on an OOM at the `--max-old-space-size=4096` this 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 its `check:test-typecheck` leg compiles the test layer, so the new pins are type-checked rather than merely executed. - **Consumer sweep, downstream direction** (`--filter '...@objectstack/service-automation'`, the direction a contract widening lands in): **18 packages** typecheck green — `cli`, `client`, `client-react`, the four `connectors/*`, `plugin-approvals`, both `triggers/*`, `verify`, `qa/dogfood`, `qa/downstream-contract`, the four `examples/*`, and `service-automation` itself. - `pnpm lint` (= `eslint . --no-inline-config`) over the **whole repo**: **exit 0**, no findings. Run at `60ca35a2f`, after the final commit — no narrowing to justify. `origin/main` was merged at `3aaea3879` through `scripts/pm/os-regen-merge.sh`; it left no generated artifact to regenerate and no `os-regen-pending` deferral, and `pnpm install --frozen-lockfile` was re-run afterwards per the stale-artefact rule. ## Acceptance notes Out of scope, noted rather than filed — each names who would meet it: - **A `subflow` CHILD that refuses is rolled up as an ordinary success by its parent.** `subflow-node.ts` branches only on `child.status === 'paused'`, so a refused child returns `success: true` and 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 #14945 ruling seat, or whoever takes lane 3. - **`plugin.ts` describes the retention scope as `{ status: { $in: ['completed', 'failed'] } }` in two comments** (`:138`, `:787`) while the object has declared four members since #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 touch `sys_automation_run`'s retention. - The naming hazard the triage seat measured is real and is answered in code rather than restated: `refused` in this package overwhelmingly means a GUARD refusal (the engine declining to execute — a failure), and this lane's `refused` means a successful evaluation that said no. The disambiguation is written at both definition sites (`FlowRefusalSignal`, and the pin file's header) so a future `grep` reads the sense at the site rather than from the word. --- _Generated by [Claude Code](https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 7c7e76f commit cca6991

9 files changed

Lines changed: 964 additions & 38 deletions

File tree

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
---
2+
'@objectstack/service-automation': minor
3+
---
4+
5+
Flow `end` nodes honour `outcome: 'refused'` — a terminal `refused` run, distinct from `failed`
6+
7+
`packages/spec` has declared the shape since 17.4.0: an `end` node accepts
8+
`outcome: 'completed' | 'refused'`, a `refused` end requires a `message`,
9+
`ExecutionStatus` carries `refused`, and `ExecutionLog` / `AutomationResult` /
10+
the trigger response carry `refusalMessage`. The engine produced none of it —
11+
it returned on every `end` node without reading its config — so an author who
12+
wrote a refusal shipped a plain completion: the run recorded `completed`, the
13+
caller got the flow's `successMessage`, and the authored reason reached nobody.
14+
15+
The `end` node now honours it:
16+
17+
- **The run terminates `refused`.** A refusal is a *successful evaluation that
18+
says no*, so the result is `success: true, status: 'refused'` with no `error`
19+
and no `errorMessage` — and, deliberately, no `successMessage`: the flow's
20+
completion toast is for a completion. All three terminal producers answer
21+
identically (a triggered run, a resumed screen flow, and an attempt under
22+
`errorHandling.strategy: 'retry'`, where a refusal also stops the ladder
23+
rather than consuming retry budget).
24+
- **The `message` is rendered per record**, through the same interpolation a
25+
`screen` node's `description` gets — one implementation (`interpolateText`),
26+
never a second template engine — so `'Refused: {record.name} is a confirmed
27+
duplicate'` reaches the caller naming the record.
28+
- **Both are persisted on the run.** `sys_automation_run.status` gains
29+
`refused` and a new `refusal_message` column carries the rendered text; the
30+
refusal is never folded into `error`, which would tell every reader the run
31+
broke. `RunRecord` gains `refusalMessage` and `TerminalRunStatus` gains
32+
`refused`, so history rows are written, aged and read back like any other
33+
terminal.
34+
- **A refused run is never resumed.** It writes no continuation, so `resume`
35+
answers `RUN_NOT_FOUND`.
36+
37+
Untouched on purpose: a paused run still returns `silent` with no
38+
`successMessage`, and a plain `end` — or one declaring `outcome: 'completed'` —
39+
completes exactly as before.
40+
41+
An `end` declaring `outcome: 'refused'` **inside a structured region** (a `loop`
42+
body, a `try`/`catch` region) is refused loudly rather than honoured: a refusal
43+
terminates the run and a region body cannot end one. Previously such a node was
44+
a silent no-op like every other `end` in a region, so nothing that ever worked
45+
stops working — put the refusing `end` on the top-level graph and route the
46+
region's exit to it.

‎packages/plugins/plugin-approvals/src/approval-service.ts‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -354,13 +354,19 @@ type RunLiveness = 'terminal' | 'live';
354354
* `ExecutionStatus.options` and drives every member through the real sweep, so
355355
* the classification and the behaviour cannot drift apart either.
356356
*
357-
* ⛔ Terminality is NOT declared machine-readably anywhere today — `refused`'s
357+
* ⛔ Terminality is NOT declared machine-readably in the SPEC — `refused`'s
358358
* terminality lives in a COMMENT beside the enum member, and a comment is not a
359359
* gate. The `TERMINAL_RUN_STATUSES` exported by `@objectstack/service-automation`
360360
* is a DIFFERENT vocabulary (which terminal states a run may be RECORDED in,
361-
* tied to `sys_automation_run.status`' options) that excludes `refused` on
362-
* purpose, and that package is only a devDependency here. Hence a local total
363-
* map rather than a shared import; see the card for the spec-level proposal.
361+
* tied to `sys_automation_run.status`' options), and that package is only a
362+
* devDependency here. Hence a local total map rather than a shared import; see
363+
* the card for the spec-level proposal.
364+
*
365+
* [#15788] That list used to exclude `refused` on purpose — nothing could write
366+
* the value — and now includes it, because the `end` executor produces it. The
367+
* two vocabularies AGREE about `refused` today; they are still not the same
368+
* question, so this map stays the authority for THIS sweep. ⛔ Do not replace it
369+
* with an import on the strength of one member currently matching.
364370
*
365371
* `completed` is classified terminal alongside the failure states. The approval
366372
* node only writes a request row on the path where it also suspends the run,

‎packages/services/service-automation/src/builtin/screen-nodes.ts‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import type { PluginContext } from '@objectstack/core';
44
import { defineActionDescriptor, ScreenConfigSchema, ScriptConfigSchema } from '@objectstack/spec/automation';
55
import type { ScreenConfigParsed, ScriptConfigParsed } from '@objectstack/spec/automation';
66
import type { AutomationEngine } from '../engine.js';
7-
import { interpolate } from './template.js';
7+
import { interpolate, interpolateText } from './template.js';
88
import { parseNodeConfig } from './parse-config.js';
99
import { judgeHeadlessScreen } from '../screen-input-contract.js';
1010

@@ -163,11 +163,13 @@ export function registerScreenNodes(engine: AutomationEngine, ctx: PluginContext
163163
// variables here (the engine does NOT pre-interpolate node config) — so
164164
// a step's title/description/field-default/object-form-default can pull
165165
// from prior nodes (e.g. `{lead_record.company}`, `{account_id}`).
166-
const interp = (v: unknown): string | undefined => {
167-
if (v == null) return undefined;
168-
const r = interpolate(v, variables, context);
169-
return r == null ? undefined : String(r);
170-
};
166+
//
167+
// [#15788] The body of this closure now lives in `template.ts` as
168+
// {@link interpolateText}, because a second authored-text slot — the
169+
// refusing `end` node's `message` (#14945 lane 2) — has to render
170+
// through THE SAME implementation, not a copy of it. Same bytes in,
171+
// same bytes out; the only change is where the four lines live.
172+
const interp = (v: unknown): string | undefined => interpolateText(v, variables, context);
171173

172174
// ── Object-form screen (master-detail wizards) ──────────────────────
173175
// When the step names an `objectName`, render that object's FULL

‎packages/services/service-automation/src/builtin/template.ts‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,36 @@ export function interpolateString(
353353
);
354354
}
355355

356+
/**
357+
* Render an authored TEXT slot — a screen `title` / `description`, an `end`
358+
* node's refusal `message` — through {@link interpolate}, coerced to a string.
359+
*
360+
* [#15788] Hoisted out of `screen-nodes.ts`'s local `interp` closure so the
361+
* refusing `end` node (#14945 lane 2) renders through the SAME implementation
362+
* rather than a second one. The ruling's words are the requirement: the
363+
* refusal message "goes through the same interpolation a screen `description`
364+
* gets" — ⛔ never a second template engine. A second spelling would start
365+
* byte-identical and drift on the first fix that landed in only one of them,
366+
* and the drift would be invisible from either side: both would still
367+
* substitute `{record.name}`.
368+
*
369+
* Absent in, absent out — a slot the author left unset renders nothing rather
370+
* than the string `"undefined"`, and a whole-string token that resolved to
371+
* `null` is the same "nothing" ({@link interpolateString} preserves the raw
372+
* value for a single-token string, so an unresolved `{missing}` arrives here as
373+
* `null`). Every other value is stringified exactly as an embedded
374+
* substitution would be, which is what keeps ONE rendering for both slots.
375+
*/
376+
export function interpolateText(
377+
value: unknown,
378+
variables: VariableMap,
379+
context: AutomationContext,
380+
): string | undefined {
381+
if (value == null) return undefined;
382+
const rendered = interpolate(value, variables, context);
383+
return rendered == null ? undefined : String(rendered);
384+
}
385+
356386
/**
357387
* Recursively interpolate template tokens in arbitrary JSON-like values.
358388
*/

0 commit comments

Comments
 (0)