feat(spec)!: refuse at publish a save_notice that a suspending node reaches

.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.<i>.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`.
+
+<!-- adr-0087: not-required (no-migration-prescription) no key of any schema is added, removed, renamed or re-shaped and no export or type changes: one more graph shape is refused at the parse with its own prescription in the error (move the node ahead of the suspending node, or use a notify node after it), which is a judgment about where the node stands, not a mechanical rewrite a migration could apply -->

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']]);
+    });
+});

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<AutomationEngine>('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);
+  });
 });

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 });
   }