Skip to content

fix(core): tell the model when the user manually exits plan mode#7682

Merged
wenshao merged 2 commits into
QwenLM:mainfrom
zjunothing:fix/7671-manual-plan-exit-notice
Jul 25, 2026
Merged

fix(core): tell the model when the user manually exits plan mode#7682
wenshao merged 2 commits into
QwenLM:mainfrom
zjunothing:fix/7671-manual-plan-exit-notice

Conversation

@zjunothing

Copy link
Copy Markdown
Collaborator

What this PR does

Tells the model — exactly once — when the user manually exits plan mode. Config.setApprovalMode() now queues a one-shot notice on every PLAN → non-PLAN transition except the approved exit_plan_mode flow (the tool passes a new fromApprovedPlanExit option at its two exit sites); re-entering plan mode clears a stale notice so it can never lie. GeminiClient's per-turn system-reminder assembly consumes the flag in the else branch of the existing plan-reminder injection and adds an explicit reminder: "The user has manually switched out of plan mode (current approval mode: …). Do NOT call exit_plan_mode … previous plan-mode restrictions no longer apply." The change stays entirely in core, exactly where the plan-mode reminder itself lives.

Why it's needed

#7671 (problem 1): the plan-mode reminder is re-injected on every model-bound turn while plan mode is active, so after a manual exit (Shift+Tab, /approval-mode, /plan, ACP mode switch) the reminder just silently stops appearing — a non-signal models don't reliably notice, as doudouOUC's triage note spelled out. The model's most recent context still says plan mode is active, so it keeps calling exit_plan_mode and gets stuck. This PR implements the one-shot-flag design from that triage note. It is complementary to #7673, which handles the other half (the unhelpful deny error when exit_plan_mode is called anyway) in different files — the two merge independently.

Reviewer Test Plan

How to verify

  1. Start qwen --approval-mode plan, chat once, press Shift+Tab (or /approval-mode default), chat again. Before: the next request to the provider carries no signal about the mode change (and the model typically tries exit_plan_mode). After: that request carries the one-shot exit reminder naming the new mode, and the following request does not repeat it.
  2. Normal plan flow is unchanged: an exit_plan_mode approved via the confirmation dialog does not inject the manual-exit reminder.
  3. npx vitest run src/config/config.test.ts src/core/client.test.ts src/tools/exitPlanMode.test.ts in packages/core (744/744).

Evidence (Before & After)

E2E drives the real compiled CLI in a scripted PTY against a fake OpenAI-compatible server and inspects the actual wire requests; the mode switch is a real Shift+Tab keypress in the TUI. Before/after via git stash + rebuild:

before/after E2E + tests

check (on captured wire requests) unpatched (origin/main) patched
turn in plan mode carries the plan-mode reminder ✅ (unchanged)
first turn after Shift+Tab carries the manual-exit notice naming the new mode ❌ (nothing — reminder just disappears)
notice is one-shot: not re-injected on the following turn ❌ (n/a)
no new plan-mode reminder after the exit ✅ (unchanged)
new unit tests (queue on manual exit / suppress on approved exit / clear on re-entry / client injection + negative pin) fail

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

macOS (Darwin 24.6), Node v22.23.1; vitest; scripted PTY + fake OpenAI SSE server for the E2E. npm run typecheck, eslint --max-warnings 0, prettier --check all clean.

Risk & Scope

  • Main risk or tradeoff: ProceedOnceAndSwitchToDefault (the "proceed once and switch to default" confirmation choice) also queues the notice when it fires inside plan mode — intentional, since that too is a user-driven exit the model wasn't told about. The notice consumes on the next UserQuery/Cron turn even if that request later fails; worst case the model misses one notice, which is today's behavior on every exit.
  • Not validated / out of scope: the context-aware deny message for calling exit_plan_mode outside plan mode is fix(core): exit_plan_mode returns guidance error from execute() instead of permission deny #7673; no CLI/UI-layer changes.
  • Breaking changes / migration notes: setApprovalMode's options gain an optional fromApprovedPlanExit field — additive, existing callers unaffected.

Linked Issues

Refs #7671 (problem 1; problem 2 is addressed by #7673)

中文说明

本 PR 做了什么

