Skip to content

fix(runtime): harden sandbox determinism, drain pending agents on failure - #30

Open
Mourey wants to merge 1 commit into
Michaelliv:mainfrom
Mourey:fix/runtime-hardening
Open

Mourey wants to merge 1 commit into
Michaelliv:mainfrom
Mourey:fix/runtime-hardening

Conversation

@Mourey

@Mourey Mourey commented Aug 14, 2026

Copy link
Copy Markdown

Summary

Code audit of the runtime surfaced four correctness issues in how workflow scripts are sandboxed and how results are typed. All fixes are covered by new regression tests (24 → 30 tests, npm test green: biome + tsc + unit).

1. Math.random aliasing bypasses the determinism check

assertDeterministicAst rejects literal Math.random() calls, but the sandbox receives the host Math object, so an alias slips straight through:

const f = Math.random   // not a CallExpression — passes the AST check
f()                     // real host Math.random runs

The sandbox now gets a Math shim that inherits every deterministic method from the host object but defines random as a throwing stub — the direct form is still caught at parse time with a nice error, and aliases now fail at runtime with the same message.

2. Parser allows Date.parse/Date.UTC, runtime crashes on them

The parser (and its tests) deliberately allows the deterministic Date statics, but Date was never added to the vm context, so any script using them died with ReferenceError: Date is not defined. The sandbox now gets a frozen Date shim exposing parse/UTC; now() throws, and since the shim is a plain object, new Date() is a TypeError even if the AST check is bypassed by aliasing.

3. Subagents leak when the script throws

runWorkflow only drained pendingAgentRuns on the success path. A script that fires un-awaited agent() calls and then throws left real LLM subagent sessions running detached — never awaited, never surfaced. The drain now happens in a finally, so pending runs are always settled before the error propagates.

4. types/workflow.d.ts contradicts documented null behavior

The runtime maps failed agent()/parallel()/pipeline() results to null, and the tool's own prompt guidelines tell the model to null-check — but the ambient types promised non-nullable results. The declarations now return T | null / Array<T | null> with doc comments explaining when.

Test plan

  • npm test — biome check, tsc build, 30/30 unit tests pass
  • New tests: aliased Math.random rejected at runtime; deterministic Math methods still work; Date.parse/Date.UTC work end-to-end; pending agents drained when the script throws; parallel preserves input order with nulls on failure; pipeline passes (prev, original, index) with nulls on failure

One note for reviewers: the new parallel-ordering test uses a Promise.withResolvers gate instead of a timer so completion order is deterministic under any concurrency level.

…lure

- Sandbox Math shim: random() throws even when aliased past the AST check
  (const f = Math.random; f()); all other methods inherited.
- Sandbox Date shim: parse()/UTC() now work at runtime, matching what the
  parser allows; now() throws and the shim is not a constructor.
- runWorkflow drains pending agent runs in a finally block so a script that
  throws after firing un-awaited agent() calls cannot leave real subagent
  sessions running detached.
- types/workflow.d.ts: agent()/parallel()/pipeline() results are nullable,
  matching the runtime's failure-to-null mapping and the tool guidelines
  that tell users to null-check.
- Regression tests for all of the above plus parallel ordering and
  pipeline (prev, original, index) semantics.
zhangys2 added a commit to zhangys2/pi-dynamic-workflows-michaelliv that referenced this pull request Aug 26, 2026
… runtime fixes

Takes Mourey's upstream hardening, which is orthogonal to everything on
this branch:

- Math shim whose random() throws, closing the alias hole (`const f =
  Math.random; f()`) the AST check cannot see.
- Frozen Date shim exposing parse()/UTC() while now() throws. The parser
  already allowed the deterministic Date statics, but Date was never in
  the vm context, so scripts using them died with a ReferenceError.
- types/workflow.d.ts now returns `T | null` / `Array<T | null>`, matching
  the runtime's documented failure behaviour.

Conflict resolutions:

- The script-execution block: kept our sync-execution timeout and adopted
  their drain-in-finally, nesting the timeout guard inside the try so a
  workflow that throws still settles its pending agents. Both fixes are
  live; neither supersedes the other.
- tests/workflow-runtime.test.ts: both sides appended tests, with no
  overlapping names or helpers, so both blocks are kept. The merge ate
  the closing brace of our last test where the two blocks abutted;
  restored.

tsconfig.check.json now sets lib ES2024. Their new parallel-ordering test
uses Promise.withResolvers, which Node 22 has but the ES2022 lib does not
declare. Upstream never saw this because its tsconfig covers src/** only
and does not typecheck tests. Type checking only; the published build
keeps the ES2022 target.

npm test green: biome, typecheck, build, 63 unit tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
zhangys2 added a commit to zhangys2/pi-dynamic-workflows-michaelliv that referenced this pull request Aug 26, 2026
Three of issue Michaelliv#11's six findings were already closed on this branch
(opts.model by PR Michaelliv#27, the determinism bypass by PR Michaelliv#30's Math shim, and
the token-budget undercount by the real-usage work). This covers what was
left that can be built.

Agent ceiling. A looping script could spawn subagents without bound: the
sync timeout cannot see `for(;;) await agent()` because it yields on every
turn, so the run continued forever. agent() calls are now counted against
maxAgents (default 1000). The count happens at call time rather than when
an agent starts, because an un-awaited fan-out queues every call before
any of them runs, so a started-agent count would not bound it. Unlike a
spent budget, which is per-agent and leaves earlier results worth
synthesizing, a breached ceiling is fatal: parallel() and pipeline()
re-throw it instead of mapping it to null.

Structured-output retry. A subagent that ended its turn without calling
structured_output failed outright. It now gets up to three turns, with a
correction between them; pi validates arguments before the tool runs, so
a schema-invalid call leaves the capture empty exactly like a skipped one
and is retried the same way. The retry policy is
promptForStructuredOutput, split from the session so it is testable
without a live agent. Token usage is now read after the retries rather
than after the first turn, so re-prompts are counted.

Nested workflow(). Scripts can call workflow(name, args) to run a saved
workflow inline; the child shares the parent's concurrency limiter, agent
ceiling, token budget and abort signal, and nests only one level deep.
Sharing required lifting those into a SharedRun passed down the call. The
fatal-breach flag lives in a by-reference object on it, so a ceiling
breach inside a child is fatal to a parent that wrapped it in parallel()
rather than being swallowed as a null.

Resolution is by name only, against the .pi/workflows and agent-directory
layout from epic Michaelliv#1. Workflow scripts are model-authored, so accepting a
path would let one read and execute any .js file on the machine; the
{ scriptPath } form waits for issue Michaelliv#3, which is what persists inline
scripts in the first place.

Not done, and why:

- isolation: 'worktree' (P1). This branch removed the option rather than
  implementing it, which was a deliberate call; re-adding real worktree
  isolation reverses that and needs a decision, not a patch.
- MCP tools for subagents (P2). Not implementable: pi 0.78.0 has no MCP
  support at all, so there are no session MCP tools to forward. The
  finding compares against Claude Code, which has them.

npm test green: biome, typecheck, build, 79 unit tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
HangxiangMa pushed a commit to HangxiangMa/pi-dynamic-workflows that referenced this pull request Sep 7, 2026
Two-pane (Phases | agents) navigator renderer with shared frame, ANSI-aware width handling, scroll windows, and narrow-terminal single-pane degrade. Includes workflow-tool/effort-command prompting refinements. Co-authored-by paulbrav.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant