Skip to content

Commit 70a1ce1

Browse files
authored
fix(automation): resume gate follows map: too; the route stops accepting engine-internal variables (#3853) (#3860)
Two holes in the #3801 gate, both demonstrated with a repro before fixing. 1. The chain walk missed `map:`. resumeInternal handles the two linked-run correlations oppositely — `subflow:` delegates the signal to the child, `map:` re-runs the map node — and the gate followed only the first, so a run parked on a `map` node was judged on `map` itself and let through while the item it was waiting on sat on an approval. Since $mapState.started is advanced past the in-flight item before the suspend, an empty-body resume of the map parent skipped that item's approval outright and orphaned its pending request. The walk now follows both: a linked-run pause waits on a CHILD, so the child's node carries the authority — read the item, not the loop. 2. Resume `inputs` could write the engine's `$` namespace, letting a caller forge the map's item handoff (recording a result for a decision nobody made) or re-point $runId. The route now answers 400 for a reserved name, enforced at the transport so the in-process bubble keeps working. Adjacent revise-window gap remains #3823.
1 parent 3949a43 commit 70a1ce1

7 files changed

Lines changed: 282 additions & 30 deletions

File tree

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
---
2+
"@objectstack/service-automation": patch
3+
"@objectstack/runtime": patch
4+
---
5+
6+
fix(automation): the resume gate follows `map:` too, and the route stops accepting engine-internal variables (#3853)
7+
8+
Two holes in the #3801 resume gate, both demonstrated with a repro.
9+
10+
**1. The chain walk missed `map:`.** `resumeInternal` handles the two linked-run
11+
correlations oppositely — a `subflow:` pause *delegates* the signal to the child,
12+
a `map:` pause *re-runs* the map node — and the gate followed only the first. So
13+
a run parked on a `map` node was judged on `map` itself (`resumeAuthority: 'any'`)
14+
and let through even while the item it was waiting on sat on an `approval`.
15+
16+
`map` is the batch-approval shape, and the map parent's run id is the one a
17+
launcher holds. Since `$mapState.started` is advanced past the in-flight item
18+
before the suspend, an empty-body resume of the parent **skipped that item's
19+
approval outright**, orphaning its still-pending request; a later real decision
20+
then bubbled into a parent already waiting on the next item, cascading the
21+
misalignment.
22+
23+
The walk now follows both prefixes: a linked-run pause is waiting on a CHILD, so
24+
the child's node carries the authority — the gate reads *the item, not the loop*.
25+
26+
**2. Resume `inputs` could write the engine's `$` namespace.** They are applied
27+
as bare flow variables, so a caller could set the exact handoff keys the engine's
28+
map bubble uses (`<nodeId>.$mapItemDone` / `$mapItemOutput`) and have the map
29+
record a per-item result for a decision nobody made — the node id is readable
30+
from `GET /automation/:name`. The same reached `$runId`, which `approval` /
31+
`wait` nodes use to correlate external state back to a run.
32+
33+
`POST /automation/:name/runs/:runId/resume` now answers **400** when `inputs`
34+
names anything in the engine namespace (`$…`, or a `.$` segment). Enforced at the
35+
transport, not in the engine, so the in-process bubble keeps working — the same
36+
trust split the gate itself uses.
37+
38+
Nothing changes for author-declared variables: `{ new_assignee: 'ada' }` and
39+
dotted names like `collect.note` are unaffected. If you were driving a batch-
40+
approval `map` by resuming the map's own run id, resume the **item's** run
41+
through its owning service instead (e.g. `client.approvals.approve`) — the map
42+
advances itself when the item completes.

content/docs/automation/flows.mdx

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -411,11 +411,28 @@ A resume of a `'service'` pause through the route answers **403** and changes
411411
nothing: the request stays pending and the run stays parked, so the real
412412
decision can still land. (Before this gate a raw resume walked the `approve`
413413
edge with no decision recorded, leaving the `sys_approval_request` row and the
414-
run permanently disagreeing.) The gate follows a **subflow** pause down to the
415-
child the signal would actually reach, so resuming the parent is no way around
416-
it. Registering a pausing node of your own? Declare
417-
`resumeAuthority: 'service'` on its descriptor when the decision to continue
418-
belongs to your service rather than to whoever holds the run id.
414+
run permanently disagreeing.)
415+
416+
The gate follows a **linked-run** pause down to the child, so the run id a
417+
launcher happens to hold is not a way around it — in both directions it reads
418+
the item rather than the loop:
419+
420+
- **`subflow`** — the signal is delegated to the suspended child, so the child's
421+
node is where it lands;
422+
- **`map`** — the signal is not delegated (the node re-runs to start the next
423+
item), so continuing here would advance the batch **past** an item whose
424+
decision is still open. Refused while that item is service-gated; the map
425+
moves on when the item completes through its owning service.
426+
427+
Two related rules on the same route: **resume `inputs` may not write the
428+
engine's `$` namespace** (`$runId`, `$record`, `$flowName`,
429+
`<nodeId>.$mapItemDone`, …) — those are the engine's own handoff variables, and
430+
a caller who could set them could forge a map item's recorded result. Ordinary
431+
author-declared variables are unaffected; a reserved name answers **400**.
432+
433+
Registering a pausing node of your own? Declare `resumeAuthority: 'service'` on
434+
its descriptor when the decision to continue belongs to your service rather
435+
than to whoever holds the run id.
419436

