workflow, subagent: fix Codex code-review round-1 blockers
Six verified A-findings from the code-stage review, each with a regression test: - parallel()/pipeline() resolved to HOST arrays inside the vm realm, exposing host Array.prototype to scripts; combinator results are now realm-built (in-realm Array.from bound at context setup). - materializeFromRealm ran proxy traps (ownKeys/getOwnPropertyDescriptor/ getPrototypeOf) during the descriptor walk — realm code on the host stack, outside the vm timeout, escaping as raw errors; proxies (root, nested, and in the prototype position) are now rejected trap-free via util.types.isProxy before any inspection. - an already-aborted signal or an immediate cancel() no longer reports 'completed' for a hook-free script: drive() checks cancellation before running the body and again when the script settles. - dispose() now waits (bounded by disposeGraceMs) for stray agent() children to FINISH disposing, not just for the script to settle: every agent() call is tracked and quiesce() drains the in-flight set. - workflow/* event payloads were live mutable aliases shared across emissions; emitWorkflowEvent now hands each listener its own structural clone. - the structured-output turn-continuation veto is now prepend: true, so an earlier-registered force-continue listener cannot short-circuit it. Docs updated in the same change (READMEs, core-data-structures/workflow.md, the dynamic-workflows RFC, regenerated cordis catalogs).
This commit is contained in:
17 files changed
+358
-51
No files matched your search
@@ -13,10 +13,12 @@
|
||||
* 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()` therefore waits a bounded grace and then ABANDONS
|
||||
* a stuck script: its pending hook promises are already rejected and its
|
||||
* settlement is contained (no unhandled rejection), but an abandoned
|
||||
* synchronous spin would still occupy the event loop.
|
||||
* 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`).
|
||||
@@ -142,12 +144,22 @@ export class VmWorkflowEngine extends WorkflowService {
|
||||
execution.cancel(reason)
|
||||
},
|
||||
dispose: (): Promise<void> => {
|
||||
// Idempotent: cancel, then wait min(settle, grace). `result` never
|
||||
// rejects, so the race needs no rejection handling; an unsettled
|
||||
// script past the grace is abandoned per the module contract.
|
||||
// Idempotent: cancel, then wait min(settle + child quiescence, grace).
|
||||
// `result` and `quiesce()` never reject, so the race needs no
|
||||
// rejection handling; a script or child still unsettled past the grace
|
||||
// is abandoned per the module contract.
|
||||
disposed ??= (async () => {
|
||||
execution.cancel('workflow disposed')
|
||||
await Promise.race([result, sleep(this.config.disposeGraceMs)])
|
||||
await Promise.race([
|
||||
(async () => {
|
||||
await result
|
||||
// The result settles with the SCRIPT; stray children a script
|
||||
// fired without awaiting are still winding down — dispose must
|
||||
// not return while they hold live resources.
|
||||
await execution.quiesce()
|
||||
})(),
|
||||
sleep(this.config.disposeGraceMs),
|
||||
])
|
||||
})()
|
||||
return disposed
|
||||
},
|
||||
|
||||
@@ -10,7 +10,13 @@
|
||||
* host containers, rejecting loud everything JSON cannot carry:
|
||||
* accessor properties, non-plain prototypes, functions, symbols (keys or
|
||||
* values), bigints, non-finite numbers, `undefined` values, cycles, sparse
|
||||
* arrays, and arrays with non-index own properties.
|
||||
* arrays, arrays with non-index own properties, and proxies. Proxies are
|
||||
* rejected via the trap-free native `util.types.isProxy` check BEFORE any
|
||||
* other inspection — a descriptor walk over a proxy would otherwise run its
|
||||
* realm-side traps (`ownKeys`, `getOwnPropertyDescriptor`, `getPrototypeOf`)
|
||||
* on the host stack, outside the vm's timed window, and a throwing trap would
|
||||
* escape as a raw realm error instead of a {@link MaterializeError}. The same
|
||||
* check guards the PROTOTYPE position (an object whose prototype is a proxy).
|
||||
*
|
||||
* Host objects are built with `Object.defineProperty` into a fresh `{}` —
|
||||
* never plain `target[key] =` assignment, which a `"__proto__"` key would turn
|
||||
@@ -24,6 +30,8 @@
|
||||
* @module @deepseek-ai/dsh-workflow-vm/realm
|
||||
*/
|
||||
|
||||
import { types } from 'node:util'
|
||||
|
||||
/** Thrown by {@link materializeFromRealm}; the caller wraps it into the right `WorkflowError` code. */
|
||||
export class MaterializeError extends Error {
|
||||
constructor(public readonly path: string, public readonly reason: string) {
|
||||
@@ -36,11 +44,13 @@ export class MaterializeError extends Error {
|
||||
* Whether an object's prototype chain is data-shaped: `null`, or a prototype
|
||||
* whose own prototype is `null` (the realm's `Object.prototype` — which we
|
||||
* cannot compare by identity across realms). A `Date`/`Map`/class instance
|
||||
* has a longer chain and is rejected.
|
||||
* has a longer chain and is rejected, as is a proxy sitting in the prototype
|
||||
* position (checked trap-free BEFORE its own prototype is dereferenced).
|
||||
*/
|
||||
function hasPlainPrototype(value: object): boolean {
|
||||
const proto: unknown = Object.getPrototypeOf(value)
|
||||
if (proto === null) return true
|
||||
if (types.isProxy(proto)) return false
|
||||
return Object.getPrototypeOf(proto) === null
|
||||
}
|
||||
|
||||
@@ -81,6 +91,11 @@ function materialize(value: unknown, path: string, seen: Set<object>): unknown {
|
||||
break
|
||||
}
|
||||
if (value === null) return null
|
||||
// BEFORE anything else touches the object: every inspection below —
|
||||
// Array.isArray aside — can trigger a proxy trap, running realm code on the
|
||||
// host stack (module doc). isProxy is a native internal-slot check (no
|
||||
// traps, catches revoked proxies, realm-agnostic).
|
||||
if (types.isProxy(value)) throw new MaterializeError(path, 'proxies cannot cross the workflow realm boundary')
|
||||
const objectValue: object = value
|
||||
if (seen.has(objectValue)) throw new MaterializeError(path, 'circular references are not JSON data')
|
||||
seen.add(objectValue)
|
||||
|
||||
@@ -9,8 +9,11 @@
|
||||
* descriptor walks; values ENTERING the realm from the host (`args`, agent()
|
||||
* results) are rebuilt INSIDE the realm through the context's own
|
||||
* `JSON.parse`, so the script never holds an object whose prototype chain
|
||||
* reaches host intrinsics. Realm functions (pipeline stages, parallel thunks)
|
||||
* are called, not materialized — their values stay realm-side.
|
||||
* reaches host intrinsics. The arrays `parallel`/`pipeline` resolve to are
|
||||
* realm-built for the same reason (their ELEMENTS are realm values already —
|
||||
* only the container needs rebuilding). Realm functions (pipeline stages,
|
||||
* parallel thunks) are called, not materialized — their values stay
|
||||
* realm-side.
|
||||
*
|
||||
* Failure discipline: fatal {@link WorkflowError}s (bad hook arguments,
|
||||
* unsupported options/schemas, tripped caps, seam start failures,
|
||||
@@ -132,7 +135,10 @@ export class WorkflowExecution {
|
||||
private currentPhase: string | undefined
|
||||
private readonly context: vm.Context
|
||||
private readonly realmJsonParse: (text: string) => unknown
|
||||
private readonly realmArrayFrom: (items: unknown[]) => unknown[]
|
||||
private readonly compiled: vm.Script
|
||||
/** Every live `agent()` call promise — awaited or stray — for {@link quiesce}. */
|
||||
private readonly inFlightAgents = new Set<Promise<unknown>>()
|
||||
|
||||
constructor(
|
||||
private readonly ctx: Context,
|
||||
@@ -162,9 +168,12 @@ export class WorkflowExecution {
|
||||
// The realm's own JSON.parse — the host→realm rebuild channel.
|
||||
const realmJson = vm.runInContext('JSON', this.context) as { parse(text: string): unknown }
|
||||
this.realmJsonParse = (text: string) => realmJson.parse(text)
|
||||
// The realm's own Array.from, bound NOW so a script reassigning its
|
||||
// globals later cannot swap it: combinator results must be realm arrays.
|
||||
this.realmArrayFrom = vm.runInContext('Array.from.bind(Array)', this.context) as (items: unknown[]) => unknown[]
|
||||
|
||||
const globals: Record<string, unknown> = {
|
||||
agent: (prompt: unknown, opts?: unknown) => this.contain(this.agent(prompt, opts)),
|
||||
agent: (prompt: unknown, opts?: unknown) => this.contain(this.track(this.agent(prompt, opts))),
|
||||
parallel: (thunks: unknown) => this.contain(this.parallel(thunks)),
|
||||
pipeline: (items: unknown, ...stages: unknown[]) => this.contain(this.pipeline(items, stages)),
|
||||
phase: (title: unknown) => { this.phase(title) },
|
||||
@@ -216,8 +225,15 @@ export class WorkflowExecution {
|
||||
*/
|
||||
async drive(): Promise<WorkflowResult> {
|
||||
try {
|
||||
// Cancelled before the body ever ran (an already-aborted start signal):
|
||||
// the script must not execute at all, let alone report `completed`.
|
||||
if (this.isCancelled()) throw this.cancelledError()
|
||||
const scriptPromise = this.compiled.runInContext(this.context, { timeout: this.limits.syncTimeoutMs }) as Promise<unknown>
|
||||
const raw: unknown = await this.contain(Promise.resolve(scriptPromise))
|
||||
// Cancelled while the body ran: a script that settled without touching
|
||||
// another hook (or without any) must still report `cancelled` — the
|
||||
// holder asked for cancellation and `completed` would be a lie.
|
||||
if (this.isCancelled()) throw this.cancelledError()
|
||||
const value = raw === undefined ? null : this.materializeResult(raw)
|
||||
return { value, stopReason: 'completed', agentsStarted: this.started }
|
||||
} catch (error: unknown) {
|
||||
@@ -245,6 +261,31 @@ export class WorkflowExecution {
|
||||
return promise
|
||||
}
|
||||
|
||||
/**
|
||||
* Register one `agent()` call promise for {@link quiesce} tracking; the
|
||||
* entry drops when the call fully settles (which is AFTER its child's
|
||||
* `dispose()` — the call wrapper disposes in its `finally`).
|
||||
*/
|
||||
private track<T>(promise: Promise<T>): Promise<T> {
|
||||
this.inFlightAgents.add(promise)
|
||||
const drop = (): void => { this.inFlightAgents.delete(promise) }
|
||||
promise.then(drop, drop)
|
||||
return promise
|
||||
}
|
||||
|
||||
/**
|
||||
* Settles once every `agent()` call — awaited or stray — has fully settled,
|
||||
* INCLUDING each child's `dispose()`. The reap in {@link drive}'s finally
|
||||
* aborts strays; this is the wait for those aborts to reach quiescence, so
|
||||
* the engine's `dispose()` cannot return while a child is still winding
|
||||
* down. Never rejects (the tracked promises' rejections are contained).
|
||||
*/
|
||||
async quiesce(): Promise<void> {
|
||||
while (this.inFlightAgents.size > 0) {
|
||||
await Promise.allSettled([...this.inFlightAgents])
|
||||
}
|
||||
}
|
||||
|
||||
private cancelledError(): WorkflowError {
|
||||
// cancel() arms cancelError before any caller can observe isCancelled()
|
||||
// === true; the fallback guards the type, not a reachable path.
|
||||
@@ -430,7 +471,7 @@ export class WorkflowExecution {
|
||||
}
|
||||
return thunk as () => unknown
|
||||
})
|
||||
return Promise.all(thunks.map(async (thunk) => {
|
||||
const settled = await Promise.all(thunks.map(async (thunk) => {
|
||||
try {
|
||||
return await thunk()
|
||||
} catch (error: unknown) {
|
||||
@@ -438,6 +479,9 @@ export class WorkflowExecution {
|
||||
return null
|
||||
}
|
||||
}))
|
||||
// The container must be a REALM array (module doc); the elements are
|
||||
// realm values already.
|
||||
return this.realmArrayFrom(settled)
|
||||
}
|
||||
|
||||
/** The `pipeline(items, ...stages)` hook: per-item stage chains, NO cross-stage barrier. */
|
||||
@@ -455,7 +499,7 @@ export class WorkflowExecution {
|
||||
}
|
||||
return stage as (previous: unknown, item: unknown, index: number) => unknown
|
||||
})
|
||||
return Promise.all(rawItems.map(async (item: unknown, index) => {
|
||||
const settled = await Promise.all(rawItems.map(async (item: unknown, index) => {
|
||||
let value: unknown = item
|
||||
try {
|
||||
for (const stage of stages) {
|
||||
@@ -469,6 +513,9 @@ export class WorkflowExecution {
|
||||
return null
|
||||
}
|
||||
}))
|
||||
// The container must be a REALM array (module doc); the elements are
|
||||
// realm values already.
|
||||
return this.realmArrayFrom(settled)
|
||||
}
|
||||
|
||||
private assertItemCap(length: number, hook: string): void {
|
||||
|
||||
Reference in New Issue
Block a user