workflow: simplify to the trust premise; settle result on cancellation

Two review responses that belong together — the same review argued the
engine was defending the wrong threat while a benign-input bug wedged
the product.

1) Drop hostile-value containment; state the trust premise.

Scripts are model-written — the same trust level as the model's bash
access — yet successive pre-push review rounds had ratcheted in defenses
that only matter against an adversarial author: trap-free proxy
rejection, accessor-never-invoked descriptor walks, realm-side
pre-rendering of thrown values, realm-built promises/arrays/error clones
with structural fatal recognition. That same author keeps a documented,
accepted, unkillable event-loop spin, so containing its error VALUES is
cost without a threat model — and the planned hardened engine
(worker/isolated-vm) gets value isolation by serialization and deletes
all of this machinery anyway.

What stays, because benign scripts hit it constantly: result never
rejects; dropped hook promises cannot become unhandled rejections; the
value boundary rejects LOUD everything JSON cannot carry (now a plain
recursive walk — getters are read ordinarily and their result is what
crosses; a throwing read fails loud); a "__proto__" key still copies as
a data property; the fatal-vs-null combinator discipline (now host
instanceof — unforgeable from the realm and simpler than clone-shape
recognition). What changes for scripts (documented in the engine
README): hooks hand back host values and host errors — in-script
`instanceof Error` on a hook failure is false (branch on e.name/e.code)
— and args are host-cloned once so a script cannot mutate the caller's
object. realm.ts drops 289 → 173 lines; the hostile-value test tables go
with it. The premise now leads the engine module doc, the README, and
the RFC's engine section, with the removed machinery recorded under
What was rejected.

2) result settles within the dispose grace of a cancellation.

Review finding (verified through the real registry + tool + engine): a
script parked on a promise no hook owns — `await new Promise(() => {})`,
`await Promise.race([])`, a returned never-settling thenable — could not
be settled by cancel(): hooks reject and children abort, but nothing
touches a promise the engine does not own, so `result` stayed pending
FOREVER (the previous cut even pinned that as intended). The tool awaits
run.result BEFORE its disposing finally, the registry awaits the tool,
the loop awaits the registry — one such script wedged the whole agent
turn past any abort, unrecoverable in-process; the mock engine in the
tool's abort test settles result on cancel, which is exactly the
behavior the real engine lacked, so no existing test could see it.

The seam contract now says it out loud: once a run is cancelled, result
SETTLES within the implementation's bounded grace even if the script
never does. The vm engine arms an abandon channel in cancel(); drive()
races the script against it, force-settling 'cancelled' at the grace
(the abandoned settlement stays contained; a post-slice synchronous spin
remains the documented limitation). dispose()'s outer race now exists
for child quiescence only, and `workflow/end` again fires exactly once
per started run. The old 'result stays pending' pin is FLIPPED to the
new contract (the pinned behavior was the bug); new regressions cover
cancel-then-settle on a parked script, a never-settling returned
thenable, and the full composition through the REAL registry + tool +
vm engine (tool-workflow gains workflow-vm/subagent devDeps for it).
agentsStarted JSDoc clarified while touching the vocabulary (accepted
calls, including ones still queued at cancellation).
This commit is contained in:
Tianyi Cui
2026-07-06 00:48:49 +08:00
parent 7234d41b91
commit 2accf85714
16 changed files with 352 additions and 609 deletions

View File

