Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions .changeset/resume-gate-map-chain-and-reserved-vars.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
---
"@objectstack/service-automation": patch
"@objectstack/runtime": patch
---

fix(automation): the resume gate follows `map:` too, and the route stops accepting engine-internal variables (#3853)

Two holes in the #3801 resume gate, both demonstrated with a repro.

**1. The chain walk missed `map:`.** `resumeInternal` handles the two linked-run
correlations oppositely — a `subflow:` pause *delegates* the signal to the child,
a `map:` pause *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 (`resumeAuthority: 'any'`)
and let through even while the item it was waiting on sat on an `approval`.

`map` is the batch-approval shape, and the map parent's run id is the one a
launcher holds. Since `$mapState.started` is advanced past the in-flight item
before the suspend, an empty-body resume of the parent **skipped that item's
approval outright**, orphaning its still-pending request; a later real decision
then bubbled into a parent already waiting on the next item, cascading the
misalignment.

The walk now follows both prefixes: a linked-run pause is waiting on a CHILD, so
the child's node carries the authority — the gate reads *the item, not the loop*.

**2. Resume `inputs` could write the engine's `$` namespace.** They are applied
as bare flow variables, so a caller could set the exact handoff keys the engine's
map bubble uses (`<nodeId>.$mapItemDone` / `$mapItemOutput`) and have the map
record a per-item result for a decision nobody made — the node id is readable
from `GET /automation/:name`. The same reached `$runId`, which `approval` /
`wait` nodes use to correlate external state back to a run.

`POST /automation/:name/runs/:runId/resume` now answers **400** when `inputs`
names anything in the engine namespace (`$…`, or a `.$` segment). Enforced at the
transport, not in the engine, so the in-process bubble keeps working — the same
trust split the gate itself uses.

Nothing changes for author-declared variables: `{ new_assignee: 'ada' }` and
dotted names like `collect.note` are unaffected. If you were driving a batch-
approval `map` by resuming the map's own run id, resume the **item's** run
through its owning service instead (e.g. `client.approvals.approve`) — the map
advances itself when the item completes.
33 changes: 26 additions & 7 deletions content/docs/automation/flows.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -411,11 +411,28 @@ A resume of a `'service'` pause through the route answers **403** and changes
nothing: the request stays pending and the run stays parked, so the real
decision can still land. (Before this gate a raw resume walked the `approve`
edge with no decision recorded, leaving the `sys_approval_request` row and the
run permanently disagreeing.) The gate follows a **subflow** pause down to the
child the signal would actually reach, so resuming the parent is no way around
it. Registering a pausing node of your own? Declare
`resumeAuthority: 'service'` on its descriptor when the decision to continue
belongs to your service rather than to whoever holds the run id.
run permanently disagreeing.)

The gate follows a **linked-run** pause down to the child, so the run id a
launcher happens to hold is not a way around it — in both directions it reads
the item rather than the loop:

- **`subflow`** — the signal is delegated to the suspended child, so the child's
node is where it lands;
- **`map`** — the signal is not delegated (the node re-runs to start the next
item), so continuing here would advance the batch **past** an item whose
decision is still open. Refused while that item is service-gated; the map
moves on when the item completes through its owning service.

Two related rules on the same route: **resume `inputs` may not write the
engine's `$` namespace** (`$runId`, `$record`, `$flowName`,
`<nodeId>.$mapItemDone`, …) — those are the engine's own handoff variables, and
a caller who could set them could forge a map item's recorded result. Ordinary
author-declared variables are unaffected; a reserved name answers **400**.

Registering a pausing node of your own? Declare `resumeAuthority: 'service'` on
its descriptor when the decision to continue belongs to your service rather
than to whoever holds the run id.

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

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

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

