docs(core): say why the Todo dependency semantics are not behind the flag - #11078
Conversation
…flag `todo_write`'s omitted-`blockedBy` preservation, and the `blockedBy` schema text that invites the omission, both reach sessions with Session Workflow disabled. That was a deliberate choice when the behaviour landed, but nothing in the file said so, so it reads as a missed gate — the `hasActivePlan` derivation sits bare, and the only nearby comment explains the dangling-edge filter rather than the gating. Record the decision and its reasoning: these are plan data-model semantics rather than presentation, and gating them on a visualization switch would give the tool two dialects, storing different plans for the same call depending on whether anyone is looking at the graph. Record the blast radius in the same place, because "reaches every session" overstates it. The merge changes an update only when the previous plan still has an unfinished item, the same id carried a non-empty `blockedBy`, and this call omits it. Plans that never used dependencies are untouched (`undefined?.filter()` is `undefined`), callers that always send the full list are unaffected, and `[]` stays the explicit clear. What does reach every session is the schema description, which is why it is named as widening this branch's exposure rather than as an independent change. Comment-only; no behaviour, no signatures.
|
|
|
Thanks for the PR — and for splitting it out of #10938, a 25-line comment change has no business riding behind a 1,542-line visual one. Template looks good ✓ (all headings present, Before/After correctly Problem: neither an observed bug nor theoretical hardening — it's a documentation gap, and it is real in the tree today. I checked the base at Direction: aligned, and there's a stronger argument for it than the PR body makes. The decision is already published — the Size: core path ( Approach: one honest question, not a blocker. Twenty-five lines across two blocks to annotate a 15-line merge is more than the why needs. The first block (data-model vs. presentation, "two dialects") is the load-bearing part. The second spends several lines recapping nullish semantics — Also a coordination note: #10938 is still open and its Risk: no elevated risk signals — no revert-correlated paths, and a comment-only diff has no runtime surface. Note the references are non-closing ( Moving on to code review. 🔍 中文说明感谢贡献！也感谢把它从 #10938 拆出来——25 行注释改动没理由压在一个 1542 行的视觉改动后面。 模板完整 ✓（所有标题齐全，纯注释 diff 的 Before/After 正确写了 问题： 既不是已观测到的 bug，也不是理论性加固——而是一处文档缺口，且这个缺口在当前代码里确实存在。我在 base 方向： 对齐，而且理由比 PR 描述里写的更硬。这个决定已经对外公布过—— 规模： 触及核心路径（ 方案： 一个真诚的问题，不是阻塞项。用两个注释块共 25 行去注解一段 15 行的合并逻辑，超出了 why 本身的需要。第一块（数据模型 vs 呈现、"两种方言"）是真正承重的部分；第二块花了好几行复述 nullish 语义—— 另外提醒协同问题：#10938 仍然开着，它的 风险： 无升级风险信号——没有命中 revert 相关性高的路径，纯注释 diff 也没有运行时面。另外关联用的是非关闭关键字（ 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewFor a comment-only diff the review is fact-checking, so I read every claim against the base tree at Claims that check out:
Two precision nits — non-blocking, but this PR's whole deliverable is prose accuracy, so they're worth a pass:
On volume (raised in Stage 1, restating as a review note, not a blocker): the second block's mechanics recap is the part that can go stale without anyone noticing, precisely because it is true of JS rather than of this code. The first block is the part that earns its lines. Test evidenceStatic verification only — I read the claims against the base tree; I did not build or run anything from this PR. There is nothing to run: the diff contains no executable code, so no test could observe it. Below is the PR's own CI, fetched once for Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Nothing is red. The Sandboxed lane: deliberately not named. 中文说明代码审查纯注释 diff 的审查本质就是事实核查，所以我把每一条论断都对照 base 核对通过的论断：
两处精确性问题——非阻塞，但这个 PR 的交付物就是文字准确性，所以值得再过一遍：
关于篇幅（Stage 1 已提，这里作为审查意见重述，非阻塞）：第二个注释块里复述语言语义的部分恰恰是那种会过期而无人察觉的内容，因为它描述的是 JS 而不是这段代码。第一个块才是真正值回行数的那个。 测试证据仅静态核查——我把论断对照 base 树读过，没有构建或运行本 PR 的任何代码。也没有可运行的东西：diff 不含可执行代码，任何测试都观测不到它。下面是本 PR 自己的 CI，针对 表中无红。真正与这个 diff 相关的是 沙箱验证通道： 刻意不点名。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — comment-only, every claim fact-checked against the base tree and holding; the two precision nits and the verbosity question are worth a pass but change nothing at runtime. Stepping back: my independent proposal from the title alone was one short block at What settles it for me is that the rationale is not being invented here. The two nits in Stage 2 are real but small. The "non-empty" enumeration misses the old-entry-with- Six-months test: I'd thank them for the first block and skim the second. Am I being a pushover? I don't think so — I checked the claims rather than accepting the framing, found two that don't quite hold, and I'm saying so on the record while still approving, because the defect class here is prose drift, not behaviour. On the pattern question: this author has ~30 PRs open and filed three in two minutes, which is exactly the volume that should make a gate more skeptical, not less. Judged on its own, this one holds up — it is the smallest of the three, it is a genuine split rather than a re-cut of the same diff, and it documents a decision the repo already made public. CI was still running at review time — 中文说明Confidence: 4/5 —— 纯注释改动，每条论断都已对照 base 树核查且成立；两处精确性问题和篇幅问题值得再过一遍，但对运行时毫无影响。 退一步看：只给我标题时，我自己的方案是在 真正让我下决心的是：这套理由不是在这里现编的。 Stage 2 里的两处问题真实存在但很小。"非空"的枚举漏掉了旧条目带 六个月后的检验：第一个块我会感谢作者，第二个块我会扫一眼就过。我是不是在当老好人？我不认为——我是去核对了论断而不是接受它的叙述框架，找出了两条不完全成立的，并且在仍然批准的同时把话写在明面上，因为这里的问题类别是文字漂移，不是行为。关于"模式"这一问：这位作者有约 30 个开着的 PR，两分钟内提了三个，这种量恰恰应该让 gate 更怀疑而不是更松。单独评判这一个，它站得住——是三个里最小的，是真正的拆分而不是同一份 diff 换个切法，而且它记录的是仓库已经公开做出的决定。 审查时 CI 仍在运行—— — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- DUP-1 'non-empty blockedBy' enumeration misses the stored-[] re-stamp — already reported (comment 5550326425)
- DUP-2 'reaches every session' ignores todo_write opt-in gate — already reported (comment 5550326425)
Not reviewed: reverse audit — stopped before round 6 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：反向审计——评审时间预算不足，未能开始第 6 轮。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| // because it is ungated (see `hasActivePlan` above): it changes an | ||
| // update only when all three hold — the previous plan still has an | ||
| // unfinished item, the same id carried a non-empty `blockedBy`, and | ||
| // this call omits `blockedBy` for it. A plan that never used |
There was a problem hiding this comment.
[Suggestion] The blast-radius comment bills itself as the "exact blast radius" of the omitted-blockedBy preservation (and the block above points here: "see the preservation merge below for the exact blast radius"), but the enumeration omits two cases this review confirmed by running the code — both inside the very branch the comment describes.
(1) Cycle fallback. When the stamped (preserved) edges would close a dependency cycle, the fallback below discards the entire stamped list (candidateTodos = todos), so all three stated conditions can hold and preservation still not happen — and it is dropped for every todo in the call, not just the cycle participants. A model reversing the chain a←b←c while omitting blockedBy (the omission the quoted schema text invites) silently loses every omitted dependency in that call, including cycle-free entries any other update would have kept.
(2) No-op exit. When the three conditions hold exactly as written and nothing else in the list changed, the restored edges make finalTodos deep-equal oldTodos, and the call takes the isDeepStrictEqual short-circuit: the model gets only "Todo list is already up to date … Your todo list was not modified because it is already current." with no echo of the stored plan — while every other preservation case takes the write path, whose reply embeds todosJson and shows the model the edges it omitted. The model can walk away believing the list it sent (a unblocked) is what is stored and schedule a before b, while the persisted plan and the Web Shell dependency graph still show a blocked by b. No test covers preservation ∩ no-op: todoWrite.test.ts:617 always pairs the omission with a status change, and the no-op tests at :348/:806 send lists with no blockedBy at all.
Witness (probe against the unmodified PR tree at 2ccb332b1a, scratch worktree):
(1) entry d satisfies all three stated conditions; arms differ only in the caller's cycle-closing edges:
ARM A stored: [...,{"id":"d",...,"blockedBy":["a"]}] ← preserved
ARM B stored: [...,{"id":"c","content":"Task C","status":"pending"},{"id":"d","content":"Task D","status":"pending"}] ← whole stamped list dropped
(2) stored [{a pending blockedBy:["b"]},{b pending}]; call omits blockedBy for a:
ARM A (nothing else changed): WRITE_CALLS=0; LLM_MENTIONS_BLOCKEDBY=false; "Todo list is already up to date. ..."
ARM B (+ one status change): WRITE_CALLS=1; LLM_MENTIONS_BLOCKEDBY=true (reply embeds "blockedBy":["b"])
flips (reverted mutations): hasActivePlan=false → WRITE_CALLS 0→1; echo appended to no-op reply → LLM_MENTIONS_BLOCKEDBY false→true
| // because it is ungated (see `hasActivePlan` above): it changes an | |
| // update only when all three hold — the previous plan still has an | |
| // unfinished item, the same id carried a non-empty `blockedBy`, and | |
| // this call omits `blockedBy` for it. A plan that never used | |
| // because it is ungated (see `hasActivePlan` above): it changes an | |
| // update only when all three hold — the previous plan still has an | |
| // unfinished item, the same id carried a non-empty `blockedBy`, and | |
| // this call omits `blockedBy` for it — unless the stamped edges would | |
| // close a cycle, in which case the fallback below drops the whole | |
| // stamped list and stores the caller-sent edges as-is. When the three | |
| // hold and nothing else in the list changed, the restored edges make | |
| // the result deep-equal the stored plan, so the call takes the | |
| // unchanged short-circuit below and the reply echoes none of what was | |
| // preserved. A plan that never used |
If you would rather remove hazard (2) than document it, echoing the preserved entries in the no-op reply is a behaviour change beyond this comment-only PR — verification found it would not break the pinned no-op contract (todoWrite.test.ts:364, :368-373, :376-378 all survive it). The wording above must keep the cycle fallback whole-list (candidateTodos = todos;, todoWrite.ts:374): non-cycle entries do not keep their preserved edges when it fires.
中文说明
影响面注释自称是省略 blockedBy 时保留逻辑的"exact blast radius"（上方注释块也指向这里："see the preservation merge below for the exact blast radius"），但这份枚举遗漏了本次评审通过实际运行代码确认的两种情形——都发生在这段注释所描述的分支内部。
（1）环回退。当被盖章（保留）的依赖边会构成环时，下方的回退逻辑会丢弃整个被盖章的列表（candidateTodos = todos）。因此三个条件可以全部成立、保留却依然不发生——而且是对该次调用中的每个 todo 一起丢弃，不只是构成环的那些。模型在省略 blockedBy 的同时反转依赖链 a←b←c（正是被引用的 schema 文本所鼓励的省略）时，这次调用里所有被省略的依赖都会被静默清空，包括那些在任何其他更新中都会被保留的无环条目。
（2）no-op 出口。当三个条件恰好按字面成立、且列表中没有其他任何变化时，恢复的依赖边会让 finalTodos 与 oldTodos 深度相等，调用走 isDeepStrictEqual 短路：模型只收到 "Todo list is already up to date … Your todo list was not modified because it is already current."，其中没有存储计划的任何回显——而其他所有保留情形都走写入路径，其回复内嵌 todosJson，会向模型展示它省略掉的依赖边。模型可能因此相信自己发送的列表（a 无阻塞）就是存储状态，进而把 a 排在 b 之前；而持久化的计划与 Web Shell 依赖图仍然显示 a 被 b 阻塞。没有测试覆盖"保留 ∩ no-op"这一形态：todoWrite.test.ts:617 总是把省略与状态变化配对，:348/:806 的 no-op 测试发送的列表完全不含 blockedBy。
（探针输出为程序输出，保持原样，见上方 Witness 代码块。）
若你更愿意消除隐患（2）而不是记录它：在 no-op 回复中回显被保留的条目属于行为变更，超出这个纯注释 PR 的范围——验证确认它不会破坏已钉死的 no-op 契约（todoWrite.test.ts:364、:368-373、:376-378 全部存活）。上面的措辞必须保持环回退是整列表语义（candidateTodos = todos;，todoWrite.ts:374）：回退触发时，无环条目同样不会保留其依赖边。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Both cases confirmed, and the fix went the other way: rather than extend the enumeration, 2d6d780 drops the claim that produced the gap.
The block no longer bills itself as the exact blast radius, and the pointer to it ("see the preservation merge below for the exact blast radius") is gone with the block above. What is left states necessary conditions only:
// Preservation only fires when the previous plan still has an
// unfinished item and the same id carried a non-empty `blockedBy`
// that this call omits; `[]` is not nullish, so it still clears.That survives (1) — the cycle fallback discarding the stamped list means the three conditions are not sufficient, which "only fires when" never asserted. The fallback also keeps its own seven-line explanation fifteen lines below, in the same branch and visible in the same screenful, so restating it here would have been the third copy of the same fact.
I did not take the suggested wording because the whole point of this revision was length: the PR owner asked for the comments to be cut, and the two blocks went 25 lines → 8. Adding the two clauses back would put most of that back.
(2) is not a claim this comment makes — it is a real hazard about what the reply shows the model, not about when preservation fires. It is worth fixing, but echoing the preserved entries in the no-op reply is a behaviour change with a test to add (preservation ∩ no-op, which as you note nothing covers), so it does not belong in a comment-only PR. Noted in Risk & Scope as out of scope; happy to open the follow-up.
中文说明
两种情形都确认成立，修法反过来：2d6d780 没有去扩充枚举，而是撤掉了产生这个缺口的断言本身。
这段注释不再自称 "exact blast radius"，上方指向它的那句（"see the preservation merge below for the exact blast radius"）也随上一段一起删掉了。剩下的只陈述必要条件（见上方代码块）。
这样就绕开了（1）：环回退会丢弃被盖章的列表，说明三个条件并不充分，而 "only fires when" 从未断言充分性。而且环回退在下方十五行处保有它自己的七行解释，处于同一分支、同一屏内，在这里再复述一遍就是同一事实的第三份副本。
我没有采用建议的措辞，因为这次修订的全部目的就是长度：PR owner 要求精简注释，两段从 25 行减到 8 行，把那两个从句加回去会把削减掉的大部分又装回去。
（2）不是这段注释所作的断言——它是关于回复向模型展示了什么的真实隐患，而不是关于保留何时触发。它值得修，但在 no-op 回复中回显被保留的条目属于行为变更，还需要新增测试（你指出无人覆盖的 保留 ∩ no-op 形态），因此不属于一个纯注释 PR。已在 Risk & Scope 中标注为范围之外；需要的话我来开这个后续 PR。
The two blocks said in 25 lines what fits in 8. Keep the parts a reader cannot get from the code — that the missing `isSessionWorkflowEnabled()` check is deliberate because dependencies are data-model semantics rather than presentation, and that `[]` still clears — and drop the restatements of the expressions directly below them.
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Approved at head abaeb085.
History. The one open Suggestion is resolved in substance on this head. git grep -n "blast radius\|exact" over packages/core/src/tools/todoWrite.ts at abaeb085 returns nothing: the phrase that over-promised ("exact blast radius") and the pointer to it are both gone, as the author's reply described. What remains states necessary conditions only — "Preservation only fires when the previous plan still has an unfinished item and the same id carried a non-empty blockedBy that this call omits" — which is not contradicted by either hazard the reviewer demonstrated: the cycle fallback that replaces candidateTodos wholesale, or the deep-equal no-op exit that echoes nothing.
Independent check. Both comment blocks are accurate against the code: isSessionWorkflowEnabled appears nowhere in this file outside the new comment, so hasActivePlan really is ungated; the blockedBy schema description is a static string in the tool schema, also ungated; and [] is indeed not nullish, so an explicit blockedBy: [] clears stored edges rather than falling through to preservation. The diff is eight added comment lines with no behavior change, so no test coverage is missing here and nothing needed re-pinning. No new Critical found.
One thing worth its own issue rather than this PR: the no-op path can leave the model with a reply that shows neither the preserved edges nor the stored plan, which is a model-visible divergence from what got persisted. Removing the over-claim was the right call for a comment-only change; the hazard itself still deserves a follow-up.
CI facts. Test (ubuntu-latest), Lint & Static and the no-AK integration lane were still queued when I submitted; nothing red. Review decision above is based on the code.
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Reviewed at head abaeb085 (the merge-from-main delta touches nothing this PR owns). No historical blocking issues exist on this PR — the only review findings were Suggestion-level and were resolved in substance at this head (the over-promising "exact blast radius" claim and its pointer were dropped in 2d6d780), and my Critical-only pass finds none.
For a comment-only diff the review is fact-checking, so I verified both claims against this head's packages/core/src/tools/todoWrite.ts:
- Claim 1 (no gating):
isSessionWorkflowEnabledappears nowhere in this file except inside the new comment itself — neither theblockedBypreservation logic nor the schema description ("Todo IDs that must be completed before this item...") is gated, exactly as the comment states. The rationale (plan data-model semantics vs. a visualization switch) matches the code's behavior: the sametodo_writecall stores the same plan regardless of the flag. - Claim 2 (preservation trigger): verified against the implementation — preservation requires
hasActivePlan(oldTodos.some(status !== 'completed')), reads the priorblockedByonly for the sametodo.id, and only when this call omits the field (todo.blockedBy ?? ...); an incoming[]is not nullish, so it passes through the coalesce and clears the stored dependencies, matching both the comment and the schema description's "use [] to remove them". The stale-edge filter (only re-injecting dependencies whose ids survive the update) and the preserved-cycle fallback are consistent with the surrounding comments.
No code, behavior, schema, or test change — zero runtime surface; the residual wording-precision nit (a stored [] re-stamp corner that is behaviorally invisible) is Suggestion-class and does not gate per review policy.
CI at this head: no failing or cancelled checks at review time; Test (ubuntu-latest, Node 22.x), Lint & Static, the no-AK integration lane and review-pr are still pending, which does not gate this review per policy.
Conflict was `todoWrite.ts`, both hunks the same cause: the two comment blocks this branch added were split out into #11078, trimmed from 25 lines to 8 during review, and merged. Resolved to main's side, so the file is now identical to main and drops out of this PR's diff — keeping the branch's older, longer version would have reverted a merged change.
6 participants
What this PR does
Two short comments in
todoWrite.ts: one athasActivePlanrecording that the missingisSessionWorkflowEnabled()check is deliberate, one on the preservation branch stating when preservation fires and that[]still clears.Comment-only. No behaviour, no signatures, no tests changed.
Why it's needed
The omitted-
blockedBypreservation and theblockedByschema text both reach sessions with Session Workflow disabled, and nothing in the file said that was on purpose. ThehasActivePlanderivation sits bare, and the only nearby comment explains the dangling-edge filter rather than the gating decision, so the next reader reasonably concludes a gate was forgotten and "fixes" it.The reasoning worth keeping is that these are plan data-model semantics rather than presentation. Gating them on a visualization switch would give
todo_writetwo dialects: the same call would store a different plan depending on whether anyone happens to be looking at the graph.Split out of #10938, where it sat behind a large visual change it has nothing to do with.
Revised after review (2d6d780): the first revision ran 25 lines and billed its second block as the "exact blast radius" of the preservation. Review showed that enumeration was not exact — the cycle fallback at
todoWrite.ts:374can discard the whole stamped list even when every stated condition holds. Rather than extend the enumeration, the claim was dropped: the comment now states only the necessary conditions, which the cycle fallback does not contradict, and the fallback keeps its own explanation fifteen lines below. 25 lines → 8.Reviewer Test Plan
How to verify
hasActivePlanalone with noisSessionWorkflowEnabled()consultation, and[]survives the??because an empty array is not nullish.git show --stattouches one file, every added line a//comment.Evidence (Before & After)
N/A — comment-only.
Tested on
Environment (optional)
Prettier clean. No test run: the diff contains no executable code.
Risk & Scope
hasActivePlan, the preservation merge), so a change to either makes the comment visibly wrong rather than quietly misleading.isDeepStrictEqualshort-circuit echoes nothing back to the model) is a behaviour change and belongs in its own PR.Linked Issues
Refs #8583, #10938.
中文说明
这个 PR 做了什么
在
todoWrite.ts中加两段简短注释：一段在hasActivePlan处，记录这里没有isSessionWorkflowEnabled()检查是刻意的；一段在保留分支上，说明保留在什么条件下才会触发，以及[]仍然是显式清空。纯注释。无行为改动、无签名改动、未改任何测试。
为什么需要
省略
blockedBy时的保留逻辑，以及那段blockedBy的 schema 描述，都会作用到关闭了 Session Workflow 的 session，而文件里没有一个字说明这是有意为之。hasActivePlan的推导光秃秃地摆在那里，附近唯一的注释解释的是悬空依赖的过滤，而不是这个 gate 决策，于是下一个读到的人会合理地判断这里漏了一道 gate，并去"修"它。值得留下的理由是：它们属于 plan 的数据模型语义，而不是呈现。把它们 gate 在一个可视化开关后面，会让
todo_write产生两种方言——同一个调用会因为有没有人正在看图，而存下不同的 plan。从 #10938 拆出：它原本压在一个与它毫无关系的大型视觉改动后面。
评审后修订（2d6d780）： 第一版有 25 行，并把第二段自称为保留逻辑的 "exact blast radius"。评审指出这份枚举并不精确——
todoWrite.ts:374的环回退会在所有列出的条件都成立时，仍然丢弃整个被盖章的列表。这里没有去扩充枚举，而是撤掉了这个断言：注释现在只陈述必要条件，环回退与之并不矛盾，而环回退本身在下方十五行处保有它自己的解释。25 行 → 8 行。评审者验证计划
如何验证
hasActivePlan上，没有查询isSessionWorkflowEnabled()；[]能穿过??，因为空数组不是 nullish。git show --stat只涉及一个文件，新增行全部是//注释。证据（改前 / 改后）
不适用——纯注释。
测试环境
运行环境（可选）
Prettier 通过。未跑测试：diff 中不含可执行代码。
风险与范围
hasActivePlan、保留合并），因此其中任何一处发生变化，注释都会明显变错，而不是悄悄误导。isDeepStrictEqual短路的保留不会向模型回显任何内容）属于行为变更，应当另开 PR。关联 Issue
Refs #8583, #10938.