Skip to content

fix(core): Allow subagents to exit plan mode - #6026

Merged
doudouOUC merged 4 commits into
QwenLM:mainfrom
doudouOUC:codex/fix-subagent-plan-mode-state
Jun 30, 2026
Merged

fix(core): Allow subagents to exit plan mode#6026
doudouOUC merged 4 commits into
QwenLM:mainfrom
doudouOUC:codex/fix-subagent-plan-mode-state

Conversation

@doudouOUC

@doudouOUC doudouOUC commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

This PR fixes subagent approval-mode overrides so a subagent can actually leave plan mode after exit_plan_mode succeeds. The override now owns mutable approval-mode and plan-gate state instead of returning the initial mode from a fixed getter, and it keeps AUTO-mode denial tracking isolated for the child config.

It also tightens the AUTO dangerous-rule cleanup path so a child that starts in AUTO, leaves AUTO, or enters AUTO during execution restores the shared permission manager rules at the right time without relying on the old initial-mode-only cleanup behavior.

Why it's needed

Subagents launched while the parent is in plan mode inherited PLAN, but the child config's getApprovalMode() always returned the initial mode captured at creation time. That meant exit_plan_mode could report success while the scheduler still believed the child was in plan mode, so exec tools such as shell calls continued to be blocked and the model could loop through repeated plan exits and failed tool calls.

This change makes the child config's approval mode behave like a normal mutable config, which breaks that loop while preserving the existing policy that subagents may inherit plan mode.

Reviewer Test Plan

How to verify

Run cd packages/core && npx vitest run src/tools/agent/agent-override.test.ts and confirm the new regression cases pass: a plan-mode child exits to default, exec tools are no longer plan-blocked after exit, parent and child plan state remain isolated, and AUTO cleanup handles child entry and exit paths. For broader validation, run cd packages/core && npx vitest run src/core/permissionFlow.test.ts, cd packages/core && npx vitest run src/tools/exitPlanMode.test.ts, cd packages/core && npx vitest run src/config/config.test.ts, cd packages/core && npm run lint, and from the repository root run npm run build && npm run typecheck.

Evidence (Before & After)

N/A. This is a non-UI core behavior fix covered by unit tests and build/typecheck validation.

Tested on

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

Environment (optional)

Local macOS worktree with Node.js v22.22.3 and npm. Validation used Vitest, ESLint, Prettier checks, repository build, and repository typecheck.

Risk & Scope

  • Main risk or tradeoff: The override now mirrors more of Config's mutable approval-mode state, so the main risk is a mismatch with existing wrapper lifecycle assumptions; regression coverage was added for plan exit, plan state isolation, and AUTO cleanup paths.
  • Not validated / out of scope: This does not change the product policy for whether ordinary subagents may use plan tools, and it does not add a generic model loop detector.
  • Breaking changes / migration notes: None expected; public APIs, subagent config shape, and tool names are unchanged.

Linked Issues

Closes #6036

中文说明

What this PR does

这个 PR 修复了子 Agent 的 approval-mode override,使子 Agent 在 exit_plan_mode 成功后能够真正退出 plan mode。override 现在拥有可变的 approval-mode 和 plan-gate 状态,不再通过固定 getter 返回创建时的初始 mode,同时也为子 Config 隔离 AUTO-mode denial tracking 状态。

它还收紧了 AUTO dangerous-rule cleanup 路径:无论子 Agent 初始就在 AUTO、运行中离开 AUTO,还是运行中进入 AUTO,都能在正确时机恢复共享 PermissionManager 的规则,不再依赖只看初始 mode 的旧 cleanup 行为。

Why it's needed

父会话在 plan mode 时启动的子 Agent 会继承 PLAN,但子 Config 的 getApprovalMode() 一直返回创建时捕获的初始 mode。这会导致 exit_plan_mode 看起来成功了,但调度器仍然认为子 Agent 处于 plan mode,于是 shell 等 exec 工具继续被拦截,模型可能进入反复退出 plan 和反复工具失败的循环。

这个改动让子 Config 的 approval mode 像普通 Config 一样可变,从而打断这个循环,同时保留现有的子 Agent 可继承 plan mode 的策略。

Reviewer Test Plan

How to verify

运行 cd packages/core && npx vitest run src/tools/agent/agent-override.test.ts,确认新增回归用例通过:plan-mode 子 Config 可以退出到 default,退出后 exec 工具不再被 plan block,父子 plan 状态保持隔离,并且 AUTO cleanup 覆盖子 Agent 进入和退出 AUTO 的路径。更完整的验证可以运行 cd packages/core && npx vitest run src/core/permissionFlow.test.tscd packages/core && npx vitest run src/tools/exitPlanMode.test.tscd packages/core && npx vitest run src/config/config.test.tscd packages/core && npm run lint,以及在仓库根目录运行 npm run build && npm run typecheck

