fix(core): preserve active Todo context across tool turns - #7919
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: this is an observed behavioral pattern, not a theoretical concern — models do lose todo salience after several tool turns, and the two merged predecessors (#6945, #7821) confirm this is a known reliability gap. The PR describes the problem clearly and the design doc lays out the reasoning well. Direction: aligned. Todo context persistence is a direct extension of the existing todo management feature, and the CHANGELOG shows sustained investment in this area (todo stop guard, todo panel, floating todos). This sits squarely within qwen-code's task-management mission. Size: core paths touched ( Approach: the prompt-ID isolation adds real complexity, but it's justified — the ACP/daemon context runs concurrent work chains (user prompts, cron, notifications), and without isolation a cron turn could clobber a user task's reminder. The every-3rd-turn cadence ( Risk: Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这是一个已观测到的行为模式，而非理论性担忧——模型在多次工具调用后确实会失去对 todo 的显著性关注，两个已合并的前置 PR（#6945、#7821）也确认了这是一个已知的可靠性缺口。PR 描述清晰，设计文档也很好地阐述了推理过程。 方向：对齐。Todo 上下文持久化是现有 todo 管理功能的直接扩展，CHANGELOG 显示该领域有持续投入（todo stop guard、todo panel、floating todos）。完全在 qwen-code 任务管理使命范围内。 规模：触及核心路径（ 方案：prompt-ID 隔离增加了真实复杂度，但这是合理的——ACP/daemon 上下文运行并发工作链（用户 prompt、cron、notification），没有隔离的话 cron 轮次可能覆盖用户任务的 reminder。每 3 轮注入一次的节奏（ 风险： 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: I would have stored the unfinished todo list as a single string on Config, appended it after function responses in both the core and ACP loops, and cleared it on new user prompts or completion. No prompt-ID isolation — just a simple field. Comparison with the PR: the PR's approach matches mine in the core mechanism (store → inject → clear) but adds three layers of justified complexity:
No critical blockers found. Specific observations:
Files changed (29 of 29 shown)
TestingThis is a CI run — no local real-scenario testing. CI on the reviewed commit
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 All completed checks passed. macOS/Windows Node tests and integration tests were skipped (not required for this PR path). Not verified: real-scenario TUI behavior (model-context change with no UI surface — 中文说明代码审查独立方案： 我会在 Config 上用一个简单字符串存储未完成 todo 列表，在 core 和 ACP 循环中把它追加到 function response 之后，并在新用户 prompt 或完成时清除。不做 prompt-ID 隔离。 与 PR 的比较： PR 的核心机制（存储→注入→清除）与我的方案一致，但增加了三层合理的复杂度：
未发现关键阻塞问题。注入顺序在两个循环中都正确， 测试CI 运行——无本地真实场景测试。被审查提交 未验证：真实场景 TUI 行为（模型上下文变更，无 UI 界面——不适用）。沙箱验证可以确认行为声明： — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 4/5 — clean review, well-justified design, CI fully green; only reservation is the inherent complexity of the acp-integration surface. Stepping back: this PR solves a real salience problem — models do lose track of pending todos after several tool turns, and the two merged predecessors confirm the team already considers this a reliability gap. The prompt-ID isolation looked like over-engineering at first glance, but the ACP/daemon context genuinely runs concurrent work chains, and without it a cron The code follows established patterns (Config's The design doc is a bonus that will help future maintainers understand why the isolation exists and what the cadence tradeoff is. Minor nits (non-blocking): the CI is fully green on 中文说明置信度：4/5 —— 审查干净，设计合理，CI 全部通过；唯一的保留是 acp-integration 表面的固有复杂度。 退后一步看：这个 PR 解决了一个真实的显著性问题——模型在多次工具轮次后确实会失去对 pending todo 的跟踪，两个已合并的前置 PR 确认团队已经将此视为可靠性缺口。prompt-ID 隔离乍看像是过度工程，但 ACP/daemon 上下文确实运行并发工作链，没有它 cron 的 代码遵循既有模式（Config 的 设计文档是帮助未来维护者理解隔离存在原因和节奏权衡的额外加分。 小问题（非阻塞）： CI 在 — Qwen Code · qwen3.8-max-preview Reviewed at |
e9143e9 to
28ddaeb
Compare
|
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）为单个提交。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] Session.worktree.test.ts: missing startActiveTodoWorkChain mock on mockConfig — TypeError when session.prompt() reaches the new code path. Same fix as Session.test.ts line 574.
[Critical] Session.review-lease.test.ts: missing startActiveTodoWorkChain mock on mockConfig — same pattern, causes TypeError in all 4 tests (RL1-RL4).
— qwen3.7-max via Qwen Code /review
|
Qwen Code review did not complete successfully. Run review failed. See workflow logs for details. A transient error is retried automatically; if you are seeing this, retry with |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
中文说明
已审查。 建议见行内评论。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。
— qwen3.7-max via Qwen Code /review
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Full review at 2d9f7f58dda2a64d258a2185ed44acc9853b710e with local verification (worktree checkout, typecheck, changed-file ESLint, and the focused test suites for config, todoWrite, client, coreToolScheduler, registries, nonInteractiveCli, and the ACP Session todo regressions — all passing). Approving.
Prior Criticals verified fixed at HEAD:
- The core cron/notification branch now uses
startAutomaticActiveTodoWorkChain(selective delete), so an automatic turn can no longer wipe the user's reminder viareminders.clear(). - Both
Session.worktree.test.tsandSession.review-lease.test.tscarry the six new config mocks; the suites pass locally. - The
currentPromptIdnon-reset in the headless loop is correct as the author argued: the teammate turn registersowners[teammateId] → userOwner, tool-result turns of that chain resolve through the mapping, and the chain only ends (in thefinally, when no tool calls remain) at exactly the point where the loop either exits or a new teammate turn refreshes the ID. There is no window where a stale ID reads the wrong key.
Independently checked beyond the existing threads:
- Ownership lifecycle:
endAutomaticActiveTodoWorkChain's reference check keeps the user-owned reminder alive across related automatic turns and releases isolated owners without leaks. The race between an ordinary prompt'sowners.clear()and an in-flight automatic turn'sfinallyis benign in both orderings (mutual no-op deletes). - Injection order: core appends after function responses (before microcompact sizing); ACP places the reminder between
toolRun.partsanddrained.parts, so mid-turn user input stays last.flatMapTextPartsguaranteesrequestToSendis an array, so the newsome/findIndex/splicecalls are safe on all message types. - The
hasToolCallshoist is behavior-preserving — its only new reader is the todo-chainfinally, and the existing prefetch check at the bottom of the try sees the same values as before. - Every added
todoWorkChainIdfield has paired write/read sites across CronJob, the three registries' notification metas,SendMessageOptions, and the ACP/headless queue items — no dead switches. Child Configs (Object.create) get isolated reminder maps, covered by tests. - CI is green on this commit (main unit suite, daemon E2E, web-shell E2E).
The two unresolved Suggestions (missing tests for the !promptId fallback and for batch-splitting by work chain) are worthwhile follow-ups but non-blocking per the project's review rules.
中文说明
在 2d9f7f58 上完成全面审查并本地验证（worktree 检出、typecheck、变更文件 ESLint、config/todoWrite/client/coreToolScheduler/registries/nonInteractiveCli/ACP Session todo 回归测试全部通过）。批准。
已确认此前 Critical 均在 HEAD 修复：core 的 cron/notification 分支改用选择性删除的 startAutomaticActiveTodoWorkChain；两个 Session 测试文件补齐了 mock；headless 循环不重置 currentPromptId 的做法经验证正确——teammate 轮建立 owner 映射，该链的 tool-result 轮通过映射解析，链结束时机与循环退出/新 teammate 轮刷新 ID 的时机吻合，不存在读错 key 的窗口。
在既有评论之外独立核查：所有权生命周期（引用计数保证用户 reminder 不被自动轮误删、清理竞态双向安全）；注入顺序（core 在 function response 后，ACP 在 mid-turn 用户输入之前）；hasToolCalls 提升不改变既有行为；所有新增 todoWorkChainId 字段写读点配对，无死开关；子 Config reminder 隔离有测试覆盖；该提交 CI 全绿。
两个未解决的 Suggestion（测试补充）值得跟进但不阻塞合并。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
中文说明
已审查——无阻断问题。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。
— qwen3.7-max via Qwen Code /review
Local runtime verification (maintainer review aid)I built this PR locally and drove the real CLI bundle against a mock OpenAI-compatible provider that records every outbound Setup — Result against the PR's own reviewer test plan
Same prompt, same mock, same tool sequence — the final model request before and after the PR.
Isolation: Finding 1 — the "user input last" invariant holds in ACP but not in the interactive CLIThe ACP path builds The interactive CLI reaches the same situation by a different route:
Both panels are real captures (ACP client vs. typing into the TUI during a 20 s shell call). This is the case the PR explicitly designed against — "the user's newest direction keeps precedence" — so it's worth closing before merge. The Retry/Cron/Notification branch a few lines above ( Finding 2 — every injected copy is retained, so the cost is linear in tool turns, not bounded by 4,000 charsThe 4,000-char cap bounds a single injection. Each injected reminder becomes a user message in history and is re-sent on every subsequent turn, and nothing dedupes or drops the older copies. With a maxed-out list (26 unfinished items):
After 7 tool turns the duplicated reminders are 29.5 KB — 37.9 % of the whole request payload, growing +4,218 chars per tool turn. A 40-tool-call task would carry ~170 KB of near-identical text. They are plain user messages, so the tool-result size-clear path in Related and cheaper to see: stale copies outlive their truth. In the completion scenario, after the final all- Neither of these is a regression (main sends no reminder at all), so I'd call both non-blocking, but dropping the previous copy when a new one is injected — or skipping injection when the list hasn't changed since the last one — would keep the win and remove most of the cost. Tests and mutation teeth
Not coveredRetry/continue through a real daemon client, teammate turns, session-change clearing, Windows/macOS. VerdictThe mechanism works as described and the isolation design earns its complexity — the cron-vs-background-agent contrast above is exactly the case a single global field would have got wrong. Finding 1 is a real gap against a stated invariant and is a small fix; finding 2 is a cost worth a follow-up. Neither blocks the behavior this PR is buying. 中文版本本地运行时验证（供合并参考）我在本地构建了该 PR，并用真实 CLI bundle 对接一个会记录每次 环境 —— 对照 PR 自带的 Reviewer 测试计划
问题 1 —— "用户输入最后"的不变式在 ACP 成立，在交互式 CLI 不成立ACP 路径构造的是 交互式 CLI 走的是另一条路： 两侧面板都是真实抓包（ACP 客户端 vs 在 TUI 里于 20 秒 shell 调用期间输入）。这恰好是 PR 明确想避免的场景（"用户最新指令保持最高优先级"），建议合并前修掉。上方几行的 Retry/Cron/Notification 分支（ 问题 2 —— 每次注入的副本都会留存，成本随工具轮次线性增长，而非被 4,000 字符封顶4,000 字符上限约束的是单次注入。每次注入的 reminder 都会成为历史里的一条 user 消息并在后续每一轮重发，且没有任何去重或淘汰。以打满的列表（26 项未完成）为例：7 个工具轮次之后，重复的 reminder 合计 29.5 KB，占整个请求负载的 37.9%，每个工具轮次增加 4,218 字符。一个包含 40 次工具调用的任务大约会携带 170 KB 近乎重复的文本。它们是普通 user 消息，因此 一个相关且更直观的现象：过期副本会比事实活得更久。在"全部完成"场景里，最后一次全 这两点都不是回归（main 根本不发 reminder），因此我认为都不阻塞合并；但在注入新副本时丢弃旧副本，或在列表未变化时跳过注入，可以保住收益并消除大部分成本。 测试与变异（mutation）验证
未覆盖真实 daemon 客户端上的 retry/continue、teammate 轮次、session 切换清理、Windows/macOS。撰写本评论时 结论机制与描述一致，prompt ID 隔离的复杂度是值得的 —— 上面 cron 与后台 agent 的对比正是单一全局字段会做错的场景。问题 1 是与既定不变式之间的真实缺口且修复成本很低；问题 2 是值得后续跟进的开销。两者都不阻塞本 PR 带来的收益。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.7-max via Qwen Code /review
|
@qwen-code /verify |
|
Sandboxed verification: merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. 沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 Scripted assertions: 33 passed · 0 failed · 33 total 脚本断言：33 通过 · 0 失败 · 33 总计 Verification report (report.md)Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
Review —
|
| todo list | per-turn | 40 tool turns, history | cumulative prompt reads |
|---|---|---|---|
| 6 items, typical (~700 chars) | ~180 tok | ~7 K tok | ~145 K tok |
| at the 4,000-char cap | ~1050 tok | ~42 K tok | ~860 K tok |
Two aggravating factors:
- Microcompaction won't reclaim it — it clears old tool results and media (
geminiChat.ts:1875), not user-role text parts. These reminders survive as dead weight and pull auto-compaction forward, which can evict real context. - Growth is quadratic in tool turns, because turn N re-reads all N-1 prior copies.
(To be fair: it does not break prompt caching — the reminder is always appended at the tail, so the cached prefix stays stable.)
Options, roughly in order of preference:
- Strip the previous copy from history before appending the new one — one live reminder at a time is exactly the salience you want.
- Re-inject only when the todo snapshot changed since the last injection, plus a periodic refresh every N turns.
- Shrink the payload:
- [in_progress] Run testslines instead of fullJSON.stringify, and drop the cap to ~800 chars.
Worth measuring on a real 30–50 tool-call task before merging, since the whole point of the feature is long multi-step runs — exactly the case where this costs the most.
🟡 The "new ordinary prompt clears stale todos" invariant is untested
I ran a mutation probe against the changed suites. 6 of 7 targeted mutations were killed — good tests. The survivor:
startActiveTodoWorkChain(promptId: string, continuedFrom?: string): void {
if (!continuedFrom) {
- reminders.clear();
owners.clear();→ config.test.ts stays green at 471/471.
That's one of the PR's headline safety properties (an abandoned task's todos must not bleed into an unrelated new prompt). The only Config-level coverage for clearing is via startNewSession; Session.test.ts and client.test.ts both mock Config, so neither can catch it. A three-line test on the real Config would close it:
config.startActiveTodoWorkChain('p1');
config.setActiveTodoReminder('p1', 'old work');
config.startActiveTodoWorkChain('p2');
expect(config.getActiveTodoReminder('p2')).toBeUndefined();For the record, the killed mutations were: removing the ToolResult injection in client.ts; removing the endAutomaticActiveTodoWorkChain cleanup; reordering the reminder after drained mid-turn input; dropping the scheduler's owner-mapping lookup; dropping the work-chain batch split in nonInteractiveCli; and including completed todos in the reminder.
🟡 The TUI wiring has no tests at all
useGeminiStream.ts gained the todoWorkChainId threading from all four registries plus the notification-batching split, and useGeminiStream.test.tsx is untouched — while the non-interactive and ACP paths both got tests. The batching split is user-visible: notifications from different work chains no longer merge into one turn, so a session with concurrent background work now issues more model turns than before. That deserves at least one test.
🟡 enterWith in Session.runToolCalls leaks into the turn loop
// Session.ts:6454
promptIdContext.enterWith(promptId);
todoWorkChainContext.enterWith(this.config.getActiveTodoWorkChainOwner(promptId));Unlike run(), enterWith mutates the current async context and persists after runToolCalls resolves — so anything registering a background task/monitor/shell later in the turn loop, outside a tool call, silently inherits the chain. The daemon executes tools directly (Session.ts:8078, not via CoreToolScheduler), so the binding is genuinely needed here, but wrapping the batch body in todoWorkChainContext.run(...) would scope it. If enterWith is deliberate, please mirror the explanatory comment the existing site at Session.ts:2652 carries — this one has none.
Nits
- Mixed part shapes.
client.tssplices/appends a barestring;Session.tsuses{ text: … }. Both work throughcreateUserContent, but it forces the dedupe check atclient.ts:2890-2897to test two shapes. Pick one. - Cron ordering diverges.
client.ts:2898-2911inserts the reminder at index 0 — before the session system reminders — whileSession.ts:5339-5346puts it after#buildInitialSystemReminders(). Harmless, but the two loops should agree. - No JSDoc on the new
ConfigAPI. Eight new public/private methods (config.ts:6043-6122) with genuinely subtle semantics: clear-all vs. selective clear, theowner === promptId ⇒ deletebranch instartAutomaticActiveTodoWorkChain, and the reverse-scan refcount inendAutomaticActiveTodoWorkChain. Neighbouring machinery in the same file (e.g.getFileReadCache'sObject.createnote) is documented in detail. Same for the newtodoWorkChainContextexport — comparesessionIdContext.ts, which documents itself by reference topromptIdContext. - Test placement. The four new Config tests land inside
describe('Model Switching and Config Updates'). They want their own describe block. todoWritekeys offpromptIdContext, not the owner the scheduler already resolved (todoWrite.ts:259). Equivalent for mapped prompt IDs; for unmapped ones (aSendMessageType.Goalturn, say) it writes an orphan entry under the raw prompt ID that nothing reads and only a laterstartActiveTodoWorkChainclears.currentPromptIdnever resets after a teammate turn (nonInteractiveCli.ts:1688-1690). Once a teammate message arrives, every subsequent turn — including plainToolResultturns of the original chain — reports under${prompt_id}/teammate/N. It's correct for todo ownership and necessary soendAutomaticActiveTodoWorkChainfires on the terminal turn, but it does shift telemetry and loop-detector attribution for the rest of the run. Worth a comment.- JSON truncation (
todoWrite.ts:266-269) cuts mid-array; the[truncated]marker helps, but a line-per-item rendering truncated at item boundaries would read better to the model.
What's good
escapeSystemReminderTagsbefore embedding model-authored todo text — correct injection defense, and the truncation happens after escaping so it can't synthesize a closing tag.- Child-
Configisolation via thehasOwnPropertyown-map trick matches the establishedgetFileReadCachepattern, and it's tested. CoreToolSchedulerbindingpromptIdContextaroundinvocation.execute(coreToolScheduler.ts:4224-4231) is a real drive-by fix: a tool resumed from a confirmation callback previously ran with no prompt ID in ALS. Good that it came with a test.- Ownership deliberately not inferred from Todo Stop Guard lineage, with a test that locks it.
todoStopGuardBackgroundBaselinenow always captured — I checked,CronScheduler's constructor is inert (no I/O, no timers), so this costs nothing even with cron enabled and the guard off.
Verification I ran
Worktree at PR head (df35e108), all green:
| suite | result |
|---|---|
todoWrite + monitorRegistry + backgroundShellRegistry + loop-wakeup |
165 passed |
config + client + client-goal + coreToolScheduler |
1113 passed |
nonInteractiveCli |
91 passed, 1 skipped |
background-tasks |
passed |
ACP Session |
460 passed |
Prettier clean on all changed files. The full ACP Session run finished in ~37 s here with no timeout, so the flake noted in the PR description didn't reproduce.
中文摘要
结论：设计方向对，合并前建议先处理历史膨胀问题。
按 work-chain owner 隔离 Todo reminder（而不是重读持久化的 todo 文件）是正确的做法，cron / notification / background task 在注册时捕获 owner、retry/continue 转移 owner、新的普通 prompt 清空——这套模型是站得住的。
🔴 主要问题：reminder 会永久累积进 history。 client.ts:2927-2929 和 Session.ts:4592-4597 在每一个 tool-result 轮次都追加 reminder，而 geminiChat.ts:2293 会把请求原样 push 进 history。4000 字符的上限只约束单份，不约束总量：40 次工具调用、按上限算约 42K token 沉淀在 history 里，累计读取约 860K token；typical 情况（6 条 todo）约 7K / 145K token。而且 microcompaction 只清理旧的 tool result 和媒体，不会回收这些 user-role 文本，反而会提前触发自动压缩、挤掉真正有用的上下文。增长是轮次的平方级。（好消息：reminder 一律追加在尾部，不会破坏 prompt cache。）建议：注入新的之前先从 history 里剥掉上一份；或只在 todo 快照变化时重注入 + 每 N 轮刷新一次；或把 payload 换成 - [status] content 行、上限降到 ~800 字符。建议在真实的 30–50 次工具调用任务上量一下再合。
🟡 变异测试：7 个变异杀掉 6 个，测试质量总体不错。 唯一存活的是删掉 startActiveTodoWorkChain 非续接分支里的 reminders.clear() —— config.test.ts 依然 471/471 全绿。这正是本 PR 的核心安全性质之一（放弃的任务不能把 todo 泄漏到新 prompt）；Session.test.ts / client.test.ts 都 mock 了 Config，抓不到。补一个三行的真实 Config 测试即可。
🟡 TUI 侧零测试。 useGeminiStream.ts 改了四个 registry 的 todoWorkChainId 透传和通知批处理切分，useGeminiStream.test.tsx 未动。批处理切分是用户可见行为变化：不同 work chain 的通知不再合并成一轮，会多产生模型轮次。
🟡 Session.ts:6454 的 enterWith 会泄漏到 turn loop。 daemon 直接执行工具（Session.ts:8078，不走 CoreToolScheduler），所以这里确实需要绑定，但 enterWith 会在 runToolCalls 返回后继续生效；建议改成 run() 包住批次，或至少补上像 Session.ts:2652 那样的说明注释。
其余为 nit：两条路径 part 形状不一致（裸 string vs {text}）、cron 分支注入位置两边不一致、新增 8 个 Config 方法零 JSDoc（语义并不显然）、新测试挂在 describe('Model Switching and Config Updates') 下、todoWrite 用 promptIdContext 而非 scheduler 已解析好的 owner、teammate 轮次后 currentPromptId 不复位、JSON 截断在数组中间。
做得好的地方： 嵌入模型产出的 todo 文本前先 escapeSystemReminderTags（且截断在转义之后，无法伪造闭合标签）；child Config 用 hasOwnProperty 隔离，与 getFileReadCache 既有模式一致且有测试；CoreToolScheduler 给 invocation.execute 绑定 promptIdContext 顺手修了确认回调恢复执行时 ALS 无 prompt id 的老洞；明确不从 Todo Stop Guard lineage 推断 ownership 并有测试锁住。todoStopGuardBackgroundBaseline 改成无条件捕获也没有代价——CronScheduler 构造函数是空的，无 I/O 无 timer。
已验证： 在 PR head 的 worktree 上跑了全部受影响用例（165 / 1113 / 91 / 460），全绿；prettier 全过；PR 描述里提到的 ACP Session 超时用例在本地约 37 秒跑完，未复现。
…y growth Every injected reminder copy lands permanently in chat history, so per-turn injection grew the live context linearly with tool turns. Tool-turn injection now re-issues the reminder only every third tool turn since the state was last presented; turn-start injections always fire and reset the cadence. The payload becomes a compact status/content line list capped at 800 characters. History stays append-only, so provider prefix caching is unaffected. Also: cover the new-ordinary-prompt-clears-stale-reminders invariant on the real Config, add TUI coverage for the work-chain notification batch split, cover todoWorkChainId continuation forwarding, and document the deliberate enterWith binding in the daemon tool runner.
|
@wenshao All four points addressed in 7c3aa85. 🔴 History accumulationI went with your options 2+3 combined rather than option 1 (stripping the previous copy), for two reasons:
Implementation: Against your table: a 40-tool-turn task with a typical 6-item list drops from ~40 copies × ~180 tok (~7K tok in history) to ~13 copies × ~60 tok (~0.8K tok), and cumulative prompt reads go from quadratic to near-linear. The microcompaction-can't-reclaim-it concern shrinks to noise at that size. History stays append-only, so prefix caching is untouched. 🟡 The untested clear invariantAdded exactly your three-line test on the real 🟡 TUI coverageAdded a 🟡
|
|
Addressed the remaining ordering gap from the runtime verification: active Todo reminders on ToolResult turns are now inserted after leading functionResponse parts and before drained mid-turn user input. Verified with a focused core client test. I did not expand this into broader history or API cleanup beyond the existing PR scope. |
Review: preserve active Todo context across tool turnsVerdict: LGTM with non-blocking follow-ups. The mechanism is sound, the ownership model holds up under the edge cases I traced, and I verified the feature works end-to-end against a real CLI build. Nothing here blocks merge. What I ran (worktree at
|
| Check | Result |
|---|---|
npm run typecheck (all workspaces) |
pass, 0 errors |
prettier --check + eslint on all 29 changed files |
clean |
packages/core: client, coreToolScheduler, config, todoWrite, background-tasks, monitorRegistry, backgroundShellRegistry, loop-wakeup, client-goal |
936 passed |
packages/cli: Session.test, Session.review-lease, Session.worktree, nonInteractiveCli, useGeminiStream |
731 passed |
The full Session.test.ts run passed all 460 tests here (30.5 s), including the three mid-turn drain cases — I did not reproduce the 30 s timeout mentioned in the PR body.
Non-vacuity spot check: flipping ACTIVE_TODO_REMINDER_REFRESH_TURNS from 3 to 1 fails re-issues the active Todo reminder only every third tool turn, so the cadence test is real.
End-to-end verification
Built dist/cli.js and ran headless (-p ... --approval-mode yolo) against a fake OpenAI SSE server with an isolated HOME, scripting the model to call todo_write (2 unfinished) then three more tool calls, logging every request body.
req 1 msgs=2 reminderMsgs=0
req 2 msgs=4 reminderMsgs=0 <- tool result for todo_write (counts as the presentation)
req 3 msgs=6 reminderMsgs=0
req 4 msgs=9 reminderMsgs=1 <- injected on the 3rd tool turn
req 5 msgs=11 reminderMsgs=1
Placement in req 4 is exactly as designed — the reminder lands as its own user message after the tool message, so the functionCall/functionResponse pairing stays intact:
6 assistant [tool_calls]
7 tool Command: echo step3 ... Exit Code: 0
8 user <system-reminder> The current task still has unfinished todo items:
- [in_progress] ALPHA-UNFINISHED-TASK
- [pending] BETA-PENDING-TASK ...
A second run where the model marks every todo completed at req 5 confirms the clear path: no further reminder is injected across reqs 5–7.
I also checked the design doc's prefix-caching claim directly — every request's message list is a strict prefix extension of the previous one (sharedPrefix === prev.length for all 6 transitions), so injection does not invalidate provider prefix caches.
Findings
1. (Important) History growth is linear and uncapped, and old copies go stale.
Every injected reminder is recorded permanently. The 1-in-3 cadence reduces the constant but the growth is still linear in tool turns, with no ceiling on total re-issues per work chain. Payload measured at 273 chars for two short todos; the cap allows ~1030. A 150-tool-turn autonomous run lands ~50 copies ≈ 15–50 KB ≈ 4–13k tokens.
Worse, the copies are not superseded: my second E2E shows reqs 5–7 still carrying a reminder that asserts items are unfinished after they were all completed. The todo_write result is the only counter-signal, and it sits further back in history each turn.
Since history must stay append-only for cache reasons, the cheap fixes are a hard cap on re-issues per work chain, or backing the cadence off (3, 6, 12, …) rather than holding it flat.
2. (Important) Notification batching now splits on todoWorkChainId — an extra model turn per work chain.
useGeminiStream.ts and nonInteractiveCli.ts both add todoWorkChainId equality to the batch-boundary loop. Two monitors launched under two different user prompts, firing while idle, previously drained as one turn and now drain as two. That is a correct consequence of the ownership model — batching them would force one owner on the whole turn — but it is a cost/latency change that lands on every user with more than one background task, and the PR's Risk section doesn't mention it. Worth a line there.
3. (Minor) GeminiClient.activeTodoWorkChainPromptId is never reset on session change.
Config.startNewSession clears the three maps, and activeAutomaticTodoWorkChainPromptIds is cleared on the next UserQuery, but activeTodoWorkChainPromptId (client.ts:330) survives. A Retry issued right after /clear calls startActiveTodoWorkChain(newId, <dead prompt id from the previous session>), so the chain's owner key is an id that no longer exists. Harmless today — prompt ids embed the session id so there's no collision, and the reminder map is empty anyway — but it's a landmine. Same for Session.activeTodoWorkChainPromptId in the daemon.
4. (Minor) PR body says the payload is capped at 4,000 characters; the code caps at 800.
MAX_ACTIVE_TODO_CONTEXT_CHARS = 800 (todoWrite.ts:25), which matches the design doc. Both the English and Chinese sections of the description say 4,000.
5. (Minor) Truncation runs after XML escaping.
escapeSystemReminderTags(...) then .slice(0, 800) can cut inside an entity (< → &l). No injection risk — escaping already neutralised the tag, and a partial entity can't re-open one — but slicing on a line boundary before escaping would be cleaner and would avoid dropping a todo mid-word.
6. (Minor) this.config.getActiveTodoWorkChainOwner?.(...) in coreToolScheduler.ts:4226.
Config declares the method non-optional, so the ?. plus the two-level ?? fallback is dead code for every real Config and exists only for partial test mocks. Either drop the ?. or narrow the scheduler's config type.
7. (Minor) Two Todo Stop Guard changes that the PR body says aren't happening.
The description states "The existing experimental Todo Stop Guard remains unchanged," but the constructor now captures todoStopGuardBackgroundBaseline unconditionally instead of only when the guard is enabled (Session.ts:1354), and ordinary prompts now call #resetTodoStopGuardBackgroundLineage() when the guard is disabled (Session.ts:2862). I believe both are right — otherwise relatedAgentIds never resets in the default configuration — but they deserve a sentence, since a reviewer scanning for "guard untouched" will trip on them.
Relatedly, the const continuesCurrentWorkChain = ... extractions in the cron and notification drains are pure churn: the value still has exactly one consumer, #prepareTodoStopGuardForAutomaticTurn.
8. (Nit / hardening) todoWorkChainContext.enterWith in Session.#runToolCalls.
The comment justifies enterWith over run well (post-resolve background registration), and the daemon rebinds on every tool batch, so the exposure window is narrow. But #executePrompt binds promptIdContext at turn start and not todoWorkChainContext, so between a turn's start and its first runToolCalls the store still holds the previous turn's owner. Any registration path that doesn't go through runToolCalls would capture it. Consider binding both at the same place.
Test coverage
Coverage of the state machine is genuinely good — the Config unit tests hit continuation, isolation, prototype-child isolation, session reset, and the terminal-release path, and client.test.ts covers ownership surviving an automatic turn's tool-result turns.
One gap, and it's the exact regression the ownership machinery exists to prevent: nothing tests that a todo_write executed inside an unrelated cron/notification turn leaves the foreground user chain's reminder alone. The Config tests drive setActiveTodoReminder directly, and the Session tests mock takeActiveTodoReminder wholesale, so the real chain — promptIdContext → TodoWriteTool → setActiveTodoReminder → owner resolution — is never exercised for an automatic turn. Worth one integration-level test.
Notes on things I checked and found fine
promptIdContext.runinCoreToolSchedulercloses a real gap: the TUI schedules tools outsidesubmitQuery's ALS frame, sopromptIdContext.getStore()inTodoWriteToolwould otherwise have beenundefinedthere. The approval-resume test confirms it also survives confirmation from an unrelated context.- The
/automatic/Nand/teammate/Nprompt-id suffixes innonInteractiveCliare safe for the OpenAI logger —sanitizeDiagnosticSuffixstrips/, andsessionIdFromPromptIdsplits on########before the suffix, so session grouping is preserved. endAutomaticActiveTodoWorkChain's owner scan correctly preserves the foreground reminder, because the ordinary chain always installs a self-mapping (owners.set(promptId, promptId)) that keeps the owner present inowners.values().- Subagent configs are
Object.create(parent), and thehasOwnPropertylazy-own-map pattern (already used forapprovalMode,memoryPressureMonitor,fileReadCache) correctly isolates them. todoWorkChainIdis not written to the background-shell status sidecar, so no stale cross-session ids come back from disk on that path.requestToSendis always a fresh array fromflatMapTextParts, so the in-placesplicecan't mutate a caller-owned list.
|
@ruoyu0214 please review this pr ！ |
qwen-code-review-bot
left a comment
qwen-code-review-bot
left a comment
There was a problem hiding this comment.
整体设计合理，work-chain ownership 隔离和 every-3rd-turn cadence 是正确的 tradeoff。几个观察：
设计层面
-
Config上用 3 个 Map +getOwn*()hasOwnProperty 守卫来处理Object.create(parent)子 Config 继承——能 work，但这个模式比较脆弱。如果后续有人直接赋值this.activeTodoReminders = ...而不是走 getter，就会静默破坏隔离。考虑过用一个单独的ActiveTodoState类封装这三个 Map 吗？这样 own-property 检查只需一处。 -
takeActiveTodoReminder的 cadence 逻辑：setActiveTodoReminder时 reset counter 到 0，然后每 3 次 tool turn 才 re-inject。如果模型在第 1 次 tool turn 就尝试 stop（此时 reminder 未注入），这个 case 完全依赖 todoStopGuard（默认关闭）。设计文档里说了不改 stop 语义，所以这是 by design，但值得在 PR description 里显式提一下这个 gap。
实现细节
-
Session.ts里promptIdContext.enterWith(promptId)+todoWorkChainContext.enterWith(...)的注释解释了为什么用enterWith而非run——合理，但这意味着 daemon 路径的 ALS 生命周期完全依赖下一次 rebind。如果runToolCalls之后有 async continuation 在下一个 turn 之前跑（比如 background task registration 的 delayed callback），它拿到的是当前 turn 的 context。这应该是期望行为，确认一下。 -
client.tsfinally 块里endAutomaticActiveTodoWorkChain的条件(!normalCompletion || !hasToolCalls)—— 如果 turn 正常完成但没有 tool calls，就清理 ownership。这意味着一个纯文本回复的 notification turn 不会把 reminder 带到后续。逻辑正确，但hasToolCalls的赋值分散在 3 个 return 点（steeredTurn / hookTurn / continueTurn），后续重构容易漏。 -
todoWrite.ts里 reminder 模板是硬编码的英文字符串。如果后续要做 i18n 或者 A/B test 不同 prompt 措辞，这里需要抽出来。当前阶段可以不动。
测试
覆盖充分，Core/ACP/nonInteractive/useGeminiStream 四层都有。config.test.ts 的 cadence 和 isolation 测试写得清楚。
总体 LGTM，上面 1 和 4 是可以后续跟进的 maintainability 建议，不 block。
|
已 review 并留言。总结： PR #7919 — 29 files, +1260/-53，在 给了 comment（非 approve/request-changes），主要建议：
不 block，整体 LGTM。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review
|
Closeout summary: no code was changed in this round. The remaining comments requested additional coverage for teammate inheritance and cron carry-back, but did not identify a current behavior failure. After the existing review rounds, repository policy limits further growth to Critical fixes, so these tests are deferred as follow-up. Current CI is passing. |
|
@qwen-code /resolve |
Re-verification at the current head
|
| file | changed lines | vs. reviewed head |
|---|---|---|
core/client.ts |
88 | identical |
config/config.ts |
125 | identical |
acp-integration/session/Session.ts |
116 | identical |
core/coreToolScheduler.ts |
43 | identical |
tools/todoWrite.ts |
24 | identical |
nonInteractiveCli.ts · useGeminiStream.ts |
19 · 18 | identical |
| 7 registry/scheduler/context files | 35 total | identical |
main's only overlap with this PR is config.ts, where it added an unrelated allowPrivateNetworkHooks accessor (+17 lines); the merge kept it and did not disturb the reminder maps. So what is below is the code I already reviewed at 62310b02 — no re-review of the logic needed, only the behaviour that was still unverified.
2. The runtime report's finding — "user input last" in the interactive loop — is fixed
That report found the ACP loop honouring the invariant while the interactive CLI did not: typing during a tool call produced function response → user's message → reminder, i.e. the reminder read last. 62310b02 moved the core-loop insertion to a splice before the first non-functionResponse part. I drove the real TUI in a pty (built dist/cli.js v0.21.1, mock provider recording every request), typed a message 4 s into an 18-second shell call, and captured the request it produced:
Order on the wire is tool → todo reminder → MIDTURN-USER-INPUT: …. The user's newest direction is last. Reverting the splice to the old append (requestToSend = [...requestToSend, activeTodoReminder]) fails one client.test.ts case, so the new test pins it.
3. The deferred coverage gap: an unrelated automatic turn cannot touch the foreground chain
This is the regression the whole ownership design exists to prevent, and — as noted in my last review — nothing tested it end to end: the Config tests drive setActiveTodoReminder directly and the Session tests mock takeActiveTodoReminder, so the real chain promptIdContext → TodoWriteTool → setActiveTodoReminder → owner resolution → injection was never exercised for an automatic turn.
One headless session, all real: the foreground task writes todos ALPHA, registers a cron job via cron_create (which captures no chain) and a wakeup via loop_wakeup (which captures the chain at registration), then ends. The cron turn fires first and writes its own todos BRAVO.
| turn | opens with | reads | writes |
|---|---|---|---|
| foreground | user prompt | — | ALPHA (injected on its 3rd tool turn) |
cron (cron_create) |
bare cron prompt, no ALPHA | its own chain only | BRAVO (own chain, injected on its own 3rd tool turn) |
loop wakeup (loop_wakeup) |
ALPHA, byte-identical (305 chars) to the foreground copy | inherited chain | — |
| next cron firing | bare cron prompt, no ALPHA, no BRAVO | — | — |
The wakeup turn is the oracle: it reads the foreground reminder after the cron turn wrote a different list, and gets ALPHA unchanged. BRAVO is released when its turn ends and never reappears. A 31-assertion probe against the compiled Config covers the rest of the lattice (retry transfer, teammate inheritance, two concurrent automatic turns on one chain, a chain id captured before a new user prompt, subagent Object.create isolation both ways, completion clearing) — 30 pass, 1 fails; that one is finding A below.
Teammate inheritance and cron carry-back, the two items deferred in the closeout, both behave correctly at the Config level; only their unit tests are missing.
4. Cadence and cost after the throttle
Measured, not inferred: a fresh reminder lands on model requests 4, 7, 10 of an 11-request run — every third tool-result turn, counting the todo_write's own result turn as the first. 305 bytes per copy for three short todos, ≤ 1.9 % of the payload across the run. Amortised per-turn cost is ~343 B against the pre-throttle ~4,218 B, so the throttle plus the 800-char cap is worth roughly 12×. A second run that marks every todo completed mid-way injects nothing further — the clear path works.
Two consequences worth stating rather than fixing:
- Short tails get no reminder at all. Requests 2 and 3 — the first two tool-result turns after the write — carry none, so a chain that ends within two tool turns of its last
todo_writeis never reminded. Thetodo_writeresult itself still carries the full list at that point, which is the design's rationale, but this is the same gap @ruoyu0214 raised and it belongs in the description. - Old copies are never superseded. After the all-
completedwrite, history still holds a copy asserting there are unfinished items. Harmless while it is one copy; it is the reason growth is linear rather than bounded.
Findings
A. (New, minor) The cadence-counter map is never released. endAutomaticActiveTodoWorkChain deletes the owner mapping and the reminder, but not the activeTodoReminderTurns entry (config.ts:6162-6169). Probe: after 500 isolated automatic chains that each wrote todos and were released, activeTodoReminders and activeTodoWorkChainOwners hold 1 entry each — activeTodoReminderTurns holds 501. It only accumulates for isolated automatic turns that call todo_write, and the next ordinary user prompt (or startNewSession) clears it, so it is bounded in interactive use; a long-lived daemon session driven only by cron/notification turns has no such boundary. One line in the release branch:
if (![...owners.values()].includes(owner)) {
this.getOwnActiveTodoReminders().delete(owner);
this.getOwnActiveTodoReminderTurns().delete(owner); // <-- add
}B. (Carried, description) The PR body still says 4,000 characters. MAX_ACTIVE_TODO_CONTEXT_CHARS = 800 (todoWrite.ts:25), and docs/design/active-todo-context.md says 800 — only the PR description (both the English and the 中文 Risk sections) still says 4,000. The description also never mentions the 1-in-3 cadence or the notification-batching split, both of which are the actual tradeoffs a reader of Risk & Scope wants — and Reviewer Test Plan step 1 is now wrong because of it: "performs at least one additional tool call. Confirm the next model request contains the unfinished Todo reminder" does not hold under the throttle, it takes three. The design doc is accurate and complete on all of this; only the body drifted.
C. (Carried, inert) activeTodoWorkChainPromptId is still not reset on session change in either loop (client.ts:330, Session.ts:1244). Probe case 8 pins why it does not matter today: a chain id captured before the boundary reads nothing, and releasing it leaves the live chain intact. Still a landmine for whoever changes the clear semantics.
Also unchanged, all cosmetic: truncation runs after XML escaping so a slice can cut inside an entity; the getActiveTodoWorkChainOwner?.() optional call in coreToolScheduler.ts:4226 is dead for every real Config; the two Todo Stop Guard changes the body says are not happening (Session.ts:1354, 2863) are correct but undocumented.
Suites and mutation teeth at e87fa929
| suite | result |
|---|---|
core: todoWrite, config, coreToolScheduler, background-tasks, monitorRegistry, backgroundShellRegistry, loop-wakeup |
1076 passed |
core: client · client-goal |
306 · 22 passed |
cli: nonInteractiveCli, Session, Session.review-lease, Session.worktree |
563 passed, 1 skipped |
cli: useGeminiStream |
168 passed |
prettier --check + eslint, all 29 changed files |
clean |
compiled-Config ownership probe |
30 / 31 (finding A) |
Correction to my first runtime report: I noted client.test.ts failing to collect locally (No "logStartSession" export) and said CI was the only authority for its new client-loop assertions. That was my environment — vitest 3.2.7 in a borrowed node_modules against the lockfile's 3.2.4. Under the pinned version the file passes 306/306, so those assertions do execute.
Mutation probes, each asserted to have actually landed before running — 7 killed, 0 survivors:
| mutation | killed by |
|---|---|
ToolResult reminder appended at the end instead of spliced before non-functionResponse parts |
client.test.ts (1 failure) |
| ACP reminder moved after the drained mid-turn input | Session.test.ts (1) |
reminders.clear() dropped from a fresh work chain |
config.test.ts (1) — the only survivor of my earlier mutation pass |
| cadence constant 3 → 1 | config.test.ts (1) |
automatic turn ignores the captured todoWorkChainId |
client.test.ts (1) |
todoWrite ignores promptIdContext |
todoWrite.test.ts (4) |
| TUI notification batching ignores work-chain boundaries | useGeminiStream.test.tsx (1) |
Setup and limits
Built from source in an isolated worktree at e87fa9295 (macOS, Node 22.23.1): tsc for channels / web-templates / core / acp-bridge, then esbuild + copy_bundle_assets; takeActiveTodoReminder confirmed present in dist/chunks/. Three drivers: headless -p … --approval-mode yolo, the interactive TUI over node-pty + @xterm/headless, and a stateless mock OpenAI provider that logs every /v1/chat/completions body. Two local-environment adaptations, neither affecting the PR's code: @octokit/rest marked external (my checkout predates packages/channels/github's dependency) and @testing-library restored from a sibling checkout.
Not covered: retry/continue through a real ACP client (probe-level only), teammate turns end to end, Windows/macOS-vs-Linux differences, and behaviour with a real model rather than a scripted one.
中文版本
在当前 head e87fa929 上重新验证
接续我在 2d9f7f58 上的 运行时验证 与在 62310b02 上的 上一轮评审。两者都固定在更早的提交上，因此本轮在 e87fa9295 重新构建，并把精力放在前两轮遗留的三件事上：合并 main 后我评审过的代码是否被改动、顺序修复在交互式循环里是否真正成立（运行时那轮唯一的实质问题），以及 ownership 机制在真实并发自动轮次下是否成立（作者作为 follow-up 推迟的那部分覆盖）。
结论：可以合并。 新增 1 个次要问题（一个无上界的 Map），PR 描述有 1 处过期表述。都不阻塞。
1. 合并 main 没有改动 PR 本身
e87fa929 是把 origin/main（26600896）合并进我上轮评审的 62310b02。对全部 14 个生产文件做 diff-of-diffs（PR 相对旧 base 的改动 vs 相对新 base 的改动）：client.ts 88 行、config.ts 125 行、Session.ts 116 行、coreToolScheduler.ts 43 行、todoWrite.ts 24 行、nonInteractiveCli.ts / useGeminiStream.ts 19 / 18 行、其余 7 个 registry/scheduler/context 文件共 35 行 —— 全部逐行相同。main 与本 PR 唯一重叠是 config.ts，它新增了无关的 allowPrivateNetworkHooks 访问器（+17 行），合并保留了它且没有触动 reminder 的三个 Map。所以下面的代码就是我在 62310b02 已经评审过的那份，逻辑无需重看，只需要补齐当时还没验证的行为。
2. 运行时那轮的问题（"用户输入最后"）在交互式循环里已修复
那轮发现 ACP 满足该不变式、交互式 CLI 不满足：工具执行期间输入会得到 function response → 用户消息 → reminder，reminder 反而最后被读到。62310b02 把 core 循环的插入改成 splice 到第一个非 functionResponse part 之前。我用 pty 驱动真实 TUI（源码构建的 dist/cli.js v0.21.1，mock provider 记录每个请求），在一个 18 秒 shell 调用开始 4 秒后输入一条消息，抓到的请求顺序是 tool → todo reminder → MIDTURN-USER-INPUT: …，用户最新指令在最后。把 splice 改回旧的 append 会让 client.test.ts 的一个用例失败，说明新测试锁住了这个顺序。
3. 被推迟的那块覆盖：无关自动轮次动不了前台工作链
这正是整套 ownership 设计要防的回归，而我上一轮评审已指出没有端到端测试：Config 测试直接调 setActiveTodoReminder，Session 测试整体 mock 了 takeActiveTodoReminder，因此 promptIdContext → TodoWriteTool → setActiveTodoReminder → owner 解析 → 注入 这条真实链路在自动轮次下从未被执行过。
一次全真实的 headless 会话：前台任务写入 todos ALPHA，用 cron_create 注册 cron（不捕获工作链）、用 loop_wakeup 注册唤醒（注册时捕获工作链），然后结束。cron 先触发，并写入自己的 todos BRAVO。
| 轮次 | 开头是什么 | 读到 | 写入 |
|---|---|---|---|
| 前台 | 用户 prompt | — | ALPHA（在其第 3 个工具轮注入） |
cron（cron_create） |
裸 cron prompt，没有 ALPHA | 只有自己的链 | BRAVO（自己的链，在其第 3 个工具轮注入） |
loop wakeup（loop_wakeup） |
ALPHA，与前台那份逐字节相同（305 字符） | 继承的链 | — |
| 下一次 cron 触发 | 裸 cron prompt，既无 ALPHA 也无 BRAVO | — | — |
wakeup 轮次就是判定器：它在 cron 轮次写过另一份列表之后读取前台 reminder，拿到的仍是原样的 ALPHA。BRAVO 在其轮次结束时释放，之后不再出现。另有一个 31 项断言的探针直接驱动编译后的 Config，覆盖 retry 转移、teammate 继承、同一条链上的两个并发自动轮次、在新用户 prompt 之前捕获的链 id、子 agent 的 Object.create 双向隔离、完成后清理等 —— 30 项通过，1 项失败，即下面的问题 A。
closeout 中推迟的 teammate 继承与 cron carry-back，在 Config 层面行为都正确，只是缺单测。
4. 节流后的注入节奏与成本
实测而非推算：11 个请求的运行中，新 reminder 出现在请求 4、7、10 —— 每 3 个 tool-result 轮次一次（把 todo_write 自己的结果轮算作第 1 轮）。3 条短 todo 时每份 305 字节，全程占负载 ≤ 1.9%。摊到每轮约 343 B，对比节流前的约 4,218 B，节流加 800 字符上限大约值 12×。另一次在中途把所有 todo 标记 completed 的运行不再注入任何内容，清理路径有效。
两个值得写清楚、但不必改的后果：
- 短尾任务完全拿不到 reminder。 请求 2、3（写入后的前两个 tool-result 轮次）都没有，因此在最后一次
todo_write之后两个工具轮内就结束的链，一次都不会被提醒。此时todo_write结果本身仍带着完整列表，这也是设计的理由；但这正是 @ruoyu0214 提的那点，应写进描述。 - 旧副本永远不会被更正。 全部标记
completed之后，history 里仍留有一份声称"还有未完成项"的副本。只有一份时无害，但这也是增长为线性而非有界的原因。
问题清单
A.（新增，次要）cadence 计数 Map 不会被释放。 endAutomaticActiveTodoWorkChain 删除了 owner 映射和 reminder，但没有删 activeTodoReminderTurns（config.ts:6162-6169）。探针：500 个各自写过 todo 并已释放的隔离自动链之后，activeTodoReminders 与 activeTodoWorkChainOwners 各剩 1 项，而 activeTodoReminderTurns 剩 501 项。它只在"调用了 todo_write 的隔离自动轮次"上累积，且下一次普通用户 prompt（或 startNewSession）会清空，所以交互式使用是有界的；但只跑 cron / notification 的长生命周期 daemon 会话没有这个边界。释放分支里加一行即可：
if (![...owners.values()].includes(owner)) {
this.getOwnActiveTodoReminders().delete(owner);
this.getOwnActiveTodoReminderTurns().delete(owner); // <-- 补这一行
}B.（延续，描述）PR 描述仍写着 4,000 字符。 代码是 MAX_ACTIVE_TODO_CONTEXT_CHARS = 800（todoWrite.ts:25），docs/design/active-todo-context.md 也写 800，只有 PR 描述（英文和中文的风险段）还是 4,000。描述同样没有提 1/3 注入节奏，也没提 notification 批处理拆分 —— 而这两点恰好是读"风险与范围"的人想看到的取舍；并且 Reviewer 测试计划第 1 步因此已经不成立："至少继续执行一次其他工具调用。确认下一次模型请求包含未完成 Todo reminder" 在节流下不成立，需要三次。设计文档这些都准确，只有描述没跟上。
C.（延续，当前无害）activeTodoWorkChainPromptId 在 session 切换时仍未复位（client.ts:330、Session.ts:1244）。探针第 8 项说明了为什么今天无害：在边界之前捕获的链 id 读不到任何东西，释放它也不会影响当前活跃链。但对后续改动清理语义的人来说仍是个坑。
其余问题保持不变，均为表面问题：截断发生在 XML 转义之后，可能切在实体中间；coreToolScheduler.ts:4226 的 getActiveTodoWorkChainOwner?.() 对任何真实 Config 都是死代码；描述声称"不动 Todo Stop Guard"，但 Session.ts:1354、2863 两处改动是对的、只是没写进描述。
e87fa929 上的测试与变异验证
| 套件 | 结果 |
|---|---|
core：todoWrite、config、coreToolScheduler、background-tasks、monitorRegistry、backgroundShellRegistry、loop-wakeup |
1076 通过 |
core：client · client-goal |
306 · 22 通过 |
cli：nonInteractiveCli、Session、Session.review-lease、Session.worktree |
563 通过，1 skipped |
cli：useGeminiStream |
168 通过 |
全部 29 个变更文件的 prettier --check + eslint |
干净 |
编译后 Config ownership 探针 |
30 / 31（问题 A） |
对首轮运行时报告的更正： 我当时提到 client.test.ts 在本地无法收集（No "logStartSession" export），并说那些 client 循环断言只能以 CI 为准。那是我的环境问题 —— 借用的 node_modules 里是 vitest 3.2.7，而 lockfile 锁定 3.2.4。用锁定版本运行，该文件 306/306 全部通过，那些断言确实执行了。
变异测试（每个变异都先断言"确实落地"再运行）—— 7 杀 0 存活：ToolResult 的 reminder 改为追加到末尾（client.test.ts 1 失败）；ACP 的 reminder 移到 drain 输入之后（Session.test.ts 1）；删掉新工作链里的 reminders.clear()（config.test.ts 1，这是我此前变异测试中唯一的存活者）；节奏常量 3 → 1（config.test.ts 1）；自动轮次忽略捕获的 todoWorkChainId（client.test.ts 1）；todoWrite 忽略 promptIdContext（todoWrite.test.ts 4）；TUI notification 批处理忽略工作链边界（useGeminiStream.test.tsx 1）。
环境与边界
在 e87fa9295 的隔离 worktree 中从源码构建（macOS，Node 22.23.1）：依次 tsc 构建 channels / web-templates / core / acp-bridge，再 esbuild + copy_bundle_assets；确认 dist/chunks/ 中存在 takeActiveTodoReminder。三条驱动路径：无头 -p … --approval-mode yolo、基于 node-pty + @xterm/headless 的交互式 TUI，以及一个记录每个 /v1/chat/completions 请求体的无状态 mock OpenAI provider。两处本地环境适配，都不涉及 PR 代码：把 @octokit/rest 标为 external（我的检出早于 packages/channels/github 引入该依赖），以及从相邻检出恢复 @testing-library。
未覆盖：通过真实 ACP 客户端的 retry/continue（仅探针层面）、teammate 轮次端到端、Windows 与 macOS/Linux 差异，以及真实模型（而非脚本化模型）下的行为。
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge resolution: PR #7919 ← origin/mainRoot causeMain merged the deferred-tool preload / ToolSearch-threshold feature Textual, not semanticThe two sides only touched adjacent lines; they modify no shared logic. The PR const MEMORY_CONTEXT_WARNING_RATIO = 0.15;
/** Re-inject the active Todo reminder every Nth tool turn, not every turn. */
const ACTIVE_TODO_REMINDER_REFRESH_TURNS = 3;
// Default `tools.toolSearch.threshold` (percent of the context window):
// mirrors the settings-schema default in packages/cli.
const DEFAULT_TOOL_SEARCH_THRESHOLD = 10;
What is load-bearingNothing order-sensitive: the two constants are independent, each consumed only What I could not verifyNo build/typecheck/tests were run (out of scope here). Both constants are 中文说明根因main 合入了延迟工具预加载 / ToolSearch 阈值功能（ 文本冲突，非语义冲突两侧仅改动相邻行，不修改任何共享逻辑。PR（保留活跃 Todo 上下文）与 main
关键依赖此处无顺序敏感点：两个常量相互独立，各自只被自己的功能使用，先后顺序无关紧要。 未能验证的部分未运行构建 / 类型检查 / 测试（本命令职责之外）。两个常量在合并后的代码中均被引用， |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 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.8-max-preview via Qwen Code /review
| todoWorkChainContext.enterWith( | ||
| this.config.getActiveTodoWorkChainOwner(promptId), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] No test verifies that this daemon-path todoWorkChainContext binding is visible during tool execution — deleting these lines would pass every Session test. Concrete cost: background tasks/shells/monitors registered during daemon tool execution capture todoWorkChainContext.getStore() at registration time (background-tasks.ts:681, backgroundShellRegistry.ts:314, monitorRegistry.ts:189); without the binding they capture undefined, their terminal notifications arrive without a todoWorkChainId, are classified as unrelated automatic turns, and the active Todo reminder is silently not injected for daemon-spawned background work. The existing coreToolScheduler.test.ts coverage exercises the TUI/core scheduler path (todoWorkChainContext.run(...)), not this daemon enterWith path. Suggested fix: in the Session mid-turn test (or a new one), read todoWorkChainContext.getStore() inside the tool's execute mock and assert it equals the expected owner.
中文说明
没有任何测试验证这个 daemon 路径的 todoWorkChainContext 绑定在工具执行期间可见——删掉这几行，所有 Session 测试仍会通过。具体代价：在 daemon 工具执行期间注册的后台任务/shell/monitor 会在注册时捕获 todoWorkChainContext.getStore()（background-tasks.ts:681、backgroundShellRegistry.ts:314、monitorRegistry.ts:189）；没有这个绑定，它们捕获到 undefined，其终止通知到达时不携带 todoWorkChainId，被归类为不相关的自动轮次，active Todo reminder 就不会注入 daemon 发起的后台工作，静默失效。coreToolScheduler.test.ts 现有覆盖只验证了 TUI/core scheduler 路径（todoWorkChainContext.run(...)），没有覆盖这个 daemon enterWith 路径。建议修复：在 Session 的 mid-turn 测试（或新测试）中，于工具的 execute mock 内读取 todoWorkChainContext.getStore()，并断言它等于预期的 owner。
— qwen3.8-max-preview via Qwen Code /review
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
Released in v0.21.2. |
6 participants
What this PR does
This change keeps the latest unfinished Todo list salient across tool turns. After a successful Todo update, Qwen Code retains a bounded prompt-scoped reminder and appends it after function responses in both the core and ACP loops. Mid-turn user input remains last so the user's newest direction keeps precedence.
Todo reminders are isolated by work-chain prompt ID, so cron jobs and background notifications cannot overwrite an ordinary user task. Retry, continue, and explicitly related automatic turns move the reminder to their new prompt ID, while session changes clear it. The existing experimental Todo Stop Guard remains unchanged and disabled by default.
Why it's needed
Today the complete Todo state is emphasized only in the immediate
todo_writeresult. After several other tool calls, that state loses salience and the model can naturally end the turn while Todo items are still pending. Re-reading the persisted Todo file is not safe because it can outlive the work chain that created it, and changing stop semantics would turn a planning aid into a completion oracle.This preserves the current task context before the stop decision without treating Todo state as proof that the task is complete or forcing an automatic retry.
Reviewer Test Plan
How to verify
Evidence (Before & After)
N/A — model-context behavior with no UI change. Focused regressions cover Todo persistence, prompt ownership, Core and ACP injection order, retry/continue transfer, and the Todo Stop Guard remaining off by default.
Tested on
Environment (optional)
Local Node.js 22 workspace.
npm run build,npm run typecheck, changed-file ESLint and Prettier, TodoWrite and Config tests, Core client and scheduler tests, non-interactive CLI tests, and focused ACP Session regressions all pass. A full ACP Session run reached 454 passing tests and one pre-existing 30-second mid-turn drain timeout; the affected ACP tests pass when run directly.Risk & Scope
todoStopGuard, and guaranteeing that every model will always follow the reminder.Linked Issues
Related to #6945 and #7821.
中文说明
这个 PR 做了什么
这个改动让最新的未完成 Todo 列表在连续工具轮次中保持显著。
todo_write成功后，Qwen Code 会保留一段有大小上限、按 prompt 隔离的 reminder，并在 Core 和 ACP 循环中把它追加到 function response 之后。轮次中途到达的用户输入仍然放在最后，因此用户最新指令保持最高优先级。Todo reminder 按工作链 prompt ID 隔离，因此 cron 任务和后台 notification 不会覆盖普通用户任务。retry、continue 和明确相关的自动轮次会把 reminder 转移到新的 prompt ID；session 切换会清理 reminder。现有实验性 Todo Stop Guard 不做修改，默认仍然关闭。
为什么需要
当前完整 Todo 状态只会在当次
todo_write结果中被重点提示。经过若干其他工具调用后，这段状态的显著性会降低，模型可能在仍有 pending Todo 时自然结束轮次。直接重新读取持久化 Todo 文件并不安全，因为文件可能比创建它的工作链存活更久；而修改 stop 语义则会把规划辅助误当成完成判定器。这个方案在模型做出停止决定之前持续保留当前任务上下文，但不会把 Todo 状态当作任务已经完成的证据，也不会强制自动重试。
Reviewer 测试计划
如何验证
证据（Before & After）
N/A —— 这是模型上下文行为变化，没有 UI 改动。聚焦回归覆盖 Todo 持久化、prompt ownership、Core 与 ACP 注入顺序、retry/continue 转移，以及 Todo Stop Guard 默认仍关闭。
测试平台
环境（可选）
本地 Node.js 22 workspace。
npm run build、npm run typecheck、变更文件 ESLint 与 Prettier、TodoWrite 与 Config 测试、Core client 与 scheduler 测试、non-interactive CLI 测试和 ACP Session 聚焦回归均通过。ACP Session 全量运行有 454 个测试通过，另有一个既有的 mid-turn drain 用例在 30 秒超时；本次影响到的 ACP 测试单独运行均通过。风险与范围
todoStopGuard，以及保证所有模型始终遵循 reminder。关联 Issue
与 #6945 和 #7821 相关。