From f525fb87f4ce899f70fac32a9379cddce5571063 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 02:11:50 +0000 Subject: [PATCH 1/3] test(service-automation): pin the refusing `end` node before implementing 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 --- .../src/end-node-refused-outcome.test.ts | 383 ++++++++++++++++++ 1 file changed, 383 insertions(+) create mode 100644 packages/services/service-automation/src/end-node-refused-outcome.test.ts diff --git a/packages/services/service-automation/src/end-node-refused-outcome.test.ts b/packages/services/service-automation/src/end-node-refused-outcome.test.ts new file mode 100644 index 0000000000..7ad6e77862 --- /dev/null +++ b/packages/services/service-automation/src/end-node-refused-outcome.test.ts @@ -0,0 +1,383 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #15788 — the `end` executor honours `outcome: 'refused'` (lane 2 of the + * #14945 ruling 2′). + * + * `packages/spec` landed the contract first (lane 1): `ExecutionStatus` gained + * `refused`, `EndConfigSchema` gained `outcome: 'completed' | 'refused'` with a + * `message` that is REQUIRED beside a refusal and REFUSED beside a completion, + * `ExecutionLogSchema` gained `refusalMessage`, and `AutomationResult` / + * `TriggerFlowResponseSchema` gained both. Nothing in this package produced any + * of it: `executeNode` opened with `if (node.type === 'end') return;`, so an + * author's refusal ran as a plain completion — the run recorded `completed`, + * the caller got the flow's `successMessage`, and the authored reason reached + * nobody. Declared, never enforced. + * + * ⚠️ Direction, predicted before running. The tests under "the defect" FAIL + * against the unfixed engine — `undefined` / `'completed'` where the refusal is + * expected. The ones under "the boundary" are green on both sides ON PURPOSE: + * they fence the change rather than demonstrate it, and each says which side it + * guards. The `successMessage` / `silent` pins are of that second kind — + * the ruling names them ⛔ do not touch, so a pin that reddened would mean the + * change had reached something it must not. + * + * ⚠️ `refused` is NOT the `refused` this package says everywhere else. A guard + * refusal (`guard-refusal.ts`, `refuseNode`, the resume-authority gate) means + * "the engine refused to execute" — a FAILURE. This file's `refused` is the + * terminal run outcome the ruling defines: *a refusal is a successful + * evaluation that says no*. Nearly opposite senses, same word, so grep hits in + * this package are mostly the other family. + */ + +import { describe, it, expect } from 'vitest'; + +import { AutomationEngine } from './engine.js'; +import { InMemorySuspendedRunStore } from './suspended-run-store.js'; +import { installBuiltinNodes } from './builtin/index.js'; +import type { AutomationContext } from '@objectstack/spec/contracts'; +import { defineActionDescriptor } from '@objectstack/spec/automation'; + +function createTestLogger(): any { + return { info: () => {}, warn: () => {}, error: () => {}, debug: () => {}, child: () => createTestLogger() }; +} + +/** The flow author's completion toast — must never ride a refusal. */ +const SUCCESS_TEXT = 'Account created — the owner has been notified.'; +/** The authored refusal template. `{record.name}` is what makes it per-record. */ +const REFUSAL_TEMPLATE = 'Refused: {record.name} is a confirmed duplicate'; + +/** + * A two-node flow whose `end` node carries the config under test. + * `successMessage` is always declared, so every refusal assertion doubles as a + * "the completion toast stayed silent" assertion. + */ +function endFlow(name: string, endConfig?: Record) { + return { + name, + label: name, + type: 'autolaunched', + successMessage: SUCCESS_TEXT, + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'finish', type: 'end', label: 'Finish', ...(endConfig ? { config: endConfig } : {}) }, + ], + edges: [{ id: 'e0', source: 'start', target: 'finish' }], + }; +} + +/** The two records of the fixture the ruling names — per-record text, not one constant. */ +const ACME = { id: 'rec_1', name: 'Acme Corp' } as const; +const GLOBEX = { id: 'rec_2', name: 'Globex Industries' } as const; + +function ctxFor(record: Record): AutomationContext { + return { event: 'manual', object: 'account', record } as unknown as AutomationContext; +} + +function engineWithStore() { + const store = new InMemorySuspendedRunStore(); + const engine = new AutomationEngine(createTestLogger(), store); + return { engine, store }; +} + +/** The newest run id for a flow — a refusal carries no `runId` (only a pause does). */ +async function newestRunId(engine: AutomationEngine, flowName: string): Promise { + const runs = await engine.listRuns(flowName, { limit: 5 }); + expect(runs.length).toBeGreaterThan(0); + return runs[0]!.id; +} + +describe('#15788 — the defect: a refusing `end` ran as a plain completion', () => { + it('a `refused` end terminates the run with status `refused` and the rendered message', async () => { + const { engine } = engineWithStore(); + engine.registerFlow('dedupe', endFlow('dedupe', { outcome: 'refused', message: REFUSAL_TEMPLATE }) as never); + + const result = await engine.execute('dedupe', ctxFor({ ...ACME })); + + // A refusal is a SUCCESSFUL evaluation that says no — `success` stays + // true and the run is not an error. + expect(result.success).toBe(true); + expect(result.status).toBe('refused'); + expect(result.refusalMessage).toBe('Refused: Acme Corp is a confirmed duplicate'); + // ⛔ Distinct from `failed`, in both directions: nothing threw, so + // there is no `error` and no `errorMessage`. + expect(result.error).toBeUndefined(); + expect(result.errorMessage).toBeUndefined(); + }); + + it('interpolates PER RECORD — the two-record fixture the ruling names', async () => { + const { engine } = engineWithStore(); + engine.registerFlow('dedupe', endFlow('dedupe', { outcome: 'refused', message: REFUSAL_TEMPLATE }) as never); + + const first = await engine.execute('dedupe', ctxFor({ ...ACME })); + const second = await engine.execute('dedupe', ctxFor({ ...GLOBEX })); + + expect(first.refusalMessage).toBe('Refused: Acme Corp is a confirmed duplicate'); + expect(second.refusalMessage).toBe('Refused: Globex Industries is a confirmed duplicate'); + // The template itself never reaches a caller — that is the whole point + // of rendering it here rather than on the wire. + expect(first.refusalMessage).not.toContain('{record.name}'); + }); + + it('the run RECORD carries the outcome and the rendered message', async () => { + const { engine, store } = engineWithStore(); + engine.registerFlow('dedupe', endFlow('dedupe', { outcome: 'refused', message: REFUSAL_TEMPLATE }) as never); + + await engine.execute('dedupe', ctxFor({ ...ACME })); + const runId = await newestRunId(engine, 'dedupe'); + + // The observability surface (`GET /automation/:name/runs/:runId`). + const entry = await engine.getRun(runId); + expect(entry?.status).toBe('refused'); + expect(entry?.refusalMessage).toBe('Refused: Acme Corp is a confirmed duplicate'); + + // …and the DURABLE row behind it, which is what survives a restart. + const record = await store.loadTerminal!(runId); + expect(record?.status).toBe('refused'); + expect(record?.refusalMessage).toBe('Refused: Acme Corp is a confirmed duplicate'); + }); + + it('a restarted process reads the refusal back off the store, not the ring', async () => { + // The second engine shares the store and has an EMPTY in-memory ring, + // so `getRun` can only answer from the durable row — the read path a + // real operator takes after a restart. + const store = new InMemorySuspendedRunStore(); + const writer = new AutomationEngine(createTestLogger(), store); + writer.registerFlow('dedupe', endFlow('dedupe', { outcome: 'refused', message: REFUSAL_TEMPLATE }) as never); + await writer.execute('dedupe', ctxFor({ ...GLOBEX })); + const runId = await newestRunId(writer, 'dedupe'); + + const reader = new AutomationEngine(createTestLogger(), store); + const entry = await reader.getRun(runId); + + expect(entry?.status).toBe('refused'); + expect(entry?.refusalMessage).toBe('Refused: Globex Industries is a confirmed duplicate'); + }); + + it('a refused run is NOT resumable — `refused` is terminal', async () => { + const { engine } = engineWithStore(); + engine.registerFlow('dedupe', endFlow('dedupe', { outcome: 'refused', message: REFUSAL_TEMPLATE }) as never); + + await engine.execute('dedupe', ctxFor({ ...ACME })); + const runId = await newestRunId(engine, 'dedupe'); + + const resumed = await engine.resume(runId, { variables: {} } as never); + + // ADR-0112 envelope, not a bare throw: the run has no suspension, so + // there is nothing for the resume verb to continue. + expect(resumed.success).toBe(false); + expect(resumed.code).toBe('RUN_NOT_FOUND'); + }); + + it('a refusal reached through a RESUMED screen flow is a refusal too', async () => { + // The second producer. `resumeInternal` is a separate terminal exit + // from `execute()`'s — the #9414 asymmetry in this same file's + // neighbourhood — so a repair that stopped at `execute()` would leave + // every screen flow's refusal recorded as a completion. + const { engine, store } = engineWithStore(); + installBuiltinNodes( + { logger: createTestLogger(), getService: () => undefined } as never, + engine, + ); + engine.registerFlow('review', { + name: 'review', + label: 'review', + type: 'screen', + successMessage: SUCCESS_TEXT, + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'ask', type: 'screen', label: 'Ask', + config: { waitForInput: true, title: 'Confirm', fields: [{ name: 'ok', label: 'OK', type: 'checkbox' }] }, + }, + { id: 'finish', type: 'end', label: 'Finish', config: { outcome: 'refused', message: REFUSAL_TEMPLATE } }, + ], + edges: [ + { id: 'e0', source: 'start', target: 'ask' }, + { id: 'e1', source: 'ask', target: 'finish' }, + ], + } as never); + + const paused = await engine.execute('review', ctxFor({ ...ACME })); + expect(paused.status).toBe('paused'); + // ⛔ The `silent` pause contract, untouched: a paused run carries NO + // completion toast. The ruling names this ⛔ do not touch. + expect(paused.successMessage).toBeUndefined(); + + const resumed = await engine.resume(paused.runId!, { variables: { ok: true } } as never); + + expect(resumed.success).toBe(true); + expect(resumed.status).toBe('refused'); + expect(resumed.refusalMessage).toBe('Refused: Acme Corp is a confirmed duplicate'); + expect(resumed.successMessage).toBeUndefined(); + + const record = await store.loadTerminal!(paused.runId!); + expect(record?.status).toBe('refused'); + expect(record?.refusalMessage).toBe('Refused: Acme Corp is a confirmed duplicate'); + }); + + it('a refusal on a RETRY attempt is a refusal, not a retried failure', async () => { + // `errorHandling.strategy: 'retry'` hands the run to `retryExecution`, + // whose attempts leave through `executeWithoutRetry` — a third terminal + // exit. A refusal there must stop the ladder (a refusal is not a + // failure to retry) and carry the same envelope. + const { engine } = engineWithStore(); + let attempts = 0; + engine.registerNodeExecutor({ + type: 'flaky', + async execute() { + attempts++; + return attempts === 1 ? { success: false, error: 'transient 503' } : { success: true }; + }, + } as never); + engine.registerFlow('dedupe_retry', { + name: 'dedupe_retry', + label: 'dedupe_retry', + type: 'autolaunched', + successMessage: SUCCESS_TEXT, + errorHandling: { strategy: 'retry', maxRetries: 1, backoffMs: 0 }, + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'work', type: 'flaky', label: 'Work' }, + { id: 'finish', type: 'end', label: 'Finish', config: { outcome: 'refused', message: REFUSAL_TEMPLATE } }, + ], + edges: [ + { id: 'e0', source: 'start', target: 'work' }, + { id: 'e1', source: 'work', target: 'finish' }, + ], + } as never); + + const result = await engine.execute('dedupe_retry', ctxFor({ ...ACME })); + + expect(attempts).toBe(2); // the ladder really ran + expect(result.success).toBe(true); + expect(result.status).toBe('refused'); + expect(result.refusalMessage).toBe('Refused: Acme Corp is a confirmed duplicate'); + }); +}); + +describe('#15788 — the boundary: what must NOT change', () => { + // ⚠️ Green before and after, deliberately. Each fences a behaviour the + // ruling names ⛔ do not touch, or a default the change must leave alone. + + it('a plain `end` (no config) still completes and still carries successMessage', async () => { + const { engine } = engineWithStore(); + engine.registerFlow('plain', endFlow('plain') as never); + + const result = await engine.execute('plain', ctxFor({ ...ACME })); + + expect(result.success).toBe(true); + expect(result.status).toBeUndefined(); // the terminal-success exit stamps none + expect(result.successMessage).toBe(SUCCESS_TEXT); + expect(result.refusalMessage).toBeUndefined(); + + const entry = await engine.getRun(await newestRunId(engine, 'plain')); + expect(entry?.status).toBe('completed'); + expect(entry?.refusalMessage).toBeUndefined(); + }); + + it('an explicit `outcome: \'completed\'` end is the same completion', async () => { + const { engine } = engineWithStore(); + engine.registerFlow('explicit', endFlow('explicit', { outcome: 'completed' }) as never); + + const result = await engine.execute('explicit', ctxFor({ ...ACME })); + + expect(result.success).toBe(true); + expect(result.successMessage).toBe(SUCCESS_TEXT); + expect(result.refusalMessage).toBeUndefined(); + expect((await engine.getRun(await newestRunId(engine, 'explicit')))?.status).toBe('completed'); + }); + + it('a genuinely FAILED run still reads `failed` — the discriminating control', async () => { + // Without this, "the refusal path reads `refused`" would be consistent + // with an engine that had started calling everything `refused`. + const { engine } = engineWithStore(); + engine.registerNodeExecutor({ + type: 'boom', + async execute() { return { success: false, error: 'downstream 503' }; }, + } as never); + engine.registerFlow('breaks', { + name: 'breaks', label: 'breaks', type: 'autolaunched', + successMessage: SUCCESS_TEXT, + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'work', type: 'boom', label: 'Work' }, + { id: 'finish', type: 'end', label: 'Finish', config: { outcome: 'refused', message: REFUSAL_TEMPLATE } }, + ], + edges: [ + { id: 'e0', source: 'start', target: 'work' }, + { id: 'e1', source: 'work', target: 'finish' }, + ], + } as never); + + const result = await engine.execute('breaks', ctxFor({ ...ACME })); + + expect(result.success).toBe(false); + expect(result.status).toBe('failed'); + expect(result.refusalMessage).toBeUndefined(); + expect((await engine.getRun(await newestRunId(engine, 'breaks')))?.status).toBe('failed'); + }); +}); + +describe('#15788 — one interpolator, not a second template engine', () => { + /** + * The ruling: the refusal message goes through *the same interpolation a + * screen `description` gets*. Asserting "it substitutes `{record.name}`" is + * far too weak — a hand-rolled `replace` would pass it. These drive both + * slots with templates whose behaviour is SPECIFIC to + * `builtin/template.ts`, and compare the two renderings for equality. + */ + const PROBES = [ + // Dotted path walk. + '{record.name}', + // Numeric segment indexing into an array. + 'first={record.tags.0}', + // Context token, resolved from `AutomationContext`, not from variables. + 'by {$User.Id}', + // The CEL-mirrored numeric stdlib (#11060) — nothing a naive + // substitution implements. + 'score {round(record.score)}', + // Unresolvable embedded token renders as the empty string, not the + // literal token and not `undefined`. + 'missing[{record.nope}]', + // Object-valued token is JSON-serialized, never `[object Object]` (#3450). + 'blob {record.meta}', + ]; + + it('renders a refusal `message` byte-identically to a screen `description`', async () => { + const record = { + id: 'rec_1', name: 'Acme Corp', tags: ['vip', 'eu'], score: 4.6, + meta: { tier: 'gold' }, + }; + const context = { event: 'manual', object: 'account', record, userId: 'usr_7' } as unknown as AutomationContext; + + for (const template of PROBES) { + const screenEngine = new AutomationEngine(createTestLogger(), new InMemorySuspendedRunStore()); + installBuiltinNodes( + { logger: createTestLogger(), getService: () => undefined } as never, + screenEngine, + ); + screenEngine.registerFlow('probe_screen', { + name: 'probe_screen', label: 'probe_screen', type: 'screen', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'ask', type: 'screen', label: 'Ask', + config: { waitForInput: true, title: 'T', description: template }, + }, + ], + edges: [{ id: 'e0', source: 'start', target: 'ask' }], + } as never); + + const refuseEngine = new AutomationEngine(createTestLogger(), new InMemorySuspendedRunStore()); + refuseEngine.registerFlow('probe_refuse', endFlow('probe_refuse', { outcome: 'refused', message: template }) as never); + + const paused = await screenEngine.execute('probe_screen', context); + const refused = await refuseEngine.execute('probe_refuse', context); + + expect(paused.screen?.description, `screen description for ${template}`).toBeDefined(); + expect(refused.refusalMessage, `refusal message for ${template}`) + .toBe(paused.screen!.description); + } + }); +}); From aeb015e2ce2efdde8dedd0a76380468597a4919e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 02:26:33 +0000 Subject: [PATCH 2/3] feat(service-automation): the `end` node honours outcome: 'refused' 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 --- .../plugin-approvals/src/approval-service.ts | 14 +- .../src/builtin/screen-nodes.ts | 14 +- .../src/builtin/template.ts | 30 ++ .../src/end-node-refused-outcome.test.ts | 65 +++- .../services/service-automation/src/engine.ts | 322 +++++++++++++++++- .../src/suspended-run-store.test.ts | 67 ++++ .../src/suspended-run-store.ts | 12 + .../src/sys-automation-run.object.ts | 66 ++-- 8 files changed, 544 insertions(+), 46 deletions(-) diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index fa932e35dd..9529b312d3 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -354,13 +354,19 @@ type RunLiveness = 'terminal' | 'live'; * `ExecutionStatus.options` and drives every member through the real sweep, so * the classification and the behaviour cannot drift apart either. * - * ⛔ Terminality is NOT declared machine-readably anywhere today — `refused`'s + * ⛔ Terminality is NOT declared machine-readably in the SPEC — `refused`'s * terminality lives in a COMMENT beside the enum member, and a comment is not a * gate. The `TERMINAL_RUN_STATUSES` exported by `@objectstack/service-automation` * is a DIFFERENT vocabulary (which terminal states a run may be RECORDED in, - * tied to `sys_automation_run.status`' options) that excludes `refused` on - * purpose, and that package is only a devDependency here. Hence a local total - * map rather than a shared import; see the card for the spec-level proposal. + * tied to `sys_automation_run.status`' options), and that package is only a + * devDependency here. Hence a local total map rather than a shared import; see + * the card for the spec-level proposal. + * + * [#15788] That list used to exclude `refused` on purpose — nothing could write + * the value — and now includes it, because the `end` executor produces it. The + * two vocabularies AGREE about `refused` today; they are still not the same + * question, so this map stays the authority for THIS sweep. ⛔ Do not replace it + * with an import on the strength of one member currently matching. * * `completed` is classified terminal alongside the failure states. The approval * node only writes a request row on the path where it also suspends the run, diff --git a/packages/services/service-automation/src/builtin/screen-nodes.ts b/packages/services/service-automation/src/builtin/screen-nodes.ts index a7a35921ee..c9f88d74df 100644 --- a/packages/services/service-automation/src/builtin/screen-nodes.ts +++ b/packages/services/service-automation/src/builtin/screen-nodes.ts @@ -4,7 +4,7 @@ import type { PluginContext } from '@objectstack/core'; import { defineActionDescriptor, ScreenConfigSchema, ScriptConfigSchema } from '@objectstack/spec/automation'; import type { ScreenConfigParsed, ScriptConfigParsed } from '@objectstack/spec/automation'; import type { AutomationEngine } from '../engine.js'; -import { interpolate } from './template.js'; +import { interpolate, interpolateText } from './template.js'; import { parseNodeConfig } from './parse-config.js'; import { judgeHeadlessScreen } from '../screen-input-contract.js'; @@ -163,11 +163,13 @@ export function registerScreenNodes(engine: AutomationEngine, ctx: PluginContext // variables here (the engine does NOT pre-interpolate node config) — so // a step's title/description/field-default/object-form-default can pull // from prior nodes (e.g. `{lead_record.company}`, `{account_id}`). - const interp = (v: unknown): string | undefined => { - if (v == null) return undefined; - const r = interpolate(v, variables, context); - return r == null ? undefined : String(r); - }; + // + // [#15788] The body of this closure now lives in `template.ts` as + // {@link interpolateText}, because a second authored-text slot — the + // refusing `end` node's `message` (#14945 lane 2) — has to render + // through THE SAME implementation, not a copy of it. Same bytes in, + // same bytes out; the only change is where the four lines live. + const interp = (v: unknown): string | undefined => interpolateText(v, variables, context); // ── Object-form screen (master-detail wizards) ────────────────────── // When the step names an `objectName`, render that object's FULL diff --git a/packages/services/service-automation/src/builtin/template.ts b/packages/services/service-automation/src/builtin/template.ts index 0d2de693e3..4727f4c76d 100644 --- a/packages/services/service-automation/src/builtin/template.ts +++ b/packages/services/service-automation/src/builtin/template.ts @@ -353,6 +353,36 @@ export function interpolateString( ); } +/** + * Render an authored TEXT slot — a screen `title` / `description`, an `end` + * node's refusal `message` — through {@link interpolate}, coerced to a string. + * + * [#15788] Hoisted out of `screen-nodes.ts`'s local `interp` closure so the + * refusing `end` node (#14945 lane 2) renders through the SAME implementation + * rather than a second one. The ruling's words are the requirement: the + * refusal message "goes through the same interpolation a screen `description` + * gets" — ⛔ never a second template engine. A second spelling would start + * byte-identical and drift on the first fix that landed in only one of them, + * and the drift would be invisible from either side: both would still + * substitute `{record.name}`. + * + * Absent in, absent out — a slot the author left unset renders nothing rather + * than the string `"undefined"`, and a whole-string token that resolved to + * `null` is the same "nothing" ({@link interpolateString} preserves the raw + * value for a single-token string, so an unresolved `{missing}` arrives here as + * `null`). Every other value is stringified exactly as an embedded + * substitution would be, which is what keeps ONE rendering for both slots. + */ +export function interpolateText( + value: unknown, + variables: VariableMap, + context: AutomationContext, +): string | undefined { + if (value == null) return undefined; + const rendered = interpolate(value, variables, context); + return rendered == null ? undefined : String(rendered); +} + /** * Recursively interpolate template tokens in arbitrary JSON-like values. */ diff --git a/packages/services/service-automation/src/end-node-refused-outcome.test.ts b/packages/services/service-automation/src/end-node-refused-outcome.test.ts index 7ad6e77862..75dfbd35b6 100644 --- a/packages/services/service-automation/src/end-node-refused-outcome.test.ts +++ b/packages/services/service-automation/src/end-node-refused-outcome.test.ts @@ -175,10 +175,7 @@ describe('#15788 — the defect: a refusing `end` ran as a plain completion', () // neighbourhood — so a repair that stopped at `execute()` would leave // every screen flow's refusal recorded as a completion. const { engine, store } = engineWithStore(); - installBuiltinNodes( - { logger: createTestLogger(), getService: () => undefined } as never, - engine, - ); + installBuiltinNodes(engine, { logger: createTestLogger(), getService: () => undefined } as never); engine.registerFlow('review', { name: 'review', label: 'review', @@ -353,10 +350,7 @@ describe('#15788 — one interpolator, not a second template engine', () => { for (const template of PROBES) { const screenEngine = new AutomationEngine(createTestLogger(), new InMemorySuspendedRunStore()); - installBuiltinNodes( - { logger: createTestLogger(), getService: () => undefined } as never, - screenEngine, - ); + installBuiltinNodes(screenEngine, { logger: createTestLogger(), getService: () => undefined } as never); screenEngine.registerFlow('probe_screen', { name: 'probe_screen', label: 'probe_screen', type: 'screen', nodes: [ @@ -381,3 +375,58 @@ describe('#15788 — one interpolator, not a second template engine', () => { } }); }); + +describe('#15788 — the region boundary, made loud', () => { + /** + * A refusal terminates the RUN, and a structured region's body cannot end + * one — the same statement `runRegion` already makes about a durable pause, + * at the same line, for the same reason. Left to propagate, the signal + * would unwind into `try_catch`'s own `catch (err)` arm, which reads every + * throw as the try region FAILING: the author's refusal would run the catch + * handler and the run would still record `completed`. + * + * ⛔ Nothing an author had is narrowed. Before #15788 an `end` inside a + * region was a no-op whatever its `outcome`, so this shape has never once + * been honoured; whether a refusal should instead propagate out of a region + * is a real question the #14945 ruling does not answer. + */ + it('a refusing `end` inside a `loop` body fails the run loudly instead of vanishing', async () => { + const { engine } = engineWithStore(); + installBuiltinNodes(engine, { logger: createTestLogger(), getService: () => undefined } as never); + engine.registerFlow('in_region', { + name: 'in_region', label: 'in_region', type: 'autolaunched', + successMessage: SUCCESS_TEXT, + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'sweep', type: 'loop', label: 'Sweep', + config: { + collection: '{items}', + iteratorVariable: 'item', + body: { + nodes: [{ id: 'nope', type: 'end', label: 'Nope', config: { outcome: 'refused', message: REFUSAL_TEMPLATE } }], + edges: [], + }, + }, + }, + ], + edges: [{ id: 'e0', source: 'start', target: 'sweep' }], + } as never); + + // `items` rides on the trigger record, which the engine flattens into + // the variable map — so `{items}` resolves without declaring an input. + const result = await engine.execute('in_region', { + event: 'manual', object: 'account', record: { ...ACME, items: [1] }, + } as unknown as AutomationContext); + + expect(result.success).toBe(false); + expect(result.status).toBe('failed'); + // The message names the shape and the one-line fix, per "absence must + // be loud" — ⛔ not a bare stringified sentinel. + expect(result.error).toContain('structured region'); + expect(result.error).toContain("outcome: 'refused'"); + // ⛔ And it is NOT recorded as a refusal: the run did not refuse, the + // engine declined to honour a shape it cannot express. + expect(result.refusalMessage).toBeUndefined(); + }); +}); diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index bba6374154..86fede46cf 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -212,6 +212,13 @@ import { summarizeRun, formatRunSummaryLine } from './run-summary.js'; // provider factory's text), so it renders it as structured `meta` rather than // interpolating it into the log message. See ./thrown-cause-diagnostics.ts. import { describeThrownForLog } from './thrown-cause-diagnostics.js'; +// [#15788] The refusing `end` node renders its `message` through the SAME +// interpolator a screen `description` gets — the ruling's own words, so a +// shared function rather than a second template engine. A value import from +// `./builtin/` is safe in this direction: `template.ts` imports only +// `../guard-refusal.js` and package-external contracts, so nothing it pulls in +// reaches back here. +import { interpolateText } from './builtin/template.js'; // ─── Node Executor Interface (Plugin Extension Point) ─────────────── @@ -996,6 +1003,16 @@ interface ExecutionLogEntry { variables?: Record; output?: unknown; error?: string; + /** + * [#15788] The rendered refusal, set only on a `refused` run — the `end` + * node's interpolated `message`. Not new vocabulary: `ExecutionLogSchema` + * (`@objectstack/spec`) declared `refusalMessage` when lane 1 landed, and + * this interface is "compatible with ExecutionLog from spec", so the key is + * consumed rather than invented. Absent on every other status — a failure's + * reason is the failing step's `error`, and a completion's `successMessage` + * is copied from the flow definition onto the RESULT, never stored here. + */ + refusalMessage?: string; /** * #4354: what the run did, folded out of the FULL step log by * {@link AutomationEngine.recordLog} — before history compaction, so a @@ -1072,6 +1089,47 @@ function isSuspendSignal(err: unknown): err is FlowSuspendSignal { return typeof err === 'object' && err !== null && (err as FlowSuspendSignal).__flowSuspend === true; } +/** + * [#15788] Internal sentinel thrown by {@link AutomationEngine.executeNode} + * when an `end` node declares `outcome: 'refused'` (#14945 ruling 2′, lane 2). + * The twin of {@link FlowSuspendSignal}: it unwinds the synchronous DAG + * recursion up to `execute()` / `resume()` / `executeWithoutRetry`, which + * convert it into a TERMINAL `refused` run rather than a failed one. + * + * ⚠️ `refused` here is the run OUTCOME, ⛔ not this package's other `refused`. + * Everywhere else in `service-automation` a refusal is a GUARD refusal — the + * engine declining to execute (`guard-refusal.ts`, `refuseNode`, the resume + * authority gate, `refuseUndeclaredSuspension`), which is a kind of FAILURE. + * This one is the maintainer's ruling verbatim: *a refusal is a successful + * evaluation that says no*. Nearly opposite senses of one word, so a `grep` for + * `refused` in this package returns mostly the other family — read the sense at + * the site, never from the name. + * + * Why a thrown sentinel and not a return value: an `end` node can sit anywhere + * in the graph, including several `traverseNext` frames deep, and the run has + * to STOP there. That is precisely what {@link FlowSuspendSignal} already + * exists to do, so this reuses the mechanism rather than inventing a second + * unwinding protocol. (Not exported — callers see `status: 'refused'`.) + */ +class FlowRefusalSignal { + readonly __flowRefused = true as const; + constructor( + /** The `end` node that refused — the last node the run reached. */ + readonly nodeId: string, + /** + * The author's `message`, already interpolated against the run's live + * variables. `undefined` only when the config carried none, which + * `EndConfigSchema`'s refinement refuses at the flow parse — recorded + * honestly rather than filled in with invented text. + */ + readonly message?: string, + ) {} +} + +function isRefusalSignal(err: unknown): err is FlowRefusalSignal { + return typeof err === 'object' && err !== null && (err as FlowRefusalSignal).__flowRefused === true; +} + /** * The definition-level input-schema guard's own throw type (#10025). * @@ -1284,16 +1342,23 @@ export interface SuspendedRun { * it: the writer ({@link AutomationEngine.recordLog}'s terminal predicate), * the reader (`ObjectStoreSuspendedRunStore`'s row gate) and the stored * column (`sys_automation_run.status`, whose `Field.select` options and - * retention `onlyWhen` scope enumerate the same four). A second copy of this - * list is how a widened writer ends up with rows a reader filters away. + * retention `onlyWhen` scope enumerate the same members). A second copy of + * this list is how a widened writer ends up with rows a reader filters away. + * + * These are exactly the `ExecutionStatus` members (`@objectstack/spec`) that + * mean "this run has stopped and will not resume". `paused`, `running`, + * `pending` and `retrying` are live states with no history row. * - * These are exactly the four `ExecutionStatus` members (`@objectstack/spec`) - * that mean "this run has stopped and will not resume". `paused`, - * `running`, `pending` and `retrying` are live states with no history row; - * `refused` is declared by the spec but no engine path produces it today, so - * adding it here would enumerate a value nothing can write. + * [#15788] `refused` joins them, and the reason it was ABSENT is the reason it + * is here now: this list may only enumerate values something can write, and + * until lane 2 of the #14945 ruling landed, nothing could — the spec declared + * the member and `executeNode` returned on every `end` node without reading its + * config. {@link AutomationEngine.executeNode} now produces it, so all three + * sites widen together, in this change. ⛔ A refusal is NOT a failure: nothing + * threw, the flow evaluated successfully and said no, and the authored reason + * rides beside the status as `refusalMessage` rather than in `error`. */ -export const TERMINAL_RUN_STATUSES = ['completed', 'failed', 'cancelled', 'timed_out'] as const; +export const TERMINAL_RUN_STATUSES = ['completed', 'failed', 'cancelled', 'timed_out', 'refused'] as const; /** One member of {@link TERMINAL_RUN_STATUSES}. */ export type TerminalRunStatus = (typeof TERMINAL_RUN_STATUSES)[number]; @@ -1337,6 +1402,19 @@ export interface RunRecord { durationMs?: number; /** Failure reason for a `failed` run — what a designer needs to fix it. */ error?: string; + /** + * [#15788] The rendered refusal, set only when {@link status} is + * `'refused'` — the `end` node's `message` template interpolated against + * the run's variables at the moment the run reached it, so the stored text + * names the record (`Refused: Acme Corp is a confirmed duplicate`). + * + * ⛔ Deliberately NOT folded into {@link error}, which would have needed no + * new column: a refusal is not a failure, and a reader that finds authored + * text in `error` has been told the run broke. The ruling is explicit that + * the two are distinct, so the row carries them in distinct places — the + * same reason `#15223` refused to keep folding `cancelled` into `failed`. + */ + refusalMessage?: string; /** * The node this record is ABOUT. On an ordinary terminal record: the run's * last step. On a stranded run's record — `consumedSuspension` present, or @@ -4425,6 +4503,12 @@ export class AutomationEngine implements IAutomationService { }, steps: r.steps ?? [], error: r.error, + // [#15788] The persisted refusal text, so the run a caller opens + // after a restart still says WHY it refused. Without this line the + // status survives the round-trip and its reason does not, which is + // the worst of the two halves to lose: `refused` with no message is + // indistinguishable from a refusal nobody authored. + refusalMessage: r.refusalMessage, // #4354 — the PERSISTED summary, never re-folded from `r.steps`: // those are compacted (200 max), so recomputing here would report a // 5000-row sweep as having acted on a couple of hundred. @@ -5037,6 +5121,20 @@ export class AutomationEngine implements IAutomationService { summary, }; } catch (err: unknown) { + // [#15788] The run reached an `end` node declaring + // `outcome: 'refused'` (#14945 ruling 2′). Tested FIRST, beside the + // pause and for the same reason: this is NOT a failure either, and + // a signal recognised only by the arm below would be recorded as + // one. The shape is `finishRefusedRun`'s — one method, all three + // producers. + if (isRefusalSignal(err)) { + return this.finishRefusedRun({ + runId, flowName, flowVersion: flow.version, + startedAt, durationMs: Date.now() - startTime, + steps, flow, variables, + refusalMessage: err.message, context, + }); + } // A node asked to suspend the run (ADR-0019 durable pause). Snapshot // the live state, record a `paused` log, and return the run id so the // caller can later `resume()` it. This is NOT a failure. @@ -6359,6 +6457,22 @@ export class AutomationEngine implements IAutomationService { summary, }; } catch (err: unknown) { + // [#15788] A resumed run reached a refusing `end` — the SECOND + // producer, and the one a screen flow actually takes: a wizard + // that collects an answer and then refuses on it leaves through + // here, never through `execute()`'s exit. Tested first, beside + // the re-suspend, for the same reason it is tested first there. + if (isRefusalSignal(err)) { + return this.finishRefusedRun({ + runId, + flowName: run.flowName, + flowVersion: run.flowVersion, + startedAt: run.startedAt, + durationMs: Date.now() - run.startTime, + steps, flow, variables, + refusalMessage: err.message, context, + }); + } // Re-suspended at a downstream node: persist a fresh continuation. if (isSuspendSignal(err)) { const durationMs = Date.now() - run.startTime; @@ -8114,6 +8228,13 @@ export class AutomationEngine implements IAutomationService { finishedAt: entry.completedAt, durationMs: entry.durationMs, error: entry.error, + // [#15788] The rendered refusal, carried onto the durable row + // beside `status: 'refused'`. Set by exactly one producer + // ({@link AutomationEngine.finishRefusedRun}) and `undefined` + // on every other terminal record, so the store writes an + // explicit NULL there — an upsert must CLEAR it, or a run id + // reused by a restore would keep a refusal it no longer has. + refusalMessage: entry.refusalMessage, userId: entry.trigger?.userId, // [#10101] The two organization-attribution inputs, from the // run context (see the `recordLog` doc): the acting tenant is @@ -8201,6 +8322,113 @@ export class AutomationEngine implements IAutomationService { return entry; } + /** + * [#15788] Finish a run that reached an `end` node declaring + * `outcome: 'refused'` — record the terminal row and build the caller's + * result (#14945 ruling 2′, lane 2). + * + * **ONE method, three producers.** `execute()`, `resumeInternal` and + * `executeWithoutRetry` each own a terminal exit, and this file's own + * history is what makes a shared chokepoint non-negotiable here: the + * author's `successMessage` (#9414) and the durable pause (#9510) were each + * implemented at one exit and missing from the others, so the run's + * user-visible outcome became a function of WHICH ROUTE it took — a + * triggered run, a resumed screen flow and a run that succeeded on retry + * are the same situation reached three ways. Same rule as + * `seedRunVariables` (#9704) and `validateNodeInputSchemas` (#9889): one + * method holds the shape, every path calls it. + * + * The envelope, member by member, is the ruling: + * + * - `success: true` — the evaluation SUCCEEDED and said no. ⛔ Not + * `false`: a caller branching on `success` must not route a refusal into + * its error path, and the transport's `status: 'failed'` arm (#9378) + * must not claim it. + * - `status: 'refused'` — terminal, and DISTINCT from `failed`. Nothing + * threw, so there is no `error` and no `errorMessage`. + * - `refusalMessage` — the authored, already-rendered per-record text. + * - ⛔ no `successMessage`. The flow's completion toast is for a + * COMPLETION; stamping it here would toast "Account created!" over a + * refusal to create one. This is also the half the ruling protects from + * the other side — the paused-run `silent` contract is untouched + * because this arm never runs for a pause. + * - ⛔ no `runId`. That member's contract is "set when `status` is + * `'paused'`, so callers can resume it", and a refused run is never + * resumed — handing one back would advertise a verb that answers + * `RUN_NOT_FOUND`. + * + * The `recordLog` call is guarded exactly as the completion sites are + * (#16274 / #15555): a history write must never break the run that + * produced it, and `summary` is recomputed from the same pure function when + * the write had to be abandoned. + */ + private finishRefusedRun(args: { + runId: string; + flowName: string; + flowVersion?: number; + startedAt: string; + durationMs: number; + steps: StepLogEntry[]; + /** Read for its `isOutput` variable declarations — see `output` below. */ + flow: FlowParsed; + variables: Map; + refusalMessage?: string; + context?: AutomationContext; + }): AutomationResult { + // The run's declared outputs, collected exactly as the three completion + // exits collect them. A refusal is terminal, and the nodes BEFORE the + // refusing `end` really ran — withholding what they produced would make + // the caller's answer depend on how the run ended rather than on what + // the flow declared. + const output: Record = {}; + if (args.flow.variables) { + for (const v of args.flow.variables) { + if (v.isOutput) output[v.name] = args.variables.get(v.name); + } + } + let logged: ExecutionLogEntry | undefined; + try { + logged = this.recordLog({ + id: args.runId, + flowName: args.flowName, + flowVersion: args.flowVersion, + status: 'refused', + refusalMessage: args.refusalMessage, + startedAt: args.startedAt, + completedAt: new Date().toISOString(), + durationMs: args.durationMs, + trigger: buildRunTrigger(args.context), + steps: args.steps, + output, + }, args.context); + } catch (bookkeeping) { + // #4632 verdict: DURABILITY, so `error` — the same judgement and + // the same consequence as the completion sites', with one word + // changed: the caller is told the truthful thing (the run refused), + // which is exactly what makes the rest invisible from outside. + // Said ONCE per run. THIRD argument per `error(message, error?, + // meta?)`; the `Error` slot stays empty on purpose (#5575). + this.logger.error( + `[Automation] run '${args.runId}' of flow '${args.flowName}' REFUSED (an 'end' node with ` + + `outcome: 'refused') but its run-history bookkeeping threw, so its terminal history row ` + + `never landed — nothing retries it, the caller is told the run refused, and after the next ` + + `restart this run is invisible to the Runs surfaces while the approvals sweeps read it as ` + + `never-finished. The run itself is TERMINAL and must NOT be re-run, retried or resumed. ` + + `Fix the history failure in this record's meta.`, + undefined, + describeThrownForLog(bookkeeping), + ); + } + return { + success: true, + status: 'refused', + refusalMessage: args.refusalMessage, + output, + durationMs: args.durationMs, + summary: logged?.summary ?? summarizeRun(args.steps), + }; + } + /** * Compact a run's step log for durable history. Delegates to the region-aware * {@link compactStepLogForHistory} (#3234): under {@link MAX_PERSISTED_HISTORY_STEPS} @@ -8933,7 +9161,36 @@ export class AutomationEngine implements IAutomationService { context: AutomationContext, steps: StepLogEntry[], ): Promise { - if (node.type === 'end') return; + if (node.type === 'end') { + // [#15788] …unless the author declared a REFUSAL here (#14945 + // ruling 2′). `end` has no executor and no descriptor — it is + // structural (`FLOW_STRUCTURAL_NODE_TYPES`), which is why this + // method opens by returning on it — so this is the one place the + // outcome can be read, and until now nothing read it: a flow + // declaring `outcome: 'refused'` ran as a plain completion and the + // author's reason reached nobody. + // + // The config arrives PARSED. `EndConfigSchema` is applied by + // `FlowNodeSchema`'s own transform (`parseEndNodeConfig`, spec + // lane 1), which runs on every node including the ones nested in a + // region, so `outcome` is defaulted and a `refused` without a + // `message` was already refused at the flow parse. ⛔ No second + // door, no `??` default, no re-parse: this reads a contract that is + // already enforced rather than defending against it. + const endConfig = node.config as { outcome?: string; message?: string } | undefined; + if (endConfig?.outcome === 'refused') { + throw new FlowRefusalSignal( + node.id, + // The SAME interpolation a screen `description` gets — the + // ruling's words, one implementation (`interpolateText`). + // Rendered HERE, against the live variable map, because + // that is what makes the text per-record; a template on the + // wire would put the rendering in every runner. + interpolateText(endConfig.message, variables, context), + ); + } + return; + } // ADR-0044 runaway guard: declared back-edges make re-entering a node // legal, so a misauthored unconditional loop could otherwise spin @@ -9543,6 +9800,30 @@ export class AutomationEngine implements IAutomationService { `durable pause inside a structured region (node '${err.nodeId}') is not supported`, ); } + // [#15788] The refusing `end` node's signal, converted at exactly + // the same boundary and for the same reason: a control signal must + // not cross a region edge silently. It would not merely leak — the + // `try_catch` executor's own `catch (err)` arm would read it as the + // try region FAILING and hand it to the catch handler, so an + // author's refusal would run the error path and the run would + // record `completed`. That is the pre-#15788 silence with an extra + // step, which is worse than a refusal that says so. + // + // ⛔ Not a narrowing of anything an author had: today an `end` in a + // region is 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. + if (isRefusalSignal(err)) { + throw new Error( + `an 'end' node declaring outcome: 'refused' inside a structured region (node ` + + `'${err.nodeId}') is not supported — a refusal terminates the RUN, and a region ` + + `body cannot end one. Put the refusing 'end' on the top-level graph and route the ` + + `region's exit to it.`, + ); + } throw err; } tag(); @@ -10479,6 +10760,29 @@ export class AutomationEngine implements IAutomationService { // work — the same route-dependent shape the fix is removing. return { success: true, output, durationMs, successMessage: flow.successMessage, summary }; } catch (err: unknown) { + // [#15788] The THIRD producer: an attempt that reached a refusing + // `end`. A flow under `errorHandling.strategy: 'retry'` is handed + // off to `retryExecution` and every one of its attempts leaves + // through this method, so a repair that stopped at `execute()` + // would record a refusal as a completion for exactly the flows most + // likely to carry one — the same route-dependent shape #9414 and + // #9510 each had to close on this very exit. + // + // ⚠️ The ladder stops here, and `retryExecution` needs no arm of + // its own for it: a refusal is `success: true`, which that loop + // already reads as "this attempt did not fail, stop retrying" — the + // true sentence about a successful evaluation that said no. ⛔ A + // refusal must never consume retry budget: re-running the flow + // would re-execute every node before the `end` in the hope of a + // different answer to a decision the author already made. + if (isRefusalSignal(err)) { + return this.finishRefusedRun({ + runId, flowName, flowVersion: flow.version, + startedAt, durationMs: Date.now() - startTime, + steps, flow, variables, + refusalMessage: err.message, context, + }); + } // [#9510] A node asked to suspend the run (ADR-0019 durable pause) // — here, on a RETRY attempt, the only way this method is ever // reached. Tested FIRST and answered exactly as `execute()`'s own diff --git a/packages/services/service-automation/src/suspended-run-store.test.ts b/packages/services/service-automation/src/suspended-run-store.test.ts index 8a09dc4a0f..a92b39a7aa 100644 --- a/packages/services/service-automation/src/suspended-run-store.test.ts +++ b/packages/services/service-automation/src/suspended-run-store.test.ts @@ -1340,3 +1340,70 @@ describe('ObjectStoreSuspendedRunStore — the persisted terminal status distinc expect(refused.refusal).toBe('RUN_CANCELLED'); }); }); + +// ─── The refusal column (#15788) ───────────────────────────────────────────── +// +// Same discipline as the trigger-attribution group above: assert the persisted +// CELL as well as what `loadTerminal` hands back, because a mapper that never +// wrote the column but happened to reconstruct the value would pass the second +// and fail the first — and the whole point of the column is that an operator +// can filter on it. +// +// ⛔ `refusal_message` is its own column and NOT a second use of `error`. A +// refusal is a successful evaluation that said no (#14945 ruling 2′); text +// found in `error` tells every reader — an operator, the Runs surface, a sweep +// filtering `error IS NOT NULL` — that the run broke. +describe('ObjectStoreSuspendedRunStore — the refused terminal and its message (#15788)', () => { + const REFUSAL = 'Refused: Acme Corp is a confirmed duplicate'; + + it('writes `refused` and the rendered message as COLUMNS, leaving `error` null', async () => { + const engine = createFakeEngine(); + const store = new ObjectStoreSuspendedRunStore(engine, createTestLogger()); + + await store.recordTerminal(terminalRecord(1, { status: 'refused', refusalMessage: REFUSAL })); + + const row = engine.rows.get('run_r1'); + expect(row.status).toBe('refused'); + expect(row.refusal_message).toBe(REFUSAL); + expect(row.error).toBeNull(); + }); + + it('round-trips both back through loadTerminal', async () => { + const engine = createFakeEngine(); + const store = new ObjectStoreSuspendedRunStore(engine, createTestLogger()); + await store.recordTerminal(terminalRecord(2, { status: 'refused', refusalMessage: REFUSAL })); + + const back = (await store.loadTerminal('r2'))!; + expect(back.status).toBe('refused'); + expect(back.refusalMessage).toBe(REFUSAL); + expect(back.error).toBeUndefined(); + }); + + it('a `refused` row is history, not a live suspension — it reaches listHistory', async () => { + // The row gate is `isTerminalRunStatus`. Had the engine started writing + // `refused` without widening that vocabulary, the row would be written + // and then filtered away on every read — the "widened writer, narrower + // reader" failure #15223 names. + const engine = createFakeEngine(); + const store = new ObjectStoreSuspendedRunStore(engine, createTestLogger()); + await store.recordTerminal(terminalRecord(3, { status: 'refused', refusalMessage: REFUSAL })); + + const history = await store.listHistory('busy_flow', 10); + expect(history.map((r) => r.runId)).toContain('r3'); + expect(history.find((r) => r.runId === 'r3')!.refusalMessage).toBe(REFUSAL); + }); + + it('a NON-refused terminal writes NULL — the upsert clears a refusal it no longer carries', async () => { + // `recordTerminal` is an upsert on `run_`. A row rewritten by a + // later terminal write must not inherit the earlier one's refusal. + const engine = createFakeEngine(); + const store = new ObjectStoreSuspendedRunStore(engine, createTestLogger()); + + await store.recordTerminal(terminalRecord(4, { status: 'refused', refusalMessage: REFUSAL })); + expect(engine.rows.get('run_r4').refusal_message).toBe(REFUSAL); + + await store.recordTerminal(terminalRecord(4, { status: 'completed' })); + expect(engine.rows.get('run_r4').refusal_message).toBeNull(); + expect((await store.loadTerminal('r4'))!.refusalMessage).toBeUndefined(); + }); +}); diff --git a/packages/services/service-automation/src/suspended-run-store.ts b/packages/services/service-automation/src/suspended-run-store.ts index 15c7f38105..633d22a22e 100644 --- a/packages/services/service-automation/src/suspended-run-store.ts +++ b/packages/services/service-automation/src/suspended-run-store.ts @@ -694,6 +694,13 @@ export class ObjectStoreSuspendedRunStore implements SuspendedRunStore { finished_at: record.finishedAt ?? now, duration_ms: record.durationMs ?? null, error: record.error ?? null, + // [#15788] The rendered refusal, in its OWN column beside `status: + // 'refused'` — ⛔ never folded into `error`, which would tell every + // reader the run broke. `?? null` for the same reason the four + // consumed-suspension keys are always written: this is an UPSERT, so a + // row rewritten by a later terminal write must CLEAR a refusal it no + // longer carries rather than inherit a stale one. + refusal_message: record.refusalMessage ?? null, steps_json: serializeStepsBounded(record.steps), // #4354 — the totals land in COLUMNS so an operator can alert on // `selected_count > 0 AND acted_count = 0`; the per-node / per-gate detail @@ -833,6 +840,11 @@ export class ObjectStoreSuspendedRunStore implements SuspendedRunStore { finishedAt: row.finished_at ?? undefined, durationMs: typeof row.duration_ms === 'number' ? row.duration_ms : undefined, error: row.error ?? undefined, + // [#15788] `?? undefined` (never `?? ''`): a row written before this + // column existed, and every non-refused row, genuinely carries no + // refusal — and an empty string would read as "refused, with nothing to + // say", which is a different and false statement. + refusalMessage: row.refusal_message ?? undefined, nodeId: row.node_id ?? undefined, organizationId: row.organization_id ?? null, userId: row.user_id ?? undefined, diff --git a/packages/services/service-automation/src/sys-automation-run.object.ts b/packages/services/service-automation/src/sys-automation-run.object.ts index 88881b712a..04eb7f433a 100644 --- a/packages/services/service-automation/src/sys-automation-run.object.ts +++ b/packages/services/service-automation/src/sys-automation-run.object.ts @@ -50,21 +50,23 @@ export const SysAutomationRun = ObjectSchema.create({ // cap (ObjectStoreSuspendedRunStore.pruneFlowOverflow, #2585) stays in the // store — a count bound the declarative contract can't express. // - // [#15223] ALL FOUR terminal members, not the two this scope used to name. + // [#15223] EVERY terminal member, not the two this scope used to name. // The list is a $in over stored values, so it is the third copy of the // vocabulary `TERMINAL_RUN_STATUSES` declares (engine.ts) — and the one with - // the quietest failure: a widened writer plus a two-member sweep scope means - // `cancelled` and `timed_out` history rows are simply never aged out, on a - // table whose whole retention posture (ADR-0057) is that history is - // telemetry. ⛔ Widen this in the same change as the writer, always. + // the quietest failure: a widened writer plus a narrower sweep scope means + // those history rows are simply never aged out, on a table whose whole + // retention posture (ADR-0057) is that history is telemetry. + // ⛔ Widen this in the same change as the writer, always — which is what + // [#15788] does for `refused`: the engine began producing that terminal in + // the same commit that added it here and to the option set below. lifecycle: { class: 'telemetry', retention: { maxAge: '30d', - onlyWhen: { status: { $in: ['completed', 'failed', 'cancelled', 'timed_out'] } }, + onlyWhen: { status: { $in: ['completed', 'failed', 'cancelled', 'timed_out', 'refused'] } }, }, }, - description: 'Durable automation run state: live suspended runs (resumable, ADR-0019) and terminal run history (completed / failed / cancelled / timed_out, for observability).', + description: 'Durable automation run state: live suspended runs (resumable, ADR-0019) and terminal run history (completed / failed / cancelled / timed_out / refused, for observability).', displayNameField: 'id', nameField: 'id', // [ADR-0079] canonical primary-title pointer (mirrors deprecated displayNameField) titleFormat: '{flow_name} · {node_id}', @@ -150,23 +152,30 @@ export const SysAutomationRun = ObjectSchema.create({ group: 'State', }), - // [#15223] The four terminal members are the ones the engine's own - // terminal predicate admits (`TERMINAL_RUN_STATUSES`, engine.ts). This - // option set used to stop at `failed`, and both ends of the store folded to - // match it: a cancelled or timed-out run was written as `failed`, so the - // distinction was destroyed at write time rather than merely unshown, and a - // restart or ring eviction turned an operator's deliberate `cancelRun` - // (ADR-0044) into an indistinguishable failure. `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). + // [#15223] The terminal members are the ones the engine's own terminal + // predicate admits (`TERMINAL_RUN_STATUSES`, engine.ts). This option set + // used to stop at `failed`, and both ends of the store folded to match it: + // a cancelled or timed-out run was written as `failed`, so the distinction + // was destroyed at write time rather than merely unshown, and a restart or + // ring eviction turned an operator's deliberate `cancelRun` (ADR-0044) into + // an indistinguishable failure. + // + // [#15788] `refused` joins them, and the note it replaces says exactly why + // it could not before: `ExecutionStatus` declared it (#14945) while no + // engine path produced it, and an option nothing can write is a + // declared-but-inert value (ADR-0078). Lane 2 makes the engine write it — + // an `end` node declaring `outcome: 'refused'` — so the option arrives in + // the same change as its producer, which is the ADR-0078 condition, not an + // exception to it. ⛔ `refused` is not a failure: the run evaluated + // successfully and said no, and its authored reason is in + // `refusal_message`, never in `error`. status: Field.select( - ['running', 'paused', 'completed', 'failed', 'cancelled', 'timed_out'], + ['running', 'paused', 'completed', 'failed', 'cancelled', 'timed_out', 'refused'], { label: 'Status', required: true, defaultValue: 'paused', - description: 'paused = a live suspended run (resumable); completed / failed / cancelled / timed_out = a terminal run kept as durable history.', + description: 'paused = a live suspended run (resumable); completed / failed / cancelled / timed_out / refused = a terminal run kept as durable history. refused = the flow reached an `end` node declaring `outcome: \'refused\'` — a successful evaluation that said no, distinct from failed, with the rendered reason in refusal_message.', group: 'State', }, ), @@ -341,6 +350,25 @@ export const SysAutomationRun = ObjectSchema.create({ group: 'Outcome', }), + // [#15788] The refusal's own column, beside `error` and deliberately NOT + // inside it. Reusing `error` would have needed no migration and cost the + // exact distinction the #14945 ruling is about: *a refusal is a successful + // evaluation that says no*, so a reader — an operator, the Runs surface, a + // sweep filtering `error IS NOT NULL` — must be able to tell a refusal from + // a breakage by looking at the row. The same reason #15223 stopped folding + // `cancelled` into `failed`: a distinction destroyed at write time cannot + // be recovered afterwards. + // + // Written only by the refusal path (`AutomationEngine.finishRefusedRun`) + // and always as an explicit value — NULL included — because the terminal + // write is an upsert. + refusal_message: Field.textarea({ + label: 'Refusal Message', + required: false, + description: 'Rendered refusal for a `refused` run — the `end` node\'s `message` template interpolated against the run\'s variables at the moment it was reached, so the stored text names the record (the same rendering a screen `description` gets). Null on every other status: a completion has nothing to say and a failure\'s reason is in `error`.', + group: 'Outcome', + }), + // ── Run summary (#4354) ──────────────────────────────────────────────── // COLUMNS, not just a blob: `selected_count > 0 AND acted_count = 0` is the // first FILTER of the broken-sweep detector, and an operator can only alert From 958a68c1e310354ec3fcb6848e24bde1d9383129 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 02:37:57 +0000 Subject: [PATCH 3/3] chore(changeset): minor for the refusing `end` node Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj Co-authored-by: Claude --- .changeset/refused-end-node-outcome.md | 46 +++++++++++++++++++ .../src/end-node-refused-outcome.test.ts | 1 - 2 files changed, 46 insertions(+), 1 deletion(-) create mode 100644 .changeset/refused-end-node-outcome.md diff --git a/.changeset/refused-end-node-outcome.md b/.changeset/refused-end-node-outcome.md new file mode 100644 index 0000000000..ec300d7750 --- /dev/null +++ b/.changeset/refused-end-node-outcome.md @@ -0,0 +1,46 @@ +--- +'@objectstack/service-automation': minor +--- + +Flow `end` nodes honour `outcome: 'refused'` — a terminal `refused` run, distinct from `failed` + +`packages/spec` has declared the shape since 17.4.0: an `end` node accepts +`outcome: 'completed' | 'refused'`, a `refused` end requires a `message`, +`ExecutionStatus` carries `refused`, and `ExecutionLog` / `AutomationResult` / +the trigger response carry `refusalMessage`. The engine produced none of it — +it returned on every `end` node without reading its config — so an author who +wrote a refusal shipped a plain completion: the run recorded `completed`, the +caller got the flow's `successMessage`, and the authored reason reached nobody. + +The `end` node now honours it: + +- **The run terminates `refused`.** A refusal is a *successful evaluation that + says no*, so the result is `success: true, status: 'refused'` with no `error` + and no `errorMessage` — and, deliberately, no `successMessage`: the flow's + completion toast is for a completion. All three terminal producers answer + identically (a triggered run, a resumed screen flow, and an attempt under + `errorHandling.strategy: 'retry'`, where a refusal also stops the ladder + rather than consuming retry budget). +- **The `message` is rendered per record**, through the same interpolation a + `screen` node's `description` gets — one implementation (`interpolateText`), + never a second template engine — so `'Refused: {record.name} is a confirmed + duplicate'` reaches the caller naming the record. +- **Both are persisted on the run.** `sys_automation_run.status` gains + `refused` and a new `refusal_message` column carries the rendered text; the + refusal is never folded into `error`, which would tell every reader the run + broke. `RunRecord` gains `refusalMessage` and `TerminalRunStatus` gains + `refused`, so history rows are written, aged and read back like any other + terminal. +- **A refused run is never resumed.** It writes no continuation, so `resume` + answers `RUN_NOT_FOUND`. + +Untouched on purpose: a paused run still returns `silent` with no +`successMessage`, and a plain `end` — or one declaring `outcome: 'completed'` — +completes exactly as before. + +An `end` declaring `outcome: 'refused'` **inside a structured region** (a `loop` +body, a `try`/`catch` region) is refused loudly rather than honoured: a refusal +terminates the run and a region body cannot end one. Previously such a node was +a silent no-op like every other `end` in a region, so nothing that ever worked +stops working — put the refusing `end` on the top-level graph and route the +region's exit to it. diff --git a/packages/services/service-automation/src/end-node-refused-outcome.test.ts b/packages/services/service-automation/src/end-node-refused-outcome.test.ts index 75dfbd35b6..6928d4bf66 100644 --- a/packages/services/service-automation/src/end-node-refused-outcome.test.ts +++ b/packages/services/service-automation/src/end-node-refused-outcome.test.ts @@ -36,7 +36,6 @@ import { AutomationEngine } from './engine.js'; import { InMemorySuspendedRunStore } from './suspended-run-store.js'; import { installBuiltinNodes } from './builtin/index.js'; import type { AutomationContext } from '@objectstack/spec/contracts'; -import { defineActionDescriptor } from '@objectstack/spec/automation'; function createTestLogger(): any { return { info: () => {}, warn: () => {}, error: () => {}, debug: () => {}, child: () => createTestLogger() };