diff --git a/.changeset/approval-revise-window-service-owned.md b/.changeset/approval-revise-window-service-owned.md new file mode 100644 index 0000000000..3d7c54755b --- /dev/null +++ b/.changeset/approval-revise-window-service-owned.md @@ -0,0 +1,60 @@ +--- +"@objectstack/spec": minor +"@objectstack/plugin-approvals": minor +"@objectstack/lint": minor +--- + +fix(approvals): the ADR-0044 revise window is a service-owned node type, not a bare `wait` (#3823) + +#3801 gated `POST /api/v1/automation/:name/runs/:runId/resume` on the **node type** +that produced the suspension: an `approval` pause declares +`resumeAuthority: 'service'`, so it continues only through `ApprovalService`. +ADR-0044's **revise window** was the same trust boundary in a shape that key +could not see. Send-back parked the run on an ordinary `wait` node the flow +author placed — correctly `resumeAuthority: 'any'`, because a signal wait is +*meant* to be resumable by an external producer — and `ApprovalService.resubmit` +was the only thing that checked anything about continuing it. + +Demonstrated (not reasoned) against the real engine: a raw `resume(runId)` with +an **empty body**, from any caller, walked the `resubmit` back-edge into the +approval node and opened round N+1 with **no submitter check and no `resubmit` +audit row** (`['submit','revise']` — no third row, ever). Worse, when another +request was already pending on the record — the exact case `resubmit` refuses +with `DUPLICATE_REQUEST` *specifically to keep the run alive* — the raw resume +went around that guard: the approval node's re-entry failed **after** the engine +consumed the suspension, and the run was **permanently destroyed** with its +round-N request stuck `returned` and no resubmit able to reach it. + +The revise pause is therefore its own node type: + +- **`approval_revise`** (`APPROVAL_REVISE_NODE_TYPE`), registered by + `@objectstack/plugin-approvals` alongside the `approval` node, declaring + `resumeAuthority: 'service'`. It stays a first-class box on the canvas, in the + run log and in the suspended-run store — only the *reuse* of `wait` was wrong. + It takes **no config**: the window ends on the submitter's explicit resubmit, + never on a signal or timer. The `resumeAuthority` gate itself is unchanged. +- `sendBack` refuses a `revise` edge whose target is not an `approval_revise` + node, **before any mutation** (like the existing missing-`revise`-edge check), + so no run can be parked in a window something else can advance. +- New gating lint `flow-approval-revise-target-not-service-owned` + (severity `error`, on `os build` / `os validate` / `os lint` and the runtime + metadata publish gate) rejects the old shape at authoring time. + +**Upgrading a flow authored against the original ADR-0044 D3.** One token: + +- **FROM:** `{ id: 'wait_revision', type: 'wait', waitEventConfig: { eventType: 'signal', … } }` +- **TO:** `{ id: 'wait_revision', type: 'approval_revise' }` — drop + `waitEventConfig` / any `config`; the window has no event to wait on. + +Until you do, such a flow keeps registering and running and its approvals stay +decidable (`approve` / `reject` / `recall` / `reassign` are untouched), but +**send-back is refused** with a message naming the node and this fix, and +re-publishing it reports the lint error. A run *already parked* in a legacy +revise window keeps its recorded node type (a republish never re-types a live +pause) and is drained by `resubmit` or `recall` as usual. + +ADR-0044's 2026-07-28 amendment records the reversal of its D3 and of its +`Alternatives` rejection of a service-owned revise pause, with the evidence +above; the implementation section there records what shipped, why the approval +node does not re-suspend itself instead, and why no ADR-0087 conversion was +added for the old shape. diff --git a/content/docs/automation/approvals.mdx b/content/docs/automation/approvals.mdx index 6696828f1a..a09a379eb6 100644 --- a/content/docs/automation/approvals.mdx +++ b/content/docs/automation/approvals.mdx @@ -422,11 +422,23 @@ continuity levers on the same request. All are `POST **Send-back → resubmit (ADR-0044).** An approver who wants changes rather than a hard reject calls `revise`: the round finalizes `returned`, the record unlocks, -and the run parks at a wait point. The submitter reworks the record and -`resubmit`s (a fresh round opens for all approvers) or `recall`s (abandons it). -Past the node's `maxRevisions` budget (default 3) a send-back **auto-rejects** -instead. The flow's approval node must declare a `revise` edge for send-back to -be available. +and the run parks in the **revise window** — an `approval_revise` node on the +flow's `revise` edge. The submitter reworks the record and `resubmit`s (a fresh +round opens for all approvers) or `recall`s (abandons it). Past the node's +`maxRevisions` budget (default 3) a send-back **auto-rejects** instead. The +flow's approval node must declare a `revise` edge, and that edge must target an +`approval_revise` node, for send-back to be available. + +The window is deliberately **not** an ordinary `wait`: `resubmit` is what +authorizes (submitter-only), orders (latest round) and records (an audit row) the +continuation, and refuses when another request is already pending on the record — +so the pause it parks on declares `resumeAuthority: 'service'` and the generic +`POST /api/v1/automation/:name/runs/:runId/resume` route answers **403** for it. +ADR-0044 D3 originally prescribed a `wait` here; its 2026-07-28 amendment +reversed that (#3823). A flow still carrying the old shape publishes with an +error (`flow-approval-revise-target-not-service-owned`) and its `revise` verb is +refused at runtime until the node's `type` is changed — approve/reject are +unaffected. ### Acting on requests in the console diff --git a/docs/adr/0044-approval-send-back-for-revision.md b/docs/adr/0044-approval-send-back-for-revision.md index ad90a21182..94ad8dcb62 100644 --- a/docs/adr/0044-approval-send-back-for-revision.md +++ b/docs/adr/0044-approval-send-back-for-revision.md @@ -1,6 +1,6 @@ # ADR-0044: Flow-level send-back-for-revision — `revise` branch + typed back-edge re-entry -**Status**: Accepted — engine + model implemented; designer pending (objectui) (proposed 2026-06-12 · calibrated 2026-06-12 · **amended 2026-07-28 (#3823): the revise pause moves to a service-owned node — D3's generic `wait` is superseded, see the amendment below**) +**Status**: Accepted — engine + model implemented; designer pending (objectui) (proposed 2026-06-12 · calibrated 2026-06-12 · **amended 2026-07-28 (#3823): the revise pause moves to a service-owned node — D3's generic `wait` is superseded; amendment ratified by the maintainer and implemented 2026-08-05 as the `approval_revise` node type, see the amendment below**) **Deciders**: ObjectStack Protocol Architects **Builds on**: [ADR-0019](./0019-approval-as-flow-node.md) (approval as a durable-pause flow node), [ADR-0039](./0039-token-scope-tree-execution.md) (single-program-counter suspend model), thread interactions (#1740), [ADR-0042](./0042-approval-sla-escalation.md) (audit-first discipline) **Closes**: [#1744](https://github.com/objectstack-ai/objectstack/issues/1744) @@ -78,7 +78,9 @@ stores all already understand it), not an invisible service limbo. > made `resume` authorization-bearing, that became exploitable (unauthorized > resubmit with no audit row; a colliding request can permanently destroy the > run). The revise pause moves to a **dedicated service-owned node** — still -> visible on the canvas, no longer raw-resumable. See the amendment below. +> visible on the canvas, no longer raw-resumable. Shipped 2026-08-05 as +> **`approval_revise`**; read D3 as "the revise edge targets an +> `approval_revise` node". See the amendment below. Resubmit is an explicit REST verb by the submitter: @@ -147,7 +149,7 @@ them, under explicit constraints: | moment | request status | lock | |---|---|---| | round N pending | `pending` | locked | -| revise window (run at wait node) | `returned` | **unlocked** (hook keys on pending) | +| revise window (run at the `approval_revise` node) | `returned` | **unlocked** (hook keys on pending) | | after resubmit (round N+1) | new row `pending` | re-locked | - **unanimous × revise**: one approver's send-back finalizes the request @@ -158,7 +160,7 @@ them, under explicit constraints: `recall` on the *latest `returned`* request (the one normal recall precondition `pending` doesn't cover) flips it `returned → recalled` (the one sanctioned terminal→terminal transition) and audits `recall`. - The run is paused at the *wait node*, which has no `reject` out-edge to + The run is paused at the *revise window* node, which has no `reject` out-edge to resume down — so this lands the engine's first **run-cancel primitive**: `cancelRun(runId, reason)` consumes the continuation and records a terminal `cancelled` log (`ExecutionStatus` already reserves the value). @@ -269,6 +271,13 @@ no new machinery. The real axis was never visibility-vs-enforcement; it was **reuse-vs-a-new-type.** ADR-0044 chose reuse (no new node type), and reuse is what seated a generic node in a privileged position. +**Ruling (2026-08-05).** The maintainer approved this reversal. The criterion set +for the implementation was that the revise pause become visible to the existing +#3801 `resumeAuthority` type gate **with zero new machinery**; the owner-claim +alternative (a per-suspension capability) was rejected outright, its +screen-inheritance hazard being part of why. See *Implementation* below for what +shipped. + **Decision of record (the short-term fix).** - The `revise` edge targets a **dedicated service-owned pause** — a distinct node @@ -322,3 +331,64 @@ case that matters; the two deferred directions are how the platform would generalise if a third case appears. Refs #3801, #3853, #3879; security lineage in ADR-0019's #3801 / #3879 addenda. + +### Implementation (2026-08-05, #3823) + +Of the two equivalent shapes the amendment allowed, the **dedicated node type** +shipped: + +- **`approval_revise`** (`APPROVAL_REVISE_NODE_TYPE`, `spec/automation/approval.zod.ts`) + — registered by `plugin-approvals` alongside the `approval` node, one call site + so no deployment can hold half the feature. Its descriptor declares + `resumeAuthority: 'service'`, `supportsPause`, `isAsync`, `category: 'human'` + and **no `configSchema`**: the window is pure position in the graph, with no + signal and no timer, so nothing invents an authorable surface that has no + reader. Its executor suspends and arms nothing, hence no + `onSuspensionReleased` pairing (contrast the wait node's timer one-shot). +- **Nothing in the engine changed.** The #3801 gate keys on the suspended node's + registry type; a node type that declares service ownership is covered as-is. + That was the ruling's criterion and it held literally — the diff touches no + file in `service-automation`. +- **Two refusals, both prescriptive.** `ApprovalService.sendBack` refuses a + `revise` edge whose target is not `approval_revise` **before any mutation** + (alongside the existing missing-edge check), so a run can never be parked in a + window something else can advance; and + `flow-approval-revise-target-not-service-owned` (`@objectstack/lint`, + severity `error`) rejects the shape at authoring time — `os build` / `os + validate` / `os lint` and the runtime metadata publish gate, via the already-wired + `lintFlowPatterns` entry. It qualifies for `error` under that module's stated + bar ("the runtime refuses"), which is why the deliberately-narrow lint promotion + needed no new rule wiring either. + +**Why the approval node does not re-suspend itself.** The equivalent shape was +available and cheaper by one node type, but it would skip the author's `revise` +edge — every node on that branch (a `notify`, a status update) would stop running, +and the window would vanish from the canvas and the run log, which is the property +D3 chose the generic `wait` for. Only the *reuse* was wrong. + +**Backward compatibility, stated plainly.** A flow authored against the original +D3 (`revise` → a plain `wait`) keeps registering and running; its approvals stay +decidable (`approve` / `reject` / `recall` / `reassign` are untouched). What +changes is that its **send-back is refused** with a message naming the node and +the one-token fix (`type: 'wait'` → `type: 'approval_revise'`), and re-publishing +it reports the lint error. A run **already parked** in a legacy revise window +before the upgrade stays raw-resumable: `SuspendedRun.nodeType` is recorded at +pause time and read recorded-first on purpose, so a republish cannot re-type a +node under a live run — such a run is drained by `resubmit` or `recall` as usual. + +An ADR-0087 D2 conversion (silently rewriting a `revise`-target `wait` to the new +type at load) was considered and **rejected**: unlike the conversions in that +layer it would not be a lossless re-spelling but a topology-conditional semantic +rewrite, and it would silently drop a timer-flavoured wait's timer or make a wait +shared by another in-edge service-only for that path too — breakage a conversion +cannot see. The measured population argues the same way: the Studio designer +cannot author revise edges yet (this ADR's own follow-up), the `cloud` repo has no +revise flow, and this repo's single one is the showcase, migrated in the same PR. +A loud refusal with a one-token fix beats a tolerance layer that would have to be +retired later — and beats it most for AI authors, who read the diagnostic. + +**Narrowing worth knowing:** the `revise` edge's **immediate** target must be the +window. A graph that wanted `revise → notify → window` is refused rather than +analysed for "every pause reachable on this branch is service-owned", which is +unbounded. Send-back already notifies the submitter itself, so the pattern has no +lost capability behind it. diff --git a/examples/app-showcase/src/automation/flows/index.ts b/examples/app-showcase/src/automation/flows/index.ts index 13bf326f8e..5d496881e8 100644 --- a/examples/app-showcase/src/automation/flows/index.ts +++ b/examples/app-showcase/src/automation/flows/index.ts @@ -232,13 +232,18 @@ export const BudgetApprovalFlow = defineFlow({ { // ADR-0044 revise window: the run parks here while the submitter reworks // the (now unlocked) record; their resubmit resumes it over the back-edge. + // + // `approval_revise`, not `wait` — the shape D3 originally prescribed and + // its 2026-07-28 amendment reversed (#3823). This pause is service-owned: + // `POST /api/v1/approvals/requests/:id/resubmit` is the only thing that + // may end it (submitter-only, audited, refusing a colliding pending + // request), and the descriptor says so with `resumeAuthority: 'service'` + // so the generic run-resume route refuses it. A `wait` here was + // raw-resumable by anyone holding the run id — hence no `waitEventConfig` + // either: the window has no signal to wait on. id: 'wait_revision', - type: 'wait', + type: 'approval_revise', label: 'Awaiting Revision', - // `waitEventConfig`, not a loose `config` — the latter is the undeclared - // back door retired in #4045. The conversion layer still rewrites it at - // load, but the showcase should demonstrate the declared spelling. - waitEventConfig: { eventType: 'signal', signalName: 'budget_revision' }, }, // A plain exclusive gateway: the predicate is on the out-edges (e4/e5). // It also carried `config.condition` — inert on every node but `start`, and @@ -275,8 +280,8 @@ export const BudgetApprovalFlow = defineFlow({ { id: 'e6', source: 'exec_review', target: 'approved', label: 'approve' }, { id: 'e7', source: 'exec_review', target: 'rejected', label: 'reject' }, // ADR-0044 send-back-for-revision loop on the manager step: revise walks - // to the wait node; the resubmit edge is the declared back-edge closing - // the cycle (type 'back' — excluded from DAG validation, traversed + // to the revise-window node; the resubmit edge is the declared back-edge + // closing the cycle (type 'back' — excluded from DAG validation, traversed // normally), re-entering the approval node as round 2. { id: 'e8', source: 'manager_review', target: 'wait_revision', label: 'revise' }, { id: 'e9', source: 'wait_revision', target: 'manager_review', label: 'resubmit', type: 'back' }, diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index bba3234e39..f00a28077b 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -459,6 +459,7 @@ export { FLOW_APPROVAL_REVISE_DEAD_END, FLOW_APPROVAL_REVISE_UNMARKED_BACKEDGE, FLOW_APPROVAL_REVISE_DISABLED, + FLOW_APPROVAL_REVISE_TARGET_NOT_SERVICE_OWNED, FLOW_RUNAS_UNSCOPED, FLOW_ERROR_LABEL_NOT_FAULT, FLOW_BRANCH_LABEL_UNMATCHED, diff --git a/packages/lint/src/lint-flow-patterns.test.ts b/packages/lint/src/lint-flow-patterns.test.ts index 56c58a8556..dfdab74a4b 100644 --- a/packages/lint/src/lint-flow-patterns.test.ts +++ b/packages/lint/src/lint-flow-patterns.test.ts @@ -12,6 +12,7 @@ import { FLOW_APPROVAL_REVISE_DEAD_END, FLOW_APPROVAL_REVISE_UNMARKED_BACKEDGE, FLOW_APPROVAL_REVISE_DISABLED, + FLOW_APPROVAL_REVISE_TARGET_NOT_SERVICE_OWNED, FLOW_RUNAS_UNSCOPED, FLOW_ERROR_LABEL_NOT_FAULT, FLOW_BRANCH_LABEL_UNMATCHED, @@ -216,13 +217,26 @@ describe('lintFlowPatterns — approval revise loop (ADR-0044)', () => { nodes: [ { id: 'start', type: 'start', config: { triggerType: 'manual' } }, { id: 'mgr', type: 'approval', config: approvalConfig }, - { id: 'wait', type: 'wait', config: { eventType: 'signal' } }, + // #3823 — the revise window is the service-owned `approval_revise` + // node, not the bare `wait` ADR-0044 D3 originally prescribed. The + // dedicated case below pins that the old shape is now an error. + { id: 'wait', type: 'approval_revise' }, { id: 'ok', type: 'end' }, { id: 'no', type: 'end' }, ], edges, }], }); + /** The same flow with the revise edge pointed at a plain `wait` (pre-#3823). */ + const legacyWaitFlow = ( + edges: Array<{ source: string; target: string; label?: string; type?: string }>, + ) => { + const stack = approvalFlow(edges) as any; + stack.flows[0].nodes = stack.flows[0].nodes.map((n: any) => + n.id === 'wait' ? { id: 'wait', type: 'wait', config: { eventType: 'signal' } } : n, + ); + return stack; + }; const declaredLoop = [ { source: 'start', target: 'mgr' }, { source: 'mgr', target: 'ok', label: 'approve' }, @@ -271,6 +285,32 @@ describe('lintFlowPatterns — approval revise loop (ADR-0044)', () => { ]; expect(lintFlowPatterns(approvalFlow(edges))).toEqual([]); }); + + // #3823 — the shape ADR-0044 D3 prescribed until its 2026-07-28 amendment. + // `error`, because `ApprovalService.sendBack` refuses this metadata outright: + // the revise branch can never run, and before that refusal the pause it + // produced was resumable by anyone holding the run id. + it('flags a revise edge into a bare wait node as an ERROR', () => { + const fnds = lintFlowPatterns(legacyWaitFlow(declaredLoop)); + expect(fnds).toHaveLength(1); + expect(fnds[0]).toMatchObject({ + rule: FLOW_APPROVAL_REVISE_TARGET_NOT_SERVICE_OWNED, + severity: 'error', + }); + expect(fnds[0].where).toContain('mgr'); + expect(fnds[0].message).toMatch(/node 'wait' of type 'wait'/); + expect(fnds[0].hint).toMatch(/type: 'approval_revise'/); + }); + + it('flags a revise edge into any other node type too (screen, not just wait)', () => { + const stack = approvalFlow(declaredLoop) as any; + stack.flows[0].nodes = stack.flows[0].nodes.map((n: any) => + n.id === 'wait' ? { id: 'wait', type: 'screen', config: { fields: [{ name: 'note', type: 'text' }] } } : n, + ); + const fnds = lintFlowPatterns(stack); + expect(fnds.map((f) => f.rule)).toEqual([FLOW_APPROVAL_REVISE_TARGET_NOT_SERVICE_OWNED]); + expect(fnds[0].message).toMatch(/type 'screen'/); + }); }); describe('lintFlowPatterns — user-less runAs unscoped (#1888 / ADR-0049 / ADR-0073 D5 / #3760)', () => { diff --git a/packages/lint/src/lint-flow-patterns.ts b/packages/lint/src/lint-flow-patterns.ts index 3715c7ee13..58b10ffcd0 100644 --- a/packages/lint/src/lint-flow-patterns.ts +++ b/packages/lint/src/lint-flow-patterns.ts @@ -15,6 +15,11 @@ * - **The runtime refuses.** {@link FLOW_RUNAS_UNSCOPED} — a user-less trigger * with `runAs:'user'` has no identity to scope to, so the data operation is * refused outright (#3760). + * {@link FLOW_APPROVAL_REVISE_TARGET_NOT_SERVICE_OWNED} — a `revise` edge + * into anything but the service-owned revise window is refused by + * `ApprovalService.sendBack` before it mutates anything, so the branch can + * never run; and the shape it replaces was a pause anyone holding the run id + * could resume (#3823, amended ADR-0044). * - **The declaration is inert and the route silently differs from what is * written.** {@link FLOW_BRANCH_LABEL_UNMATCHED} — a decision computes a * branch no out-edge carries, so the branch is discarded and every out-edge @@ -40,6 +45,8 @@ * block of the correct pattern), keeping false positives near zero. */ +import { APPROVAL_NODE_TYPE, APPROVAL_REVISE_NODE_TYPE } from '@objectstack/spec/automation'; + export interface FlowLintFinding { where: string; message: string; @@ -90,6 +97,12 @@ export const FLOW_BARE_DOLLAR_REF = 'flow-bare-dollar-reference'; export const FLOW_APPROVAL_REVISE_DEAD_END = 'flow-approval-revise-dead-end'; export const FLOW_APPROVAL_REVISE_UNMARKED_BACKEDGE = 'flow-approval-revise-unmarked-backedge'; export const FLOW_APPROVAL_REVISE_DISABLED = 'flow-approval-revise-disabled'; +/** + * #3823 — the `revise` edge targets a node that is not the service-owned revise + * window. `error`: `ApprovalService.sendBack` refuses this metadata outright + * (see {@link scanApprovalReviseLoops}). + */ +export const FLOW_APPROVAL_REVISE_TARGET_NOT_SERVICE_OWNED = 'flow-approval-revise-target-not-service-owned'; /** * #3760 — renamed from `flow-schedule-runas-unscoped`. The old id named the * *schedule*, which was never the boundary: the rule is about a trigger that @@ -292,13 +305,18 @@ function edgeLabelOf(e: AnyRec): string { /** * ADR-0044 send-back-for-revision footguns on an approval node that declares a - * `revise` out-edge — the two shapes an AI authoring an approval flow gets wrong: + * `revise` out-edge — the shapes an AI authoring an approval flow gets wrong: * - the revise branch never loops back to the approval (the submitter reworks * the record with nowhere to resubmit). This is a VALID DAG, so `registerFlow` * ACCEPTS it — the linter is the only place that catches the dead end. * - the loop DOES return to the approval, but the closing edge isn't declared * `type: 'back'`, so `registerFlow` rejects it as an un-declared cycle. The * lint fires at compile time with the specific fix (mark the resubmit edge). + * - the revise edge targets a plain `wait` — the shape ADR-0044 D3 originally + * prescribed, reversed by its 2026-07-28 amendment (#3823). An AI author + * following the old text generates it verbatim, which is exactly why this one + * is an `error` rather than a warning: nothing in the metadata itself said the + * node sat in a privileged position. */ /** * #3863 — flag edges labelled like an error path but left at the default type. @@ -538,9 +556,14 @@ function scanApprovalReviseLoops( edges: AnyRec[], findings: FlowLintFinding[], ): void { - const approvals = nodes.filter((n) => n.type === 'approval'); + const approvals = nodes.filter((n) => n.type === APPROVAL_NODE_TYPE); if (approvals.length === 0) return; const nodeIds = new Set(nodes.map((n) => (typeof n.id === 'string' ? n.id : '')).filter(Boolean)); + const nodeTypeById = new Map( + nodes + .filter((n) => typeof n.id === 'string') + .map((n) => [n.id as string, typeof n.type === 'string' ? n.type : '']), + ); const outEdges = new Map(); for (const e of edges) { const src = typeof e.source === 'string' ? e.source : ''; @@ -559,6 +582,34 @@ function scanApprovalReviseLoops( if (reviseTargets.length === 0) continue; // only approvals that declare a revise branch const where = `flow '${flowName}' \u00b7 approval '${aid}'`; + // #3823 / amended ADR-0044 \u2014 the revise window must be the service-owned + // pause. `error`, under this module's stated bar ("the runtime refuses"): + // `ApprovalService.sendBack` refuses any other target before it mutates + // anything, so on this metadata send-back cannot run at all. Before that + // refusal existed the shape was worse than dead \u2014 the pause landed on a + // node anyone holding the run id could resume, walking the resubmit + // back-edge with no submitter check and no audit row. + for (const target of reviseTargets) { + const targetType = nodeTypeById.get(target) ?? ''; + if (targetType === APPROVAL_REVISE_NODE_TYPE) continue; + findings.push({ + where, + severity: 'error', + message: + `has a 'revise' out-edge into node '${target}' of type '${targetType || '(untyped)'}' \u2014 the revise ` + + `window must be an '${APPROVAL_REVISE_NODE_TYPE}' node. Send-back parks the run there while the ` + + `record is unlocked, and only the approvals service may continue it (submitter-only, audited, and ` + + `refusing a colliding pending request); \`sendBack\` refuses any other target, so this flow's ` + + `revise branch cannot run.`, + hint: + `Set node '${target}' to \`type: '${APPROVAL_REVISE_NODE_TYPE}'\` (drop any \`waitEventConfig\` \u2014 the ` + + `window is ended by POST /api/v1/approvals/requests/:id/resubmit, not by a signal). ADR-0044 D3 ` + + `originally said 'wait' here; its 2026-07-28 amendment reversed that, because a 'wait' is ` + + `resumable by anyone with the run id (#3823, #3801).`, + rule: FLOW_APPROVAL_REVISE_TARGET_NOT_SERVICE_OWNED, + }); + } + // maxRevisions:0 alongside a revise edge is self-contradictory — send-back is // disabled, so the branch always auto-rejects and never actually runs. const cfg = (a.config ?? {}) as AnyRec; @@ -599,8 +650,9 @@ function scanApprovalReviseLoops( `has a 'revise' out-edge but no path loops back to it — the submitter reworks the record with ` + `nowhere to resubmit, so the revise branch dead-ends. (registerFlow accepts this — it's a valid DAG.)`, hint: - `Close the loop: the 'revise' edge should reach a wait node whose resubmit edge returns to ` + - `'${aid}' marked \`type: 'back'\` (ADR-0044). See examples/app-showcase showcase_budget_approval.`, + `Close the loop: the 'revise' edge should reach an '${APPROVAL_REVISE_NODE_TYPE}' node whose resubmit ` + + `edge returns to '${aid}' marked \`type: 'back'\` (ADR-0044). See examples/app-showcase ` + + `showcase_budget_approval.`, rule: FLOW_APPROVAL_REVISE_DEAD_END, }); } else if (!returnEdges.some((e) => e.type === 'back')) { diff --git a/packages/plugins/plugin-approvals/src/approval-actor-impersonation.test.ts b/packages/plugins/plugin-approvals/src/approval-actor-impersonation.test.ts index 0406a937df..26d2a834dc 100644 --- a/packages/plugins/plugin-approvals/src/approval-actor-impersonation.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-actor-impersonation.test.ts @@ -27,6 +27,7 @@ */ import { describe, it, expect, beforeEach } from 'vitest'; +import { APPROVAL_REVISE_NODE_TYPE } from '@objectstack/spec/automation'; import { ApprovalService, SLA_ACTOR_ID } from './approval-service.js'; interface FakeRow { [k: string]: any } @@ -144,7 +145,12 @@ function makeAutomationStub() { async getFlow() { return { name: 'deal_approval', - nodes: [{ id: 'approve_step', type: 'approval' }, { id: 'wait_revision', type: 'wait' }], + // #3823 — the revise window is the service-owned node type; `sendBack` + // refuses a bare `wait` target, so this stub declares the real shape. + nodes: [ + { id: 'approve_step', type: 'approval' }, + { id: 'wait_revision', type: APPROVAL_REVISE_NODE_TYPE }, + ], edges: [ { id: 'e1', source: 'approve_step', target: 'ok', label: 'approve' }, { id: 'e2', source: 'approve_step', target: 'no', label: 'reject' }, diff --git a/packages/plugins/plugin-approvals/src/approval-node-degradation.test.ts b/packages/plugins/plugin-approvals/src/approval-node-degradation.test.ts index 792b1d5d53..98f97a2007 100644 --- a/packages/plugins/plugin-approvals/src/approval-node-degradation.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-node-degradation.test.ts @@ -17,7 +17,7 @@ import { describe, it, expect } from 'vitest'; import { ApprovalsServicePlugin } from './approvals-plugin.js'; -import { APPROVAL_NODE_TYPE } from '@objectstack/spec/automation'; +import { APPROVAL_NODE_TYPE, APPROVAL_REVISE_NODE_TYPE } from '@objectstack/spec/automation'; /** Minimal ObjectQL stand-in — enough for start() to build the service. */ function fakeObjectql() { @@ -72,7 +72,7 @@ describe('ApprovalsServicePlugin — missing automation engine is reported at wa expect(logs.warn.filter((m) => m.includes('no automation engine'))).toHaveLength(1); }); - it('registers the `approval` executor and says nothing when the engine is present', async () => { + it('registers the `approval` + revise-window executors and says nothing when the engine is present', async () => { const registered: string[] = []; const automation = { registerNodeExecutor: (e: { type: string }) => registered.push(e.type), @@ -81,7 +81,10 @@ describe('ApprovalsServicePlugin — missing automation engine is reported at wa const { ctx, logs } = makeCtx({ objectql: fakeObjectql(), automation }); await new ApprovalsServicePlugin({ disableAutoHooks: true }).start(ctx); - expect(registered).toEqual([APPROVAL_NODE_TYPE]); + // #3823 — the service-owned revise window registers with the approval node, + // never separately: a deployment holding one without the other would accept + // approvals whose send-back can never run. + expect(registered).toEqual([APPROVAL_REVISE_NODE_TYPE, APPROVAL_NODE_TYPE]); expect(logs.warn.filter((m) => m.includes('no automation engine'))).toEqual([]); }); }); diff --git a/packages/plugins/plugin-approvals/src/approval-node.ts b/packages/plugins/plugin-approvals/src/approval-node.ts index 728fed6c71..5a0786a7be 100644 --- a/packages/plugins/plugin-approvals/src/approval-node.ts +++ b/packages/plugins/plugin-approvals/src/approval-node.ts @@ -26,6 +26,7 @@ import { } from '@objectstack/spec/automation'; import type { SharingExecutionContext } from '@objectstack/spec/contracts'; import type { ApprovalService } from './approval-service.js'; +import { registerApprovalReviseNode } from './approval-revise-node.js'; /** Minimal surface of the automation engine this provider depends on. */ export interface ApprovalAutomationSurface { @@ -80,15 +81,23 @@ function nestVariables(variables: Map): Record } /** - * Register the `approval` node executor on the automation engine. Idempotent at - * the engine level (re-registering replaces). Safe to skip when no automation + * Register the `approval` node executor on the automation engine, plus the + * `approval_revise` window the ADR-0044 send-back parks on. Idempotent at the + * engine level (re-registering replaces). Safe to skip when no automation * service is present. + * + * The two register together deliberately (#3823): send-back resumes the run + * down the `revise` edge onto the revise-window node, and `sendBack` refuses a + * flow whose revise edge targets anything else — so an engine holding the + * `approval` node without the revise window would accept approvals whose + * send-back can never run. */ export function registerApprovalNode( automation: ApprovalAutomationSurface, service: ApprovalService, logger?: MinimalLogger, ): void { + registerApprovalReviseNode(automation, logger); automation.registerNodeExecutor({ type: APPROVAL_NODE_TYPE, descriptor: defineActionDescriptor({ diff --git a/packages/plugins/plugin-approvals/src/approval-revise-node.ts b/packages/plugins/plugin-approvals/src/approval-revise-node.ts new file mode 100644 index 0000000000..91302ad87f --- /dev/null +++ b/packages/plugins/plugin-approvals/src/approval-revise-node.ts @@ -0,0 +1,104 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The ADR-0044 **revise window** as its own flow node (amended ADR-0044, #3823). + * + * A send-back resumes the run down the approval node's `revise` edge; the run + * parks on the node that edge targets while the record is unlocked and the + * submitter reworks it, and `ApprovalService.resubmit` drives it back into the + * approval node over the declared back-edge (round N+1). + * + * That pause is **service-owned**: `resubmit` is the thing that authorizes + * (submitter-only), orders (latest request for the run) and records (a + * `resubmit` audit row) the continuation, and refuses up front when another + * request is pending on the record — the guard that keeps the run alive, + * because the approval node's re-entry would otherwise fail *after* the engine + * consumed the suspension and leave the run terminally dead. + * + * ADR-0044 D3 originally parked it on an ordinary `wait` node, which is + * `resumeAuthority: 'any'` — correctly so for a signal wait, and exactly wrong + * here: a raw `POST /automation/:name/runs/:runId/resume` (empty body suffices) + * walked the back-edge around every one of those checks. The #3801 resume gate + * keys on the node type that produced the suspension, so the fix is a node type + * that declares itself service-owned. Nothing about the gate changes. + * + * Why a dedicated type rather than re-suspending inside the approval node: the + * revise window stays *visible flow state* — a box on the canvas, a step in the + * run log, a row in the suspended-run store — which is the property ADR-0044 + * chose the generic `wait` for in the first place. Only the reuse was wrong. + */ + +import { + defineActionDescriptor, + APPROVAL_REVISE_NODE_TYPE, +} from '@objectstack/spec/automation'; +import type { ApprovalAutomationSurface } from './approval-node.js'; + +interface MinimalLogger { + info?: (msg: any, ...rest: any[]) => void; + warn?: (msg: any, ...rest: any[]) => void; +} + +/** + * Correlation prefix for a revise-window suspension — what the suspended-run + * record carries so an operator (or `listSuspendedRuns`) can tell a revise + * window from a timer or signal wait at a glance. + * + * Deliberately NOT one of the engine's linked-run prefixes (`subflow:` / + * `map:`): those mean "this run is waiting on another run", and the resume gate + * follows them to judge the child's node. A revise window waits on a person. + */ +export const APPROVAL_REVISE_CORRELATION_PREFIX = 'approval_revise:'; + +/** + * Register the `approval_revise` node executor. Called from + * {@link registerApprovalNode} rather than wired separately on purpose: a + * deployment that has the `approval` node must have this one too, or every + * send-back in it is refused at runtime — there is no half of this feature + * worth installing. + */ +export function registerApprovalReviseNode( + automation: ApprovalAutomationSurface, + logger?: MinimalLogger, +): void { + automation.registerNodeExecutor({ + type: APPROVAL_REVISE_NODE_TYPE, + descriptor: defineActionDescriptor({ + type: APPROVAL_REVISE_NODE_TYPE, + version: '1.0.0', + name: 'Revise Window', + description: + 'Durable pause an approval send-back parks the run on while the submitter reworks the ' + + 'record. Continues only through the approvals service (resubmit), which re-enters the ' + + 'approval node over the declared back-edge.', + icon: 'pencil', + // Waits on a human — the same category as `approval` and `screen`. + category: 'human', + paradigms: ['flow'], + source: 'plugin', + supportsPause: true, + isAsync: true, + // #3823 / amended ADR-0044: THE point of this node type. The revise + // window is a service-owned continuation, so the #3801 gate must refuse + // a raw resume of it — which it does for any node type declaring this. + resumeAuthority: 'service', + // No config: the window is pure position in the graph. Left schemaless + // (like `wait` / `subflow` / `decision`) rather than declaring an empty + // object, so nothing invents an authorable surface that has no reader. + }), + async execute(node) { + // Arms nothing on entry — no timer, no job, no external subscription — so + // there is deliberately no `onSuspensionReleased` hook to pair with it + // (contrast the wait node's timer one-shot, #5529). The only thing that + // ends this pause is `ApprovalService.resubmit` (or a `recall`, which + // cancels the run). + return { + success: true, + suspend: true, + correlation: `${APPROVAL_REVISE_CORRELATION_PREFIX}${node.id}`, + }; + }, + }); + + logger?.info?.('[approvals] approval revise-window node executor registered'); +} diff --git a/packages/plugins/plugin-approvals/src/approval-revise.test.ts b/packages/plugins/plugin-approvals/src/approval-revise.test.ts index 8e0feaa02a..ff412b89dc 100644 --- a/packages/plugins/plugin-approvals/src/approval-revise.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-revise.test.ts @@ -14,7 +14,8 @@ */ import { describe, it, expect, beforeEach } from 'vitest'; -import { AutomationEngine } from '@objectstack/service-automation'; +import { AutomationEngine, registerScreenNodes } from '@objectstack/service-automation'; +import { APPROVAL_REVISE_NODE_TYPE } from '@objectstack/spec/automation'; import { ApprovalService } from './approval-service.js'; import { registerApprovalNode } from './approval-node.js'; import { bindApprovalLockHook, APPROVALS_HOOK_PACKAGE } from './lifecycle-hooks.js'; @@ -98,11 +99,17 @@ describe('Send back for revision (ADR-0044)', () => { async execute(node: any) { marks.push(node.id); return { success: true }; }, }); // Signal-flavor wait stand-in: suspends until an external resume — the - // same contract as the built-in wait node's non-timer path. + // same contract as the built-in wait node's non-timer path. Still here + // because the LEGACY revise shape (#3823) is a wait, and one case below + // pins that send-back now refuses it. automation.registerNodeExecutor({ type: 'wait', async execute(node: any) { return { success: true, suspend: true, correlation: `wait:${node.id}` }; }, }); + // The real screen executor, for the "approve → screen stays UI-resumable" + // case: the revise window's service ownership must not leak onto other + // pauses downstream of an approval. + registerScreenNodes(automation, { logger: noopLogger } as any); }); function registerReviseFlow(opts?: { @@ -124,7 +131,9 @@ describe('Send back for revision (ADR-0044)', () => { ...(opts?.maxRevisions !== undefined ? { maxRevisions: opts.maxRevisions } : {}), }, }, - { id: 'wait_revision', type: 'wait', label: 'Awaiting Revision' }, + // The revise window is its own service-owned node type (#3823) — a + // plain `wait` here is raw-resumable and no longer accepted. + { id: 'wait_revision', type: APPROVAL_REVISE_NODE_TYPE, label: 'Awaiting Revision' }, { id: 'on_approved', type: 'mark', label: 'Approved' }, { id: 'on_rejected', type: 'mark', label: 'Rejected' }, { id: 'end', type: 'end', label: 'End' }, @@ -393,7 +402,7 @@ describe('Send back for revision (ADR-0044)', () => { id: 'review', type: 'approval', label: 'Review', config: { approvers: [{ type: 'user', value: 'u1' }], approvalStatusField: 'approval_status' }, }, - { id: 'wait_revision', type: 'wait', label: 'Awaiting Revision' }, + { id: 'wait_revision', type: APPROVAL_REVISE_NODE_TYPE, label: 'Awaiting Revision' }, { id: 'end', type: 'end', label: 'End' }, ], edges: [ @@ -415,4 +424,160 @@ describe('Send back for revision (ADR-0044)', () => { await service.resubmit(req.id, { actorId: 'submitter' }, asUser('submitter')); expect(await mirror()).toBe('pending'); // round 2 re-mirrors }); + + /** + * #3823 — the revise window is a SERVICE-OWNED pause. + * + * Demonstrated on `main` before this changed (issue comment, 2026-07-28): with + * the window parked on an ordinary `wait`, a raw `resume(runId)` — empty body, + * any caller — walked the resubmit back-edge into the approval node. Round 2 + * opened with no `resubmit` audit row and no submitter check; and when another + * request was pending on the record, the re-entry failed AFTER the engine had + * consumed the suspension, killing the run for good. + * + * Reverse verification: re-typing `wait_revision` back to `wait` in this + * describe's flow makes every case below fail — the raw resume succeeds, round + * 2 opens, and the collision case leaves zero suspended runs. The refusal is + * the node TYPE's doing, so the type is the only thing that has to change to + * see it. + */ + describe('the revise window is service-owned (#3823)', () => { + it('declares resumeAuthority: service on the revise-window node type', () => { + const descriptor = automation.getActionDescriptors().find(d => d.type === APPROVAL_REVISE_NODE_TYPE); + expect(descriptor).toMatchObject({ + type: APPROVAL_REVISE_NODE_TYPE, + resumeAuthority: 'service', + supportsPause: true, + isAsync: true, + }); + // The generic `wait` stays open to its external producer — this fix must + // not gate every author-placed wait in the system. + expect(automation.getActionDescriptors().find(d => d.type === 'wait')?.resumeAuthority) + .not.toBe('service'); + }); + + it('refuses a raw resume of a run parked in the revise window', async () => { + registerReviseFlow(); + const { runId, req } = await startFlow(); + await service.sendBack(req.id, { actorId: 'u1' }, asUser('u1')); + expect(automation.listSuspendedRuns()).toMatchObject([{ runId, nodeId: 'wait_revision' }]); + + // The demonstrated bypass, verbatim: empty body, no service marker. + const out = await automation.resume(runId); + expect(out).toMatchObject({ success: false, code: 'PERMISSION_DENIED' }); + expect(out.error).toMatch(/only its owning service may resume/); + + // Nothing moved: no round 2, no audit row, the window still open. + expect(await pendingReq()).toBeUndefined(); + expect(await actionsOf(req.id)).toEqual(['submit', 'revise']); + expect(automation.listSuspendedRuns()).toMatchObject([{ runId, nodeId: 'wait_revision' }]); + + // …and the legitimate door still works. + const re = await service.resubmit(req.id, { actorId: 'submitter' }, asUser('submitter')); + expect(re.resumed).toBe(true); + expect(await actionsOf(req.id)).toEqual(['submit', 'revise', 'resubmit']); + expect(automation.listSuspendedRuns()).toMatchObject([{ runId, nodeId: 'review' }]); + }); + + it('a raw resume can no longer destroy the run when a pending request collides', async () => { + registerReviseFlow(); + const { runId, req } = await startFlow(); + await service.sendBack(req.id, { actorId: 'u1' }, asUser('u1')); + + // A record-change trigger re-fired off an edit made inside the window. + await fake.insert('sys_approval_request', { + id: 'areq_collider', object_name: 'fin_expense', record_id: 'x1', + status: 'pending', flow_run_id: 'run_other', flow_node_id: 'review', + submitter_id: 'submitter', process_name: 'flow:expense_approval', + created_at: new Date().toISOString(), + }); + + // `resubmit` refuses BEFORE consuming the suspension (already pinned + // above); the raw resume used to go around that guard and consume it. + const out = await automation.resume(runId); + expect(out).toMatchObject({ success: false, code: 'PERMISSION_DENIED' }); + expect(automation.listSuspendedRuns().some(r => r.runId === runId)).toBe(true); + expect((await fake.find('sys_approval_request', { where: { id: req.id } }))[0].status).toBe('returned'); + + // Clear the collision and the approval is still resolvable — the run was + // never consumed, so the revise window is still there to resubmit from. + await fake.delete('sys_approval_request', { where: { id: 'areq_collider' } }); + const re = await service.resubmit(req.id, { actorId: 'submitter' }, asUser('submitter')); + expect(re.resumed).toBe(true); + expect(automation.listSuspendedRuns()).toMatchObject([{ runId, nodeId: 'review' }]); + }); + + it('refuses send-back into a bare wait node before anything mutates (legacy ADR-0044 D3 shape)', async () => { + automation.registerFlow('legacy_revise', { + name: 'legacy_revise', label: 'Legacy Revise', type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'review', type: 'approval', label: 'Review', config: { approvers: [{ type: 'user', value: 'u1' }] } }, + // The shape ADR-0044 D3 originally prescribed. + { id: 'wait_revision', type: 'wait', label: 'Awaiting Revision' }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'review' }, + { id: 'e2', source: 'review', target: 'end', label: 'approve' }, + { id: 'e3', source: 'review', target: 'end', label: 'reject' }, + { id: 'e4', source: 'review', target: 'wait_revision', label: 'revise' }, + { id: 'e5', source: 'wait_revision', target: 'review', label: 'resubmit', type: 'back' }, + ], + }); + const paused = await automation.execute('legacy_revise', { + object: 'fin_expense', record: { id: 'x7' }, userId: 'submitter', + }); + const req = await pendingReq(); + + await expect(service.sendBack(req.id, { actorId: 'u1' }, asUser('u1'))) + .rejects.toThrow(/revise window must be an 'approval_revise' node/); + + // Refused before any mutation: still pending, no `revise` audit row, and + // the run is still parked on the approval — so the request stays + // decidable (approve / reject) while the flow's metadata is fixed. + expect((await fake.find('sys_approval_request', { where: { id: req.id } }))[0].status).toBe('pending'); + expect(await actionsOf(req.id)).toEqual(['submit']); + expect(automation.listSuspendedRuns()).toMatchObject([{ runId: paused.runId, nodeId: 'review' }]); + }); + + it('leaves an approve-branch screen resumable through the generic route', async () => { + automation.registerFlow('approve_then_screen', { + name: 'approve_then_screen', label: 'Approve then Screen', type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'review', type: 'approval', label: 'Review', config: { approvers: [{ type: 'user', value: 'u1' }] } }, + { + id: 'collect', type: 'screen', label: 'Collect Shipping', + config: { title: 'Shipping', fields: [{ name: 'carrier', label: 'Carrier', type: 'text' }] }, + }, + { id: 'wait_revision', type: APPROVAL_REVISE_NODE_TYPE, label: 'Awaiting Revision' }, + { id: 'done', type: 'mark', label: 'Done' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'review' }, + { id: 'e2', source: 'review', target: 'collect', label: 'approve' }, + { id: 'e3', source: 'review', target: 'done', label: 'reject' }, + { id: 'e4', source: 'review', target: 'wait_revision', label: 'revise' }, + { id: 'e5', source: 'wait_revision', target: 'review', label: 'resubmit', type: 'back' }, + { id: 'e6', source: 'collect', target: 'done' }, + ], + }); + const paused = await automation.execute('approve_then_screen', { + object: 'fin_expense', record: { id: 'x8' }, userId: 'submitter', + }); + const req = await pendingReq(); + const out = await service.decide(req.id, { decision: 'approve', actorId: 'u1' }, SYSTEM_CTX); + expect(out).toMatchObject({ finalized: true, resumed: true }); + + // The run is now parked on the SCREEN, which the UI owns… + expect(automation.listSuspendedRuns()).toMatchObject([{ runId: paused.runId, nodeId: 'collect' }]); + // …and a plain resume — the flow runner's, no service marker — advances it. + const advanced = await automation.resume(paused.runId!, { variables: { carrier: 'DHL' } }); + expect(advanced.success).toBe(true); + expect(advanced.code).toBeUndefined(); + expect(marks).toEqual(['done']); + expect(automation.listSuspendedRuns()).toHaveLength(0); + }); + }); }); diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index f845124f5f..3fc272fdb7 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -10,6 +10,7 @@ */ import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { APPROVAL_REVISE_NODE_TYPE } from '@objectstack/spec/automation'; import { ApprovalService, REMIND_COOLDOWN_MS } from './approval-service.js'; import { bindApprovalLockHook, bindDelegationWriteGuard, unbindAllHooks } from './lifecycle-hooks.js'; @@ -3087,6 +3088,13 @@ describe('in-band transitions finalise before they resume (#3456 invariant)', () /** A flow whose approval node declares the `revise` out-edge send-back needs. */ const REVISE_FLOW = { name: 'deal_approval', + // The revise window must be the service-owned node type — `sendBack` + // checks the edge's TARGET as well as its existence (#3823), so the stub + // has to declare the node, not just the edge. + nodes: [ + { id: 'approve_step', type: 'approval' }, + { id: 'wait_revision', type: APPROVAL_REVISE_NODE_TYPE }, + ], edges: [{ id: 'e_rev', source: 'approve_step', target: 'wait_revision', label: 'revise' }], }; @@ -3202,6 +3210,13 @@ describe('status mirror identity (#3783)', () => { const REVISE_FLOW = { name: 'deal_approval', + // The revise window must be the service-owned node type — `sendBack` + // checks the edge's TARGET as well as its existence (#3823), so the stub + // has to declare the node, not just the edge. + nodes: [ + { id: 'approve_step', type: 'approval' }, + { id: 'wait_revision', type: APPROVAL_REVISE_NODE_TYPE }, + ], edges: [{ id: 'e_rev', source: 'approve_step', target: 'wait_revision', label: 'revise' }], }; diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index a154c4c869..ef32ae1f89 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -3,6 +3,7 @@ import { createHash, randomBytes } from 'node:crypto'; import { APPROVAL_BRANCH_LABELS, + APPROVAL_REVISE_NODE_TYPE, approverTypeIsOrgScoped, canonicalApproverType, normalizeDecisionOutputs, @@ -89,7 +90,7 @@ export interface ApprovalResumeSurface { getFlow?(name: string): Promise; /** * Terminally cancel a suspended run (ADR-0044). Used when a recall lands - * during a revision window — the run is paused at the revise wait node, + * during a revision window — the run is paused at the revise-window node, * which has no reject edge to resume down. */ cancelRun?(runId: string, reason?: string): Promise; @@ -2132,7 +2133,7 @@ export class ApprovalService implements IApprovalService { * * ADR-0044: also valid on the LATEST `returned` request of its run — the * submitter abandons the revision window instead of resubmitting. The run - * is then paused at the revise wait node (no reject edge), so it is + * is then paused at the revise-window node (no reject edge), so it is * terminally cancelled via {@link ApprovalResumeSurface.cancelRun} rather * than resumed. */ @@ -2193,7 +2194,7 @@ export class ApprovalService implements IApprovalService { let resumed = false; let resumeError: string | undefined; if (inReviseWindow) { - // ADR-0044: the run is paused at the revise wait node, which has no + // ADR-0044: the run is paused at the revise-window node, which has no // reject out-edge to resume down — terminally cancel it instead. if (runId) { resumeError = this.missingRunCapability(runId, requestId, 'the recall', 'cancelRun'); @@ -2236,7 +2237,7 @@ export class ApprovalService implements IApprovalService { * ADR-0044 send back for revision. Finalises the pending request as * `returned` (a third terminal state — approver-initiated rework, distinct * from submitter-initiated `recalled`) and resumes the owning flow run down - * its `revise` edge to a wait point: the record lock (keyed on `pending`) + * its `revise` edge to the revise window: the record lock (keyed on `pending`) * releases, the submitter reworks the data, then {@link resubmit}s. * * Requires the approval node to declare a `revise` out-edge — validated @@ -2263,7 +2264,7 @@ export class ApprovalService implements IApprovalService { const runId: string | null = raw.flow_run_id ?? null; await this.assertReviseEdge(raw, nodeId); - // A send-back exists to move the run to its revise wait point. If the run + // A send-back exists to move the run to its revise window. If the run // is gone there is nothing to send back TO, so refuse before writing — // same reasoning as decideNode's pre-flight (#4420). await this.assertRunResumable(runId, requestId); @@ -2381,7 +2382,7 @@ export class ApprovalService implements IApprovalService { /** * ADR-0044 resubmit after rework. Valid on the LATEST `returned` request of * its run, submitter-only. Audits `resubmit` on the returned (round-N) - * request and resumes the run from the revise wait node; traversal walks + * request and resumes the run from the revise-window node; traversal walks * the declared back-edge into the approval node, whose executor opens the * round-N+1 request — fresh approver slate, record re-locks. */ @@ -2458,6 +2459,21 @@ export class ApprovalService implements IApprovalService { * out-edge before send-back is allowed — the engine's branch-label fallback * (no matching label ⇒ ALL out-edges) must never be reachable from a user * action. + * + * Since #3823 it also checks WHAT that edge targets: the revise window must + * be an `approval_revise` node, the pause this service owns. ADR-0044 D3 + * pointed the edge at an ordinary `wait`, which is `resumeAuthority: 'any'`, + * so a raw engine resume walked the resubmit back-edge with no submitter + * check, no `resubmit` audit row, and — with a pending request colliding on + * the record — destroyed the run by consuming the suspension before the + * re-entry failed. Refused HERE, before any mutation, for the same reason the + * missing-edge check is: a run must never be parked in a revise window that + * something other than {@link resubmit} can advance. + * + * Also refused at authoring time — `flow-approval-revise-target-not-service-owned` + * in `@objectstack/lint` gates `os build` / `os validate` / `os lint` and the + * runtime metadata publish path — so a flow reaching this check at all is one + * published before that gate existed. */ private async assertReviseEdge(raw: any, nodeId: string | null): Promise { const processName = String(raw.process_name ?? ''); @@ -2466,14 +2482,34 @@ export class ApprovalService implements IApprovalService { throw new Error('VALIDATION_FAILED: send-back requires the owning flow definition (automation engine unavailable)'); } const flow: any = await this.automation.getFlow(flowName); - const hasRevise = Array.isArray(flow?.edges) - && flow.edges.some((e: any) => e?.source === nodeId && e?.label === APPROVAL_BRANCH_LABELS.revise); - if (!hasRevise) { + const reviseEdges = Array.isArray(flow?.edges) + ? flow.edges.filter((e: any) => e?.source === nodeId && e?.label === APPROVAL_BRANCH_LABELS.revise) + : []; + if (reviseEdges.length === 0) { throw new Error( `VALIDATION_FAILED: approval node '${nodeId}' has no '${APPROVAL_BRANCH_LABELS.revise}' out-edge — ` + 'the flow does not support send-back for revision', ); } + const nodeTypeById = new Map( + (Array.isArray(flow?.nodes) ? flow.nodes : []) + .filter((n: any) => typeof n?.id === 'string') + .map((n: any) => [n.id as string, typeof n.type === 'string' ? n.type : '']), + ); + for (const edge of reviseEdges) { + const target = typeof edge?.target === 'string' ? edge.target : ''; + const targetType = nodeTypeById.get(target); + if (targetType === APPROVAL_REVISE_NODE_TYPE) continue; + throw new Error( + `VALIDATION_FAILED: approval node '${nodeId}' has a '${APPROVAL_BRANCH_LABELS.revise}' out-edge into ` + + `node '${target || '(unknown)'}'` + + (targetType === undefined ? ' which the flow does not declare' : ` of type '${targetType || '(untyped)'}'`) + + `, but the revise window must be an '${APPROVAL_REVISE_NODE_TYPE}' node — that pause continues only ` + + 'through this service (submitter-only, audited, and refusing a colliding pending request), and any ' + + `other node type there is resumable by anyone with the run id (amended ADR-0044, #3823). Fix the flow: ` + + `set node '${target || ''}' to type '${APPROVAL_REVISE_NODE_TYPE}'.`, + ); + } } /** diff --git a/packages/plugins/plugin-approvals/src/index.ts b/packages/plugins/plugin-approvals/src/index.ts index bfc7d9a9ec..5790c2ee3e 100644 --- a/packages/plugins/plugin-approvals/src/index.ts +++ b/packages/plugins/plugin-approvals/src/index.ts @@ -34,6 +34,13 @@ export { registerApprovalNode, type ApprovalAutomationSurface, } from './approval-node.js'; +// #3823 — the service-owned revise window. Registered by `registerApprovalNode` +// (the two are one feature); exported for hosts that compose node executors by +// hand and for tests that drive the send-back path. +export { + registerApprovalReviseNode, + APPROVAL_REVISE_CORRELATION_PREFIX, +} from './approval-revise-node.js'; export type { IApprovalService, ApprovalRequestRow, diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index cbb28aa677..0b801ceadb 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -2039,6 +2039,7 @@ "./automation": [ "APPROVAL_BRANCH_LABELS (const)", "APPROVAL_NODE_TYPE (const)", + "APPROVAL_REVISE_NODE_TYPE (const)", "APPROVER_EXPRESSION_ROOTS (const)", "APPROVER_ORG_SCOPED (const)", "APPROVER_ORG_SYMBOLS (const)", diff --git a/packages/spec/src/automation/approval.zod.ts b/packages/spec/src/automation/approval.zod.ts index d2fbc34aa4..f2197162e3 100644 --- a/packages/spec/src/automation/approval.zod.ts +++ b/packages/spec/src/automation/approval.zod.ts @@ -290,6 +290,35 @@ export function approverTypeIsOrgScoped(type: string): boolean { */ export const APPROVAL_NODE_TYPE = 'approval' as const; +/** + * Registry node type for the **revise window** — the durable pause an ADR-0044 + * send-back parks the run on while the submitter reworks the record. + * + * ADR-0044 D3 originally pointed the `revise` edge at an ordinary `wait` node + * and reused the pause that had already shipped for timers and signals. The + * 2026-07-28 amendment to that ADR (#3823) reversed it: once #3801 made + * `resume` authorization-bearing (a node descriptor declares who may continue + * a pause it produced), a generic `wait` — correctly `resumeAuthority: 'any'`, + * because a signal-flavored wait is *meant* to be resumable by an external + * producer — sat in a service-owned position. A raw + * `POST /automation/:name/runs/:runId/resume` walked the resubmit back-edge + * into the approval node with no submitter check, no `resubmit` audit row, and + * (when a pending request collided on the record) destroyed the run outright by + * consuming the suspension before the re-entry failed. + * + * So the revise pause is its own node type, registered by `plugin-approvals` + * with `resumeAuthority: 'service'`: still a first-class box on the canvas and + * in the run log, no longer raw-resumable. `ApprovalService.resubmit` is the + * only door — which is what keeps the submitter-only check, the latest-request + * check, the `resubmit` audit row and the `DUPLICATE_REQUEST` run-preservation + * guard on the only path that can advance it. + * + * A `revise` edge that targets anything else is rejected at authoring time + * (`flow-approval-revise-target-not-service-owned` in `@objectstack/lint`) and + * refused by `sendBack` before any mutation. + */ +export const APPROVAL_REVISE_NODE_TYPE = 'approval_revise' as const; + /** * Canonical decisions an Approval node emits. The engine selects the * downstream branch by matching these against out-edge `label`s @@ -308,14 +337,18 @@ export const APPROVAL_BRANCH_LABELS = { reject: 'reject', /** * ADR-0044 send-back-for-revision: the request finalizes `returned` and the - * flow walks this edge to a wait point where the submitter reworks the + * flow walks this edge to the revise window — an + * {@link APPROVAL_REVISE_NODE_TYPE} node — where the submitter reworks the * record; a later resubmit re-enters the approval node via a declared * back-edge (round N+1). + * + * The edge's target must be that node type (amended ADR-0044, #3823): a + * generic `wait` there is a service-owned pause anyone could resume. */ revise: 'revise', /** - * ADR-0044: informational label a resubmit resume passes so the wait node's - * out-edge selection is explicit when authors label the back-edge. + * ADR-0044: informational label a resubmit resume passes so the revise + * window's out-edge selection is explicit when authors label the back-edge. */ resubmit: 'resubmit', } as const; diff --git a/skills/objectstack-automation/SKILL.md b/skills/objectstack-automation/SKILL.md index d008ff0979..f50d570638 100644 --- a/skills/objectstack-automation/SKILL.md +++ b/skills/objectstack-automation/SKILL.md @@ -375,26 +375,34 @@ is one diagram a reviewer (or AI) can read end-to-end. Approval centers also model **send back for revision** (退回修改) — distinct from `reject` (terminate) and from a comment thread (which keeps the request pending). Send-back is a **flow movement**: the request finalizes as `returned`, the run -walks a **`revise`** out-edge to a `wait` node where the record unlocks and the -submitter reworks it, and an explicit *resubmit* re-enters the approval node over -a **declared back-edge**, opening round N+1 with a fresh approver slate. +walks a **`revise`** out-edge to an **`approval_revise`** node (the *revise +window*) where the record unlocks and the submitter reworks it, and an explicit +*resubmit* re-enters the approval node over a **declared back-edge**, opening +round N+1 with a fresh approver slate. ``` approval ──approve──▶ … ──reject───▶ … - ──revise───▶ wait (signal; record unlocked, submitter edits) + ──revise───▶ approval_revise (record unlocked, submitter edits) └──resubmit──[type:'back']──▶ approval (round N+1) ``` Three pieces author it: 1. **`revise` out-edge** — a third branch label alongside `approve` / `reject`, - targeting an ordinary `wait` node (signal flavour). -2. **`type: 'back'` resubmit edge** — the edge from the wait node back into the - approval node MUST be typed `'back'`. This is the *only* thing that legalizes - the cycle: `registerFlow` validates the graph **minus `back` edges** as a DAG, - so an **unmarked** cycle is rejected — you opt in, edge by edge. At run time a - back-edge traverses normally (it just re-enters the node). + targeting an **`approval_revise`** node. It must be that node type: the window + is a *service-owned* pause (`resumeAuthority: 'service'`), so only + `POST /api/v1/approvals/requests/:id/resubmit` can end it. ADR-0044 D3 first + prescribed an ordinary `wait` here and its **2026-07-28 amendment reversed + that** (#3823) — a `wait` is `resumeAuthority: 'any'`, so a raw + `POST /api/v1/automation/:name/runs/:runId/resume` walked the back-edge with no + submitter check and no audit row, and could destroy the run outright. The + `approval_revise` node takes **no config** — there is no signal to wait on. +2. **`type: 'back'` resubmit edge** — the edge from the revise window back into + the approval node MUST be typed `'back'`. This is the *only* thing that + legalizes the cycle: `registerFlow` validates the graph **minus `back` edges** + as a DAG, so an **unmarked** cycle is rejected — you opt in, edge by edge. At + run time a back-edge traverses normally (it just re-enters the node). 3. **`maxRevisions`** on the approval `config` (default `3`) — the budget of send-backs per run; exceeding it **auto-rejects** (resumes down the `reject` edge). `maxRevisions: 0` disables send-back, so never pair `0` with a `revise` @@ -405,21 +413,21 @@ Three pieces author it: id: 'manager_review', type: 'approval', label: 'Manager Review', config: { approvers: [{ type: 'position', value: 'manager' }], lockRecord: true, maxRevisions: 2 }, }, -// The signal keys may also live in the spec-canonical node-level -// `waitEventConfig` block (FlowNodeSchema); the wait executor reads -// `waitEventConfig` first and falls back to these loose `config` keys. -{ id: 'wait_revision', type: 'wait', label: 'Awaiting Revision', - config: { eventType: 'signal', signalName: 'budget_revision' } }, +// No config and no `waitEventConfig`: the window ends on the submitter's +// explicit resubmit, not on a signal or a timer. +{ id: 'wait_revision', type: 'approval_revise', label: 'Awaiting Revision' }, // …among the approval's edges… { id: 'rev', source: 'manager_review', target: 'wait_revision', label: 'revise' }, { id: 'back', source: 'wait_revision', target: 'manager_review', label: 'resubmit', type: 'back' }, ``` -> Two mistakes the compile-time flow lint flags: a `revise` edge whose wait node -> never loops back (a dead end `registerFlow` accepts but that leaves the -> submitter nowhere to resubmit), and a resubmit edge left **without** -> `type: 'back'` (an unmarked cycle `registerFlow` rejects). Resubmit is an -> explicit verb (`POST /api/v1/approvals/requests/:id/resubmit`), never a +> Three mistakes the compile-time flow lint flags: a `revise` edge into anything +> but an `approval_revise` node (an **error** — `sendBack` refuses that metadata, +> so the branch cannot run; `flow-approval-revise-target-not-service-owned`), a +> `revise` edge whose window never loops back (a dead end `registerFlow` accepts +> but that leaves the submitter nowhere to resubmit), and a resubmit edge left +> **without** `type: 'back'` (an unmarked cycle `registerFlow` rejects). Resubmit +> is an explicit verb (`POST /api/v1/approvals/requests/:id/resubmit`), never a > record-save. See the `showcase_budget_approval` flow in the showcase app in > the framework repo for the canonical shape. @@ -614,10 +622,11 @@ These are wired on the **graph**, not in node config: earlier node so the submitter can revise (the old `back_to_previous`). - **Send back for revision (ADR-0044)** — distinct from a plain reject: an Approval node can emit a third decision **`revise`** on a `revise`-labeled - out-edge that routes to a rework wait point. The submitter edits and - resubmits, re-entering the node via an edge `type: 'back'` (a declared - back-edge — traversed at run time but excluded from DAG cycle validation). - `maxRevisions` (node config, default `3`) caps the loop before auto-reject. + out-edge that routes to an **`approval_revise`** rework window (not a plain + `wait` — #3823). The submitter edits and resubmits, re-entering the node via an + edge `type: 'back'` (a declared back-edge — traversed at run time but excluded + from DAG cycle validation). `maxRevisions` (node config, default `3`) caps the + loop before auto-reject. - **Hard reject** — route the `reject` edge to an `end` node (the old `reject_process`). diff --git a/skills/objectstack-automation/evals/approvals/test-revise-loop.md b/skills/objectstack-automation/evals/approvals/test-revise-loop.md index 7cac2bfb27..2178489122 100644 --- a/skills/objectstack-automation/evals/approvals/test-revise-loop.md +++ b/skills/objectstack-automation/evals/approvals/test-revise-loop.md @@ -1,9 +1,9 @@ # Eval: approval send-back-for-revision loop (ADR-0044) Validates that an AI assistant authoring an approval flow with a *send back for -revision* step emits the full ADR-0044 shape — a `revise` branch, a signal -`wait` node, and a resubmit edge typed `type: 'back'` — so the flow **registers** -and the loop actually works at run time. +revision* step emits the full ADR-0044 shape — a `revise` branch, an +`approval_revise` window node, and a resubmit edge typed `type: 'back'` — so the +flow **registers** and the loop actually works at run time. Skill rule referenced: `SKILL.md` → "Send-back for revision (ADR-0044)". @@ -17,8 +17,8 @@ Skill rule referenced: `SKILL.md` → "Send-back for revision (ADR-0044)". ## Expected Output -An approval node with **three** labelled out-edges, a signal `wait` node for the -revision window, and a **declared back-edge** closing the loop: +An approval node with **three** labelled out-edges, an `approval_revise` node +for the revision window, and a **declared back-edge** closing the loop: ```typescript { @@ -32,8 +32,9 @@ revision window, and a **declared back-edge** closing the loop: { id: 'manager_review', type: 'approval', label: 'Manager Review', config: { approvers: [{ type: 'position', value: 'manager' }], lockRecord: true, maxRevisions: 2 } }, // send-back budget - { id: 'wait_revision', type: 'wait', label: 'Awaiting Revision', - config: { eventType: 'signal', signalName: 'budget_revision' } }, + // `approval_revise`, not `wait`: the window is service-owned, ended only by + // the submitter's resubmit — so it takes no config (#3823). + { id: 'wait_revision', type: 'approval_revise', label: 'Awaiting Revision' }, { id: 'approved', type: 'end', label: 'Approved' }, { id: 'rejected', type: 'end', label: 'Rejected' }, ], @@ -57,9 +58,10 @@ the framework repo. |---|---|---| | Missing `label` on the flow or on a node | `label` is required by `FlowSchema` — `FlowSchema.parse` / `registerFlow` rejects the definition before any graph validation runs | `registerFlow` (schema parse) | | Resubmit edge **without** `type: 'back'` | `registerFlow` validates the graph-minus-back-edges as a DAG, so it rejects the cycle as un-declared | `registerFlow`; lint `flow-approval-revise-unmarked-backedge` | -| `revise` edge to a wait node that **never loops back** | A valid DAG (registerFlow accepts it), but the submitter has nowhere to resubmit — the branch dead-ends | lint `flow-approval-revise-dead-end` | +| `revise` edge into a plain **`wait`** node (or any other type) | The window is a service-owned pause: a `wait` is `resumeAuthority: 'any'`, so a raw run-resume walks the back-edge with no submitter check and no audit row, and can destroy the run. `sendBack` refuses this metadata (#3823, amended ADR-0044) | lint `flow-approval-revise-target-not-service-owned` (**error**) | +| `revise` edge to a window that **never loops back** | A valid DAG (registerFlow accepts it), but the submitter has nowhere to resubmit — the branch dead-ends | lint `flow-approval-revise-dead-end` | | `maxRevisions: 0` together with a `revise` edge | Send-back is disabled, so every revise auto-rejects and the branch never runs | lint `flow-approval-revise-disabled` | -| Re-suspending the approval node in a "revise mode" (no wait node, no edge) | Hides a state machine inside one node — invisible to the canvas/run log; not the ADR-0044 model | design review | +| Re-suspending the approval node in a "revise mode" (no window node, no edge) | Hides a state machine inside one node — invisible to the canvas/run log; not the ADR-0044 model. (The 2026-07-28 amendment made the window a dedicated node TYPE, which keeps it visible; it did not move the pause inside the approval node.) | design review | | Reusing `reject` for send-back | `reject` terminates; send-back is a *movement* that returns the record for rework (status `returned`, not `rejected`) | semantics | ## Validation Criteria @@ -69,9 +71,9 @@ Score the generated flow: 1. **Registers** — `registerFlow` accepts it (no un-declared-cycle error). *(required)* 2. **Revise branch** — the approval node has an out-edge labelled `revise`. *(required)* 3. **Back-edge** — exactly one edge closes the loop into the approval node, typed `type: 'back'`. *(required)* -4. **Wait window** — the `revise` edge targets a `wait` node (signal flavour). *(required)* +4. **Revise window** — the `revise` edge targets an `approval_revise` node. *(required)* 5. **Guard** — `maxRevisions >= 1` on the approval config (the default `3` is fine; `0` fails). *(required)* -6. **No lint findings** — `lint-flow-patterns` emits none of the three `flow-approval-revise-*` warnings. *(required)* +6. **No lint findings** — `lint-flow-patterns` emits none of the four `flow-approval-revise-*` findings. *(required)* 7. **Approve / reject intact** — the approval still has `approve` and `reject` out-edges. *(preferred)* Pass = criteria 1–6 all hold. The canonical failure this eval guards against is a