Evidence (Before & After)

N/A。这是非 UI 的 core 行为修复,已通过单元测试、构建和类型检查验证。

Tested on

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

Environment (optional)

本地 macOS worktree,Node.js v22.22.3 和 npm。验证使用了 Vitest、ESLint、Prettier check、仓库构建和仓库类型检查。

Risk & Scope

  • Main risk or tradeoff: override 现在会镜像更多 Config 的可变 approval-mode 状态,主要风险是与既有 wrapper 生命周期假设不一致;本 PR 已为 plan 退出、plan 状态隔离和 AUTO cleanup 路径添加回归覆盖。
  • Not validated / out of scope: 本 PR 不改变普通子 Agent 是否可以使用 plan 工具的产品策略,也不新增通用模型循环检测器。
  • Breaking changes / migration notes: 预期没有破坏性变更;public API、subagent 配置形态和工具名均未改变。

Linked Issues

Closes #6036

@doudouOUC
doudouOUC marked this pull request as ready for review June 29, 2026 16:07
Copilot AI review requested due to automatic review settings June 29, 2026 16:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@qwen-code-ci-bot

qwen-code-ci-bot commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — all required sections present, bilingual, test plan included.

On direction: this is a real bug. Subagents inheriting plan mode get a frozen getApprovalMode closure, so exit_plan_mode succeeds at the API level but the scheduler still blocks exec tools — the model loops on repeated plan exits and failed shell calls. This directly degrades the multi-agent experience and is squarely within core's responsibility. No CHANGELOG reference needed; this is a correctness fix, not a feature debate.

On approach: the fix is minimal and well-targeted. The root cause is one line — override.getApprovalMode = (): ApprovalMode => mode — and the fix replaces it with own properties + prototype delegation so Config's existing getApprovalMode/setApprovalMode just work. The plan state isolation (shallow-copied planGateState, independent planGateEntryCounter) and AUTO denial state reset are the right amount of defense. The custom setApprovalMode wrapper that nulls permissionManager when the parent is in AUTO is a bit subtle but necessary — it prevents Config's built-in strip/restore from double-firing on the parent's lifecycle. Two files, +318/-27, all test additions except the implementation change. No scope creep.

Moving on to code review and testing. 🔍

中文说明

感谢贡献!

模板完整 ✓ — 所有必填部分齐全,中英双语,测试计划完整。

方向:这是一个真实的 bug。子 Agent 继承 plan mode 后 getApprovalMode 是固定闭包,exit_plan_mode 在 API 层面成功了但调度器仍然拦截 exec 工具——模型会反复退出 plan 并反复工具调用失败。这直接影响多 Agent 体验,属于 core 的职责范围。不需要 CHANGELOG 参考,这是正确性修复。

方案:修复精准且最小化。根因是一行代码 override.getApprovalMode = (): ApprovalMode => mode,修复用自有属性 + 原型委托替代,让 Config 现有的 getApprovalMode/setApprovalMode 直接生效。plan 状态隔离(浅拷贝 planGateState、独立 planGateEntryCounter)和 AUTO denial 状态重置是恰当的防御。自定义 setApprovalMode 包装器在父级处于 AUTO 时临时置空 permissionManager 有些微妙但是必要的——防止 Config 内置的 strip/restore 在父级生命周期上重复触发。两个文件,+318/-27,除实现改动外全是测试新增。没有范围蔓延。

进入代码审查和测试 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

qwen-code-ci-bot commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal: Given the bug description (subagent stuck in plan mode after exit_plan_mode), I'd fix it by making the override's getApprovalMode read from a mutable own property instead of a frozen closure, and let setApprovalMode delegate to Config's prototype method. Plan gate state and pre-plan mode need own properties to avoid mutating the parent. AUTO denial tracking should also be isolated. The cleanup function should only restore AUTO rules if the child finishes in AUTO while the parent isn't in AUTO.

Comparison with the PR: The PR's approach matches this exactly. It replaces the closure with override.approvalMode = mode + override.getApprovalMode = Config.prototype.getApprovalMode, adds own prePlanMode, planGateState (shallow-copied), planGateEntryCounter, and autoModeDenialState (fresh via createDenialState()). The custom setApprovalMode wrapper correctly handles the AUTO-parent edge case by temporarily nulling permissionManager to suppress Config's built-in strip/restore when the parent owns the AUTO lifecycle.

Reuse check: The PR reuses Config.prototype.getApprovalMode and Config.prototype.setApprovalMode directly — no parallel implementation. createDenialState() from denialTracking.ts is reused for fresh AUTO state. No duplication.

