Skip to content

feat(core): make todo_write opt-in - #10645

Merged
DragonnZhang merged 9 commits into
QwenLM:mainfrom
DragonnZhang:dragon/todo-write-opt-in
Sep 5, 2026
Merged

DragonnZhang merged 9 commits into
QwenLM:mainfrom
DragonnZhang:dragon/todo-write-opt-in

Conversation

@DragonnZhang

Copy link
Copy Markdown
Collaborator

What this PR does

This change removes the built-in todo_write tool from the default tool surface in every interaction mode. Users who still rely on Todo can opt in with tools.todoWrite.enabled: true and restart Qwen Code.

When Todo is disabled, the default system prompt and Agent tool schema no longer advertise Todo-specific behavior, and an attempted legacy call returns an actionable error naming the setting. Enabling the setting restores registration, prompt guidance, Todo-to-Agent association, persistence, hooks, ACP rendering, and daemon Stop Guard compatibility.

The built-in new-application workflow now tracks implementation steps without requiring Todo. Existing Todo history and sidecars remain readable; no data is deleted.

Why it's needed

In headless mode, Todo bookkeeping adds an entire model round while text output hides the call and JSON formats expose it only as a generic tool event. Stronger models can maintain and revise their approach without a mandatory checklist, while agent-team workflows already have incremental shared task objects. Making Todo opt-in reduces the default prompt and tool surface and avoids invisible latency without removing the capability.

Reviewer Test Plan

How to verify

  1. Start a headless run with isolated settings and no Todo setting. Verify the first model request does not declare todo_write, the stable base prompt does not mention it, and a forced legacy call explains how to enable it.
  2. Add "tools": { "todoWrite": { "enabled": true } }, restart, and repeat. Verify todo_write is declared, the Task Management guidance is present, and a Todo call succeeds and reaches the follow-up model turn.
  3. Inspect the Agent declaration in both configurations. Verify todo_id appears only when Todo is enabled.
  4. Enable the daemon Todo Stop Guard together with Todo and verify the existing Stop Guard behavior remains available.

Evidence (Before & After)

Before: Latest main declared todo_write in headless mode by default. A forced call required two model turns; text mode showed only the final answer, while JSON modes emitted generic tool events.

After: The deterministic bundled-CLI integration test omits the declaration and Todo guidance by default and returns an actionable opt-in error for a forced call. With the switch enabled, declaration, prompt guidance, execution, and follow-up delivery are restored.

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

Local macOS worktree on Node.js v24.18.0 and npm 11.16.0. Verification included the repository build, typecheck, lint, bundle, targeted unit tests, generated settings-schema consistency, and deterministic headless integration tests against a local fake OpenAI-compatible server.

Risk & Scope

  • Main risk or tradeoff: This intentionally changes the default tool surface for users and integrations that assumed Todo was always present.
  • Not validated / out of scope: Windows and Linux runtime checks; a live-provider rerun of the existing model-driven Todo integration scenario; replacing Todo with agent-team Task tools outside team mode.
  • Breaking changes / migration notes: Set tools.todoWrite.enabled to true and restart Qwen Code to preserve the previous behavior. Existing Todo data and historical records are retained.

Linked Issues

None.

中文说明

本 PR 做了什么

这个改动从所有交互模式的默认工具集合中移除了内置 todo_write 工具。仍然依赖 Todo 的用户可以设置 tools.todoWrite.enabled: true,并重启 Qwen Code 来显式开启它。

Todo 关闭时,默认系统提示词和 Agent 工具 schema 不再引导模型使用 Todo 专属行为;旧模型或历史上下文仍尝试调用时,会收到包含开关名称的可操作错误提示。开启设置后,工具注册、提示词指导、Todo 与 Agent 的关联、持久化、hooks、ACP 渲染以及 daemon Stop Guard 兼容行为都会恢复。

内置的新应用工作流现在会直接按实施步骤推进,不再强制依赖 Todo。已有的 Todo 历史和 sidecar 仍可读取,不会删除任何数据。

为什么需要这个改动

在 headless mode 下,Todo 记账会额外消耗一整个模型轮次,而文本输出会隐藏工具调用,JSON 格式也只会把它展示成通用工具事件。能力更强的模型可以在没有强制清单的情况下维护并调整实施思路,而 agent-team 工作流已经提供了可增量更新的共享任务对象。把 Todo 改成显式开启,可以缩小默认提示词和工具集合,消除用户看不到的额外延迟,同时保留原有能力。

Reviewer 测试计划

如何验证

  1. 使用隔离设置启动 headless 运行,不配置 Todo 开关。确认第一次模型请求没有声明 todo_write,稳定基础提示词不包含它,并且强制发出的旧式调用会说明如何开启该工具。
  2. 加入 "tools": { "todoWrite": { "enabled": true } },重启后重复验证。确认请求声明了 todo_write,提示词包含 Task Management 指导,Todo 调用成功,并进入携带工具结果的下一模型轮次。
  3. 分别检查两种配置下的 Agent 声明。确认只有 Todo 开启时才会出现 todo_id
  4. 同时开启 Todo 和 daemon Todo Stop Guard,确认原有 Stop Guard 行为仍然可用。

证据(改动前后)

改动前:最新 main 会在 headless mode 中默认声明 todo_write。一次强制调用需要两个模型轮次;文本模式只显示最终答案,JSON 模式则输出通用工具事件。

改动后:基于 bundle 的确定性 CLI 集成测试证明,默认配置不再声明工具或包含 Todo 指导,强制调用会返回可操作的开启提示;打开开关后,工具声明、提示词指导、成功执行和后续轮次传递都会恢复。

测试平台

OS 状态
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

环境(可选)

本地 macOS worktree,Node.js v24.18.0,npm 11.16.0。验证范围包括仓库 build、typecheck、lint、bundle、定向单元测试、生成设置 schema 一致性检查,以及针对本地假 OpenAI 兼容服务的确定性 headless 集成测试。

风险与范围

  • 主要风险或取舍:这会有意改变默认工具集合,影响此前假设 Todo 始终存在的用户和集成。
  • 未验证或不在范围内:Windows 和 Linux 运行时检查;使用真实模型服务重新运行现有的模型驱动 Todo 集成场景;在 team mode 之外用 agent-team Task 工具替换 Todo。
  • 破坏性变更或迁移说明:若需保留旧行为,请将 tools.todoWrite.enabled 设为 true 并重启 Qwen Code。已有 Todo 数据和历史记录会保留。

关联 Issue

无。

@DragonnZhang

Copy link
Copy Markdown
Collaborator Author

E2E Test Report

Baseline

On main at 3a0c4c6108, real headless runs declared todo_write by default. A forced Todo call consumed two model turns. Text mode showed only the final answer; JSON and stream-JSON exposed generic tool events rather than a dedicated Todo progress view. A successful call persisted its Todo sidecar.

Branch result

The bundled CLI was tested with isolated settings against a deterministic local fake OpenAI-compatible server:

  • Default configuration: the initial request omitted the todo_write declaration; the stable base system prompt and every other declared tool description contained no Todo guidance; a forced hallucinated call returned an actionable error naming tools.todoWrite.enabled; the model then received that result in its follow-up turn.
  • tools.todoWrite.enabled: true: the initial request declared todo_write; the stable base system prompt included # Task Management; the forced Todo call succeeded; the next request received the created item.

Result: 2 passed in 3.80 seconds.

Additional verification

  • npm run build — passed
  • npm run typecheck — passed
  • npm run lint — passed
  • npm run bundle — passed
  • Targeted core suites — 1,444 assertions passed across config, prompts, prompt examples, Agent, scheduler, and Arena coverage
  • CLI config suite — 369 assertions passed
  • npm run generate:settings-schema followed by git diff --check — passed

Not run

The existing live-provider, model-driven Todo integration scenario was updated to opt in but was not rerun as part of this deterministic check. Windows and Linux runtime checks were not performed locally.

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@DragonnZhang
DragonnZhang enabled auto-merge August 31, 2026 14:39
@qwen-code-ci-bot qwen-code-ci-bot added category/core Core engine and logic category/tools Tool integration and execution need-discussion scope/settings Settings and preferences type/enhancement Non-bug improvement or optimization labels Aug 31, 2026
@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Re-run — the direction question I stopped on last time is settled, so this pass does the full review. @wenshao and @qqqys both approved this exact head (b6f33cef), which is the human call I said the gate shouldn't make on its own. Everything below is against that head; the PR has grown considerably since the pass at 821eb0a1.

Template ✓ — all sections present, bilingual, concrete reviewer test plan, and the Risk section is honest about what wasn't validated.

Problem. Same split as last time, and it still holds. The mechanism cost is observed and checkable against the code: headless runs declare todo_write by default and every Todo update buys an extra model round that text output hides. What remains unevidenced is user demand — there is no linked issue, and "stronger models can maintain their approach without a mandatory checklist" is a reasonable hypothesis rather than an observation. That is a product judgment, and two maintainers have now made it. I'm not re-litigating it.

