Skip to content

feat(cli): add daemon Todo stop guard - #6945

Merged
doudouOUC merged 6 commits into
QwenLM:mainfrom
doudouOUC:agent/daemon-todo-stop-guard
Jul 18, 2026
Merged

feat(cli): add daemon Todo stop guard#6945
doudouOUC merged 6 commits into
QwenLM:mainfrom
doudouOUC:agent/daemon-todo-stop-guard

Conversation

@doudouOUC

@doudouOUC doudouOUC commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

This PR adds an opt-in, daemon/ACP-only Todo Stop Guard. After a successful top-level todo_write leaves unfinished items, a daemon Session may automatically continue the current work chain at most twice instead of silently ending on a natural model stop. The guard is disabled by default, does not affect TUI or the shared Core agent loop, and is force-disabled in safe, bare, and plan modes.

The implementation reuses the existing Session continuation loop and coordinates external Stop hooks, mid-turn input, FIFO prompts, background agents and commands, monitors, wakeups, notifications, cron turns, retries, rewinds, reconnect replay, cancellation, compaction, and hard terminal paths. Guard status is emitted as replayable metadata without exposing Todo text, and the ACP bridge now reports whether a complete non-aborted prompt is already queued so user input always wins over automatic continuation.

The change also adds the experimental setting schema, daemon configuration documentation, a design document, focused state and Session coverage, bridge protocol coverage, and a bundled daemon streaming regression test.

Why it's needed

Long-running daemon clients can disconnect or wait asynchronously while the accepted task continues in the Session. A model may naturally stop even though its current top-level Todo list still contains pending work, leaving the daemon chain incomplete and forcing a client or user to notice and manually continue it. A bounded, explicitly enabled Session guard closes that gap while preserving permissions, queued user input priority, existing Stop-hook semantics, and resource-protection termination behavior.

Reviewer Test Plan

How to verify

  1. Start qwen serve with experimental.todoStopGuard disabled and confirm natural stops and external Stop hooks behave exactly as before.
  2. Enable the setting, issue a daemon prompt whose successful top-level todo_write leaves an unfinished item, and confirm the Session makes no more than two additional main-model calls, emits replayable todo_stop_guard status events, and never exposes Todo text in those events.
  3. Confirm a queued ordinary prompt or mid-turn message runs before any Guard continuation, while background work created by the current work chain defers the Guard without consuming an attempt.
  4. Confirm completing or clearing Todos disarms the Guard, hard terminal paths prevent related background results from reviving it, and a new ordinary prompt must execute its own successful top-level todo_write before the Guard can run.
  5. Confirm safe mode, bare mode, and approval plan mode disable the feature, while disableAllHooks does not disable an explicitly enabled built-in Guard.
  6. Disconnect the REST/SSE client after prompt acceptance and confirm the bounded continuation finishes in the daemon and its status and response events are replayable on reconnect.

Automated verification completed locally after rebasing onto the latest main: 638 CLI tests, 513 ACP bridge tests, and 4 bundled qwen serve streaming integration tests passed. ESLint, Prettier, generated settings schema checks, diff checks, the full repository build and typecheck, the CLI-only build, and bundle generation also passed.

Evidence (Before & After)

Before: an accepted daemon task can naturally end with trusted top-level Todos still pending, and no built-in Session mechanism schedules bounded continuation.

After: when the experimental setting is enabled, the Session performs at most two consecutive automatic continuations without new user input, exposes each attempt as a replayable status event, yields to user and related background input, and reports an explicit exhausted status if unfinished items remain.

Tested on

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

Environment (optional)

macOS, Node.js 22.22.3, bundled dist/cli.js, QWEN_SANDBOX=false for daemon integration coverage.

Risk & Scope

  • Main risk or tradeoff: Enabling the feature can add model cost, bounded to two consecutive main-model calls without new user input; it is disabled by default and emits visible usage and Guard status metadata.
  • Not validated / out of scope: Windows and Linux were not tested locally; process-crash recovery is intentionally unsupported; no TUI setting UI is included.
  • Breaking changes / migration notes: None. The feature requires explicit experimental.todoStopGuard: true configuration and a daemon restart.

Linked Issues

Closes #6946

中文说明

本 PR 做了什么

本 PR 增加了一个需显式启用、仅作用于 daemon/ACP 的 Todo Stop Guard。顶层 todo_write 成功执行且仍有未完成事项时,daemon Session 可以在模型自然停止后最多自动继续当前工作链两次,而不是静默结束。Guard 默认关闭,不影响 TUI 或共享的 Core agent loop,并且在 safe、bare 和 plan 模式中强制关闭。

实现复用了现有 Session continuation loop,并协调了外部 Stop hooks、mid-turn 输入、FIFO prompt、后台 Agent 和命令、Monitor、wakeup、notification、cron 轮次、retry、rewind、断线回放、取消、压缩和硬终止路径。Guard 状态以可回放 metadata 发送且不暴露 Todo 文本;ACP bridge 还会报告是否已有完整且未取消的 prompt 排队,确保用户输入始终优先于自动 continuation。

本次变更还增加了实验设置 schema、daemon 配置文档、设计文档、状态机与 Session 的聚焦测试、bridge 协议测试,以及基于 bundle 的 daemon streaming 回归测试。

为什么需要它

长时间运行的 daemon 客户端可能在已接纳任务仍由 Session 执行时断连或异步等待。模型可能在当前顶层 Todo 列表仍有 pending 工作时自然停止,导致 daemon 工作链不完整,并要求客户端或用户主动发现后手工继续。有界且显式启用的 Session Guard 可以弥补这个缺口,同时保持权限系统、排队用户输入优先级、现有 Stop-hook 语义和资源保护终止行为不变。

Reviewer 测试计划

如何验证

  1. 在关闭 experimental.todoStopGuard 的情况下启动 qwen serve,确认自然停止和外部 Stop hooks 的行为与以前完全一致。
  2. 启用设置,向 daemon 发送一个会成功执行顶层 todo_write 且留下未完成事项的 prompt,确认 Session 最多增加两次主模型调用、发送可回放的 todo_stop_guard 状态事件,并且事件不暴露 Todo 文本。
  3. 确认排队的普通 prompt 或 mid-turn 消息先于任何 Guard continuation 执行,而当前工作链创建的后台工作会让 Guard 延后且不消耗次数。
  4. 确认完成或清空 Todos 会解除 Guard,硬终止路径会阻止相关后台结果重新激活 Guard,新普通 prompt 必须再次成功执行自己的顶层 todo_write 才能运行 Guard。
  5. 确认 safe mode、bare mode 和 approval plan mode 会关闭该功能,而 disableAllHooks 不会关闭已显式启用的内建 Guard。
  6. 在 prompt 被接纳后断开 REST/SSE 客户端,确认有界 continuation 会继续在 daemon 内完成,并且状态和响应事件可在重连后回放。

rebase 到最新 main 后,本地自动验证已经完成:638 个 CLI 测试、513 个 ACP bridge 测试和 4 个 bundle 版本的 qwen serve streaming 集成测试通过。ESLint、Prettier、生成 settings schema 的检查、diff 检查、全仓 build 和 typecheck、CLI-only build 和 bundle 生成也均通过。

证据(Before & After)

Before:已接纳的 daemon 任务可能在可信顶层 Todos 仍有未完成事项时自然结束,Session 没有内建机制安排有界 continuation。

