refactor(core): move fork subagent params from execute() to construction time#3255
Merged
Merged
Conversation
…ion time Identity-shaping fork inputs (parent history, generationConfig, tool decls, env-skip flag) were threaded through `AgentHeadless.execute()`'s options bag and re-passed by the SubagentStop hook retry loop. They belong on the agent's construction-time configs, not its per-invocation options. - PromptConfig gains `renderedSystemPrompt` (verbatim, bypasses templating and userMemory injection) and drops the `systemPrompt`/`initialMessages` XOR so fork can carry both. createChat skips env bootstrap when `initialMessages` is non-empty. - AgentHeadless.execute() shrinks to (context, signal?). Fork dispatch in agent.ts builds synthetic PromptConfig/ModelConfig/ToolConfig from the parent's cache-safe params and calls AgentHeadless.create directly (bypassing SubagentManager). Parent's tool decls flow through verbatim including the `agent` tool itself for cache parity. - Recursive-fork prevention switches from fork-side tool stripping to a runtime guard. The previous `isInForkChild(history)` helper was dead code (it scanned the main GeminiClient's history, not the fork child's chat). Replaced with `isInForkExecution()` backed by AsyncLocalStorage: the fork's background execution runs inside `runInForkContext`, and the ALS frame propagates through the standard async chain into nested AgentTool.execute() calls where the guard fires.
Collaborator
Author
E2E Test Report — Fork Subagent Refactor Safety NetTest plan: Binary: Results
¹ T4 was run via the headless alternative explicitly endorsed by the test plan ("Avoids the tmux race"). The interactive tmux variant of T4 with the current proxy model collapses the nested prompt structure and does not exercise the recursive-fork path — same model behavior pre- and post-refactor, not a code regression. Expected failure-mode shift in T4 (intentional)T4's accepted error string changed from Unit tests
InvestigationRecursive-fork block dead-code discovery and the ALS fix are journaled at |
Contributor
📋 Review SummaryThis PR refactors the fork subagent dispatch path introduced in #2936, moving identity-shaping fork inputs from 🔍 General Feedback
🎯 Specific Feedback🔴 Critical
🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
…ectory Move agent.ts, agent.test.ts, and fork-subagent.ts under tools/agent/ and update all import paths accordingly.
These fields were never populated from subagent frontmatter and served no purpose in the fork path either. The ModelConfig interface retains only the actively-used model field.
…CacheSafeParams Fork subagent now reads system instruction and tool declarations from the live GeminiChat via getGenerationConfig() instead of the global getCacheSafeParams() snapshot. This removes the cross-module coupling between the agent tool and the followup infrastructure.
82f5569 to
e1e6885
Compare
…ly inline decls prepareTools() treated asStrings.length === 0 as "add all registry tools", which is correct when no tools are specified at all, but wrong when the caller provides only inline FunctionDeclaration[] (no string names). The fork path passes parent tool declarations as inline decls for cache parity, so prepareTools was adding the full registry set on top — duplicating every non-excluded tool. Add onlyInlineDecls.length === 0 to the condition so that pure-inline toolConfigs bypass the registry entirely.
…t-construction-time # Conflicts: # packages/core/src/core/client.ts # packages/core/src/tools/agent/agent.ts
tanzhenxin
added a commit
that referenced
this pull request
Apr 16, 2026
…ion-time' into feat/background-subagent Resolve conflict in agent.ts: adopt #3255's runSubagentWithHooks method and fork dispatch, add background execution path before the fork/normal dispatch with its own hook firing and fire-and-forget pattern.
LaZzyMan
requested changes
Apr 17, 2026
LaZzyMan
left a comment
Collaborator
There was a problem hiding this comment.
Downstream dead-code cleanup needed
This PR correctly removes temp/top_p from ModelConfig and strips the options bag from AgentHeadless.execute(). However, two downstream call sites that depend on this repo are left with stale code that silently becomes dead after merge.
1. packages/core/src/utils/forkedAgent.ts — two dead fields on AgentPathParams
/** AgentHeadless path: multi-turn, full tool access, isolated session. */
export interface AgentPathParams {
...
/** Sampling temperature (default: 0 for deterministic output). */
temp?: number; // ← dead: ModelConfig.temp was removed in this PR
...
skipEnvHistory?: boolean; // ← dead: execute() no longer accepts this option
...
}tempis declared but never forwarded toModelConfig(which no longer has the field anyway).skipEnvHistorywas previously threaded intoexecute(options), butexecute()now takes no options. The field is accepted silently and does nothing.
Suggested fix: Remove both fields from AgentPathParams.
2. packages/core/src/memory/extractionAgentPlanner.ts — passing a field that no longer exists
const result = await runForkedAgent({
name: 'managed-auto-memory-extractor',
...
extraHistory,
skipEnvHistory: true, // ← silently ignored: AgentPathParams no longer has this field
});Since AgentPathParams.skipEnvHistory is removed (per point 1), this line is a no-op. The intent — skipping env bootstrap when extraHistory is provided — is now handled automatically by initialMessages being non-empty (per agent-core.ts line 248), so the line should simply be deleted.
Suggested fix: Remove the skipEnvHistory: true line from the runForkedAgent call.
Both fixes are mechanical one-liners; happy to contribute them as a follow-up if preferred.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
…PathParams These fields were carried over from earlier designs but have no remaining effect after the fork subagent refactor: - `temp` was never forwarded into ModelConfig, which this PR already stripped of the temperature field. - `skipEnvHistory` is redundant with the auto-skip in `AgentCore.createChat`, which already bypasses env bootstrap whenever `initialMessages` is non-empty — the condition under which any caller would set this flag. Also drops the corresponding `skipEnvHistory: true` at the one caller in the memory extraction planner.
Collaborator
Author
|
Thanks for the careful trace @LaZzyMan — both points are valid, so I folded the cleanup into this PR rather than a follow-up. Pushed in 706f71a:
As you noted, the env-bootstrap skip is already handled automatically by |
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
…ion time (QwenLM#3255) * refactor(core): move fork subagent params from execute() to construction time Identity-shaping fork inputs (parent history, generationConfig, tool decls, env-skip flag) were threaded through `AgentHeadless.execute()`'s options bag and re-passed by the SubagentStop hook retry loop. They belong on the agent's construction-time configs, not its per-invocation options. - PromptConfig gains `renderedSystemPrompt` (verbatim, bypasses templating and userMemory injection) and drops the `systemPrompt`/`initialMessages` XOR so fork can carry both. createChat skips env bootstrap when `initialMessages` is non-empty. - AgentHeadless.execute() shrinks to (context, signal?). Fork dispatch in agent.ts builds synthetic PromptConfig/ModelConfig/ToolConfig from the parent's cache-safe params and calls AgentHeadless.create directly (bypassing SubagentManager). Parent's tool decls flow through verbatim including the `agent` tool itself for cache parity. - Recursive-fork prevention switches from fork-side tool stripping to a runtime guard. The previous `isInForkChild(history)` helper was dead code (it scanned the main GeminiClient's history, not the fork child's chat). Replaced with `isInForkExecution()` backed by AsyncLocalStorage: the fork's background execution runs inside `runInForkContext`, and the ALS frame propagates through the standard async chain into nested AgentTool.execute() calls where the guard fires. * refactor(core): move agent tool files into dedicated tools/agent/ directory Move agent.ts, agent.test.ts, and fork-subagent.ts under tools/agent/ and update all import paths accordingly. * refactor(core): remove dead temp and top_p fields from ModelConfig These fields were never populated from subagent frontmatter and served no purpose in the fork path either. The ModelConfig interface retains only the actively-used model field. * refactor(core): read parent generation config directly instead of getCacheSafeParams Fork subagent now reads system instruction and tool declarations from the live GeminiChat via getGenerationConfig() instead of the global getCacheSafeParams() snapshot. This removes the cross-module coupling between the agent tool and the followup infrastructure. * fix(core): prevent duplicate tool declarations when toolConfig has only inline decls prepareTools() treated asStrings.length === 0 as "add all registry tools", which is correct when no tools are specified at all, but wrong when the caller provides only inline FunctionDeclaration[] (no string names). The fork path passes parent tool declarations as inline decls for cache parity, so prepareTools was adding the full registry set on top — duplicating every non-excluded tool. Add onlyInlineDecls.length === 0 to the condition so that pure-inline toolConfigs bypass the registry entirely. * refactor(core): remove dead temp and skipEnvHistory fields from AgentPathParams These fields were carried over from earlier designs but have no remaining effect after the fork subagent refactor: - `temp` was never forwarded into ModelConfig, which this PR already stripped of the temperature field. - `skipEnvHistory` is redundant with the auto-skip in `AgentCore.createChat`, which already bypasses env bootstrap whenever `initialMessages` is non-empty — the condition under which any caller would set this flag. Also drops the corresponding `skipEnvHistory: true` at the one caller in the memory extraction planner.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR
Cleans up the fork subagent's internal plumbing so that everything an agent needs to know is decided when the agent is created, not when it starts running. Also removes dead code and eliminates a cross-module coupling that was making the fork path harder to reason about.
No user-facing behavior change — the fork subagent feature works exactly as before.
Screenshots / Video Demo
N/A — no user-facing change. This is an internal refactor of the fork dispatch path. The fork subagent UI, behavior, and outputs are unchanged.
Dive Deeper
Background: how fork subagents work
When the model calls the Agent tool without specifying a subagent type, we create a "fork" — a background copy of the current conversation that can work independently while the parent continues. The fork needs three things from the parent: the system instruction (the long prompt that tells the model how to behave), the list of available tools, and the conversation history so far. Getting these three things to the fork correctly — and in a way that preserves prompt caching — is the core challenge.
Problem 1: identity-level config was threaded through the wrong seam
The original implementation passed the parent's system instruction, tool list, conversation history, and an env-skip flag through the agent's
execute()method — the function that actually runs the agent's reasoning loop. But these values are determined once, before the agent is created, and never change. They describe what the agent is, not how it should run this particular execution.This created a practical problem: the stop-hook retry loop (which can restart the agent if a review gate blocks it) had to carefully re-thread all four values into a second
execute()call. If any were missed, the restarted agent would silently lose its parent context.After this PR, all identity-level config is set at agent creation time. The execute method's signature shrinks to just a context object and an abort signal. The retry loop no longer needs to know anything about fork-specific plumbing.
Problem 2: the recursive fork guard relied on tool stripping
To prevent infinite recursion (a fork spawning another fork spawning another fork...), the original code removed the Agent tool from the fork child's tool list. This worked but broke prompt cache sharing — the fork's tool set no longer matched the parent's byte-for-byte, so the API couldn't reuse the parent's cached prompt prefix.
After this PR, the parent's tool declarations are passed through verbatim, including the Agent tool itself. Recursive forks are instead blocked at runtime using Node's AsyncLocalStorage: when a fork starts executing, we mark the async context, and any nested Agent tool call checks for that marker before dispatching.
Problem 3: the fork was reading from a global snapshot instead of the live conversation
To get the parent's system instruction and tool declarations, the fork path was reaching into a global singleton managed by the followup/suggestion system — a completely separate subsystem that happens to snapshot the same data for its own cache-sharing needs. This worked by coincidence, not by design.
After this PR, the fork reads directly from the parent's live chat session, which is already accessible through the config object. This is both simpler and more correct — it removes the cross-module dependency and eliminates the risk of reading stale data from a snapshot that was designed for a different purpose.
Problem 4: dead configuration fields
The agent model configuration carried
temp(temperature) andtop_p(nucleus sampling) fields that were never populated from agent definitions and served no purpose. They were artifacts from an earlier design that was never completed. Removed.File organization
The agent tool's source files were also moved into a dedicated
tools/agent/directory, grouping the tool definition, its invocation logic, and the fork protocol helpers together.Reviewer Test Plan
Unit tests and static checks:
Manual verification — ask the model to delegate work using the Agent tool and confirm these behaviors still hold:
Full E2E test suite with scripted assertions:
knowledge/qwen-code/e2e-tests/pr-2936-fork-subagent.md(T1–T5). Results posted as a follow-up comment.Testing Matrix
Linked issues / bugs
Follow-up to #2936 (fork subagent feature). No issue closed by this PR — refactor only.