@@ -1,6 +1,6 @@
import { describe, expect, it } from 'vitest'
import * as vm from 'node:vm'
import { materializeFromRealm, MaterializeError, describeThrown, thrownRendering } from '../src/realm.ts'
import { materializeFromRealm, MaterializeError, renderThrown } from '../src/realm.ts'
/** Evaluate an expression inside a fresh vm realm and hand back the raw realm value. */
function inRealm(expression: string): unknown {
@@ -35,17 +35,21 @@ describe('materializeFromRealm', () => {
expect(rejection(inRealm('{ a: undefined }'))).toContain('value.a')
})
it('never invokes accessors: a counting getter is rejected, not read', () => {
it('invokes getters ordinarily — the getter RESULT is what crosses (trust premise)', () => {
const counter = inRealm(`
(() => {
globalThis.reads = 0
return { get x() { globalThis.reads += 1; return 1 } }
return { get x() { globalThis.reads += 1; return globalThis.reads } }
})()
`)
expect(rejection(counter)).toContain('accessor properties cannot cross')
// The getter body never ran — descriptor inspection only.
expect((counter as { x?: unknown }).x).toBe(1) // sanity: reading DOES run it…
expect(rejection(counter)).toContain('accessor') // …but materialization still never did
expect(materializeFromRealm(counter)).toEqual({ x: 1 })
})
it('a getter that THROWS surfaces as a MaterializeError carrying the rendered failure', () => {
const hostile = inRealm("{ get x() { throw new Error('read failed') } }")
const message = rejection(hostile)
expect(message).toContain('reading the value threw')
expect(message).toContain('read failed')
})
it('a "__proto__" key becomes an OWN data property of the copy, never a prototype mutation', () => {
@@ -81,39 +85,18 @@ describe('materializeFromRealm', () => {
expect(materializeFromRealm(inRealm('Object.assign(Object.create(null), { a: 1 })'))).toEqual({ a: 1 })
})
it('rejects proxies (root, nested, revoked, host-realm) WITHOUT running any trap', () => {
const trapped = inRealm(`new Proxy({ a: 1 }, {
ownKeys() { throw new Error('trap ran') },
getOwnPropertyDescriptor() { throw new Error('trap ran') },
getPrototypeOf() { throw new Error('trap ran') },
})`)
// A trap firing would surface 'trap ran' (a non-MaterializeError) instead.
expect(rejection(trapped)).toContain('proxies cannot cross')
expect(rejection(inRealm('{ nested: new Proxy([], {}) }'))).toContain('value.nested')
const revoked = inRealm('(() => { const r = Proxy.revocable({}, {}); r.revoke(); return r.proxy })()')
expect(rejection(revoked)).toContain('proxies cannot cross')
expect(rejection(new Proxy({}, {}))).toContain('proxies cannot cross')
})
it('rejects an object whose PROTOTYPE is a proxy without dereferencing through it', () => {
const value = inRealm(`Object.create(new Proxy({}, {
getPrototypeOf() { throw new Error('trap ran') },
}))`)
expect(rejection(value)).toContain('exotic prototype')
})
it('rejects cycles and accepts the same object reused as a sibling (a DAG)', () => {
expect(rejection(inRealm('(() => { const o = {}; o.self = o; return o })()'))).toContain('circular')
const dag = inRealm('(() => { const leaf = { v: 1 }; return { a: leaf, b: leaf } })()')
expect(materializeFromRealm(dag)).toEqual({ a: { v: 1 }, b: { v: 1 } })
})
it('rejects sparse arrays, accessor elements, and non-index array properties', () => {
it('rejects sparse arrays and non-index array properties; an array getter element materializes its value', () => {
expect(rejection(inRealm('[1, , 3]'))).toContain('sparse')
expect(rejection(inRealm('(() => { const a = [1]; Object.defineProperty(a, 0, { get: () => 1 }); return a })()')))
.toContain('accessor')
expect(rejection(inRealm('(() => { const a = [1]; a.total = 3; return a })()')))
.toContain('non-index')
expect(materializeFromRealm(inRealm('(() => { const a = [1]; Object.defineProperty(a, 0, { get: () => 7, enumerable: true }); return a })()')))
.toEqual([7])
})
it('skips non-enumerable own properties (matching JSON.stringify exactly)', () => {
@@ -134,45 +117,29 @@ describe('materializeFromRealm', () => {
})
})
describe('describeThrown (host-side thrown-value rendering)', () => {
it('renders a HOST Error via its identity-verified native stack getter', () => {
const error = new Error('host failure')
const rendered = describeThrown(error)
expect(rendered).toContain('host failure')
expect(rendered).toContain('at ') // a real stack, not just the message
})
it('never invokes a REALM error stack getter (identity mismatch) — message renders instead', () => {
describe('renderThrown', () => {
it('prefers the stack, for host and realm errors alike', () => {
const host = renderThrown(new Error('host failure'))
expect(host).toContain('host failure')
expect(host).toContain('at ') // a real stack, not just the message
const realmError: unknown = vm.runInNewContext('(() => { try { throw new Error("realm failure") } catch (e) { return e } })()')
expect(describeThrown(realmError)).toBe('realm failure')
expect(renderThrown(realmError)).toContain('realm failure')
})
it('reads a data-property stack directly and falls through a setter-only accessor', () => {
expect(describeThrown({ stack: 'data stack' })).toBe('data stack')
const setterOnly = { message: 'via message' }
Object.defineProperty(setterOnly, 'stack', { set() { /* swallow */ } })
expect(describeThrown(setterOnly)).toBe('via message')
it('falls back from stack to message to String()', () => {
expect(renderThrown({ stack: 'custom data stack' })).toBe('custom data stack')
const stackless = new Error('stackless failure')
delete stackless.stack
expect(renderThrown(stackless)).toBe('stackless failure')
expect(renderThrown({ code: 42 })).toBe('[object Object]')
expect(renderThrown('plain')).toBe('plain')
expect(renderThrown(42)).toBe('42')
expect(renderThrown(undefined)).toBe('undefined')
expect(renderThrown(null)).toBe('null')
})
it('labels proxies and functions without touching them; primitives stringify', () => {
expect(describeThrown(new Proxy({}, { getOwnPropertyDescriptor() { throw new Error('trap ran') } }))).toBe('[thrown proxy]')
expect(describeThrown(() => 1)).toBe('[thrown function]')
expect(describeThrown('plain')).toBe('plain')
expect(describeThrown(42)).toBe('42')
expect(describeThrown(undefined)).toBe('undefined')
expect(describeThrown(null)).toBe('null')
expect(describeThrown({ code: 42 })).toBe('[object Object]')
})
})
describe('thrownRendering (the realm-catch wrapper reader)', () => {
it('extracts the pre-rendered string from a wrapper and nothing else', () => {
expect(thrownRendering({ __wfThrown: 'rendered text' })).toBe('rendered text')
expect(thrownRendering({ __wfThrown: 42 })).toBeUndefined()
expect(thrownRendering({ other: 'x' })).toBeUndefined()
expect(thrownRendering(new Error('plain'))).toBeUndefined()
expect(thrownRendering('string')).toBeUndefined()
expect(thrownRendering(null)).toBeUndefined()
expect(thrownRendering(new Proxy({ __wfThrown: 'forged' }, { getOwnPropertyDescriptor() { throw new Error('trap ran') } }))).toBeUndefined()
it('is total: a value whose accessors/toString throw renders as a fixed label', () => {
expect(renderThrown({ get stack() { throw new Error('nope') } })).toBe('[unrenderable thrown value]')
expect(renderThrown({ [Symbol.toPrimitive]() { throw new Error('nope') } })).toBe('[unrenderable thrown value]')
})
})