After:启用实验设置后,Session 在没有新用户输入时最多连续自动继续两次,将每次尝试作为可回放状态事件暴露,让行用户输入和相关后台输入,并在仍有未完成事项时发送明确的耗尽状态。

测试平台

OS 状态
🍏 macOS ✅ 已测试
🪟 Windows ⚠️ 未测试
🐧 Linux ⚠️ 未测试

环境(可选)

macOS、Node.js 22.22.3、bundle 后的 dist/cli.js,daemon 集成覆盖使用 QWEN_SANDBOX=false

风险与范围

  • 主要风险或权衡:启用功能可能增加模型成本;在没有新用户输入时最多连续增加两次主模型调用。功能默认关闭,并发送可见的 usage 和 Guard 状态 metadata。
  • 未验证或不在范围内:本地未测试 Windows 和 Linux;进程崩溃恢复按设计不支持;不包含 TUI 设置界面。
  • 破坏性变更或迁移说明:无。需要显式配置 experimental.todoStopGuard: true 并重启 daemon。

关联 Issues

关联并在合并时关闭 #6946

@doudouOUC doudouOUC self-assigned this Jul 15, 2026
@doudouOUC
doudouOUC force-pushed the agent/daemon-todo-stop-guard branch from cb40333 to 1982e29 Compare July 15, 2026 09:12
@github-actions

Copy link
Copy Markdown
Contributor

Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration.

中文

请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动压缩(squash)为单个提交。

@doudouOUC
doudouOUC requested a review from wenshao July 15, 2026 11:12

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

Full review (approach + code). Verdict: approach is sound, implementation quality is high; nothing blocking.

  • Right layer: daemon/ACP-only Session guard, TUI/core/TodoWrite untouched; not shipping it as a built-in Stop hook is the right call given the FIFO/mid-turn/background-baseline coupling and the disableAllHooks semantics.
  • The trust model is well-conceived: only a successful top-level structured TodoWriteTool result arms; args, replayed history, disk state, and MCP shadow tools are rejected; every hard terminal path suspends; budget is bounded per uninterrupted stage with default-off + safe/bare/plan force-off + in-memory-only.
  • Design doc matches the implementation closely — I cross-checked stop ordering, tool-closure, compaction preservation, and bridge compatibility sections against the code.
  • Verified locally on this branch: daemon-todo-stop-guard.test.ts 18/18, full Session.test.ts 343/343 (incl. the 75 guard scenarios), bridgeClient.test.ts + compactionEngine.test.ts 118/118.

Findings inline: 1 should-fix (silently swallowed guard-release notification), 2 minor, 2 nits, 1 docs suggestion.

One non-blocking maintainability note: Session.ts grows 6.4k → 7.4k lines with guard touch-points spread across ~40 sites, and #runStopContinuation's beforeSend closure mutates six outer variables (including reassigning nextMessage from inside the callback). Correctness is currently carried by the (excellent) test suite; consider extracting the stop-ordering orchestration into its own module in a follow-up PR.

Comment thread packages/acp-bridge/src/bridge.ts Outdated
Comment thread packages/cli/src/acp-integration/session/Session.ts Outdated
Comment thread packages/cli/src/acp-integration/session/Session.ts
Comment thread packages/cli/src/acp-integration/session/Session.ts
Comment thread packages/cli/src/acp-integration/session/Session.ts
Comment thread docs/users/qwen-serve.md
@doudouOUC
doudouOUC marked this pull request as ready for review July 15, 2026 12:37
@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Re-triage at fe64ed2a58

Branch rebased onto latest main since the previous triage at 03d4ffe6ca. The only new commit is fe64ed2a58 ("codex: resolve PR merge conflict") — a pure merge-conflict resolution with no logic change. The PR's own six commits are patch-equivalent after rebase.

Template ✓ — all required headings present and complete.

Problem: observed and documented. The linked issue #6946 describes a real gap: the model can naturally stop while a top-level todo_write leaves unfinished items, breaking daemon work chains. The PR body provides before/after evidence and a concrete 6-step verification plan. This is not theoretical hardening.

Direction: aligned. Daemon reliability for qwen serve is core product territory. The feature is opt-in (default: false, requiresRestart: true), scoped to daemon only (no TUI or Core agent loop changes), and forced off in safe/bare/plan modes. No direction concerns.

Size:

Category Lines
Production (core paths) 10 (settingsSchema.ts)
Production (non-core) ~1,940 (Session, guard state machine, bridge, acpAgent, compaction)
Tests ~5,100
Docs ~223
Schema 5

Core module exposure is minimal — just one settings schema entry. No escalation needed.

Approach: the scope is large but justified. The guard must coordinate with every session lifecycle path (stop hooks, FIFO, mid-turn input, background agents, monitors, wakeups, cron, retry, rewind, cancellation, compaction). A simpler implementation would just miss edge cases. The safety posture is conservative: bounded to 2 attempts, no Todo text in telemetry, explicit suspension on abort/cancel, and a fresh todo_write required per prompt.

Rebase integrity: clean. The merge-conflict commit only touches context offsets in the experimental settings table — no semantic drift in PR logic.

Moving to code review. 🔍

中文说明

fe64ed2a58 的重新审查

自上次在 03d4ffe6ca 的审查以来,分支已 rebase 到最新 main。唯一的新提交是 fe64ed2a58("codex: 解决 PR 合并冲突")——纯合并冲突解决,无逻辑变更。PR 自身的六个提交在 rebase 后 patch 等价。

模板完整 ✓

问题: 已观测且有文档。关联 issue #6946 描述了真实缺口。这不是理论性加固。

方向: 对齐。功能需显式启用,仅作用于 daemon,在 safe/bare/plan 模式下强制关闭。

规模: 核心模块暴露极小(10 行)。非核心生产代码约 1,940 行,测试约 5,100 行。无需 escalation。

方案: 范围大但合理。Guard 必须与所有 Session 生命周期路径协调。安全姿态保守。Rebase 完整性:干净,无语义漂移。

进入代码审查 🔍

Qwen Code · qwen3.7-max

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

@qwen-code-ci-bot qwen-code-ci-bot added status/in-review This issue is currently in review. category/integration External integrations scope/session-management Session state and persistence daemon labels Jul 15, 2026
ytahdn
ytahdn previously approved these changes Jul 15, 2026

@ytahdn ytahdn 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. 高质量的实现——状态机设计严谨,guard 模块封装干净,Session 集成正确处理了 mid-turn input、queued prompt、background agents、cancel、compaction、replay 等复杂协调场景。测试覆盖充分(75 guard 场景 + bridge 协议 + E2E streaming replay)。

核心确认:

  • decide() / commitContinuation() / decideToolClosure() 边界条件正确
  • 用户输入优先级正确:mid-turn 和 queued prompt 先于 guard continuation
  • 硬终止路径(cancel/safe/plan)正确 suspend guard
  • status event 不暴露 Todo 文本,仅发送 count
  • bridge abort listener 正确清理 todoStopGuardAwaitingQueuedPrompt

作者自审到位,open comments 均为非阻塞项。后续可考虑将 #runStopContinuation 的 stop-ordering orchestration 提取为独立模块(beforeSend closure 6+ 外部变量确实需要更好的封装)。

— qwen3.7-plus via Qwen Code /review

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Re-verification at fe64ed2a58

Re-triage after clean rebase onto latest main. The previous code review at 03d4ffe6ca covered the full diff in detail — two independent reviewers across 7+ rounds, all Critical findings resolved. The only commit since then is fe64ed2a58 ("codex: resolve PR merge conflict"), a pure markdown column re-pad of the experimental.artifact settings table row. No PR source logic was touched.

Code review — unchanged from previous pass

The DaemonTodoStopGuard state machine and Session integration were reviewed line-by-line in the previous pass. Key verified properties:

  • while (true) loop is bounded: attempt counter monotonic within each stage, hard-capped at 2 by commitContinuation; no path resets the counter without new user input.
  • Feature off = true no-op: all drain and beforeSend paths gated on needsStopInspection/guard sponsorship, which is false when disabled.
  • Trust gate is real: triple-check on Kind.Think + constructor.name + ToolNames.TODO_WRITE; {type:'todo_list'} envelope matches core validator.
  • FIFO/mid-turn race matrix: all combinations walked (drain + release ext, drain timeout, concurrent mid-turn + queued prompt, bridge abort, beforeSend commit failure). Self-consistent.
  • Abort-path gaps closed: cbae68ffb (rebased into current history) added todoStopGuard.suspend() at cron and background-notification abort sites — all 24 signal.aborted sites audited clean.
  • Notification overflow: 1112bde73 (rebased into current history) drops the incoming related notification with explicit warning instead of silently evicting a queued one.

Remaining non-blocking findings from previous pass

  1. [Suggestion] Related automatic-turn streaming errors may leave delayed automatic work stuck until next user action (notification/cron catch pauseForTrustedRetry() with no remaining rescuer). Worth a follow-up test.
  2. [Nit] Tool-closure attempt status event wording ("Automatic continuation 2/2 started; 0 todo item(s) remain unfinished") is confusing.

CI status at fe64ed2a58

Check Result
Test (ubuntu-latest, Node 22.x) ✅ pass (24m50s)
web-shell E2E Smoke ✅ pass (5m47s)
precheck-pr ✅ pass
Classify PR ✅ pass

All CI green at the new head. The previous local verification (1431 unit tests + 4 bundled-daemon E2E on both Linux and macOS at earlier heads) covered the same PR logic that is present after this rebase.

Rebase integrity

The compare between old triage head and new head shows only upstream main commits being folded in. The PR's own six commits are patch-equivalent. The merge-conflict commit (fe64ed2a58) only adjusts markdown column widths in the experimental settings documentation table — todoStopGuard appears exactly once, no duplicate keys, no semantic drift.

中文说明

fe64ed2a58 的复验

在干净 rebase 到最新 main 后重新审查。此前在 03d4ffe6ca 的代码审查已覆盖完整 diff——两位独立审查者、7+ 轮、所有 Critical 发现已解决。此后唯一的新提交是 fe64ed2a58("codex: 解决 PR 合并冲突"),仅调整 experimental.artifact 设置表格的 markdown 列宽。未触及 PR 源码逻辑。

代码审查 — 与此前审查一致。状态机、Session 集成、abort 路径缺口修复、通知溢出修复均已验证。

CI 状态:ubuntu 测试(24m50s)、web-shell E2E smoke、precheck 全部通过。

Rebase 完整性:干净。PR 自身的六个提交 patch 等价。合并冲突提交仅调整 markdown 列宽。

Qwen Code · qwen3.7-max

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

@ytahdn
ytahdn dismissed their stale review July 15, 2026 13:18

撤回 approve,作者要求暂不通过

@wenshao

wenshao commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

✅ Local build + real-test verification (Linux)

I built and exercised this PR end-to-end on Linux, which the PR description lists as not-yet-tested. Everything is green — including a live run of the bundled qwen serve daemon with the guard armed.

Environment: Linux x64 · Node v22.22.2 · clean npm ci (lockfile identical to main, so deps are unchanged) · PR head 1982e29a0 on base 89ab15d2f.

Results

Area Command Result
Build npm run build -- --cli-only tsc --build clean across all bundled packages
Bundle npm run bundle dist/cli.js, qwen serve boots (--version 0.19.10)
Types tsc --noEmit (cli + acp-bridge) ✅ 0 errors
Lint eslint --max-warnings 0 (15 changed .ts) ✅ 0 warnings / 0 errors
Format prettier --check ✅ all changed files match
Schema generate:settings-schema ✅ no drift (committed schema == regenerated)
Unit daemon-todo-stop-guard.test.ts 18 passed
Unit Session.test.ts + acpAgent.test.ts 588 passed
Unit acp-bridge suite (14 files) 824 passed
E2E qwen-serve-streaming.test.ts (bundled daemon, QWEN_SANDBOX=false) 4 passed

Local verification matrix — Linux x64

Live daemon run

Beyond the suite, I ran the real bundled daemon against a fake model endpoint with experimental.todoStopGuard: true, admitted a prompt with no SSE subscriber attached (client "offline"), then reconnected and replayed the stream:

  • The guard performed a bounded 2-attempt continuation → exactly 4 marker-bearing model calls total, matching the design's "at most two consecutive automatic streams without new user input".
  • Exactly 3 replayable todo_stop_guard status events, attempt sequence [1, 2, 2].
  • Exhaustion status emitted: Automatic continuation stopped after 2 attempts; 1 todo item(s) remain unfinished.
  • Todo text never leaked into any guard event _meta.
  • turn_complete replayed correctly after reconnect (the disconnected-client path).

Live bundled qwen serve daemon — Todo Stop Guard bounded continuation

Notes for merge

  • Opt-in is correct: default: false, requiresRestart: true, showInDialog: false, and the design forces it off in safe / bare / Approval-plan modes.
  • Bridge changes are backward compatible — hasQueuedPrompt is optional and an omitting (older) client is treated as false.
  • The VS Code settings.schema.json is in sync (regeneration is a no-op).
  • Out of scope here: Windows (the streaming suite skips non-POSIX) and process-crash recovery (intentionally unsupported per the design doc).

LGTM from a build/test standpoint on Linux.

中文说明

✅ 本地构建 + 真实测试验证(Linux)

我在 Linux 上对本 PR 做了端到端的构建与运行验证(PR 描述里 Linux 标注为未测试)。全部通过,其中包括用启用 guard 的 bundle 版 qwen serve daemon 做的一次真实运行。

环境: Linux x64 · Node v22.22.2 · 全新 npm ci(lockfile 与 main 完全一致,依赖无变化)· PR head 1982e29a0,base 89ab15d2f

结果

