Просмотр исходного кода

workflow: pin thenable-return semantics as documented async-JS behavior

Codex code-review round 4 flagged the return channel: an async IIFE
Promise-assimilates a returned thenable, so its then() runs past the sync
slice and the RESOLUTION replaces the raw object. Verified against the real
engine and judged behavior, not defect:

- Assimilation is standard JavaScript (an async function's returned thenable
  resolves before the caller sees it) and is load-bearing ergonomics: an
  un-awaited 'return agent(...)' / 'return parallel(...)' resolves to the
  intended value precisely because of it. Rejecting callable-then returns
  would break that; intercepting pre-assimilation is spec-impossible (the
  Get(v,'then') and job enqueue are internal to promise resolution).
- The realm-boundary guard applies to the RESOLUTION (a thenable resolving to
  non-JSON is still RESULT_UNSERIALIZABLE), so nothing crosses unmaterialized.
- A spin inside a returned thenable's then() is the same accepted class as any
  post-slice spin (it runs on the microtask queue, past the vm timeout's
  reach); the docs previously said 'after the first await', which was too
  narrow — reworded to 'past the initial synchronous slice (an await
  continuation, or a thenable's then invoked by promise resolution)'.

Pinned with an engine test (un-awaited return agent(); custom thenable
resolution as the return value; thenable resolving to non-JSON rejects), and
the limitation wording updated in the module doc, README, and RFC.
Tianyi Cui 3 месяцев назад
Родитель
Сommit
95c8c878e1

+ 1 - 1
docs/rfc/implemented/feature/2026-07-05-dynamic-workflows.md

@@ -24,7 +24,7 @@ One deliberate strictness DIVERGENCE from CC: hook misuse — unknown or deferre
 
 ### The engine (dsh-workflow-vm): in-process node:vm
 
-**Why node:vm and not isolated-vm/worker threads**: isolated-vm is in maintenance mode, needs `--no-node-snapshot` on EVERY consumer process (including the published bins) on Node ≥ 20, and falls back to node-gyp source builds; a worker-thread engine turns every hook into RPC and complicates the per-file coverage gate. Scripts are model-written — the same trust level as the model's existing bash access — so genuine sandboxing is not the current requirement. The interface/implementation split exists precisely so a hardened engine can swap in later. Accepted, documented limitations: vm is not a security boundary, and the vm timeout covers only the initial synchronous slice — a pathological synchronous spin after the first await cannot be killed in-process; `dispose()` cancels, waits a bounded grace for the script to settle and its children to finish disposing, then abandons.
+**Why node:vm and not isolated-vm/worker threads**: isolated-vm is in maintenance mode, needs `--no-node-snapshot` on EVERY consumer process (including the published bins) on Node ≥ 20, and falls back to node-gyp source builds; a worker-thread engine turns every hook into RPC and complicates the per-file coverage gate. Scripts are model-written — the same trust level as the model's existing bash access — so genuine sandboxing is not the current requirement. The interface/implementation split exists precisely so a hardened engine can swap in later. Accepted, documented limitations: vm is not a security boundary, and the vm timeout covers only the initial synchronous slice — a pathological synchronous spin in realm code past that slice (an await continuation, or a thenable's `then` invoked by promise resolution — a returned thenable resolves per JavaScript semantics, which is what makes an un-awaited `return agent('x')` work) cannot be killed in-process; `dispose()` cancels, waits a bounded grace for the script to settle and its children to finish disposing, then abandons.
 
 **Meta extraction**: a string/comment-aware brace scanner (template interpolation rejected) finds the literal; it is evaluated ALONE in an empty, timed vm context; the result must materialize to plain JSON data and pass shape validation (unknown fields rejected loud); the statement is blanked line-preservingly so stacks keep script line numbers.
 

+ 1 - 1
packages/workflow/workflow-vm/README.md

