feat(core): allow sub-agents to spawn nested sub-agents up to a configurable depth - #6189
Conversation
E2E verification reportBinary: local bundle with the full feature + hardening. Model under test: glm-5.2 (environment default). Evidence source:
Not E2E-tested (covered by collocated unit tests): depth restoration across background resume and deferred-approval continuations, launch-time cap persistence across restarts, fork-child spawn blocking, teammate exclusion, in-process interactive agent framing. |
|
Thanks for the PR! Template looks good ✓ — all required sections present, bilingual 中文说明 included, reviewer test plan with "How to verify" steps and before/after table filled in. On direction: this is a well-motivated, high-value feature. Sub-agent nesting without a depth limit is a real operational risk — runaway recursive spawning has been a recurring complaint in agent CLIs broadly. Claude Code's own CHANGELOG confirms the same direction landed recently: "Fixed foreground subagents spawning unbounded nested chains; they now respect the same 5-level depth limit as background subagents". That's a strong external signal that qwen-code is converging on the same user-facing need. The PR also closes a parity gap between foreground and background sub-agents that previously only the background path enforced. Aligned. ✓ On approach: the layered design is sound and the scope feels right for what it's trying to do:
One concern worth flagging (not a blocker): Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — 必填章节齐全，包含双语中文说明、带有 "How to verify" 步骤的 reviewer test plan，以及 before/after 表格。 方向：这是一个动机明确、价值较高的功能。没有深度限制的 sub-agent 嵌套是一个真实的运维风险——递归失控 spawning 在各类 agent CLI 中一直是一个反复出现的投诉。Claude Code 自身的 CHANGELOG 也印证了同样的方向最近已经落地："Fixed foreground subagents spawning unbounded nested chains; they now respect the same 5-level depth limit as background subagents"。这是一个强有力的外部信号，说明 qwen-code 与业界在解决同一个用户痛点上方向一致。这个 PR 还填补了 foreground 与 background sub-agent 之间的对等性缺口——之前只有 background 路径强制执行深度限制。方向对齐 ✓。 方案：分层设计合理，范围与目标匹配：
一个值得标记的顾虑（不是阻断项）： 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Sub-agents may now spawn sub-agents up to a configurable maximum nesting depth (default 5; 1 reproduces the previous no-nesting behavior). Enforced in two layers sharing one predicate: prepareTools() hides the agent tool from leaf-depth sub-agents, and AgentTool.execute() rejects over-depth spawns as an authoritative backstop. Teammates, forks, and the workflow tool remain excluded from nesting. Launch depth is persisted in the agent meta sidecar and restored on resume (including deferred-approval continuations and in-process AgentInteractive frames) so a resumed nested agent cannot regain spawn capacity. See knowledge/qwen-code/design/nested-subagents.md.
3017364 to
b8e8046
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
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. |
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Implements nested sub-agent depth limiting (maxSubagentDepth, default 5) with a clean separation of concerns: schema-level gating in prepareTools(), runtime guard in execute(), and depth persistence for background/foreground resume and deferred approval. Fork and teammate exclusion rules are properly enforced. Depth arithmetic is consistent across childLaunchDepth(), canSpawnNestedAgent(), and runWithAgentContext(depthOverride). Comprehensive test coverage including edge cases (frameless prepareTools, fork spawn blocking, teammate routing from nested agents). CI all pass. No issues found.
— qwen3-coder via Qwen Code /review
| // control-plane tool follows the static exclusion set unchanged. | ||
| const isExcluded = (name: string | undefined): boolean => { | ||
| if (!name) return false; | ||
| if (name === ToolNames.AGENT) return !nestingAllowed; |
There was a problem hiding this comment.
[Critical] This re-admits agent for every non-team, non-fork agent frame while depth allows it, but workflow-launched agents are also wrapped in runWithAgentContext and their workflow deny list does not include agent. That means a workflow leaf now receives the spawning tool, even though the PR description says workflow agents remain non-nesting. A workflow child can then launch arbitrary sub-agents outside the workflow orchestrator's agent accounting and return-value contract. Please either add ToolNames.AGENT to the workflow subagent deny floor or add an explicit workflow-agent execution guard here before allowing nesting.
— gpt-5 via Qwen Code /review
There was a problem hiding this comment.
Fixed in 2c50077 — added agent to the workflow-subagent disallowed floor, so workflow-spawned agents never receive the tool regardless of remaining depth. Both spawn paths (default dispatch and custom agentType, which unions the floor) are covered, and the orchestrator tests now assert it.
| // check), so a well-behaved model never reaches here. Teammate spawns | ||
| // returned above and are not gated by depth. See | ||
| // knowledge/qwen-code/design/nested-subagents.md. | ||
| const maxSubagentDepth = this.config.getMaxSubagentDepth(); |
There was a problem hiding this comment.
[Critical] The depth guard is now the only check before a nested spawn proceeds, but the spawn path below loads the requested child config from scratch. It does not propagate the parent's effective tool constraints, so a parent subagent limited with tools: [read_file, agent] or disallowedTools: [run_shell_command] can call agent and get a default general-purpose child with tools: ['*'], regaining tools the parent was explicitly denied. Please either prevent agent from being offered to constrained parents, or carry the parent's effective allow/deny set into nested children and intersect/union it with the child config.
— gpt-5 via Qwen Code /review
There was a problem hiding this comment.
I'd push back on this one. Keeping the child's tool surface independent of the parent's is the reference behavior — claude-code assembles the worker's pool independently, with an explicit code comment that workers "aren't affected by the parent's tool restrictions". The reasoning holds here too:
- A subagent's
toolslist is a focus mechanism, not a privilege boundary. The privilege boundary is the permission system, which gates every nested child's tool executions exactly as it does the parent's. - Spawn capability is opt-in: an explicit tools list only carries
agentif its author granted it, and the child types it can name are agent definitions from the same trust domain (the operator's own configs plus builtins). - Intersecting surfaces would break legitimate patterns — e.g. a read-only coordinator agent that delegates edits to a writer child.
If we want subtree-wide restriction later, per-agent constraints on spawnable child types would be a cleaner mechanism than silently intersecting tool sets. Happy to file that as follow-up work, but I'd keep this PR aligned with the reference semantics.
| this.maxSubagentDepth = | ||
| params.maxSubagentDepth == null | ||
| ? 5 | ||
| : Math.max(1, Math.floor(params.maxSubagentDepth)); |
There was a problem hiding this comment.
[Critical] This clamp preserves non-finite values. JSON settings like { "model": { "maxSubagentDepth": 1e309 } } parse to Infinity; Math.floor(Infinity) and Math.max(1, Infinity) both remain Infinity, so canSpawnNestedAgent(maxDepth) never reaches the cap. Since this setting is the main recursion guardrail, please require a finite integer and cap or reject absurdly large values before storing it.
— gpt-5 via Qwen Code /review
There was a problem hiding this comment.
Fixed in 2c50077 — non-finite values (NaN, Infinity from JSON 1e309) now fall back to the default of 5, and finite values clamp to the range 1–100, mirroring the maxToolCalls typo ceiling. Tests cover Infinity, NaN, and an absurd 5000.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Design doc reference: multiple comment blocks across the PR reference knowledge/qwen-code/design/nested-subagents.md but this file does not exist in the repository. Consider creating it or inlining the key design decisions into the code comments.
Test coverage gaps (needs human review):
- No direct unit tests for
canSpawnNestedAgent/childLaunchDepth/isTopLevelSessionprimitives inagent-context.test.ts - No
prepareTools()fork-context test inagent-core.test.ts - No background-agent-resume depth restoration test
- No test for nested sub-agent requesting fork fallback to regular sub-agent
buildSpawnBlockedResultreturnDisplayshape not asserted in depth guard tests (onlyllmContentchecked)AgentInteractivenested-construction depth pinning untested (only top-level construction covered)
| this.fileDiscoveryService = params.fileDiscoveryService ?? null; | ||
| this.bugCommand = params.bugCommand; | ||
| this.maxSessionTurns = params.maxSessionTurns ?? -1; | ||
| // Default 5 (nesting on). Explicit values below 1 clamp to 1 so the knob |
There was a problem hiding this comment.
[Suggestion] No NaN/Infinity guard on maxSubagentDepth. Math.floor(NaN) propagates NaN (blocks all nesting silently), Infinity passes through (unbounded nesting, defeating the safety cap). The Number.isFinite pattern is already used at line 1224 for maxToolCalls. Also no upper-bound clamp — maxToolCalls caps at 1,000,000 "to catch typos" but this knob has no equivalent.
| // Default 5 (nesting on). Explicit values below 1 clamp to 1 so the knob | |
| this.maxSubagentDepth = | |
| params.maxSubagentDepth == null | |
| ? 5 | |
| : Math.min( | |
| 100, | |
| Math.max( | |
| 1, | |
| Math.floor( | |
| Number.isFinite(params.maxSubagentDepth) | |
| ? params.maxSubagentDepth | |
| : 5, | |
| ), | |
| ), | |
| ); |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Applied in 2c50077 with one tweak on the suggestion: a non-finite value falls back to the default (5) before the range clamp, and the ceiling is 100. Tests cover Infinity, NaN, and the cap.
| export function runWithAgentContext<T>( | ||
| agentId: string, | ||
| fn: () => Promise<T>, | ||
| depthOverride?: number, |
There was a problem hiding this comment.
[Suggestion] Stale comment at line 38: the AgentContext.depth docstring says "Auto-incremented; callers do not pass it" but this new depthOverride parameter explicitly allows callers to pin the depth (used by background-resume, AgentInteractive, and deferred-approval paths). Update to e.g. "Auto-incremented by default; depthOverride restores the original launch depth on resume paths."
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 2c50077 — the docstring now says the depth is auto-incremented by default and that resume paths (background resume, AgentInteractive, deferred approvals) pin the original launch depth via depthOverride.
| // Authoritative runtime backstop for sub-agent nesting: reject the spawn | ||
| // when it would exceed maxSubagentDepth. AgentCore.prepareTools() also | ||
| // hides the AgentTool from leaf-depth sub-agents (same canSpawnNestedAgent | ||
| // check), so a well-behaved model never reaches here. Teammate spawns |
There was a problem hiding this comment.
[Suggestion] No debugLogger call when the nesting depth guard or fork spawn guard fires. The rejection is only visible in the LLM-facing ToolResult — invisible to operators, log aggregators, and alerting. The file has 20+ debugLogger calls elsewhere for similar events. Consider adding:
debugLogger.debug(
'[AgentTool] Nesting depth guard blocked spawn: depth=%d childLevel=%d max=%d type=%s',
getCurrentAgentDepth(), childLaunchDepth() + 1, maxSubagentDepth, this.params.subagent_type,
);(Same for the fork guard below.)
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Added in 2c50077 — both the depth guard and the fork containment guard now emit a debug log (child level, cap, and requested type for the depth guard) before returning the blocked result.
| isForkRequested && | ||
| isForkSubagentEnabled(this.config) && | ||
| // v1: fork is a top-level-only capability. A sub-agent — which may now | ||
| // carry the AgentTool via nesting — that requests a fork falls back to |
There was a problem hiding this comment.
[Suggestion] Silent fork-to-subagent downgrade. When a nested sub-agent requests subagent_type: "fork", isTopLevelSession() makes isFork false and the request silently falls through to a regular isolated sub-agent. The model asked for context-sharing execution but gets isolation with no diagnostic. Consider adding a debug log:
if (isForkRequested && !isFork && !isTopLevelSession()) {
debugLogger.debug('[AgentTool] Fork downgraded to regular sub-agent at depth %d', getCurrentAgentDepth());
}— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Added in 2c50077 — the downgrade now logs its reason (forks do not nest vs. forking unavailable in this session), and a new test pins the fallback: a nested fork request resolves to the awaitable general-purpose sub-agent.
| const toolsList: FunctionDeclaration[] = []; | ||
|
|
||
| const excludedFromSubagents = getExcludedToolsForCurrentContext(); | ||
|
|
There was a problem hiding this comment.
[Suggestion] Stale comment on EXCLUDED_TOOLS_FOR_SUBAGENTS (line 113): says "AgentTool prevents recursive subagent spawning" implying unconditional exclusion, but isExcluded() now depth-gates AgentTool — it IS available to sub-agents when nesting is permitted. The set's ToolNames.AGENT entry is now a dead fallback. Consider updating the comment to: "AgentTool is depth-gated (excluded when canSpawnNestedAgent() returns false); see isExcluded() in prepareTools()."
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Updated in 2c50077. One correction to the note: the ToolNames.AGENT entry is not dead — the fork spawn path filters the parent's inline tool declarations through the raw set (and background-agent-resume consults it too), so the entry stays as the fail-closed floor for consumers of the set itself. The comment now spells out the depth-gating relationship.
| argv.maxSessionTurns ?? settings.model?.maxSessionTurns ?? -1, | ||
| maxWallTimeSeconds: resolveMaxWallTimeSeconds(argv, settings), | ||
| maxToolCalls: resolveMaxToolCalls(argv, settings), | ||
| // Undefined flows through to Config's default (5) and clamp logic. |
There was a problem hiding this comment.
[Suggestion] No --max-subagent-depth CLI flag, unlike sibling settings --max-session-turns (resolved via resolveMaxSessionTurns(argv, settings)) and --max-tool-calls (resolved via resolveMaxToolCalls(argv, settings)). Users cannot override nesting depth for a single run from the command line. Consider adding a --max-subagent-depth flag with a resolver for consistency.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Added in 2c50077 — --max-subagent-depth mirrors the sibling budget flags: flag takes precedence over model.maxSubagentDepth, validates loudly as an integer in 1–100 (yargs NaN from a typo fails at startup instead of silently falling back), and the setting is now documented in settings.md alongside the flag.
- Deny the agent tool to workflow-spawned subagents: depth gating would otherwise re-admit it, letting a workflow leaf spawn outside the orchestrator's concurrency cap, agent accounting, and token budget. - Reject non-finite maxSubagentDepth values (JSON 1e309 parses to Infinity and would unbound the recursion cap; NaN would silently block all nesting) and cap the knob at 100 to catch typos. - Add a --max-subagent-depth CLI flag mirroring sibling budget flags, with loud validation for flag typos, and document the setting. - Log guard rejections (depth, fork containment) and silent fork-to-subagent downgrades through the agent debug logger. - Refresh stale comments (depthOverride resume pinning, depth-gated AgentTool exclusion) and drop references to a design doc that lives outside the repository. - Fill review-noted test gaps: nesting predicate primitives, fork-context prepareTools, persisted-depth restoration on background resume, nested AgentInteractive depth pinning, nested fork fallback, and the blocked-spawn returnDisplay shape.
|
Re the review-level notes (design doc + coverage), addressed in 2c50077: Design doc references — dropped all of them. The path pointed at a doc in my local knowledge workspace, not this repository; the key design decisions (two-layer enforcement sharing one predicate, resume depth pinning, fork/teammate/workflow exclusions) were already inlined in the comments at each enforcement point, so the references added nothing for repo readers. Test gaps — all six are now covered:
|
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Review Summary
Nested Sub-agents (PR #6189) — Introduces configurable sub-agent nesting depth via model.maxSubagentDepth (default 5).
Analysis
Well-structured implementation of recursive sub-agent spawning with proper depth tracking:
- Depth tracking:
runWithAgentContextextended withdepthOverridefor resume correctness. New predicates (isTopLevelSession,childLaunchDepth,canSpawnNestedAgent) provide a clean API. - Dual guards: Schema-level gating in
prepareTools()(prevents tool exposure) + runtime guard inexecute()(defense in depth). - Persistence:
AgentMeta.depthandAgentPersistedCliFlags.maxSubagentDepthensure resumed agents retain their nesting level. - Exclusions: Teammates, forks, and workflow-spawned agents are correctly excluded from nesting in v1.
- Fork downgrade: Nested agents requesting
forktype gracefully fall back to regular sub-agents.
Comprehensive test coverage across all edge cases (depth limits, resume, fork containment, teammate exclusion, deferred approvals).
No high-confidence issues found.
— qwen3-coder via Qwen Code /review
The exhaustive CliArgs mock in gemini.test.tsx missed the new field, failing CI's clean tsc build (the local incremental build skipped the test file).
| // runtime backstop necessary. Blocks ALL agent calls from a fork child | ||
| // (not just fork-in-fork). This runs before the fork branch below, so the | ||
| // recursive-fork case is subsumed here. | ||
| if (isInForkExecution()) { |
There was a problem hiding this comment.
Defense-in-depth asymmetry: execute() backstops forks but not teammates
prepareTools() gates nesting on three exclusions — !isTopLevelSession() && !isTeammate() && !isInForkExecution() && canSpawnNestedAgent(...). But the runtime guards here in execute() only re-check depth (L1833) and isInForkExecution() (this line); there is no !isTeammate() equivalent.
The PR frames the fork guard as a backstop against a hallucinated spawn call even when schema-hiding holds. By that same threat model a teammate is uncovered at this layer: with maxSubagentDepth >= 2, a teammate at depth 0 passes the depth guard (childLevel 2 <= max), isInForkExecution() is false, and the name-routing branch above is skipped (either no name, or !isTopLevelSession() short-circuits it) — so an unadvertised agent call would spawn a regular nested sub-agent, violating the v1 "teammates do not nest" rule.
Not reachable today: the in-process teammate path runs through the AgentCore reasoning loop, which rejects any call outside prepareTools()'s output (allowedToolNames, agent-core.ts:1298), and isTeammate() reliably strips agent from the teammate schema. So this is defense-in-depth only. But since the fork backstop exists here precisely for the non-filtering/wildcard path, a symmetric !isTeammate() guard — or folding all three exclusions into one shared predicate mirroring prepareTools() — would stop the two layers from drifting apart.
There was a problem hiding this comment.
Good catch — agreed the backstops should cover the same exclusions as the schema gate. Added a teammate guard in 07c002b, symmetric with the fork guard (debug log + blocked result), with a test pinning it (teammate frame, depth would permit the spawn, guard fires anyway).
On folding all three into one shared predicate: the layers intentionally differ by exactly one term. prepareTools() only ever serves agents, so it uses !isTopLevelSession() to fail closed on a missing frame; execute() must allow the top-level session — it is the normal spawn path. That asymmetry is why they share canSpawnNestedAgent() but not a full predicate; the guard comment now spells this out so the layers don't drift silently.
execute() backstopped depth and fork containment but not the teammate exclusion, so its guards covered less than prepareTools() gates. A teammate spawn call that slipped past schema-hiding would have nested. Block it symmetrically with the fork guard, log the rejection, and pin the behavior in a test.
| ov.getMaxToolCalls = () => flags.maxToolCalls; | ||
| } | ||
| if (flags.maxSubagentDepth !== undefined) { | ||
| ov.getMaxSubagentDepth = () => flags.maxSubagentDepth; |
There was a problem hiding this comment.
[Critical] persistedCliFlags.maxSubagentDepth is trusted directly when rebuilding the resumed agent config, so the resume path bypasses the new Config clamp/default logic. A malformed or tampered sidecar with maxSubagentDepth: 5000 or JSON 1e309 would make getMaxSubagentDepth() return that raw value after restart, effectively removing the documented 1-100 nesting cap for resumed agents. Please normalize the persisted value before overriding the getter, using the same semantics as Config.
| ov.getMaxSubagentDepth = () => flags.maxSubagentDepth; | |
| const maxSubagentDepth = Number.isFinite(flags.maxSubagentDepth) | |
| ? Math.min(100, Math.max(1, Math.floor(flags.maxSubagentDepth))) | |
| : 5; | |
| ov.getMaxSubagentDepth = () => maxSubagentDepth; |
— gpt-5 via Qwen Code /review
There was a problem hiding this comment.
Fixed in 5477164 — and it was worse than the 1e309 case: JSON.stringify turns a legitimate in-memory Infinity into null, which passed the !== undefined guard and made the getter return null. Rather than duplicating the clamp, I extracted it into a shared normalizeMaxSubagentDepth() used by both the Config constructor and the flag-restore path, so the two cannot drift. The resume test now persists 5000 and asserts the restored config reports 100.
| requiresRestart: false, | ||
| default: 5, | ||
| description: | ||
| 'Maximum number of nested sub-agent levels. 1 disables nesting (sub-agents cannot spawn sub-agents); the default 5 allows a sub-agent to spawn sub-agents up to five levels deep. Values below 1 are clamped to 1. Teammates, forks, and the workflow tool are excluded from nesting.', |
There was a problem hiding this comment.
[Suggestion] This schema description is already stale relative to the runtime and settings.md: it only mentions clamping values below 1 and says "the workflow tool" is excluded, but the implementation now caps values at 100, falls back to the default for non-finite values, and the scoped exclusion is workflow-spawned agents. The generated VS Code schema carries the same wording, so settings UI/schema consumers will see behavior that disagrees with the actual config handling.
| 'Maximum number of nested sub-agent levels. 1 disables nesting (sub-agents cannot spawn sub-agents); the default 5 allows a sub-agent to spawn sub-agents up to five levels deep. Values below 1 are clamped to 1. Teammates, forks, and the workflow tool are excluded from nesting.', | |
| 'Maximum sub-agent nesting depth (1-based levels: a top-level sub-agent is level 1). 1 keeps sub-agents available but disables nesting; the default 5 allows nesting up to five levels deep. Values clamp to the range 1-100; non-finite values fall back to the default. Teammates, forks, and workflow-spawned agents never nest regardless of this setting. Overridable via --max-subagent-depth.', |
— gpt-5 via Qwen Code /review
There was a problem hiding this comment.
Fixed in 5477164 — took the suggested wording for settingsSchema.ts and regenerated the VS Code schema from it (scripts/generate-settings-schema.ts), so both now match settings.md and the runtime behavior.
The resume path trusted the raw sidecar value, bypassing the Config clamp — a tampered or malformed sidecar (1e309 parses to Infinity; JSON.stringify turns Infinity into null) would remove the nesting cap for resumed agents. Extract the clamp into a shared normalizeMaxSubagentDepth used by both the Config constructor and the flag-restore path, and refresh the stale settings schema description (clamp range, non-finite fallback, workflow-agent wording).
| // tampered sidecar (JSON `1e309` parses to Infinity; JSON.stringify | ||
| // turns Infinity into null) must not bypass the nesting cap for | ||
| // resumed agents. Same semantics as the Config constructor. | ||
| const maxSubagentDepth = normalizeMaxSubagentDepth(flags.maxSubagentDepth); |
There was a problem hiding this comment.
[Suggestion] This resume-side normalization now intentionally handles persisted null values, which can happen when a non-finite in-memory value is serialized through JSON. The new resume regression test covers 5000 -> 100, but it does not pin the null -> default 5 case that motivated this branch of the normalization.
Please add or parameterize a resume-path test so a sidecar with persistedCliFlags.maxSubagentDepth: null asserts that overriddenConfig.getMaxSubagentDepth() returns 5.
— gpt-5 via Qwen Code /review
There was a problem hiding this comment.
Added in ed1177b — the resume test is now parameterized over both cases (5000 → 100 and null → 5), and the persisted flag type is widened to number | null with a comment documenting the JSON.stringify(Infinity) origin of the null.
…epth JSON.stringify(Infinity) === 'null', so a sidecar can legitimately carry null; widen the persisted flag type to admit it and parameterize the resume test over both the clamp (5000 -> 100) and the null fallback (null -> 5).
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM.
— gpt-5 via Qwen Code /review
|
@qwen-code /triage |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed the depth-gating design end to end (schema layer, runtime guards, resume/deferred-approval restoration, and the containment story for teammates/forks/workflow agents). The two-layer enforcement built on a shared canSpawnNestedAgent() is solid, and the test coverage for the boundary cases is thorough. I verified a few suspicions that did not pan out, for the record: a hallucinated agent call from a workflow subagent is already rejected at dispatch (processFunctionCalls validates against the prepared declaration list before any registry lookup), so the schema-level WORKFLOW_SUBAGENT_DISALLOWED_TOOLS entry is sufficient there; and the formatting churn in config.ts is prettier-compliance, fine.
8 findings below as inline comments, most severe first:
meta.depthis pinned unvalidated on resume (background-agent-resume.ts:1035) — the one field of the sidecar that can still bypass the cap the PR hardens against.- Monitor notifications from in-process interactive agents are now silently dropped (
agent-interactive.ts:151) — a regression from framingrunLoop(): owned monitors route only throughagentNotificationCallbacks, which nothing registers for these agents. - Nested background spawns have a broken completion contract (
agent.ts:2049) — the launcher is told to usesend_message/task_stopit doesn't have, and the terminal notification lands in the top-level conversation. - A nested sub-agent's
nameis dropped with zero signal (agent.ts:1825) — the model believes it delegated to a teammate; not even the debug log fires on this path. - Spawn-eligibility policy duplicated between
prepareTools()andexecute()(agent.ts:1866). maxSubagentDepth?: number | nullwidening defends an impossible state, and its justifying comment is factually wrong (agent-transcript.ts:149).- The 5/1/100 triple is hand-duplicated across three sites (
packages/cli/src/config/config.ts:1260). - Constructor-time ambient depth capture in
AgentInteractiverests on undocumented non-local invariants (agent-interactive.ts:94) — a note, not a blocker.
1–4 are correctness issues worth addressing before merge (1 and 2 especially); 5–8 are cleanups that could also be follow-ups.
🤖 Generated with Claude Code — Claude Fable 5
| // its original nesting level (and spawn eligibility) instead of | ||
| // recomputing to depth 0 from this top-level resume frame. | ||
| const framedRunBody = () => | ||
| runWithAgentContext(meta.agentId, runBody, meta.depth); |
There was a problem hiding this comment.
meta.depth flows into the depth override unvalidated — the other half of the tampered-sidecar defense is missing.
meta comes from readAgentMeta(), which is a bare JSON.parse(...) as AgentMeta, and nothing downstream clamps the value: runWithAgentContext only does depthOverride ?? ... (catches null/undefined only), and childLaunchDepth() / canSpawnNestedAgent() do raw arithmetic.
So a malformed or hand-edited sidecar with "depth": -50 pins the resumed frame at −50 → childLaunchDepth() = −49 → canSpawnNestedAgent() passes for every cap, and the resumed subtree gains ~50 levels beyond maxSubagentDepth. Worse, "depth": -1e309 parses to -Infinity, and since -Infinity + 1 === -Infinity, every descendant inherits it — the cap is permanently defeated for the whole subtree.
This is exactly the threat model the same resume path already defends against for the sibling field: applyPersistedCliFlagOverrides re-normalizes persistedCliFlags.maxSubagentDepth because "a malformed or tampered sidecar … must not bypass the nesting cap". The cap check is effectively depth + 2 <= maxDepth; right now the right-hand operand is hardened and the left-hand operand is trusted raw from the same JSON file. This is also the only place a disk-persisted depth re-enters the runtime, so it's the complete fix site.
Suggest mirroring the normalize pattern before pinning:
const restoredDepth = Number.isFinite(meta.depth)
? Math.max(0, Math.floor(meta.depth!))
: undefined; // fall back to auto-increment
const framedRunBody = () =>
runWithAgentContext(meta.agentId, runBody, restoredDepth);There was a problem hiding this comment.
Fixed in 4b0c5ac — normalizeResumedAgentDepth() (next to the AgentMeta type) now guards the depthOverride. Absent values resume as a fresh launch; anything but an integer in 0–100 fails CLOSED to the depth ceiling — clamping a corrupt value down to 0 would fail open by granting full spawn capacity, so the resumed agent keeps running but cannot spawn. Unit tests cover -50, ±Infinity, NaN, fractionals, and >100; the resume-path test is parameterized over a tampered -50 sidecar asserting the frame pins at 100.
| */ | ||
| private async runLoop(): Promise<void> { | ||
| private runLoop(): Promise<void> { | ||
| return runWithAgentContext( |
There was a problem hiding this comment.
Regression: framing runLoop() makes Monitor notifications from in-process interactive agents silently disappear.
Every tool body now runs under this agent's identity frame, so a monitor call stamps ownerAgentId = getCurrentAgentId() (monitor.ts:318). But MonitorRegistry.dispatchNotification (services/monitorRegistry.ts:591-601) routes owned monitors only through agentNotificationCallbacks, with no session fallback:
const callback = entry.ownerAgentId
? this.agentNotificationCallbacks.get(entry.ownerAgentId)
: this.notificationCallback;
if (!callback) { /* "Dropping monitor notification …" */ return; }The only registration sites are the AgentTool spawn paths (agent.ts:1064) and background-agent-resume.ts:860 — nothing registers a callback for agents spawned via InProcessBackend (Arena sessions, in-process teammates). And emitTerminalNotification sets entry.notified = true before dispatch, so the one-shot completion notice is consumed even when dropped.
Before this PR, runLoop() was frame-less, these monitors were session-owned, and their notifications reached the session callback. After it: MONITOR is absent from both EXCLUDED_TOOLS_FOR_SUBAGENTS and EXCLUDED_TOOLS_FOR_TEAMMATES, Arena/teammate spawns pass no toolConfig, and the Monitor tool is start-only (no poll action; task_stop is excluded from their toolsets) — so an Arena agent that starts a build/test under a monitor now waits forever with no way to learn it finished.
Options, in order of preference: register an agentNotificationCallbacks entry for in-process interactive agents (routing into their AsyncMessageQueue, analogous to registerOwnedMonitorNotifications); or exclude MONITOR from their toolsets the way WORKFLOW_SUBAGENT_DISALLOWED_TOOLS already does for workflow agents; or make owned-monitor dispatch fall back to the session callback when the owner has no registered callback.
There was a problem hiding this comment.
Good catch — this was a real regression from framing runLoop(). Fixed in 4b0c5ac with your first-preference option: InProcessBackend.spawnAgent now registers an agent notification callback that routes owned-monitor notifications into the agent's message queue (enqueueMessage self-wakes the pump, so no lifecycle callback is needed), and releaseAgentResources cancels running monitors and drops the callback — mirroring AgentTool's registerOwnedMonitorNotifications lifecycle. A test pins registration on spawn, delivery into the queue, and teardown on stop.
| // the awaitable general-purpose sub-agent (via effectiveSubagentType | ||
| // below) instead of opening a nested fork. Fork nesting is deferred | ||
| // past v1. | ||
| isTopLevelSession(); |
There was a problem hiding this comment.
Nested background spawns are left ungated here, and their completion contract is broken for the launcher.
Fork requests from a nested sub-agent get downgraded on this exact line — but run_in_background: true (or an agent type defined with background: true) from a nested launcher goes through with only the concurrency cap (assertCanStartBackgroundAgent), and then:
- The success
llmContent(~agent.ts:2917-2926) tells the launcher to "Use send_message to continue this agent, or task_stop to cancel" — but both tools are inEXCLUDED_TOOLS_FOR_SUBAGENTS, and the newisExcluded()re-admits onlyagent. The launcher is instructed to use tools it provably doesn't have. - "You will be notified automatically when it completes" is also wrong for it:
BackgroundTaskRegistryhas a single session-levelnotificationCallbackandregister()records no owner, so the child's terminal<task-notification>is injected into the top-level conversation (with atool-use-idforeign to that history), while the launcher — which typically returns before the child finishes — never hears back. The child's work is orphaned unless the launcher happens to tail the output file.
This state is newly reachable: pre-PR no sub-agent ever carried the AgentTool, and it's on by default (cap 5). Suggest handling it the way the fork case is handled right here: downgrade run_in_background to a foreground (awaited) run when !isTopLevelSession(), deferring nested background delegation until notifications can be routed to the launching agent — or at minimum make the background branch top-level-only in the schema text and fix the returned guidance for nested launchers.
There was a problem hiding this comment.
Fixed in 4b0c5ac, taking the downgrade option: a background request (run_in_background or an agent type's background: true) from a nested launcher now runs as an awaited foreground task, exactly parallel to the fork downgrade — with a debug log and a schema-text note that background is top-level-only. Nested background delegation stays deferred until notifications can be routed to the launching agent. Test asserts the nested request completes inline (result text returned, registered with isBackgrounded: false).
| // sub-agent could pass `name` and reach executeTeammate, bypassing both | ||
| // the v1 "teammates do not nest" rule and the depth guard below. A nested | ||
| // sub-agent's `name` falls through to the normal path. | ||
| if (this.params.name && !isTeammate() && isTopLevelSession()) { |
There was a problem hiding this comment.
A nested sub-agent's name is now dropped with zero signal — not even the debug log fires.
With a team active, a nested sub-agent (which now legitimately holds this tool at the default cap of 5) calling agent with name: "alice" skips this block via the new isTopLevelSession() term, and params.name is never referenced again on the fall-through path — a fresh generic one-shot agent spawns at the next level. The debugLogger.debug("Ignoring teammate name …") line can't fire for this case: it's inside the skipped block and gated on !getTeamManager().
The security direction is right (and the depth-1 test pins it), but the silent-success path at default depth is untested and inconsistent with this function's own conventions: the schema served to nested agents still advertises name ("spawn as a named teammate via the active team" — prepareTools filters whole tools, it doesn't rewrite schemas), and every sibling guard (depth, teammate, fork, plan_mode_required) returns a model-visible Error:. Here the model walks away believing alice handled the task — it may report that upward or try to follow up with her.
Suggest either rejecting with an explicit error (mirroring the plan-mode guard) or spawning but prepending a note to the returned llmContent that name was ignored because teammate delegation is top-level-only, plus a debug log on this branch.
There was a problem hiding this comment.
Fixed in 4b0c5ac — the fall-through now logs ("Ignoring teammate name … from a nested sub-agent; spawning a regular sub-agent instead"), and a test pins the silent-success path at the default cap: team active, nested frame, name passed → spawnTeammate never called, loadSubagent called with the requested type.
| // frame — that asymmetry is why the two layers share canSpawnNestedAgent | ||
| // rather than one full predicate.) Teammate spawns via `name` returned | ||
| // in the team-routing branch above, which requires !isTeammate(). | ||
| if (isTeammate()) { |
There was a problem hiding this comment.
Cleanup: the three-exclusion spawn policy is now maintained in two hand-written copies.
prepareTools() composes !isTopLevelSession() && !isTeammate() && !isInForkExecution() && canSpawnNestedAgent(...) (agent-core.ts:519-523); execute() re-states the same teammate/fork/depth exclusions as three sequential guards in a different order. Only the depth term is shared — the symmetry of the other two is maintained by the "symmetric with the fork guard below" comment, and the fork guard's own rationale (wildcard/fallback lists) shows what a missed runtime-side entry costs: a silent spawn bypass. Missed on the schema side it burns model turns on guaranteed-rejected calls.
The long comment above this line argues the two layers can't share one full boolean predicate — which is true, but a reason-enum dissolves the asymmetry:
// agent-context.ts
export function spawnBlockReason(maxDepth: number): 'depth' | 'teammate' | 'fork' | null;execute() switches on the reason for its per-cause messages (evaluated in the current guard order, preserving which reason wins for a teammate at leaf depth); prepareTools() checks !isTopLevelSession() && spawnBlockReason(...) === null, keeping the fail-closed frame check local, exactly as the comment prescribes. All four predicates are pure ALS reads, so the composition is order-insensitive on the schema side; the fork-before-downgrade ordering in execute() is positional and untouched; no import cycle (identity.ts and fork-subagent.ts don't reach back into agent-context.ts). ~15 net lines, deletes this comment, and turns a comment-enforced invariant into a structural one — the same anti-drift rationale canSpawnNestedAgent's own doc gives.
There was a problem hiding this comment.
Taken as proposed in 4b0c5ac — spawnBlockReason(maxDepth): 'depth' | 'teammate' | 'fork' | null lives in agent-context.ts, evaluated in execute()'s guard order so the winning reason stays stable. execute() switches on it for per-cause messages (with an exhaustiveness check), prepareTools() composes !isTopLevelSession() && spawnBlockReason(...) === null, and the hand-symmetry comment is gone. Unit tests pin the reason precedence including the teammate-at-leaf-depth case.
| * serialization (JSON.stringify(Infinity) === 'null'); the resume path | ||
| * normalizes it back to the default. | ||
| */ | ||
| maxSubagentDepth?: number | null; |
There was a problem hiding this comment.
The | null widening defends a state this codebase cannot produce, and the justifying comment is factually wrong.
The only writer of persistedCliFlags is capturePersistedCliFlags (agent.ts:469, via the two writeAgentMeta call sites), which persists config.getMaxSubagentDepth() — and that is a normalized finite integer 1–100 at every definition: the Config constructor (this.maxSubagentDepth = normalizeMaxSubagentDepth(...)) and the resume override closure both normalize. JSON.stringify(Infinity) === 'null' is true of JavaScript, but Infinity can never be in memory for this field, so "null occurs when a non-finite in-memory value crossed JSON serialization" describes an impossible path. (Pre-PR sidecars just lack the key → undefined, never null.)
Only a hand-tampered/foreign sidecar can carry null — and normalizeMaxSubagentDepth's value == null check in the resume path already covers that without the type widening (reverting to number still typechecks; the runtime hardening stays). AGENTS.md's first principle is "No error handling for impossible scenarios."
Suggest: type it plain number, reword this comment (and the matching one in background-agent-resume.test.ts, "the sidecar can legitimately carry null") to attribute null to malformed/tampered sidecars — the honest framing applyPersistedCliFlagOverrides already uses — and keep or drop the null test row under that label.
There was a problem hiding this comment.
You're right — the in-memory value is normalized at every writer, so the JSON.stringify(Infinity) story described an impossible path. Fixed in 4b0c5ac: the type is plain number again, and the comments here, in the resume override, and in the test row now attribute null to malformed/hand-edited sidecars (the test keeps the null row under that honest label, cast explicitly as tampered input).
| ): number | undefined { | ||
| const value = argv.maxSubagentDepth; | ||
| if (value !== undefined && value !== null) { | ||
| if (!Number.isInteger(value) || value < 1 || value > 100) { |
There was a problem hiding this comment.
Cleanup: the {default 5, min 1, max 100} triple is hand-duplicated at three sites.
This validator (bounds in the condition and the error text), normalizeMaxSubagentDepth in core config.ts, and settingsSchema.ts's default: 5 (the vscode settings.schema.json is generated from it, so that one's free). If core's cap or default ever changes, this flag validator drifts — and the failure mode is precisely the one this function's docstring says it exists to prevent: an explicitly passed --max-subagent-depth accepted here, then silently clamped by core.
The codebase already has the exact pattern for this: DEFAULT_STOP_HOOK_BLOCK_CAP and SENSITIVE_SPAN_ATTRIBUTE_MAX_LENGTH_LIMIT are exported from core and consumed by settingsSchema.ts (both as default: and maximum:). Suggest exporting DEFAULT_MAX_SUBAGENT_DEPTH / MAX_SUBAGENT_DEPTH_LIMIT next to normalizeMaxSubagentDepth, using them there, here (condition + interpolated message), and in the schema default.
There was a problem hiding this comment.
Fixed in 4b0c5ac following the DEFAULT_STOP_HOOK_BLOCK_CAP pattern — DEFAULT_MAX_SUBAGENT_DEPTH and MAX_SUBAGENT_DEPTH_LIMIT are exported from core next to the normalizer and consumed by it, the CLI flag validator (condition + interpolated error message), and settingsSchema's default:. The vscode schema stays generated.
| constructor(config: AgentInteractiveConfig, core: AgentCore) { | ||
| this.config = config; | ||
| this.core = core; | ||
| this.agentDepth = childLaunchDepth(); |
There was a problem hiding this comment.
Note (non-blocking): constructor-time ambient capture is correct today only via invariants enforced in other files.
Sampling childLaunchDepth() here is safe right now because the single construction site (InProcessBackend.spawnAgent) sits on the spawner's await chain, and both entry paths are top-level-gated elsewhere (Arena via the slash command; teammates via the isTopLevelSession() gate in agent.ts). None of that is visible or documented at the call site, and InProcessBackend.ts isn't touched by this PR — so a future factory/queue/deferred construction would silently record depth 0. That's the same deferral-loses-frame class this PR fixes explicitly for resume (persisted meta.depth) and deferred approvals (inheritedAgentDepth); exposure is bounded (a lost frame grants only level-1 capacity), which is why this is a note rather than a bug.
Cheap hardening: accept an optional explicit depth in AgentInteractiveConfig (the identity is already half-explicit — agentId lives there) and have InProcessBackend pass childLaunchDepth() at the spawn site, or at least document the same-frame precondition at the new AgentInteractive(...) call.
There was a problem hiding this comment.
Documented in 4b0c5ac — the constructor now carries the invariant at the capture site: correct only while construction stays on the spawner's await chain, a deferred construction would silently record depth 0 (the same deferral-loses-frame class the resume path solves via persisted meta.depth), and the escape hatch if construction ever moves off the spawn chain is threading depth through AgentInteractiveConfig.
- Normalize persisted meta.depth on resume: the sidecar is untrusted JSON, and a tampered negative depth (or -1e309 → -Infinity) would pin the resumed frame below zero and pass canSpawnNestedAgent for every cap. Invalid values fail closed to the depth ceiling — the agent keeps running but cannot spawn. - Register monitor notification routing for in-process interactive agents: framing runLoop() made their monitors agent-owned, and owned dispatch has no session fallback, so notifications were silently dropped. InProcessBackend now routes them into the agent's message queue and tears the routing down on release. - Downgrade background spawn requests from nested launchers to awaited foreground runs: a nested launcher cannot honor the background completion contract (send_message/task_stop excluded, notifications session-scoped), which orphaned the child's results. - Extract spawnBlockReason() as the single spawn-exclusion policy shared by prepareTools() and execute(), replacing two hand-kept copies of the depth/teammate/fork rules. - Share DEFAULT_MAX_SUBAGENT_DEPTH / MAX_SUBAGENT_DEPTH_LIMIT across the core normalizer, the CLI flag validator, and the settings schema. - Log dropped teammate names from nested spawns; revert the impossible |null persisted-flag widening to an honest tampered-sidecar framing; document the constructor-time depth capture invariant.
|
Re-reviewed
Ran the six touched suites locally at 🤖 Generated with Claude Code — Claude Fable 5 |
settingsSchema.ts now imports the shared constant, so CLI tests that mock @qwen-code/qwen-code-core with an explicit export list need the new export.
* feat(core): allow bounded nested sub-agent spawning via maxSubagentDepth Sub-agents may now spawn sub-agents up to a configurable maximum nesting depth (default 5; 1 reproduces the previous no-nesting behavior). Enforced in two layers sharing one predicate: prepareTools() hides the agent tool from leaf-depth sub-agents, and AgentTool.execute() rejects over-depth spawns as an authoritative backstop. Teammates, forks, and the workflow tool remain excluded from nesting. Launch depth is persisted in the agent meta sidecar and restored on resume (including deferred-approval continuations and in-process AgentInteractive frames) so a resumed nested agent cannot regain spawn capacity. See knowledge/qwen-code/design/nested-subagents.md. * feat(cli): display nested sub-agents as a tree in the TUI Render nested agents depth-first with indent + dim '↳' in the live agent panel and background tasks view; promote orphaned children to root with a '· from <parent>' annotation. Detail view gains a level badge, Parent breadcrumb, and Sub-agents section. The [blocking] tag and two-step cancel confirm now apply only to provably user-blocking foreground chains. Parent completion summaries carry a '· N sub-agents' tail (guard-rejected spawns now record as failed tool calls so the count stays honest). Also fixes the live-panel Enter-for-detail order mismatch by sharing one display order between the panel render and the composer keyboard mapping. * fix(core): address round-1 review on nested sub-agent spawning - Derive launch metadata (hooks, spans, task rows, meta sidecar) from the resolved subagent config instead of the raw requested type, so a fork request that falls back to the awaitable path no longer reports "fork". - Pin the blocked-spawn failure contract in tests: error is set and returnDisplay.status is 'failed' for both the depth and fork guards; also document the failure-path routing at buildSpawnBlockedResult. - Drop source-comment references to private knowledge/ design docs that do not exist in this repository. * test: address round-2 review on sub-agent counting and fork fallback - Exercise the legacy 'task' alias in the scrollback sub-agent count so the migration-aware name set is covered, not just the canonical name. - Pin the nested-fork downgrade: a sub-agent requesting a fork falls back to the awaitable general-purpose subagent even in interactive mode. - Drop a duplicated 'nesting depth guard' describe block left behind by the automated base-branch merge (kept the copy with the failure-shape assertions). * fix(core): keep actionable guidance in blocked-spawn error messages The scheduler's failure path sends only error.message to the model and the scrollback, discarding llmContent. With the terse terminateReason as the message, a blocked spawn lost its "do the task yourself instead" instruction, inviting retry loops. Carry the full guidance text in error.message and keep terminateReason for the display card. * test(cli): pin the tree indent clamp at depth beyond TREE_INDENT_MAX_LEVELS Maintainer mutation-testing on the PR found that removing the clamp in treeRowPrefix survived the suite. Assert a depth-4 row indents 3 levels (12 spaces), plus the base marker/indent behavior. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>
|
Closing the loop on the review notes @wenshao posted on the stacked UI PR (#6191, #6191 (review)), which scoped four findings to this PR's code. All four are now addressed on this branch:
Just verified the three suites locally on this branch: 81/81 green. |
✅ Maintainer local verification — real binary + tmux E2EBuilt the real bundle at PR head Verdict: the feature behaves exactly as specified in all scenarios. Recommend merge (one non-blocking note on the PR description at the end). 1) Backend depth-gating — the decisive per-level evidence
Every captured request was classified by its agent's nesting level (via a per-level seed marker that only lands in that agent's own
An allowed level advertises 51 tools; a leaf level advertises 50 — exactly the 2) Both enforcement layers verified
3) TUI nested tree (the
|
| 场景 | 上限（来源） | L0 | L1 | L2 | 结果 |
|---|---|---|---|---|---|
| S1 嵌套（默认） | 5 · 默认 | agent✓·15 | agent✓·51 | agent✓·51 | 两层链路执行；叶子 nonce 回传到 stdout |
| S2 上限边界 | 2 · model.maxSubagentDepth |
✓·15 | agent✓·51 | agent✗·50 | L2 未获得 agent；无 L3 |
| S3 旧行为 | 1 · --max-subagent-depth |
✓·15 | agent✗·50 | — | L1 无法委派 —— 与功能前契约一致 |
| S4 无回归 | 5 · 默认 | ✓·15 | agent✓（叶子） | — | 普通单层委派保持不变 |
| S5 钳制 0→1 | 0 → 钳制为 1 |
✓·15 | agent✗·50 | — | maxSubagentDepth:0 归一化为 1（≡ S3） |
允许派生的层级声明 51 个工具；叶子层级声明 50 个 —— 恰好少了 agent 这一个工具，而且这个翻转精确地落在每个上限边界处。当叶子层级的模型仍被强制调用 agent 时，qwen 以 Tool "agent" not found 拒绝 —— 所以越界的幻觉派生不会产生更深的代理。S1 中叶子返回的随机 nonce 出现在父代理的 role:tool 消息里、再出现在顶层 stdout，证明子代理确实运行且其结果沿链路逐层回传。
2）两层强制机制均已验证
- 第一层 —— 模式门控（
prepareTools）：上面已端到端观察到；叶子层级根本看不到agent。 - 第二层 —— 运行时兜底（
AgentTool.execute→spawnBlockReason）：8 个改动的 core 测试套件（629 个测试）全部通过。对共享深度判定做变异（canSpawnNestedAgent → true）会导致三层共 8 个测试失败，其中包括运行时守卫的测试 "rejects a spawn that would exceed maxSubagentDepth"（expected 'Subagent "file-search" not found' to contain 'nesting depth limit reached'）—— 而 teammate/fork 守卫保持绿色（独立分支）。这证明两层来自同一判定、不会漂移，与 PR 的说法一致。 - flag 校验：
--max-subagent-depth 0|3.5|200|abc全部被明确报错拒绝（must be an integer between 1 and 100）；settings 路径则静默钳制 —— 这是有意的不对称设计。
3）TUI 嵌套树（#6191 的树形工作已在本分支内）
（见上方"nested tree"图）
- 上： 在 tmux 中从运行中的真实二进制实时捕获 —— 顶层会话派生一个后台代理，后台代理再派生一个嵌套子代理；真实的
LiveAgentPanel以树形（↳）渲染它们，状态栏显示2 local agents。 - 下： 用三层 fixture 渲染真实
BackgroundTasksDialog—— 深度优先缩进、↳连接线，且[blocking]只正确地出现在前台根节点（嵌套子代理阻塞的是其后台父代理，而非用户回合）。
⚠️ 一条非阻断说明：描述与实际 diff 不一致
描述写着 "本 PR 仅包含后端…… 嵌套的树形展示……将作为叠加在本 PR 之上的后续 PR 提交。" 而那个后续 PR —— #6191 —— 已于 2026-07-03 合入 feat/nested-subagents（提交 699cb26），因此它的树形代码（agent-forest.ts、LiveAgentPanel、BackgroundTasksDialog、ToolMessage、InputPrompt）已经在本分支内，且尚未进入 main。也就是说合并 #6189 会一并带入 TUI 树。这没问题 —— 两层这里都已验证 —— 但"仅后端"的措辞应更新，以便审阅者了解真实的合并范围。
其他
- 审阅时 head 未变（
699cb26）；Test (ubuntu-latest)绿色，mac/win/Integration 是惯常的 named-job skip，blocked是评审门（非 CI 失败）。 - 与 PR 自述风险一致：并发仍受限（前台嵌套串行执行；嵌套的后台请求会被降级为等待式前台运行），但深层树的累计 token 消耗没有单独预算 —— 树级预算是合理的后续工作。
Verification harness: real dist/cli.js @ 699cb26 · fake-OpenAI forced greedy nesting · per-request tools[] capture · vitest + predicate mutation · live tmux TUI capture. No source modified (throwaway test/driver reverted).
|
Thanks for the thorough maintainer verification — the per-level Description updated per your note: it no longer claims backend-only scope. Both the English and Chinese versions now state that #6191's tree rendering has merged into this branch, so merging this PR brings the backend enforcement and the TUI tree together, and the out-of-scope list drops the stale follow-up-PR line. Agreed on tree-wide token budgeting as the natural follow-up. |
|
@qwen-code /triage |
|
Thanks for the PR @tanzhenxin! Template looks good ✓ — all required sections present, bilingual body, clear test plan. On direction: this is squarely aligned with the project's multi-agent roadmap ( On approach: the two-layer enforcement design is the right call. Sharing a single The scope is large (40 files, ~2700 additions) but cohesive: backend enforcement and TUI tree rendering are tightly coupled (the tree display needs the lineage metadata the backend now persists), and splitting them would have meant shipping a feature with no visible UX until a follow-up PR landed. The The default-on cap of 5 is a behavior change worth noting — any sub-agent can now delegate further — but Moving on to code review. 🔍 中文说明感谢 @tanzhenxin 的贡献！ 模板完整 ✓ — 所有必填部分齐全，双语描述，测试计划清晰。 方向：与项目的多代理路线图（ 方案：两层强制机制设计正确。在模式门控（ 范围较大（40 个文件，约 2700 行新增）但内聚：后端强制与 TUI 树形渲染紧耦合（树形展示需要后端持久化的血缘元数据），拆分意味着在没有后续 PR 的情况下功能没有可见 UI。 默认开启的层数上限 5 是行为变更 —— 任何子代理现在可以继续委派 —— 但 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal (before reading the diff): I'd add a Comparison with the diff: the PR's approach matches and exceeds my proposal in several ways:
No critical blockers found. No clear AGENTS.md violations — no over-abstraction, no unnecessary duplication, code in the right packages. The comments are substantive (explaining why each guard exists, not what it does) and load-bearing for future maintainers. Unit TestsRan all 8 key test suites in the worktree:
Build & Typecheck
Real-Scenario Testing (tmux, bundled binary)Flag validation against Full E2E with real model calls requires API keys not available in this CI environment. Maintainer @wenshao's independent verification at 中文说明代码审查独立方案（读 diff 之前）： 我会添加 与 diff 的对比： PR 的方案在多个方面匹配并超越了我的方案：
未发现关键阻断问题。 无明显的 AGENTS.md 违规 —— 无过度抽象、无不必要的重复、代码在正确的包中。注释是实质性的（解释每个守卫为什么存在，而非做了什么），对未来维护者有承载价值。 单元测试在工作树中运行了全部 8 个关键测试套件：332 个测试全部通过。 构建与类型检查
真实场景测试（tmux，打包产物）针对 使用真实模型调用的完整 E2E 需要此 CI 环境中不可用的 API 密钥。维护者 @wenshao 在 — Qwen Code · qwen3.7-max |
ReflectionStepping back and looking at the whole picture: this PR solves a real problem — single-level sub-agent delegation forces all coordination back through the top-level context, which is a genuine bottleneck for complex tasks like multi-module migrations or fan-out research. Does the approach match or exceed my independent proposal? Yes. The shared Is the code straightforward? The core enforcement code in The comments throughout the diff are substantive — they explain why each guard exists (e.g., "a rule missed on the runtime side is a silent spawn bypass, missed on the schema side it burns model turns on guaranteed-rejected calls"), which is exactly the kind of comment that helps future maintainers. This is the right density. Is every change necessary? The 40-file surface breaks down into: backend enforcement (~15 files), TUI tree rendering (~15 files), tests (~8 files), docs/settings (~2 files). The backend and UI changes are coupled (the tree display needs the lineage metadata the backend now persists), and splitting them would ship a feature with no visible UX. The test additions are proportional — 332 tests covering the enforcement predicates, resume depth, fork/teammate/workflow guards, and tree rendering. Do the results match the promise? Build clean, typecheck clean, 332 tests pass, flag validation confirmed in tmux. Maintainer @wenshao's independent E2E at The main risk is real and acknowledged: default-on nesting changes behavior for every existing setup. A sub-agent can now delegate further. But the mitigation is straightforward ( One minor note: the PR body has been updated to reflect that #6191's tree rendering is now in this branch — the description is now consistent with the diff. This is a well-designed, well-tested feature PR that ships what it promises. The two-layer enforcement with a shared predicate is the right architecture, the resume identity pinning handles the subtle edge cases, and the v1 scoping is disciplined. LGTM. 中文说明反思退一步看全局：这个 PR 解决了一个真实问题 —— 单层子代理委派迫使所有协调回流到顶层上下文，这对多模块迁移或扇出研究等复杂任务是真正的瓶颈。 方案是否匹配或超越独立方案？ 是的。共享的 代码是否直观？ 通篇注释是实质性的 —— 它们解释每个守卫为什么存在，这正是帮助未来维护者的注释类型。密度正确。 每个改动是否必要？ 40 个文件的范围分解为：后端强制（约 15 个文件）、TUI 树形渲染（约 15 个文件）、测试（约 8 个文件）、文档/设置（约 2 个文件）。后端和 UI 变更是耦合的（树形展示需要后端现在持久化的血缘元数据），拆分意味着发布一个没有可见 UI 的功能。测试添加与范围相称 —— 332 个测试覆盖了强制判定、恢复深度、fork/队友/工作流守卫和树形渲染。 结果是否符合承诺？ 构建干净、类型检查干净、332 个测试通过、tmux 中确认了 flag 验证。维护者 @wenshao 在 主要风险是真实且已被承认的： 默认开启的嵌套对所有现有环境是行为变更。子代理现在可以继续委派。但缓解措施直接（ 一个小备注： PR 正文已更新以反映 #6191 的树形渲染现在在本分支中 —— 描述与 diff 一致。 这是一个设计良好、测试充分的功能 PR，交付了它所承诺的。共享判定函数的两层强制是正确的架构，恢复身份锚定处理了微妙的边界情况，v1 的范围控制严谨。LGTM。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
4 participants
What this PR does
Sub-agents can now launch sub-agents of their own, up to a configurable nesting depth. A new
model.maxSubagentDepthsetting (default5) bounds how many levels of delegation are allowed:1reproduces the previous behavior exactly (sub-agents exist but cannot delegate further), and values below 1 are clamped so the knob can never disable sub-agents outright.Enforcement is layered so the cap holds even against misbehaving models. At schema-preparation time, a sub-agent is only offered the agent-spawning tool while another level is still permitted — a sub-agent at the deepest allowed level never sees it. At runtime, a guard on the spawning tool independently rejects any attempt that would exceed the cap, returning a clear error telling the model to finish the task with its own tools. Both layers derive from one shared depth predicate so they cannot drift apart.
Nesting is deliberately scoped in this first version, and the scoping removes no existing capability: teammates, fork children, and workflow-launched sub-agents could not spawn sub-agents before this change (the spawning tool was unconditionally excluded from each of their toolsets), and they still cannot. The explicit guards this PR adds for them exist because the exclusion is no longer a single unconditional rule — depth gating re-admits the spawning tool for ordinary sub-agents, and forks and workflow agents share the same wildcard tool path, so without their own guards they would have silently gained it as a side effect. Fork children additionally get a runtime backstop so even a hallucinated spawn call is rejected. The workflow tool itself stays excluded from all sub-agents, unchanged. One capability is deliberately granted: in-process interactive agents (Arena sessions), which previously could never spawn, now carry a proper agent identity and can nest like any first-level agent.
The PR also hardens every path where an agent's nesting identity could previously be lost: a resumed background agent restores its original depth and its launch-time depth cap (both persisted with the agent's metadata) instead of being treated as a fresh top-level agent after a restart; deferred tool-approval continuations re-enter the agent's context at its original depth rather than depth zero; and in-process interactive agents (Arena sessions and in-process teammates) now carry a proper agent identity, so they are depth-gated as agents instead of being mistaken for the top-level session.
This PR now also includes the TUI tree display of nesting: the stacked UI PR (#6191 — indented parent/child rows in the live agent panel and background-tasks view, depth-aware navigation and cancellation) has been reviewed and merged into this branch, so merging this PR brings both the backend enforcement and the tree rendering.
Why it's needed
Delegation is currently hard-capped at one level: the main session can spawn a sub-agent, but that sub-agent is a leaf. For larger tasks — a research agent that wants to fan out its own focused readers, or a migration agent that delegates per-module work — the single level forces everything back through the top-level context. Allowing bounded recursive delegation lets sub-agents decompose their own work while the depth cap, the existing global background-concurrency limit, and serial foreground execution together keep the blast radius controlled.
Reviewer Test Plan
How to verify
All scenarios run headless with auto-approval; the decisive evidence is which tools each sub-agent's API request advertises (enable request logging and compare tool lists between levels — the allowed level has exactly one more tool, the agent tool, than the leaf level).
"model": { "maxSubagentDepth": 2 }in the project settings and repeat. Expect the level-2 sub-agent's schema to exclude the agent tool and no third-level agent to appear.1and repeat. Expect the first sub-agent to lack the agent tool entirely and report it cannot delegate — identical to the pre-feature contract.Evidence (Before & After)
Before: a sub-agent's tool schema never includes the agent tool; asking for a nested chain makes the sub-agent report it cannot delegate. After: with the default cap, the nested chain completes and the marker word propagates; at the cap boundary the leaf level's schema drops exactly the agent tool. Full E2E table (4 groups, all passing, with API-log evidence) will be posted as a PR comment.
Tested on
Environment (optional)
Local bundle (
npm run build && npm run bundle, thennode dist/cli.js) with real model calls; unit suites via vitest per package.Risk & Scope
model.maxSubagentDepthto1restores the old behavior. A tree-wide spawn/cost budget is the natural follow-up.Linked Issues
None.
中文说明
本 PR 的内容
子代理现在可以启动自己的子代理，嵌套层数由新的
model.maxSubagentDepth设置项控制（默认5）。设为1时完全复现旧行为（子代理存在但不能继续委派），小于 1 的值会被钳制为 1，因此该设置永远不会彻底禁用子代理。强制机制分为两层，即使模型行为异常，深度上限也依然有效。在工具模式准备阶段，只有在仍允许再嵌套一层时，子代理才会看到用于派生代理的工具——处于最深允许层级的子代理根本看不到它。在运行时，派生工具上还有一道独立的守卫，会拒绝任何超出上限的派生请求，并返回明确的错误提示，让模型改用自身工具完成任务。两层机制共享同一个深度判定函数，因此不会出现不一致。
第一版有意收窄了范围，且这种收窄没有移除任何既有能力：在本次变更之前，队友（teammate）、fork 子代理和 workflow 启动的子代理本来就无法派生子代理（派生工具在它们各自的工具集中被无条件排除），现在依然如此。本 PR 为它们添加显式守卫，是因为排除规则不再是单一的无条件规则——深度门控会为普通子代理重新引入派生工具，而 fork 和 workflow 代理走的是同一条通配符工具路径，没有各自的守卫就会作为副作用悄悄获得该工具。fork 子代理还额外获得一道运行时兜底，即使模型凭空调用派生工具也会被拒绝。workflow 工具本身对所有子代理保持排除，不变。有一项能力是有意授予的：进程内交互式代理（Arena 会话）之前完全无法派生，现在拥有正确的代理身份，可以像任何一级代理一样嵌套。
本 PR 还加固了所有可能丢失代理嵌套身份的路径：后台代理恢复时会还原其原始深度和启动时的深度上限（二者均随代理元数据持久化），重启后不会被当作全新的顶层代理；延迟的工具审批续接会以原始深度重新进入代理上下文，而不是深度 0；进程内交互式代理（Arena 会话与进程内队友）现在拥有正确的代理身份，会按代理而非顶层会话来做深度门控。
本 PR 现在同时包含 TUI 的嵌套树形展示：叠加的 UI PR（#6191 —— 实时代理面板与后台任务视图中的父/子行缩进、感知深度的导航与取消）已经过评审并合入本分支，因此合并本 PR 会同时带入后端强制机制和树形渲染。
为什么需要
目前委派被硬性限制在一层：主会话可以派生子代理，但子代理是叶子节点。对于更大的任务——例如想要展开多个专注阅读器的研究代理，或按模块委派工作的迁移代理——单层限制迫使所有内容都回流到顶层上下文。允许有界的递归委派后，子代理可以自行分解工作，同时深度上限、现有的全局后台并发限制和前台串行执行共同控制影响范围。
审阅者测试计划
如何验证
所有场景均以无头模式加自动批准运行；决定性证据是每个子代理的 API 请求中声明了哪些工具（开启请求日志，对比不同层级的工具列表——允许嵌套的层级恰好比叶子层级多一个工具，即派生代理的工具）。
"model": { "maxSubagentDepth": 2 }后重复。预期第 2 层子代理的模式中不含派生工具，且不出现第三层代理。1后重复。预期第一个子代理完全没有派生工具，并报告无法继续委派——与功能之前的契约一致。证据（前后对比）
之前：子代理的工具模式从不包含派生工具；要求嵌套链时子代理会报告无法委派。之后：默认上限下嵌套链完整执行且标记词逐层传回；在上限边界处，叶子层级的模式恰好少了派生工具这一项。完整的 E2E 表格（4 组全部通过，附 API 日志证据）将作为 PR 评论发布。
已测试平台
macOS ✅；Windows / Linux 未本地验证（依赖 CI）。
风险与范围
model.maxSubagentDepth设为1可恢复旧行为。树级派生/成本预算是自然的后续工作。关联 Issue
无。