From ec47ffb869a4678081a1a3c9659b5db512efab5a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Nov 2026 21:50:48 +0000 Subject: [PATCH 1/2] fix(spec): refuse a save_notice that a suspending node reaches A save_notice reachable along the flow's edges from a node that always suspends the run (screen, wait, approval, approval_revise) is refused at the flow parse, naming that node. On that path the write has already answered, so the node can only refuse at run time and, after an approval, the decision strands its run. The persisted round-trip test now asks its approval through a subflow, the shape the parse cannot see, and pins the parse refusal at registerFlow. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01FEkQ5omF1nCbQTGgW1nbRA --- ...nvalid-metadata-422-face-inventory.test.ts | 54 +++++++ ...ce-approval-round-trip.integration.test.ts | 136 +++++++++++++----- packages/spec/src/automation/flow.zod.ts | 5 +- .../src/automation/save-notice-node.test.ts | 118 +++++++++++++++ .../src/automation/save-notice-placement.ts | 125 +++++++++++++++- 5 files changed, 395 insertions(+), 43 deletions(-) diff --git a/packages/metadata-protocol/src/protocol.invalid-metadata-422-face-inventory.test.ts b/packages/metadata-protocol/src/protocol.invalid-metadata-422-face-inventory.test.ts index 1a1e87cd16a..fc69ec19fb7 100644 --- a/packages/metadata-protocol/src/protocol.invalid-metadata-422-face-inventory.test.ts +++ b/packages/metadata-protocol/src/protocol.invalid-metadata-422-face-inventory.test.ts @@ -732,3 +732,57 @@ describe('[#22088] a flow edge that names no node, or repeats an earlier edge, i expect([...rows.values()].map((r) => [r.type, r.name])).toEqual([['flow', 'repro_edges']]); }); }); + +describe('[#28806] a save_notice reachable from a suspending node is refused at the metadata door', () => { + const NOTICE_FLOW = (order: 'after' | 'before') => ({ + name: 'deal_submit', + label: 'Deal submit', + type: 'record_change', + nodes: [ + { id: 'start', type: 'start', label: 'Start', config: { objectName: 'deal', triggerType: 'record-after-create' } }, + { id: 'ask', type: 'approval', label: 'Manager', config: { approvers: [{ type: 'user', value: 'u_mgr' }] } }, + { id: 'tell', type: 'save_notice', label: 'Tell the saver', config: { message: 'Sent {{ record.name }} for approval' } }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: order === 'after' + ? [ + { id: 'e1', source: 'start', target: 'ask' }, + { id: 'e2', source: 'ask', target: 'tell', label: 'approve' }, + { id: 'e3', source: 'ask', target: 'end', label: 'reject' }, + { id: 'e4', source: 'tell', target: 'end' }, + ] + : [ + { id: 'e1', source: 'start', target: 'tell' }, + { id: 'e2', source: 'tell', target: 'ask' }, + { id: 'e3', source: 'ask', target: 'end', label: 'approve' }, + { id: 'e4', source: 'ask', target: 'end', label: 'reject' }, + ], + }); + + it.each([ + ['publish', undefined], + ['draft', 'draft'], + ] as const)('%s mode — 422 INVALID_METADATA at the node\'s type, naming the approval, nothing stored', async (_label, mode) => { + const { protocol, rows } = makeProtocol(); + const err = await saveFlowAsAdministrator(protocol, NOTICE_FLOW('after'), mode); + + expect(err).toBeInstanceOf(Error); + expect({ code: err.code, status: err.status }).toEqual({ code: 'INVALID_METADATA', status: 422 }); + const issues = err.issues as Array<{ code?: string; path?: string; message: string }>; + expect(issues.map((i) => [i.code, i.path])).toEqual([['custom', 'nodes.2.type']]); + expect(issues[0]!.message).toContain( + "This one is reached after `approval` node 'ask' suspends the run. 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 'ask', or tell the person after it with a " + + '`notify` node.', + ); + expect(rows.size).toBe(0); + }); + + it('CONTROL — the same flow with the notice ahead of the approval is stored', async () => { + const { protocol, rows } = makeProtocol(); + const result = await saveFlowAsAdministrator(protocol, NOTICE_FLOW('before')); + expect(result instanceof Error ? `${result.message} ${JSON.stringify((result as any).issues ?? [])}` : 'stored').toBe('stored'); + expect([...rows.values()].map((r) => [r.type, r.name])).toEqual([['flow', 'deal_submit']]); + }); +}); 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 b61cf86a8ab..93cc8bcd184 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 @@ -8,17 +8,24 @@ * * The record-change trigger hands a record-triggered run the triggering * write's notice channel (`AutomationContext.saveNotice`). A `save_notice` - * node BEFORE the approval node sets the sentence on that write's answer. The - * approval node then suspends the run, the run state is persisted as JSON in + * node BEFORE the approval sets the sentence on that write's answer. The + * approval then suspends the run, the run state is persisted as JSON in * `sys_automation_run`, and the decision resumes it from there. JSON keeps the * channel's `locale` and drops its `set` function, so a `save_notice` node on * the resumed leg finds no channel and is refused by name: there is no write * left to answer. * - * The executor's own suite pins that refusal with a hand-built context. This - * suite pins it through the real round trip, so a store that started keeping - * (or re-binding) the channel, or an engine that resumed from an in-memory copy - * instead of the store, is caught here. + * [#28806] A `save_notice` that a suspending node reaches on the flow's own + * graph is now refused at the parse, so the resumed leg is reached through a + * `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. + * + * 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 + * started keeping (or re-binding) the channel, or an engine that resumed from + * an in-memory copy instead of the store, is caught here. * * ## The rig * @@ -27,10 +34,10 @@ * (`ObjectStoreSuspendedRunStore` over `sys_automation_run`, which the plugin * registers), and `RecordChangeTriggerPlugin`. The real `ApprovalService` is * attached to the real automation engine behind the real approval node, so the - * decision resumes the run the way a deployment does. A resumed leg that fails - * makes the decision answer the approvals' stranded-decision failure (#4420), - * which names the run's own refusal; that is existing behaviour, read here, - * not a new refusal. + * decision resumes the run the way a deployment does. A resumed subflow parent + * that fails makes the decision answer the approvals' existing subflow-parent + * strand (#4420: `resumeFailure` names the parent run, `resumeError` its + * refusal); that is existing behaviour, read here, not a new refusal. * * ## Coverage pin * @@ -46,7 +53,6 @@ import { SqlDriver } from '@objectstack/driver-sql'; import { AutomationServicePlugin, type AutomationEngine } from '@objectstack/service-automation'; import { RecordChangeTriggerPlugin } from '@objectstack/trigger-record-change'; import type { EngineQueryOptions } from '@objectstack/spec/data'; -import { strandedDecisionDetails } from '@objectstack/types'; import { ApprovalService } from './approval-service.js'; import { ApprovalsServicePlugin } from './approvals-plugin.js'; import { registerApprovalNode } from './approval-node.js'; @@ -74,18 +80,64 @@ const deal = { /** * The shape #28251 documents: tell the saver it went for approval, then ask. - * The approve leg carries a second `save_notice`, which runs only after the - * decision resumed the run from storage. + * + * [#28806] A `save_notice` that a suspending node reaches on the flow's own + * graph is refused at the parse now, so the second notice cannot stand right + * after the `approval` node any more (`DIRECT_AFTER` below is that refusal's + * pin at `registerFlow`). The approval is asked in a SUBFLOW instead: a + * `subflow` pauses only when its child does, which the parse cannot see, so + * 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. */ +const CHILD = 'deal_manager_review'; +const child = { + name: CHILD, + label: 'Manager review', + type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'ask', type: 'approval', label: 'Manager', config: { approvers: [{ type: 'user', value: APPROVER }] } }, + { id: 'ok', type: 'end', label: 'Approved' }, + { id: 'no', type: 'end', label: 'Rejected' }, + ], + edges: [ + { id: 'c1', source: 'start', target: 'ask' }, + { id: 'c2', source: 'ask', target: 'ok', label: 'approve' }, + { id: 'c3', source: 'ask', target: 'no', label: 'reject' }, + ], +}; +const START = { id: 'start', type: 'start', label: 'Start', config: { objectName: deal.name, triggerType: 'record-after-create' } }; +const TELL = { id: 'tell', type: 'save_notice', label: 'Tell', config: { message: 'Sent {{ record.name }} to your manager for approval' } }; +const TELL_AFTER = { id: 'tell_after', type: 'save_notice', label: 'Tell again', config: { message: '{{ record.name }} was approved' } }; const flow = { name: FLOW, label: 'Deal sent for approval', type: 'record_change', nodes: [ - { id: 'start', type: 'start', label: 'Start', config: { objectName: deal.name, triggerType: 'record-after-create' } }, - { id: 'tell', type: 'save_notice', label: 'Tell', config: { message: 'Sent {{ record.name }} to your manager for approval' } }, + START, + TELL, + { id: 'review', type: 'subflow', label: 'Manager review', config: { flowName: CHILD } }, + TELL_AFTER, + { id: 'ok', type: 'end', label: 'Done' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'tell' }, + { id: 'e2', source: 'tell', target: 'review' }, + { id: 'e3', source: 'review', target: 'tell_after' }, + { id: 'e4', source: 'tell_after', target: 'ok' }, + ], +}; +/** The #28786 shape: the second notice right after the approval, on this flow's own graph. */ +const DIRECT_AFTER = { + name: 'deal_notice_after_approval', + label: 'Deal notice after approval', + type: 'record_change', + nodes: [ + START, + TELL, { id: 'ask', type: 'approval', label: 'Manager', config: { approvers: [{ type: 'user', value: APPROVER }] } }, - { id: 'tell_after', type: 'save_notice', label: 'Tell again', config: { message: '{{ record.name }} was approved' } }, + TELL_AFTER, { id: 'ok', type: 'end', label: 'Approved' }, { id: 'no', type: 'end', label: 'Rejected' }, ], @@ -145,6 +197,7 @@ describe('[#28786] a save_notice before an approval answers the write, and one o svc = new ApprovalService({ engine: ql as any, logger: silent }); svc.attachAutomation(automation as any); registerApprovalNode(automation as any, svc, silent); + automation.registerFlow(CHILD, child as any); automation.registerFlow(FLOW, flow as any); }); @@ -164,40 +217,51 @@ describe('[#28786] a save_notice before an approval answers the write, and one o // ── Suspended and persisted: one live row, its context stored as JSON. const [pause] = await runRows(); - expect(pause, 'the run suspended at the approval node and was persisted').toBeDefined(); - expect(pause.node_id).toBe('ask'); + expect(pause, 'the run suspended with its child and was persisted').toBeDefined(); + expect(pause.node_id).toBe('review'); expect(pause.status, 'a live pause, not a terminal history row').toBe('paused'); const stored = JSON.parse(String(pause.context_json)); // What survives JSON: the channel object without its function. expect(stored.saveNotice === undefined || typeof stored.saveNotice === 'object').toBe(true); expect(typeof stored.saveNotice?.set, 'no function survives the persisted context').toBe('undefined'); - // ── The decision resumes the run from the store. + // ── The decision completes the child, which resumes the parent from the store. const [request] = await ql.find('sys_approval_request', { where: { record_id: created.id, status: 'pending' }, context: SYSTEM, } satisfies EngineQueryOptions); - expect(request?.flow_node_id, 'the approval node opened its request').toBe('ask'); - // The resumed leg fails at its save_notice, so the decision answers the - // approvals' existing stranded-decision failure (#4420): the decision is - // recorded and the run's own refusal is named in it. + expect(request?.flow_node_id, "the child's approval node opened its request").toBe('ask'); + // The parent's resumed leg fails at its save_notice. The child's own run + // resumed and completed, so the decision answers with the parent named as + // the stranded run (the approvals' existing subflow-parent strand, #4420): + // recorded, not thrown, and the parent's own refusal named in it. + const answered: any = await svc.decide(request.id, { decision: 'approve', actorId: APPROVER }, asUser(APPROVER)); + expect([answered.finalized, answered.decision, answered.resumed]).toEqual([true, 'approve', true]); + expect(answered.resumeFailure).toMatchObject({ code: 'RESUME_FAILED', runId: pause.id, status: 'stranded' }); + expect(String(answered.resumeError)).toContain("save_notice 'tell_after': this run holds no record write to answer"); + + // ── The resumed leg's save_notice is refused by name, and nothing reaches the write. + let terminal: any; + for (let i = 0; i < 100 && !terminal; i++) { + terminal = (await runRows()).find((r) => String(r.id) === `run_${pause.id}`); + if (!terminal) await new Promise((r) => setTimeout(r, 20)); + } + expect(terminal, 'the resumed parent left its history row').toBeDefined(); + expect((await runRows()).some((r) => r.id === pause.id), 'the live pause was consumed').toBe(false); + expect(terminal.status).toBe('failed'); + expect(String(terminal.error)).toContain("save_notice 'tell_after': this run holds no record write to answer"); + expect(heard, 'the resumed leg set nothing on the answer already sent').toEqual(['Sent Acme to your manager for approval']); + }, 20000); + + it('[#28806] the same notice right after the approval on the flow\'s own graph is refused at registerFlow, naming the approval', () => { + const automation = kernel.getService('automation'); let thrown: unknown; try { - await svc.decide(request.id, { decision: 'approve', actorId: APPROVER }, asUser(APPROVER)); + automation.registerFlow(DIRECT_AFTER.name, DIRECT_AFTER as any); } catch (err) { thrown = err; } expect(String((thrown as Error | undefined)?.message)).toContain( - "save_notice 'tell_after': this run holds no record write to answer", + "This one is reached after `approval` node 'ask' suspends the run.", ); - expect(strandedDecisionDetails(thrown)).toMatchObject({ finalized: true, decision: 'approve', runId: pause.id }); - - // ── The resumed leg's save_notice is refused by name, and nothing reaches the write. - const rows = await runRows(); - expect(rows.some((r) => r.id === pause.id), 'the live pause was consumed').toBe(false); - const terminal = rows.find((r) => String(r.id) === `run_${pause.id}`); - expect(terminal, 'the resumed run left its history row').toBeDefined(); - expect(terminal.status).toBe('failed'); - expect(String(terminal.error)).toContain("save_notice 'tell_after': this run holds no record write to answer"); - expect(heard, 'the resumed leg set nothing on the answer already sent').toEqual(['Sent Acme to your manager for approval']); - }, 20000); + }); }); diff --git a/packages/spec/src/automation/flow.zod.ts b/packages/spec/src/automation/flow.zod.ts index 75e2a2d949a..f375ad5aee3 100644 --- a/packages/spec/src/automation/flow.zod.ts +++ b/packages/spec/src/automation/flow.zod.ts @@ -1639,8 +1639,9 @@ export const FlowSchema = lazySchema(() => strictObject( // [#28251] A `save_notice` node answers the write that fired the run, so it // stands only in a flow a create or update record trigger starts — anywhere // else no write waits on the run. Refused at each such node, at any region - // depth; the reasons are `save-notice-placement.ts`'s header. - for (const refusal of saveNoticePlacementRefusals(flow)) { + // depth; the reasons are `save-notice-placement.ts`'s header. [#28806] Also + // refused where a node that always suspends the run reaches it. + for (const refusal of saveNoticePlacementRefusals(flow, FLOW_PAUSE_CAPABLE_NODE_TYPES)) { ctx.addIssue({ code: 'custom', path: [...refusal.path], message: refusal.message }); } diff --git a/packages/spec/src/automation/save-notice-node.test.ts b/packages/spec/src/automation/save-notice-node.test.ts index 806c3776f71..b55b04ba2f7 100644 --- a/packages/spec/src/automation/save-notice-node.test.ts +++ b/packages/spec/src/automation/save-notice-node.test.ts @@ -131,6 +131,124 @@ describe('FlowSchema — a save_notice node stands only in a flow a record creat }); }); +/** + * [#28806] A `save_notice` reachable from a node that suspends the run. On that + * path the write has already answered, so the node is refused by name at run + * time and, after an approval, the decision strands its run. The parse refuses + * it: a path start → … → suspending node → … → `save_notice` exists (loops + * included), whichever other paths also reach it. + */ +describe('FlowSchema — a save_notice reachable from a suspending node is refused (#28806)', () => { + const START = { id: 'start', type: 'start', label: 'Start', config: { objectName: 'deal', triggerType: 'record-after-create' } }; + const TELL = { id: 'tell', type: 'save_notice', label: 'Tell the saver', config: { message: NOTICE } }; + const ASK = { id: 'ask', type: 'approval', label: 'Manager', config: { approvers: [{ type: 'user', value: 'u_mgr' }] } }; + const END = { id: 'end', type: 'end', label: 'End' }; + const flow = (nodes: Array>, edges: Array<[string, string, string?]>) => ({ + name: 'deal_submit', + label: 'Deal submit', + type: 'record_change', + nodes, + edges: edges.map(([source, target, label], i) => ({ id: `e${i + 1}`, source, target, ...(label ? { label } : {}) })), + }); + const sentence = (type: string, id: string) => + `This one is reached after \`${type}\` node '${id}' suspends the run. 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 '${id}', or tell the person after it with a ` + + '`notify` node.'; + + it('is refused after an approval, at the node\'s `type`, naming the approval', () => { + const issues = placementIssues(FlowSchema.safeParse(flow( + [START, ASK, TELL, END], + [['start', 'ask'], ['ask', 'tell', 'approve'], ['ask', 'end', 'reject'], ['tell', 'end']], + ))); + expect(issues.map((i) => [i.code, i.path])).toEqual([['custom', ['nodes', 2, 'type']]]); + expect(issues[0].message).toContain(sentence('approval', 'ask')); + }); + + it.each([ + ['wait', { id: 'pause', type: 'wait', label: 'Pause', waitEventConfig: { eventType: 'timer', timerDuration: 'PT1H' } }], + ['screen', { id: 'pause', type: 'screen', label: 'Pause', config: {} }], + ['approval_revise', { id: 'pause', type: 'approval_revise', label: 'Pause', config: {} }], + ])('is refused after a `%s` node', (type, pause) => { + const issues = placementIssues(FlowSchema.safeParse(flow( + [START, pause, TELL, END], + [['start', 'pause'], ['pause', 'tell'], ['tell', 'end']], + ))); + expect(issues.map((i) => i.path)).toEqual([['nodes', 2, 'type']]); + expect(issues[0].message).toContain(sentence(type, 'pause')); + }); + + it('is refused several hops after the suspension', () => { + const issues = placementIssues(FlowSchema.safeParse(flow( + [START, ASK, { id: 'set', type: 'assignment', label: 'Set', config: {} }, TELL, END], + [['start', 'ask'], ['ask', 'set', 'approve'], ['set', 'tell'], ['tell', 'end'], ['ask', 'end', 'reject']], + ))); + expect(issues.map((i) => i.path)).toEqual([['nodes', 3, 'type']]); + expect(issues[0].message).toContain(sentence('approval', 'ask')); + }); + + it('is refused when only ONE path reaches it through the suspension (a mixed graph)', () => { + const issues = placementIssues(FlowSchema.safeParse(flow( + [START, { id: 'big', type: 'decision', label: 'Big deal?' }, ASK, TELL, END], + [['start', 'big'], ['big', 'ask', 'true'], ['big', 'tell', 'false'], ['ask', 'tell', 'approve'], ['ask', 'end', 'reject'], ['tell', 'end']], + ))); + expect(issues.map((i) => i.path)).toEqual([['nodes', 3, 'type']]); + expect(issues[0].message).toContain(sentence('approval', 'ask')); + }); + + it('is refused when an edge after the approval loops back to before the notice', () => { + const issues = placementIssues(FlowSchema.safeParse(flow( + [START, { id: 'set', type: 'assignment', label: 'Set', config: {} }, TELL, ASK, END], + [['start', 'set'], ['set', 'tell'], ['tell', 'ask'], ['ask', 'end', 'approve'], ['ask', 'set', 'reject']], + ))); + expect(issues.map((i) => i.path)).toEqual([['nodes', 2, 'type']]); + expect(issues[0].message).toContain(sentence('approval', 'ask')); + }); + + it('is refused inside a region body whose container stands after the approval', () => { + const loop = { + id: 'each', type: 'loop', label: 'Each', + config: { + collection: [1, 2], + iteratorVariable: 'item', + body: { nodes: [{ ...TELL }], edges: [] }, + }, + }; + const issues = placementIssues(FlowSchema.safeParse(flow( + [START, ASK, loop, END], + [['start', 'ask'], ['ask', 'each', 'approve'], ['ask', 'end', 'reject'], ['each', 'end']], + ))); + expect(issues.map((i) => i.path)).toEqual([['nodes', 2, 'config', 'body', 'nodes', 0, 'type']]); + expect(issues[0].message).toContain(sentence('approval', 'ask')); + }); + + it('CONTROL — a save_notice before the approval still parses', () => { + const result = FlowSchema.safeParse(flow( + [START, TELL, ASK, END], + [['start', 'tell'], ['tell', 'ask'], ['ask', 'end', 'approve'], ['ask', 'end', 'reject']], + )); + expect(placementIssues(result)).toEqual([]); + expect(result.success).toBe(true); + }); + + it('CONTROL — a save_notice on a branch the approval never reaches still parses', () => { + const result = FlowSchema.safeParse(flow( + [START, { id: 'big', type: 'decision', label: 'Big deal?' }, ASK, TELL, END], + [['start', 'big'], ['big', 'ask', 'true'], ['big', 'tell', 'false'], ['ask', 'end', 'approve'], ['ask', 'end', 'reject'], ['tell', 'end']], + )); + expect(placementIssues(result)).toEqual([]); + expect(result.success).toBe(true); + }); + + it('CONTROL — a subflow ahead of the notice is not a suspending node (it pauses only when its child does)', () => { + const result = FlowSchema.safeParse(flow( + [START, { id: 'child', type: 'subflow', label: 'Child', config: { flowName: 'deal_child' } }, TELL, END], + [['start', 'child'], ['child', 'tell'], ['tell', 'end']], + )); + expect(placementIssues(result)).toEqual([]); + }); +}); + describe('flows..notices..message — the translation address', () => { it('is spelled by flowNoticeMessageKey', () => { expect(flowNoticeMessageKey('deal_submit', 'tell')).toBe('flows.deal_submit.notices.tell.message'); diff --git a/packages/spec/src/automation/save-notice-placement.ts b/packages/spec/src/automation/save-notice-placement.ts index 22a7c5a039a..1a6a1080bf6 100644 --- a/packages/spec/src/automation/save-notice-placement.ts +++ b/packages/spec/src/automation/save-notice-placement.ts @@ -20,9 +20,23 @@ * dropped at run time: a node an author placed and the run silently ignored is * exactly the declared-but-not-enforced shape the platform refuses. * - * What the parse cannot see is left to the run: a run that pauses (an - * `approval` or a `wait`) and resumes no longer holds the write, so a - * `save_notice` reached on the resumed leg refuses the node by name there. + * [#28806] A run that suspends (an `approval`, a `wait`, a `screen`, an + * `approval_revise`) resumes after the write has answered, so a `save_notice` + * reached on the resumed leg can only refuse by name at run time, and after an + * approval the decision then strands its run (`RESUME_FAILED`). The parse sees + * that leg in the graph: a `save_notice` reachable along the flow's edges from + * a node that always suspends (`FLOW_PAUSE_CAPABLE_NODE_TYPES`, handed in by + * the caller) is refused here, naming that node. "Reachable" is ANY path from + * the start through such a node, loops included, not "every path": a static + * graph cannot rule the suspended path out, so a node with a clean path beside + * it is still a trap on the other one. A pausing node stands on the top-level + * graph only (a region body refuses one), so a `save_notice` inside a region + * is judged by its top-level container. + * + * What the parse still cannot see is left to the run: a `subflow` or a `map` + * pauses only when the child flow it names does (a different record), and a + * plugin-registered pausing type is not in the list. A `save_notice` reached + * after one of those refuses the node by name there. */ import { collectFlowGraphs } from './control-flow.zod'; @@ -45,12 +59,71 @@ function startTriggerType(flow: { nodes?: unknown }): unknown { return start?.config?.triggerType; } +/** A top-level node's `id` and `type`, when both are strings. */ +function idAndType(node: unknown): { id: string; type: string } | undefined { + if (node === null || typeof node !== 'object') return undefined; + const { id, type } = node as { id?: unknown; type?: unknown }; + return typeof id === 'string' && typeof type === 'string' ? { id, type } : undefined; +} + +/** + * [#28806] For each top-level node a path start → … → suspending node → … → + * that node reaches, the suspending nodes on such a path, in node order. + * + * Reads the raw top-level `nodes` / `edges`: this runs inside the + * `FlowSchema` parse, so a malformed member (no string `id`, an edge with no + * string `source` / `target`) is skipped, never read through. + */ +function suspendedReach( + flow: { nodes?: unknown; edges?: unknown }, + pauseCapableTypes: readonly string[], +): Map> { + const nodes = (Array.isArray(flow.nodes) ? flow.nodes : []).map(idAndType).filter((n) => n !== undefined); + const next = new Map(); + for (const edge of Array.isArray(flow.edges) ? flow.edges : []) { + if (edge === null || typeof edge !== 'object') continue; + const { source, target } = edge as { source?: unknown; target?: unknown }; + if (typeof source !== 'string' || typeof target !== 'string') continue; + const list = next.get(source); + if (list) list.push(target); + else next.set(source, [target]); + } + const walk = (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; + }; + const reach = new Map>(); + const start = nodes.find((n) => n.type === 'start'); + if (!start) return reach; + const fromStart = walk([start.id]); + for (const pause of nodes) { + if (!pauseCapableTypes.includes(pause.type) || !fromStart.has(pause.id)) continue; + // From its successors, not itself: the node is "after" the pause only on a + // path that leaves it, which is also how a loop back to it counts. + for (const id of walk(next.get(pause.id) ?? [])) { + const list = reach.get(id); + if (list) list.push(pause); + else reach.set(id, [pause]); + } + } + return reach; +} + /** * Every `save_notice` node of `flow` (at any region depth) standing in a flow - * whose start is not a record trigger on a create or an update. + * whose start is not a record trigger on a create or an update, or (#28806) + * reachable from a node whose type is in `pauseCapableTypes`. */ export function saveNoticePlacementRefusals( flow: Parameters[0], + pauseCapableTypes: readonly string[], ): SaveNoticePlacementRefusal[] { const out: SaveNoticePlacementRefusal[] = []; const graphs = collectFlowGraphs(flow); @@ -59,7 +132,9 @@ export function saveNoticePlacementRefusals( ); if (!hasNotice) return out; const trigger = startTriggerType(flow as { nodes?: unknown }); - if (typeof trigger === 'string' && SAVE_NOTICE_TRIGGER.test(trigger.trim())) return out; + if (typeof trigger === 'string' && SAVE_NOTICE_TRIGGER.test(trigger.trim())) { + return suspendedLegRefusals(flow as { nodes?: unknown; edges?: unknown }, graphs, pauseCapableTypes); + } // [#28786] The ARRAY form (`['record-after-create', 'record-after-update']`) // is refused with the same verdict, but named as what it is: multi-event // unions are unsupported and never bind (#3457), so "no record trigger" @@ -87,3 +162,43 @@ export function saveNoticePlacementRefusals( } return out; } + +/** + * [#28806] The `save_notice` nodes of a record-triggered flow that a + * suspending node reaches, each refused naming the first such node. + */ +function suspendedLegRefusals( + flow: { nodes?: unknown; edges?: unknown }, + graphs: ReturnType, + pauseCapableTypes: readonly string[], +): SaveNoticePlacementRefusal[] { + const out: SaveNoticePlacementRefusal[] = []; + const reach = suspendedReach(flow, pauseCapableTypes); + if (reach.size === 0) return out; + const topLevel = Array.isArray(flow.nodes) ? flow.nodes : []; + for (const graph of graphs) { + // 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 + ? undefined + : idAndType(topLevel[graph.path[1] as number]); + graph.nodes.forEach((node, index) => { + if ((node as { type?: unknown } | null)?.type !== 'save_notice') return; + const self = graph.path.length === 0 ? idAndType(node) : container; + const pauses = self ? reach.get(self.id) : undefined; + if (!pauses || pauses.length === 0) return; + const { id, type } = pauses[0]; + out.push({ + path: [...graph.path, 'nodes', index, 'type'], + message: + 'A `save_notice` node tells the person who made the write that fired this run what the save set in ' + + 'motion, so it runs only while the run is still inside that write. ' + + `This one is reached after \`${type}\` node '${id}' suspends the run. 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 '${id}', or tell the person after it with a ` + + '`notify` node.', + }); + }); + } + return out; +} From bb9723f5d03561fa6aa5a69e11af7b4777b27712 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Nov 2026 21:53:26 +0000 Subject: [PATCH 2/2] chore: changeset for the save_notice suspension refusal Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01FEkQ5omF1nCbQTGgW1nbRA --- ...806-save-notice-after-suspension-refused.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 .changeset/28806-save-notice-after-suspension-refused.md diff --git a/.changeset/28806-save-notice-after-suspension-refused.md b/.changeset/28806-save-notice-after-suspension-refused.md new file mode 100644 index 00000000000..ff68d004e33 --- /dev/null +++ b/.changeset/28806-save-notice-after-suspension-refused.md @@ -0,0 +1,18 @@ +--- +'@objectstack/spec': minor +--- + +feat(spec)!: a `save_notice` node that a suspending node reaches is refused at the flow parse (#28806) + +Clause-②: no (narrowing) +Metadata-protocol: narrowed + +**BREAKING** for a flow that places a `save_notice` node after an `approval`, `wait`, `screen` or `approval_revise` node on a path from its start. + +- **Before.** The placement check (#28251) read only the start trigger, so a record-triggered flow with `start → approval → save_notice` parsed and saved. At run time the leg after the suspension holds no write to answer, so the node was refused by name. After an approval the decision was already recorded when the resumed run failed, so every decision on such a flow stranded its run (`RESUME_FAILED`). +- **Now.** The `FlowSchema` parse also refuses a `save_notice` node reachable along the flow's edges from a node that always suspends the run (`FLOW_PAUSE_CAPABLE_NODE_TYPES`: `screen`, `wait`, `approval`, `approval_revise`). Any one such path counts, loops included, even when another path reaches the node before any suspension, because a static graph cannot rule the suspended path out. A `save_notice` inside a region body is judged by its top-level container. The refusal is the same `INVALID_METADATA` 422 at `nodes..type` as the trigger case. It names the suspending node and tells the author to move the `save_notice` ahead of it, or to tell the person after it with a `notify` node. `objectstack validate`, the metadata save door and `registerFlow` refuse it alike. +- **Not refused.** A `save_notice` reachable only before any suspension. A `subflow` or `map` ahead of the node: those pause only when the child flow they name pauses, which the parse cannot see, so a `save_notice` after one still meets the run-time refusal by name. +- **The fix.** Move the `save_notice` ahead of the suspending node the refusal names. To tell someone after the approval, use a `notify` node there. +- **Who moves.** No shipped flow has the refused shape: the showcase's one `save_notice` (`showcase_task_completed`) has no suspending node, and HotCRM's `main` carries no `save_notice`. + +