Findings:

  • No critical blockers. The implementation is correct. The permissionManager nulling trick in the custom setApprovalMode is well-justified — it prevents Config's built-in AUTO strip/restore from double-firing on the parent's shared PM when the parent is already in AUTO. The finally block correctly restores the inherited PM even if a TrustGateError is thrown.

  • No AGENTS.md violations. Minimal change (one function rewritten, one cleanup simplified), no over-abstraction, code stays in the right file.

  • One minor observation (non-blocking): the override.setApprovalMode wrapper calls Config.prototype.setApprovalMode.call(override, ...), which means the override is exercising Config's internal field names (approvalMode, prePlanMode, planGateState, permissionManager, autoModeDenialState) as own properties. This coupling to Config's private field names is already documented in the comment and is the standard prototype-delegation pattern. Acceptable.

Test Results

All 20 tests in agent-override.test.ts pass, including the 12 new regression cases covering plan-mode exit, plan state isolation, AUTO cleanup paths, and permission manager restoration on error.

Related regression suites also pass cleanly: permissionFlow.test.ts (20 tests), exitPlanMode.test.ts (33 tests), config.test.ts (292 tests) — 345 total, no failures.

Build succeeds with 0 errors (15 pre-existing lint warnings in unrelated files).

github-runner@iZt4neqpisqczs6hsm7xn2Z:~/actions-runner-5/_work/qwen-code/qwen-code$ cd packages/core && npx vitest run src/tools/agent/agent-override.test.ts 2>&1

 RUN  v3.2.4 /home/github-runner/actions-runner-5/_work/qwen-code/qwen-code/packages/core
      Coverage enabled with v8

 ✓ src/tools/agent/agent-override.test.ts (20 tests) 85ms

 Test Files  1 passed (1)
      Tests  20 passed (20)
   Start at  09:41:08
   Duration  5.04s (transform 1.86s, setup 17ms, collect 2.89s, tests 85ms, environment 0ms, prepare 76ms)

JUNIT report written to /home/github-runner/actions-runner-5/_work/qwen-code/qwen-code/packages/core/junit.xml
 % Coverage report from v8
github-runner@iZt4neqpisqczs6hsm7xn2Z:~/actions-runner-5/_work/qwen-code/qwen-code/packages/core$
中文说明

代码审查

独立方案: 根据 bug 描述(子 Agent 在 exit_plan_mode 后卡在 plan mode),我会让 override 的 getApprovalMode 读取可变的自有属性而非冻结闭包,并让 setApprovalMode 委托给 Config 原型方法。Plan gate 状态和 pre-plan mode 需要自有属性以避免修改父级。AUTO denial 跟踪也应隔离。cleanup 函数仅在子级以 AUTO 结束且父级不在 AUTO 时恢复规则。

与 PR 对比: PR 方案完全一致。用 override.approvalMode = mode + override.getApprovalMode = Config.prototype.getApprovalMode 替代闭包,添加自有 prePlanModeplanGateState(浅拷贝)、planGateEntryCounterautoModeDenialState(通过 createDenialState() 新建)。自定义 setApprovalMode 包装器正确处理了父级在 AUTO 时的边界情况——临时置空 permissionManager 以抑制 Config 内置 strip/restore 对父级共享 PM 的重复触发。

复用检查: PR 直接复用 Config.prototype.getApprovalModeConfig.prototype.setApprovalMode——没有平行实现。createDenialState()denialTracking.ts 复用于新建 AUTO 状态。无重复。

发现:

  • 无阻断性问题。 实现正确。自定义 setApprovalMode 中的 permissionManager 置空技巧是合理的——当父级已拥有 AUTO 生命周期时,防止 Config 内置 AUTO strip/restore 在共享 PM 上重复触发。finally 块即使抛出 TrustGateError 也正确恢复继承的 PM。

  • 无 AGENTS.md 违规。 最小化改动(一个函数重写,一个 cleanup 简化),无过度抽象,代码在正确文件中。

测试结果

agent-override.test.ts 全部 20 个测试通过,包括 12 个新增回归用例,覆盖 plan-mode 退出、plan 状态隔离、AUTO cleanup 路径和错误时的 permission manager 恢复。

相关回归套件也全部通过:permissionFlow.test.ts(20 个)、exitPlanMode.test.ts(33 个)、config.test.ts(292 个)——共 345 个,无失败。

构建成功,0 错误(15 个无关文件的预存 lint 警告)。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

qwen-code-ci-bot commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

This is a clean, well-understood bug fix. The core problem — a frozen getApprovalMode closure on the prototype-delegated override — is exactly the kind of subtle bug that's easy to introduce when wiring up prototype chains and hard to catch without the specific scenario (subagent inherits plan mode, tries to exit it, exec tools still blocked). The fix is the natural one: own properties + prototype delegation, so Config's existing machinery just works.

