fix(automation): one chokepoint for the resume signal — output reopened the hole inputs had just closed (#3879) (#3880)

#3853 guarded `signal.variables` at the route. That closed one of two
equivalent paths into the same variable map and left the other open:
`signal.output` keys are merged under `${run.nodeId}.${key}`, and for a run
parked on a `map` node `run.nodeId` IS the map node — so

  { "output": { "$mapItemDone": true, "$mapItemOutput": … } }

writes exactly the `<mapNodeId>.$mapItemDone` the `inputs` guard had refused,
making the map record a result for an item nobody decided. Demonstrated with a
repro, then fixed.

Scope: the #3853 map gate still held, so a batch whose pending item sits on an
`approval` was refused before any of this — the approval bypass stayed closed.
The residual was forging the recorded result of an item on an ungated pause.

Two escapes with one shape is a design signal, not two bugs. The seam had three
open-coded writers into one variable map (`output` prefixed, `variables` bare,
the engine's own map handoff), so "guard the field that was exploited" was
always going to invite the next field. Structural fix:

- applyResumeSignal is the ONE place a resume signal reaches the variable map.
  Both fields become a single write list (already in final, prefixed form),
  checked, then applied — a new signal field is covered by construction.
- All-or-nothing, and checked before the suspension is consumed: a rejected
  signal applies nothing and the run stays parked, so the real continuation
  still lands.
- The engine owns the rule; the transport maps the verdict. resume returns
  code 'invalid_signal', the route answers 400. The SDK and any future adapter
  inherit it. This corrects #3853's placement argument: "strict at the
  untrusted boundary" is right about where a rule BINDS, not where it LIVES.
- Engine-built signals (subflow output mapping, map item handoff) are exempt
  via a module-private symbol — deliberately not RESUME_AUTHORITY_SERVICE,
  which answers a different question and does not license writing internals.


Claude-Session: https://claude.ai/code/session_013gvN32u1EiuvY9uQEMJiMR

Co-authored-by: Claude <noreply@anthropic.com>

Author: os-zhuang

.changeset/resume-signal-chokepoint.md

@@ -0,0 +1,51 @@
+---
+"@objectstack/spec": minor
+"@objectstack/service-automation": patch
+"@objectstack/runtime": patch
+---
+
+fix(automation): one chokepoint for the resume signal — `output` reopened the hole `inputs` had just closed (#3879)
+
+#3853 guarded `signal.variables` at the route. That closed one of **two**
+equivalent paths into the same variable map and left the other open:
+`signal.output` keys are merged under `${run.nodeId}.${key}`, and for a run
+parked on a `map` node `run.nodeId` **is** the map node — so
+
+```jsonc
+{ "output": { "$mapItemDone": true, "$mapItemOutput": { "result": "FORGED" } } }
+```
+
+writes exactly the `<mapNodeId>.$mapItemDone` the `inputs` guard had refused,
+making the map record a result for an item nobody decided. Demonstrated with a
+repro, then fixed.
+
+Scope: the #3853 map gate still held, so a batch whose pending item sits on an
+`approval` was refused before any of this — the **approval bypass stayed
+closed**. The residual was forging the recorded result of an item on an
+*ungated* pause.
+
+Two escapes with one shape is a design signal, not two bugs, so the fix is
+structural rather than a third patch:
+
+- **`applyResumeSignal` is the one place a resume signal reaches the variable
+  map.** Both fields are collected into a single write list (already in final,
+  prefixed form), checked, then applied — a new signal field is covered by
+  construction rather than by remembering.
+- **All-or-nothing**, and checked *before* the suspension is consumed: a
+  rejected signal applies nothing (not even legitimate keys sent alongside) and
+  the run stays parked, so the real continuation still lands.
+- **The engine owns the rule; the transport maps the verdict.** `resume` returns
+  `{ success: false, code: 'invalid_signal' }`; the route answers **400**. The
+  SDK and any future adapter inherit it — implemented in one transport it
+  protected exactly one transport, and one field of it.
+- Engine-built signals (the subflow output mapping, the map item handoff) are
+  exempt via a module-private symbol. Deliberately *not*
+  `RESUME_AUTHORITY_SERVICE`: that marker means "the owning service authorized
+  this decision", and a service still has no business writing engine internals.
+
+`AutomationResult.code` gains `'invalid_signal'` alongside `'forbidden'` — a
+`switch` over it needs a new arm; a plain read does not.
+
+Nothing changes for authoring: ordinary variables pass, `$` mid-name (`price$`)
+and dotted names (`collect.note`) included. Only names the engine reserves —
+`$…` or a `.$` segment — are refused.

content/docs/automation/flows.mdx

@@ -425,11 +425,16 @@ the item rather than the loop:
   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**.
+A second rule on the same seam: **a resume signal 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 or re-point the run id an `approval` /
+`wait` node correlates on. It covers **both** signal fields: `inputs` land under
+their plain names, and `output` keys land under the *suspended node's* id —
+which for a map-parked run is the map node itself, i.e. the very same reserved
+key. A reserved name answers **400**, nothing is applied (not even legitimate
+keys sent alongside it), and the run stays parked. Ordinary author variables are
+unaffected, `$` mid-name (`price$`) included.
 
 Registering a pausing node of your own? Declare `resumeAuthority: 'service'` on
 its descriptor when the decision to continue belongs to your service rather

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

@@ -277,3 +277,43 @@ segment) with a 400. Deliberately at the transport and not in the engine: `bubbl
 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.
+
+> **Half of this was wrong — see the addendum below (#3879).** Guarding `inputs` at the transport left
+> `output` untouched, and `output` keys land under `${nodeId}.${key}`, which for a map-parked run is
+> the map node itself — the identical reserved key, forgeable through the other field. The *placement*
+> argument above was the error: "strict at the untrusted boundary" is right about where the rule
+> BINDS, not about where it LIVES. The rule moved into the engine, at the one place a signal touches
+> the variable map.
+
+## Addendum (2026-07-28, #3879) — one chokepoint for the resume signal, because guarding a field at a time failed twice
+
+The addendum above guarded `signal.variables` **at the route**. That closed one of two equivalent
+paths into the same variable map and left the other open: `signal.output` keys are merged under
+`${run.nodeId}.${key}`, and for a run parked on a `map` node `run.nodeId` **is** the map node — so
+`{ "output": { "$mapItemDone": true, "$mapItemOutput": … } }` writes exactly the
+`<mapNodeId>.$mapItemDone` the `inputs` guard had just refused. Demonstrated, then fixed.
+
+Note what the map gate (#3853) still bought: a batch whose pending item sits on an `approval` is
+refused before any of this, so the **approval bypass stayed closed**. The residual was forging the
+recorded result of an item on an *ungated* pause — map-state corruption, not a decision bypass.
+
+Two escapes with one shape is a design signal, not two bugs. The seam had **three** open-coded writers
+into one variable map (`output` prefixed, `variables` bare, and the engine's own map handoff), so
+"guard the field that was exploited" was always going to invite the next field. The fix is structural:
+
+- **`applyResumeSignal` is the one place a resume signal reaches the variable map.** Both fields are
+  collected into a single write list — already in final, prefixed form — checked, then applied. A new
+  signal field is covered by construction rather than by remembering.
+- **All-or-nothing.** A rejected signal applies nothing, not even legitimate keys sent alongside, and
+  the check runs *before* the suspension is consumed, so the run stays parked and the real
+  continuation still lands.
+- **The engine owns the rule; the transport maps the verdict.** `resume` returns
+  `{ success: false, code: 'invalid_signal' }` and the route answers 400. This corrects the placement
+  argument in the previous addendum: "strict at the untrusted boundary" is right about where a rule
+  BINDS, not where it LIVES — implemented in the transport it protected exactly one transport and one
+  field of it, and the SDK, any future adapter, and `output` all sat outside it.
+- **Engine-built signals are exempt via a module-private symbol** (`ENGINE_BUILT_SIGNAL`), stamped by
+  `bubbleToParent` and the subflow output mapping — the only writers that legitimately set the handoff
+  keys, and unreachable from a transport. Deliberately *not* `RESUME_AUTHORITY_SERVICE`: that marker
+  answers "the owning service authorized this decision", and a service still has no business writing
+  engine internals. Two different questions, two different markers.

packages/runtime/src/domains/automation.ts

@@ -216,38 +216,33 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str
         // `ApprovalService`, which records the decision and enforces the slate —
         // on a SYMBOL-keyed marker. Assembling the signal field-wise (never
         // spreading the body) keeps that unforgeable even if a caller invents
-        // extra keys; a refused resume comes back `code: 'forbidden'` and is
-        // answered 403 rather than a 200 carrying `success: false`.
+        // extra keys.
+        //
+        // Two REFUSAL codes come back from the engine and are answered as such
+        // rather than a 200 carrying `success: false` (which reads as "your
+        // resume ran and the flow failed"):
+        //   forbidden      → 403, the suspension is service-owned (#3801)
+        //   invalid_signal → 400, the signal wrote the engine's `$` variable
+        //                    namespace (#3853 follow-up)
+        // Both are enforced in the ENGINE, at the one place a signal reaches the
+        // variable map — deliberately not re-implemented here. Guarding a field
+        // at a time in the transport is what let `output` reopen the hole
+        // `inputs` had just closed; every transport now inherits one rule.
         if (parts[1] === 'runs' && parts[2] && parts[3] === 'resume' && m === 'POST') {
             if (typeof automationService.resume === 'function') {
                 const b = (body && typeof body === 'object') ? body : {};
                 const inputs = (b.inputs ?? b.variables);
                 const signal: any = {};
-                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 (inputs && typeof inputs === 'object') 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);
                 if (result?.success === false && result.code === 'forbidden') {
                     return { handled: true, response: deps.error(result.error ?? 'Resume forbidden', 403) };
                 }
+                if (result?.success === false && result.code === 'invalid_signal') {
+                    return { handled: true, response: deps.error(result.error ?? 'Invalid resume signal', 400) };
+                }
                 return { handled: true, response: deps.success(result) };
             }
             return { handled: true, response: deps.error('Resume not supported', 501) };

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

@@ -358,33 +358,34 @@ 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();
+        // #3853 follow-up: the reserved-name rule lives in the ENGINE, at the one
+        // place a signal reaches the variable map — the route only maps its
+        // verdict onto a status. (Guarding one body field at a time here is what
+        // let `output` reopen the hole `inputs` had just closed.)
+        it('should answer 400 when the engine rejects the signal as engine-internal', async () => {
+            mockAutomationService.resume.mockResolvedValue({
+                success: false, code: 'invalid_signal',
+                error: "Resume signal may not set engine-internal variables (signoffs.$mapItemDone) — " +
+                    "names starting with '$' (or containing '.$') are reserved by the flow engine",
+            });
+            const result = await dispatcher.handleAutomation(
+                'flow_a/runs/run_1/resume', 'POST',
+                { output: { $mapItemDone: true } }, { request: {} },
+            );
+            expect(result.response?.status).toBe(400);
+            expect(result.response?.body?.error?.message).toMatch(/reserved by the flow engine/);
         });
 
-        it('should still accept ordinary screen inputs alongside the reserved-name guard', async () => {
+        // Both body fields reach the engine verbatim — it, not the route, decides.
+        it('should forward `output` and `inputs` unfiltered for the engine to judge', async () => {
             await dispatcher.handleAutomation(
                 'flow_a/runs/run_1/resume', 'POST',
-                { inputs: { new_assignee: 'ada', 'collect.note': 'hi', price$: 3 } }, { request: {} },
+                { inputs: { new_assignee: 'ada', 'collect.note': 'hi', price$: 3 }, output: { decision: 'ok' } },
+                { request: {} },
             );
             expect(mockAutomationService.resume).toHaveBeenCalledWith('run_1', {
                 variables: { new_assignee: 'ada', 'collect.note': 'hi', price$: 3 },
+                output: { decision: 'ok' },
             });
         });