diff --git a/.changeset/28840-subflow-notice-arming.md b/.changeset/28840-subflow-notice-arming.md new file mode 100644 index 00000000000..7b0ce9a13ad --- /dev/null +++ b/.changeset/28840-subflow-notice-arming.md @@ -0,0 +1,19 @@ +--- +'@objectstack/service-automation': patch +'@objectstack/spec': patch +--- + +feat(service-automation)!: a flow whose `save_notice` runs after a `subflow` / `map` whose child can suspend is registered but not armed (#28840) + +Clause-②: yes (narrowing) +Metadata-protocol: none + +**BREAKING** for a record-triggered flow that places a `save_notice` node after a `subflow` or `map` node whose child flow can suspend the run. + +- **Before.** The flow parse refuses a `save_notice` that an `approval`, `wait`, `screen` or `approval_revise` node reaches (#28806), but it cannot see into a `subflow` / `map` child. A flow that asked the approval in a subflow and told the saver after it registered and armed. Its child suspended it, the write answered, and on the resume the `save_notice` was refused by name, so every decision stranded the run (`RESUME_FAILED`). +- **Now.** The arming gate in `activateFlowTrigger` declines such a flow, beside its disabled-subflow half (ADR-0126 §7.3). "Can suspend" means the child holds a node of an always-suspending type reachable from its start, or calls a child that can suspend, read through every call with cycle protection. The flow still registers, is not armed, and warns once, naming the `save_notice`, the `subflow` / `map` node and the child's suspending node. `GET /automation/_status` reports it `bound: false` with that reason and the new `unboundCause` value `notice_after_suspending_subflow` (added to `FLOW_UNBOUND_CAUSES` in `@objectstack/spec/contracts`). Whenever a flow is registered, every flow whose calls reach it is judged again, so the decline does not depend on load order, and a child republished without its suspension arms the caller again. +- **Not declined.** A `subflow` / `map` whose child cannot suspend, and a `save_notice` reachable only before the call. Registration, the save door and `objectstack validate` accept the same flows as before. +- **The fix.** Move the `save_notice` ahead of the `subflow` / `map` node the reason names, or tell the person after it with a `notify` node, or republish the child without the node that suspends. The flow arms on its own once the shape is gone. +- **Who moves.** No shipped flow has the shape: the showcase's one `save_notice` (`showcase_task_completed`) has no `subflow` or `map` node, the dogfood fixtures carry no `save_notice`, and HotCRM's `main` carries none. + + diff --git a/packages/plugins/plugin-approvals/src/save-notice-approval-round-trip.integration.test.ts b/packages/plugins/plugin-approvals/src/save-notice-approval-round-trip.integration.test.ts index 93cc8bcd184..b905d419b7d 100644 --- a/packages/plugins/plugin-approvals/src/save-notice-approval-round-trip.integration.test.ts +++ b/packages/plugins/plugin-approvals/src/save-notice-approval-round-trip.integration.test.ts @@ -20,7 +20,20 @@ * `subflow` that asks the approval: the parse cannot see that a subflow * pauses (it does only when its child does), so that is where the run-time * refusal is still the guard. The parse refusal itself is pinned at - * `registerFlow` in the second case. + * `registerFlow` in the last case. + * + * [#28840] That subflow shape is now declined by the arming gate: a + * `save_notice` after a `subflow` whose child can suspend registers but is + * NOT armed, so the create no longer starts it. The first case pins that + * decline through the real record-change trigger: the flow registers, the + * create fires nothing, and `/_status` names the `subflow` node and the + * child's `approval` node. The resumed leg stays reachable through a run + * started directly (`automation.execute`) with the context the record-change + * trigger hands a run, the write's notice channel included, so the second + * case still pins that the stored run drops the channel and its resumed + * `save_notice` refuses by name. What the direct start skips is the trigger's + * hand-off of the channel, which the record-change trigger's own suite pins + * (#28251). * * The executor's own suite pins the run-time refusal with a hand-built * context. This suite pins it through the real round trip, so a store that @@ -43,7 +56,8 @@ * * Ablated: with the executor's no-channel refusal replaced by a success, the * resumed-leg assertions go red (`save-notice-node.ts`), and are green again - * once it is restored. + * once it is restored. With the arming gate's save-notice half removed, the + * first case goes red (the flow is armed and the create starts it). */ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; @@ -89,6 +103,9 @@ const deal = { * the graph is accepted, the parent suspends with its child, persists, and * resumes from storage when the decision completes the child. The resumed * parent leg is where the second `save_notice` meets the run-time refusal. + * + * [#28840] The arming gate declines this graph, so the create never starts + * it; the run-time case starts it directly. */ const CHILD = 'deal_manager_review'; const child = { @@ -205,15 +222,42 @@ describe('[#28786] a save_notice before an approval answers the write, and one o try { await kernel?.shutdown(); } catch { /* noop */ } }); - it('the create hears the sentence, the run is persisted without the channel, and the resumed save_notice refuses', async () => { - // ── The write: the save_notice ahead of the approval answers it. + it('[#28840] the flow registers but is not armed: the create starts nothing, and /_status names the subflow node and the child\'s approval', async () => { + const automation = kernel.getService('automation'); + expect(automation.getFlow(FLOW), 'the flow is registered').toBeDefined(); + const row = automation.getFlowRuntimeStates().find((s) => s.name === FLOW); + expect(row).toMatchObject({ enabled: true, bound: false, unboundCause: 'notice_after_suspending_subflow' }); + expect(row?.reason).toContain("`save_notice` node 'tell_after' is reached after `subflow` node 'review'"); + expect(row?.reason).toContain(`child flow '${CHILD}' can suspend the run at \`approval\` node 'ask'`); + const heard: unknown[] = []; - const created: any = await ql.insert( + await ql.insert( deal.name, - { name: 'Acme' }, + { name: 'Globex' }, { context: { userId: SUBMITTER }, onNotice: (message: unknown) => { heard.push(message); } } as any, ); - expect(heard, "the create's own answer carried the notice").toEqual(['Sent Acme to your manager for approval']); + expect(heard, 'no run answered the create').toEqual([]); + expect(await runRows(), 'the create started no run').toEqual([]); + }); + + it('a run started with the write\'s channel is persisted without it, and its resumed save_notice refuses by name', async () => { + const automation = kernel.getService('automation'); + const created: any = await ql.insert(deal.name, { name: 'Acme' }, { context: SYSTEM }); + + // ── The run, started as the record-change trigger starts one: the record, + // the writer, and the write's notice channel, whose `set` answers the write. + const heard: unknown[] = []; + const started: any = await automation.execute(FLOW, { + record: { ...created }, + previous: undefined, + object: deal.name, + event: 'insert', + userId: SUBMITTER, + params: { ...created }, + saveNotice: { set: (message: string) => { heard.push(message); } }, + } as any); + expect(started.success, 'the run suspended at its subflow, it did not fail').toBe(true); + expect(heard, 'the save_notice ahead of the subflow set the sentence').toEqual(['Sent Acme to your manager for approval']); // ── Suspended and persisted: one live row, its context stored as JSON. const [pause] = await runRows(); diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index 7bb7e786279..9ba736695fd 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -221,6 +221,7 @@ import { rejudgeSubflowCallers, refuseCustomerAuthoredToggle, } from './subflow-guards.js'; +import { describeSubflowNoticeTrap, subflowNoticeTrap } from './subflow-notice-guard.js'; // [#25222] The consumed-suspension restore's bodies: journaling a consumed // suspension, resolving its witnesses, and restoring, peeking at or inspecting // one. Each `AutomationEngine` method of the same name calls its body. @@ -2904,6 +2905,16 @@ export class AutomationEngine implements IAutomationService { * {@link unregisterFlow} drops it with the flow. */ private readonly subflowDeclinedFlows = new Map(); + /** + * [#28840] Flows {@link activateFlowTrigger} declined to arm because a + * `save_notice` of theirs runs after a `subflow` / `map` node whose child + * flow can suspend, each with the reason `/_status` and the binding audit + * read. A record for {@link subflowDeclinedFlows}'s reasons: deleted the + * moment the flow gets past the gate, re-asked by + * {@link rejudgeSubflowCallers} whenever a flow its calls reach is + * registered, and dropped with the flow by {@link withdrawFlow}. + */ + private readonly subflowNoticeDeclinedFlows = new Map(); /** Connectors registered by integration plugins, keyed by connector name (ADR-0018 §Addendum). */ private connectors = new Map(); /** Connector provider factories keyed by provider name (ADR-0097 §2 — `openapi`/`mcp`/`rest`/…). */ @@ -3880,6 +3891,14 @@ export class AutomationEngine implements IAutomationService { // trigger would change nothing, so "no trigger registered" would name // a remedy that cannot work. if (this.declineOntoDisabledSubflows(flowName, resolved.triggerType)) return; + // [#28840] The save-notice half, a decline for the same reasons: a + // `save_notice` after a `subflow` / `map` node whose child can suspend + // is refused by name when the suspended run resumes, after the write + // answered, and a decision that resumes it is stranded. The flow parse + // cannot see the child (#28806), which may register after this flow, + // so the gate asks here and {@link rejudgeSubflowCallers} asks again + // when the child arrives or changes. + if (this.declineOntoSuspendingSubflowNotice(flowName, resolved.triggerType)) return; const trigger = this.triggers.get(resolved.triggerType); if (!trigger) return; try { @@ -4629,6 +4648,7 @@ export class AutomationEngine implements IAutomationService { flowVersionHistory: this.flowVersionHistory, policyDisabledFlows: this.policyDisabledFlows, subflowDeclinedFlows: this.subflowDeclinedFlows, + subflowNoticeDeclinedFlows: this.subflowNoticeDeclinedFlows, flowRefusals: this.flowRefusals, logger: this.logger, }; @@ -4759,6 +4779,7 @@ export class AutomationEngine implements IAutomationService { boundFlowTriggers: this.boundFlowTriggers, policyDisabledFlows: this.policyDisabledFlows, subflowDeclinedFlows: this.subflowDeclinedFlows, + subflowNoticeDeclinedFlows: this.subflowNoticeDeclinedFlows, triggers: this.triggers, }; return describeUnboundReason.call(host, name, resolved); @@ -5489,6 +5510,48 @@ export class AutomationEngine implements IAutomationService { return true; } + /** + * [#28840] The arming gate's save-notice half, asked by + * {@link activateFlowTrigger} of an enabled flow it is about to arm: does a + * `save_notice` of it run after a `subflow` / `map` node whose child flow + * can suspend ({@link subflowNoticeTrap}: a pausing node reachable from the + * child's start, or a child of the child that can suspend, transitively)? + * Then it is declined: recorded on {@link subflowNoticeDeclinedFlows} for + * `/_status` and the binding audit, and said once as a `warn` naming the + * notice, the call node and where the child suspends. + * + * Any flow, packaged or authored here: the stranded decision is the same. + * ⛔ A decline, never a refusal, for {@link declineOntoDisabledSubflows}'s + * reason and one of its own: which flows suspend depends on what else is + * registered, so `registerFlow` cannot refuse it without depending on load + * order. + * + * #4632 verdict: FUNCTIONAL, `warn`, as the disabled-subflow decline: the + * flow is visibly smaller than declared and `/_status` says why. Said once + * per decline reason. + * + * @returns `true` when the flow is declined and must not be armed. + */ + private declineOntoSuspendingSubflowNotice(flowName: string, triggerType: string): boolean { + const trap = subflowNoticeTrap(this.flows, flowName); + if (!trap) { + this.subflowNoticeDeclinedFlows.delete(flowName); + return false; + } + const why = describeSubflowNoticeTrap(trap); + const reason = + `not armed — ${why}, and it is armed; it is also armed once '${trap.child}' is republished without a ` + + `node that suspends`; + const previous = this.subflowNoticeDeclinedFlows.get(flowName); + this.subflowNoticeDeclinedFlows.set(flowName, reason); + if (previous === reason) return true; + this.logger.warn( + `Flow '${flowName}' is registered but NOT armed on trigger '${triggerType}' — ${why}. It is armed the ` + + `moment the \`save_notice\` moves, or '${trap.child}' is republished without a node that suspends.`, + ); + return true; + } + /** * [#20725, ADR-0126 §7.3] `subflow` just changed state — (re)registered, * switched on or off, or read off the ledger at boot — so ask the arming @@ -5505,12 +5568,22 @@ export class AutomationEngine implements IAutomationService { * Every other caller is left exactly as it is: an armed caller whose * subflows are all enabled is not stopped and restarted, and a caller * unbound for a reason of its own is not re-offered here. + * + * [#28840] And the save-notice half of the gate, for every flow whose + * calls reach `subflow`, directly or through other flows, packaged or + * not: whether a child can suspend is read through its own calls, so a + * change two calls away moves the answer. A caller that half declined is + * re-offered, and an armed caller it would now decline is unbound and + * offered again, both through the one gate. */ private rejudgeSubflowCallers(subflow: string): void { // The body lives in ./subflow-guards.ts (#25222). const host: ThisParameterType = { boundFlowTriggers: this.boundFlowTriggers, subflowDeclinedFlows: this.subflowDeclinedFlows, + subflowNoticeDeclinedFlows: this.subflowNoticeDeclinedFlows, + flows: this.flows, + subflowTargets: (...a) => this.subflowTargets(...a), packagedSubflowCallers: (...a) => this.packagedSubflowCallers(...a), disabledPackagedSubflows: (...a) => this.disabledPackagedSubflows(...a), deactivateFlowTrigger: (...a) => this.deactivateFlowTrigger(...a), diff --git a/packages/services/service-automation/src/flow-definitions.ts b/packages/services/service-automation/src/flow-definitions.ts index b6dcde998e5..b6bcf48b2c4 100644 --- a/packages/services/service-automation/src/flow-definitions.ts +++ b/packages/services/service-automation/src/flow-definitions.ts @@ -74,6 +74,7 @@ export interface FlowDefinitionsHost { readonly boundFlowTriggers: AutomationEngine['boundFlowTriggers']; readonly policyDisabledFlows: AutomationEngine['policyDisabledFlows']; readonly subflowDeclinedFlows: AutomationEngine['subflowDeclinedFlows']; + readonly subflowNoticeDeclinedFlows: AutomationEngine['subflowNoticeDeclinedFlows']; readonly triggers: AutomationEngine['triggers']; readonly flowCredentialSource: AutomationEngine['flowCredentialSource']; readonly flowVersionHistory: AutomationEngine['flowVersionHistory']; @@ -287,9 +288,9 @@ export function getFlowRuntimeStates(this: Pick, + this: Pick, name: string, resolved: { triggerType: string } | undefined, ): { reason: string; cause: FlowUnboundCause } | undefined { @@ -304,6 +305,12 @@ export function describeUnboundReason( // child off, the trigger would not arm it either. const declined = this.subflowDeclinedFlows.get(name); if (declined !== undefined) return { reason: declined, cause: 'disabled_subflow' }; + // [#28840] The gate's save-notice half declined it: a `save_notice` runs + // after a `subflow` / `map` node whose child can suspend. Its own cause, + // for its own owner and remedy (the author moves the notice, or the child + // stops suspending). + const noticeDeclined = this.subflowNoticeDeclinedFlows.get(name); + if (noticeDeclined !== undefined) return { reason: noticeDeclined, cause: 'notice_after_suspending_subflow' }; return this.triggers.has(resolved.triggerType) ? { reason: `trigger '${resolved.triggerType}' is registered but binding failed — see earlier warnings`, @@ -415,8 +422,8 @@ export function unregisterConnectorProvider(this: Pick, name: string): void { +/** The body of {@link AutomationEngine.withdrawFlow}; that method's docblock is its contract. */ +export function withdrawFlow(this: Pick, name: string): void { this.deactivateFlowTrigger(name); this.flows.delete(name); // [ADR-0126 §7.2] `flowLedgerDisabled` is deliberately NOT cleared. It @@ -433,6 +440,7 @@ export function withdrawFlow(this: Pick, subflow: string): void { - if (this.boundFlowTriggers.size === 0 && this.subflowDeclinedFlows.size === 0) return; +/** The body of {@link AutomationEngine.rejudgeSubflowCallers}; that method's docblock is its contract. */ +export function rejudgeSubflowCallers(this: Pick, subflow: string): void { + if (this.boundFlowTriggers.size === 0 && this.subflowDeclinedFlows.size === 0 && this.subflowNoticeDeclinedFlows.size === 0) return; + const reoffered = new Set(); for (const caller of this.packagedSubflowCallers(subflow)) { const settled = this.boundFlowTriggers.has(caller) ? this.disabledPackagedSubflows(caller).length === 0 @@ -255,6 +258,18 @@ export function rejudgeSubflowCallers(this: Pick l; + return l; +} + +/** The engine's arming-gate warnings for `caller`. */ +const declineWarnings = (logger: any, caller: string): string[] => + logger.warn.mock.calls + .map((c: unknown[]) => String(c[0])) + .filter((m: string) => m.includes(`Flow '${caller}'`) && m.includes('NOT armed')); + +/** A recording trigger: binding state is read off the trigger, never inferred. */ +function recordingTrigger() { + const bound = new Set(); + const trigger: FlowTrigger = { + type: 'record_change', + start(binding: FlowTriggerBinding, _cb: (ctx: AutomationContext) => Promise) { + bound.add(binding.flowName); + }, + stop(flowName: string) { + bound.delete(flowName); + }, + }; + return { trigger, isBound: (n: string) => bound.has(n) }; +} + +const nodeCtx = { logger: createTestLogger(), getService: () => undefined } as any; + +function engineWithTrigger() { + const logger = createTestLogger(); + const engine = withLoaderSetFromPull(new AutomationEngine(logger)); + registerSubflowNode(engine, nodeCtx); + registerMapNode(engine, nodeCtx); + const trigger = recordingTrigger(); + engine.registerTrigger(trigger.trigger); + return { engine, logger, isBound: trigger.isBound }; +} + +/** The row `GET /automation/_status` serves for one flow. */ +function stateOf(engine: AutomationEngine, name: string) { + return engine.getFlowRuntimeStates().find((s) => s.name === name); +} + +type Node = Record; + +const notice = (id: string): Node => ({ id, type: 'save_notice', label: 'Tell the saver', config: { message: 'Sent for review' } }); +const WAIT: Node = { id: 'hold', type: 'wait', label: 'Hold', waitEventConfig: { eventType: 'timer', timerDuration: 'PT1H' } }; +const END: Node = { id: 'end', type: 'end', label: 'End' }; + +/** A flow whose edges run through `nodes` in order, plus `backEdges` (each a `type: 'back'` rework edge). */ +function chain(name: string, nodes: Node[], backEdges: Array<[string, string]> = []) { + const pairs = nodes.slice(1).map((n, i) => ({ source: nodes[i].id as string, target: n.id as string })); + const back = backEdges.map(([source, target]) => ({ source, target, type: 'back' })); + return { + name, + label: name, + type: 'autolaunched', + nodes, + edges: [...pairs, ...back].map((edge, i) => ({ id: `e${i}`, ...edge })), + }; +} + +const start = (trigger: boolean): Node => ({ + id: 'start', type: 'start', label: 'Start', + config: trigger ? { objectName: 'deal', triggerType: 'record-after-create' } : {}, +}); + +const call = (id: string, flowName: string, type: 'subflow' | 'map' = 'subflow'): Node => ({ + id, type, label: `Call ${flowName}`, + config: { flowName, ...(type === 'map' ? { collection: [1] } : {}) }, +}); + +/** A record-triggered caller: `start → review(child) → tell(save_notice) → end`. */ +const caller = (child: string, type: 'subflow' | 'map' = 'subflow') => + chain('deal_submit', [start(true), call('review', child, type), notice('tell'), END]); + +/** A child flow: `start → [hold (wait)] → [call grandchild] → end`. */ +const child = (name: string, opts: { suspends?: boolean; calls?: string } = {}) => + chain(name, [ + start(false), + ...(opts.suspends ? [WAIT] : []), + ...(opts.calls ? [call('deeper', opts.calls)] : []), + END, + ]); + +describe('the arming gate declines a save_notice after a subflow whose child can suspend (#28840)', () => { + it('registers the caller but does not arm it: one warn, and a /_status reason naming the subflow node and the suspending node', () => { + const { engine, logger, isBound } = engineWithTrigger(); + engine.registerFlow('deal_review', child('deal_review', { suspends: true })); + engine.registerFlow('deal_submit', caller('deal_review')); + + expect(engine.getFlow('deal_submit')).toBeDefined(); + expect(isBound('deal_submit')).toBe(false); + const row = stateOf(engine, 'deal_submit'); + expect(row).toMatchObject({ enabled: true, bound: false, unboundCause: 'notice_after_suspending_subflow' }); + expect(row?.reason).toContain("`save_notice` node 'tell' is reached after `subflow` node 'review'"); + expect(row?.reason).toContain("child flow 'deal_review' can suspend the run at `wait` node 'hold'"); + expect(row?.reason).toContain("Move the `save_notice` ahead of 'review', or tell the person after it with a `notify` node"); + + const said = declineWarnings(logger, 'deal_submit'); + expect(said).toHaveLength(1); + expect(said[0]).toContain("Flow 'deal_submit' is registered but NOT armed on trigger 'record_change'"); + expect(said[0]).toContain("`subflow` node 'review'"); + expect(said[0]).toContain("`wait` node 'hold'"); + + // Said once per decline: re-registering the same caller records it again without repeating it. + engine.registerFlow('deal_submit', caller('deal_review')); + expect(isBound('deal_submit')).toBe(false); + expect(declineWarnings(logger, 'deal_submit')).toHaveLength(1); + }); + + it('declines the same shape through a `map` node', () => { + const { engine, isBound } = engineWithTrigger(); + engine.registerFlow('deal_review', child('deal_review', { suspends: true })); + engine.registerFlow('deal_submit', caller('deal_review', 'map')); + + expect(isBound('deal_submit')).toBe(false); + const row = stateOf(engine, 'deal_submit'); + expect(row?.unboundCause).toBe('notice_after_suspending_subflow'); + expect(row?.reason).toContain("is reached after `map` node 'review'"); + }); + + it('declines when the child suspends only through a flow it calls in turn, naming that flow', () => { + const { engine, isBound } = engineWithTrigger(); + engine.registerFlow('deal_ask', child('deal_ask', { suspends: true })); + engine.registerFlow('deal_review', child('deal_review', { calls: 'deal_ask' })); + engine.registerFlow('deal_submit', caller('deal_review')); + + expect(isBound('deal_submit')).toBe(false); + const row = stateOf(engine, 'deal_submit'); + expect(row?.unboundCause).toBe('notice_after_suspending_subflow'); + expect(row?.reason).toContain("child flow 'deal_review' can suspend the run at `wait` node 'hold' of 'deal_ask'"); + }); + + it('declines a save_notice inside a region body whose container stands after the subflow node', () => { + const { engine, isBound } = engineWithTrigger(); + engine.registerFlow('deal_review', child('deal_review', { suspends: true })); + const loop: Node = { + id: 'each', type: 'loop', label: 'Each', + config: { collection: [1, 2], iteratorVariable: 'item', body: { nodes: [notice('tell')], edges: [] } }, + }; + engine.registerFlow('deal_submit', chain('deal_submit', [start(true), call('review', 'deal_review'), loop, END])); + + expect(isBound('deal_submit')).toBe(false); + expect(stateOf(engine, 'deal_submit')?.reason).toContain("`save_notice` node 'tell' is reached after `subflow` node 'review'"); + }); + + it('declines when an edge after the subflow node loops back to before the save_notice', () => { + const { engine, isBound } = engineWithTrigger(); + engine.registerFlow('deal_review', child('deal_review', { suspends: true })); + engine.registerFlow( + 'deal_submit', + chain('deal_submit', [start(true), notice('tell'), call('review', 'deal_review'), END], [['review', 'tell']]), + ); + + expect(isBound('deal_submit')).toBe(false); + expect(stateOf(engine, 'deal_submit')?.unboundCause).toBe('notice_after_suspending_subflow'); + }); + + it('does not depend on load order: a caller armed before its child registers is disarmed when the child arrives suspending', () => { + const { engine, logger, isBound } = engineWithTrigger(); + engine.registerFlow('deal_submit', caller('deal_review')); + // The child is not registered yet: nothing is known to suspend. + expect(isBound('deal_submit')).toBe(true); + + engine.registerFlow('deal_review', child('deal_review', { suspends: true })); + expect(isBound('deal_submit')).toBe(false); + expect(stateOf(engine, 'deal_submit')?.unboundCause).toBe('notice_after_suspending_subflow'); + expect(declineWarnings(logger, 'deal_submit')).toHaveLength(1); + }); + + it('re-judges a caller two calls away when the flow that suspends is registered', () => { + const { engine, isBound } = engineWithTrigger(); + engine.registerFlow('deal_review', child('deal_review', { calls: 'deal_ask' })); + engine.registerFlow('deal_submit', caller('deal_review')); + expect(isBound('deal_submit')).toBe(true); + + engine.registerFlow('deal_ask', child('deal_ask', { suspends: true })); + expect(isBound('deal_submit')).toBe(false); + expect(stateOf(engine, 'deal_submit')?.unboundCause).toBe('notice_after_suspending_subflow'); + }); + + it('arms the caller again when the child is republished without its suspension, and declines it again when the suspension comes back', () => { + const { engine, logger, isBound } = engineWithTrigger(); + engine.registerFlow('deal_review', child('deal_review', { suspends: true })); + engine.registerFlow('deal_submit', caller('deal_review')); + expect(isBound('deal_submit')).toBe(false); + + engine.registerFlow('deal_review', child('deal_review')); + expect(isBound('deal_submit')).toBe(true); + const armed = stateOf(engine, 'deal_submit'); + expect(armed).toMatchObject({ enabled: true, bound: true }); + expect(armed?.reason).toBeUndefined(); + expect(armed?.unboundCause).toBeUndefined(); + + engine.registerFlow('deal_review', child('deal_review', { suspends: true })); + expect(isBound('deal_submit')).toBe(false); + expect(stateOf(engine, 'deal_submit')?.unboundCause).toBe('notice_after_suspending_subflow'); + expect(declineWarnings(logger, 'deal_submit')).toHaveLength(2); + }); + + it('arms the caller again when the flow two calls away stops suspending', () => { + const { engine, isBound } = engineWithTrigger(); + engine.registerFlow('deal_ask', child('deal_ask', { suspends: true })); + engine.registerFlow('deal_review', child('deal_review', { calls: 'deal_ask' })); + engine.registerFlow('deal_submit', caller('deal_review')); + expect(isBound('deal_submit')).toBe(false); + + engine.registerFlow('deal_ask', child('deal_ask')); + expect(isBound('deal_submit')).toBe(true); + }); + + it('terminates on a cycle of calls and arms when nothing on it suspends', () => { + const { engine, isBound } = engineWithTrigger(); + engine.registerFlow('deal_a', child('deal_a', { calls: 'deal_b' })); + engine.registerFlow('deal_b', child('deal_b', { calls: 'deal_a' })); + engine.registerFlow('deal_submit', caller('deal_a')); + expect(isBound('deal_submit')).toBe(true); + }); + + it('CONTROL: a subflow whose child cannot suspend, followed by a save_notice, still arms, with no warn', () => { + const { engine, logger, isBound } = engineWithTrigger(); + engine.registerFlow('deal_review', child('deal_review')); + engine.registerFlow('deal_submit', caller('deal_review')); + + expect(isBound('deal_submit')).toBe(true); + const row = stateOf(engine, 'deal_submit'); + expect(row).toMatchObject({ enabled: true, bound: true }); + expect(row?.unboundCause).toBeUndefined(); + expect(declineWarnings(logger, 'deal_submit')).toEqual([]); + }); + + it('CONTROL: a save_notice ahead of a subflow whose child suspends still arms', () => { + const { engine, logger, isBound } = engineWithTrigger(); + engine.registerFlow('deal_review', child('deal_review', { suspends: true })); + engine.registerFlow('deal_submit', chain('deal_submit', [start(true), notice('tell'), call('review', 'deal_review'), END])); + + expect(isBound('deal_submit')).toBe(true); + expect(stateOf(engine, 'deal_submit')?.unboundCause).toBeUndefined(); + expect(declineWarnings(logger, 'deal_submit')).toEqual([]); + }); +}); diff --git a/packages/services/service-automation/src/subflow-notice-guard.ts b/packages/services/service-automation/src/subflow-notice-guard.ts new file mode 100644 index 00000000000..cc5e9e6ec07 --- /dev/null +++ b/packages/services/service-automation/src/subflow-notice-guard.ts @@ -0,0 +1,210 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#28840] The reading behind the arming gate's save-notice half: does a + * `save_notice` of a flow run after a `subflow` / `map` node whose child flow + * can suspend? + * + * The flow parse refuses a `save_notice` that an always-suspending node + * reaches (#28806, `save-notice-placement.ts` in `@objectstack/spec`). It + * cannot see past a `subflow` / `map` node: whether that node suspends is a + * fact of ANOTHER flow, which may register after the caller. A suspended child + * suspends its caller, the write that fired the caller answers, and the + * `save_notice` after the call then refuses by name when the run resumes. So + * the engine reads it here, over the flows it holds, and + * {@link AutomationEngine.activateFlowTrigger} declines to arm such a caller. + * + * The walk is the #28806 one: any path over the top-level `edges`, loops + * included, with a region body's node judged by its top-level container. It + * is repeated here rather than exported from `@objectstack/spec`, which keeps + * no internal entry: the order prefers that to a new public helper. + * + * Only top-level `subflow` / `map` nodes are read, the ones + * {@link AutomationEngine.subflowTargets} answers, so the gate and the + * re-judge read the same calls. + */ + +import { FLOW_PAUSE_CAPABLE_NODE_TYPES, collectFlowGraphs } from '@objectstack/spec/automation'; +import type { FlowParsed } from '@objectstack/spec/automation'; + +/** A top-level node's `id` and `type`, and the flow a `subflow` / `map` node names. */ +interface TopNode { + readonly id: string; + readonly type: string; + readonly calls?: string; +} + +/** The top-level nodes in order, and the successors of each node id. */ +function topLevel(flow: FlowParsed): { nodes: TopNode[]; next: Map } { + const raw = flow as { nodes?: unknown; edges?: unknown }; + const nodes: TopNode[] = []; + for (const node of Array.isArray(raw.nodes) ? raw.nodes : []) { + const n = node as { id?: unknown; type?: unknown; config?: { flowName?: unknown } } | null; + if (!n || typeof n.id !== 'string' || typeof n.type !== 'string') continue; + const calls = (n.type === 'subflow' || n.type === 'map') && typeof n.config?.flowName === 'string' + ? n.config.flowName + : undefined; + nodes.push({ id: n.id, type: n.type, ...(calls !== undefined ? { calls } : {}) }); + } + const next = new Map(); + for (const edge of Array.isArray(raw.edges) ? raw.edges : []) { + const e = edge as { source?: unknown; target?: unknown } | null; + if (!e || typeof e.source !== 'string' || typeof e.target !== 'string') continue; + const list = next.get(e.source); + if (list) list.push(e.target); + else next.set(e.source, [e.target]); + } + return { nodes, next }; +} + +/** Every node id a path from any of `from` reaches, `from` included. */ +function reach(next: ReadonlyMap, from: readonly string[]): Set { + const seen = new Set(); + const queue = [...from]; + while (queue.length > 0) { + const id = queue.shift() as string; + if (seen.has(id)) continue; + seen.add(id); + queue.push(...(next.get(id) ?? [])); + } + return seen; +} + +/** The node ids reachable from the flow's start node, or none without one. */ +function fromStart(graph: { nodes: TopNode[]; next: Map }): Set { + const start = graph.nodes.find((n) => n.type === 'start'); + return start ? reach(graph.next, [start.id]) : new Set(); +} + +/** + * Where a flow can suspend: the pausing node, the flow that holds it, and the + * calls that lead there, from the flow asked about to that one. + */ +export interface SuspendingNode { + readonly id: string; + readonly type: string; + readonly flow: string; + readonly through: readonly string[]; +} + +/** + * The first node of an always-suspending type reachable from `name`'s start, + * or else the first one a reachable `subflow` / `map` node's child reaches, + * transitively. `undefined` when the flow is not registered or cannot suspend. + * A flow already on the walk is not read again, so a cycle of calls ends. + */ +export function suspendingNode( + flows: ReadonlyMap, + name: string, + seen: Set = new Set(), +): SuspendingNode | undefined { + if (seen.has(name)) return undefined; + seen.add(name); + const flow = flows.get(name); + if (!flow) return undefined; + const graph = topLevel(flow); + const reached = fromStart(graph); + const pause = graph.nodes.find((n) => reached.has(n.id) && FLOW_PAUSE_CAPABLE_NODE_TYPES.includes(n.type)); + if (pause) return { id: pause.id, type: pause.type, flow: name, through: [name] }; + for (const node of graph.nodes) { + if (node.calls === undefined || !reached.has(node.id)) continue; + const deeper = suspendingNode(flows, node.calls, seen); + if (deeper) return { ...deeper, through: [name, ...deeper.through] }; + } + return undefined; +} + +/** A `save_notice` a suspending `subflow` / `map` node reaches. */ +export interface SubflowNoticeTrap { + /** The `save_notice` node's own id (inside a region body, the nested node's). */ + readonly noticeId: string; + /** The `subflow` / `map` node the notice runs after. */ + readonly callNode: { readonly id: string; readonly type: string }; + /** The flow that node calls. */ + readonly child: string; + /** Where that child can suspend. */ + readonly suspends: SuspendingNode; +} + +/** + * The first `save_notice` of `name` that a path start → … → `subflow` / `map` + * node whose child can suspend → … → that notice reaches, or `undefined`. + */ +export function subflowNoticeTrap(flows: ReadonlyMap, name: string): SubflowNoticeTrap | undefined { + const flow = flows.get(name); + if (!flow) return undefined; + const notices: Array<{ judgedAs: string; id: string }> = []; + const topNodes = (flow as { nodes?: unknown }).nodes; + for (const graph of collectFlowGraphs(flow)) { + // A region body's node is judged by its top-level container, the node + // `path` = ['nodes', , 'config', …] starts at. + const container = graph.path.length === 0 || !Array.isArray(topNodes) + ? undefined + : (topNodes[graph.path[1] as number] as { id?: unknown } | undefined)?.id; + for (const node of graph.nodes) { + const n = node as { id?: unknown; type?: unknown } | null; + if (n?.type !== 'save_notice' || typeof n.id !== 'string') continue; + const judgedAs = graph.path.length === 0 ? n.id : container; + if (typeof judgedAs === 'string') notices.push({ judgedAs, id: n.id }); + } + } + if (notices.length === 0) return undefined; + const graph = topLevel(flow); + const reached = fromStart(graph); + for (const node of graph.nodes) { + if (node.calls === undefined || !reached.has(node.id)) continue; + const suspends = suspendingNode(flows, node.calls); + if (!suspends) continue; + // From its successors, not itself: a node is "after" the call only on + // a path that leaves it, which is also how a loop back counts. + const after = reach(graph.next, graph.next.get(node.id) ?? []); + const hit = notices.find((n) => after.has(n.judgedAs)); + if (hit) return { noticeId: hit.id, callNode: { id: node.id, type: node.type }, child: node.calls, suspends }; + } + return undefined; +} + +/** + * Every registered flow other than `name` whose `subflow` / `map` calls reach + * `name`, directly or through other flows: the callers whose save-notice + * reading can move when `name` changes. + */ +export function transitiveSubflowCallers( + flows: ReadonlyMap, + subflowTargets: (flow: FlowParsed) => string[], + name: string, +): string[] { + const found = new Set(); + const pending = [name]; + while (pending.length > 0) { + const target = pending.pop() as string; + for (const [callerName, flow] of flows) { + if (callerName === name || found.has(callerName)) continue; + if (!subflowTargets(flow).includes(target)) continue; + found.add(callerName); + pending.push(callerName); + } + } + return [...found]; +} + +/** + * The sentence the warn and the `/_status` reason share, in the #28806 shape: + * the notice, the call node, where the child suspends, and the two moves that + * make the flow armable. + */ +export function describeSubflowNoticeTrap(trap: SubflowNoticeTrap): string { + const { noticeId, callNode, child, suspends } = trap; + const where = suspends.flow === child + ? '' + : ` of '${suspends.flow}'${suspends.through.length > 2 + ? `, which it reaches through ${suspends.through.slice(1, -1).map((f) => `'${f}'`).join(', ')}` + : ''}`; + return ( + `its \`save_notice\` node '${noticeId}' is reached after \`${callNode.type}\` node '${callNode.id}', whose ` + + `child flow '${child}' can suspend the run at \`${suspends.type}\` node '${suspends.id}'${where}. A suspended ` + + `run resumes after the write has already answered, so on that path the node is refused by name at run time, ` + + `and after an approval the decision cannot finish its run. Move the \`save_notice\` ahead of ` + + `'${callNode.id}', or tell the person after it with a \`notify\` node` + ); +} diff --git a/packages/spec/src/contracts/automation-service.test.ts b/packages/spec/src/contracts/automation-service.test.ts index d6f0697eb7f..7d2e8ea9c5c 100644 --- a/packages/spec/src/contracts/automation-service.test.ts +++ b/packages/spec/src/contracts/automation-service.test.ts @@ -60,7 +60,7 @@ export type FlowRuntimeStateUnboundCauseIsTheClosedTuple = Assert< Eq >; export type FlowUnboundCauseIsTheTupleUnion = Assert< - Eq + Eq >; describe('Automation Service Contract', () => { diff --git a/packages/spec/src/contracts/automation-service.ts b/packages/spec/src/contracts/automation-service.ts index f97289473e8..490f2a7cc22 100644 --- a/packages/spec/src/contracts/automation-service.ts +++ b/packages/spec/src/contracts/automation-service.ts @@ -943,6 +943,12 @@ export interface FlowRuntimeState { * - `disabled_subflow` — the arming gate declined the flow because a packaged * subflow it calls is disabled (ADR-0126 §7.3). Admin remedy: enable the * subflow, and the caller arms. + * - `notice_after_suspending_subflow` — the arming gate declined the flow + * because a `save_notice` of it runs after a `subflow` / `map` node whose + * child flow can suspend (#28840): resumed after the write answered, the + * notice would be refused by name and strand the run. Author remedy: move the + * `save_notice` ahead of that node or use a `notify` node, or republish the + * child without a node that suspends, and the flow arms. * - `missing_trigger` — no trigger of the declared type is registered on this * kernel. Author remedy: depend on the trigger package * (`requires: ['triggers']`). @@ -952,6 +958,7 @@ export interface FlowRuntimeState { export const FLOW_UNBOUND_CAUSES = [ 'deployment_policy', 'disabled_subflow', + 'notice_after_suspending_subflow', 'missing_trigger', 'binding_failure', ] as const;