Expand Down
42 changes: 39 additions & 3 deletions docs/adr/0019-approval-as-flow-node.md
Original file line number Diff line number Diff line change
Expand Up @@ -225,9 +225,11 @@ discriminates by **what the run is parked on**, not by the route:
(`serviceResume`), on the tail of a decision it already authorized and recorded. The transport
builds its signal out of a JSON body, and no JSON body can produce a symbol-keyed property, so the
route cannot forge it; the gate does not need to trust the caller's claims about itself.
- **The gate follows the subflow chain.** A parent parked on a `subflow` node *delegates* the signal
to its suspended child (see the 2026-06-10 addendum), so the gate resolves the effective suspension
first and judges the node the signal actually lands on. Resuming the parent is not a way around it.
- **The gate follows the linked-run chain.** A parent parked on a `subflow` node *delegates* the
signal to its suspended child (see the 2026-06-10 addendum), so the gate resolves the effective
suspension first and judges the node the signal actually lands on. Resuming the parent is not a way
around it. *(As shipped this covered `subflow:` only; `map:` was added in #3853 — see the addendum
below.)*
- **Refusal is an authorization answer.** `resume` returns `{ success: false, code: 'forbidden' }`
and the route answers **403** — not a 200 carrying `success: false`, which reads as "your resume
ran and the flow failed". Nothing is consumed: the request stays pending and the run stays parked,
Expand All @@ -241,3 +243,37 @@ already-authorized call started.
(signal flavor) placed by the flow author, which the type-keyed gate cannot distinguish from any
other wait. A raw resume there still forces a resubmit without a `sys_approval_action` row. Filed as
a follow-up — closing it needs a per-suspension owner claim rather than a node-type one.

## Addendum (2026-07-28, #3853) — the chain walk covers `map:` too, and the transport owns the `$` namespace

Two defects in the gate the addendum above describes, both demonstrated with a repro rather than
reasoned:

**1. `map:` was not walked.** `resumeInternal` handles the two linked-run correlations *oppositely*:
a `subflow:` pause DELEGATES the signal down to the child, while a `map:` pause RE-RUNS the map node.
The gate's chain walk followed only the first, so a run parked on a `map` node was judged on `map`
itself — `resumeAuthority: 'any'` — and let through even while the item it was waiting on sat on an
`approval`. Since `$mapState.started` is advanced past the in-flight item *before* the suspend, and
the re-entry records a result only when `$mapItemDone` is set, an empty-body resume of the map parent
**skipped that item's approval outright**, orphaning its still-pending request; a later real decision
then bubbled into a parent already waiting on the *next* item, cascading the misalignment. The map
parent's run id is the one a launcher holds, which is what made it reachable.

The walk now follows both prefixes. The unifying rule is that a linked-run pause is waiting on a
CHILD, so the child's node carries the authority — the gate reads *the item, not the loop*. The two
differ only in what continuing would mean: for `subflow` the signal lands on the child, for `map` it
advances past it. Both are refusals for the same reason.

**2. The transport could write the engine's variable namespace.** `signal.variables` are applied as
BARE flow variables, so a caller could set the exact handoff keys `bubbleToParent` uses
(`<nodeId>.$mapItemDone` / `$mapItemOutput`) and have the map record a per-item result for a decision
nobody made — with the node id readable from `GET /automation/:name`. The same hole reaches `$runId`,
which `approval` / `wait` nodes use to correlate external state back to a run. `$` is the engine's
namespace (`$runId`, `$flowName`, `$flowLabel`, `$record`, `$error`, `$parentRunId`, `$parentMapNode`,
`$parentOutputVariable`, `<nodeId>.$mapState`); authors never write there.