@@ -16,7 +16,7 @@ Values ENTERING the host (the meta literal, hook options/schemas, the script's r
 
 Per-run: a concurrency semaphore (`maxConcurrentAgents`), a total-`agent()` cap (`maxTotalAgents`), and a per-call item cap (`maxItemsPerCall`), all config. `cancel()` aborts every child (a shared `AbortSignal`), rejects waiting `agent()` slots, and makes every future hook call throw `CANCELLED` — the script dies at its next await and the run settles `cancelled`; a cancellation that lands before the body runs (or before it settles) reports `cancelled` even if the script itself needed no hooks. Once a run settles, stray children a script fired without awaiting are aborted too, and `dispose()` waits for those children to finish disposing (bounded by the grace) before returning. Every hook-returned promise carries a no-op rejection consumer, so a dropped promise cannot surface an unhandled rejection (the app boot layer exits the process on those). Thrown script values are pre-rendered to a string INSIDE the realm's execution window (the body is compiled into a realm-side catch), so a hostile `stack` getter is subject to the vm sync-slice timeout like any other script code; the host catch only descriptor-reads that string, falling back to `describeThrown` (fixed labels, own-data reads, an identity-verified host-native stack getter) — `result` cannot reject.
 
-**Documented limitations** (the accepted cost of the in-process mechanism; the seam exists so a worker-thread/isolated-vm engine can swap in): vm is NOT a security boundary — scripts are model-written, the same trust level as the model's bash access — and the vm `timeout` covers only the initial synchronous slice, so a pathological synchronous spin after the first await cannot be killed; `dispose()` waits `disposeGraceMs` then ABANDONS such a script (its settlement stays contained, but an abandoned spin would still occupy the event loop).
+**Documented limitations** (the accepted cost of the in-process mechanism; the seam exists so a worker-thread/isolated-vm engine can swap in): vm is NOT a security boundary — scripts are model-written, the same trust level as the model's bash access — and the vm `timeout` covers only the initial synchronous slice, so a pathological synchronous spin in realm code past that slice (an await continuation, or a thenable's `then` invoked by promise resolution) cannot be killed; `dispose()` waits `disposeGraceMs` then ABANDONS such a script (its settlement stays contained, but an abandoned spin would still occupy the event loop). A returned promise or thenable resolves per JavaScript semantics BEFORE materialization — that is what makes an un-awaited `return agent('x')` work — and the realm-boundary guard applies to the resolution.
 
 ## Config
 

+ 11 - 7
packages/workflow/workflow-vm/src/index.ts

@@ -12,13 +12,17 @@
  *   level as the model's bash access — and the realm-boundary materialization
  *   is correctness containment, not a sandbox.
  * - The vm `timeout` covers only the initial SYNCHRONOUS slice of the script;
- *   a pathological synchronous spin after the first await cannot be killed
- *   in-process. `dispose()` waits a bounded grace for the script to settle
- *   AND its children (stray `agent()` calls included) to finish disposing,
- *   then ABANDONS whatever is left: pending hook promises are already
- *   rejected and the script's settlement is contained (no unhandled
- *   rejection), but an abandoned synchronous spin would still occupy the
- *   event loop.
+ *   realm code that runs past that slice — an await continuation, a
+ *   thenable's `then` invoked by promise resolution (including one the script
+ *   RETURNS: a returned thenable resolves per JavaScript semantics before
+ *   materialization, which is what makes an un-awaited `return agent('x')`
+ *   work) — is beyond the timeout, so a pathological synchronous spin there
+ *   cannot be killed in-process. `dispose()` waits a bounded grace for the
+ *   script to settle AND its children (stray `agent()` calls included) to
+ *   finish disposing, then ABANDONS whatever is left: pending hook promises
+ *   are already rejected and the script's settlement is contained (no
+ *   unhandled rejection), but an abandoned synchronous spin would still
+ *   occupy the event loop.
  *
  * Plugin export shape: a default-exported {@link WorkflowService} subclass
  * (the class-based service form, like `dsh-bash-local`).

+ 13 - 0
packages/workflow/workflow-vm/tests/workflow-vm.spec.ts

@@ -217,6 +217,19 @@ describe('dsh-workflow-vm', () => {
       expect(result.stopReason).toBe('completed')
       expect(result.value).toBeNull()
     })
+
+    it('a returned promise/thenable resolves per async-JS semantics before materialization', async () => {
+      const { ctx, parent } = await setup()
+      // Load-bearing ergonomics: forgetting await on the final hook call works.
+      expect((await run(ctx, parent, script("return agent('x')"))).value).toBe('stub reply')
+      // A hand-built thenable is assimilated by the async return — the
+      // RESOLUTION is the script's return value (standard JavaScript), and the
+      // realm-boundary guard applies to that resolution, not the thenable.
+      expect((await run(ctx, parent, script('return { value: 1, then(resolve) { resolve({ ok: true }) } }'))).value).toEqual({ ok: true })
+      const nonJson = await run(ctx, parent, script('return { then(resolve) { resolve({ bad: new Date(0) }) } }'))
+      expect(nonJson.stopReason).toBe('error')
+      expect(nonJson.error).toContain('not plain JSON data')
+    })
   })
 
   describe('combinator semantics', () => {