feat(core): Workflow tool P1 — minimal node:vm sandbox + sequential agent() (#4721)

[LaZzyMan]

What this PR does

Implements P1 of the Dynamic Workflows / Ultracode port — a minimal Workflow tool that runs a model-authored JavaScript script in a node:vm sandbox with args, phase, log, and sequential agent() globals. The tool is registered behind isWorkflowsEnabled() (off by default; opt-in via setting or QWEN_CODE_ENABLE_WORKFLOWS=1).

Architecture is three layers with clean injection seams:

• WorkflowTool (packages/core/src/tools/workflow/workflow.ts) — BaseDeclarativeTool subclass that owns the tool registration, params validation, and error marshalling
• WorkflowOrchestrator + createProductionDispatch(config, signal?) (packages/core/src/agents/runtime/workflow-orchestrator.ts) — owns run lifecycle, dispatch construction, and wraps AgentHeadless for the production code path
• createWorkflowSandbox(opts) (packages/core/src/agents/runtime/workflow-sandbox.ts) — owns the node:vm context, hardened globals, and the determinism stubs (Date.now / Math.random throw)

P2/P3/P5 forward-compatibility seams are already in SandboxOptions (parallel?, pipeline?, budget?) so future phases can wire real implementations without re-opening the sandbox's already-reviewed security surface.

The subagent system prompt is verbatim from the claude-code 2.1.160 binary §XmO constant — five bullets including the "return ONLY the raw JSON" instruction critical for P3 schema mode.

Why it's needed

See #4721 for the full motivation and phased plan. TL;DR: this adds a third tier above the existing /swarm tool (#3433) and Agent Team (#2886, in progress) — model-authored JS that orchestrates many subagents through a small set of primitives, matching upstream's Agent + Workflow coexistence.

P1 is the minimum shippable skeleton:

• The Workflow tool is registered and gated; the JS sandbox executes scripts
• Sequential agent() only — parallel / pipeline / workflow / budget are throwing stubs that surface a clear "scheduled for PN" error (not ReferenceError)
• Date.now() / Math.random() / new Date() throw to guarantee resume determinism
• Foreground execution (no background queue yet — that's P4)
• No schema / structured output (P3) / no budget tracking (P5) / no resume (P6) / no UI extension (P4)

Local design artifacts (not committed; they live in .qwen/design/ per repo convention) document the full upstream alignment study:

• .qwen/design/dynamic-workflow-alignment-claude-code-2.1.160.md (788 lines) — per-axis upstream findings, qwen-code fit matrix, gap analysis
• .qwen/design/dynamic-workflow-alignment-claude-code-2.1.160-liveprobe-delta.md — API surface confirmation against the real /deep-research workflow script captured from ~/.claude/projects/<session>/
• .qwen/design/p1-implementation-plan.md — the task-by-task plan this branch executes

Reviewer Test Plan

How to verify

# 1. Run the new P1 tests in isolation (5 files, 80 tests):
cd packages/core && npx vitest run \
  src/agents/runtime/workflow-sandbox.test.ts \
  src/agents/runtime/workflow-orchestrator.test.ts \
  src/tools/workflow/workflow.test.ts \
  src/config/config.workflows.test.ts \
  src/config/config.workflow-registration.test.ts

# Expected: 80 tests pass.

# 2. Full core regression (363 files, 9920 tests):
cd packages/core && npm test

# 3. Type and lint:
cd packages/core && npm run typecheck
cd packages/core && npm run lint

# 4. Confirm the tool is gated by default:
QWEN_CODE_ENABLE_WORKFLOWS=  node -e '
  const { Config, ToolNames } = await import("./packages/core/dist/index.js");
  const cfg = new Config({ /* minimal */ });
  const reg = await cfg.createToolRegistry(undefined, { skipDiscovery: true });
  console.log("workflow registered?", reg.getAllToolNames().includes(ToolNames.WORKFLOW));
' 2>&1
# Expected: "workflow registered? false"
# Then with QWEN_CODE_ENABLE_WORKFLOWS=1 — expected "workflow registered? true"

Notable test categories under workflow-sandbox.test.ts:

• stripExportMeta — 8 cases covering brace balancing, string-literal escapes
• createWorkflowSandbox — args verbatim, Date.now/Math.random throw, top-level return capture
• createWorkflowSandbox security — 18 cases including realm-escape PoCs that have been confirmed-blocked against host process (covers args.constructor.constructor, Math.__proto__, Math.toString.constructor, Math.abs.constructor, Date.constructor, array-args [].constructor, budget.spent.constructor, the 30s vm timeout, log/phases caps, agent({schema/isolation/model/agentType}) throws, and the injection seams for parallel/pipeline/budget)

Evidence (Before & After)

N/A — no user-visible UI change in P1. The tool is registered but the model only emits a Workflow call when explicitly prompted; no Footer//workflows UI lands until P4.

Tested on

OS	Status
🍏 macOS	✅
🪟 Windows	⚠️ not tested
🐧 Linux	⚠️ not tested

Environment (optional)

Node v22.21.1, npm 10.x.

Risk & Scope

Main risk or tradeoff: the JS sandbox is node:vm (zero dep), not isolated-vm. node:vm is not a true V8 isolate — six prototype-escape paths were identified and blocked over six review rounds (PoCs in the security test file act as regressions). Two architectural limitations remain documented and not fixed:

• node:vm does not enforce a memory cap — a hostile script can OOM the host. Operators should configure --max-old-space-size; future phases may move to worker_threads.
• Synchronous regex catastrophic backtracking can block the event loop up to the 30s vm timeout. The 30s value matches the upstream binary's EW6 constant.

Neither limitation is unique to qwen-code — claude-code 2.1.160 (binary inspected) ships the same trade-off.

Not validated / out of scope:

• parallel / pipeline / workflow / budget are throwing stubs in P1 (clear "scheduled for PN" errors, not ReferenceError). Real implementations land in P2/P5 — injection seams in SandboxOptions are pre-wired so neither phase needs to modify the sandbox.
• agent({schema}) throws with a clear "scheduled for P3" message. Until P3 lands, scripts that rely on StructuredOutput (including upstream's /deep-research) will fail at the first such call — by design.
• No /workflows slash command or progress UI yet. The BackgroundTasksPill does not show workflow runs. All planned for P4.
• No resume / resumeFromRunId. Planned for P6.

Breaking changes / migration notes: None. The tool is off by default (isWorkflowsEnabled() === false unless setting or env var is set). All changes are additive; no existing tool, slash command, or config field is modified.

Decisions deferred to maintainer sign-off (listed in issue #4721 §"Decisions needed before P1"):

1. Sandbox choice: node:vm (P1) vs isolated-vm (potential P2+)
2. workflowKeywordTriggerEnabled default: false (qwen) vs true (upstream)
3. Whether to add QWEN_CODE_MAX_TOKENS_PER_WORKFLOW in P5
4. Saved workflows directory: .qwen/workflows/ vs .claude/workflows/
5. Cross-session resume scope in P6

Resolves the implementation half of #4721 — only P1 lands here; P2–P7 will be separate PRs per the phased plan.

[github-actions]

📋 Review Summary

This PR implements P1 of the Dynamic Workflows / Ultracode port — a minimal Workflow tool that executes model-authored JavaScript in a node:vm sandbox with sequential agent() dispatch. The implementation demonstrates exceptional security hardening with multiple defense-in-depth layers, comprehensive test coverage (80 tests across 5 files), and clean architectural separation between the tool, orchestrator, and sandbox layers. The code quality is outstanding with detailed inline documentation explaining security rationales.

🔍 General Feedback

• Exceptional security-first design: The sandbox implementation shows deep understanding of vm escape vectors with multiple independent mitigation layers (JSON roundtrip, deepNullProto, hardenInjected, vm-realm primordials)
• Clean architectural layering: Three clear layers (WorkflowTool → WorkflowOrchestrator → createWorkflowSandbox) with well-defined injection seams for future phases
• Outstanding test coverage: 80 tests covering security PoCs, functionality, and edge cases — the security tests document the exact attack vectors being blocked
• Excellent inline documentation: Comments reference specific FIX numbers, security classifications (SEC-C1, UP-C1), and phase schedules (P1/P2/P3/P5)
• Forward-compatible design: Injection seams for parallel/pipeline/budget are already in place without compromising P1 security surface
• Verbatim upstream alignment: Subagent system prompt is copied from claude-code 2.1.160 binary with documentation of the source

🎯 Specific Feedback

🔵 Low

• File: packages/core/src/agents/runtime/workflow-sandbox.ts:~280 — The deepNullProto function has a depth cap of 64 with a clear error message, which is good. However, consider whether 64 is the right default — a deeply nested but legitimate args object could hit this. Consider documenting what typical nesting depth looks like in practice, or making this configurable via SandboxOptions.

• File: packages/core/src/agents/runtime/workflow-sandbox.ts:~400 — The MAX_LOG_LINES and MAX_PHASE_ENTRIES caps (both 10,000) are reasonable, but consider whether these should be tuned based on expected workflow behavior. A workflow that logs 10,000 lines is already pathological, so this seems safe, but documenting the memory impact (~10KB-1MB depending on line length) would help future reviewers.

• File: packages/core/src/tools/workflow/workflow.ts:147 — The error handling correctly suppresses stack traces from the LLM, but the known limitation note about scripts that capture e.stack and return it is worth documenting in user-facing docs as well. Consider adding this to the tool's description string or a separate security guidance doc.

• File: packages/core/src/config/config.ts:3193 — The feature gate logic is clear, but the precedence order (env var kill switch → env var enable → params.workflowsEnabled) could use a comment documenting why this order was chosen. The test covers it, but a future maintainer might wonder about the rationale.

• File: packages/core/src/agents/runtime/workflow-orchestrator.ts:17 — The WorkflowRunRequest interface has a good comment explaining why signal was removed, but consider whether this pattern (removing unused fields with documentation) should be called out in a contributing guide as a best practice.

✅ Highlights

• Security hardening is exemplary: The multi-layer approach to blocking vm escape (JSON roundtrip → deepNullProto → hardenInjected → vm-realm primordials) represents best-in-class secure coding. Each layer is independently documented with the specific attack vector it blocks.

• Test-driven security: The security PoC tests (TST-C1/C2/C3) that verify each escape vector is blocked demonstrate a mature security mindset. Tests like "blocks args.constructor.constructor realm escape" and "blocks Math.abs.constructor host-Function escape" are textbook examples of how to test security boundaries.

• Clear phase scheduling: Every unsupported feature (parallel, pipeline, budget, schema, workflow nesting) has a throwing stub with a clear message indicating which phase it's scheduled for. This prevents semantic drift from silent no-ops.

• Excellent error surfacing: Errors from the sandbox surface only the message (never stack traces with host paths) to the LLM, while still preserving full debuggability via stderr/logging. The distinction between user-facing and debug-facing error info is correctly implemented.

• Comprehensive exclusion list: Adding WORKFLOW to EXCLUDED_TOOLS_FOR_SUBAGENTS prevents unbounded recursive fan-out — a subtle but critical security decision that shows systems-level thinking.

• Type-safe forward compatibility: The WorkflowAgentOpts interface documents unsupported fields with clear runtime errors rather than silent dropping, and the type is re-exported from the orchestrator so external consumers get the same type definition.

[LaZzyMan]

Cross-check against upstream (@anthropic-ai/claude-code 2.1.160) — drift notes

Re-checked P1 implementation + phase plan against the local reverse-engineering artifacts. Three findings worth flagging:

1. Subagent disallowedTools missing (minor defense-in-depth gap)

Upstream Tg8 config (packages/core/src/agents/runtime/workflow-orchestrator.ts:71):

{ tools: ['*'], disallowedTools: [SendUserMessage, ExitPlanMode] }

P1 dispatch currently passes only { tools: ['*'] }. The §XmO system prompt explicitly says "Do NOT use SendUserMessage" so behavioral compliance comes from prompt discipline alone, not tool gating. Upstream uses both layers (prompt + tool block). Recommend adding disallowedTools: [ToolNames.SEND_USER_MESSAGE, ToolNames.EXIT_PLAN_MODE] to the AgentHeadless.create() call site in createProductionDispatch — a 1-line change. Holding for reviewer steer on whether to fold into this PR or land separately in P2.

2. runId format diverges from upstream

P1 generates wf_<16 hex chars> from randomBytes(8).toString('hex').
Upstream generates wf_<12 chars> from randomUUID().slice(0,12).

No functional impact today, but cross-tool interop (e.g. P6 resume reading a runId emitted by claude-code) would break. Cheap to align if maintainers prefer.

3. Documentation correction — schema retry count (already fixed in issue #4721)

The original issue #4721 said "Schema-mismatch retries: 5 per agent". This conflated two separate counters in the binary:

• Schema-mismatch nudges: 2 in-conversation nudges (binary line 307789 — the literal error message text)
• Stall retries: 5 (binary VOK = 5, fires on no-progress agents, not schema validation)

Issue #4721 has been edited to correct both the "Hard caps" section and the P3 phase description ("2-nudge in-conversation retry"). The fix is documentation-only — no code in this PR is affected since P1 throws on agent({schema}) rather than implementing retries.

4. P4 plan refinement — meta extraction (already fixed in issue #4721)

P1 uses stripExportMeta to remove export const meta = {...} because Node's vm script mode rejects ES-module syntax. P4 needs the meta fields (name, description, whenToUse, phases) for the /workflows UI, so P4 must replace stripExportMeta with extractAndStripMeta(source) → { meta, body } and thread meta to the task registry. Issue #4721 P4 description has been updated to call this out explicitly. No P1 code change needed.

---

The first two items are flagged for reviewer judgment (fold in here vs. defer). The third and fourth are documentation-only corrections to the issue, no PR change required.

[wenshao]

Fresh review pass at HEAD e767902. CI is green and this is not a self-PR, so no verdict downgrade applies.

Three new Critical issues are posted inline, plus three Suggestions. The headline is a host-realm escape on the success path of agent() (the always-on twin of the thrown-Error leak already flagged at workflow-sandbox.ts:293) — I confirmed it with a PoC that read process.env from inside a sandbox script with no error thrown.

For the record, the three earlier Critical inline comments still reproduce at this HEAD and were each re-verified by PoC — they block merge alongside the new findings:

• sandbox escape via thrown host Error objects (workflow-sandbox.ts:293)
• deepNullProto severing array prototypes so for...of / .map / spread on array args throw (workflow-sandbox.ts:175)
• eager value-export of WorkflowTool vs export type siblings (index.ts:119)

Good things confirmed: typecheck + lint + the 80 new tests all pass, the build is green, the node:vm Math/Date in-realm init is genuinely not subvertible, __proto__ args keys do not pollute host Object.prototype, and the anti-recursion guard (WORKFLOW in EXCLUDED_TOOLS_FOR_SUBAGENTS) holds on the tools: ['*'] path.

— claude-opus-4-8 via Claude Code /qreview

[wenshao]

⚠️ Second-opinion review (qwen3.7-max) — R1 findings by claude-opus-4-6 are acknowledged. This round found 1 new Critical + 5 new Suggestions.

Also noted (Nice to have, not posted as inline):

• 6 new files missing @license headers (workflow-sandbox.ts, workflow-orchestrator.ts, workflow-prompts.ts, and 3 test files). Only workflow.ts and the two config test files have the standard header.
• Stale comment in config.workflows.test.ts:10 and config.workflow-registration.test.ts:10: references config-session-env.test.ts which does not exist in the codebase.

— qwen3.7-max via Qwen Code /review

[LaZzyMan]

E2E test report — real model (qwen3.7-plus)

Built locally from lazzy/lucid-pare-974192 HEAD and driven through node packages/cli/dist/index.js -p ... --approval-mode yolo --output-format json with QWEN_CODE_ENABLE_WORKFLOWS=1 set (except scenario 0). Raw outputs captured under /tmp/p1-e2e/scenario-*.json. Model: qwen3.7-plus. Total LLM cost across all 7 scenarios ≈ 200k input tokens.

Scenario 0 — Default-off gate

qwen -p "say hi" --output-format json   # no env var

init.tools did not contain workflow. ✅ Default-off gate works as designed.

Scenario 1 — Gate-on registration

QWEN_CODE_ENABLE_WORKFLOWS=1 qwen -p "say hi" --output-format json
| jq '.[] | select(.type=="system") | .tools | map(select(.=="workflow"))'
# → ["workflow"]

workflow appears in the init.tools list. ✅

Scenario 2 — Real sequential agent() dispatch (end-to-end)

Prompt: ask model to invoke workflow with an inline script that calls phase('plan') + two await agent(...) + returns a structured object.

Model emitted (raw tool_use.input):

{"name":"workflow","input":{"script":"\nphase('plan');\nconst a = await agent('Respond with exactly the literal text HELLO and nothing else', { label: 'h1' });\nconst b = await agent('Respond with exactly the literal text WORLD and nothing else', { label: 'h2' });\nreturn { greeting: a + ' ' + b, runIdShape: 'wf_<16hex>' };\n"}}

Tool result (tool_result.content):

{
  "runId": "wf_ea903721a899b1a9",
  "phases": ["plan"],
  "logs": [],
  "result": { "greeting": "HELLO WORLD", "runIdShape": "wf_<16hex>" }
}

Verified:

• ✅ runId matches ^wf_[0-9a-f]{16}$
• ✅ phases: ["plan"] — phase('plan') was honored
• ✅ Two real AgentHeadless subagents fired (telemetry stats.models.bySource keys: ["h1", "h2", "main"])
• ✅ Subagents returned HELLO and WORLD as instructed by the §XmO system prompt ("Output the literal result")
• ✅ Script-side concatenation a + ' ' + b → "HELLO WORLD" made it through vm.runInContext
• ✅ llmContent carries the script's raw result (not the wrapper); main-loop model summarized it as "greeting: HELLO WORLD" — confirming the FIX-7 (UP-C2) unwrap behaviour

Stats: 2 main-loop turns, 75 168 input / 256 output tokens, 10 001 ms wall-clock (includes 2 subagent LLM round-trips).

Scenario 3 — Date.now() throws (binary §axO verbatim)

{"is_error": true, "content": "Error: Date.now() / new Date() are unavailable in workflow scripts (breaks resume). Stamp results after the workflow returns, or pass timestamps via args."}

✅ Exact binary verbatim message. is_error: true propagates.

Scenario 4 — Math.random() throws (binary §sxO verbatim)

{"is_error": true, "content": "Error: Math.random() is unavailable in workflow scripts (breaks resume). For N independent samples, include the index in the agent label or prompt."}

✅ Exact binary verbatim message.

Scenario 5 — agent({schema}) throws with P3 scheduling note

{"is_error": true, "content": "Error: agent({schema}) is not supported in P1. Schema enforcement / StructuredOutput contract is scheduled for P3."}

✅ Clear "scheduled for P3" message — NOT ReferenceError. Per Round 1 review decision: throw is safer than silently dropping the opt, even though it means scripts like /deep-research won't run until P3 ships.

Scenario 6 — parallel() throws with P2 scheduling note

{"is_error": true, "content": "Error: parallel() is not supported in P1. Sequential agent() is the only execution mode in P1. Concurrent fan-out is scheduled for P2."}

✅ Clear "scheduled for P2" — NOT ReferenceError. Confirms the FIX-F throwing-stub approach reaches the model correctly.

Scenario 7 — pipeline() throws with P2 scheduling note

{"is_error": true, "content": "Error: pipeline() is not supported in P1. Staggered multi-stage execution is scheduled for P2."}

✅ Same pattern as parallel.

---

Summary matrix

#	Behaviour	Result
0	Default-off gate (no env var)	✅ tool absent from init.tools
1	QWEN_CODE_ENABLE_WORKFLOWS=1 gate	✅ workflow in init.tools
2	Sequential phase() + 2× agent() + structured return	✅ runId, phases, subagent dispatch, llmContent unwrap all verified end-to-end
3	Date.now()	✅ throws — binary §axO verbatim
4	Math.random()	✅ throws — binary §sxO verbatim
5	agent({schema})	✅ throws — "scheduled for P3"
6	parallel(...)	✅ throws — "scheduled for P2"
7	pipeline(...)	✅ throws — "scheduled for P2"

All 7 P1-scope behaviours verified against a real model on real hardware. The 80 unit tests + 9920 core regression already in the PR cover the implementation seams; this report covers the integration path the user actually sees: model → tool description → tool_use → sandbox → subagent dispatch → tool_result → main-loop summary.

PR moving from draft → ready for review.

[LaZzyMan]

Round 1 — all 21 findings addressed

Commits (4): eba53fbad · 7ca5f82cb · 4b53619e5 · b787c9448

Full P1 test suite: 108 passed (was 80 — 28 new regression cases). Full core suite: 9960 passed / 4 skipped / 0 failed. typecheck: 0 errors. lint: clean on workflow files (only pre-existing unrelated errors in .claude/skills/e2e-testing/scripts/ remain).

Critical (all 4 unique roots fixed; 7 inline threads closed)

Root issue	Fix	Commit
Host-realm escape via thrown Errors (T1) and async Promises (T8/T14) — confirmed PoC read process.env and process.platform	All async/sync globals (agent, parallel, pipeline, workflow, budget, console, phase, log, args) are now built inside the vm-realm via the existing init script. Host exposes only a primitive bridge that the init script reads once and deletes from globalThis. Both resolve and reject cross the boundary as vm-realm values	eba53fbad
Array iteration broken (T2) — for...of, .map, .filter, spread, destructuring all threw on array args because Object.setPrototypeOf(out, null) removed Array.prototype	Args are built from vm-realm JSON.parse, retaining vm-realm Array.prototype (whose constructor chain still cannot reach host process)	eba53fbad
WorkflowTool value export defeats lazy loading (T3)	Changed to export type to align with AgentTool/SkillTool siblings	7ca5f82cb
stripExportMeta silently deletes script on apostrophe-in-comment / unbalanced braces / regex literal (T9/T16/T17)	Parser now recognises //, /* */, regex literals; throws on unbalanced braces instead of returning ''	eba53fbad
Cancellation silently non-functional (T10) — runReasoningLoop returns terminateMode rather than throwing	createProductionDispatch calls getTerminateMode() after execute() and throws on non-GOAL	4b53619e5

Suggestions (all 12 fixed)

Thread	Fix	Commit
T4 — missing ToolErrorType.EXECUTION_FAILED assertion	added in workflow.test.ts	4b53619e5
T5 — missing console.log/warn/error → getLogs() routing test	added	eba53fbad
T6 — missing non-serializable args validation test	validateArgs() rejects functions/BigInt/circular/depth; 3 regressions added	eba53fbad
T7 — vi as vitest alias inconsistent	replaced with vi	4b53619e5
T11 — workflow subagents unbounded + over-privileged	runConfig: { max_turns: 50, max_time_minutes: 10 } + disallowedTools: [SEND_MESSAGE, EXIT_PLAN_MODE] (mirrors upstream Tg8)	4b53619e5
T12 / T18 — BigInt / circular masquerades as workflow failure	safeStringifyResult / safeStringifyDisplayPayload with their own try/catch + clear placeholder	4b53619e5
T13 — [key:string]: unknown defeats opts guard	runtime allowlist inside vm-realm agent() wrapper throws on any opt not in [label, phase, schema, model, isolation, agentType]	eba53fbad
T15 — budget injected hardening untested	regression "opts.budget: spent/remaining constructors stay vm-realm-safe" added	eba53fbad
T19 — phases/logs lost on script failure	WorkflowExecutionError carries phases + logs through the rejection; surfaced in returnDisplay	4b53619e5

Nits (review-body)

Thread	Fix	Commit
T20 — 6 new files missing @license headers	added Copyright 2025 Qwen header (matches sibling tools/agent/agent.ts style)	b787c9448
T21 — stale mirrors config-session-env.test.ts reference	removed dangling reference from both config test files	b787c9448

Architecture note

The unified vector for T1/T8/T14 is "any host object handed to the script keeps its host prototype chain." Round 1's fix moves the cross-realm boundary to a primitive-only host bridge: the init script reads a host object exposed at globalThis.__workflowBridge, deletes the binding, and builds every script-visible global (Math, Date, args, agent, parallel, pipeline, workflow, budget, console, phase, log) inside the vm-realm. Closures (prompt) => bridge.hostAgent(prompt) cross via primitive args + primitive returns; vm-realm new Promise((res, rej) => { hostPromise.then(res, rej) }) wraps the host async result; vm-realm new Error(msg) wraps host rejection messages. The script never sees a host object on either the success or failure path.

The same pattern also lets P2's parallel(thunks) and P5's real budget slot in without re-opening the security surface: their host implementations get wired through __workflowBridge and the init-script wrappers stay unchanged.

[LaZzyMan]

@wenshao friendly ping for re-review when you have a moment — all 42 review threads across R1–R5 are now resolved, CI 8/8 green at HEAD 610c4005b. The R5 finding (createChildAbortController reuse) was already landed in a2fee2cfe before your review reached me; GitHub auto-marked it isOutdated. PR is currently BLOCKED on the active CHANGES_REQUESTED — could you re-review or dismiss when convenient? Happy to address anything new that comes up.

[wenshao]

✅ Local runtime verification — P1 Workflow tool

I verified this PR end-to-end at runtime (not just the unit suite) on the true post-merge state, as a merge reference.

Setup — tested the merged tree, not the branch in isolation

The branch is 38 commits behind current main, and main independently changed config.ts by +148 lines since the branch point (the same file this PR edits for tool registration). So I merged the PR head into origin/main@6a99ed150 first — clean auto-merge, no conflicts — and ran everything against that merged tree. Node v22, TypeScript 5.8.3.

1. Unit suites — 118 / 118 pass

npx vitest run  (5 workflow suites)
 ✓ workflow-sandbox.test.ts              (79)
 ✓ workflow-orchestrator.test.ts         (17)
 ✓ workflow.test.ts                      (14)
 ✓ config.workflow-registration.test.ts   (4)
 ✓ config.workflows.test.ts               (4)
 Tests  118 passed (118)    Duration 32.4s

2. Typecheck — all 13 changed files clean

tsc --noEmit on packages/core reports zero diagnostics in any file this PR touches.
_{(The only diagnostic emitted is a pre-existing environmentContext.test.ts(599,1) TS1005 — a file this PR does not modify; its blob is byte-identical to origin/main (b4768393). Reproduces in isolation, so it's a standing main property unrelated to this PR — flagging separately for awareness.)}

3. Real runtime exercise (node:vm, driven in tmux) — 33 / 33 checks pass

I drove the actual WorkflowOrchestrator + createWorkflowSandbox with real model-style scripts and an injected mock dispatch (no LLM needed), so this exercises the real sandbox, not mocks:

Area	Verified
Sequential agent()	A → B call order; B's prompt receives A's output (chaining); args / phase / log captured in order; runId = wf_<hex>; script return value passed through verbatim
Determinism	Date.now(), new Date(), Math.random() all throw (resume-safety guarantee)
P1 gating	parallel(), pipeline(), workflow(), agent({schema|model|isolation|agentType}), budget.spent() all throw with clear "scheduled for P2/P3/P5" messages; an unknown agent() opt (typo scema) is rejected by the runtime allowlist
Sandbox isolation	globalThis.constructor.constructor('return process')(), thrown-Error realm escape, and agent-result realm escape are all blocked (process is not defined); process / require are undefined inside the sandbox
export const meta	leading meta is stripped and the body runs; an unbalanced-brace meta is refused (throws, no silent truncation)
Robustness	non-JSON args (a function) is rejected; a script failure preserves phases / logs and surfaces a clean message (no realm-leaked Error: prefix); the async wall-clock timeout fires at 800 ms and aborts the linked abortOnTimeout controller

4. Gating (off by default) — confirmed by the config suites

Not registered unless isWorkflowsEnabled(); QWEN_CODE_ENABLE_WORKFLOWS=1 enables it; QWEN_CODE_DISABLE_WORKFLOWS=1 is a kill-switch that overrides; ToolNames.WORKFLOW is in EXCLUDED_TOOLS_FOR_SUBAGENTS, so a subagent can't recursively invoke the workflow.

Verdict

✅ Functionally verified at runtime. The node:vm sandbox, sequential dispatch, determinism guarantees, P1 forward-compat stubs, and the host-realm isolation boundary all behave as designed on the merged tree. No functional blockers found; safe to merge from a runtime-verification standpoint. (Scope: behavior/runtime verification — not a full line-by-line security audit of the sandbox.)

[DragonnZhang]

Code Review: PR #4732 — feat(core): Workflow tool P1

Deterministic checks

Check	Result
TypeScript (tsc --noEmit)	PASS (clean)
ESLint (8 changed source files)	PASS (clean)
Tests (5 workflow test files, 118 tests)	PASS (118/118)

Overall assessment

This is a well-engineered PR with thorough security hardening, comprehensive test coverage, and clean three-layer architecture (WorkflowTool / WorkflowOrchestrator / createWorkflowSandbox). The six rounds of iterative security fixes addressing node:vm prototype-escape vectors are impressive — the PoC regression tests in workflow-sandbox.test.ts are some of the best sandbox-hardening test coverage I have seen in a PR of this scope.

No blocking issues found. Five non-blocking observations below.

Findings (5 non-blocking)

F1. [Security - Suggestion] No script size cap in validateToolParamValues

validateToolParamValues checks only that script is a non-empty string but imposes no maximum length. While mitigated by the ask permission gate and model output token limits, a defense-in-depth cap (e.g., 256KB) would prevent a hostile or hallucinating model from submitting a multi-megabyte script that pressures the vm parser or the stripExportMeta brace walker.

// packages/core/src/tools/workflow/workflow.ts — validateToolParamValues
const MAX_SCRIPT_LENGTH = 256_000; // ~256KB
if (params.script.length > MAX_SCRIPT_LENGTH) {
  return `WorkflowTool: script exceeds max length of ${MAX_SCRIPT_LENGTH} characters.`;
}

Not blocking — the opt-in feature gate and ask permission provide meaningful mitigation already.

F2. [Security - Informational] Async microtask leak after wall-clock timeout

After Promise.race rejects on wall-clock timeout, an in-script async microtask loop (e.g., (async () => { while(true) await 0 })()) continues consuming microtasks on the host event loop. node:vm provides no mechanism to halt async execution once started. The PR documents this limitation and correctly identifies worker_threads isolation as the proper fix for a future phase.

Acceptable for P1 given the opt-in gate and the 30-minute default cap. Should be tracked as a known limitation for the issue (#4721).

F3. [Security - Informational] No memory cap for vm context

node:vm does not enforce a memory limit. A script like const a=[]; while(true) a.push(new ArrayBuffer(1e8)) can OOM the host process. The PR documents this and suggests --max-old-space-size as an operator mitigation. Same disposition as F2 — acceptable for P1, tracked for future worker_threads migration.

F4. [Correctness - Nit] Copyright header inconsistency

config.workflows.test.ts and config.workflow-registration.test.ts carry Copyright 2025 Google LLC while the other six new workflow source/test files carry Copyright 2025 Qwen. Cosmetic only — worth aligning if there is a project convention on copyright holder.

F5. [Performance - Informational] Per-call dynamic import in dispatch closure

createProductionDispatch does await import('./agent-headless.js') inside every dispatch invocation. Node caches module resolution after the first import, so subsequent calls resolve in a single microtask tick. Negligible overhead in practice. No action needed — the dynamic import pattern is load-bearing for test mockability.

Reverse audit (things NOT changed that might need attention)

• EXCLUDED_TOOLS_FOR_SUBAGENTS in agent-core.ts: Correctly updated with ToolNames.WORKFLOW. Anti-recursion test in config.workflow-registration.test.ts locks this in.
• tool-names.ts: Both ToolNames.WORKFLOW and ToolDisplayNames.WORKFLOW added. No migration mapping needed (new tool).
• index.ts barrel export: Changed to export type only — avoids eager loading of the node:vm module chain. Correct.
• config.ts: Feature gate follows the established isForkSubagentEnabled pattern (env var + settings + kill switch). Registration uses registerLazy. Correct.

Summary

Axis	Verdict
Correctness	PASS — architecture is sound, cross-realm Error handling is correct, AbortController chain (caller -> child -> dispatch -> subagent) is well-designed
Security	PASS with notes — six rounds of prototype-escape hardening with PoC regression tests; node:vm limitations are honestly documented; opt-in gate is the primary defense
Performance	PASS — no hot-path concerns; log/phase caps bound memory; wall-clock timer is unref'd
Test coverage	STRONG — 118 tests across 5 files covering security PoCs, edge cases, error paths, and injection seams

LGTM with optional improvements (F1 script size cap being the most actionable).

[LaZzyMan]

R6 + R7 summary — HEAD a008fd648

Two reviews in this batch, both positive on PR readiness:

@wenshao — runtime verification (top-level comment above)

Thanks for the end-to-end local verification on the merged tree (118/118 unit + 33/33 real-runtime checks + sandbox isolation PoCs). The verdict "✅ Functionally verified at runtime. Safe to merge from a runtime-verification standpoint" is the most rigorous external check this PR has received and addresses any "did the unit tests actually exercise the integration?" doubt directly.

The 4 inline findings (T45-T48) are all [Suggestion]-class and have been replied to individually:

#	File	Disposition
T45	sandbox.ts:478 (Object.keys allowlist)	❌ out of scope — explicit throws cover P1 surface (you noted this); preemptive hardening belongs in P2/P3 PR
T46	sandbox.ts:36 (stripExportMeta leading comments)	❌ out of scope — reverses my R4 T33 design decision (clean SyntaxError as constraint signal)
T47	orchestrator.ts:46 (stale cause JSDoc)	❌ out of scope — R6+ inline-doc polish defaults to overthinking; folded into future doc-pass
T48	workflow.test.ts:57 (external-abort layer test)	❌ out of scope — R6+ test-coverage additions default to overthinking; helper-contract tests already in utils/abortController.test.ts

@DragonnZhang — LGTM with optional improvements

Thanks for the independent code review. The "LGTM with optional improvements" plus the deterministic checks (tsc clean / eslint clean / 118/118) is great signal alongside @wenshao's runtime verification.

Disposition of F1-F5:

#	Disposition
F1 (script size cap)	❌ out of scope — hostile-provider hardening; you noted opt-in gate + ask permission are the meaningful mitigation already
F2 (async microtask leak)	✅ documented as known P1 limitation in #4721 (#4721) — worker_threads migration tracked for future phase
F3 (vm memory cap)	✅ documented as known P1 limitation in #4721 alongside F2
F4 (copyright header inconsistency)	✅ fixed in a008fd648 — both config.workflows.test.ts and config.workflow-registration.test.ts now Copyright 2025 Qwen to match the other 6 PR-new files
F5 (per-call dynamic import)	✅ acknowledged — your assessment ("No action needed — dynamic import is load-bearing for test mockability") is correct

---

Status: HEAD a008fd648, CI 8/8 green, all 46 review threads across R1-R7 resolved, known P1 limitations documented in parent #4721. Pending: re-review or CHANGES_REQUESTED dismissal to unblock merge.

[qwen-code-ci-bot]

Qwen Code review did not complete successfully: Qwen review timed out after 55 minutes. See workflow logs.

[wenshao]

[Suggestion] subagent.getFinalText() can return '' (empty string) on GOAL mode — the dispatch silently returns "" with no warning.

Looking at agent-core.ts, finalText starts as '' and is only updated when a round produces text output. If the model returns GOAL with no text (tool-only execution, content-filtered response, empty output), the script receives "" from await agent("..."). A subsequent JSON.parse(agentResult) or string operation would throw a confusing error with no indication that the root cause was an empty agent response.

Suggested change

	return subagent.getFinalText();
	const text = subagent.getFinalText();
	if (!text) {
	throw new Error(
	`Workflow subagent '${opts?.label ?? 'workflow-agent'}' completed with GOAL mode but returned empty text. This usually indicates a model-side issue (content filter, empty response, or tool-only execution).`,
	);
	}
	return text;

— qwen3.7-max via Qwen Code /review

[wenshao]

[Suggestion] No upper bound on script size. The only check is script.length === 0 — a multi-megabyte model-generated script is accepted and passed to new vm.Script(), which compiles synchronously and is NOT covered by the 30s vm timeout (that only applies to runInContext).

A model hallucination loop could produce a 5MB+ script that blocks the event loop during V8 compilation with no timeout and no abort signal.

Suggested change

	if (typeof params.script !== 'string' || params.script.length === 0) {
	if (typeof params.script !== 'string' || params.script.length === 0) {
	return 'WorkflowTool: `script` parameter is required and must be a non-empty string.';
	}
	const MAX_SCRIPT_LENGTH = 256_000;
	if (params.script.length > MAX_SCRIPT_LENGTH) {
	return `WorkflowTool: script is ${params.script.length} chars, exceeds limit of ${MAX_SCRIPT_LENGTH}. This usually indicates a model generation loop.`;
	}

— qwen3.7-max via Qwen Code /review

[wenshao]

[Suggestion] import * as vm from 'node:vm' appears after ~120 lines of code (two exported functions: stripExportMeta and isRegexContext). Every other file in the codebase — including the two sibling files added in this PR — clusters all imports at the top, immediately after the license header.

ESM hoists imports so this works at runtime, but it is visually inconsistent. Move to line 7, directly below the license header.

— qwen3.7-max via Qwen Code /review

[wenshao]

[Suggestion] workflowsEnabled is the only mutable (non-readonly) feature flag on Config. Every other *Enabled field (cronEnabled, forkSubagentEnabled, computerUseEnabled, experimentalZedIntegration) is private readonly. The setWorkflowsEnabled setter is the only public setter for any feature flag on the entire Config class.

This breaks Config's post-construction immutability contract. If a caller mutates this flag mid-session, isWorkflowsEnabled() could return a different value than what the tool registry was built with (constructed once at startup via registerLazy).

Make workflowsEnabled readonly like its siblings. Tests that need workflowsEnabled: true can pass it via ConfigParameters (as the existing "honors workflowsEnabled passed via ConfigParameters" test already demonstrates).

— qwen3.7-max via Qwen Code /review

[wenshao]

[Suggestion] await import('./agent-headless.js') is inside the returned closure, so every agent() call re-executes the dynamic import(). Node's module cache prevents re-loading from disk, but each call still goes through the async module-resolution pipeline. Every other consumer (subagent-manager.ts, background-agent-resume.ts) uses a static import.

Hoist to the factory level so it resolves once per createProductionDispatch call:

Suggested change

	const { AgentHeadless, ContextState } = await import('./agent-headless.js');
	export function createProductionDispatch(
	config: Config,
	signal?: AbortSignal,
	): WorkflowAgentDispatch {
	const modulePromise = import('./agent-headless.js');
	return async (prompt, opts) => {
	const { AgentHeadless, ContextState } = await modulePromise;

This preserves the test-mock property (dynamic import lets mocks swap the module) while eliminating repeated resolution.

— qwen3.7-max via Qwen Code /review

[wenshao]

[Suggestion] Missing test for dispatchController.abort() in the finally block at workflow.ts:174. Every test passes new AbortController().signal but never inspects whether the internal child controller created via createChildAbortController is actually aborted after execution.

A regression that removes the finally { dispatchController.abort() } block would pass all existing tests. In production this leaks: every successful workflow would leave the dispatch controller un-aborted, meaning any straggler subagent keeps burning tokens until its own max_time_minutes limit.

Add a test that captures the dispatch signal and asserts signal.aborted === true after execute() resolves on both success and failure paths.

— qwen3.7-max via Qwen Code /review

[wenshao]

Verification Report — local runtime build & drive

Verdict: PASS ✅ (one documentation/wiring note below)

Built the branch from a clean checkout and drove the real bundled CLI in tmux, exercising the Workflow tool through the model at its actual surface (tool-call → node:vm sandbox → sequential agent() dispatch). This is runtime observation, not a test re-run.

Environment

• Linux (Debian 13, kernel 6.12), Node v22.22.2, npm 10.9.7 — complements the macOS-only matrix in the PR (Linux was marked ⚠️ not tested).
• Worktree off PR head a008fd648; real npm ci + npm run bundle → dist/cli.js (Qwen Code v0.17.1, glm-4.7 backend). Build clean.

What was verified (all at the live CLI surface)

1. Gating — off by default
/tools with no env var → 29 tools, no Workflow. ✅

2. Gating — opt-in via env var
QWEN_CODE_ENABLE_WORKFLOWS=1 → /tools now lists Workflow (30 tools). ✅

3. Kill switch
QWEN_CODE_ENABLE_WORKFLOWS=1 QWEN_CODE_DISABLE_WORKFLOWS=1 → Workflow absent again. The disable flag correctly wins over enable. ✅

4. Happy path — single sequential agent
Script phase('Greet'); log('starting'); const reply = await agent('…PONG…'); …; return { agentSaid: reply, ok: true }. Tool prompted for approval (getDefaultPermission: 'ask' confirmed), then returned:

{ "runId": "wf_31ebe7f9247fa960", "phases": ["Greet","Wrap"],
  "logs": ["starting","agent replied: PONG"],
  "result": { "agentSaid": "PONG", "ok": true } }

Real subagent dispatched and its value flowed back into the script. The LLM received the unwrapped result; the full metadata went to the display payload. ✅

5. Headline feature — two agents in order
Script with phase('A'); one = await agent('…1'); phase('B'); two = await agent('…2'); …; return {first, second} →

{ "runId": "wf_3663a65961a670d4", "phases": ["A","B"],
  "logs": ["got 1 then 2"], "result": { "first": "1", "second": "2" } }

Two subagents executed sequentially, phases and log captured in order. ✅

Probes (off the happy path)

• 🔍 Date.now() → Date.now() / new Date() are unavailable in workflow scripts (breaks resume)… — exact determinism error, message only, no stack leak. ✅
• 🔍 parallel([...]) → parallel() is not supported in P1. … Concurrent fan-out is scheduled for P2. — clean P2 stub, not a ReferenceError. ✅
• 🔍 agent('hi', {schema:{…}}) → agent({schema}) is not supported in P1. … scheduled for P3. — clean P3 stub. ✅
• 🔍 Realm escape — args.constructor.constructor('return process')() with args={x:1} → process is not defined. The vm-realm Function constructor cannot reach the host process. Sandbox boundary holds. ✅
• 🔍 Syntax error — malformed return ((( → Unexpected token '}', surfaced as a graceful tool error (no crash). ✅

Findings

• ⚠️ opt-in via setting is not wired in the bundled CLI. The PR text says workflows are enabled "via setting or QWEN_CODE_ENABLE_WORKFLOWS=1". The env-var path works (verified live). But there is no experimental.workflows field in settingsSchema.ts and no line mapping workflowsEnabled from settings in packages/cli/src/config/config.ts — its siblings cronEnabled (settings.experimental?.cron) and emitToolUseSummaries are wired there; workflows is not. Config.workflowsEnabled / setWorkflowsEnabled() exist for programmatic embedders, but a user editing settings.json cannot currently turn the tool on through the CLI. Not a blocker for P1 (the security-critical default-off property holds and the env var works), but either the one-line settings mapping should be added or the description should say "env var only" until then.
• 👍 Error hygiene is consistently good: every failure path surfaced only the message (no vm stack frames) to both the LLM and the display, matching the duck-typed extractErrorMessage intent.
• 👍 The subagent-recursion guard is real — WORKFLOW is in EXCLUDED_TOOLS_FOR_SUBAGENTS, so the spawned agent() can't re-enter Workflow.

Out of scope (per PR, not exercised)

pipeline/workflow/budget stubs, resume, /workflows UI, background queue — all deferred to P2–P7 and explicitly not in this PR.

---

Verified on Linux via a tmux-driven bundled CLI against the real model backend. Evidence = the tool-result payloads captured above, reproduced verbatim from the running app.

[tanzhenxin]

Second approval based on the converged review history: the multi-round sandbox-escape findings were all addressed, wenshao's runtime verification passed (118/118 unit, 33/33 runtime, isolation PoCs), and the feature is off by default so the blast radius is contained.