Direction. Settled by the approvals above. Worth recording for the changelog that this flips a default rather than adding an option: StickyTodoList / TodoDisplay in the interactive TUI simply never receive data once the tool is off, so the visible progress list disappears for everyone who hasn't set the flag. The PR says so in its Risk section, which is the right place to say it.

Size. Core paths are touched (packages/core/src/{config,core,tools,agents}, packages/cli/src/{config,acp-integration}) — cross-package. Of 1254 changed lines (911 + 343): 267 production logic across 9 files, 561 test, 284 snapshot + integration helper, 13 generated schema, 129 docs. feat-type and well under the 500-line maintainer-awareness threshold, and under the 1000-line large-PR advisory. No Stage 0 escalation on size.

Approach. This is the part I'd want a reviewer to look at, and it holds up. The flag follows the existing tools.listDirectory.enabled pattern rather than inventing one, and the default is fail-safe in the direction that matters: getCoreSystemPrompt takes the flag as a trailing parameter defaulted to false, and the Config field defaults to false, so a caller that forgets to pass it gets a prompt that omits Todo while registration is gated off by the same flag — the two can't disagree in the unsafe direction. I traced every production consumer of the prompt builder and all four funnel through the two gated entry points, so this isn't a switch nobody flips.

Two scope notes, neither blocking. The list_directory.test.ts churn (−74/+27) looks like drive-by refactoring from the title but isn't: it moves the fake-server forced-tool-call scaffolding into test-helper.ts so the new Todo test reuses it instead of copy-pasting ~50 lines, and the original consumer still passes green in CI. That's the right call. The other is that the diff widened into acp-integration/session/Session.ts — the daemon Todo Stop Guard has to AND with the new flag or it stays live over an unregistered tool — which is necessary, but it's what pulls this PR into elevated-risk territory (below).

Risk. Elevated-risk signal now matches, and did not last pass: packages/cli/src/acp-integration/session/Session.ts is on the revert-correlated path list (acp-integration). Per that signal this pass carries the full Stage 2 enrichment and conditions approval on real CI evidence — which is where I land in Stage 3, because the unit-test lane on this head ended cancelled rather than green.

Moving on to code review. 🔍

中文说明

重新运行——上次停下的方向问题已经有结论,所以这一轮做完整审查。@wenshao@qqqys 都在这个 head(b6f33cef)上批准了,而那正是我上次认为不该由 gate 自己决定的人工判断。以下全部针对该 head;相比 821eb0a1 那一轮,PR 已经大幅增长。

模板 ✓——各部分齐全,中英双语,有具体的验证计划,风险部分也如实说明了未验证的内容。

问题。 与上次相同的分野,而且依然成立。机制成本是可观测、可对照代码核实的:headless 运行默认声明 todo_write,每次 Todo 更新都会额外消耗一个模型轮次,而文本输出中用户看不到。仍未证实的是用户需求——没有关联 issue,"能力更强的模型可以在没有强制清单的情况下维护实施思路"是合理假设而非观测结论。这属于产品判断,而现在已有两位维护者做出了判断。我不再重复讨论。

方向。 已由上述批准确定。值得为 changelog 记录的一点是:这改变的是默认值,而不是新增一个选项——工具关闭后,交互式 TUI 中的 StickyTodoList / TodoDisplay 根本收不到数据,未设置开关的用户会看不到进度清单。PR 在风险部分说明了这一点,位置是合适的。

规模。 触及核心路径(packages/core/src/{config,core,tools,agents}packages/cli/src/{config,acp-integration}),跨 package。1254 行改动(911 + 343)中:生产逻辑 267 行,分布在 9 个文件;测试 561 行;快照与集成测试辅助 284 行;生成的 schema 13 行;文档 129 行。feat 类型,远低于 500 行的维护者关注阈值,也低于 1000 行的大 PR 建议阈值。Stage 0 不因规模升级。

方案。 这是我希望 reviewer 重点看的部分,而它站得住。开关沿用了既有的 tools.listDirectory.enabled 模式,而不是另造一套;默认值在关键方向上是安全的:getCoreSystemPrompt 以末尾参数接收该开关且默认为 falseConfig 字段同样默认 false,因此漏传的调用方只会得到"提示词不含 Todo",而注册也被同一个开关关闭——两者不会朝不安全的方向不一致。我追踪了提示词构造函数的每一个生产调用方,四条路径全部收敛到两个受控入口,所以这不是一个没人拨动的死开关。

两点范围说明,都不构成阻塞。list_directory.test.ts 的改动(−74/+27)从标题看像顺手重构,实际不是:它把 fake-server 强制工具调用的脚手架移到 test-helper.ts,让新的 Todo 测试复用而不是复制约 50 行代码,而原调用方在 CI 中仍然通过。这个处理是正确的。另一点是 diff 扩展到了 acp-integration/session/Session.ts——daemon 的 Todo Stop Guard 必须与新开关做与运算,否则会在未注册的工具上保持生效——这是必要的,但也正是它把本 PR 带入了较高风险区域(见下)。

风险。 本轮命中升级风险信号,上一轮没有:packages/cli/src/acp-integration/session/Session.ts 在与 revert 相关的路径列表中(acp-integration)。按该信号,本轮保留完整的 Stage 2 补充内容,并把批准建立在真实 CI 证据之上——这正是我在 Stage 3 的结论所在,因为该 head 上的单元测试 lane 最终是 cancelled,而不是 green。

进入代码审查 🔍

Qwen Code · qwen3.8-max-2026-09-02

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

@qqqys

qqqys commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

E2E test report (tmux, built at head 821eb0a130)

Code review of the full diff found no Critical issues (mechanism verified: CLI setting → Config.todoWriteEnabled → registration gate; prompt gating in both buildDefaultBasePrompt and the QWEN_WRITE_SYSTEM_MD dump path; getMainSessionBaseSystemPrompt forwards the flag via an optional-safe read; ArenaManager forwards it; AgentTool gates todo_id in schema + description; legacy call gets an actionable error with a workspace-toggle nuance branch). Two structural checks worth naming: the coreTools re-enable path is isLsToolEnabled()-specific, so an allowlist cannot sneak todo_write back in, matching the design doc; and the Todo Stop Guard is behaviorally inert when the tool is unregistered (it only arms on a successful todo_write in the current work chain), consistent with the updated "Requires tools.todoWrite.enabled" docs. The product-direction question (global default flip vs mode/model-scoped) the triage bot escalated is a maintainer call, not a review blocker.

Build: scratch tree from the exact PR head, npm ci (prepare green — the SDK bundle gate passed) + npm run bundle; gate code confirmed present in the bundle chunks.

TUI e2e (tmux, real model qwen3.8-max via API key, --approval-mode default, hermetic workspaces outside the repo):

Default session (no settings):

  1. QWEN_WRITE_SYSTEM_MD dump: 0 occurrences of todo_write and of # Task Management (the section and all guidance removed).
  2. /tools: 32 tools, no Todo tool listed.
  3. Forced attempt ("call todo_write now, nothing else"): the model could not even locate it — ToolSearch select:todo_write → "1 missing", keyword search → no matches — and correctly reported the tool absent instead of fabricating a call. So the disabled state holds at all three levels: eager registration, deferred/ToolSearch registry, and prompt advertising.

Enabled session (workspace .qwen/settings.json with tools.todoWrite.enabled: true):

  1. Dump now contains # Task Management (section + tool-guidance bullet), todo_write ×3, and the todo_id association guidance (+10 lines vs the disabled dump).
  2. /tools: 33 tools — exactly one addition, TodoList.
  3. Same forced call: ✓ TodoList Create todos succeeded, TUI rendered the todo row (○ probe).
  4. Persistence: ~/.qwen/todos/<sessionId>.json contains exactly {id: "1", content: "probe", status: "pending"} with the matching sessionId.

CI at check: Integration Tests (no-AK) green at this head — that lane runs the PR's new fake-server todo_write integration tests (default-exclusion + enabled-restores); the unit Test lane was still pending. No bot/maintainer approval on record (triage deferred to a maintainer for the direction call), so per review policy this is not approvable from my side regardless of CI.


中文说明

对完整 diff 做了代码审查,未发现 Critical 问题(机制核实:CLI 设置 → Config.todoWriteEnabled → 注册关卡;buildDefaultBasePromptQWEN_WRITE_SYSTEM_MD dump 两条提示词路径均受控;getMainSessionBaseSystemPrompt 以可选安全读取转发该标志;ArenaManager 已转发;AgentTool 的 schema 与描述均对 todo_id 做了门控;遗留调用会收到指明开关的可操作错误,且含 workspace toggle 的分支措辞)。两个结构性核查:coreTools 的重新启用路径是 isLsToolEnabled() 专属,因此白名单无法把 todo_write 偷偷带回来,与设计文档一致;Todo Stop Guard 在该工具未注册时行为上不会生效(只有当前工作链成功执行 todo_write 才会武装),与更新后的 "Requires tools.todoWrite.enabled" 文档一致。triage bot 升级的产品方向问题(全局默认翻转 vs 按模式/模型区分)属于维护者决策,不是审查阻断项。

构建: 从 PR 精确 head 展开的临时目录,npm ci(prepare 通过,SDK bundle 关卡为绿)+ npm run bundle;bundle chunks 中确认含新门控代码。

