Skip to content

Commit a962749

Browse files
LaZzyMandoudouOUC
authored andcommitted
feat(core): Workflow P3 — agent({schema, agentType, model, isolation:'worktree'}) (#4721) (#5034)
* feat(core): Workflow P3 — agent({schema, agentType, model, isolation:'worktree'}) (#4721) Adds the P3 dispatch options to the workflow runtime, completing the contract qwen-code's workflow tool matches against upstream Claude Code 2.1.168. P1/P2 stubs (workflow-sandbox.ts:508-527) are replaced with production paths routed through `SubagentManager.createAgentHeadless` so per-call model overrides go through `buildRuntimeContentGeneratorView` (provider routing), per-agent MCP servers / hooks get isolated lifecycles, and worktree-isolated subagents run against a rebound Config. - agent({agentType: 'X'}) resolves against the declarative-agents registry (#4842 + #4996) via findSubagentByName; unresolved names throw "agent({agentType}): agent type 'X' not found" verbatim from upstream. - agent({model: 'qwen3-max'}) is threaded into SubagentConfig.model so the runtime view sees it (modelConfigOverrides alone would only swap the model name within the existing provider's view). - Workflow's disallowed-tool floor [SendMessage, ExitPlanMode] is unioned with the agentType's own disallowedTools so a permissive agentType cannot re-enable them for a workflow subagent. - agent({isolation: 'worktree'}) provisions a fresh worktree via GitWorktreeService.createUserWorktree (slug agent-<7hex>, mirrors AgentTool 1849-1963), rebinds cwd/getTargetDir/getFileService/ getWorkspaceContext on a prototype-chained Config override, and on completion auto-removes the worktree if clean or preserves the path + branch (appended to the result string) when the subagent left changes. Parent-dirty trees are refused with a clear error to avoid silently running the subagent against a stale HEAD. - agent({isolation: 'remote'}) throws "agent({isolation:'remote'}) is not available in this build" verbatim (upstream 2.1.168 parity). - agent({schema: S}) injects a per-call SyntheticOutputTool (existing tools/syntheticOutput.ts, AJV-backed) into a fresh per-subagent ToolRegistry built via rebuildToolRegistryOnOverride, then watches AgentEventEmitter TOOL_CALL/TOOL_RESULT events for `structured_output` invocations. A successful call's args are captured as the dispatch return value (object, not string); after two failed attempts the third failure aborts the dispatch and throws "subagent completed without calling StructuredOutput (after 2 in-conversation nudges)" verbatim. No agent-core.ts changes — the entire 2-nudge counter lives in the dispatch layer so the shared subagent loop is unaffected. The sandbox's agent() wrapper now revives per-call object returns into the vm realm (JSON round-trip inside the vm runInContext block), closing the same T1/T8/T14 host-prototype-escape vector that P2's per-element revival closed for parallel/pipeline. Two new sandbox security tests (constructor-chain probe + non-JSON-serializable collapse) regress this. WorkflowAgentResult widens from `string` to `string | object`; the fast-path (no agentType/model/isolation/schema) is preserved byte-for-byte to keep P1/P2 zero-overhead. Tests: 159 workflow-suite tests + 217 adjacent (subagents / syntheticOutput / agent-override) all green. Real-LLM E2E follow-up planned (mirroring P2's 13/13 qwen3-max validation). Related #4721 (parent design — multi-phase, not closed by this PR) Related #4732 (P1 merged) #4947 (P2 merged) #4842 #4996 (declarative agents) * chore(core): P3 self-review R1 — align worktree suffix wording + 6 test gaps R1 of pre-push adversarial self-review on PR #5034 surfaced 6 confirmed findings across 6 diverse lenses (correctness / security / reuse-altitude / self-invariant / consumer-breakage / test-gaps). Each finding faced 2 independent skeptics defaulting to refuted=true; 6 survived majority challenge. Source code: - Worktree-preserved suffix wording now matches AgentTool's formatWorktreeSuffix (agent.ts:1700-1719) verbatim, including the `git worktree add <path> <branch>` recovery hint for the directory- removed-but-branch-preserved race. Test gaps closed: - schema-mode success after 1 nudge (round-2 args captured) - schema-mode success after 2 nudges (round-3 args captured) - schema-mode + agentType together — floor disallowedTools still unioned - schema-mode caller-abort takes priority over the StructuredOutput terminal error (signal.aborted check at workflow-orchestrator.ts:489-490) - override path dispose() runs in finally on the success path - override path dispose() runs in finally on the terminate-mode-error path Declined R1 finding: negative tests for invalid opt types (schema/model/ agentType passed null/number/empty-string). Adding upfront type validation is scope creep — upstream does not, P1/P2 do not, and the workflow tool is model-authored where these inputs are extremely unlikely. Existing AJV / SubagentManager downstream errors are descriptive enough. Will revisit if R2 makes a stronger case. 166/166 tests pass (workflow suite + adjacent + workflow-orchestrator). typecheck + lint clean across packages/core, packages/cli, integration-tests, sdk, webui. * chore(core): P3 self-review R2 — vm-realm opts revive + error-msg sanitize + 12 tests R2 of pre-push adversarial self-review on PR #5034. 6 diverse-lens finders (60 agents, ~2.5M tokens, 24 min) over the R1-fix-applied code, with 2 independent skeptics defaulting to refuted=true. 12 confirmed survivors after adversarial verify; decisions below. Security (FIX): - agent() wrapper in workflow-sandbox.ts now JSON-revives agentOpts inside the vm runInContext block BEFORE passing them to the host dispatch. Closes a Proxy/inherited-getter escape that P3 introduced along with the user-supplied schema object: a script could have wrapped agentOpts.schema in a Proxy whose getter ran host-side code during SyntheticOutputTool construction / AJV compile. Same mechanism as args / parallel-result revival. - runOverridePath now sanitizes opts.agentType through sanitizeForErrorMessage() (control chars → space) before interpolation into the "agent type 'X' not found" error message. Prevents a model-authored agentType containing CRLF / NUL from fragmenting a single-line error across log records / OTLP fields. Reuse-altitude (FIX): - Added JSDoc block to WorkflowWorktreeIsolation interface documenting each field's role for cleanup. Test gaps (FIX, 12 new tests): - agentType control-char sanitization regression - dispose() runs in finally when subagent.execute throws - isolation:'worktree' provision error branches (5): nested parent / git unavailable / not a git repo / parent dirty / createUserWorktree returns failure - isolation:'worktree' cleanup branches (3): removeUserWorktree fails / branchPreserved race / removeUserWorktree throws — each preserves the worktree (or branch) with the right user-facing suffix - combinations (2): model + isolation:'worktree' threads model AND provisions worktree; schema + isolation:'worktree' returns structured payload verbatim (preserved suffix only on string return) Test infrastructure: vi.mock'd GitWorktreeService at the module level (partial mock; preserves the existing exports the unrelated worktreeCleanup.ts depends on) with a per-test beforeEach reset. Declined R2 findings (kept the R1 line): - [major] Schema parameter upfront validation: same scope-creep decline as R1. Upstream doesn't do it; AJV's downstream error is descriptive enough. - [major] Worktree provision extracted to shared util with AgentTool: agreed in principle but out of P3 scope. A separate refactor PR should land that with AgentTool maintainers in the loop. 178/178 tests pass (workflow + adjacent suites). typecheck + lint clean across packages/core, packages/cli, integration-tests, sdk, webui. * fix(core): address wenshao R1+R2 review on Workflow P3 (PR #5034) Round 1 (15:41) + Round 2 (17:24) review from wenshao surfaced 7 inline findings across schema-mode dispatch correctness, worktree cleanup coverage, and error attribution. Each fix is paired with a regression test that was RED before the change landed. T0 [Critical] Worktree leak when schema setup throws after provision workflow-orchestrator.ts: outer try MOVED to start immediately after provisionWorkflowWorktree. Previously the try opened only after createSchemaConfigOverride / createSchemaModeState / signal listener attachment — so any throw in those three (broken MCP server during the per-call ToolRegistry rebuild was the trigger wenshao cited) orphaned the just-provisioned worktree under .qwen/worktrees/. Test: "isolation:'worktree' + schema setup throws → worktree is still cleaned up" — simulates createToolRegistry failure during createSchemaConfigOverride; asserts removeUserWorktree was called. T1 [Critical] / T4 [H1] agentType + schema silently dead-ended workflow-orchestrator.ts: schema-mode augmented config now (a) appends ToolNames.STRUCTURED_OUTPUT to baseConfig.tools when the allowlist is restricted (no '*' and doesn't already contain it), so prepareTools / getFunctionDeclarationsFiltered doesn't filter structured_output out of the subagent's surface; (b) preserves the resolved agentType's persona by APPENDING the schema-contract instruction block instead of replacing the systemPrompt outright. Replace remains only on the ephemeral no-agentType path where baseConfig.systemPrompt IS WORKFLOW_SUBAGENT_SYSTEM_PROMPT (schema variant is its strict superset; avoids two near-identical prompts). Tests: structured_output appears in the allowlist alongside the agentType's existing tools; persona prompt is contained in the effective systemPrompt. T2 [Suggestion] / T5 [M1] Parent-abort listener leaked per schema call workflow-orchestrator.ts: named listener stored at outer scope, removed in the outer finally regardless of how the dispatch ended. Previous `{ once: true }` only auto-removed on actual parent abort; the happy-path schema dispatch — success capture / 3-failure abort fires the CHILD controller without the parent ever aborting — left the listener stuck on the per-run signal. With N schema calls per workflow N listeners + N child-controller closures accumulated. Test: 5 sequential schema dispatches over the same parent signal end with zero live listeners. T6 [M2] Terminate mode misdiagnosed as nudge exhaustion workflow-orchestrator.ts: schema path now distinguishes terminateMode before attributing failure to schema mode. TIMEOUT / MAX_TURNS / ERROR throw the existing "did not complete (terminate mode: X)" message that the non-schema path uses. Only the actual schema-failure cases produce schema wording, and those are split: attempts > 2 keeps the upstream-verbatim "(after 2 in-conversation nudges)" wording; attempts === 0 throws an accurate "no validation attempt — model produced plain-text content" instead of misleadingly citing nudges that never happened. (The existing 0-call test was updated to match the new accurate message; the 3-failure test retains the verbatim wording.) Tests: parametric over TIMEOUT/MAX_TURNS/ERROR asserting "did not complete"; companion test pinning the verbatim wording to the 3-failure path. T3 [Suggestion] Schema-mode JSON revival sentinel — clarified workflow-sandbox.ts: added a block comment documenting that the JSON-round-trip + null-on-throw is a SECURITY backstop (errors-as-data convention from parallel/pipeline) rather than a contract path — unreachable in production schema mode because the host return is LLM tool_call args, always JSON-serializable. No behavior change. Tests: 75/75 orchestrator + 111/111 sandbox/tool/limiter green. typecheck + lint clean across packages/core and packages/cli. R1+R2 self-review commits (e1c5ec7 / 62624a9) precede this commit on the same branch — they predate wenshao's review and address distinct findings; reviewer L1 (worktree-lifecycle unit coverage) is already closed by R2's 11 worktree tests.
1 parent f753bd3 commit a962749

7 files changed

Lines changed: 2346 additions & 81 deletions

File tree

0 commit comments

Comments
 (0)