The implementation is minimal and focused. Two files changed, the implementation is ~90 lines of logic in createApprovalModeOverride, and the rest is 12 regression tests that cover every edge case I can think of — plan exit, plan state isolation, AUTO cleanup on entry/exit, parent-AUTO suppression, and TrustGateError recovery. The setApprovalMode wrapper that nulls permissionManager to prevent double strip/restore is the one subtle piece, and it's well-justified by the parent AUTO lifecycle concern.

My independent proposal matched the PR's approach exactly. I didn't find a simpler path. The tests all pass (20/20 primary + 345/345 related), build is clean. Ships the fix without scope creep.

Approving. ✅

中文说明

这是一个干净、理解透彻的 bug 修复。核心问题——原型委托 override 上冻结的 getApprovalMode 闭包——正是在搭建原型链时容易引入、没有特定场景难以发现的那种微妙 bug(子 Agent 继承 plan mode,尝试退出,exec 工具仍被拦截)。修复方案是自然的:自有属性 + 原型委托,让 Config 现有机制直接生效。

实现最小化且聚焦。改动两个文件,实现约 90 行逻辑在 createApprovalModeOverride 中,其余是 12 个回归测试,覆盖了能想到的每个边界情况——plan 退出、plan 状态隔离、AUTO 进入/退出的 cleanup、父级 AUTO 抑制、以及 TrustGateError 恢复。setApprovalMode 包装器置空 permissionManager 以防止双重 strip/restore 是唯一微妙的部分,父级 AUTO 生命周期的考量充分证明了其必要性。

我的独立方案与 PR 方案完全一致。没有找到更简单的路径。测试全部通过(20/20 主测试 + 345/345 相关测试),构建干净。无范围蔓延。

批准。✅

Qwen Code · qwen3.7-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@doudouOUC
doudouOUC force-pushed the codex/fix-subagent-plan-mode-state branch from 3291693 to b58176e Compare June 29, 2026 16:29
Comment thread packages/core/src/tools/agent/agent.ts
Prevent a child approval-mode override from restoring shared AUTO permission rules while the parent config is still in AUTO mode.

Add regression coverage for cleanup and child AUTO exits when the parent owns the AUTO lifecycle.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Ensure approval-mode overrides own the Config prototype getter so resumed agents and other wrapped configs read the child approvalMode instead of an inherited parent mock getter.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Comment thread packages/core/src/tools/agent/agent.ts
Comment thread packages/core/src/tools/agent/agent-override.test.ts
Comment thread packages/core/src/tools/agent/agent.ts
Comment thread packages/core/src/tools/agent/agent-override.test.ts
Document the Config private-field coupling in the approval override and add focused assertions for plan blocking, plan gate copies, denial-state isolation, and AUTO-parent permission-manager restoration on throws.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>

@DragonnZhang DragonnZhang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No additional findings at this head. The mutable override.approvalMode pattern with Config.prototype.getApprovalMode delegation breaks the fixed-getter loop correctly. The AUTO dangerous-rule cleanup suppression when the parent is already in AUTO, and the parent/child plan state isolation, are all covered by the new test suite. The implementation mirrors Config's own mutable approval-mode lifecycle without introducing new shared state risks.

— claude-sonnet-4-6 via Qwen Code /review


Generated by Claude Code

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Downgraded from Approve to Comment: CI still running.

No review findings. The core approach — replacing the frozen getApprovalMode closure with mutable own properties on the prototype-delegated override — is correct and well-reasoned. The AUTO strip/restore lifecycle is carefully handled across all mode-transition scenarios. Build, typecheck, and all 18 tests pass.

— qwen3.7-max via Qwen Code /review

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No review findings. Downgraded from Approve to Comment: CI still running.

The core approach — replacing the frozen getApprovalMode closure with mutable own properties delegated through the prototype chain — is correct and well-tested. The AUTO cleanup tightening properly handles child entry/exit paths.

— qwen3.7-max via Qwen Code /review

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found. LGTM.

— GPT-5 via Qwen Code /review

@wenshao

wenshao commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@wenshao

wenshao commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