TUI e2e(tmux,真实模型 qwen3.8-max,API key 认证,--approval-mode default,仓库外的独立工作区):

默认会话(无设置):

  1. QWEN_WRITE_SYSTEM_MD dump:todo_write# Task Management 出现次数均为 0(章节与全部引导已移除)。
  2. /tools:32 个工具,没有 Todo 工具
  3. 强制尝试("立即调用 todo_write,不要用其他工具"):模型甚至找不到它——ToolSearch select:todo_write 返回 "1 missing",关键词搜索无匹配——并正确报告工具不存在而非伪造调用。即禁用状态在三个层面均成立:eager 注册、deferred/ToolSearch 注册、提示词宣传。

启用会话(工作区 .qwen/settings.jsontools.todoWrite.enabled: true):

  1. dump 中出现 # Task Management(章节 + 工具引导项)、todo_write ×3、todo_id 关联引导(比禁用 dump 多 10 行)。
  2. /tools:33 个工具——恰好新增 TodoList 一个。
  3. 同样的强制调用:✓ TodoList Create todos 成功,TUI 渲染出 todo 行(○ probe)。
  4. 持久化:~/.qwen/todos/<sessionId>.json 内容恰为 {id: "1", content: "probe", status: "pending"},sessionId 匹配。

CI(核查时): Integration Tests (no-AK) 在该 head 为绿——该 lane 会跑本 PR 新增的 fake-server todo_write 集成测试(默认排除 + 启用恢复);单元 Test lane 当时仍在运行。无 bot/维护者 approve 记录(triage 因方向问题转交维护者),按审查规则本侧不予 approve

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

Not explored to full depth (tool budget reached): "agent 4": none** — all planned checks completed within budget..

[Critical] R1-10: [fails-closed] [regression] This PR breaks 5 existing unit tests in packages/core/src/core/client.test.ts — the CI job 'Test (ubuntu-latest, Node 22.x)' is red on this PR because of it. getMainSessionBaseSystemPrompt now passes a 6th positional argument (config.isTodoWriteEnabled?.() ?? false) into getCoreSystemPrompt (packages/core/src/core/client.ts:383), but the five expect(getCoreSystemPrompt).toHaveBeenCalledWith(...) assertions in client.test.ts (around lines 14259, 14291, 14323 and the two following) still expect the old 5-argument call — client.test.ts is not updated in this diff. Failing tests: 'should append config appendSystemPrompt to the core system prompt', 'passes the active output style to the core system prompt', 'should pass interactive mode to the core system prompt', 'should pass acp mode to the core system prompt', 'should pass headless mode to the core system prompt'. Witness (run-produced): CI run 33403794522 job 99526325819 — 'Tests 8 failed | 22716 passed (22734)', every one of the five showing the assertion diff '+ false' as the extra 6th argument; and local reproduction in the PR worktree: npx vitest run src/core/client.test.ts -t "core system prompt" → 'Test Files 1 failed (1); Tests 5 failed | 385 skipped (390)' with the identical '+ false' diff. The base-side call has 5 arguments (merge-base client.ts), so these tests pass on the merge base — netNew by measurement. Fix: update the five assertions to include the new 6th argument (false with the current mocks, or stub isTodoWriteEnabled), ideally together with the R1-3 tightening (move isTodoWriteEnabled into the required Pick) so future callers cannot silently diverge.

中文说明

未探索到全部深度(达到工具调用预算):"agent 4"none** — all planned checks completed within budget.

[Critical] R1-10: [fails-closed] [regression] This PR breaks 5 existing unit tests in packages/core/src/core/client.test.ts — the CI job 'Test (ubuntu-latest, Node 22.x)' is red on this PR because of it. getMainSessionBaseSystemPrompt now passes a 6th positional argument (config.isTodoWriteEnabled?.() ?? false) into getCoreSystemPrompt (packages/core/src/core/client.ts:383), but the five expect(getCoreSystemPrompt).toHaveBeenCalledWith(...) assertions in client.test.ts (around lines 14259, 14291, 14323 and the two following) still expect the old 5-argument call — client.test.ts is not updated in this diff. Failing tests: 'should append config appendSystemPrompt to the core system prompt', 'passes the active output style to the core system prompt', 'should pass interactive mode to the core system prompt', 'should pass acp mode to the core system prompt', 'should pass headless mode to the core system prompt'. Witness (run-produced): CI run 33403794522 job 99526325819 — 'Tests 8 failed | 22716 passed (22734)', every one of the five showing the assertion diff '+ false' as the extra 6th argument; and local reproduction in the PR worktree: npx vitest run src/core/client.test.ts -t "core system prompt" → 'Test Files 1 failed (1); Tests 5 failed | 385 skipped (390)' with the identical '+ false' diff. The base-side call has 5 arguments (merge-base client.ts), so these tests pass on the merge base — netNew by measurement. Fix: update the five assertions to include the new 6th argument (false with the current mocks, or stub isTodoWriteEnabled), ideally together with the R1-3 tightening (move isTodoWriteEnabled into the required Pick) so future callers cannot silently diverge.

— qwen3.8-max via Qwen Code /review (v0.22.3)

Comment thread packages/core/src/core/coreToolScheduler.ts Outdated
Comment thread packages/core/src/core/coreToolScheduler.ts Outdated
Comment thread packages/core/src/core/coreToolScheduler.ts Outdated
Comment thread docs/users/qwen-serve.md Outdated
Comment thread packages/core/src/core/coreToolScheduler.test.ts
Comment thread packages/core/src/core/client.ts Outdated
Comment thread packages/cli/src/config/settingsSchema.ts
Comment thread packages/core/src/core/coreToolScheduler.ts Outdated
Comment thread integration-tests/cli/todo_write.test.ts
# Conflicts:
#	docs/users/configuration/settings.md

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

Partially reviewed — gaps disclosed.

2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • R1-6 todo_write not-found branch is a structural copy of the list_directory branch — dropped as location overlap: packages/core/src/core/coreToolScheduler.ts:2160 is occupied by the round-1 R1-8 comment (comment 3896564552)
  • R1-8 daemon/ACP unregistered-tool branch returns a bare error without the opt-in hint — dropped as location overlap: packages/core/src/core/coreToolScheduler.ts:2164 is occupied by the round-1 R1-9 comment (comment 3896564544)

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not reviewed: build-and-test — the workspace unit-test phase did not complete under build-test (budget exhausted in the build phase); targeted verifier runs covered the changed suites.

Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:

  • packages/core/src/core/client.ts:383 — [probe] D2-1 Todo prompt/schema gates key off the settings flag, not effective availability (flag on + workspace toggle off still advertises todo_write)
  • packages/core/src/config/config.ts:9372 — [review] D2-2 coreTools allowlist non-promotion of todo_write is intentional per design doc but undocumented in the settings entries and pinned by no test
  • docs/users/qwen-serve.md:331 — [review] D2-3 added prose 'a new ordinary prompt must enable tools.todoWrite.enabled' ascribes a restart-required operator setting to the prompt
  • integration-tests/cli/todo_write.test.ts:19 — [test] D2-4 new integration test is unreachable by any workspace test lane and the CLI integration lane is skipping on this fork PR
  • integration-tests/cli/qwen-serve-streaming.test.ts:321 — [test] D2-5 updated serve/streaming integration test is likewise unreachable by workspace lanes
中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 2 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

未审查:build-and-test — the workspace unit-test phase did not complete under build-test (budget exhausted in the build phase); targeted verifier runs covered the changed suites。

收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 5 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.22.3)

Comment thread packages/core/src/core/client.ts Outdated
Comment thread docs/users/qwen-serve.md Outdated
Comment thread packages/core/src/core/coreToolScheduler.ts Outdated
Comment thread packages/core/src/core/client.ts Outdated
Comment thread packages/core/src/core/coreToolScheduler.test.ts
Comment thread packages/cli/src/config/settingsSchema.ts
Comment thread integration-tests/cli/todo_write.test.ts
@DragonnZhang

Copy link
Copy Markdown
Collaborator Author

Addressed all unresolved R1 review comments in fa2b819e34 and resolved the corresponding threads.

Highlights:

  • unified opt-in tool remediation across core/headless and daemon/ACP, including workspace, deny-rule, and core-tools allowlist causes
  • enforced and tested the Todo Stop Guard dependency on tools.todoWrite.enabled, including warning behavior
  • made isTodoWriteEnabled a required prompt-config contract and fixed the five regressed expectations
  • extracted the duplicated forced-tool integration runner and updated the qwen-serve recipe

Validation:

  • core client: 394/394; core scheduler: 389/389
  • CLI context: 18/18; full Session: 769/769
  • forced-tool integration: 5/5; qwen-serve streaming: 11/11
  • npm run typecheck, npm run lint, npm run build, and npm run bundle all pass

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

Partially reviewed — gaps disclosed. Suggestions are inline.