在用户手动退出 plan mode 时明确告知模型——且只告知一次。Config.setApprovalMode() 在每次 PLAN → 非 PLAN 转换时排队一个一次性通知,唯批准的 exit_plan_mode 流程除外(该工具在两处退出点传入新的 fromApprovedPlanExit 选项);重新进入 plan mode 会清除过期通知,保证它不会说谎。GeminiClient 逐轮的 system-reminder 装配在既有 plan reminder 注入的 else 分支消费该标志,注入明确提醒:"用户已手动退出 plan mode(当前审批模式:…)。不要调用 exit_plan_mode……此前的 plan mode 限制不再适用。"改动完全在 core 内——正是 plan-mode reminder 本身所在之处。

为什么需要

#7671(问题 1):plan mode 激活期间,plan reminder 在每一轮模型请求中重复注入;手动退出(Shift+Tab、/approval-mode/plan、ACP 模式切换)后 reminder 只是静默消失——如 doudouOUC 的 triage 笔记所说,这种"无信号"模型不可靠感知。模型最近的上下文仍写着 plan mode 激活,于是反复调用 exit_plan_mode 卡死。本 PR 实现了该笔记中的一次性标志设计。与 #7673 互补(后者处理另一半:调用被拒时的无用错误信息),两者改动文件不同、可独立合并。

审阅测试计划

如何验证

  1. qwen --approval-mode plan 启动,对话一轮,按 Shift+Tab(或 /approval-mode default),再对话:之前下一次请求对模式变更毫无信号(模型通常会尝试 exit_plan_mode);之后该请求携带注明新模式的一次性退出提醒,再下一轮不重复。
  2. 正常 plan 流程不变:经确认框批准的 exit_plan_mode 不注入手动退出提醒。
  3. packages/core 下三个测试文件 744/744。

证据(Before & After)

E2E 用脚本化 PTY 驱动真实编译 CLI 对接 fake OpenAI 服务器、检查真实 wire 请求;模式切换是真实的 TUI Shift+Tab 按键。git stash + 重建对照:未修复时退出后请求无任何信号;修复后一次性注入、后续轮不重复、批准退出不触发。新单测在未修复源码上失败。

测试平台

macOS 已本地验证(✅);Windows / Linux 依赖 CI(⚠️)。

环境

macOS(Darwin 24.6)、Node v22.23.1;vitest;E2E 用脚本化 PTY + fake OpenAI SSE server;typecheck / eslint / prettier 全绿。

风险与范围

  • 主要风险/权衡:plan mode 内触发的 ProceedOnceAndSwitchToDefault("仅此一次并切换到默认模式")同样排队通知——这是有意的,它也是模型未被告知的用户驱动退出。通知在下一次 UserQuery/Cron 轮消费,即使该请求随后失败也不重放;最坏情况模型错过一次通知,即今天每次退出的现状。
  • 未验证/超出范围:plan mode 外调用 exit_plan_mode 的上下文化拒绝信息由 fix(core): exit_plan_mode returns guidance error from execute() instead of permission deny #7673 处理;无 CLI/UI 层改动。
  • 破坏性变更/迁移说明:setApprovalMode 的 options 新增可选 fromApprovedPlanExit 字段——纯新增,既有调用方不受影响。

关联 Issue

Refs #7671(问题 1;问题 2 由 #7673 处理)

🤖 Generated with Claude Code

