fix(core): fire active-todo reminders at delegation boundaries and user turns - #10963
fix(core): fire active-todo reminders at delegation boundaries and user turns#10963yiliang114 wants to merge 3 commits into
Conversation
…er turns A session that delegates to foreground subagents earns one parent tool turn per tens of minutes of real work, so the 3-turn budget of the active-todo reminder never comes due and the persisted plan freezes (#10953). Any ordinary user turn additionally discards a registered reminder, so the very message asking about progress destroys the plan context. - Force the reminder due on a tool-result batch that carries a top-level Agent tool result, in both the TUI (client.ts) and ACP (Session.ts) paths, putting the nudge exactly where delegated progress arrives. - Continue the previous work chain on an ordinary user turn while a reminder is still registered — a registered reminder implies unfinished items, since todo_write deletes it on plan completion. This mirrors the existing retry continuation. The observational contract of todo_id is unchanged. Time-aware cadence (direction c) and the delayed tool-response recording (direction e) are separate concerns and intentionally not included. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-issue-patrol/jmtlu4sumgg
github-actions
Bot
added
the
review/self-reported
|
|
|
Thanks for the PR — this one is unusually well set up: the linked issue already carries the forensics, and the diff is small enough to hold in your head. Template ✓ — every section present, including the Risk & Scope bullets and the Chinese translation. Problem: observed, not theoretical. #10953 documents a plan frozen for 55m44s across four nodes, a reminder injected 0 times in a ~1.6 MB transcript (and 0 times in a second, unrelated ~7.5 MB session), with session/plan/call IDs and absolute UTC timestamps. Our own issue triage verified root causes 1–4 line-by-line against Direction: aligned. Plan-state observability during delegation is squarely inside the session-management mission, and it's the exact session shape the plan feature exists for. It touches no auth, sandbox, model-selection, telemetry, release, or public-contract surface, so nothing to escalate on direction. For what it's worth, the reference agent's CHANGELOG has no direct todo-reminder entry, but delegation-boundary state propagation is a heavily worked area there (foreground-subagent result streaming, backgrounded-turn tool handling, nested subagent transcript delivery) — the area is relevant even without a 1:1 match. Size: core paths are touched and the change is cross-package ( Approach: this matches what I'd have proposed independently before reading the diff — reuse the machinery that already exists rather than add any. Both halves land on existing primitives ( Cutting 80% wouldn't work here: (a) alone leaves the "asking about progress destroys the plan context" failure, and (b) alone leaves the reminder never coming due. Both are separately observed in the issue. Risk: Stage 1e matched a high-risk path — Moving on to code review. 🔍 中文说明感谢贡献！这个 PR 的前期准备非常充分：关联 issue 已经带了完整的取证信息，diff 也小到可以一眼看完。 模板 ✓ —— 各部分齐全，包括 Risk & Scope 三个要点和中文翻译。 问题： 已观测到的 bug，不是理论性加固。#10953 记录了 plan 冻结 55m44s、四个节点被一次性补刷，整份约 1.6 MB transcript 里提醒注入 0 次（另一份无关的约 7.5 MB 会话同样是 0 次），并给出了 session/plan/call ID 与绝对 UTC 时间戳。我们自己的 issue triage 已针对 方向： 对齐。委派期间的 plan 状态可观测性正落在 session-management 的核心使命内，而这恰恰是 plan 功能为之存在的会话形态。改动不涉及 auth、sandbox、模型选择、telemetry、发布或对外契约，因此方向上无需上升。补充一点：参考 agent 的 CHANGELOG 里没有直接对应的 todo 提醒条目，但"委派边界上的状态传递"在那边是被大量打磨的领域（前台子 agent 结果流式转发、后台化轮次的工具处理、嵌套子 agent transcript 投递）——即使不是 1:1 对应，这个方向也是相关的。 规模： 触及核心路径且跨包（ 方案： 与我在看 diff 之前独立想到的做法一致——复用已有机制，不新增任何东西。两处改动都落在既有原语上（ 砍掉 80% 在这里行不通：只有 (a) 会留下"询问进展反而销毁 plan 上下文"的故障，只有 (b) 会留下提醒永不到期的问题。两者在 issue 里都是各自独立观测到的。 风险： Stage 1e 命中高风险路径 —— 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewNo critical blockers. I checked every symbol this diff leans on rather than taking the diff's word for it, and the mechanism holds together. What I verified at this commit:
Three non-blocking notes, in descending order of how much I'd want a maintainer to look at them:
I also considered whether the 4-line predicate should be shared between the two packages instead of written twice, and concluded no: the two sites operate on genuinely different part types (ACP's The post-fix flow, since this bug is entirely about when things fire relative to turns: sequenceDiagram
participant P1 as User
participant P2 as Parent session (TUI or ACP)
participant P3 as Config reminder state
participant P4 as Foreground subagent
P1->>P2: todo_write registers 6 nodes
P2->>P3: setActiveTodoReminder (registered)
P2->>P4: Agent tool call carrying todo_id
Note over P4: tens of minutes of real work
P4-->>P2: Agent tool result
P2->>P3: takeActiveTodoReminder force=true (was budgeted, never came due)
P3-->>P2: reminder text
P2->>P1: spliced into next request, model updates plan
P1->>P2: user turn asking about progress
P2->>P3: getActiveTodoReminder(previous chain)
P3-->>P2: still registered, so plan is unfinished
P2->>P3: startActiveTodoWorkChain(new, continuedFrom previous)
Note over P2,P3: chain and reminder survive (were cleared)
TestingEvidence carried: the PR's own CI checks, read through the API. No PR code was built, run, or checked out — this is an unattended Nothing is red on this commit. The two gates that matter most for this diff — the Linux unit suite and lint/typecheck — were still in progress when I fetched, so I'm reporting them as pending rather than guessing. The SDK Java matrix and Worth naming explicitly: One housekeeping note, because it nearly produced a false finding: this runner is shared, and a concurrent review of a different PR (#10962, head Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。
Not verified: the end-to-end claim. You say it yourself under Risk & Scope — no live multi-hour delegation session was run, and the original defect was only observable in one. The unit tests pin the mechanism convincingly (force flag passed, reminder present in the outgoing message), but they do so against a mocked config, so they cannot show that a real foreground subagent returning actually causes the injection, nor that the plan sidecar consequently stops freezing. A green suite also can't tell us the tests would go red without the fix. Sandboxed verification would settle this, and since you have write access both lanes are open:
中文说明代码审查没有致命阻塞项。 我没有只看 diff 就采信，而是逐个核实了它依赖的符号，机制是自洽的。 在本 commit 上核实的内容：
三条非阻塞意见，按我希望维护者关注的程度递减：
我也考虑过这 4 行谓词是否该在两个包之间共享而不是写两遍，结论是不必：两处操作的 part 类型确实不同（ACP 的 修复后的流程如下，因为这个 bug 完全在于"何时"相对于轮次触发：（时序图见英文部分，此处不重复） 测试所携证据：通过 API 读取的本 PR 自身 CI 检查结果。没有构建、运行或检出任何 PR 代码——这是一次无人值守的 本 commit 上没有红色项。对这个 diff 最重要的两道闸——Linux 单元测试与 lint/typecheck——在我抓取时仍在进行中，因此我如实报告为 pending，而不是猜测结果。SDK Java 矩阵与 需要明确点出：本 PR 的 一条流程说明，因为它差点造成一个假发现：这台 runner 是共享的，一次针对另一个 PR（#10962，head （CI 表格见英文部分，机器可读区域由 finalize 任务在 CI 结束后就地更新。表中的 未验证：端到端结论。 你自己在 Risk & Scope 里也写了——没有跑过真实的数小时委派会话，而原始缺陷恰恰只能在那种会话里观测到。单元测试很有说服力地钉住了机制（传了 force 标志、提醒出现在发出的消息里），但它们是对 mock config 做的，因此无法证明真实的前台子 agent 返回确实触发了注入，也无法证明 plan sidecar 因此不再冻结。绿色的测试套件同样无法告诉我们：去掉修复后这些测试会不会变红。 沙箱验证可以解决这一点，而你有写权限，两条通道都开放：
— Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — the mechanism is verifiably correct and the scope is genuinely minimal; the only thing keeping it off 5 is that the end-to-end behavioural claim is still unsettled, not that I doubt the code. Stepping back: this is what a well-run bugfix looks like. You filed the issue with forensics rather than vibes, we verified the root cause against a named commit, you confirmed both delivery paths with red tests before writing the fix, and then the fix came in at 55 production lines that reuse machinery already in the tree. I wrote down what I'd have done before reading the diff — force the existing The checks I'd normally expect to fail on a change like this all came back clean. Coverage of the budgeted call sites is complete — there's no third frontend still starved, and no already-forced path got double-forced. The invariant change (b) depends on is actually enforced in What holds it at 4 rather than 5 is honest uncertainty about the thing the PR is for. The unit tests pin the mechanism against a mocked config; nothing yet shows a real foreground subagent returning and the plan sidecar consequently stopping its 55-minute freeze, and a green suite wouldn't tell us whether these tests go red without the fix. The Linux unit suite and lint were still running when I looked, and the macOS/Windows matrices are skipped on this PR against a Linux-only local run. That's a gap in evidence, not a defect I found — but I'd rather name it than paper over it, especially with My three notes from the review are all things I checked and concluded were not bugs: the reminder now outliving a pivoted-away plan is the tradeoff #10953 explicitly asked for and it's bounded; the background-launch force is harmless on a path that already forces; and the two adjacent So: approving, with the approval deferred until CI lands green on 中文说明Confidence: 4/5 —— 机制经核实是正确的，范围也确实最小；没能给到 5 分的唯一原因是端到端的行为结论仍未落定，而不是我对代码本身有疑虑。 退一步看：这是一次做得很规范的 bugfix。你在 issue 里给的是取证信息而不是感觉，我们针对具名 commit 核实了根因，你在写修复之前先用红测试确认了两条投递路径，然后修复只用了 55 行生产代码，且复用了树里已有的机制。我在看 diff 之前先写下了自己会怎么做——在 agent 结果边界强制使用既有的 这类改动上我通常预期会出问题的检查，全部干净通过。按预算消费的调用点覆盖是完整的——没有第三个前端仍被饿死，也没有已经强制注入的路径被重复强制。改动 (b) 依赖的不变量确实在 停在 4 分而不是 5 分，是对这个 PR 真正目的那份诚实的不确定。单元测试是对 mock config 钉住机制；目前还没有任何东西证明真实的前台子 agent 返回后 plan sidecar 就此停止它 55 分钟的冻结，而一套绿色的测试也无法告诉我们：去掉修复后这些测试会不会变红。我查看时 Linux 单元测试与 lint 仍在运行，而本 PR 的 macOS/Windows 矩阵被跳过，本地又只在 Linux 上跑过。这是证据上的缺口，不是我发现的缺陷——但我宁愿把它说出来，也不愿粉饰过去，尤其是改动路径里包含 我在审查中提的三条意见，都是经查证后判定不是 bug 的：提醒活过已被放弃的 plan，是 #10953 明确要求的权衡，且有界；后台启动触发的强制注入无害，因为那条路径本来就会强制注入；ACP session 里相邻的两个 结论：批准，但批准推迟到 CI 在 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| const continuedFrom = | ||
| this.activeTodoWorkChainPromptId !== undefined && | ||
| this.config.getActiveTodoReminder(this.activeTodoWorkChainPromptId) !== |
There was a problem hiding this comment.
[Suggestion] R1-1: Once a reminder is registered, no ordinary user turn can start a fresh work chain again: continuation here is keyed only on getActiveTodoReminder(...) !== undefined, and startActiveTodoWorkChain with continuedFrom never clears that owner's reminder. An abandoned (never-completed) plan therefore pins its stale chain for the rest of the session — the only escapes left are a later todo_write or a session reset. Concretely: the user registers a 5-item plan, then pivots ("never mind, review PR Y instead") and the model never calls todo_write again. Pre-PR the next UserQuery cleared all reminders; post-PR every subsequent user turn continues the dead chain, the ACP turn-start force take (Session.ts:5699) now finds the reminder (pre-PR it found nothing) and injects "Keep the todo list current and continue the task" into turns about unrelated work, and every Agent delegation boundary re-injects it — steering the model back to abandoned work indefinitely. The mirror guard in Session.ts (continuesTodoWorkChain) has the same shape. Consider giving the subsystem an explicit abandonment decision instead of the reminder-presence proxy — e.g. expire continuation after N user turns without a todo_write refresh — while keeping the immediate follow-up turn continuing (the #10953 behavior). The bound must keep the continuation the new tests pin (client.test.ts asserts the follow-up turn calls startActiveTodoWorkChain('prompt-user-followup', 'prompt-userQuery'); Session.test.ts asserts ('test-session-id########2', 'test-session-id########1')). Acceptance: a new test that registers a reminder, runs user turns past the bound with no todo_write between, and asserts startActiveTodoWorkChain is called with undefined — removing the expiry must make it red.
Witness:
[probe] PR arm: takeActiveTodoReminder(id, true) returned the reminder on 4/4
abandoned turns (p2..p5, no todo_write in between); pre-PR arm (no
continuedFrom): undefined. 2/2 probe tests passed.
中文说明
一旦提醒被注册，普通用户轮次就再也无法开启新的工作链：此处的续链仅以 getActiveTodoReminder(...) !== undefined 为条件，而带 continuedFrom 的 startActiveTodoWorkChain 从不清除该 owner 的提醒。因此被放弃（未完成）的计划会在会话余下时间里一直钉住其陈旧工作链——仅剩的退出方式是后续调用 todo_write 或重置会话。具体场景：用户注册了一个 5 项计划后转向其他任务（"算了，改看 PR Y"），模型不再调用 todo_write。修复前，下一个 UserQuery 会清空所有提醒；修复后，之后每个用户轮都延续这条死链，ACP 轮首强制取用（Session.ts:5699）现在会找到该提醒（修复前找不到）并向无关工作的轮次注入"保持 todo 列表最新并继续任务"，且每次 Agent 委派边界都会再次注入——无限期地把模型推回已放弃的工作。Session.ts 中的镜像守卫（continuesTodoWorkChain）同理。建议给子系统一个显式的"放弃"判定，而非以"提醒是否存在"作代理——例如在连续 N 个用户轮未调用 todo_write 后让续链过期——同时保留紧随其后的那一轮继续续链（即 #10953 要求的行为）。任何上界都必须保留新测试钉住的续链行为（client.test.ts 断言后续轮调用 startActiveTodoWorkChain('prompt-user-followup', 'prompt-userQuery')；Session.test.ts 断言 ('test-session-id########2', 'test-session-id########1')）。验收标准：新增测试——注册提醒后，在不调用 todo_write 的情况下推进用户轮超过上界，断言 startActiveTodoWorkChain 以 undefined 被调用；移除该过期机制后测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| const continuedFrom = | ||
| this.activeTodoWorkChainPromptId !== undefined && | ||
| this.config.getActiveTodoReminder(this.activeTodoWorkChainPromptId) !== | ||
| undefined |
There was a problem hiding this comment.
[Suggestion] R1-4: The cleared-reminder branch of this new guard has no test in either frontend, so deleting the getActiveTodoReminder(...) !== undefined conjunct (here and in Session.ts's continuesTodoWorkChain) ships green: each test gets a fresh client (top-level beforeEach), the existing 'carries active todos…' pin runs while activeTodoWorkChainPromptId is still undefined, the two new continuation tests only exercise the registered state, and Session.test.ts's 'clears active todo context when an ordinary prompt starts' uses retry: true for its second prompt, which rides continuesCurrentWorkChain, not this guard. The behaviour that would then ship: once any chain has started, every ordinary user turn keeps continuing it even after the plan completed (todo_write deleted the reminder), so chain owners never reset. Extend the new continuation tests with a third step: reset getActiveTodoReminder to return undefined (simulating plan completion), send one more ordinary UserQuery/prompt, and assert startActiveTodoWorkChain was last called with (newPromptId, undefined) — in both client.test.ts and Session.test.ts. Acceptance: that third-step assertion itself — with the conjunct removed the follow-up turn passes the previous chain id instead of undefined, so it must go red.
Witness:
[probe] MUTANT (conjunct removed from client.ts:3391 and Session.ts:5582):
all todo tests green — 7 passed (core) + 143 passed (cli); mutation survives
the whole shipped suite.
Discriminating probe on the MUTANT FAILED: expected last spy call to have been
called with ['prompt-after-clear', undefined] — received
['prompt-after-clear', 'prompt-user-followup']; INTACT code: passed (402/402).
中文说明
这个新守卫的"提醒已清除"分支在两个前端都没有测试，因此删除 getActiveTodoReminder(...) !== undefined 这一条件（此处及 Session.ts 的 continuesTodoWorkChain）后所有测试仍然绿色：每个测试都使用全新的 client（顶层 beforeEach），既有的 'carries active todos…' 断言运行时 activeTodoWorkChainPromptId 还是 undefined，两个新的续链测试只覆盖"提醒已注册"状态，而 Session.test.ts 的 'clears active todo context when an ordinary prompt starts' 第二个 prompt 是 retry: true，走的是 continuesCurrentWorkChain 而非此守卫。若按此发布，行为将是：任何链一旦开始，即便计划已完成（todo_write 已删除提醒），每个普通用户轮仍会继续续链，链 owner 永远不会重置。建议在两个前端的新续链测试中追加第三步：将 getActiveTodoReminder 重置为返回 undefined（模拟计划完成），再发送一个普通 UserQuery/prompt，断言 startActiveTodoWorkChain 最后一次以 (newPromptId, undefined) 被调用。验收标准：即该第三步断言本身——删除上述条件后，后续轮会传入上一个链 id 而非 undefined，断言必须变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| const activeTodoReminder = carriesAgentToolResult | ||
| ? this.config.takeActiveTodoReminder(prompt_id, true) | ||
| : this.config.takeActiveTodoReminder(prompt_id); |
There was a problem hiding this comment.
[Suggestion] R1-5: This new mid-turn use of force leaves its contract doc stale: takeActiveTodoReminder's docstring (packages/core/src/config/config.ts) still says "force is for turn-start injections (retry / related automatic turns)", while both frontends now pass force mid-turn at Agent-result boundaries (client.ts:3932 and Session.ts:7931). A maintainer refactoring per the documented contract could "restore" mid-turn takes to cadence on the doc's authority and regress #10953 precisely in the delegation-heavy sessions this PR targets. Extend the docstring with the third force case — e.g. "force is for turn-start injections (retry / related automatic turns) and for the mid-turn injection when a top-level Agent tool result returns — delegation advanced the plan while the parent earned only one tool turn, so the cadence cannot come due on its own." The forced take's counter reset (turns.set(owner, 0)) must stay documented alongside it.
Witness:
witness: not run — no execution capability settles a documentation-vs-code
contradiction; settled by direct comparison of committed text: docstring
"force is for turn-start injections (retry / related automatic turns)" vs
client.ts:3932 / Session.ts:7931 passing force mid-turn.
中文说明
force 的这一新用法（轮中注入）使其契约文档过时：takeActiveTodoReminder 的 docstring（packages/core/src/config/config.ts）仍写着 "force is for turn-start injections (retry / related automatic turns)"，而两个前端现在都在 Agent 结果边界于轮中传入 force（client.ts:3932 与 Session.ts:7931）。维护者若按文档契约重构，可能依据文档"恢复"轮中取用的节奏限制，从而在本 PR 针对的委派密集会话中使 #10953 回归。建议扩展 docstring，补充第三种 force 场景——例如："force 用于轮首注入（retry / 相关自动轮次），也用于顶层 Agent 工具结果返回时的轮中注入——委派推进了计划而父会话只产生了一个工具轮，节奏无法自然到期。"同时应一并保留对强制取用重置计数器（turns.set(owner, 0)）的说明。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| mockToolRegistry.getTool.mockReturnValue({ | ||
| name: 'agent', | ||
| kind: core.Kind.Execute, |
There was a problem hiding this comment.
[Suggestion] R1-3: This force test only exercises the canonical 'agent' name (both the registry mock and the streamed functionCalls), but Session.ts:7927 calls canonicalToolName() specifically to also catch the legacy task alias — and nothing on the ACP side witnesses that alias (the twin TUI test pins both names via it.each(['agent', 'task'])). Dropping canonicalToolName() at Session.ts:7927 and comparing the raw name leaves the entire ACP suite green, while tool-result batches whose functionResponse carries the legacy name task — still supported per ToolNamesMigration — silently stop forcing the reminder due, re-freezing exactly the delegation-heavy sessions of #10953 on the ACP frontend. Parameterize the test like the core one: it.each(['agent', 'task'])('forces the active todo reminder due when a %s tool result returns', ...), feeding the name into both mockToolRegistry.getTool.mockReturnValue({ name: ... }) and the streamed chunk. The alias the parameterized case must use is task: ToolNames.AGENT in ToolNamesMigration (packages/core/src/tools/tool-names.ts). Acceptance: the 'task' case asserting takeActiveTodoReminder is called with (promptId, true) — removing canonicalToolName at Session.ts:7927 must make it fail.
Witness:
[probe] MUTANT (canonicalToolName dropped at the Session.ts force path):
existing ACP force test still passed (1/1) — mutation survives.
MUTANT + it.each(['agent','task']) fix: 'agent' passed, 'task' FAILED —
AssertionError: expected [] to include '<system-reminder>unfinished todo: fol…'
INTACT code restored: both cases green (2/2).
中文说明
该强制注入测试只覆盖了规范名 'agent'（registry mock 与流式 functionCalls 都是），但 Session.ts:7927 特意调用 canonicalToolName() 以同时捕获旧名 task 别名——而 ACP 侧没有任何测试见证该别名（TUI 的对应测试通过 it.each(['agent', 'task']) 钉住了两个名字）。若在 Session.ts:7927 去掉 canonicalToolName() 改为直接比较原始名字，整个 ACP 测试套件仍然全绿，而 functionResponse 携带旧名 task 的工具结果批次（ToolNamesMigration 仍然支持该名字）将不再强制提醒到期，在 ACP 前端重新冻结 #10953 所针对的委派密集会话。建议像 core 一样参数化该测试：it.each(['agent', 'task'])('forces the active todo reminder due when a %s tool result returns', ...)，并将该名字同时传入 mockToolRegistry.getTool.mockReturnValue({ name: ... }) 与流式数据块。参数化用例必须使用的别名是 ToolNamesMigration（packages/core/src/tools/tool-names.ts）中的 task: ToolNames.AGENT。验收标准：'task' 用例断言 takeActiveTodoReminder 以 (promptId, true) 被调用——移除 Session.ts:7927 的 canonicalToolName 后该断言必须失败。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| // continue that chain instead of discarding its context with the | ||
| // very turn that may be asking about it (#10953). | ||
| const continuesTodoWorkChain = |
There was a problem hiding this comment.
[Suggestion] R1-5: This behaviour change leaves docs/design/active-todo-context.md — the committed design spec for this exact mechanism — describing the old semantics: it says the reminder is cleared when "a new ordinary work chain starts", and its Verification section asserts "An ordinary new prompt clears stale state while retry/continue retains it". After this diff an ordinary prompt RETAINS the chain whenever a reminder is registered (pinned by this PR's own new tests), and the doc's cadence paragraph omits the new forced injection at delegation boundaries. The next engineer debugging reminder lifecycle reads that doc — committed under docs/design/ precisely to be the reference — and investigates the wrong direction, e.g. files this continuation as a leak because the doc says it must clear. Update the design doc: replace the clear-on-ordinary-prompt rule with "an ordinary prompt continues the chain while a reminder is registered (unfinished items) and starts a fresh one otherwise", add the delegation-boundary force to the injection/cadence paragraphs, and fix the Verification bullet to match.
Witness:
witness: not run — no execution capability settles a documentation-vs-code
contradiction; settled by direct comparison of committed text:
docs/design/active-todo-context.md "Clear it when all todos complete, a new
ordinary work chain starts, or the session changes" and "An ordinary new
prompt clears stale state while retry/continue retains it" vs the new tests
pinning continuation on ordinary prompts in both packages.
中文说明
此行为变更使 docs/design/active-todo-context.md——正是该机制的已提交设计文档——仍在描述旧语义：文档说提醒会在"新的普通工作链启动时"被清除，其 Verification 一节断言"普通新 prompt 清除陈旧状态，而 retry/continue 保留"。本 diff 之后，只要提醒已注册，普通 prompt 就会保留工作链（由本 PR 自己的新测试钉住），且文档的节奏段落未提及委派边界的新增强制注入。下一位调试提醒生命周期的工程师阅读该文档（它被提交在 docs/design/ 下，正是作为权威参考），会朝错误方向排查——例如按文档"应当清除"的说法把新的续链行为当作泄漏上报。建议更新设计文档：将"普通 prompt 清除"规则改为"普通 prompt 在提醒已注册（仍有未完成项）时延续工作链，否则开启新链"，在注入/节奏段落中补充委派边界强制注入，并同步修正 Verification 条目。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| const carriesAgentToolResult = toolRun.parts.some( | ||
| (part) => | ||
| canonicalToolName(part.functionResponse?.name ?? '') === | ||
| ToolNames.AGENT, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R1-8: On an ACP user turn with a registered reminder, the reminder is now injected twice into permanent history: the pre-existing turn-start force take (Session.ts:5698) resets the counter to 0 but does not delete the reminder, and this new agent-result force takes it again when the delegation returns — an identical second copy in the same turn. Pre-PR the second take was cadence-gated (elapsed = 1 < 3 → undefined), so such a turn stored one copy. This exceeds the PR's own Risk-section accounting ("at most one reminder injection per completed top-level delegation") and the cadence's documented purpose of bounding permanent history growth; core Retry/Cron/Notification/Teammate turns that delegate have the same double shape (client.ts:3842 + 3932). Note the new Session.test.ts force test already exhibits this: the first send receives the reminder via the turn-start force take, yet the test only asserts the last send contains it. Suppress the agent-result force when this turn already force-injected the reminder: capture whether the turn-start take returned a reminder and pass that into #buildNextMessageAfterToolRun so it only forces when it did not; mirror in client.ts for the Retry/Cron/Notification/Teammate turn-start branch. Dedup cannot key on the cadence counter: both the turn-start force take and setActiveTodoReminder reset turns to 0 (turns.set(owner, 0), ACTIVE_TODO_REMINDER_REFRESH_TURNS = 3, config.ts:292); and the agent-result force must still fire for reminders registered mid-turn via todo_write — the property pinned by this PR's own force tests. Acceptance: with the reminder registered at turn start, assert the reminder text appears exactly once across the user-prompt send and the tool-result send combined; removing the dedup condition makes it appear twice → red.
Witness:
[probe] Intact PR code: PROBE_R1_8 copies_per_send=[1,1] total=2 sends=2
Agent-result force reverted (pre-PR cadence):
PROBE_R1_8 copies_per_send=[1,0] total=1 sends=2
中文说明
在提醒已注册的 ACP 用户轮中，提醒现在会被注入两次进永久历史：既有的轮首强制取用（Session.ts:5698）把计数器重置为 0 但并不删除提醒，而新增的 Agent 结果强制取用在委派返回时再次取用——同一轮内向历史追加一份完全相同的副本。修复前，第二次取用受节奏限制（elapsed = 1 < 3 → undefined），因此这类轮只存一份。这超出了本 PR Risk 一节自己的核算（"每次完成的顶层委派至多一次提醒注入"），也超出了节奏机制文档中"约束永久历史增长"的设计目的；core 侧会委派的 Retry/Cron/Notification/Teammate 轮同样是双份形态（client.ts:3842 + 3932）。注意 Session.test.ts 的新增强制注入测试已经暴露了这一点：第一次发送经由轮首强制取用收到了提醒，而测试只断言最后一次发送包含它。建议：当本轮已经强制注入过提醒时，抑制 Agent 结果强制注入——记录轮首取用是否返回了提醒，并将其传入 #buildNextMessageAfterToolRun，仅在未注入过时强制；并在 client.ts 的 Retry/Cron/Notification/Teammate 轮首分支做镜像处理。去重不能以节奏计数器为键：轮首强制取用与 setActiveTodoReminder 都会把 turns 重置为 0（turns.set(owner, 0)，ACTIVE_TODO_REMINDER_REFRESH_TURNS = 3，config.ts:292）；且 Agent 结果强制注入必须对轮中经 todo_write 注册的提醒仍然生效——这是本 PR 自己的强制注入测试钉住的性质。验收标准：在轮首已注册提醒的情况下，断言提醒文本在"用户 prompt 发送"与"工具结果发送"合计仅出现一次；移除去重条件后出现两次 → 变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
…nges The doc still promised that an ordinary new prompt clears stale Todo state, and described the turn cadence as the reminder's only delivery path. This PR changes both: an ordinary turn continues the chain while a reminder is registered, and a tool-result batch carrying a top-level Agent result forces the reminder due because a delegated run earns the parent a single turn. Record the accepted cost (an abandoned plan resurfaces until a later write completes or clears it) and that the ACP todo-stop-guard lineage reset stays keyed to the retry/continue flag, so carrying a plan never widens guard trust.
|
Ran both rules end-to-end against real builds. Both behave as intended, so this can replace the Arms — one commit apart, so any difference is attributable to this change:
Both run as Oracle caveat, worth knowing before anyone re-tests this. The session transcript cannot observe these injections. The reminder is appended to the outgoing request after that turn's tool-result record has already been written, so it never lands in the transcript as its own part — every transcript match for the reminder string is a tool's own output echoing it. I measured from the raw request payloads ( Delegation boundary (headless;
The whole delta is one message: Before's final request has 6 messages ending at the Same run without User turn keeps an unfinished plan (interactive tmux, two user turns; both arms produced the identical sequence
The fresh one lands on turn 2's first tool result ( Independent corroboration that the After arm really delivered it: at the identical point in the headless scenario the model's own reasoning differs by arm. Before: "There's no actual task or question yet — just setup… A short acknowledgment is the right move." After: "The system reminder is pushing me to update todos, but the user's explicit instruction takes priority here." It can only object to a reminder it received. Not covered here: the ACP frontend half ( Also pushed Non-blocking: #10953 originally measured this from transcripts and got it wrong — it reported 0 injections, which that oracle cannot actually establish. Corrected there with the reason, so the invalid oracle doesn't get reused. 中文两条规则都在真实构建上端到端跑过了，行为符合预期，可以用来替换描述里的 两臂——只差一个 commit，所以任何差异都可归因于本改动：
两臂都以 口径提醒，别人复测前值得知道。 会话 transcript 观测不到这些注入。提醒是在该轮 tool-result 记录写盘之后才追加到出站请求上的，所以它根本不会作为独立 part 落进 transcript——transcript 里每一次命中该字符串，都是某个工具自己输出的回声。我改用原始请求体（ 委派边界（headless；
整个差异就是一条消息：Before 的最后一个请求有 6 条消息、止于 同样的流程不带 用户轮次保留未完成 plan（交互式 tmux，两轮用户输入；两臂产出完全相同的序列
真注入落在 turn 2 的第一个 tool result（ 另一条独立佐证，说明 After 臂确实送达了：在 headless 场景的同一位置上，模型自己的推理因臂而异。Before："There's no actual task or question yet — just setup… A short acknowledgment is the right move." After："The system reminder is pushing me to update todos, but the user's explicit instruction takes priority here." 它只能对自己收到的提醒表示异议。 **本次未覆盖：**ACP 前端那一半（ 另外往本分支推了 非阻塞：#10953 最初用 transcript 度量这件事，测错了——它报告注入 0 次，而那个口径根本无法证明这一点。已在那边更正并说明原因，避免这个无效口径被再次复用。 |
|
CI attribution: the What actually fails is the per-package loop inside The same log also carries npm's own Three things pin this to the registry rather than to this branch: 1. The advisories are not what turns the step red. The root audit reports 2. This branch changes no dependency input. Eight files, all docs / TS source / tests — no manifest, no lockfile. 3. The same sub-audit passed two minutes earlier on byte-identical input.
Same input, same window, different outcome — so this is a re-run, not a fix. If it keeps recurring across PRs, the durable fix belongs in the workflow itself (move to the bulk advisory endpoint, or retry on |
|
The The finding is the jsdiff DoS in
中文本分支上 命中的是
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
6 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-1 abandoned-plan chain never expires (continuation keyed only on reminder presence) — still stands, already reported (comment 3928922276)
- R1-3 ACP force test only exercises 'agent', not the legacy 'task' alias — still stands, already reported (comment 3928922296)
- R1-4 cleared-reminder branch of the continuation guard has no test in either frontend — still stands, already reported (comment 3928922285)
- R1-5 takeActiveTodoReminder 'force' docstring stale after the new mid-turn use — still stands, already reported (comment 3928922289)
- R1-8 reminder injected twice into permanent history on an ACP user turn with a registered reminder — still stands, already reported (comment 3928922307)
- R2-1 TUI/ACP continued-user-turn injection asymmetry (probe-confirmed) — dropped as overlap with the R1-1/R1-4 threads at packages/core/src/core/client.ts:3393-3394 (comments 3928922276, 3928922285); full finding in the findings artifact an…
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/core/src/core/client.ts:3924 (+3 locations) — [review] Both new reminder-policy rules are re-derived inline in both frontends instead of owned once in Config/core — one-sided future edits ship silent TUI/ACP divergencepackages/core/src/core/client.ts:3924 — [review] No logging at any of the four new reminder-decision sites while adjacent mechanisms in the same functions log their decisions
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 6 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查：反向审计——评审时间预算不足，未能开始第 1 轮。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.23.0)
4 participants
What this PR does
When a session delegates work to a foreground subagent, the parent earns one tool turn per tens of minutes of real work, so the 3-turn budget of the active-todo reminder never comes due and the persisted plan sidecar freezes while work advances. This PR makes two changes, both on the already-existing reminder machinery (
takeActiveTodoReminder/startActiveTodoWorkChain), in both frontends (TUIclient.tsand ACPSession.ts):todo_writedeletes the reminder when the plan completes; this mirrors the existing retry continuation.Why it's needed
Fixes the failure reported in #10953: the compensating reminder was injected 0 times across an entire delegation-heavy session, the plan froze for ~56 minutes, and the very user message asking about progress discarded the plan context. The reminder existed precisely to bound plan staleness; both of its delivery paths were structurally unreachable in delegation-heavy sessions.
Directions (c) time-aware cadence, (d) making
todo_idload-bearing, and (e) delayed tool-response recording are intentionally not included: (c) needs a growth bound of its own, (d) contradicts the observational contract indocs/design/ordinary-session-plan-execution.mdand needs a deliberate decision, (e) is a separate defect.Reviewer Test Plan
How to verify
Unit tests added on both frontends (red before the fix, green after):
forces the active todo reminder due when a agent/task tool result returns(core) andforces the active todo reminder due when an Agent tool result returns(cli): a ToolResult turn whose batch carries anagent/taskfunctionResponse must calltakeActiveTodoReminder(promptId, true)and inject the reminder, even though the ordinary budget is not filled.continues the todo work chain on a user turn while a reminder is registered(core + cli): with a registered reminder, the next ordinary user turn must callstartActiveTodoWorkChain(newPromptId, previousPromptId).keeps the turn budget for tool results without an Agent execution(non-agent results stay budgeted),clears active todo context when an ordinary prompt starts(no registered reminder → chain still cleared), retry continuation unchanged, plan-completion deletion unchanged (todoWrite.test.ts,config.test.ts).Typecheck passes for both packages (
tsc --noEmitinpackages/coreandpackages/cli).Evidence (Before & After)
N/A — no user-visible UI change; the reminder text and plan view rendering are unchanged, only when the reminder is delivered. The full-suite runs above are the evidence.
Tested on
Environment (optional)
Unit tests + typecheck only (
npm installin a worktree offorigin/main@661f41eef).Risk & Scope
todo_writereports all items completed (reminder deleted → next user turn starts fresh).todo_idbecoming load-bearing (direction d); delayed tool-response recording (direction e). The 5 failing tests inpackages/corefull suite (config.test.tscron/eager-registration,skill-curator,session-writer-lease,git-branches) fail identically on a pristineorigin/maincheckout in this environment and are unrelated.Linked Issues
Fixes #10953
中文说明
本 PR 做了什么
当会话把活委派给前台子 agent 时，父会话几十分钟才产生一个 tool turn，active-todo 提醒的 3-turn 预算永远凑不满，持久化的 plan sidecar 因此在工作推进时冻结。本 PR 在已有提醒机制（
takeActiveTodoReminder/startActiveTodoWorkChain）上做了两处修改，且同时覆盖两个前端（TUIclient.ts与 ACPSession.ts）：todo_write在 plan 完成时会删除提醒；这与既有的 Retry 续链行为一致。为什么需要
修复 #10953 报告的故障：整场委派密集的会话里补偿提醒注入 0 次，plan 冻结约 56 分钟，而且恰恰是询问进展的那条用户消息销毁了 plan 上下文。这个提醒本来就是为约束 plan 陈旧而存在的，但它的两条投递路径在委派密集的会话形态下结构性地不可达。
方向 (c) 时间感知节奏、(d) 让
todo_id承载状态、(e) 工具响应延迟记录均刻意不包含：(c) 需要单独的增长上界设计；(d) 与docs/design/ordinary-session-plan-execution.md的观测性契约冲突，需要明确决策；(e) 是独立缺陷。审阅者测试计划
如何验证
两个前端各新增单元测试（修复前红、修复后绿）：命令同上英文部分。核心断言：带
agent/taskfunctionResponse 的 ToolResult 轮必须以takeActiveTodoReminder(promptId, true)强制注入；有已注册提醒时，下一个普通用户轮必须调用startActiveTodoWorkChain(新promptId, 上一条promptId)。回归保护保持绿色：非 agent 结果仍按预算、无提醒时用户轮照旧清链、Retry 续链不变、plan 完成即删提醒（todoWrite.test.ts、config.test.ts）。两个包tsc --noEmit均通过。前后对比证据
N/A——无用户可见 UI 变化；提醒文本与 plan 视图渲染不变，只是投递时机改变，以上全量测试即为证据。
测试环境
环境（可选）
仅单元测试 + typecheck（在基于
origin/main@661f41eef的 worktree 中npm install）。风险与范围
todo_write报告全部完成时链仍会结束（提醒被删除→下一个用户轮重新开链）。todo_id承载状态（方向 d）；工具响应延迟记录（方向 e）。core 包全量测试中的 5 个失败（config.test.tscron/eager 注册、skill-curator、session-writer-lease、git-branches）在干净origin/main检出上于本环境同样失败，与本改动无关。关联 Issue
Fixes #10953