3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • integration tests unreachable from workspace test lanes (integration-tests/cli/todo_write.test.ts) — already recorded in the round-2 deferral list (D2-4, review 5074197635)
  • integration tests unreachable from workspace test lanes (integration-tests/cli/list_directory.test.ts) — already recorded in the round-2 deferral list (D2-4/D2-5, review 5074197635)
  • integration test helper unverifiable by workspace lanes (integration-tests/test-helper.ts) — already recorded in the round-2 deferral list (D2-4/D2-5, review 5074197635)

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": end-to-end execution of the new/changed integration tests (integration-tests/cli/todo_write.test.ts, list_directory.test.ts) — requires a full npm run build &&….

Convergence: round 3 posted 3 inline comment(s), 3 of them reported for the first time; the previous round posted 7 (0 new). Findings keep coming back to the same files: packages/core/src/core/coreToolScheduler.ts (findings in round 1; 2 more now); docs/users/qwen-serve.md (findings in round 1; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)

中文说明

仅完成部分审查,审查缺口已披露。 建议见行内评论。

本轮确认的 3 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 2)"end-to-end execution of the new/changed integration tests (integration-tests/cli/todo_write.test.ts, list_directory.test.ts) — requires a full npm run build &&…

收敛情况:第 3 轮发布了 3 条行内评论,其中 3 条是首次提出;上一轮发布了 7 条(其中 0 条首次提出)。发现反复回到同一批文件:packages/core/src/core/coreToolScheduler.ts(第 1 轮已出过发现,本轮又有 2 条);docs/users/qwen-serve.md(第 1 轮已出过发现,本轮又有 1 条)。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。本轮没有未决的 Critical,因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。(仅为观察——本轮评审未因此扣留任何内容。)

— qwen3.8-max via Qwen Code /review (v0.22.3)

Comment thread packages/core/src/core/coreToolScheduler.ts Outdated
Comment thread packages/core/src/core/coreToolScheduler.ts
Comment thread docs/users/qwen-serve.md Outdated
@wenshao

wenshao commented Sep 1, 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.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with @qwen-code /review. See workflow logs.

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

1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • integration-tests/cli/todo_write.test.ts is collected by no pull_request-time CI lane — already recorded in the round-2 deferral list (D2-4, review 5074197635)