The **route** now refuses a resume whose `inputs` name anything in that namespace (`$…` or a `.$`
segment) with a 400. Deliberately at the transport and not in the engine: `bubbleToParent` legitimately
writes those keys in-process, and this is the same trust split the gate itself uses — strict at the
untrusted boundary, unrestricted for the code that already holds the authority. Refuse rather than
silently strip, so a mis-authored screen input fails at the door instead of many nodes downstream.
20 changes: 19 additions & 1 deletion packages/runtime/src/domains/automation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,25 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str
const b = (body && typeof body === 'object') ? body : {};
const inputs = (b.inputs ?? b.variables);
const signal: any = {};
if (inputs && typeof inputs === 'object') signal.variables = inputs;
if (inputs && typeof inputs === 'object') {
// #3853: `inputs` land as BARE flow variables, and `$` is the
// engine's own variable namespace (`$runId`, `$record`,
// `$flowName`, `<nodeId>.$mapItemDone`/`$mapItemOutput`/
// `$mapState`, …). A caller who could write those could forge
// the map node's item handoff — recording a per-item result
// for an approval nobody made — or re-point `$runId`, which is
// how approval/wait nodes correlate external state back to a
// run. Author-declared variables never live in that namespace,
// so refuse rather than silently drop: a screen whose input is
// quietly discarded fails much further downstream.
const reserved = Object.keys(inputs).filter(k => k.startsWith('$') || k.includes('.$'));
if (reserved.length) {
return { handled: true, response: deps.error(
`Resume inputs may not set engine-internal variables (${reserved.join(', ')}) — ` +
`names starting with '$' (or containing '.$') are reserved by the flow engine`, 400) };
}
signal.variables = inputs;
}
if (b.output && typeof b.output === 'object') signal.output = b.output;
if (typeof b.branchLabel === 'string') signal.branchLabel = b.branchLabel;
const result = await automationService.resume(parts[2], signal);
Expand Down
30 changes: 30 additions & 0 deletions packages/runtime/src/http-dispatcher.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,36 @@ describe('HttpDispatcher', () => {
expect(result.response?.body?.data?.success).toBe(false);
});

// #3853: `inputs` land as BARE flow variables, so a caller who could
// write the engine's `$` namespace could forge the `map` node's item
// handoff (recording a per-item result for an approval nobody made) or
// re-point `$runId`, which is how approval/wait nodes correlate.
it('should refuse resume inputs that write engine-internal variables', async () => {
for (const inputs of [
{ 'signoffs.$mapItemDone': true, 'signoffs.$mapItemOutput': { forged: true } },
{ $runId: 'someone_elses_run' },
{ $record: { id: 'other' } },
]) {
const result = await dispatcher.handleAutomation(
'flow_a/runs/run_1/resume', 'POST', { inputs }, { request: {} },
);
expect(result.response?.status).toBe(400);
expect(result.response?.body?.error?.message).toMatch(/reserved by the flow engine/);
}
// Refused at the door — the engine is never asked.
expect(mockAutomationService.resume).not.toHaveBeenCalled();
});

it('should still accept ordinary screen inputs alongside the reserved-name guard', async () => {
await dispatcher.handleAutomation(
'flow_a/runs/run_1/resume', 'POST',
{ inputs: { new_assignee: 'ada', 'collect.note': 'hi', price$: 3 } }, { request: {} },
);
expect(mockAutomationService.resume).toHaveBeenCalledWith('run_1', {
variables: { new_assignee: 'ada', 'collect.note': 'hi', price$: 3 },
});
});

it('should return 501 when the automation service cannot resume', async () => {
delete mockAutomationService.resume;
const result = await dispatcher.handleAutomation(
Expand Down
63 changes: 44 additions & 19 deletions packages/services/service-automation/src/engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2001,12 +2001,13 @@ export class AutomationEngine implements IAutomationService {
* the flow-runner that owns it. The service-side resume proves itself with
* an in-process symbol the transport cannot mint from a JSON body.
*
* Resolves the EFFECTIVE suspension first: a run parked on a `subflow`
* node delegates the signal to its suspended child, so the gate follows
* that chain and judges the node the signal actually lands on. Anything it
* cannot resolve (unknown run, missing flow, unregistered node type) is
* left to `resumeInternal`, which reports the machine-state error — the
* gate only ever speaks to authorization.
* Resolves the EFFECTIVE suspension first: a run parked on a `subflow` or
* `map` node is really waiting on a CHILD run, so the gate follows that
* chain and judges the node the signal lands on (subflow) or would advance
* past (map) — see {@link LINKED_RUN_PREFIXES}. Anything it cannot resolve
* (unknown run, missing flow, unregistered node type) is left to
* `resumeInternal`, which reports the machine-state error — the gate only
* ever speaks to authorization.
*/
private async refuseGatedResume(runId: string, signal?: ResumeSignal): Promise<AutomationResult | null> {
const run = await this.resolveEffectiveSuspension(runId);
Expand All @@ -2019,17 +2020,21 @@ export class AutomationEngine implements IAutomationService {
// decision's tail, not a way around it.
if (signal?.[RESUME_AUTHORITY_SERVICE]) return null;

const at = run.runId === runId ? `'${run.nodeId}'` : `'${run.nodeId}' (subflow run '${run.runId}')`;
const direct = run.runId === runId;
const at = direct ? `'${run.nodeId}'` : `'${run.nodeId}' (linked run '${run.runId}')`;
this.logger.warn(
`[automation] refused resume of run '${runId}': parked on ${nodeType} node ${at}, which is resumable ` +
`only through its owning service (resumeAuthority: 'service')`,
);
return {
success: false,
code: 'forbidden',
error:
`Run '${runId}' is paused at a '${nodeType}' node, which only its owning service may resume — ` +
`drive it through that service's API (e.g. an approval decision), not a raw resume`,
error: direct
? `Run '${runId}' is paused at a '${nodeType}' node, which only its owning service may resume — ` +
`drive it through that service's API (e.g. an approval decision), not a raw resume`
: `Run '${runId}' is waiting on run '${run.runId}', which is paused at a '${nodeType}' node that ` +
`only its owning service may resume — resuming here would continue past a decision that has not ` +
`been made; drive it through that service's API instead`,
};
}

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

/**
* Follow the subflow delegation chain from `runId` to the suspension a
* resume signal would actually land on. A run paused at a `subflow` node
* (correlation `subflow:<childRunId>`) forwards the signal down, so the
* deepest reachable suspension — not the id the caller holds — is what a
* resume really continues. Returns `null` when nothing is suspended under
* that id; stops at the last resolvable link when a child row is gone
* (that run is where `resumeInternal` will continue).
* Linked-run correlation prefixes the gate walks (#3853). Both park a
* parent on a child run, so in both the pending work — and therefore the
* authority that governs continuing it — belongs to the CHILD, even though
* `resumeInternal` handles the two oppositely:
*
* - `subflow:` — the signal is DELEGATED down to the child, so the child's
* node is literally where it lands.
* - `map:` — the signal is not delegated; the `map` node RE-RUNS, and since
* `$mapState.started` was advanced past the in-flight item before the
* suspend, continuing here advances the map *past* the item whose child
* is still parked. Judging the parent's own `map` node (always
* `resumeAuthority: 'any'`) let a raw resume skip a pending approval in a
* batch-approval flow — the gate has to read the item, not the loop.
*/
private static readonly LINKED_RUN_PREFIXES = ['subflow:', 'map:'] as const;

/**
* Follow the linked-run chain from `runId` to the suspension whose node
* actually governs a resume — see {@link LINKED_RUN_PREFIXES}. The deepest
* reachable suspension, not the id the caller holds, is what a resume
* really continues (or skips past). Returns `null` when nothing is
* suspended under that id; stops at the last resolvable link when a child
* row is gone (that run is where `resumeInternal` will continue).
*/
private async resolveEffectiveSuspension(runId: string): Promise<SuspendedRun | null> {
const seen = new Set<string>();
let run = await this.loadSuspendedRun(runId);
for (let depth = 0; run && depth < AutomationEngine.MAX_SUSPENSION_CHAIN_DEPTH; depth++) {
if (typeof run.correlation !== 'string' || !run.correlation.startsWith('subflow:')) return run;
const childRunId = run.correlation.slice('subflow:'.length);
const correlation = run.correlation;
const prefix = typeof correlation === 'string'
? AutomationEngine.LINKED_RUN_PREFIXES.find(p => correlation.startsWith(p))
: undefined;
if (!prefix) return run;
const childRunId = correlation!.slice(prefix.length);
if (!childRunId || seen.has(childRunId)) return run;
seen.add(childRunId);
const child = await this.loadSuspendedRun(childRunId);
Expand Down
Loading
Loading