420437
### Parallel approvals — one aggregating node, not two pauses
421438

@@ -475,8 +492,10 @@ the map waits for that item's decision, then moves to the next.
475492

476493
The run holds a **single program counter** the whole time: only one item's
477494
approval is open at any moment; when it is decided the engine **re-enters** the
478-
map node to start the next item. v1 is sequential and fail-fast (the first item
479-
whose subflow fails fails the map). Concurrent fan-out (all items at once) is the
495+
map node to start the next item. Resuming the map's own run id while an item is
496+
still awaiting a service-gated decision is **refused** (403) — otherwise the
497+
batch would step past that item as if it had been decided. v1 is sequential and
498+
fail-fast (the first item whose subflow fails fails the map). Concurrent fan-out (all items at once) is the
480499
larger [ADR-0039](https://github.com/objectstack-ai/objectstack/blob/main/docs/adr/0039-token-scope-tree-execution.md)
481500
Track B work. Worked example: `showcase_release_signoff``showcase_one_task_signoff`.
482501

docs/adr/0019-approval-as-flow-node.md

Lines changed: 39 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -225,9 +225,11 @@ discriminates by **what the run is parked on**, not by the route:
225225
(`serviceResume`), on the tail of a decision it already authorized and recorded. The transport
226226
builds its signal out of a JSON body, and no JSON body can produce a symbol-keyed property, so the
227227
route cannot forge it; the gate does not need to trust the caller's claims about itself.
228-
- **The gate follows the subflow chain.** A parent parked on a `subflow` node *delegates* the signal
229-
to its suspended child (see the 2026-06-10 addendum), so the gate resolves the effective suspension
230-
first and judges the node the signal actually lands on. Resuming the parent is not a way around it.
228+
- **The gate follows the linked-run chain.** A parent parked on a `subflow` node *delegates* the
229+
signal to its suspended child (see the 2026-06-10 addendum), so the gate resolves the effective
230+
suspension first and judges the node the signal actually lands on. Resuming the parent is not a way
231+
around it. *(As shipped this covered `subflow:` only; `map:` was added in #3853 — see the addendum
232+
below.)*
231233
- **Refusal is an authorization answer.** `resume` returns `{ success: false, code: 'forbidden' }`
232234
and the route answers **403** — not a 200 carrying `success: false`, which reads as "your resume
233235
ran and the flow failed". Nothing is consumed: the request stays pending and the run stays parked,
@@ -241,3 +243,37 @@ already-authorized call started.
241243
(signal flavor) placed by the flow author, which the type-keyed gate cannot distinguish from any
242244
other wait. A raw resume there still forces a resubmit without a `sys_approval_action` row. Filed as
243245
a follow-up — closing it needs a per-suspension owner claim rather than a node-type one.
246+
247+
## Addendum (2026-07-28, #3853) — the chain walk covers `map:` too, and the transport owns the `$` namespace
248+
249+
Two defects in the gate the addendum above describes, both demonstrated with a repro rather than
250+
reasoned:
251+
252+
**1. `map:` was not walked.** `resumeInternal` handles the two linked-run correlations *oppositely*:
253+
a `subflow:` pause DELEGATES the signal down to the child, while a `map:` pause RE-RUNS the map node.
254+
The gate's chain walk followed only the first, so a run parked on a `map` node was judged on `map`
255+
itself — `resumeAuthority: 'any'` — and let through even while the item it was waiting on sat on an
256+
`approval`. Since `$mapState.started` is advanced past the in-flight item *before* the suspend, and
257+
the re-entry records a result only when `$mapItemDone` is set, an empty-body resume of the map parent
258+
**skipped that item's approval outright**, orphaning its still-pending request; a later real decision
259+
then bubbled into a parent already waiting on the *next* item, cascading the misalignment. The map
260+
parent's run id is the one a launcher holds, which is what made it reachable.
261+
262+
The walk now follows both prefixes. The unifying rule is that a linked-run pause is waiting on a
263+
CHILD, so the child's node carries the authority — the gate reads *the item, not the loop*. The two
264+
differ only in what continuing would mean: for `subflow` the signal lands on the child, for `map` it
265+
advances past it. Both are refusals for the same reason.
266+
267+
**2. The transport could write the engine's variable namespace.** `signal.variables` are applied as
268+
BARE flow variables, so a caller could set the exact handoff keys `bubbleToParent` uses
269+
(`<nodeId>.$mapItemDone` / `$mapItemOutput`) and have the map record a per-item result for a decision
270+
nobody made — with the node id readable from `GET /automation/:name`. The same hole reaches `$runId`,
271+
which `approval` / `wait` nodes use to correlate external state back to a run. `$` is the engine's
272+
namespace (`$runId`, `$flowName`, `$flowLabel`, `$record`, `$error`, `$parentRunId`, `$parentMapNode`,
273+
`$parentOutputVariable`, `<nodeId>.$mapState`); authors never write there.
274+
275+
The **route** now refuses a resume whose `inputs` name anything in that namespace (`$…` or a `.$`
276+
segment) with a 400. Deliberately at the transport and not in the engine: `bubbleToParent` legitimately
277+
writes those keys in-process, and this is the same trust split the gate itself uses — strict at the
278+
untrusted boundary, unrestricted for the code that already holds the authority. Refuse rather than
279+
silently strip, so a mis-authored screen input fails at the door instead of many nodes downstream.

packages/runtime/src/domains/automation.ts

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,25 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str
223223
const b = (body && typeof body === 'object') ? body : {};
224224
const inputs = (b.inputs ?? b.variables);
225225
const signal: any = {};
226-
if (inputs && typeof inputs === 'object') signal.variables = inputs;
226+
if (inputs && typeof inputs === 'object') {
227+
// #3853: `inputs` land as BARE flow variables, and `$` is the
228+
// engine's own variable namespace (`$runId`, `$record`,
229+
// `$flowName`, `<nodeId>.$mapItemDone`/`$mapItemOutput`/
230+
// `$mapState`, …). A caller who could write those could forge
231+
// the map node's item handoff — recording a per-item result
232+
// for an approval nobody made — or re-point `$runId`, which is
233+
// how approval/wait nodes correlate external state back to a
234+
// run. Author-declared variables never live in that namespace,
235+
// so refuse rather than silently drop: a screen whose input is
236+
// quietly discarded fails much further downstream.
237+
const reserved = Object.keys(inputs).filter(k => k.startsWith('$') || k.includes('.$'));
238+
if (reserved.length) {
239+
return { handled: true, response: deps.error(
240+
`Resume inputs may not set engine-internal variables (${reserved.join(', ')}) — ` +
241+
`names starting with '$' (or containing '.$') are reserved by the flow engine`, 400) };
242+
}
243+
signal.variables = inputs;
244+
}
227245
if (b.output && typeof b.output === 'object') signal.output = b.output;
228246
if (typeof b.branchLabel === 'string') signal.branchLabel = b.branchLabel;
229247
const result = await automationService.resume(parts[2], signal);

packages/runtime/src/http-dispatcher.test.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -358,6 +358,36 @@ describe('HttpDispatcher', () => {
358358
expect(result.response?.body?.data?.success).toBe(false);
359359
});
360360

361+
// #3853: `inputs` land as BARE flow variables, so a caller who could
362+
// write the engine's `$` namespace could forge the `map` node's item
363+
// handoff (recording a per-item result for an approval nobody made) or
364+
// re-point `$runId`, which is how approval/wait nodes correlate.
365+
it('should refuse resume inputs that write engine-internal variables', async () => {
366+
for (const inputs of [
367+
{ 'signoffs.$mapItemDone': true, 'signoffs.$mapItemOutput': { forged: true } },
368+
{ $runId: 'someone_elses_run' },
369+
{ $record: { id: 'other' } },
370+
]) {
371+
const result = await dispatcher.handleAutomation(
372+
'flow_a/runs/run_1/resume', 'POST', { inputs }, { request: {} },
373+
);
374+
expect(result.response?.status).toBe(400);
375+
expect(result.response?.body?.error?.message).toMatch(/reserved by the flow engine/);
376+
}
377+
// Refused at the door — the engine is never asked.
378+
expect(mockAutomationService.resume).not.toHaveBeenCalled();
379+
});
380+
381+
it('should still accept ordinary screen inputs alongside the reserved-name guard', async () => {
382+
await dispatcher.handleAutomation(
383+
'flow_a/runs/run_1/resume', 'POST',
384+
{ inputs: { new_assignee: 'ada', 'collect.note': 'hi', price$: 3 } }, { request: {} },
385+
);
386+
expect(mockAutomationService.resume).toHaveBeenCalledWith('run_1', {
387+
variables: { new_assignee: 'ada', 'collect.note': 'hi', price$: 3 },
388+
});
389+
});
390+
361391
it('should return 501 when the automation service cannot resume', async () => {
362392
delete mockAutomationService.resume;
363393
const result = await dispatcher.handleAutomation(

packages/services/service-automation/src/engine.ts

Lines changed: 44 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2001,12 +2001,13 @@ export class AutomationEngine implements IAutomationService {
20012001
* the flow-runner that owns it. The service-side resume proves itself with
20022002
* an in-process symbol the transport cannot mint from a JSON body.
20032003
*
2004-
* Resolves the EFFECTIVE suspension first: a run parked on a `subflow`
2005-
* node delegates the signal to its suspended child, so the gate follows
2006-
* that chain and judges the node the signal actually lands on. Anything it
2007-
* cannot resolve (unknown run, missing flow, unregistered node type) is
2008-
* left to `resumeInternal`, which reports the machine-state error — the
2009-
* gate only ever speaks to authorization.
2004+
* Resolves the EFFECTIVE suspension first: a run parked on a `subflow` or
2005+
* `map` node is really waiting on a CHILD run, so the gate follows that
2006+
* chain and judges the node the signal lands on (subflow) or would advance
2007+
* past (map) — see {@link LINKED_RUN_PREFIXES}. Anything it cannot resolve
2008+
* (unknown run, missing flow, unregistered node type) is left to
2009+
* `resumeInternal`, which reports the machine-state error — the gate only
2010+
* ever speaks to authorization.
20102011
*/
20112012
private async refuseGatedResume(runId: string, signal?: ResumeSignal): Promise<AutomationResult | null> {
20122013
const run = await this.resolveEffectiveSuspension(runId);
@@ -2019,17 +2020,21 @@ export class AutomationEngine implements IAutomationService {
20192020
// decision's tail, not a way around it.
20202021
if (signal?.[RESUME_AUTHORITY_SERVICE]) return null;
20212022

2022-
const at = run.runId === runId ? `'${run.nodeId}'` : `'${run.nodeId}' (subflow run '${run.runId}')`;
2023+
const direct = run.runId === runId;
2024+
const at = direct ? `'${run.nodeId}'` : `'${run.nodeId}' (linked run '${run.runId}')`;
20232025
this.logger.warn(
20242026
`[automation] refused resume of run '${runId}': parked on ${nodeType} node ${at}, which is resumable ` +
20252027
`only through its owning service (resumeAuthority: 'service')`,
20262028
);
20272029
return {
20282030
success: false,
20292031
code: 'forbidden',
2030-
error:
2031-
`Run '${runId}' is paused at a '${nodeType}' node, which only its owning service may resume — ` +
2032-
`drive it through that service's API (e.g. an approval decision), not a raw resume`,
2032+
error: direct
2033+
? `Run '${runId}' is paused at a '${nodeType}' node, which only its owning service may resume — ` +
2034+
`drive it through that service's API (e.g. an approval decision), not a raw resume`
2035+
: `Run '${runId}' is waiting on run '${run.runId}', which is paused at a '${nodeType}' node that ` +
2036+
`only its owning service may resume — resuming here would continue past a decision that has not ` +
2037+
`been made; drive it through that service's API instead`,
20332038
};
20342039
}
20352040

@@ -2063,20 +2068,40 @@ export class AutomationEngine implements IAutomationService {
20632068
private static readonly MAX_ALIAS_HOPS = 4;
20642069

20652070
/**
2066-
* Follow the subflow delegation chain from `runId` to the suspension a
2067-
* resume signal would actually land on. A run paused at a `subflow` node
2068-
* (correlation `subflow:<childRunId>`) forwards the signal down, so the
2069-
* deepest reachable suspension — not the id the caller holds — is what a
2070-
* resume really continues. Returns `null` when nothing is suspended under
2071-
* that id; stops at the last resolvable link when a child row is gone
2072-
* (that run is where `resumeInternal` will continue).
2071+
* Linked-run correlation prefixes the gate walks (#3853). Both park a
2072+
* parent on a child run, so in both the pending work — and therefore the
2073+
* authority that governs continuing it — belongs to the CHILD, even though
2074+
* `resumeInternal` handles the two oppositely:
2075+
*
2076+
* - `subflow:` — the signal is DELEGATED down to the child, so the child's
2077+
* node is literally where it lands.
2078+
* - `map:` — the signal is not delegated; the `map` node RE-RUNS, and since
2079+
* `$mapState.started` was advanced past the in-flight item before the
2080+
* suspend, continuing here advances the map *past* the item whose child
2081+
* is still parked. Judging the parent's own `map` node (always
2082+
* `resumeAuthority: 'any'`) let a raw resume skip a pending approval in a
2083+
* batch-approval flow — the gate has to read the item, not the loop.
2084+
*/
2085+
private static readonly LINKED_RUN_PREFIXES = ['subflow:', 'map:'] as const;
2086+
2087+
/**
2088+
* Follow the linked-run chain from `runId` to the suspension whose node
2089+
* actually governs a resume — see {@link LINKED_RUN_PREFIXES}. The deepest
2090+
* reachable suspension, not the id the caller holds, is what a resume
2091+
* really continues (or skips past). Returns `null` when nothing is
2092+
* suspended under that id; stops at the last resolvable link when a child
2093+
* row is gone (that run is where `resumeInternal` will continue).
20732094
*/
20742095
private async resolveEffectiveSuspension(runId: string): Promise<SuspendedRun | null> {
20752096
const seen = new Set<string>();
20762097
let run = await this.loadSuspendedRun(runId);
20772098
for (let depth = 0; run && depth < AutomationEngine.MAX_SUSPENSION_CHAIN_DEPTH; depth++) {
2078-
if (typeof run.correlation !== 'string' || !run.correlation.startsWith('subflow:')) return run;
2079-
const childRunId = run.correlation.slice('subflow:'.length);
2099+
const correlation = run.correlation;
2100+
const prefix = typeof correlation === 'string'
2101+
? AutomationEngine.LINKED_RUN_PREFIXES.find(p => correlation.startsWith(p))
2102+
: undefined;
2103+
if (!prefix) return run;
2104+
const childRunId = correlation!.slice(prefix.length);
20802105
if (!childRunId || seen.has(childRunId)) return run;
20812106
seen.add(childRunId);
20822107
const child = await this.loadSuspendedRun(childRunId);

0 commit comments

Comments
 (0)