fix(core): always declare exit_plan_mode so plan mode can call it (#5210) - #5251
Conversation
|
Thanks for the PR @yiliang114! Template looks good ✓ — all required sections present, bilingual, test plan included. On direction: this fixes a real and well-documented bug. On approach: this is as minimal as it gets — one boolean flip ( Moving on to code review. 🔍 中文说明感谢贡献 @yiliang114! 模板完整 ✓ — 所有必需章节齐全,中英双语,包含测试计划。 方向:修复了一个真实且有充分记录的 bug。 方案:改动极小——一个布尔值翻转( 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
There was a problem hiding this comment.
Pull request overview
This PR fixes plan-mode behavior by ensuring the exit_plan_mode tool is always included in the function-declaration list sent to the model, even though it remains categorized as a deferred tool. This aligns the tool availability with the plan-mode prompt instruction that explicitly tells the model to call exit_plan_mode.
Changes:
- Set
ExitPlanModeTooltoalwaysLoad: truewhile keepingshouldDefer: true, ensuring the schema is declared by default. - Add regression coverage to verify
exit_plan_moderemains ingetFunctionDeclarations()and is excluded fromgetDeferredToolSummary(). - Add a focused unit assertion that
ExitPlanModeToolis both deferred-category and always-loaded.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/core/src/tools/exitPlanMode.ts | Makes exit_plan_mode always declared via alwaysLoad: true to unblock plan-mode completion. |
| packages/core/src/tools/exitPlanMode.test.ts | Adds a regression assertion for the tool’s deferred-category + always-declared flags. |
| packages/core/src/tools/tool-registry.test.ts | Adds a registry-level regression test ensuring exit_plan_mode is declared and not listed as deferred-summary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
647979c to
0a1040f
Compare
…enLM#5210) exit_plan_mode was marked shouldDefer, so it was excluded from the function-declaration list sent to the model. In plan mode the prompt tells the model to call exit_plan_mode directly, but its schema was withheld until the model first discovered it via ToolSearch. Weaker models (e.g. qwen3.7-max) failed to make that extra ToolSearch hop and stalled indefinitely 'stuck on ExitPlanMode'. Set alwaysLoad=true so the tool is always declared. Cost: its full description (~1.4KB) now rides in the declaration; the deferred-tool summary previously carried only a truncated ~160-char line of it (and now drops it), so the net add is a few hundred tokens per request — cheap for the one deferred tool the prompt tells the model to call directly. Add regression tests asserting the real tool stays in getFunctionDeclarations() and drops out of getDeferredToolSummary().
0a1040f to
4c6b8d3
Compare
|
@qwen-code /triage |
Code reviewThe diff is clean and focused — exactly what the PR description promises:
I verified the mechanism in Unit testsAll 64 tests pass (30 exitPlanMode + 34 tool-registry), including the 2 new regression tests: Real-scenario testing (tmux)Before (installed qwen 0.18.2, plan mode, Verify: runner@runnervm1li68:~/work/qwen-code/qwen-code/.qwen/worktrees/triage$ npm run dev -- --approval-mode plan -p 'Add a divide(a,b) function to calc.js' 2>&1 | tee /tmp/triage-5251-after/after2.log
DEV is set to true, but the React DevTools server is not running. Warning: Tool "exit_plan_mode" requires user approval but cannot execute in non-interactive mode. I'm blocked — plan mode requires confirmation to proceed, but non-interactive mode doesn't support the confirmation flow. Here's the plan I'd execute: Summary: No
|
|
Stepping back: this is a textbook one-line fix for a well-understood bug. The diagnosis is sharp — The diff carries nothing beyond the fix and its regression tests. No drive-by refactors, no scope creep. The token cost (~1.4KB per request) is the right tradeoff — cheap insurance against the "stuck for hours" symptom weaker models hit. My independent proposal would have been the same: flip The tmux test confirms no regression. We can't reproduce the exact failure mode (weaker model looping on missing tool) without Clean, minimal, correct. LGTM. ✅ 中文说明退一步看:这是一个教科书级的单行修复,针对一个理解清楚的 bug。诊断精准—— Diff 除了修复本身和回归测试外没有多余内容。没有顺手重构,没有范围蔓延。token 成本(每次请求约 1.4KB)是正确的权衡——用很小的代价防止弱模型遇到的"卡住好几个小时"问题。 如果我独立提出方案,也会是同样的做法:翻转 tmux 测试确认无回归。我们无法在没有 干净、精简、正确。LGTM。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — qwen3.7-max via Qwen Code /review
What this PR does
Makes
exit_plan_modealways part of the tool schema list sent to the model, instead of a deferred tool the model has to load on demand. The one-line change isalwaysLoad: false → trueon theExitPlanModeTool; the rest is regression tests.Why it's needed
In plan mode the system prompt tells the model to "present your plan by calling the
exit_plan_modetool". Butexit_plan_modewas markedshouldDefer, so its schema was excluded from the function-declaration list — a separate prompt section says deferred tools must first be loaded viaToolSearch. So the model is told to call a tool that isn't actually in its tool list, and has to bridge two non-cross-referencing instructions on its own. Stronger models make theToolSearch select:exit_plan_modehop and recover; weaker ones (e.g.qwen3.7-max, the model in #5210) don't, and loop trying to call a tool they can't see — the "stuck on ExitPlanMode for hours" symptom.exit_plan_modeis the only deferred tool the prompt tells the model to call directly, so it's precisely the one that shouldn't be deferred. The deferral was introduced in #3589, which lines up with the reporter's "it used to work, recently it gets stuck".Reviewer Test Plan
How to verify
npx vitest run packages/core/src/tools/exitPlanMode.test.ts packages/core/src/tools/tool-registry.test.ts. The new tests assert the realexit_plan_modestays ingetFunctionDeclarations()and drops out ofgetDeferredToolSummary().qwen --approval-mode plan -m qwen3.7-maxand give a small coding task (e.g. "add adivide()to calc.js"). Expected: the model callsexit_plan_modedirectly. Before the fix it first emits aToolSearch select:exit_plan_modecall, and weaker models may stall there.Evidence (Before & After)
Manual tmux run, plan mode,
qwen3.7-max, same task. Before is the released 0.18.2; after is this branch built locally. Watched the tool calls the model makes between reading the file and presenting the plan.Ran 3 cases on the fixed build (divide / power / "add input validation + plan tests"). The
ToolSearch select:exit_plan_modehop appeared 0 times across all three (it appeared on the released build). All three reached the plan-approval dialog directly; the validation case correctly usedAskUserQuestionfirst to resolve an ambiguity, thenexit_plan_mode.Tested on
Environment (optional)
Built locally with
npm run build, run frompackages/cli/dist. Unit tests via vitest.Risk & Scope
exit_plan_mode's full schema (including its ~1.4KB description) is now always in the tool list. Previously the deferred-tool summary carried only a truncated ~160-char first line of that description (truncateDeferredToolDescription), which it now drops out of. So the net cost is on the order of a few hundred tokens per request — cheap, and worth it for the one deferred tool the prompt tells the model to call directly.ToolSearchindirection for other deferred tools —exit_plan_modeis the only deferred tool the prompt tells the model to call directly, so the fix is scoped to it.shouldDeferstaystrueso the tool is still categorised with the deferred set andToolSearch select:exit_plan_modekeeps working (it just resolves an already-declared tool).Linked Issues
Closes #5210
中文说明
这个 PR 做了什么
让
exit_plan_mode始终出现在发给模型的工具声明列表里,而不是作为需要按需加载的 deferred 工具。核心改动只有一行:ExitPlanModeTool的alwaysLoad从false改成true,其余是回归测试。为什么需要
在规划模式下,系统提示词让模型"调用
exit_plan_mode工具来提交计划"。但exit_plan_mode被标记为shouldDefer,导致它的 schema 被排除在 function-declaration 列表之外——另一段提示词又要求 deferred 工具必须先用ToolSearch加载。于是模型被要求调用一个根本不在它工具列表里的工具,还得自己把两段互不引用的指令对接起来。强模型会先走ToolSearch select:exit_plan_mode这一跳然后恢复;弱模型(比如 #5210 里的qwen3.7-max)做不到,就会反复尝试调用一个它看不到的工具——这就是"卡在 ExitPlanMode 好几个小时"的症状。exit_plan_mode是唯一一个被提示词要求"直接调用"的 deferred 工具,所以它恰恰是不该被 defer 的那个。这个 defer 行为是 #3589 引入的,和报告者说的"以前能用、最近卡住"对得上。复审验证
npx vitest run packages/core/src/tools/exitPlanMode.test.ts packages/core/src/tools/tool-registry.test.ts。新增断言验证真实的exit_plan_mode留在getFunctionDeclarations()里、并从getDeferredToolSummary()中移除。qwen --approval-mode plan -m qwen3.7-max,给一个小的写代码任务(比如"给 calc.js 加一个divide()")。预期模型直接调用exit_plan_mode;修复前它会先发一个ToolSearch select:exit_plan_mode,弱模型可能就卡在那里。证据(前 / 后)
手动 tmux 实跑,规划模式,
qwen3.7-max,同一任务。Before 是 released 0.18.2,After 是本分支本地 build:在修复后的 build 上跑了 3 个 case(divide / power / "加参数校验并规划测试"),
ToolSearch select:exit_plan_mode这一跳出现 0 次(released build 上会出现)。3 个都直接走到计划确认框;校验那个 case 还正确地先用AskUserQuestion澄清了一个歧义再调用exit_plan_mode。风险与范围
exit_plan_mode完整的 schema(含约 1.4KB 描述)现在常驻工具列表。此前 deferred-tool 摘要里只放了该描述被截断到约 160 字符的首行(truncateDeferredToolDescription),现在它从摘要里移除了。所以净增成本约为每次请求几百 token——很便宜,对这个唯一被提示词要求直接调用的 deferred 工具来说值得。ToolSearch间接层——exit_plan_mode是唯一被要求直接调用的 deferred 工具,所以修复仅限于它。shouldDefer仍为true,工具仍归类在 deferred 集合里,ToolSearch select:exit_plan_mode也照常工作(只是解析到一个已声明的工具)。