feat(core): auto-reveal exit_plan_mode tool when entering plan mode - #5311
Conversation
|
@qwen-code /triage |
|
Thanks for the PR! Template looks good ✓ On direction: this is a genuine friction point — after entering plan mode, the model has to make an unnecessary ToolSearch round-trip just to find On approach: 32 lines, one file, zero scope creep. The implementation mirrors the existing ToolSearch reveal pattern exactly. I don't see a simpler path — this is already the minimal change. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 方向:这是一个真实的摩擦点——进入计划模式后,模型必须额外调用一次 ToolSearch 才能找到 方案:32 行,一个文件,零范围蔓延。实现完全复用了现有的 ToolSearch 揭示模式。没有更简路径了——这已经是最小改动。进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewThe implementation mirrors ToolSearch's reveal pattern faithfully — Two observations, neither blocking: Missing No unit tests: ToolSearch has 33 tests covering the reveal/rollback paths. The PR adds no tests for the new reveal logic in Build & Test ResultsReal-Scenario Test (tmux)No API keys available in this CI environment, so interactive plan mode (where the auto-reveal actually triggers) can't be exercised. The non-interactive guard works correctly — plan mode is properly blocked before the reveal code is reached, which is the expected behavior. The CLI starts and responds to prompts without issues. Verdict on testing: build/lint pass, related unit tests pass, CLI smoke test passes. The actual auto-reveal behavior requires an interactive session with API access to verify end-to-end. 中文说明代码审查实现忠实地复用了 ToolSearch 的揭示模式—— 两个观察,均不构成阻塞: 缺少 没有单元测试: ToolSearch 有 33 个测试覆盖揭示/回滚路径。PR 没有为 测试构建、Lint、相关单元测试全部通过。无 API 密钥,无法在 CI 中测试交互式计划模式的实际揭示行为。非交互模式正确阻止了计划模式进入。 — Qwen Code · qwen3.7-max |
|
Stepping back: this is a clean, focused PR that solves a real friction point. The model shouldn't need to search for a tool it's about to need — auto-revealing The implementation is exactly what I would have written (and then some — the idempotency guard is a nice touch). 32 lines in one file, mirrors the existing ToolSearch pattern, proper error handling with rollback, and non-fatal on failure so plan mode entry always succeeds. Build, lint, and all 33 related unit tests pass. I couldn't exercise the interactive plan mode path in CI (no API keys), but the code path is straightforward and well-guarded. The missing unit tests are a fair observation but not a blocker — the behavior is simple and the integration path is the meaningful verification. Worth following up, not worth blocking on. LGTM ✅ 中文说明总结:这是一个干净、聚焦的 PR,解决了真实的摩擦点。模型不应该需要搜索一个即将用到的工具——进入计划模式时自动揭示 实现与我自己的方案一致(甚至更好——幂等保护是加分项)。32 行代码,一个文件,复用现有 ToolSearch 模式,有完善的错误处理和回滚,揭示失败时计划模式进入仍然成功。 构建、Lint、33 个相关单元测试全部通过。CI 环境中无 API 密钥无法测试交互式计划模式路径,但代码路径简单且有充分的保护。 缺少单元测试是合理的观察但不构成阻塞——行为简单,集成测试是有意义的验证方式。建议后续补充。 可以合并 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
✅ Real-build verification — behavior correct; one ask (add a test)I built the actual What I checked1. CI / existing tests — green. All checks pass (Lint, CodeQL, Test ×3 OS); the existing 2. API & pattern correctness. The PR calls real registry API — 3. Behavioral harness — A/B over 4 scenarios (real
Scenario A is the headline: the PR is precisely what flips raw tmux capture (fixed vs pre-fix)Notes
Verdict: behavior is correct, matches the proven ToolSearch pattern, no regression — LGTM to merge. Recommend adding the unit test the PR currently omits. 👍 中文说明(完整对应)✅ 真实构建验证 —— 行为正确;一个请求(补测试)我在隔离 worktree 里从本 PR 的 head( 我检查了什么1. CI / 既有测试 —— 全绿。 所有检查通过(Lint、CodeQL、三平台 Test);既有的 2. API 与模式正确性。 PR 调用的是真实注册表 API —— 3. 行为 harness —— 4 个场景的 A/B(真实
场景 A 是核心:本 PR 正是进入 plan mode 时把 备注
结论:行为正确、与成熟的 ToolSearch 模式一致、无回归 —— LGTM 可合并。建议补上本 PR 目前缺失的单元测试。 👍 |
What this PR does
When the model calls
enter_plan_mode, this PR automatically reveals theexit_plan_modedeferred tool so the model can invoke it without needing to search for it first. The implementation mirrors the existing pattern inToolSearch's select path: reveal the deferred tool, sync the tool declaration set viasetTools(), and roll back the reveal if the sync fails to keep the registry consistent with the chat's declaration list.Why it's needed
Currently, after entering plan mode, the model must call
ToolSearchto discoverexit_plan_modebefore it can transition back to implementation. This adds an unnecessary round-trip. The reveal-on-enter pattern eliminates that friction while preserving correctness through the rollback-on-failure guard.Reviewer Test Plan
How to verify
/planor have the model callenter_plan_mode).exit_plan_modeappears in the available tool set without the model callingToolSearch.exit_plan_mode.setToolsfailure by injecting a network error after reveal, verify the reveal is rolled back and the model can still useToolSearchas fallback.Evidence (Before & After)
N/A (non-UI, tool registry internal change).
Tested on
Risk & Scope
setTools()is an async IPC round-trip; if it fails, the reveal is rolled back so the model can still discoverexit_plan_modeviaToolSearch. No hard dependency introduced.Linked Issues
N/A (self-discovered improvement, not from an upstream issue).
中文说明
本 PR 做了什么
当模型调用
enter_plan_mode进入计划模式时,自动揭示exit_plan_mode延迟工具,使模型无需先调用ToolSearch即可直接退出计划模式。实现方式与ToolSearch的选择路径一致:揭示延迟工具 → 通过setTools()同步工具声明 → 同步失败时回滚揭示,保持注册表与聊天声明列表一致。为什么需要
当前进入计划模式后,模型必须先调用
ToolSearch才能发现exit_plan_mode,多了一轮不必要的往返。进入时自动揭示消除了这个摩擦,同时通过回滚保护保证正确性。审查者测试计划
验证方法
/plan或让模型调用enter_plan_mode)。exit_plan_mode出现在可用工具列表中,无需模型调用ToolSearch。exit_plan_mode能正确退出计划模式。setTools失败,验证揭示被回滚,模型仍可通过ToolSearch作为回退方案。测试环境
风险与范围
setTools()是异步 IPC 往返;失败时揭示被回滚,模型仍可通过ToolSearch发现exit_plan_mode。无硬依赖。关联 Issue
无(本地自行挖掘的改进,非来自上游 Issue)。