✅ Maintainer local verification — real build + tmux (PR #6026)

Verified 7fce62056 (PR head; merge-base 565e4bc43) in an isolated worktree with a fresh npm ci, running everything in a real tmux session. This goes beyond the PR's stated test plan: every regression test was mutation-tested to prove it is load-bearing, and the fix was validated against the real compiled dist (not just src), with a before/after A/B.

Verdict: the change does exactly what it claims, the new tests are non-vacuous, and no regressions or bugs were found. LGTM to merge.


1. Unit tests — all green

Suite Result
agent-override.test.ts (the PR's regression file) 20/20 pass (11 new cases)
permissionFlow.test.ts + exitPlanMode.test.ts + config.test.ts 345/345 pass

The new cases use the real Config and the real isPlanModeBlocked exec gate — not mocks of the gate — so a pass means the production gating logic actually unblocks exec after the child exits plan mode.

2. Mutation testing — every new test is load-bearing

I reverted each load-bearing line of the fix one at a time and confirmed exactly the intended test(s) fail (and nothing spurious):

Mutation (reverted to pre-PR behavior) Tests that fail
A — central fix: getApprovalMode = Config.prototype.getApprovalMode → fixed closure () => mode 4 incl. "stops plan-mode blocking exec tools after a child exits plan mode"expected 'plan' to be 'default'
B — drop autoModeDenialState = createDenialState() (denial-state isolation) 1"starts child AUTO denial state independent from the parent"
C — drop the cleanup base !== AUTO guard 1"does not restore AUTO rules on cleanup when the parent is already in AUTO mode"
D — bypass the setApprovalMode PM-suppression wrapper 1"does not restore AUTO rules when a child leaves AUTO while the parent stays in AUTO"

Each mutation was restored before the next; the working tree was confirmed byte-identical to PR head afterward.

3. Real compiled-dist A/B probe (the actual product, not src)

Built the production source to dist (clean, declarations emitted) and drove the actual built createApprovalModeOverride + Config + isPlanModeBlocked through the exact bug scenario (parent in PLAN → child inherits PLAN → child runs what exit_plan_mode does → check the exec gate):

Compiled build child after exit_plan_modesetApprovalMode(DEFAULT) exec gate parent Result
Fixed (this PR) reports DEFAULT un-blocked stays DEFAULT (isolated) PASS
Reverted (Mut A), rebuilt stuck reporting PLAN still BLOCKED stays DEFAULT bug reproduced

The reverted build reproduces precisely the symptom described in the PR ("exit_plan_mode reports success while the scheduler still believes the child is in plan mode → exec tools stay blocked → the model loops"). The "parent stays DEFAULT" assertion holds in both builds, which correctly confirms the bug was never parent-leakage — it was solely the child's getApprovalMode() ignoring writes.

4. Build / typecheck / lint

  • Production build: clean — 0 errors, dist + .d.ts emitted.
  • No property mangling: core builds via tsc --build (not esbuild prop-mangling), and the compiled output preserves every mirrored private-field name (approvalMode, prePlanMode, planGateState, autoModeDenialState). This is the one thing a src-only test can't catch, so it's worth stating explicitly: the own-property/prototype-method trick is build-safe.
  • The PR's 2 files: 0 typecheck errors.
  • ESLint on both changed files: clean.

Note: a full-suite tsc --noEmit over packages/core reports 8 errors, but all 8 are in unrelated src/providers/__tests__/* (a package self-import that needs a freshly built dist). They reproduce identically on the merge-base without this PR, and clear once dist is built — i.e. a pre-existing local-toolchain artifact, not introduced by this PR.

5. Code review notes

  • Integration chain traced end-to-end: spawn site (agent.ts resolveSubagentApprovalMode → child inherits PLAN) → exit_plan_mode calls this.config.setApprovalMode(...) (exitPlanMode.ts) → scheduler gates exec via getApprovalMode() through isPlanModeBlocked. The fix is the missing link that makes the child's setApprovalMode observable.
  • planGateState deep-copy is complete: the only nested-mutable field on PlanGateState is lastFindings (an array), which the PR copies; every other field is primitive. No parent/child aliasing.
  • AUTO strip/restore throw-safety: the setApprovalMode wrapper nulls the shared PM inside a try/finally and restores it even when the trust-gate throws (covered by the new throw test).
🇨🇳 中文版(完整对应)

✅ 维护者本地验证 —— 真实构建 + tmux(PR #6026

独立 worktree + 全新 npm ci 环境、全程真实 tmux 会话中验证了 7fce62056(PR head;merge-base 565e4bc43)。验证深度超出 PR 自带的 test plan:每个回归测试都做了变异测试以证明它真正承重,并且把修复放到真实编译产物 dist(而非仅 src)上做了前后 A/B 对照。

结论:改动确实实现了它声称的功能,新增测试非空过,未发现回归或 bug。同意合并。


1. 单元测试 —— 全绿

套件 结果
agent-override.test.ts(本 PR 的回归文件) 20/20 通过(11 个新用例)
permissionFlow.test.ts + exitPlanMode.test.ts + config.test.ts 345/345 通过

新用例用的是真实 Config真实isPlanModeBlocked exec 门(不是对门做 mock),因此通过即代表生产环境的门控逻辑在子 Agent 退出 plan 后确实解除了 exec 拦截。

2. 变异测试 —— 每个新测试都承重

我逐行回退修复的承重代码,确认恰好是预期的测试失败(且没有多余失败):

变异(回退成 PR 前行为) 失败的测试
A —— 核心修复:getApprovalMode = Config.prototype.getApprovalMode → 固定闭包 () => mode 4 个,含 "stops plan-mode blocking exec tools after a child exits plan mode"expected 'plan' to be 'default'
B —— 去掉 autoModeDenialState = createDenialState()(denial 状态隔离) 1 个 —— "starts child AUTO denial state independent from the parent"
C —— 去掉 cleanup 的 base !== AUTO 守卫 1 个 —— "does not restore AUTO rules on cleanup when the parent is already in AUTO mode"
D —— 绕过 setApprovalMode 的 PM 抑制包装 1 个 —— "does not restore AUTO rules when a child leaves AUTO while the parent stays in AUTO"

每个变异在下一个之前都已还原;之后确认工作树与 PR head 逐字节一致。

3. 真实编译 dist 的 A/B 探针(真实产物,非 src

把生产源码构建到 dist(干净、生成声明文件),然后驱动真实编译后的 createApprovalModeOverride + Config + isPlanModeBlocked 走完整 bug 场景(父在 PLAN → 子继承 PLAN → 子执行 exit_plan_mode 所做的事 → 检查 exec 门):

编译产物 exit_plan_modesetApprovalMode(DEFAULT) 后的子 exec 门 结果
修复版(本 PR) 报告 DEFAULT 解除拦截 保持 DEFAULT(隔离) PASS
回退版(变异 A)重建 仍卡在 PLAN 仍被拦截 保持 DEFAULT 复现 bug

回退版精确复现了 PR 描述的症状("exit_plan_mode 看起来成功了,但调度器仍认为子在 plan mode → exec 工具继续被拦 → 模型陷入循环")。"父保持 DEFAULT" 这个断言在两个版本里都成立,恰好印证了 bug 从来不是父状态泄漏,而纯粹是子的 getApprovalMode() 忽略了写入。

4. 构建 / 类型检查 / Lint

  • 生产构建:干净 —— 0 错误,产出 dist + .d.ts
  • 无属性名混淆:core 用 tsc --build 构建(不是会混淆属性名的 esbuild),编译输出保留了所有镜像的私有字段名(approvalModeprePlanModeplanGateStateautoModeDenialState)。这正是仅看 src 的测试覆盖不到的一点,值得明确指出:这个 own-property/原型方法的技巧是构建安全的。
  • 本 PR 的 2 个文件0 类型错误。
  • ESLint 对两个改动文件:干净。

说明:对 packages/core 跑全量 tsc --noEmit 会报 8 个错误,但全部位于无关的 src/providers/__tests__/*(一个需要先构建 dist 的包内自引用)。它们在不含本 PR 的 merge-base 上完全一样复现,并在构建 dist 后消失 —— 即本地工具链的既有产物,并非本 PR 引入

5. 代码评审备注

  • 端到端集成链已核对:spawn 处(agent.ts resolveSubagentApprovalMode → 子继承 PLAN)→ exit_plan_mode 调用 this.config.setApprovalMode(...)exitPlanMode.ts)→ 调度器经 isPlanModeBlockedgetApprovalMode() 门控 exec。本修复正是让子的 setApprovalMode 变得可观测的那一环。
  • planGateState 深拷贝完整PlanGateState 上唯一的嵌套可变字段是 lastFindings(数组),本 PR 已拷贝;其余字段均为基础类型。父子之间无别名共享。
  • AUTO strip/restore 抛错安全setApprovalMode 包装在 try/finally 内置空共享 PM,即使 trust-gate 抛错也会还原(已被新增的抛错测试覆盖)。

Verified locally by the maintainer with a real isolated build + mutation testing + compiled-dist A/B. Tested on 🍏 macOS (Node v22.22.2).

@chiga0 chiga0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overview — Independent Review (Phase 1 blind → Phase 2 cross-validation)

Verdict: LGTM. Clean, well-understood bug fix. The root cause — a frozen getApprovalMode closure on the prototype-delegated override — is correctly addressed by switching to mutable own properties with Config.prototype delegation. All 5 existing Suggestion findings are stale against HEAD 7fce6205.

Round 0 — Design & Intent

The PR fixes a real bug (#6036): subagents inheriting PLAN mode get a frozen getApprovalMode() that always returns the initial mode, so exit_plan_mode succeeds but the scheduler still blocks exec tools. The fix is the natural one — make the override's approval mode mutable by using own properties and delegating to Config.prototype.getApprovalMode/setApprovalMode. My independent proposal matched the PR's approach exactly.

Round 1 — Architecture

The override correctly mirrors Config's internal state model:

  • approvalMode own property + Config.prototype.getApprovalMode delegation — allows setApprovalMode to mutate the child's mode
  • prePlanMode initialized from parent's current mode (when child starts in PLAN) — correct pre-plan snapshot
  • planGateState deep-copied from parent (spread + lastFindings array copy) — isolates child plan gate mutations from parent
  • planGateEntryCounter seeded from parent's current counter — ensures monotonically increasing entry IDs across parent/child
  • autoModeDenialState fresh via createDenialState() — child starts with zero counters regardless of parent

The setApprovalMode override correctly handles two paths:

  1. Parent not in AUTO: delegates directly to Config.prototype.setApprovalMode.call(override, ...) — full Config behavior including PM strip/restore
  2. Parent in AUTO: temporarily nulls override.permissionManager during the call, preventing Config from modifying the shared PM's rules (parent owns the AUTO strip lifecycle). The try/finally correctly restores the previous PM state even on throw.

Round 2 — Robustness

  • Cleanup correctness: only restores dangerous rules when child finishes in AUTO AND parent is not in AUTO — prevents double-restore when parent owns the lifecycle
  • PM null-out safety: try/finally ensures override.permissionManager is always restored, even if Config.prototype.setApprovalMode throws (e.g., TrustGateError for untrusted folders)
  • Deep copy of plan gate state: { ...basePlanGateState, lastFindings: [...basePlanGateState.lastFindings] } — child mutations don't affect parent's lastFindings array
  • Idempotent cleanup: cleanup doesn't depend on being called only once — it checks current state each time

Round 5 — Bug Fix Specifics

The root cause is fixed, not symptom-masked. The key insight is that Object.create(base) creates a prototype chain where Config methods look up this.approvalMode — by setting own properties with the same names as Config's TS-private fields, the override intercepts these lookups. This is well-documented with the inline comment: "These own properties intentionally mirror Config's TS-private field names."

The Config import was changed from type to value import — needed because Config.prototype.getApprovalMode and Config.prototype.setApprovalMode are now called directly. This is the correct approach: it avoids duplicating Config's complex state transition logic.

Round 5.5 — Caller/Consumer Impact

createApprovalModeOverride is called from 6 files: agent.ts, forkedAgent.ts, subagent-manager.ts, gateReviewAgents.ts, background-agent-resume.ts, and config.ts. All callers receive the same { config, cleanup } handle — no API changes. The behavior change (mutable mode instead of frozen) is the intended fix and should not break any caller.

Test Coverage

12 new tests covering all critical paths:

  • Plan-mode child exits to DEFAULT, parent unchanged
  • Exec tools no longer plan-blocked after exit
  • Child plan state isolated from parent (gate state deep copy, entry counter)
  • Pre-plan mode inherited correctly from non-plan parent
  • AUTO denial state independent from parent
  • AUTO cleanup: child finishes in AUTO → restores rules
  • AUTO cleanup: child leaves AUTO itself → no double-restore
  • AUTO cleanup: parent in AUTO → no restore (parent owns lifecycle)
  • AUTO cleanup: child leaves AUTO while parent in AUTO → no restore
  • AUTO cleanup: non-AUTO child enters AUTO and finishes → restores
  • PM restoration on throw (TrustGateError) → permissionManager properly cleaned up

Cross-Validation

Finding Reviewer My Assessment
S1: Cleanup guard test gap (parent in AUTO) qwen-code-ci-bot Stale — HEAD has does not restore AUTO rules on cleanup when the parent is already in AUTO mode and similar
S2: Private field naming fragility qwen-code-ci-bot Addressed — documented with explicit comment; trade-off is acceptable given test coverage
S3: Tautological isPlanModeBlocked test qwen-code-ci-bot Partially valid — the isPlanModeBlocked assertion with hardcoded false is trivially true, but the test still validates the override mechanism sets mode correctly
S4: Missing test for createDenialState / planGateEntryCounter qwen-code-ci-bot Stale — HEAD has starts child AUTO denial state independent from the parent and isolates child plan state tests
S5: Missing distinct-copy assertion for plan gate qwen-code-ci-bot Stale — HEAD has expect(childGateState).not.toBe(parentGateState) and expect(childGateState?.lastFindings).not.toBe(parentGateState?.lastFindings)

Additional Audit Coverage

  • Config prototype field alignment: verified Config's private fields (approvalMode, prePlanMode, planGateState, planGateEntryCounter, autoModeDenialState) at config.ts:1361-1365 match the override's own property names exactly
  • setApprovalMode PM null-out: verified Config's setApprovalMode at config.ts:4392-4404 checks if (this.permissionManager) before calling strip/restore — nulling it during the call correctly suppresses PM mutation
  • createDenialState returns fresh object: verified it returns { consecutiveBlock: 0, consecutiveUnavailable: 0, totalBlock: 0, totalUnavailable: 0 } — correct zero-state
  • Cleanup timing: cleanup is returned to the caller and called after the subagent finishes — the current override.approvalMode at cleanup time reflects any mode changes made during the subagent's lifetime

This review was generated by QoderWork AI

Merged via the queue into QwenLM:main with commit 05be7b1 Jun 30, 2026
60 checks passed

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Post-merge Review

整体质量优秀。核心设计——用可变 own property + Config 原型方法委托替代固定 getter——正确解决了子代理退出 plan mode 的 bug。strip/restore 生命周期处理严谨,测试覆盖全面(12 个新用例覆盖 plan 退出、状态隔离、AUTO strip/restore 6 个场景、异常恢复)。以下为几个低优先级改进建议。

override.planGateState = basePlanGateState
? {
...basePlanGateState,
lastFindings: [...basePlanGateState.lastFindings],

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: 浅拷贝对 MergedGateFinding 对象存在引用共享

[...basePlanGateState.lastFindings] 仅浅拷贝数组,但数组中的 MergedGateFinding 对象仍然是共享引用。当前 MergedGateFinding 字段全是 primitive 所以安全,但如果将来增加嵌套对象字段,父子两侧会互相污染且不会有编译错误提示。

考虑改为 deep copy:

lastFindings: basePlanGateState.lastFindings.map((f) => ({ ...f })),

'permissionManager',
);
const ownPermissionManager = override.permissionManager;
override.permissionManager = null;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: permissionManager = null 依赖 Config 内部 truthiness check

此处设 null 是为了让 Config.prototype.setApprovalMode 中的 if (this.permissionManager) 跳过 strip/restore。这依赖于 Config 使用 truthiness 检查而非 !== undefined。如果将来 Config 内部改为显式 null 检查,此处会静默失效。当前由 'restores the inherited permission manager when AUTO-parent mode changes throw' 测试守护,风险可控。

await rebuildToolRegistryOnOverride(override as Config, base);

let cleanup: () => void = () => {};
const cleanup = () => {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: cleanup 可被多次调用——考虑加 guard

从调用方来看(agent.ts 中 finally 块 + 异常路径),cleanup() 可能被调用两次。虽然 restoreDangerousRules 的 sentinel 模式保证了幂等性,但加一个 cleaned 布尔标志可以让意图更明确:

let cleaned = false;
const cleanup = () => {
  if (cleaned) return;
  cleaned = true;
  // ...
};

expect(child.getApprovalMode()).toBe(ApprovalMode.DEFAULT);
const isPlanMode = child.getApprovalMode() === ApprovalMode.PLAN;

expect(isPlanModeBlocked(isPlanMode, false, false, execDetails)).toBe(

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: 断言冗余

前面已经断言了 child.getApprovalMode() === DEFAULT,然后手动计算 isPlanMode = (DEFAULT === PLAN)false,再传入 isPlanModeBlocked(false, ...) 断言结果为 false。这实际上只验证了 isPlanModeBlocked(false, ...) === false 这一纯函数行为,对子代理退出 plan mode 的路径验证在第一个 expect 已完成。考虑直接验证调度器层面的行为而非手动构造 boolean 传入纯函数。

AmariahAK pushed a commit to AmariahAK/qwen-code that referenced this pull request Jun 30, 2026
Add explicit plan-tool policy for ordinary subagents: they must not
receive or execute enter_plan_mode or exit_plan_mode. Plan mode is a
conversation-level approval contract — subagents are delegated workers
that report back, not lifecycle owners.

Changes:
- Add ENTER_PLAN_MODE and EXIT_PLAN_MODE to EXCLUDED_TOOLS_FOR_SUBAGENTS
  and EXCLUDED_TOOLS_FOR_TEAMMATES in agent-core.ts
- Fix inline FunctionDeclaration filter to use full exclusion set
  (not just recursionGuardOnly)
- Add ENTER_PLAN_MODE to WORKFLOW_SUBAGENT_DISALLOWED_TOOLS
- Add agent-context runtime guards in enterPlanMode.ts and
  exitPlanMode.ts that reject calls from subagent context
- Update workflow-orchestrator.test.ts to expect enter_plan_mode
  in disallowed tools list

Preserves state-isolation fix from QwenLM#6026. Main-thread plan mode
behavior unchanged.

Closes QwenLM#6083
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Subagents can remain stuck in plan mode after exit_plan_mode

6 participants