While plan mode is active the plan-mode system reminder is re-injected
on every model-bound turn, so after a manual exit (Shift+Tab,
/approval-mode, /plan, ACP mode switch) the reminder just silently
stops appearing — a non-signal models do not reliably notice. The
model's most recent context still says plan mode is active, so it keeps
calling exit_plan_mode and gets stuck (QwenLM#7671, problem 1).

Config.setApprovalMode now queues a one-shot notice on every
PLAN -> non-PLAN transition except the approved exit_plan_mode flow
(which passes the new fromApprovedPlanExit option); re-entering plan
mode clears a stale notice. GeminiClient's system-reminder assembly
consumes the flag on the next turn and injects an explicit "the user
has manually switched out of plan mode" reminder naming the new mode.

Fixes the first half of QwenLM#7671; the deny-message half is QwenLM#7673.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments above for the result.

Qwen Triage 已完成 —— 查看运行。结果见上方各阶段评论。

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — all sections present, with a real reviewer test plan and a bilingual body.

Problem: observed, not theoretical. #7671 documents a concrete failure: while plan mode is active the plan-mode reminder is re-injected on every model-bound turn, so after a manual exit (Shift+Tab, /approval-mode, /plan, ACP mode switch) the reminder just silently stops appearing — a non-signal the model doesn't reliably notice, leaving it stuck calling exit_plan_mode. The linked issue is a triaged type/bug (P2, welcome-pr) with a clear root-cause analysis, and the PR includes a before/after E2E against captured wire requests.

Direction: aligned. This is exactly problem 1 from the issue (inject an explicit one-shot notice on manual exit), and it's deliberately scoped to complement #7673 (problem 2, the unhelpful deny message) in disjoint files so the two merge independently. CHANGELOG has no direct reference, but the area is clearly relevant — plan-mode UX is active surface.

Size: touches core paths (config, core/client, core/prompts, tools/exitPlanMode). Production logic is ~70 lines (config 36, client 10, prompts 16, exitPlanMode 8) against ~169 lines of tests — well under any threshold, and as a fix it isn't size-gated anyway.

Approach: the scope feels right. Routing the signal through setApprovalMode() is the clean move — it's the single chokepoint every manual exit funnels through, so one boolean flag plus a fromApprovedPlanExit opt-out for the model-initiated exit_plan_mode flow covers Shift+Tab, the slash commands, the settings dialog, the "proceed once and switch" confirmation, and ACP setSessionMode uniformly. I don't see a simpler path, and nothing in the diff looks unrelated.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓——各节齐全,有真实的审阅测试计划,正文中英双语。

问题: 已观测到的真实 bug,非理论性加固。#7671 记录了一个具体故障:plan mode 激活期间 plan reminder 在每一轮模型请求中重复注入,因此手动退出(Shift+Tab、/approval-mode/plan、ACP 模式切换)后 reminder 只是静默消失——模型不可靠感知这种"无信号",于是卡死在反复调用 exit_plan_mode。关联 issue 是经分诊的 type/bug(P2、welcome-pr),有清晰的根因分析;PR 附带对捕获 wire 请求的 before/after E2E。

方向: 对齐。这正是 issue 中的问题 1(手动退出时注入一次性明确通知),并有意识地与 #7673(问题 2:无用的拒绝信息)互补,两者改动文件不相交、可独立合并。CHANGELOG 无直接条目,但该领域显然相关——plan mode UX 是活跃面。

规模: 触及核心路径(configcore/clientcore/promptstools/exitPlanMode)。生产逻辑约 70 行(config 36、client 10、prompts 16、exitPlanMode 8),测试约 169 行——远低于任何阈值;且作为 fix 本就不按规模设卡。

方案: 范围合理。把信号经由 setApprovalMode() 是干净的做法——它是所有手动退出共同经过的唯一咽喉点,于是一个布尔标志加上对模型发起的 exit_plan_mode 流程的 fromApprovedPlanExit 豁免,就能统一覆盖 Shift+Tab、各 slash 命令、设置对话框、"仅此一次并切换"确认以及 ACP setSessionMode。没看到更简路径,diff 中也没有无关改动。

进入代码审查 🔍

Qwen Code · qwen3.8-max-preview

Reviewed at 18bb51ba2746e6130ecbd88ea6830abd7715e6ab · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Code review

Before reading the diff, my own take on the fix was: track a one-shot "you just left plan mode" flag at the single point every mode change passes through (Config.setApprovalMode), set it on a PLAN → non-PLAN transition, clear it on re-entry so it can never lie, suppress it for the model's own approved exit_plan_mode, and consume it once in the client's per-turn reminder assembly — in the else branch of the existing plan-reminder injection so the two are mutually exclusive. The PR does essentially this, and does it cleanly.

A few things I checked specifically, since this is core:

  • Chokepoint coverage. I grepped every setApprovalMode caller. The only path that should not queue the notice is the model-initiated approved exit, and both of its call sites (exitPlanMode.ts regular approval + leader-approved approval) now pass fromApprovedPlanExit: true. Every other PLAN → non-PLAN transition — Shift+Tab cycling, /approval-mode, /plan, the settings dialog, ACP setSessionMode, and the ProceedOnceAndSwitchToDefault confirmation in coreToolScheduler — is a user/host-driven exit the model was never told about, and correctly queues the notice. The "proceed once and switch" case is called out in the PR body as intentional, and I agree.
  • Placement & one-shot semantics. The flag is set inside the PLAN → non-PLAN bookkeeping block (after the fallible trust-gate work, so a rejected mode change never sets it), cleared on PLAN re-entry, and consumePendingManualPlanExitNotice() reads-and-clears. The client consumes it in the else if of getApprovalMode() === PLAN, so you can never get both reminders on one turn, and the notice fires on the next model-bound turn regardless of message type — matching how the plan reminder itself is injected.
  • No downstream breakage. The new fromApprovedPlanExit option is additive and optional; existing callers are unaffected. getManualPlanExitSystemReminder reuses the already-imported ToolNames.EXIT_PLAN_MODE, consistent with the surrounding prompt code.

No correctness, security, or convention issues found. Test coverage is solid — queue-on-manual-exit, suppress-on-approved-exit, clear-on-re-entry, no-queue-for-non-plan, client inject/no-inject, reminder wording, and both approved-exit call sites.

One non-blocking nit: the PR body says the notice "consumes on the next UserQuery/Cron turn," but the code actually consumes on the next turn of any type (the else if isn't message-type-gated, mirroring the plan reminder). The code behavior is fine — arguably better — so this is just a wording mismatch in the description, not a defect.

CI test evidence

Unattended run — I did not build or execute PR code; the signal below is the PR's own CI on the reviewed commit, read via the API. Ubuntu unit tests, the precheck (lint/typecheck/format), and the web-shell E2E smoke are green; macOS/Windows/Integration are skipped by matrix gating, not failed. The single cancelled route check is bot orchestration (pull_request_target), unrelated to the PR. No red check is attributable to this change.

Check Conclusion
Test (ubuntu-latest, Node 22.x) ✅ success
precheck-pr / precheck ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success
Test (macos-latest, Node 22.x) ⏭️ skipped
Test (windows-latest, Node 22.x) ⏭️ skipped
Integration Tests (CLI, No Sandbox) ⏭️ skipped

Real-scenario testing: N/A for this unattended run. The behavioral surface here is what gets sent to the model on a manual mode switch, which the unit tests pin directly (client inject/no-inject on captured request parts). The author also reports a scripted-PTY E2E against a fake OpenAI server with a real Shift+Tab keypress (screenshot in the PR body) — that is the author's claim, not independently re-run here. If a maintainer wants live TUI confirmation on-repo, the isolated @qwen-code /tmux job can be triggered.

中文说明

代码审查

读 diff 之前,我自己的方案是:在所有模式变更都经过的唯一点(Config.setApprovalMode)跟踪一个一次性"刚离开 plan mode"标志,在 PLAN → 非 PLAN 转换时置位、重新进入时清除以保证它不会说谎、对模型自身批准的 exit_plan_mode 豁免,并在客户端逐轮 reminder 装配中消费一次——放在既有 plan reminder 注入的 else 分支,使两者互斥。PR 基本就是这么做的,而且很干净。

由于触及核心,我重点核对了:

  • 咽喉点覆盖。 我 grep 了所有 setApprovalMode 调用方。唯一不应排队通知的路径是模型发起的批准退出,而它的两处调用点(exitPlanMode.ts 常规批准 + leader 批准)现在都传入 fromApprovedPlanExit: true。其余所有 PLAN → 非 PLAN 转换——Shift+Tab 循环、/approval-mode/plan、设置对话框、ACP setSessionMode、以及 coreToolScheduler 中的 ProceedOnceAndSwitchToDefault 确认——都是模型未被告知的用户/宿主驱动退出,正确排队通知。"仅此一次并切换"的情形 PR 正文已说明是有意的,我认同。
  • 位置与一次性语义。 标志在 PLAN → 非 PLAN 的簿记块内置位(在可能失败的 trust-gate 之后,因此被拒绝的模式变更绝不会置位),PLAN 重入时清除,consumePendingManualPlanExitNotice() 读后即清。客户端在 getApprovalMode() === PLANelse if 中消费它,所以同一轮绝不会同时出现两个 reminder,且通知在下一个模型轮触发(不论消息类型)——与 plan reminder 本身的注入方式一致。
  • 无下游破坏。 新增的 fromApprovedPlanExit 选项是纯新增、可选;既有调用方不受影响。getManualPlanExitSystemReminder 复用了已导入的 ToolNames.EXIT_PLAN_MODE,与周边 prompt 代码一致。

未发现正确性、安全性或规范问题。测试覆盖扎实——手动退出排队、批准退出豁免、重入清除、非 plan 变更不排队、客户端注入/不注入、reminder 措辞,以及两处批准退出调用点。

一个非阻塞的小问题:PR 正文说通知"在下一个 UserQuery/Cron 轮消费",但代码实际上在任意类型的下一轮消费(else if 未按消息类型设门,与 plan reminder 一致)。代码行为没问题——甚至更好——所以这只是描述措辞不一致,并非缺陷。

CI 测试证据

无人值守运行——我未构建或执行 PR 代码;下方信号是 PR 自身在被审 commit 上的 CI,经 API 读取。Ubuntu 单测、precheck(lint/typecheck/format)、web-shell E2E smoke 均绿;macOS/Windows/Integration 因矩阵门控为 skipped,并非失败。唯一 cancelledroute 检查是 bot 编排(pull_request_target),与 PR 无关。没有可归因于本改动的红检查。

(CI 表格见上方英文区。)

真实场景测试: 本次无人值守运行为 N/A。这里的行为面是手动模式切换时发给模型的内容,单测已直接钉住(对捕获的请求片段做客户端注入/不注入断言)。作者还报告了对 fake OpenAI 服务器的脚本化 PTY E2E、含真实 Shift+Tab 按键(截图见 PR 正文)——那是作者的陈述,未在此独立复跑。若维护者想在仓库内做实时 TUI 确认,可触发隔离的 @qwen-code /tmux 任务。

Qwen Code · qwen3.8-max-preview

Reviewed at 18bb51ba2746e6130ecbd88ea6830abd7715e6ab · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean, minimal fix for a real observed bug; the only thing keeping it from a 5 is that I couldn't drive the live TUI flow myself in this unattended run (verification is CI unit tests + the author's PTY E2E).

Stepping back: this is a textbook small fix. The problem is real and well-evidenced (#7671 — the plan-mode reminder silently disappearing on manual exit, leaving the model stuck calling exit_plan_mode), and the solution is the minimum that solves it — one boolean flag at the mode-change chokepoint, consumed once, with a single opt-out for the model's own approved exit. My independent proposal and the PR converged on the same design, which is a good sign the approach isn't overthought.

The things that usually go wrong with a change like this are all handled: the flag can't lie after a re-entry (cleared on PLAN entry), it can't double-fire (read-and-clear), it can't fire on the approved path (both call sites opt out), and it can't be set by a rejected mode change (set after the fallible trust-gate work). I walked every setApprovalMode caller to confirm the manual-vs-approved split is complete, and it is. The diff carries nothing unrelated, the option is additive, and the test coverage pins the actual behavior rather than the implementation.

If I had to maintain this in six months I'd be fine with it — it sits right next to the plan-mode reminder it mirrors and the comments explain the why (#7671) without narrating the code.

Not blocking, just noted: the description's "next UserQuery/Cron turn" is slightly looser than the code (fires on any turn), and live TUI confirmation here rests on CI + the author's E2E rather than my own tmux drive — a maintainer can trigger the isolated @qwen-code /tmux job if they want that on-repo. Neither changes the verdict.

LGTM, approving. ✅

中文说明

置信度:4/5——对一个真实观测到的 bug 的干净、最小修复;唯一让它不到 5 分的是,本次无人值守运行中我无法亲自驱动实时 TUI 流程(验证为 CI 单测 + 作者的 PTY E2E)。

退一步看:这是一个教科书式的小修复。问题真实且有证据(#7671——手动退出时 plan-mode reminder 静默消失,使模型卡死在调用 exit_plan_mode),方案是解决问题所需的最小量——模式变更咽喉点上的一个布尔标志、消费一次、对模型自身批准的退出留一个豁免。我的独立方案与 PR 收敛到同一设计,这是方案未被过度设计的良信号。

这类改动常出错的地方都被处理了:重入后标志不会说谎(PLAN 进入时清除)、不会重复触发(读后即清)、不会在批准路径触发(两处调用点都豁免)、不会被拒绝的模式变更置位(在可能失败的 trust-gate 之后置位)。我遍历了所有 setApprovalMode 调用方以确认手动/批准的划分是完整的,确实完整。diff 不夹带无关改动,选项是纯新增,测试覆盖钉住的是实际行为而非实现。

若六个月后由我维护,我乐于接受——它就放在其所镜像的 plan-mode reminder 旁边,注释解释了"为什么"(#7671)而非复述代码。

非阻塞、仅记录:描述中的"下一个 UserQuery/Cron 轮"比代码略宽松(代码在任意轮触发);此处的实时 TUI 确认依赖 CI + 作者 E2E,而非我亲自的 tmux 驱动——若维护者想在仓库内确认,可触发隔离的 @qwen-code /tmux 任务。两者都不改变结论。

LGTM,批准。✅

Qwen Code · qwen3.8-max-preview

Reviewed at 18bb51ba2746e6130ecbd88ea6830abd7715e6ab · re-run with @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. ✅

@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 issues found. LGTM! ✅

— qwen3.7-max via Qwen Code /review

@doudouOUC doudouOUC 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.

Reviewed — no blockers. Suggestions are inline.

— qwen3.7-max via Qwen Code /review

Comment on lines +336 to +338
this.config.setApprovalMode(decision.targetMode, {
fromApprovedPlanExit: true,
});

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.

[Suggestion] The leader-approval setApprovalMode call site has no test verifying { fromApprovedPlanExit: true } is passed — only the regular approval call site (line 237) has this assertion. — Failure scenario: if a future change drops or renames the option on the leader path but not the regular path, no test fails. After a leader-approved plan exit, the model would receive a spurious "The user has manually switched out of plan mode" reminder, contradicting the approved-exit semantics.

Add a happy-path test for leader approval that asserts config.setApprovalMode was called with { fromApprovedPlanExit: true }, mirroring the assertion at line 118.

— qwen3.7-max via Qwen Code /review

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.

Added in 18bb51b — new test marks a successful leader-approved exit as an approved plan exit: a leader approval resolving { action: 'approve', targetMode: DEFAULT } asserts setApprovalMode(DEFAULT, { fromApprovedPlanExit: true }), so dropping the option on the leader path now fails a test instead of queuing a spurious manual-exit reminder. 中文:已补 leader 批准成功路径的断言(18bb51ba2),漏传选项会直接挂测试。

Comment on lines +1111 to +1114
export function getManualPlanExitSystemReminder(currentMode: string): string {
return `<system-reminder>
The user has manually switched out of plan mode (current approval mode: ${currentMode}). You are no longer in plan mode. Do NOT call ${ToolNames.EXIT_PLAN_MODE} — there is no plan approval pending. Continue working in the current mode; previous plan-mode restrictions on edits and state-modifying tools no longer apply.
</system-reminder>`;

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.

[Suggestion] No unit test in prompts.test.ts for getManualPlanExitSystemReminder, while its counterpart getPlanModeSystemReminder has 4 dedicated tests. — Concrete cost: the client tests only assert stringContaining('manually switched out of plan mode') — they would pass even if currentMode renders as undefined or EXIT_PLAN_MODE is replaced with a wrong tool name.

Add a describe('getManualPlanExitSystemReminder', ...) block that verifies: (1) output contains the supplied mode string, (2) output contains the exit_plan_mode tool name, (3) output matches the <system-reminder>...</system-reminder> wrapper.

— qwen3.7-max via Qwen Code /review

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.

Added in 18bb51b — a getManualPlanExitSystemReminder describe in prompts.test.ts: pins the <system-reminder> envelope, the "manually switched out of plan mode" phrasing, the rendered mode name (current approval mode: default / yolo / auto-edit), the Do NOT call exit_plan_mode prohibition, and the restrictions-lifted line — so an undefined mode or a renamed tool constant now fails here rather than slipping past the client tests' stringContaining check. 中文:已补 prompts 单测(18bb51ba2),钉住信封、模式名渲染与 exit_plan_mode 禁令,undefined 模式或工具名改动会在此失败。

… text

Adds the two review-suggested pins: a successful leader-approved exit
asserts setApprovalMode receives { fromApprovedPlanExit: true } (only the
regular approval site was pinned), and getManualPlanExitSystemReminder
gets dedicated prompts tests covering the rendered mode name, the
exit_plan_mode prohibition, and the reminder envelope.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@zjunothing

Copy link
Copy Markdown
Collaborator Author

Both suggestions addressed in 18bb51b (see inline replies): the leader-approval call site now has its own fromApprovedPlanExit assertion, and getManualPlanExitSystemReminder has dedicated prompts tests. exitPlanMode 29/29 + prompts 87/87, eslint/prettier clean. 中文:两条建议均已在 18bb51b 落实(见行内回复),测试全绿。

@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.

Reviewed — no blockers. Suggestions are inline.

中文说明

已审查——无阻断问题。 建议见行内评论。

— qwen3.7-max via Qwen Code /review

Comment on lines +2462 to +2464
systemReminders.push(
getManualPlanExitSystemReminder(this.config.getApprovalMode()),
);

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.

[Suggestion] The exit notice is only consumed on UserQuery/Cron turns (line 2425 gates the whole system-reminder block). If the user Shift+Tabs out of plan mode mid-tool-loop, the next model-bound turn is a ToolResult — the flag stays pending and no exit reminder is injected. — Concrete cost: the model's most recent system reminder still says "plan mode is active"; it may call exit_plan_mode and burn a round-trip before the next user message delivers the reminder. The staleness guard in exitPlanMode.ts prevents harm, but the one wasted turn is the exact scenario this feature targets.

中文说明

退出通知仅在 UserQuery/Cron 轮次消费(第 2425 行的门控覆盖了整个 system-reminder 块)。如果用户在工具循环中途按 Shift+Tab 退出 plan mode,下一个发给模型的轮次是 ToolResult——标志保持 pending 状态,不会注入退出提醒。

具体代价: 模型最近的 system reminder 仍显示"plan mode 激活中";它可能调用 exit_plan_mode,浪费一轮交互后才能在下一条用户消息中获得提醒。exitPlanMode.ts 中的过期检查防止了实际损害,但这一轮浪费正是此功能要解决的问题。

— qwen3.7-max via Qwen Code /review

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.

Real bound, deliberately kept: the notice rides the same UserQuery/Cron cadence as the plan-mode reminder it replaces — no reminder in this assembly is injected on ToolResult turns today, and giving this one reminder its own injection point outside that gate would be a new pattern for the block (with its own questions about interleaving system reminders into an active tool loop). The mid-loop window is bounded: the flag stays pending (it is only cleared by consumption or plan re-entry, never dropped), so the reminder lands on the very next user/cron turn, and a premature exit_plan_mode call in that window is caught by the staleness/permission guard — with #7673 it additionally returns explicit guidance instead of a generic deny. If maintainers want reminders on ToolResult turns, happy to do that as a follow-up gate change rather than special-casing this one reminder.

中文

真实边界,且是有意保留的:通知与它所替代的 plan-mode reminder 走同一个 UserQuery/Cron 注入节奏——该装配块今天没有任何 reminder 在 ToolResult 轮注入,为这一个 reminder 单开注入点会给这个块引入新模式(还牵出「向进行中的工具循环插入 system reminder」的问题)。循环中途的窗口是有界的:标志保持 pending(只被消费或重进 plan mode 清除,绝不丢弃),下一个 user/cron 轮必然送达;窗口内过早的 exit_plan_mode 调用被过期/权限守卫拦住——配合 #7673 还会返回明确指引而非泛化拒绝。如果维护者希望 ToolResult 轮也注入 reminder,可以作为 follow-up 改门控整体处理,而不是给单个 reminder 开特例。

@wenshao

wenshao commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

✅ Local verification — merge reference

I independently reproduced this PR locally as a maintainer. Verified at head 18bb51ba2 (merge-base 9e7acec863) with a real npm ci + build in an isolated worktree. Everything reproduces — this is merge-ready.

Environment: macOS (Darwin 24.6), Node v22.23.1, vitest v3.2.4.


1. Real-CLI end-to-end — the actual wire, before & after

This is the load-bearing check. I drove the real built CLI (node packages/cli/dist/index.js --approval-mode plan) inside a pseudo-terminal (@lydell/node-pty + @xterm/headless) against a fake OpenAI-compatible server that records every request body. The plan-mode exit is a genuine ESC [ Z (Shift+Tab) keypress — nothing in the UI path is mocked. Script: send a message in plan mode → Shift+Tab (plan→default) → send two more messages. I then rebuilt packages/core from the merge-base and ran the same script for a true before/after.

wire before/after

Turn (first main-agent request) origin/main (before) PR #7682 (after)
T1 — message while in plan mode plan reminder ✅ plan reminder ✅ (unchanged)
T2 — first message after Shift+Tab no signal at all — reminder just disappears exit notice ✅ naming current approval mode: default
T3 — the following message none none — one-shot, not repeated

The mode switch is real — the footer flips plan mode⏸ Ask permissions (= default) after the single keypress:

real CLI TUI


2. Unit tests & discriminating A/B

All four affected test files are green on the PR head:

npx vitest run src/config/config.test.ts src/core/client.test.ts \
               src/tools/exitPlanMode.test.ts src/core/prompts.test.ts
  ✓ prompts.test.ts (87)  ✓ exitPlanMode.test.ts (29)  ✓ config.test.ts (430)  ✓ client.test.ts (286)
  Test Files  4 passed (4)   Tests  832 passed (832)

To confirm the new tests actually pin the fix (and aren't vacuous), I overlaid the merge-base source while keeping the PR's test files. 11 tests fail on base, 0 on the PR head:

unit + A/B

File Fails on base Reason
config.test.ts 4 config.consumePendingManualPlanExitNotice is not a function
exitPlanMode.test.ts 4 setApprovalMode not called with { fromApprovedPlanExit: true }
prompts.test.ts 2 getManualPlanExitSystemReminder is not a function
client.test.ts 1 manual-exit reminder absent from the assembled request

3. Notes

  • The approved exit_plan_mode flow stays silent (no manual-exit notice) — both exit sites in exitPlanMode.ts pass fromApprovedPlanExit: true, and the A/B above pins all four such call sites (3 confirmation choices + leader-approval).
  • Shift+Tab is what I drove end-to-end; the other manual-exit triggers (/approval-mode, /plan, ACP setSessionMode, confirm-and-switch) all route through the same Config.setApprovalMode transition, which is unit-covered.
  • Change is entirely within packages/core, exactly where the plan-mode reminder lives; fromApprovedPlanExit is additive so existing callers are unaffected. Complementary to fix(core): exit_plan_mode returns guidance error from execute() instead of permission deny #7673 (different files). No concerns from my side. LGTM.
🇨🇳 中文说明

✅ 本地验证 —— 合并参考

作为维护者,我在本地独立复现了本 PR。验证基于 head 18bb51ba2(merge-base 9e7acec863),在隔离 worktree 中执行了真实的 npm ci + 构建。全部复现通过 —— 可以合并。

环境:macOS(Darwin 24.6)、Node v22.23.1、vitest v3.2.4。

1. 真实 CLI 端到端 —— 真实 wire 的 before/after(最关键的一项)

我用伪终端(@lydell/node-pty + @xterm/headless)驱动真实编译产物 CLInode packages/cli/dist/index.js --approval-mode plan),对接一个记录每个请求体的 fake OpenAI 兼容服务器。退出 plan mode 是真实的 ESC [ Z(Shift+Tab)按键——UI 路径无任何 mock。脚本:plan 模式下发一条消息 → Shift+Tab(plan→default)→ 再发两条。随后我从 merge-base 重新构建 packages/core,用同一脚本跑出真正的 before/after。

轮次(该轮首个 main-agent 请求) origin/main(修复前) PR #7682(修复后)
T1 —— plan 模式下发消息 plan reminder ✅ plan reminder ✅(不变)
T2 —— Shift+Tab 后首条消息 完全没有任何信号 —— reminder 直接消失 退出提醒 ✅,注明 current approval mode: default
T3 —— 下一条消息 无 —— 一次性,不重复

模式切换是真实的 —— 单次按键后 footer 从 plan mode 翻为 ⏸ Ask permissions(即 default 模式)。(见上方英文部分截图)

2. 单元测试与「判别性」A/B

PR head 上四个受影响测试文件全绿:prompts.test.ts(87)exitPlanMode.test.ts(29)config.test.ts(430)client.test.ts(286) —— 4 文件 / 832 用例全部通过

为确认新测试确实锁定了修复(而非空测试),我把 merge-base 的源码覆盖回去、保留 PR 的测试文件:base 上 11 个用例失败,PR head 上 0 失败

文件 base 失败数 原因
config.test.ts 4 config.consumePendingManualPlanExitNotice is not a function
exitPlanMode.test.ts 4 setApprovalMode 未带 { fromApprovedPlanExit: true } 调用
prompts.test.ts 2 getManualPlanExitSystemReminder is not a function
client.test.ts 1 组装出的请求中缺少手动退出提醒

3. 其他

  • 被批准的 exit_plan_mode 流程保持静默(不注入手动退出提醒)—— exitPlanMode.ts 两处退出点都传了 fromApprovedPlanExit: true,上面的 A/B 锁定了全部四个此类调用点(3 个确认选项 + leader 批准)。
  • 我端到端驱动的是 Shift+Tab;其余手动退出触发(/approval-mode/plan、ACP setSessionMode、confirm-and-switch)都走同一个 Config.setApprovalMode 转换,已被单测覆盖。
  • 改动完全在 packages/core 内,正是 plan-mode reminder 所在之处;fromApprovedPlanExit 为纯新增字段,既有调用方不受影响。与 fix(core): exit_plan_mode returns guidance error from execute() instead of permission deny #7673 互补(改动文件不同)。我这边无异议。LGTM。

Independent local verification by a maintainer. Assets hosted on branch pr-assets/pr-7682-verify.

@zjunothing

Copy link
Copy Markdown
Collaborator Author

Thanks for the deep verification — driving the real Shift+Tab through node-pty and A/B-ing the same harness against merge-base is exactly the evidence this needed, and the 11-failure discriminating check confirms all four approved-exit call sites are pinned. 中文:感谢深度验证——真实按键 + 同一 harness 的 A/B 对照正是这个修复最需要的证据。

@wenshao

wenshao commented Jul 25, 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 added this pull request to the merge queue Jul 25, 2026
Merged via the queue into QwenLM:main with commit a3d78c9 Jul 25, 2026
138 of 139 checks passed
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.

4 participants