From 8ed175039405c4f9409e676723c1ae6c4044f371 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Wed, 2 Sep 2026 03:28:24 +0000 Subject: [PATCH] fix(automation): hold a signal-less resume to the screen-input contract `AutomationEngine.refuseInvalidScreenInput` opened with a bare `if (!signal) return null;`, so `resume(runId)` with no signal object skipped the `required` screen-field check that `resume(runId, {})` is held to. The engine already names its one legitimate exemption through `ENGINE_BUILT_SIGNAL`; the early return was a second, unnamed spelling. The public `resume` door now normalises an absent signal to `{}` (the shape the HTTP route has always assembled for an empty body), and `resumeInternal` / `refuseInvalidScreenInput` / `applyResumeSignal` take a non-optional signal, so a falsy-signal branch cannot grow back. Pauses that declare no input contract are unaffected. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --- .../signal-less-resume-screen-contract.md | 5 + .../builtin/screen-resume-signal-less.test.ts | 258 ++++++++++++++++++ .../services/service-automation/src/engine.ts | 42 ++- 3 files changed, 296 insertions(+), 9 deletions(-) create mode 100644 .changeset/signal-less-resume-screen-contract.md create mode 100644 packages/services/service-automation/src/builtin/screen-resume-signal-less.test.ts diff --git a/.changeset/signal-less-resume-screen-contract.md b/.changeset/signal-less-resume-screen-contract.md new file mode 100644 index 0000000000..f91c177e7b --- /dev/null +++ b/.changeset/signal-less-resume-screen-contract.md @@ -0,0 +1,5 @@ +--- +'@objectstack/service-automation': patch +--- + +`AutomationEngine.resume(runId)` called with no signal object is now held to the suspended `screen` node's declared field contract exactly like a signal-carrying resume: a run paused on a screen with an unconditional `required` field is refused with `INVALID_SCREEN_INPUT` and stays paused, instead of proceeding with that variable unbound. The bare `if (!signal) return null` early return in `refuseInvalidScreenInput` is gone — an absent signal is an empty submission, the same shape the HTTP resume route has always assembled for an empty body — and the engine's own continuations (subflow output mapping, `map` item handoff) remain exempt only through the existing engine-built-signal mechanism. Pauses that declare no input contract are unaffected: `wait` and `approval` nodes, message-only and object-form screens, and screens whose fields are all optional or hidden resume without a signal exactly as before. diff --git a/packages/services/service-automation/src/builtin/screen-resume-signal-less.test.ts b/packages/services/service-automation/src/builtin/screen-resume-signal-less.test.ts new file mode 100644 index 0000000000..7a83f17207 --- /dev/null +++ b/packages/services/service-automation/src/builtin/screen-resume-signal-less.test.ts @@ -0,0 +1,258 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * A signal-less `resume(runId)` is held to the suspended screen's declared + * field contract exactly like a signal-carrying one (#13648). + * + * `refuseInvalidScreenInput` (#4477) used to open with `if (!signal) return + * null;` — so `resume(runId, { variables: {} })` was refused with + * `INVALID_SCREEN_INPUT` while `resume(runId)` completed the run with every + * unconditional `required` field unbound. The engine already had a NAMED + * exemption for the one legitimate case — its own continuations, tagged + * `ENGINE_BUILT_SIGNAL` — and the bare early return was a second, unnamed + * spelling of an exemption nobody had asked for. Ruling (triage, 2026-08-31): + * the governed side wins — the early return is gone, an absent signal is an + * empty submission, and the engine-built flag is the only exemption left. + * + * The HTTP door (`POST …/runs/:runId/resume`) never reached the hole — it + * assembles `{}` for an empty body — so these pins sit on the in-process door + * `AutomationEngine.resume`, which is also what the wait node's timer wake + * calls with no signal (and must keep doing: a `wait` pause declares no + * screen contract, so an empty submission against it is conformant; + * `wait-node.test.ts` owns that half). + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { AutomationEngine } from '../engine.js'; +import type { NodeExecutor } from '../engine.js'; +import { installBuiltinNodes } from './index.js'; + +function silentLogger() { + return { info() {}, warn() {}, error() {}, debug() {}, child() { return silentLogger(); } } as any; +} +function ctx() { + return { logger: silentLogger(), getService() { return undefined; } } as any; +} + +/** A one-screen flow whose screen declares exactly `fields`. */ +function screenFlow(name: string, fields: Array>, screenConfig: Record = {}) { + return { + name, + label: name, + type: 'screen', + status: 'active', + version: 1, + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'ask', type: 'screen', label: 'Ask', config: { ...screenConfig, ...(fields.length ? { fields } : {}) } }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'ask', type: 'default' }, + { id: 'e2', source: 'ask', target: 'end', type: 'default' }, + ], + }; +} + +const REQUIRED_KIND = [{ name: 'kind', label: 'Kind', type: 'text', required: true }]; + +describe('signal-less resume of a screen with a required field (#13648)', () => { + let engine: AutomationEngine; + + beforeEach(() => { + engine = new AutomationEngine(silentLogger()); + installBuiltinNodes(engine, ctx()); + engine.registerFlow('triage', screenFlow('triage', REQUIRED_KIND) as any); + }); + + async function pause(): Promise { + const started = await engine.execute('triage', {} as any); + expect(started.status).toBe('paused'); + expect(started.screen?.nodeId).toBe('ask'); + return started.runId!; + } + + it('refuses `resume(runId)` with INVALID_SCREEN_INPUT and leaves the run paused', async () => { + const runId = await pause(); + + const res = await engine.resume(runId); + + // The ADR-0112 envelope, not a bare "it failed": the same code and the + // same first sentence the signal-carrying refusal answers. + expect(res.success).toBe(false); + expect(res.code).toBe('INVALID_SCREEN_INPUT'); + expect(res.error).toMatch(/^Invalid screen input: /); + expect(res.error).toContain('"kind"'); + expect(res.error).toMatch(/required/i); + // The pause was NOT consumed — the run is exactly where it was. + expect(await engine.hasSuspendedRun(runId)).toBe(true); + expect((await engine.getSuspendedScreen(runId))?.nodeId).toBe('ask'); + }); + + it('answers the signal-less and the empty-bag resume with the SAME envelope', async () => { + const runId = await pause(); + const bare = await engine.resume(runId); + const empty = await engine.resume(runId, { variables: {} }); + expect(bare).toEqual(empty); + }); + + it('resumes the same run once the field is supplied', async () => { + const runId = await pause(); + expect((await engine.resume(runId)).code).toBe('INVALID_SCREEN_INPUT'); + + const good = await engine.resume(runId, { variables: { kind: 'normal' } }); + + expect(good.success).toBe(true); + expect(good.code).toBeUndefined(); + expect(good.status).toBeUndefined(); // ran to completion + expect(await engine.hasSuspendedRun(runId)).toBe(false); + }); +}); + +describe('signal-less resume of a pause that declares no contract proceeds (#13648)', () => { + let engine: AutomationEngine; + + beforeEach(() => { + engine = new AutomationEngine(silentLogger()); + installBuiltinNodes(engine, ctx()); + }); + + async function pauseOn(flow: Record): Promise { + engine.registerFlow(flow.name as string, flow as any); + const started = await engine.execute(flow.name as string, {} as any); + expect(started.status).toBe('paused'); + return started.runId!; + } + + it('a screen whose fields are all optional', async () => { + const runId = await pauseOn(screenFlow('optional_only', [ + { name: 'note', label: 'Note', type: 'text' }, + { name: 'flag', label: 'Flag', type: 'boolean', required: false }, + ])); + const res = await engine.resume(runId); + expect(res.success).toBe(true); + expect(res.code).toBeUndefined(); + expect(await engine.hasSuspendedRun(runId)).toBe(false); + }); + + it('a MESSAGE-ONLY screen — no keys declared, none constrained', async () => { + const runId = await pauseOn(screenFlow('message_only', [], { title: 'Confirm', waitForInput: true })); + const res = await engine.resume(runId); + expect(res.success).toBe(true); + expect(res.code).toBeUndefined(); + }); + + it('an OBJECT-FORM screen — the record write path enforces its own required fields', async () => { + const runId = await pauseOn(screenFlow('object_form', [], { + objectName: 'crm_account', mode: 'create', idVariable: 'account_id', + })); + expect((await engine.getSuspendedScreen(runId))?.kind).toBe('object-form'); + const res = await engine.resume(runId); + expect(res.success).toBe(true); + expect(res.code).toBeUndefined(); + }); + + it('a required field the screen HIDES (`visibleWhen` false) — the visibility layer applies to an empty bag too', async () => { + const runId = await pauseOn(screenFlow('hidden_required', [ + { name: 'reason', label: 'Reason', type: 'text', required: true, visibleWhen: 'false' }, + ])); + const res = await engine.resume(runId); + expect(res.success).toBe(true); + expect(res.code).toBeUndefined(); + }); +}); + +describe("engine-built continuation stays exempt — the flag is the ONLY exemption (#13648 negative control)", () => { + let engine: AutomationEngine; + let captured: unknown[]; + + beforeEach(() => { + engine = new AutomationEngine(silentLogger()); + installBuiltinNodes(engine, ctx()); + captured = []; + // Copies the screen-collected `kind` into the child's declared output. + engine.registerNodeExecutor({ + type: 'copier', + async execute(_node, variables) { + variables.set('result', variables.get('kind')); + return { success: true }; + }, + } as NodeExecutor); + // Parent step after the subflow: captures the mapped output variable. + engine.registerNodeExecutor({ + type: 'parentcheck', + async execute(_node, variables) { + captured.push(variables.get('subResult')); + return { success: true }; + }, + } as NodeExecutor); + engine.registerFlow('child', { + name: 'child', + label: 'Child', + type: 'autolaunched', + variables: [{ name: 'result', type: 'text', isOutput: true }], + nodes: [ + { id: 's', type: 'start', label: 'Start' }, + { id: 'ask', type: 'screen', label: 'Ask', config: { fields: REQUIRED_KIND } }, + { id: 'copy', type: 'copier', label: 'Copy' }, + { id: 'e', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'c1', source: 's', target: 'ask' }, + { id: 'c2', source: 'ask', target: 'copy' }, + { id: 'c3', source: 'copy', target: 'e' }, + ], + } as any); + engine.registerFlow('parent', { + name: 'parent', + label: 'Parent', + type: 'autolaunched', + nodes: [ + { id: 'ps', type: 'start', label: 'Start' }, + { id: 'call', type: 'subflow', label: 'Call Child', config: { flowName: 'child', outputVariable: 'subResult' } }, + { id: 'chk', type: 'parentcheck', label: 'Check' }, + { id: 'pe', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'p1', source: 'ps', target: 'call' }, + { id: 'p2', source: 'call', target: 'chk' }, + { id: 'p3', source: 'chk', target: 'pe' }, + ], + } as any); + }); + + it("a child's completion bubbles up through an engine-built signal whose bag lacks the parent's surfaced required field", async () => { + const started = await engine.execute('parent', {} as any); + expect(started.status).toBe('paused'); + const parentRunId = started.runId!; + // The parent surfaces the CHILD's screen — required `kind` included — + // so the up-bubble below is judged against a screen with a required + // field, and only the engine-built flag lets it through. + expect((await engine.getSuspendedScreen(parentRunId))?.fields?.map((f) => f.name)).toEqual(['kind']); + const child = engine.listSuspendedRuns().find((r) => r.flowName === 'child')!; + expect(child).toBeDefined(); + + // Resume the CHILD directly (the approval/wait-style path) with the + // field it asked for; its completion resumes the parent with the + // engine's own output-mapping signal, which never carries `kind`. + const childRes = await engine.resume(child.runId, { variables: { kind: 'escalate' } }); + + expect(childRes.success).toBe(true); + expect(childRes.status).toBeUndefined(); + expect(captured).toEqual([{ result: 'escalate' }]); + expect(engine.listSuspendedRuns()).toHaveLength(0); + }); + + it("a signal-less resume of the CHILD is still refused — the flag exempts the engine's signal, not the run", async () => { + const started = await engine.execute('parent', {} as any); + const child = engine.listSuspendedRuns().find((r) => r.flowName === 'child')!; + + const res = await engine.resume(child.runId); + + expect(res.success).toBe(false); + expect(res.code).toBe('INVALID_SCREEN_INPUT'); + expect(await engine.hasSuspendedRun(child.runId)).toBe(true); + expect(await engine.hasSuspendedRun(started.runId!)).toBe(true); + expect(captured).toEqual([]); + }); +}); diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index bb6ca93701..fd009382c2 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -1004,14 +1004,15 @@ function isEngineVariable(name: string): boolean { * @returns the rejected key names (already in their final, prefixed form). * Empty ⇒ every write was applied. An engine-built signal * ({@link ENGINE_BUILT_SIGNAL}) is exempt: `bubbleToParent` legitimately - * writes the handoff keys, and it is not reachable from a transport. + * writes the handoff keys, and it is not reachable from a transport. The + * signal is never absent here — `resume` normalises a missing one to `{}` + * (#13648), which folds nothing and rejects nothing. */ function applyResumeSignal( variables: Map, - signal: ResumeSignal | undefined, + signal: ResumeSignal, nodeId: string, ): string[] { - if (!signal) return []; const trusted = (signal as Record)[ENGINE_BUILT_SIGNAL] === true; const rejected: string[] = []; const writes: Array<[string, unknown]> = []; @@ -4207,7 +4208,21 @@ export class AutomationEngine implements IAutomationService { async resume(runId: string, signal?: ResumeSignal): Promise { const refusal = await this.refuseGatedResume(runId, signal); if (refusal) return refusal; - return this.resumeInternal(runId, signal, false); + // An ABSENT signal is an EMPTY caller submission, never an exemption + // (#13648). This is the in-process door, and `resume(runId)` used to + // skip the screen contract that `resume(runId, {})` is held to: + // `refuseInvalidScreenInput` short-circuited on a falsy signal — a + // second, unnamed spelling of the exemption the engine already states + // through `ENGINE_BUILT_SIGNAL`, so a run parked on a screen with an + // unconditional `required` field proceeded with that variable unbound. + // The HTTP door has always assembled `{}` for an empty body; this makes + // the two doors agree, and the only exemption left is the engine's own + // continuation, which proves itself by BUILDING an engine-built signal. + // A pause with no screen contract — `wait`, `approval`, a message-only + // or object-form screen — is untouched: an empty submission against no + // declared fields is conformant, so the wait node's timer wake + // (`engine.resume(runId)`) continues exactly as before. + return this.resumeInternal(runId, signal ?? {}, false); } /** @@ -4668,7 +4683,12 @@ export class AutomationEngine implements IAutomationService { */ private async resumeInternal( runId: string, - signal: ResumeSignal | undefined, + // Never `undefined` past the public door: `resume` normalises an + // absent caller signal to `{}` (#13648), and the engine's own + // continuations (subflow delegation / up-bubble, `map` re-entry) + // always hand over a built signal. Typed so, the chokepoints below + // cannot grow a falsy-signal branch again. + signal: ResumeSignal, skipBubble: boolean, childSummary?: FlowRunSummary, ): Promise { @@ -4893,7 +4913,7 @@ export class AutomationEngine implements IAutomationService { if (typeof run.correlation === 'string' && run.correlation.startsWith('map:')) { await this.executeNode(node, flow, variables, context, steps); } else { - await this.traverseNext(node, flow, variables, context, steps, signal?.branchLabel); + await this.traverseNext(node, flow, variables, context, steps, signal.branchLabel); } // Collect output variables @@ -5047,7 +5067,12 @@ export class AutomationEngine implements IAutomationService { * pass-through `enforceActionParams` gives a param-less action). * - **Never an engine-built signal.** The subflow output mapping and the * `map` item handoff are the engine's own continuations; they carry - * author-named output variables, not a screen submission. + * author-named output variables, not a screen submission. This is the + * ONLY exemption, and it is spelled once: an absent signal is not a + * case here — `resume` normalises it to `{}` (#13648) — because a bare + * `if (!signal)` beside the flag was a second, unnamed spelling of the + * same exemption that let `resume(runId)` skip every `required` the + * author wrote. * * `visibleWhen` is evaluated against the SUBMITTED values first (layered * over the run's variables, so a predicate may reference a prior node), @@ -5059,9 +5084,8 @@ export class AutomationEngine implements IAutomationService { private refuseInvalidScreenInput( run: SuspendedRun, runId: string, - signal: ResumeSignal | undefined, + signal: ResumeSignal, ): AutomationResult | null { - if (!signal) return null; if ((signal as Record)[ENGINE_BUILT_SIGNAL] === true) return null; if (!screenDeclaresInputContract(run.screen)) return null; const fields = run.screen!.fields;