fix(core): reliably deliver manual plan-exit notices - #7744
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Thanks for the PR! Template looks good ✓ Problem: observed bug with a linked issue (#7745, Direction: aligned. Plan mode exit signaling is core to the plan-mode feature (#7671), and moving delivery to the chat history commit boundary is the natural chokepoint that covers all model-bound message types without per-protocol injection. No CHANGELOG reference needed — this fixes a gap in a recently merged feature. Size: 222 production lines, 718 test lines, 100 docs lines across 16 files. Well under the 500-line threshold — no maintainer escalation needed. The test-to-production ratio (3.2:1) is strong. Approach: the scope feels right. The event/cursor separation ( Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到的 bug,有关联 issue(#7745, 方向:对齐。Plan 模式退出信号是 plan-mode 功能的核心(#7671),将投递移到 chat history 提交边界是覆盖所有 model-bound 消息类型的自然咽喉点,无需逐协议注入。 规模:222 行生产代码、718 行测试代码、100 行文档,共 16 个文件。远低于 500 行阈值,无需维护者升级。测试与生产代码比率(3.2:1)很强。 方案:范围合理。event/cursor 分离( 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: given the problem (notice only delivered on UserQuery/Cron paths, single boolean consumed by first conversation), I would move injection to Findings: no critical blockers or AGENTS.md violations found. The implementation is clean and follows the established
Test coverage is comprehensive: 718 lines covering transition semantics, inherited event ownership, independent cursors, stale restore, opt-in delivery, part ordering, setup rollback, provider retry, reactive compression (both model-tail and user-tail), chat recreation, and agent isolation. CI Test Evidence
macOS, Windows, and Integration checks are skipped — this is a fork PR limitation, not a PR defect. The Ubuntu unit suite passed. Not verified: cross-platform behavior (the PR author also notes macOS-only local testing). 中文说明代码审查独立方案: 给定问题(通知仅在 UserQuery/Cron 路径投递,单一布尔值被第一个会话消费),我会将注入移到 发现: 未发现关键阻塞问题或 AGENTS.md 违规。 实现干净,遵循既有的 CI 测试证据Ubuntu 单元测试通过。macOS、Windows 和集成测试被跳过——这是 fork PR 的限制,不是 PR 的缺陷。未验证:跨平台行为。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 4/5 — solid fix for a real coverage gap; clean design, comprehensive tests, CI green on Ubuntu. Only non-blocking nit: cross-platform CI was skipped (fork limitation). This is a well-executed fix. The problem is real — #7682 shipped a notice that only fires on two of the many model-bound paths, and the shared boolean creates a race between conversations. The event/cursor design is the minimal structure that solves both issues, and the implementation lands it cleanly at the chat history commit boundary. Every change in the diff earns its place: the rollback handles setup failure, the compression recovery handles reactive rebuilds, the agent isolation handles multi-conversation ownership. The 3.2:1 test-to-production ratio gives real confidence in the edge cases. My independent proposal matched the PR's approach almost exactly, which is a good sign — there isn't a simpler path being missed. The code follows established Config prototype patterns, the notice text improvement is a bonus, and the backward-compatible wrapper means no downstream breakage. If I had to maintain this in six months, the design doc ( 中文说明置信度:4/5 —— 对真实覆盖缺口的扎实修复;设计干净,测试全面,Ubuntu CI 通过。唯一非阻塞的小点:跨平台 CI 被跳过(fork 限制)。 这是一个执行良好的修复。问题是真实的——#7682 发布的通知只在许多 model-bound 路径中的两条上触发,共享布尔值在会话之间产生竞争。event/cursor 设计是解决两个问题的最小结构,实现在 chat history 提交边界干净地落地。diff 中的每个改动都有其价值:回滚处理 setup 失败,压缩恢复处理 reactive 重建,Agent 隔离处理多会话所有权。3.2:1 的测试与生产代码比率给了边界情况真正的信心。 我的独立方案与 PR 的方案几乎完全一致——没有更简单的路径被遗漏。代码遵循既有的 Config 原型模式,通知文本改进是额外的加分,向后兼容的包装意味着没有下游破坏。 — Qwen Code · qwen3.8-max-preview Reviewed at |
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.
Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.7-max via Qwen Code /review
yiliang114
left a comment
There was a problem hiding this comment.
PR Review: fix(core): reliably deliver manual plan-exit notices
Summary
This PR fixes a gap where manual plan-exit notices weren't reliably delivered across all conversation types. The previous implementation using a single global pendingManualPlanExitNotice boolean only covered UserQuery and Cron reminders, missing ACP/daemon direct sends, tool-result continuations, steering continuations, hooks, and interactive agents.
Architecture Assessment ✅
The redesign is well-considered:
- Event vs Cursor separation: Separating the approval-mode event (
ManualPlanExitNoticeEventState) from per-conversation cursors (ManualPlanExitNoticeCursorState) is the key insight. Each conversation can independently claim the same inherited event once, solving the "one conversation steals another's notice" problem. - Version-based tracking: Using a monotonically incrementing version number provides a clean mechanism for:
- Detecting stale restore attempts
- Preventing duplicate delivery
- Supporting inheritance isolation when child configs copy event state on first mode ownership
- Commit boundary delivery: Moving notice injection to the history commit point in
GeminiChat.sendMessageStreamis architecturally cleaner than protocol-specific injection paths.
Code Quality ✅
Strengths:
- Comprehensive rollback handling in the synchronous setup path
- Proper preservation of notice across reactive context-overflow compression
- Clean opt-in API (
enableManualPlanExitNotices()) with explicit enablement for main chats and interactive agents only - Backward compatibility maintained via
consumePendingManualPlanExitNotice()
Small observations:
- The inheritance pattern via
Object.create(parent)and lazy state ownership is consistent with the existing codebase patterns - The notice text is clear and actionable for the model
Test Coverage ✅
Excellent coverage across:
- State machine transitions (rapid Plan round trips, mode switches)
- Inheritance semantics (parent/child isolation, cursor independence)
- Rollback scenarios (setup failure, provider retry, reactive compression)
- Agent integration (interactive vs headless, fork/speculation exclusion)
- Edge cases (stale restore, replacement chat with same cursor)
The 379 new lines in geminiChat.test.ts thoroughly exercise the delivery semantics.
Security Considerations ✅
No security concerns identified:
- The notice is purely informational to the model about mode state
- No user data or credentials involved
- No execution privilege escalation
Potential Minor Suggestions
-
Documentation comment clarity (optional): In
getManualPlanExitNoticeEventState(), the comment about lazy ownership could briefly explain why the inheritance copy happens (to isolate child from parent's subsequent events). The design doc covers this, but inline it might help future maintainers. -
Type safety: The prototype method calls like
Config.prototype.getManualPlanExitNoticeEventState.call(this)are necessary for the inheritance pattern, but consider adding a brief comment explaining why direct calls would be insufficient.
CI Status
- ✅ Test (ubuntu-latest, Node 22.x) passed
- Tests on macOS/Windows were skipped (noted in PR body as partially validated)
Conclusion
This is a well-designed fix with thorough test coverage. The state machine approach cleanly solves the delivery reliability problem while maintaining backward compatibility and proper isolation semantics.
One minor suggestion for future consideration: the design doc mentions that "Notice persistence across process restarts" is out of scope. If this becomes a requirement later, the version-based state machine provides a clean foundation for adding persistence.
|
Released in v0.21.1. |
|
|
What this PR does
This PR makes the one-shot manual Plan-exit notice reliable across every supported live conversation. It separates approval-mode events from per-conversation delivery cursors, so the main conversation and each interactive agent can receive the same inherited mode change independently while replacement chats in the same conversation do not redeliver it.
Notice delivery now happens at the live chat history commit boundary after proactive compression and hard-rescue checks. The notice is appended as the final user-content text part, including tool-result continuations, and synchronous setup rollback safely restores the claim. Provider retries and fallbacks reuse the committed history entry, while reactive context-overflow compression preserves the exact notice when rebuilding the retry history.
The opt-in is enabled for main chats and interactive agent chats. Fork/speculation chats, headless agents, workflows, memory helpers, and compaction side queries remain excluded.
Why it's needed
The implementation merged in #7682 consumes a single pending boolean while assembling only UserQuery and Cron reminders. That leaves ACP/daemon direct sends, tool-result and steering continuations, hooks, and interactive agents uncovered. Because the flag is shared, one conversation can also consume the notice before another live conversation sees it.
Moving delivery to the chat history commit boundary covers all model-bound message kinds without protocol-specific injection paths. Separating the mode event from each conversation's cursor preserves one-shot behavior without allowing one conversation to steal another's notice.
Reviewer Test Plan
How to verify
session/set_mode, then send the next prompt or continuation. Confirm the next wire payload contains one notice without requiring ACP-specific injection logic.exit_plan_modeflow and confirm no manual-exit notice is added.Evidence (Before & After)
The macOS PTY wire capture reproduced the missing post-Shift+Tab signal with the global CLI and verified that the current bundle appends the exact notice on the next request, keeps the total historical count at one on later turns, and preserves tool-result ordering.
Tested on
Environment (optional)
macOS 26.4.1, Node v22.22.3, npm 10.9.8; local source build, Vitest, and PTY wire-payload capture. Live DashScope completion was unavailable in the sandbox because DNS resolution failed, so ACP and interactive-agent ownership were additionally verified through their exact construction and send paths.
Risk & Scope
Linked Issues
Closes #7745.
Refs #7671 (Problem 1). Follow-up to #7682 and complementary to #7673.
中文说明
本 PR 做了什么
本 PR 让手动退出 Plan 的一次性通知能够可靠覆盖所有受支持的当前进程内会话。实现将审批模式事件与每个会话的投递游标分离,因此主会话和每个交互式 Agent 可以分别收到同一个继承的模式变化,而同一会话中的 chat 重建不会重复投递。
通知现在在 proactive compression 和 hard-rescue 检查完成后的 live chat history 提交边界投递。通知作为 user content 的最后一个文本 part 追加,包含 tool-result 续轮;同步 setup 回滚会安全恢复 claim。provider retry 和 fallback 复用已经提交的 history entry,reactive context-overflow compression 在重建重试 history 时会保留完全相同的通知。
该 opt-in 只为主 chat 和交互式 Agent chat 开启。fork/speculation chat、headless Agent、workflow、memory helper 和 compaction side query 继续保持关闭。
为什么需要
#7682 中合入的实现只在 UserQuery 和 Cron reminder 装配期间消费一个全局 pending boolean,因此 ACP/daemon 直发、tool-result 和 steering 续轮、hook 以及交互式 Agent 仍然无法覆盖。由于该标志是共享的,一个会话还可能在其他 live conversation 看见通知之前先将它消费。
将投递移动到 chat history 提交边界后,无需增加协议专用注入路径即可覆盖所有 model-bound message 类型。将 mode event 与每个会话的 cursor 分离,则能维持 one-shot 行为,同时防止一个会话抢走另一个会话的通知。
审阅测试计划
如何验证
session/set_mode将 live session 从 Plan 切换到 Default,然后发送下一次 prompt 或 continuation。确认下一个 wire payload 包含一次通知,且不需要 ACP 专用注入逻辑。exit_plan_mode流程离开 Plan,确认不会添加手动退出通知。证据(Before & After)
macOS PTY wire capture 使用全局 CLI 复现了 Shift+Tab 后缺少明确信号的问题,并验证当前 bundle 会在下一次请求中追加完全相同的通知、后续轮次的历史总数保持为一,以及 tool-result 顺序保持正确。
测试平台
环境(可选)
macOS 26.4.1、Node v22.22.3、npm 10.9.8;本地源码构建、Vitest 和 PTY wire-payload capture。由于 sandbox 中 DNS 解析失败,无法完成真实 DashScope provider 响应,因此 ACP 和交互式 Agent 所有权还通过其准确的构造与发送路径进行了验证。
风险与范围
关联 Issues
关闭 #7745。
关联 #7671(Problem 1)。这是 #7682 的后续改进,并与 #7673 互补。