项目 命令 结果
构建 npm run build -- --cli-only ✅ 所有打包内的包 tsc --build 通过
打包 npm run bundle ✅ 生成 dist/cli.jsqwen serve 可启动(--version 0.19.10)
类型 tsc --noEmit(cli + acp-bridge) ✅ 0 报错
Lint eslint --max-warnings 0(15 个改动 .ts ✅ 0 warning / 0 error
格式 prettier --check ✅ 全部符合规范
Schema generate:settings-schema ✅ 无漂移(提交的 schema == 重新生成的)
单测 daemon-todo-stop-guard.test.ts 18 通过
单测 Session.test.ts + acpAgent.test.ts 588 通过
单测 acp-bridge 套件(14 个文件) 824 通过
E2E qwen-serve-streaming.test.ts(bundle daemon,QWEN_SANDBOX=false 4 通过

Local verification matrix — Linux x64

真实 daemon 运行

除测试套件外,我用真实的 bundle daemon 连到一个 fake 模型端点,开启 experimental.todoStopGuard: true,在没有 SSE 订阅者(客户端“离线”)的情况下接纳一个 prompt,然后重连并回放事件流:

  • guard 执行了有界的 2 次自动继续 → 总共恰好 4 次带 marker 的模型调用,符合设计中「无新用户输入时最多连续两次自动流」的约束。
  • 恰好 3 个可回放的 todo_stop_guard 状态事件,attempt 序列为 [1, 2, 2]
  • 发出耗尽状态:Automatic continuation stopped after 2 attempts; 1 todo item(s) remain unfinished.
  • guard 事件的 _meta从不泄露 Todo 文本
  • 重连后 turn_complete 正确回放(断线客户端路径)。

Live bundled qwen serve daemon — Todo Stop Guard bounded continuation

合并参考

  • opt-in 设计正确:default: falserequiresRestart: trueshowInDialog: false,且在 safe / bare / Approval-plan 模式下强制关闭。
  • bridge 改动向后兼容 —— hasQueuedPrompt 为可选字段,旧客户端不带该字段时按 false 处理。
  • VS Code 的 settings.schema.json 保持同步(重新生成为 no-op)。
  • 本次未覆盖:Windows(streaming 套件在非 POSIX 上跳过)与进程崩溃恢复(按设计文档明确不支持)。

从 Linux 的构建/测试角度看,LGTM。


🤖 Built and tested locally with Claude Code (Opus 4.8, 1M context) on Linux x64. Every command above was run against the PR head; screenshots are captured from those runs.

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

Test-only diff chunks (2, 5, 11) were reviewed by the whole-diff Test Coverage Matrix agent and mapped to ~60 test cases covering all acceptance criteria; the territory fan-out did not re-review them.

Not reviewed: chunk 2, chunk 5, chunk 11 — no territory agent reported covering these.

— qwen3.7-max via Qwen Code /review

Comment thread packages/cli/src/acp-integration/session/Session.ts Outdated
Comment thread packages/acp-bridge/src/bridge.ts Outdated

@ytahdn ytahdn 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 with focus on blocker re-check and core source changes.

Blocker re-check:

  • Session.ts:2719 (systemMessage dropped on mid-turn continue) — still stands, but severity is Suggestion-level, not Critical. The systemMessage is informational content meant for the user; dropping it silently is collateral damage from the continue. Consider emitting it before the continue, or adding a comment stating the drop is intentional.
  • Session.ts:2737 (stopHookIterationCount off-by-one) — still stands, Nice-to-have. Self-correcting on the next iteration.

Core guard module (daemon-todo-stop-guard.ts): No defects. State transitions verified correct across all paths. decide() / commitContinuation() / decideToolClosure() edge cases all handled properly.

Session integration: No Critical defects. All abort/cancel paths properly call suspend(). commitContinuation timing correct (after stream starts). Compaction preserves guard state. Replay events properly emitted. beforeSend closure mutations are sequential and correct (single-threaded, no race).

Overall: High-quality implementation. Author's self-review is thorough. The two open blockers are real but minor — neither blocks merge.

— qwen3.7-plus via Qwen Code /review

@wenshao

wenshao commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Code review — daemon Todo Stop Guard

Companion to my earlier build/test verification comment (Linux, PR head 1982e29a0): this is the correctness pass over the design + implementation. I read the design doc, the guard state machine, the full Session.ts integration (all ~40 touch-points against the surrounding code, not just the hunks), the bridge/bridgeClient/compaction/acpAgent changes, and the test matrix.

Verdict: LGTM, non-blocking findings only. The state machine held up against every race I could construct on paper. One new [Suggestion] below (a liveness gap the existing findings don't cover), one nit, and my read on the two findings already on the thread.

What I traced and confirmed

  • The while (true) loop is actually bounded. Guard attempts are monotonic per stage and hard-capped by commitContinuation (attempt > TODO_STOP_GUARD_MAX_ATTEMPTS refuses); the external-hook branch preserves the old increment-then-check cap semantics exactly (including the >= cap warning + abortGoalForStopHookCap); every mid-turn iteration consumes real queued user input, so it cannot spin without a user feeding it. A todo_write during attempt 2 that rewrites the list unfinished still lands in exhausted (attempts don't reset). The supersededAutomaticContinuation refund path also consumes the drained input, so it makes progress too.
  • Feature-off is a true no-op. Every new drain and the beforeSend hook are gated on needsStopInspection / guard sponsorship, which are constant-false when disabled; the success-path queue drain in prompt() is byte-identical to main (the finally-block drain is additive, for guard-deferred error exits). Default users get one extra no-op loop iteration and nothing else.
  • The trust gate is real. Kind.Think + constructor.name === 'TodoWriteTool' + ToolNames.TODO_WRITE triple-check matches core (todoWrite.ts:569), the {type:'todo_list'} envelope matches todoWrite.ts:470, and the guard's status union is byte-identical to core's validator (todoWrite.ts:592) — a core status enum change fails closed (parse → null → no arm).
  • FIFO/mid-turn race matrix. I walked: release-ext arriving mid-drain (begin/finish pairing converges to suspended+released), drain timeout (queued-prompt snapshot correctly discarded), mid-turn + queued prompt in one drain (mid-turn runs first, todoStopGuardQueuedPromptPriority survives and the loop-top check at Session.ts:2641 ends the turn — exactly design step 2), bridge abort-listener self-exclusion with a second queued prompt, and commit-fails-after-beforeSend (no double emit, no extra attempt possible; the stale guardForThisSend is neutralized by decideToolClosure's attempts !== currentAttempt check). All coherent.
  • Nice touch I want to call out: #prepareTodoStopGuardForPrompt suspending at prep time doubles as child-side FIFO priority for direct-ACP clients that never answer the drain ext — the bridge hasQueuedPrompt only has to cover the queue the child can't see.
  • Compaction/replay: guard events ride qwenDiscreteMessage + source into non-merging misc slots; #isUserTextContent excludes guard-sponsored sends (the some() is right — any pure-text message carrying the guard prompt is guard-sponsored, including coalesced hook+guard parts; tool-result sends were already excluded via functionResponse), so rewind indices stay stable.

New findings

1. [Suggestion] A stream error inside a related automatic turn can strand deferred automatic work until the next user action. #executeBackgroundNotificationPromptInner's catch (Session.ts:4814) and the cron catch (Session.ts:4379) call pauseForTrustedRetry(). A retry-paused chain still has blocksUnrelatedAutomaticTurns === true (daemon-todo-stop-guard.ts:81 doesn't consult #retryPaused) but needsStopInspection === false (:90 does), so the guard will never inspect another stop, never exhausts, and never releases. For a foreground failure this is the designed contract — the daemon client sees the failed prompt and sends retry/continue. But an automatic turn's failure has no retryer: if the errored related notification was the chain's last related input, unrelated cron/loop fires and old-task notifications stay deferred (bounded and coalesced, but never run) indefinitely on an unattended session — the exact deployment this feature targets. Recovery today requires a new user prompt, an explicit continue, or a fresh related background event. Suggested direction: in the automatic-turn catches, after pauseForTrustedRetry(), check whether any related background input is still live/queued; if none, suspend() and drain — a pause with no possible autonomous resumer is a wedge, not a retry. Whichever way you decide, this path deserves a test (the excellent 75-scenario matrix covers the foreground pause + related-result rescue, but not an automatic-turn error with no rescue coming).

2. [Nit] The tool-closure attempt's status event reads Automatic continuation 2/2 started; 0 todo item(s) remain unfinished. (Session.ts:3274) — accurate but confusing for a consumer deciding whether work is stuck ("continuation started" + "0 remain"). Consider a closure-specific wording, e.g. "finishing response after Todo completion".

On the findings already on the thread

  • The bridge fire-and-forget release (bridge.ts:4378, author should-fix + bot suggestion): agree, and note my finding 1 is the same "silently wedged automatic work" failure family reached without any transport failure — if you add the child-side self-check escape hatch there, consider making it cover both.
  • The bot's [Critical] (mid-turn drain drops the Stop hook's blocking decision/systemMessage, Session.ts:~2737): verified real, but I'd rate it should-fix, not blocking — it only occurs with the guard enabled and armed, the dropped invocation doesn't increment the cap counter (so the hook re-fires next stop rather than being lost outright; the observable wart is a double invocation for side-effectful hooks plus a swallowed systemMessage). The author's "emit systemMessage before draining" is the cheap 90% fix.
  • Concur with the shared maintainability note: #runStopContinuation's beforeSend closure mutates six outer variables, and correctness in the refresh-mismatch branch depends on selectedMessage re-reading the reassigned nextMessage after the filter — it is correct today (I checked each path), but it's the spot a future edit will most likely break. +1 to extracting the stop-ordering orchestration in a follow-up, with the tests as the safety net.

Quality / security / tests

  • Security posture is solid: no Todo text in any guard telemetry (asserted in unit + E2E), args/replay/disk/MCP-shadow can't arm, safe/bare/plan force-off is belt-and-braces (constructor + per-turn + in-loop + beforeSend), and no permission-path code is touched, so a disconnected client still can't approve anything.
  • Test coverage is exceptional for this kind of coordination code — the 75 Session scenarios map almost 1:1 to the design doc's clauses, the bridge protocol tests cover watch/no-watch drains, and the E2E replay test pins the [1, 2, 2] attempt sequence with no SSE subscriber attached (matches what I observed live in my verification run).
  • Perf: up to three drain round-trips per armed natural stop (bot's point) is real but acceptable for an opt-in bounded feature; nothing on the default path.
  • Conventions: kebab-case module, collocated tests, ESM, no any, schema regen in sync — all clean.
中文说明

代码审查 — daemon Todo Stop Guard

这是对我之前构建/测试验证评论(Linux,PR head 1982e29a0)的补充:本条是针对设计与实现的正确性审查。我通读了设计文档、guard 状态机、Session.ts 集成的全部约 40 个接触点(对照周边代码而非仅 diff 片段)、bridge/bridgeClient/compaction/acpAgent 变更以及测试矩阵。

结论:LGTM,仅有非阻塞发现。 我在纸面上构造的所有竞态下状态机都成立。下面有一个既有评论未覆盖的新 [Suggestion](liveness 缺口)、一个 nit,以及我对已有两个发现的判断。

我核实过的关键路径

  • while (true) 循环实际有界。 Guard 尝试次数在每个 stage 内单调递增且被 commitContinuation 硬性封顶;外部 hook 分支完整保留了旧的先增后查 cap 语义(含 >= cap 警告与 abortGoalForStopHookCap);每次 mid-turn 迭代都会消耗真实排队的用户输入,没有用户投喂就无法空转。attempt 2 中 todo_write 重写出未完成列表仍会落入 exhausted(attempts 不重置)。
  • 功能关闭时是真正的 no-op。 所有新增 drain 与 beforeSend 都由 needsStopInspection/guard 赞助门控(禁用时恒为 false);prompt() 成功路径的队列 drain 与 main 完全一致(finally 里的 drain 是新增的,用于 guard 延迟的错误退出场景)。
  • 信任门是真实的。 Kind.Think + constructor.name === 'TodoWriteTool' + ToolNames.TODO_WRITE 三重检查与 core 一致(todoWrite.ts:569),{type:'todo_list'} 信封与 todoWrite.ts:470 一致,guard 的 status 联合类型与 core 校验器逐字一致(todoWrite.ts:592)——core 若改动 status 枚举会 fail closed。
  • FIFO/mid-turn 竞态矩阵。 我走查了:drain 进行中收到 release ext、drain 超时(队列快照正确丢弃)、同一次 drain 同时观察到 mid-turn 与排队 prompt(mid-turn 先跑,Session.ts:2641 的循环顶检查随后结束 turn——正是设计第 2 步)、bridge abort 监听器对第二个排队 prompt 的自排除、beforeSend 之后 commit 失败(不会双发事件、不会多计次数)。全部自洽。
  • 值得点名的巧思: #prepareTodoStopGuardForPrompt 在准备期挂起 guard,等价于直连 ACP 客户端侧的 FIFO 优先级——bridge 的 hasQueuedPrompt 只需覆盖子进程看不到的那个队列。
  • 压缩/回放: guard 事件以不可合并的 misc slot 保留;#isUserTextContent 排除 guard 赞助的发送(some() 语义正确;带工具结果的发送已先被 functionResponse 检查排除),rewind 索引保持稳定。

新发现

1. [Suggestion] 相关自动轮次内的流错误可能把延迟的自动工作卡到下一次用户操作。 通知 catch(Session.ts:4814)与 cron catch(Session.ts:4379)调用 pauseForTrustedRetry()。retry-paused 的链仍然 blocksUnrelatedAutomaticTurns === truedaemon-todo-stop-guard.ts:81 不看 #retryPaused)但 needsStopInspection === false:90 看),guard 从此不再检查停止、不会耗尽、不会释放。前台失败是设计契约——daemon 客户端能看到失败的 prompt 并 retry/continue;但自动轮次的失败没有重试者:若出错的相关通知是链的最后一个相关输入,无关 cron/loop 与旧任务通知将在无人值守的会话中被无限期延迟(有界且合并,但永不执行)——恰恰发生在这个功能面向的场景。建议方向:自动轮次 catch 中 pauseForTrustedRetry() 后检查是否仍有存活/排队的相关后台输入;若没有则 suspend() 并 drain——没有任何自主恢复者的暂停不是重试而是卡死。无论如何决定,该路径值得补一个测试(75 个场景覆盖了前台暂停 + 相关结果救援,但没有覆盖无救援的自动轮次错误)。

2. [Nit] tool-closure 尝试的状态事件文案是 Automatic continuation 2/2 started; 0 todo item(s) remain unfinished.Session.ts:3274)——准确但对判断"工作是否卡住"的消费者有歧义,建议 closure 专用文案。

对已有发现的判断

  • bridge 的 fire-and-forget release(bridge.ts:4378,作者 should-fix + bot suggestion):同意;并且我的发现 1 是同一"自动工作被静默卡住"故障族的无传输失败版本——如果在那里加子进程侧自检兜底,建议把两者一起覆盖。
  • bot 的 [Critical](mid-turn drain 丢弃 Stop hook 的阻塞决定/systemMessage):核实为真,但我评级为 should-fix 而非阻塞——仅在 guard 启用已 armed 时发生,被丢弃的调用不计入 cap(hook 下次自然停止会重新触发,可观察的问题是副作用型 hook 的双重调用加被吞掉的 systemMessage)。作者提出的"drain 前先发 systemMessage"是廉价的 90% 修复。
  • 认同共同的可维护性意见:beforeSend 闭包改写六个外部变量,refresh-mismatch 分支的正确性依赖 selectedMessage 在过滤后重读被重新赋值的 nextMessage——今天是对的(我逐路径核实过),但这是未来改动最可能踩坏的位置。+1 后续 PR 提取 stop-ordering 编排。

质量 / 安全 / 测试

  • 安全面扎实:guard 遥测不含 Todo 文本(单测 + E2E 均有断言),args/回放/磁盘/MCP 影子工具无法 arm,safe/bare/plan 多层强制关闭,未触碰权限路径——断连客户端依然无法批准任何东西。
  • 测试覆盖对这类协调代码而言非常出色——75 个 Session 场景与设计文档条款几乎一一对应,E2E 回放测试在无 SSE 订阅者的情况下钉住了 [1, 2, 2] 尝试序列(与我验证运行中的实测一致)。
  • 性能:armed 自然停止最多三次 drain 往返(bot 的观点)属实但对 opt-in 有界功能可接受;默认路径零开销。
  • 规范:kebab-case 模块、同址测试、ESM、无 any、schema 再生成同步——全部干净。

doudouOUC added a commit to doudouOUC/qwen-code that referenced this pull request Jul 15, 2026
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>

@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 thread packages/cli/src/acp-integration/session/daemon-todo-stop-guard.ts
Comment thread packages/cli/src/acp-integration/session/daemon-todo-stop-guard.ts
Comment thread packages/acp-bridge/src/bridge.ts
Comment thread packages/cli/src/acp-integration/session/Session.ts
Comment thread packages/cli/src/acp-integration/acpAgent.ts
Comment thread packages/cli/src/acp-integration/session/daemon-todo-stop-guard.ts
@wenshao

wenshao commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

✅ Re-verification at new head bdbc6267b (macOS)

This re-runs the full local build + real-test verification at the current PR head. My earlier build/test verification and code review were pinned to the first commit 1982e29a0. Two commits have landed since:

  • c05f2e3d8codex: address PR review feedback
  • bdbc6267bfix(cli): Preserve Stop hook output on mid-turn input

Those are exactly the commits that answer the bot's [Critical] (mid-turn drain dropping the Stop hook's decision/systemMessage) and the fire-and-forget release finding from the thread, so the whole point of this pass is to confirm they land green and that the newest fix is real. Everything is green, and the fix is load-bearing. This run is on macOS (my first pass was Linux).

Results — all green at bdbc6267b

Verification matrix

Area Command Result
Bundle npm run bundlenode dist/cli.js --version ✅ boots · 0.19.10
Guard state machine vitest daemon-todo-stop-guard.test.ts 18 passed
Session + ACP agent vitest Session.test.ts acpAgent.test.ts (+guard) 607 passed
ACP bridge suite vitest run (14 files) 824 passed
Daemon streaming E2E vitest --root integration-tests qwen-serve-streaming (QWEN_SANDBOX=false, bundled dist/cli.js) 4 passed
Format prettier --check (19 changed files) ✅ all match
Lint eslint --max-warnings 0 (15 changed .ts) ✅ 0 warn / 0 err
Settings schema generate:settings-schemagit diff ✅ no drift
Types — acp-bridge tsc --noEmit ✅ 0 errors
Types — cli (PR files) tsc --noEmit, siblings → source ✅ 0 errors in touched files

1431 unit tests (607 CLI + 824 bridge) + 4 bundled-daemon E2E, all green. Every command ran against bdbc6267b in an isolated worktree.

The newest commit is load-bearing (controlled experiment)

bdbc6267b adds one focused regression test, accounts for a slow blocking Stop hook after handling mid-turn input. To prove the fix — not just the test — is real, I reverted only the ~20-line Session.ts change back to its parent (c05f2e3d8) while keeping the new test:

Controlled experiment

  • Fixed head: the test passes.
  • Fix reverted, test kept: the test fails with expected 4 to be 1 — the Stop hook fires instead of once, because the pre-fix continue re-runs the whole stop loop instead of preserving the hook's single invocation (its systemMessage and cap accounting are lost). The full daemon Todo Stop Guard describe goes 75 passed / 1 failed — exactly one scenario flips, so the regression is surgical.
  • Restored: green again, tree clean.

Live bundled daemon at the new head

The streaming E2E spawns the real bundled dist/cli.js serve against a fake model, arms the guard, admits a prompt with no SSE subscriber, then reconnects and replays. Observed at bdbc6267b:

Live guard demo

  • Exactly 4 marker-bearing model calls (1 original + 2 bounded guard attempts + 1 closure).
  • Attempt sequence [1, 2, 2], three replayable todo_stop_guard status events, exhaustion status emitted.
  • Todo text never leaks into any guard event; turn_complete replays correctly after reconnect.

This matches my first-commit run exactly — the review-feedback refactor + mid-turn fix did not regress the bounded-continuation loop, the exhaustion status, the no-leak guarantee, or disconnected-client replay.

What the two new commits changed (and how it behaves)

  • Session.ts — a mid-turn queued prompt arriving while a Stop hook is running now falls through to process the hook's output, so its systemMessage and blocking-cap accounting survive; guardDecision is suppressed and the turn ends after the hook is honored. Continuation events are also gated on commitContinuation succeeding (no emit for an attempt that never commits).
  • bridge.ts — the fire-and-forget queued-prompt release now logs failures to stderr instead of swallowing them (the thread finding).

Notes for merge / environment honesty

  • Two workspace-wide build steps (npm run build, full npm run typecheck) fail in my symlinked worktree, but only on pre-existing, PR-unrelated debt: a stale @qwen-code/channel-base .d.ts for the (untouched) dingtalk package, a ChannelMemoryCallbacks mismatch in non-PR files, and a @lydell/node-pty declaration artifact. None touch the guard; a clean npm ci (as in my Linux pass) never hits them. The bundle (esbuild-from-source), the acp-bridge typecheck, and a source-mapped cli typecheck of the PR files are all clean.
  • Opt-in posture unchanged: default: false, requiresRestart: true, showInDialog: false; forced off in safe / bare / Approval-plan modes.
  • Out of scope (unchanged): Windows (the streaming suite skips non-POSIX) and process-crash recovery (intentionally unsupported per the design doc).

LGTM at bdbc6267b from a build/test standpoint — the review-feedback commits are correct, tested, and load-bearing.

中文说明

✅ 在新 head bdbc6267b 上的复验(macOS)

本次在 当前 PR head 上重跑了完整的本地构建 + 真实测试验证。我之前的构建/测试验证代码审查都钉在首个提交 1982e29a0。此后新增了两个提交:

  • c05f2e3d8处理 PR review 反馈
  • bdbc6267bfix(cli): 在 mid-turn 输入下保留 Stop hook 输出

这两个提交正是回应 bot 的 [Critical](mid-turn drain 丢弃 Stop hook 的决定/systemMessage)以及线程里 fire-and-forget release 的发现,所以本次复验的重点就是确认它们通过、且最新的修复确实起作用。全部通过,且该修复是「load-bearing」的。本次运行在 macOS(我第一次是 Linux)。

结果 —— 在 bdbc6267b 上全绿

(见上方验证矩阵截图)

项目 命令 结果
打包 npm run bundlenode dist/cli.js --version ✅ 可启动 · 0.19.10
Guard 状态机 vitest daemon-todo-stop-guard.test.ts 18 通过
Session + ACP agent vitest Session.test.ts acpAgent.test.ts(含 guard) 607 通过
ACP bridge 套件 vitest run(14 文件) 824 通过
Daemon streaming E2E vitest --root integration-tests qwen-serve-streamingQWEN_SANDBOX=false,bundle 版 dist/cli.js 4 通过
格式 prettier --check(19 个改动文件) ✅ 全部符合
Lint eslint --max-warnings 0(15 个改动 .ts ✅ 0 warn / 0 err
Settings schema generate:settings-schemagit diff ✅ 无漂移
类型 — acp-bridge tsc --noEmit ✅ 0 报错
类型 — cli(PR 文件) tsc --noEmit,sibling → 源码 ✅ 改动文件 0 报错

1431 个单测(607 CLI + 824 bridge)+ 4 个 bundle daemon E2E 全绿。所有命令都在隔离 worktree 中针对 bdbc6267b 运行。

最新提交是 load-bearing 的(对照实验)

bdbc6267b 新增了一个聚焦回归测试 accounts for a slow blocking Stop hook after handling mid-turn input。为证明「修复本身」而非仅仅测试是真实的,我Session.ts 的约 20 行改动回退到其父提交(c05f2e3d8),保留新测试:

(见上方对照实验截图)

  • 修复态: 测试通过。
  • 回退修复、保留测试: 测试报 expected 4 to be 1 —— Stop hook 触发了 4 次而非 1 次,因为修复前的 continue 会重跑整个 stop 循环,而不是保留 hook 的单次调用(其 systemMessage 与 cap 计数被丢弃)。整个 daemon Todo Stop Guard describe 变为 75 通过 / 1 失败 —— 只有 1 个场景翻红,说明回归是外科手术式的、精准的。
  • 恢复后: 重新全绿,工作树干净。

新 head 上的真实 bundle daemon

streaming E2E 会拉起真实 bundle 的 dist/cli.js serve 连到 fake 模型,开启 guard,在没有 SSE 订阅者时接纳一个 prompt,然后重连并回放。在 bdbc6267b 实测:

(见上方 live guard demo 截图)

  • 恰好 4 次带 marker 的模型调用(1 次原始 + 2 次有界 guard 尝试 + 1 次 closure)。
  • attempt 序列 [1, 2, 2],3 个可回放 todo_stop_guard 状态事件,发出耗尽状态。
  • guard 事件中从不泄露 Todo 文本;重连后 turn_complete 正确回放。

这与我第一次(首提交)的实测完全一致 —— review 反馈引入的重构 + mid-turn 修复没有破坏有界 continuation 循环、耗尽状态、无泄露保证或断线客户端回放。

两个新提交改了什么(以及行为)

  • Session.ts —— 当 mid-turn 排队 prompt 在 Stop hook 运行期间 到达时,现在会继续向下处理 hook 的输出,使其 systemMessage 和阻塞 cap 计数得以保留;guardDecision 被抑制,hook 被处理后结束该 turn。continuation 事件也改为仅在 commitContinuation 成功时才发(未 commit 的尝试不发事件)。
  • bridge.ts —— fire-and-forget 的排队 prompt release 失败现在会写入 stderr 而不是静默吞掉(线程里的发现)。

合并参考 / 环境说明(如实)

  • 两个全仓构建步骤(npm run build、完整 npm run typecheck)在我的 symlink worktree 中失败,但只因与本 PR 无关的既有欠账:(未改动的)dingtalk 包引用了过期的 @qwen-code/channel-base .d.ts、非 PR 文件里的 ChannelMemoryCallbacks 不匹配,以及 @lydell/node-pty 声明缺失。它们都不涉及 guard;干净的 npm ci(如我 Linux 那次)不会触发。bundle(esbuild 直接编译源码)、acp-bridge typecheck,以及对 PR 文件做 source-mapped 的 cli typecheck 均干净。
  • opt-in 姿态不变:default: falserequiresRestart: trueshowInDialog: false;在 safe / bare / Approval-plan 模式下强制关闭。
  • 未覆盖(不变):Windows(streaming 套件在非 POSIX 上跳过)与进程崩溃恢复(按设计明确不支持)。

从构建/测试角度,在 bdbc6267bLGTM —— review 反馈的两个提交正确、有测试覆盖、且是 load-bearing 的。


🤖 Built and tested locally with Claude Code (Opus 4.8, 1M context) on macOS (arm64). Every command above was run against PR head bdbc6267b; screenshots are captured from those runs.

wenshao
wenshao previously approved these changes Jul 16, 2026
doudouOUC added a commit to doudouOUC/qwen-code that referenced this pull request Jul 16, 2026
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean rebase, CI green, all prior review findings addressed.

This is a re-triage after the branch was rebased onto latest main (the /resolve bot run failed, and the author resolved the conflict manually). The only new commit since the previous triage at 03d4ffe6ca is fe64ed2a58 — a pure merge-conflict resolution touching only markdown column widths in the experimental settings table. No PR source logic changed.

The previous two passes by independent reviewers covered the full implementation in depth: the DaemonTodoStopGuard state machine, the Session coordination matrix (75+ test scenarios), the bridge protocol extension, the abort-path gap closures, and the notification overflow tightening. All Critical findings were resolved with targeted regression tests. The CI at the new head is fully green (ubuntu test suite 24m50s, web-shell E2E smoke, precheck).

Two non-blocking items from the previous review remain open for follow-up work (an automatic-turn liveness suggestion and a status-event wording nit) — appropriate to defer after 7+ review rounds.

Approving. ✅

中文说明

置信度:4/5 — 干净的 rebase,CI 全绿,所有先前审查发现已解决。

这是在分支 rebase 到最新 main 后的重新审查(/resolve bot 运行失败,作者手动解决了冲突)。自上次在 03d4ffe6ca 的审查以来,唯一的新提交是 fe64ed2a58——纯合并冲突解决,仅调整 markdown 列宽。PR 源码逻辑未变。

此前两位独立审查者已深入覆盖完整实现。所有 Critical 发现已通过针对性回归测试解决。新 head 上的 CI 全绿。

两条非阻塞建议留给后续工作处理。

批准 ✅

Qwen Code · qwen3.7-max

Reviewed at fe64ed2a58de9a8fc772c47e4d72b80aff8d2768 · 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. ✅

@wenshao

wenshao commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /resolve

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

Qwen Code attempted to resolve merge conflicts but the run did not complete successfully.

Check the workflow run for full logs.

doudouOUC and others added 6 commits July 18, 2026 07:42
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@doudouOUC
doudouOUC dismissed stale reviews from qwen-code-ci-bot and wenshao via fe64ed2 July 17, 2026 23:50
@doudouOUC
doudouOUC force-pushed the agent/daemon-todo-stop-guard branch from 03d4ffe to fe64ed2 Compare July 17, 2026 23:50
@wenshao

wenshao commented Jul 18, 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. ✅

@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. Suggestions are inline. 2 Suggestion-level finding(s) could not be anchored to the diff; see the terminal output. Not reviewed: coverage — no plan was given, so this run cannot show that any of the diff was read.

— qwen3.7-max via Qwen Code /review

Comment thread packages/cli/src/acp-integration/session/Session.ts
@doudouOUC
doudouOUC added this pull request to the merge queue Jul 18, 2026
Merged via the queue into QwenLM:main with commit b4559dc Jul 18, 2026
60 checks passed
@doudouOUC
doudouOUC deleted the agent/daemon-todo-stop-guard branch July 18, 2026 01:53

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

Not reviewed: Verification agent — no generated brief exists.

Not reviewed: Verification agent — no generated brief exists.

— Codex GPT-5 via Qwen Code /review

Comment thread packages/cli/src/acp-integration/session/Session.ts
Comment thread packages/acp-bridge/src/bridge.ts
Comment thread packages/cli/src/acp-integration/acpAgent.ts
Comment thread packages/cli/src/acp-integration/session/Session.ts
Comment thread packages/cli/src/acp-integration/session/Session.ts
Comment thread packages/cli/src/acp-integration/session/Session.ts
Comment thread packages/cli/src/acp-integration/session/Session.ts
Comment thread packages/cli/src/acp-integration/session/Session.ts
Comment thread packages/cli/src/acp-integration/session/Session.ts
Comment thread packages/cli/src/acp-integration/session/Session.ts
@yiliang114

Copy link
Copy Markdown
Collaborator

PR Review: feat(cli): add daemon Todo Stop Guard

Overview

This PR adds an opt-in Todo Stop Guard feature for daemon/ACP sessions that automatically continues execution when a model stops naturally with unfinished top-level Todo items. The implementation is well-designed with appropriate safety boundaries.


✅ Strengths

1. Security & Safety Design

  • Feature is disabled by default and requires explicit experimental.todoStopGuard: true configuration
  • Properly forced off in safe mode, bare mode, and Approval plan mode
  • Bounded continuations: Maximum of 2 automatic continuations prevents runaway costs
  • Todo text is never exposed in guard telemetry - good privacy consideration
  • Only trusts TodoWriteTool (validated via constructor.name === 'TodoWriteTool') - discovered tools and MCP shadows are rejected

2. State Machine Implementation
The DaemonTodoStopGuard class (daemon-todo-stop-guard.ts) is well-structured:

  • Clear state transitions with explicit methods (suspend, pauseForTrustedRetry, clearTrust)
  • Proper handling of mid-turn user input (resets budget correctly)
  • FIFO prompt priority is correctly implemented via awaitQueuedPrompt() mechanism
  • Tool closure handling after Todo completion is thoughtful

3. Priority Handling
Correct ordering:

  1. Mid-turn user messages (resets budget)
  2. Complete FIFO prompts (Guard yields)
  3. External Stop hooks
  4. Guard evaluation (only when armed, no relevant background input)

This ensures user input always wins over automatic continuation.

4. Test Coverage

  • Comprehensive unit tests in daemon-todo-stop-guard.test.ts (254 lines)
  • Extensive integration tests in Session.test.ts (~4000+ lines of Guard-specific tests)
  • E2E test in qwen-serve-streaming.test.ts validates replay behavior
  • Bridge protocol coverage in bridge.test.ts and bridgeClient.test.ts

5. Documentation

  • Design doc (docs/design/daemon-todo-stop-guard.md) is thorough
  • User-facing docs in qwen-serve.md explain usage and tradeoffs

⚠️ Areas for Attention

1. Complex State Machine Edge Cases

The Guard state machine has many interacting states (#armed, #suspended, #retryPaused, #awaitingQueuedPrompt, #exhaustionReported). Consider adding a state diagram to the design doc for future maintainers.

Example of subtle interaction in decideToolClosure:

if (this.#unfinishedCount > 0) {
  return this.decide(hasRelevantBackgroundInput);
}

This means an unfinished count check happens before other guards - verify this ordering is intentional across all call sites.

2. Concurrency Considerations

The todoStopGuardQueuedPromptPriority flag in Session is set via:

entry.todoStopGuardAwaitingQueuedPrompt = hasQueuedPrompt;

in the bridge, then read via releaseTodoStopGuardQueuedPromptWait() in the session. The cross-process coordination appears sound, but worth verifying timing when:

  • Queue drain happens concurrently with prompt cancellation
  • Guard compression fails while external Stop hook is active

3. Background Baseline Tracking

The design relies on snapshotting background task IDs at work chain start:

this.#hasRelevantTodoStopGuardBackgroundInput()

Verify that background tasks created during a Guard continuation are correctly excluded from "relevant" status (they should start a new baseline after the Guard stream commits).

4. Token Limit Interaction

In Session.test.ts:

it('does not consume or revive the guard when token limits block the continuation stream', ...)

Good test, but verify that a token limit hit during a Guard stream (not before) correctly suspends without consuming the attempt.

5. Large Diff Size

The PR adds 6951 lines across 19 files, making thorough review challenging. The core Session changes (~1340 additions) touch critical paths. Consider if future features could be split into smaller PRs for easier review.


🔍 Specific Code Notes

daemon-todo-stop-guard.ts:63-66

get blocksUnrelatedAutomaticTurns(): boolean {
  return (
    this.enabled &&
    this.#armed &&
    !this.#suspended &&
    this.#unfinishedCount > 0
  );
}

This property name is clear, but verify all call sites check enabled first (the guard class does, but double-check Session usage).

Session.ts - Guard prompt building

#buildTodoStopGuardPrompt(state: TodoStopGuardContinuation): string {
  const prompt = `${TODO_STOP_GUARD_PROMPT_PREFIX}${state.unfinishedCount}${TODO_STOP_GUARD_PROMPT_BODY_SUFFIX}`;
  if (state.attempt < state.maxAttempts) return prompt;
  return prompt + TODO_STOP_GUARD_FINAL_PROMPT_SUFFIX;
}

Good that the prompt doesn't include Todo content - only the count. This prevents inadvertent Todo text exposure via logs/traces.

bridge.ts:4724-4748 - Prompt abort handling
The abort handler correctly releases the Guard when the last queued prompt is cancelled:

if (hasAnotherQueuedPrompt) return;
entry.todoStopGuardAwaitingQueuedPrompt = false;
void entry.connection.extMethod(TODO_STOP_GUARD_QUEUE_RELEASE_METHOD, ...)

Good error handling with writeStderrLine for failures.


📋 Verification Checklist

Based on the PR description's test plan, verify:

  • Feature-off behavior matches existing Stop hook semantics (tests confirm)
  • Guard never exposes Todo text (verified in tests)
  • Queued prompt preempts Guard continuation (tested)
  • Background results defer Guard without consuming attempt (tested)
  • Safe/bare/plan modes disable feature (tested via it.each)
  • Disconnected client replay works (E2E test confirms)

Summary

This is a well-implemented feature with appropriate safety boundaries, comprehensive test coverage, and good documentation. The state machine is complex but appears correct based on test coverage.

Recommendation: ✅ Approve with minor suggestions for documentation improvements (state diagram) and careful monitoring of edge cases in production.

The bounded nature of the feature (max 2 continuations) and opt-in configuration mitigate the main risk of unexpected model costs. The implementation correctly prioritizes user input over automatic continuation.

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

Labels

category/integration External integrations daemon scope/session-management Session state and persistence status/in-review This issue is currently in review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(serve): add bounded Todo continuation for daemon sessions

6 participants