Not explored to full depth (tool budget reached): chunk 11: could not execute prompts.test.ts / coreToolScheduler.test.ts to confirm green — the review worktree has no node_modules or packages/core/dist , so test …; "agent reverse-audit (round 1)": did not execute integration-tests/cli/qwen-serve-streaming.test.ts (daemon Todo Stop Guard replay) to confirm the added tools: { todoWrite: { enabled: true }….

Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:

  • packages/cli/src/acp-integration/session/Session.ts:11626 — [probe] the daemon builds the new opt-in guidance but no client frame ever carries it (sessionUpdateCount: 0)
  • packages/cli/src/acp-integration/session/Session.ts:11632 — [probe] three unregistered todo_write calls trip the parameter-loop detector and abort the whole daemon turn with -32603
  • packages/core/src/core/coreToolScheduler.ts:323 — [probe] the CLI never reaches the new deny/allowlist arms, and the message that intercepts gives advice this diff made insufficient
  • packages/core/src/config/config.ts:9569 — [probe] exitPlanMode.ts:283 and :381 still tell the model to update a todo list that is no longer declared
  • docs/users/configuration/settings.md:621 — [probe] experimental.sessionWorkflow silently goes inert at the new default with no doc precondition and no warning
  • packages/core/src/core/prompts.test.ts:92 — [probe] six enabled-variant prompt strings are pinned by no test; one added snapshot closes all six
  • packages/core/src/core/coreToolScheduler.ts:324 (+2 locations) — [probe] the settingAction ternaries in the workspace and allowlist arms are each pinned in only one direction
  • packages/core/src/core/coreToolScheduler.ts:293 — [probe] the list_directory branch skips the deny-rule diagnosis todo_write gets, naming a setting already on
  • packages/core/src/core/prompts.ts:588 — [probe] the QWEN_WRITE_SYSTEM_MD dump is now flag-dependent while the QWEN_SYSTEM_MD consumer is flag-blind
  • packages/core/src/config/config.ts:7700 — [probe] allowlist non-promotion of todo_write is recorded in neither a JSDoc nor a test, unlike its sibling getter
  • packages/core/src/config/config.test.ts:9470 — [probe] the deferred-registration exclusion for todo_write is unpinned; only registerFactory is inspected
  • docs/developers/tools/todo-write.md:33 — [probe] the 'When the AI uses this tool' section still describes the old default in unconditional present tense
  • packages/core/src/config/config.ts:9568 — [review] the TodoCreated/TodoCompleted hook docs and recipe carry no tools.todoWrite.enabled precondition
  • packages/cli/src/acp-integration/session/Session.test.ts:35156 — [probe] the new daemon guidance test's assertion is satisfied by both todo_write messages, so it pins neither
中文说明

已审查。 建议见行内评论。

本轮确认的 1 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未探索到全部深度(达到工具调用预算):chunk 11:could not execute prompts.test.ts / coreToolScheduler.test.ts to confirm green — the review worktree has no node_modules or packages/core/dist , so test …"agent reverse-audit (round 1)"did not execute integration-tests/cli/qwen-serve-streaming.test.ts (daemon Todo Stop Guard replay) to confirm the added tools: { todoWrite: { enabled: true }…

收敛姿态下延后(第 4 轮,非阻断)——已记录,本轮不要求修改:共 14 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)

Comment thread integration-tests/cli/todo_write.test.ts Outdated

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

Partially reviewed — gaps disclosed.

4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • R5-1 todo_write coreTools non-promotion pinned by no test — already reported in the round-2 deferral list (D2-2, review 5074197635)
  • R5-2 experimental.sessionWorkflow silently requires tools.todoWrite.enabled without warn/docs parity — already reported in the round-4 deferral list (settings.md:621, review 5091170739)
  • R5-3 default-off deferred-registration cell unpinned (only registerFactory inspected) — already reported in the round-4 deferral list (config.test.ts:9470, review 5091170739)
  • R5-4 enabled-variant prompt rendering pinned only by placement-blind substrings — already reported in the round-4 deferral list (prompts.test.ts:92, review 5091170739)

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not explored to full depth (tool budget reached): chunk 6: run cd packages/core && npx vitest run src/core/prompts.test.ts to mechanically confirm the regenerated snapshot matches (worktree has no node_modules; instal…; chunk 9: running packages/core prompts.test.ts to confirm snapshots pass — node_modules is absent in this review worktree; verified instead by verbatim text comparison…; chunk 5: run prompts.test.ts to execute the regenerated snapshots — the worktree has no node_modules / dist , and a monorepo install + build was out of budget; substi….

Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:

  • packages/core/src/core/coreToolScheduler.ts:335 — [probe] D5-1 the todo_write attribution fallback branch is pinned by no test (probe: reachable in bare mode with the flag on and a null permission manager)
  • packages/core/src/core/coreToolScheduler.ts:293 — [probe] D5-2 the LS branch never attributes the core-tools allowlist on the daemon/ACP path this PR newly wires into Session.ts (advice that can never register the tool)
中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 4 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

未探索到全部深度(达到工具调用预算):chunk 6:run cd packages/core && npx vitest run src/core/prompts.test.ts to mechanically confirm the regenerated snapshot matches (worktree has no node_modules; instal…;chunk 9:running packages/core prompts.test.ts to confirm snapshots pass — node_modules is absent in this review worktree; verified instead by verbatim text comparison…;chunk 5:run prompts.test.ts to execute the regenerated snapshots — the worktree has no node_modules / dist , and a monorepo install + build was out of budget; substi…

收敛姿态下延后(第 5 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.22.3)

@wenshao

wenshao commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /resolve

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

Qwen Code resolved the merge conflicts and pushed the branch update.

Root cause

main's PR #10761 (commit d0847308f1, custom output styles) collided with this PR's isTodoWriteEnabled plumbing. #10761 rewrote output-style resolution — trust gating in client.ts's MainSessionPromptConfig type and peer-style inheritance in ArenaManager.ts — at the exact lines where this PR added the new parameter, and both also extended the same createMockConfig mock in ArenaManager.test.ts.

Textual or semantic

Both sides edited the same constructs, but the changes are orthogonal — the union is correct:

// client.ts: PR's Pick member + #10761's intersection
type MainSessionPromptConfig = Pick<Config,  | 'isTodoWriteEnabled'> &
  Partial<Pick<Config, 'isTrustedFolder'>>;

// ArenaManager.ts: #10761 replaced the style arg; PR appended a new arg
getCoreSystemPrompt(undefined, model.modelId, undefined, 'headless',
  this.resolvePeerOutputStyle(),      // #10761: replaces getOutputStyle()
  this.config.isTodoWriteEnabled(),   // PR: new last argument
)

The test mock keeps the PR's isTodoWriteEnabled plus #10761's getSystemPrompt / getExperimentalZedIntegration / isInteractive members.

What is load-bearing

  • Arg order: getCoreSystemPrompt(…, interactionMode, outputStyle, todoWriteEnabled)isTodoWriteEnabled must stay the LAST argument; the style slot now belongs to resolvePeerOutputStyle(), not getOutputStyle().
  • isTodoWriteEnabled is a required (non-Partial) member of MainSessionPromptConfig. Both production callers pass the full Config, which defines it.

What I could not verify

  • packages/core/src/core/prompts.test.ts is broken by this merge; it auto-merged without a textual conflict, so it was out of scope to edit here. feat: load custom output styles from ~/.qwen/output-styles and .qwen/output-styles #10761's main-session style: project trust gate block hand-builds configs without isTodoWriteEnabled and passes them to getMainSessionBaseSystemPrompt (~lines 994/1002/1012). Since this PR makes that member required and calls it unconditionally, those three tests will fail typecheck and throw TypeError: config.isTodoWriteEnabled is not a function at runtime. Fix: add isTodoWriteEnabled: () => false to that block's makeConfig.
  • No build/typecheck/tests were run. By inspection, the merged ArenaManager test still holds: resolvePeerOutputStyle() returns the built-in Concise style (keepCodingInstructions: true, not Learning), so the PR's # Output Style: Concise / # Task Management assertions remain satisfiable; main's snapshot file was untouched by feat: load custom output styles from ~/.qwen/output-styles and .qwen/output-styles #10761's changes, so the PR's snapshot rewrites stay valid.
中文说明

根因:main 的 PR #10761(commit d0847308f1,自定义 output styles)重写了 output style 解析(client.tsMainSessionPromptConfig 的信任门控、ArenaManager.ts 中 peer 样式继承),恰好与本 PR 添加 isTodoWriteEnabled 参数的行重叠,双方还同时扩展了 ArenaManager.test.tscreateMockConfig

文本还是语义:双方改了同一处结构,但改动正交,取并集即正确(见上方代码块)。

关键点isTodoWriteEnabled 必须是 getCoreSystemPrompt 的最后一个参数;样式参数位现在归 resolvePeerOutputStyle()isTodoWriteEnabledMainSessionPromptConfig 的必需成员,两个生产调用方都传完整 Config

未能验证:未运行构建/类型检查/测试。prompts.test.ts 在合并后存在破损(该文件无文本冲突,按规则本次不可修改):#10761 新增的 trust gate 测试手工构造的 config 缺少 isTodoWriteEnabled,而本 PR 使其成为必需成员且无条件调用,相关三个测试会类型检查失败并抛 TypeError。修复方法:在该块的 makeConfig 中加 isTodoWriteEnabled: () => false。经静态检查,ArenaManager 测试断言(Concise 样式、# Task Management)在合并后依然成立。

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

Partially reviewed — gaps disclosed.

1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • integration tests unreachable by workspace test lanes (integration-tests/cli/todo_write.test.ts, list_directory.test.ts, qwen-serve-streaming.test.ts, test-helper.ts) — already recorded in the round-2 deferral list (D2-4, review 5074197635)

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:

  • packages/core/src/core/prompts.ts:590 — [probe] D6-1 QWEN_WRITE_SYSTEM_MD dump forwards todoWriteEnabled but no test pins the dumped content (mutant survives; re-feeding the dump as QWEN_SYSTEM_MD silently drops Todo guidance)
中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 1 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查(原文为英文):build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

收敛姿态下延后(第 6 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.23.0)

Comment thread packages/core/src/core/client.ts

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

⚠️ Downgraded from Approve to Comment: CI still running. Reviewed.

Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:

  • packages/core/src/core/prompts.test.ts:942 — [probe] the new wiring test pins only the enabled direction at getMainSessionBaseSystemPrompt; the disabled direction is unguarded at the client.ts:392 read site
中文说明

⚠️ 已从批准降级为评论:CI still running。 已审查。

收敛姿态下延后(第 7 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.23.0)

@wenshao

wenshao commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

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

APPROVE (verified at head b6f33ce)

Historical blocking items — re-verified against the exact head

  • R6-1 (Critical, round-6 CHANGES_REQUESTED at fd73049) — fixed at head. The prescribed member is present in the trust-gate makeConfig (prompts.test.ts:973, inside the main-session style: project trust gate describe starting at :952), mirroring the sibling helper at :845 — and the compile symptom that made it Critical (packages/core TS2345 halting every dependent build) is disproven at this head by the green Lint & Static typecheck lane, which is the job that would halt before any test could emit. The three named trust-gate tests were not re-run in this pass (the Test (ubuntu-latest) leg shows a cancellation, not a failure); the code-level member presence plus the green typecheck is what retires the blocker.
  • R1-10 (Critical, round-2) — long closed. The broken client.test.ts mocks now carry the member, no later round re-raised it, and rounds 3–7 carry no unresolved Critical list.
  • The reviewDecision flag still reads CHANGES_REQUESTED only because the requesting bot's later rounds were COMMENT-class and never procedurally flipped it; the head round posted zero findings, and the maintainer approved this exact commit.

My Critical-only scan of the opt-in surface

  • Default-off is fail-safe in the right direction: todoWriteEnabled defaults to false at every prompt entry point, so guidance can never advertise todo_write when registration is gated off by the same flag (registerLazy(TODO_WRITE) sits behind isTodoWriteEnabled(); the AgentTool todo_id schema field and description drop with it; the 192-line snapshot shift tracks the prompt change).
  • experimental.todoStopGuard now ANDs with the tool flag and emits a debug warning naming the dependency when configured-but-inert — no silently-live stop guard over an unregistered tool.
  • getOptInToolNotFoundMessage is bounded diagnostics only (exact-map + Object.hasOwn on canonicalized names, registered-tool short-circuit, precedence workspace-toggle → deny-rule → core-tools-allowlist → setting, otherwise the original not-found error); it cannot gate execution or expose anything beyond setting names already documented.
  • The new-app SKILL step and docs were de-referenced from todo_write consistently with the opt-in posture.

CI at head

25 green including Lint & Static; Test (ubuntu-latest) shows the recurring shared-pool cancellation (not a failure) with one run still queued — non-attributable, non-gating per policy. The thread also carries headless and tmux E2E reports from earlier heads; the human maintainer approved this exact head.

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Sandboxed verification: ⚠️ incomplete — infrastructure failure - workflow run

The verification job did not complete (checkout, runner, or setup error) and produced no report. See the workflow run for details.

中文 — 判定:⚠️ 未完成 · 基础设施故障

验证作业未完成(检出、runner 或初始化错误),未生成报告。详见工作流运行日志。

Qwen Code · sandboxed verification

@DragonnZhang
DragonnZhang added this pull request to the merge queue Sep 5, 2026
Merged via the queue into QwenLM:main with commit e36a536 Sep 5, 2026
112 of 113 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code review

No Critical findings. I read the whole production diff (267 lines across 9 files) and traced every consumer of the new flag rather than taking the description's word for it.

What I verified, because these are the places a change like this usually goes wrong:

  • The flag is not a dead switch. getCoreSystemPrompt gained a trailing todoWriteEnabled = false. I grepped every production call site across .ts/.tsx: there are exactly two (core/client.ts:381 inside getMainSessionBaseSystemPrompt, and agents/arena/ArenaManager.ts:1108), and both pass config.isTodoWriteEnabled(). The other two production prompt paths — core/client.ts:1547 and cli/src/ui/commands/contextCommand.ts:122 — call getMainSessionBaseSystemPrompt, so they inherit it. Nothing advertises Todo without registering it, and nothing registers it without advertising it.
  • Defaults fail safe. Config.todoWriteEnabled is params.todoWriteEnabled ?? false with a = false field initializer, and the CLI side is settings.tools?.todoWrite?.enabled === true. An absent or malformed setting yields disabled, never accidentally enabled.
  • The Stop Guard can't outlive the tool. Session.ts now ANDs experimental.todoStopGuard with the tool flag and emits a debug warning naming the dependency when it's configured-but-inert. That's the single choke point — the guard is constructed once from that value, and the acp-bridge references are bookkeeping driven by it, so gating there is sufficient.
  • The new error path is diagnostics only. getOptInToolNotFoundMessage short-circuits when the canonical tool is registered, matches on an exact map with Object.hasOwn, and otherwise returns the pre-existing not-found error. It can't gate execution and discloses nothing beyond setting names already in docs/users/configuration/settings.md. Its precedence (workspace toggle → deny rule → core-tools allowlist → setting) is right: the most specific user action is named first.
  • Schema stayed in lockstep. settings.schema.json matches settingsSchema.ts including the todoStopGuard description update, and the Check settings schema is up-to-date CI gate is green at this head — that check regenerates the file and fails on drift, so this is machine-verified rather than eyeballed.
  • Remaining todo_write references are inert, not broken. acp-bridge/transcript-replay.ts, webui/.../routing.ts and labelUtils.ts, sdk-typescript/daemon/ui/*, and followup/speculationToolGate.ts all key off a tool call that can no longer occur. They render nothing and match nothing; historical transcripts still replay, which is what the PR promises.

Two Suggestions, neither blocking:

  1. The two new deterministic integration tests never ran in CI at this head. integration-tests/cli/todo_write.test.ts gained exactly the assertions the PR body cites as its "After" evidence — default-off omits the declaration and the prompt guidance and returns the opt-in error; enabled restores both — and they're fully deterministic against the fake OpenAI server, no API key needed. But the token-free test:integration:no-ak:sandbox:none lane runs a hardcoded file allowlist that includes ./cli/list_directory.test.ts and ./cli/qwen-serve-streaming.test.ts and not ./cli/todo_write.test.ts; the lane that would run it, Integration Tests (CLI, No Sandbox), was skipped here. The file can't just be allowlisted wholesale because it also holds the pre-existing model-driven scenario. Splitting the two deterministic cases into their own file (or a -t name filter in the allowlist) would put the PR's headline evidence on every future PR instead of only on the AK lane. Pre-existing gap that this PR now depends on, not one it introduced.
  2. A trailing positional boolean on a 6-parameter signature is easy to omit silently. Both current call sites are correct and the default is the safe one, so there's no defect today — but the next caller who forgets gets a prompt that quietly drops Todo guidance while the tool stays registered. An options object, or reading the flag off a Config the function already receives, would make the omission impossible. Worth weighing, not worth blocking.
sequenceDiagram
    participant P1 as settings tools.todoWrite.enabled
    participant P2 as CLI config loader
    participant P3 as core Config
    participant P4 as ToolRegistry
    participant P5 as system prompt
    participant P6 as Agent tool schema
    participant P7 as ACP Session stop guard
    participant P8 as CoreToolScheduler
    P1->>P2: read setting, absent means false
    P2->>P3: todoWriteEnabled, default false
    P3->>P4: register todo_write only when enabled
    P3->>P5: Task Management guidance only when enabled
    P3->>P6: todo_id field only when enabled
    P3->>P7: stop guard ANDs with enabled, warns when inert
    P4-->>P8: legacy call arrives, tool not registered
    P8-->>P8: actionable error naming the setting
Loading
Files changed (16 of 30 shown)
File What changed
packages/core/src/core/coreToolScheduler.ts Biggest production hunk (108 lines). Generalizes the old list_directory-only not-found explanation into a shared opt-in-tool helper covering both tools, with deny-rule and allowlist precedence.
packages/core/src/core/prompts.ts Task Management section and the tool-guidance bullet become conditional; the Plan/Adapt bullets get Todo-free wording. New trailing flag defaults false at both prompt entry points.
packages/cli/src/acp-integration/session/Session.ts Stop guard ANDs with the tool flag and warns when configured-but-inert; the ACP tool-not-found path reuses the new helper.
packages/cli/src/config/settingsSchema.ts New tools.todoWrite.enabled entry mirroring listDirectory, plus a dependency note on todoStopGuard.
packages/core/src/config/config.ts New todoWriteEnabled param, field, and accessor; todo_write lazy registration moves behind it.
packages/core/src/tools/agent/agent.ts todo_id schema field and its usage-guidance line both drop when Todo is off.
packages/core/src/core/client.ts Passes the flag into the main-session base prompt; widens the config Pick.
packages/cli/src/config/config.ts One line wiring the setting into core config params.
packages/core/src/agents/arena/ArenaManager.ts One line passing the flag to the arena peer prompt.
packages/vscode-ide-companion/schemas/settings.schema.json Generated schema, consistent with the source entry; CI freshness gate green.
integration-tests/cli/todo_write.test.ts Two new deterministic cases (default-off, enabled) and the existing model-driven case now opts in.
integration-tests/test-helper.ts New shared runForcedToolCallScenario helper extracted from the list_directory test.
integration-tests/cli/list_directory.test.ts Rewritten onto the extracted helper; same two scenarios, 47 fewer lines.
packages/core/src/core/coreToolScheduler.test.ts 181 new lines covering the helper's precedence branches and the registered-tool short-circuit.
packages/core/src/core/snapshots/prompts.test.ts.snap 192-line reduction tracking the prompt guidance removal.
docs/design/todo-write-opt-in.md New design doc, which is what AGENTS.md asks for on a multi-file change.
…and 14 more files Test additions in Session/agent/config/prompts/client/ArenaManager/contextCommand, plus user and developer docs.

Testing

Evidence carried here: the PR's own CI, read through the API. Per the review rules I did not build, run, or execute anything from this PR's tree — no gh pr checkout, no test run, no script.

The headline: the unit-test lane is cancelled, not green, and the legs that would have covered the gap were skipped. But the cancellation happened after a full suite pass completed, and the log records what that pass found.

Test (ubuntu-latest, Node 22.x) ran 00:02:04 → 02:02:40 and hit the two-hour job limit during its --retry=2 re-run; ##[error]The operation was canceled. at 02:02:18, with the orphaned npm run test:ci:workspaces processes reaped afterwards. Test (macos-latest) and Test (windows-latest) show skipped, as does Integration Tests (CLI, No Sandbox). The completed pass reported:

Test Files  3 failed | 1008 passed (1011)
     Tests  4 failed | 28497 passed | 90 skipped (28591)
  Duration  4782.28s

All four failures are outside this PR's diff, and I classified each from the code rather than from the log's own prose:

  1. src/acp-integration/acpAgent.test.ts — the runtime-root pinning guard, which asserts acpAgent.ts names runWithAcpRuntimeOutputDir directly exactly once. At this head it found two (lines 4595 and 9200). This is stale-main breakage the PR inherited, already fixed upstream: the merge base 419e8d57 also has two mentions, current main 7fca0eed has one, and this PR does not touch acpAgent.ts at all — a merge takes main's fixed version.
  2. src/ui/components/SkillReviewDialog.test.tsxvi.waitFor 5s timeout waiting on a file-watcher refresh.
  3. src/ui/hooks/useWorktreeSession.test.tsxvi.waitFor 2s timeout on sidecar deletion.
  4. src/ui/hooks/useWorktreeSession.test.tsxvi.waitFor 2s timeout on sidecar creation.

Items 2–4 are all filesystem-watcher timing assertions with 2–5 second budgets, on a self-hosted runner whose own telemetry shows a sustained load average of 220–240 for the entire two hours (DFSAMPLE ... load[227.25 229.13 239.35] hosttests[94], disk 87% full). None of the three files is in this diff. Load-induced watcher flakes, not regressions.

Meanwhile every test file this PR touches passed in that same run:

Test file Result
src/core/coreToolScheduler.test.ts ✓ 395 tests
src/acp-integration/session/Session.test.ts ✓ 815 tests
src/core/client.test.ts ✓ 395 tests
src/tools/agent/agent.test.ts ✓ 283 tests
src/core/prompts.test.ts ✓ 156 tests
src/config/config.test.ts (core) ✓ 618 tests
src/config/config.test.ts (cli) ✓ 406 tests
src/agents/arena/ArenaManager.test.ts ✓ 30 tests
src/ui/commands/contextCommand.test.ts ✓ 18 tests

Typecheck evidence, and a correction to the record: ci.yml has no repo-wide npm run typecheck step — the only one is typecheck:integration, scoped to integration-tests/. So the earlier approval's reliance on "the green Lint & Static typecheck lane" doesn't hold as stated; that job runs ESLint, Prettier, actionlint, yamllint, schema freshness and i18n. The real compile evidence is the tsc --build that npm ci triggers through the workspace prepare script: it ran for every package including @qwen-code/qwen-code-core@0.23.0 inside the green Install dependencies step, and the log contains zero error TS lines. Compilation at this head is therefore genuinely verified — just by a different lane than was cited. That matters here because the last round's open Critical was a TS2345, and this is what actually retires it (along with b6f33cef being the mock-completion fix itself).

Check Conclusion
Test (ubuntu-latest, Node 22.x) cancelled
Test (macos-latest, Node 22.x) skipped
Test (windows-latest, Node 22.x) skipped
Integration Tests (CLI, No Sandbox) skipped
Lint & Static (ubuntu-latest, Node 22.x) success
Integration Tests (no-AK, No Sandbox) success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) success
Desktop Shell (ubuntu-22.04) success
Desktop Shell (windows-2022) success
TUI parity snapshots (ink vs opentui) success
OpenTUI no-flicker gate success
Real daemon E2E / Java 11 success
SDK Java (ubuntu/macos/windows, Java 11/17/21) success
Security Checks success
Secret scan (TruffleHog) success
Dependency CVE audit success
Classify PR / label / assign / authorize / precheck-pr success

The green Integration Tests (no-AK, No Sandbox) lane is worth noting for what it does cover: npm run typecheck:integration (so the new integration test compiles), cli/list_directory.test.ts ✓ 2 tests (which validates the test-helper.ts extraction the PR made), and cli/qwen-serve-streaming.test.ts ✓ 11 tests — 22 files passed in total.

Not verified, and why:

  • The PR's two new deterministic integration tests never executed in CI at this head — their lane was skipped, and the token-free lane's allowlist doesn't include the file. The default-off declaration/prompt/error behaviour and the enabled-path restoration are therefore not end-to-end substantiated by any green check here. The unit tests above pin each gated surface individually; nothing pins the assembled bundled-CLI behaviour.
  • Windows and Linux runtime behaviour. The PR's own Tested-on table marks macOS ✅ and both others ⚠️, and those Test legs were skipped.
  • The interactive TUI consequence. StickyTodoList / TodoDisplay receiving no data by default is reasoned from the code, not observed.

Sandboxed verification would settle both open ends, and the author has write access so neither needs sponsoring: @qwen-code /verify — that the default-off path really omits todo_write from the declared tools and the base prompt in a bundled headless run, and that a forced legacy call returns the opt-in error naming tools.todoWrite.enabled, which is exactly what the two never-executed integration tests assert; and @qwen-code /tmux — that the interactive TUI degrades sensibly with Todo off rather than leaving an empty progress region, since no check on this head renders that surface.

中文说明

代码审查

未发现 Critical 问题。我读完了全部生产代码 diff(9 个文件、267 行),并亲自追踪了新开关的每一个使用方,而不是照搬 PR 描述。

以下是我核实过的、这类改动最容易出问题的地方:

  • 这个开关不是死开关。 getCoreSystemPrompt 新增了末尾参数 todoWriteEnabled = false。我在全部 .ts/.tsx 中检索了每一个生产调用点:恰好两个(core/client.ts:381getMainSessionBaseSystemPrompt 内部,以及 agents/arena/ArenaManager.ts:1108),两者都传入了 config.isTodoWriteEnabled()。另外两条生产提示词路径——core/client.ts:1547cli/src/ui/commands/contextCommand.ts:122——调用的是 getMainSessionBaseSystemPrompt,因此自动继承。不存在"提示词宣传了 Todo 但工具未注册",也不存在反向情况。
  • 默认值是安全的。 Config.todoWriteEnabledparams.todoWriteEnabled ?? false 且字段初始化为 = false;CLI 侧为 settings.tools?.todoWrite?.enabled === true。设置缺失或异常时结果是关闭,绝不会意外开启。
  • Stop Guard 不会比工具活得更久。 Session.ts 现在把 experimental.todoStopGuard 与工具开关做与运算,并在"已配置但失效"时输出指明依赖关系的 debug 警告。这里是唯一的收口点——guard 由该值构造一次,acp-bridge 中的引用只是随之驱动的记账,因此在此处设卡就足够了。
  • 新的报错路径只做诊断。 getOptInToolNotFoundMessage 在规范名工具注册时立即短路,用精确映射配合 Object.hasOwn 匹配,其余情况返回原有的 not-found 错误。它无法干预执行,也不会泄露 docs/users/configuration/settings.md 中已有的设置名之外的信息。其优先级(workspace 开关 → deny 规则 → core-tools 白名单 → 设置)是正确的:最先指出最具体的用户操作。
  • schema 保持同步。 settings.schema.jsonsettingsSchema.ts 一致,包含 todoStopGuard 描述更新,且该 head 上 Check settings schema is up-to-date CI 关卡为 green——该检查会重新生成文件并在漂移时失败,所以这是机器验证而非人工目测。
  • 残留的 todo_write 引用是惰性的,不是坏的。 acp-bridge/transcript-replay.tswebui/.../routing.tslabelUtils.tssdk-typescript/daemon/ui/*followup/speculationToolGate.ts 都依赖一个已不可能发生的工具调用。它们不会渲染也不会匹配任何东西;历史 transcript 仍可回放,这正是 PR 承诺的行为。

两条 Suggestion,均不阻塞:

  1. 两个新增的确定性集成测试在该 head 上从未在 CI 中运行过。 integration-tests/cli/todo_write.test.ts 新增的正是 PR 正文引用为"After"证据的断言——默认关闭时不声明工具、不含提示词指导并返回开启提示;开启后两者恢复——而且它们完全基于 fake OpenAI server,无需 API key。但免 token 的 test:integration:no-ak:sandbox:none lane 运行的是硬编码文件白名单,其中包含 ./cli/list_directory.test.ts./cli/qwen-serve-streaming.test.ts不包含 ./cli/todo_write.test.ts;而会运行它的 Integration Tests (CLI, No Sandbox) lane 在此被 skipped。该文件也不能整体加入白名单,因为它同时包含既有的模型驱动场景。把两个确定性用例拆到独立文件(或在白名单中使用 -t 名称过滤),就能让 PR 的核心证据出现在今后每个 PR 上,而不只出现在需要 AK 的 lane。这是本 PR 现在依赖的既有缺口,而非它引入的。
  2. 6 参数签名末尾的位置布尔值很容易被静默漏传。 当前两个调用点都正确,默认值也是安全的一侧,所以今天没有缺陷——但下一个忘记传的调用方会得到"工具仍注册、提示词却悄悄丢掉 Todo 指导"的结果。改成 options 对象,或让函数从它已经接收的 Config 上读取该开关,就能让漏传变得不可能。值得权衡,不值得阻塞。

(流程图见上,此处不重复。)

测试

本节携带的证据:PR 自身的 CI,通过 API 读取。 按审查规则,我没有构建、运行或执行本 PR 代码树中的任何东西——没有 gh pr checkout,没有跑测试,没有执行脚本。

核心结论:单元测试 lane 是 cancelled,不是 green,而本可补上缺口的其他 leg 被 skipped。但取消发生在一次完整套件跑完之后,日志记录了那次的结果。

Test (ubuntu-latest, Node 22.x) 从 00:02:04 运行到 02:02:40,在 --retry=2 重跑期间触及两小时 job 上限;02:02:18 出现 ##[error]The operation was canceled.,随后清理了遗留的 npm run test:ci:workspaces 进程。Test (macos-latest)Test (windows-latest) 显示 skippedIntegration Tests (CLI, No Sandbox) 同样。已完成的那次报告:

Test Files  3 failed | 1008 passed (1011)
     Tests  4 failed | 28497 passed | 90 skipped (28591)
  Duration  4782.28s

四个失败全部在本 PR diff 之外,且我依据代码而非日志自述来分类:

  1. src/acp-integration/acpAgent.test.ts——runtime-root pinning 守卫,断言 acpAgent.ts 直接提及 runWithAcpRuntimeOutputDir 恰好一次。该 head 上找到两处(4595 与 9200 行)。这是 PR 继承到的、上游已修复的 main 陈旧破坏:merge base 419e8d57 同样是两处,当前 main 7fca0eed 是一处,而本 PR 完全没有改动 acpAgent.ts——合并会采用 main 的已修复版本。
  2. src/ui/components/SkillReviewDialog.test.tsx——vi.waitFor 5 秒超时,等待文件 watcher 刷新。
  3. src/ui/hooks/useWorktreeSession.test.tsx——vi.waitFor 2 秒超时,sidecar 删除。
  4. src/ui/hooks/useWorktreeSession.test.tsx——vi.waitFor 2 秒超时,sidecar 创建。

第 2–4 项都是预算 2–5 秒的文件系统 watcher 时序断言,而运行它们的 self-hosted runner 自身遥测显示整整两小时负载均值维持在 220–240DFSAMPLE ... load[227.25 229.13 239.35] hosttests[94],磁盘占用 87%)。三个文件都不在本 diff 中。属于负载导致的 watcher 抖动,不是回归。

同时,本 PR 触及的每一个测试文件在同一次运行中都通过(见上表)。

关于类型检查证据,以及对既有记录的一处更正:ci.yml没有仓库级的 npm run typecheck 步骤——唯一的一个是 typecheck:integration,范围限于 integration-tests/。因此先前批准所依据的"green 的 Lint & Static typecheck lane"这一说法并不成立;该 job 运行的是 ESLint、Prettier、actionlint、yamllint、schema 新鲜度与 i18n。真正的编译证据是 npm ci 通过 workspace prepare 脚本触发的 tsc --build:它在 greenInstall dependencies 步骤中为包括 @qwen-code/qwen-code-core@0.23.0 在内的每个 package 运行,且日志中 error TS 出现次数为 0。所以该 head 的编译确实得到了验证——只是来自与被引用者不同的 lane。这一点在此重要,因为上一轮遗留的 Critical 是一个 TS2345,而这才是真正让它失效的证据(外加 b6f33cef 本身就是补全 mock 的修复)。

(CI 结论表见上,位于机器可读标记区内,不在此重复。)

green 的 Integration Tests (no-AK, No Sandbox) lane 值得说明它覆盖了什么:npm run typecheck:integration(因此新集成测试可编译)、cli/list_directory.test.ts ✓ 2 tests(验证了 PR 所做的 test-helper.ts 抽取)、cli/qwen-serve-streaming.test.ts ✓ 11 tests——共 22 个文件通过。

未验证项及原因:

  • PR 新增的两个确定性集成测试在该 head 上从未在 CI 中执行——其 lane 被 skipped,而免 token lane 的白名单不含该文件。因此"默认关闭时的声明/提示词/报错行为"与"开启路径的恢复"在此没有任何 green 检查提供端到端佐证。上述单元测试分别钉住了每个受控面;但没有东西钉住组装后的 bundled-CLI 行为。
  • Windows 与 Linux 运行时行为。 PR 自己的测试平台表格标注 macOS ✅、其余两者 ⚠️,而那些 Test leg 被 skipped。
  • 交互式 TUI 的后果。 StickyTodoList / TodoDisplay 默认收不到数据这一点是从代码推断的,未实际观察。

沙箱验证可以了结这两个悬而未决的点,且作者具备 write 权限,因此两者都无需 sponsored:@qwen-code /verify——验证默认关闭路径在 bundled headless 运行中确实既不声明 todo_write 也不在基础提示词中包含它,且强制发出的旧式调用会返回指明 tools.todoWrite.enabled 的开启提示,这正是那两个从未执行的集成测试所断言的;以及 @qwen-code /tmux——验证 Todo 关闭时交互式 TUI 是优雅降级,而不是留下一个空的进度区域,因为该 head 上没有任何检查会渲染这个界面。

Qwen Code · qwen3.8-max-2026-09-02

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 3/5 — the code review is clean and I'd defend every line of it; what I can't give you is a green end-to-end run of the behaviour this PR exists to change.

Going back to the proposal I wrote before opening the diff: I said I'd add a tools.todoWrite.enabled setting mirroring tools.listDirectory.enabled, plumb it into core Config with a default-false field, gate registration, gate the prompt block at every entry point, gate the todo_id field on the Agent schema, make the not-found path name the setting, and then chase the dependents — the daemon Stop Guard, ACP rendering, the arena prompt, the bundled new-app skill. That is what this PR does, file for file. I did not find a simpler path it missed. The one design choice I'd have made differently is the trailing positional boolean on getCoreSystemPrompt, and I've said so in Stage 2 as a Suggestion — it's a maintainability trap, not a defect, because both call sites are correct and the default is the safe one.

If I had to maintain this in six months I'd thank the author. The flag defaults false at every layer, so the failure mode of forgetting to wire something up is "Todo guidance missing" rather than "prompt advertises a tool that isn't registered" — that asymmetry is the whole ballgame for a change like this and they got it right. Generalizing the existing list_directory not-found explanation into one shared helper instead of copy-pasting it for a second tool was the correct instinct, and the precedence order in that helper is more thoughtful than it needed to be. There's a design doc, the schema was regenerated rather than hand-edited, and the existing model-driven integration scenario was opted in instead of being left to rot. Nothing here is trying too hard.

So why not approve. Two things, and neither is hygiene.

First, the lane that would prove the end-to-end behaviour never ran. Test (ubuntu-latest, Node 22.x) finished cancelled at the two-hour mark during its retry, and Test (macos/windows) plus Integration Tests (CLI, No Sandbox) were skipped behind it. The PR's own two new deterministic integration tests — the ones asserting the bundled CLI omits the declaration and the prompt guidance by default and returns the opt-in error — are not in the token-free lane's hardcoded allowlist, so no green check on this head has ever executed them. The unit suite does pin each gated surface individually and all of it passed, and I've classified all four unrelated failures against the code (one is stale-main breakage already fixed upstream, three are watcher flakes on a runner sitting at load 220–240). But "each surface pinned separately" is not the same as "the assembled behaviour observed once," and the assembled behaviour is the entire claim.

Second, this pass grew into acp-integration/session/Session.ts, which is on the revert-correlated path list. That signal doesn't block anything, but it does condition approval on real CI evidence, and the CI evidence at this head is a cancelled check.

I want to be precise about what I am not saying. I found no Critical. I don't think this PR is wrong, over-scoped, or solving a fake problem — the direction question I stopped on at 821eb0a1 was a genuine product call, @wenshao and @qqqys made it, and I'm treating it as made. If a maintainer reads the cancelled lane as infra noise and merges on the strength of the two approvals already standing, that is a defensible call and my review supplies nothing that contradicts it. I'm deferring because the gate's rule on elevated-risk paths is to want CI evidence before approving, and I don't have it — not because I doubt the code.

The gap is cheap to close and doesn't need a new push: @qwen-code /verify would A/B the default-off declaration and prompt omission plus the opt-in error against the base build, and @qwen-code /tmux would show whether the interactive TUI degrades cleanly with Todo off. Either one settles what the cancelled lane left open.

⏸️ Deferring to @jifeng — no code-level blocker, but the unit-test lane on this head ended cancelled rather than green and the PR's own end-to-end Todo tests have never executed in CI, so I'm not willing to approve on static review alone for a change that touches acp-integration. Needs a human call on whether the cancelled lane is infra noise or worth a /verify first.

中文说明

Confidence: 3/5 —— 代码审查是干净的,其中每一行我都愿意为其辩护;我给不出的,是这个 PR 存在的目的所要改变的行为的一次 green 端到端运行。

回到我在打开 diff 之前写下的方案:我会新增一个 tools.todoWrite.enabled 设置,对齐 tools.listDirectory.enabled;把它接入 core Config 并使用默认 false 的字段;对注册设卡;在每一个入口对提示词段落设卡;对 Agent schema 上的 todo_id 字段设卡;让 not-found 路径指明该设置;然后追查依赖方——daemon Stop Guard、ACP 渲染、arena 提示词、内置 new-app skill。这正是本 PR 逐文件所做的事。我没有找到它遗漏的更简路径。唯一我会做得不同的设计选择是 getCoreSystemPrompt 末尾的位置布尔值,我已在 Stage 2 作为 Suggestion 提出——它是可维护性陷阱,不是缺陷,因为两个调用点都正确,默认值也在安全的一侧。

如果六个月后由我来维护这份代码,我会感谢作者。开关在每一层都默认 false,所以"漏接某处"的失败模式是"缺少 Todo 指导",而不是"提示词宣传了一个未注册的工具"——对这类改动而言,这个不对称性就是全部关键,而他们做对了。把既有的 list_directory not-found 说明泛化为一个共享 helper、而不是为第二个工具复制粘贴,是正确的直觉;该 helper 中的优先级顺序也比必需的更为周全。这里有设计文档,schema 是重新生成而非手改的,既有的模型驱动集成场景被显式开启而不是任其腐坏。这份改动没有任何用力过猛之处。

那么为什么不批准。两件事,都不是卫生问题。

第一,能够证明端到端行为的 lane 从未运行。Test (ubuntu-latest, Node 22.x) 在重跑期间于两小时上限处以 cancelled 结束,其后的 Test (macos/windows)Integration Tests (CLI, No Sandbox) 被 skipped。PR 自己新增的两个确定性集成测试——断言 bundled CLI 默认不声明工具、不含提示词指导并返回开启提示的那两个——不在免 token lane 的硬编码白名单中,因此该 head 上没有任何 green 检查执行过它们。单元套件确实分别钉住了每个受控面,而且全部通过;四个无关失败我也都对照代码做了分类(一个是上游已修复的 main 陈旧破坏,三个是负载 220–240 的 runner 上的 watcher 抖动)。但"每个面分别被钉住"不等于"组装后的行为被观察过一次",而组装后的行为正是全部主张所在。

第二,本轮改动扩展到了 acp-integration/session/Session.ts,它位于与 revert 相关的路径列表中。该信号不阻塞任何东西,但确实把批准建立在真实 CI 证据之上,而该 head 的 CI 证据是一个 cancelled 检查。

我想精确说明我没有在说什么。我没有发现 Critical。我不认为这个 PR 是错的、范围过大的,或在解决一个假问题——我在 821eb0a1 上停下的方向问题是真实的产品判断,@wenshao@qqqys 已经做出,我视其为已定。如果维护者把 cancelled 的 lane 读作基础设施噪声,并基于已经存在的两个批准合并,那是站得住的判断,我的审查没有提供任何与之矛盾的东西。我之所以推迟,是因为 gate 在较高风险路径上的规则是"批准前需要 CI 证据",而我拿不到——不是因为我对代码有疑虑。

这个缺口关闭成本很低,且不需要新的 push:@qwen-code /verify 可以对照 base build 做默认关闭时的声明与提示词省略以及开启提示错误的 A/B;@qwen-code /tmux 可以显示 Todo 关闭时交互式 TUI 是否干净降级。任一项都能了结 cancelled lane 留下的空白。

⏸️ 转交 @jifeng —— 没有代码级阻塞项,但该 head 上的单元测试 lane 以 cancelled 而非 green 结束,且 PR 自己的端到端 Todo 测试从未在 CI 中执行过,因此对于一个触及 acp-integration 的改动,我不愿意仅凭静态审查批准。需要人工判断:cancelled 的 lane 属于基础设施噪声,还是值得先跑一次 /verify

Qwen Code · qwen3.8-max-2026-09-02

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.23.1.

@DragonnZhang
DragonnZhang deleted the dragon/todo-write-opt-in branch September 11, 2026 08:57
Fe2-O3 pushed a commit to Fe2-O3/rulesync that referenced this pull request Sep 15, 2026
Qwen Code v0.23.1 made the built-in todo_write tool opt-in through tools.todoWrite.enabled (QwenLM/qwen-code#10645). Generate already spread a hand-written value, but the curated import allow-list dropped it, so an import -> generate round trip lost the key. Add it to QWEN_OVERRIDE_TOOLS_KEYS with a global-machine-wide scope note (upstream lists it in no workspace-scope restriction at v0.23.4), the round-trip and note tests, and the two prose key lists.

Refs dyoshikawa#2668

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019yLN6CVqeZ8HCrjRb9o7mK
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category/core Core engine and logic category/tools Tool integration and execution need-discussion scope/settings Settings and preferences type/enhancement Non-bug improvement or optimization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants