docs(todos): state the user-role update validation gate in the contract - #3293
Conversation
Address the two non-blocking P2 notes from the huangruiteng#3291 review: - project-agent-todo-contract.md now says explicitly that a user-role `todo update --status done` on a todo with a declared validation command runs the same completion validation gate as `todo complete` and fails closed with a typed receipt, so the earlier non-terminal-update framing cannot be misread as a gate bypass. - the pre-lock gate in update_goal_todo drops the dead try/except around normalize_todo_status: it returns None for an invalid status rather than raising, so the except branch was unreachable and the None comparison already skips the gate unchanged. Signed-off-by: 牛瑞博 <912906590@qq.com>
huangruiteng
left a comment
There was a problem hiding this comment.
Review: docs(todos): state the user-role update validation gate in the contract
Reviewed exact head: af008d216edce58278d3392d01d2d112c7285a5f (docs/user-todo-update-validation-gate), base main.
动机
这是对 #3291 评审中两条非阻塞 P2 意见的直接回应:#3291 让 user-role 的 todo update --status done 也执行已声明的 validation gate,但契约文档仍以"todo update 用于非终态变更"为主框架,容易被误读为存在绕过路径;同时 #3291 引入的 pre-lock 门含有一段不可达的 try/except ValueError(normalize_todo_status 对非法状态返回 None 而非抛错)。本 PR 用 7 行契约说明 + 5 行死代码删除把这两处补齐,范围极小且精确。
改动思路
- 文档:在
docs/project-agent-todo-contract.md的完成契约段落后新增一段,明确 user-role 的 done 更新在声明了 validation 命令时与todo complete走同一道 gate、失败关闭并返回 typedvalidation_blocked_completionreceipt、不落盘;未声明者保持快路径。放在 agent 完成段落与--resume-when示例之间,位置与上下文衔接自然,直接消除前文的误读风险。 - 代码:
loopx/todos.py的 pre-lock 门去掉死 try/except,改为直接比较normalize_todo_status(status) == TODO_STATUS_DONE,并加注释说明normalize_todo_status不会抛错。非法状态比较结果为None == "done"→ 跳过 gate,随后仍由 in-lock 写入路径(apply_todo_update_to_lines第 199-201 行)抛出与之前完全相同的ValueError。
具体改动
关键内容讲解
新增文档段落的语义逐句与实现一致:validation_command 与 --validation-command-json 两种声明形式都会在 todo add 时归一化为存储字段 validation_command / validation_command_argv(loopx/todos.py:554-621),update_goal_todo 的 pre-lock gate 对两者同等执行(#3291 已实现),失败负载包含 validation_blocked_completion=True 且不提交 done。
关键代码讲解
update_goal_todo 中:
if normalize_todo_status(status) == TODO_STATUS_DONE:normalize_todo_status(loopx/control_plane/todos/contract.py:774-778)对合法值返回小写字符串、对非法值返回 None,从不抛 ValueError,因此删除 try/except 后:合法 done → gate 照常运行;非法状态 → 比较为 False → 跳过 gate → in-lock apply_todo_update_to_lines 抛出原样错误。行为逐字节等价,仅减少不可达代码。
对主干的风险
- 无行为变化:唯一 runtime 改动是删除不可达分支,25 个 completion-validation 测试(含 4 个 #3291 的 update-path 用例)本地全部通过。
- 文档表述准确,与 #3291 已合入(
cca1e8ef)的行为一致,不引入新的承诺。 - CI 现状:pytest 仍在运行;唯一失败是 Frontstage Pages 的
build检查,日志显示是 GitHub codeload 下载actions/configure-pages时 503/429(基础设施故障),与本 PR 的 docs + 死代码改动无关,需要重跑该 job 后合并。Sign-off / dependency-review / Release build 均通过。 - 非阻塞 P2:文档写的是
validation_command_json,而规范存储字段名是validation_command_argv(CLI 旗标才是--validation-command-json);建议改为"validation_command(或--validation-command-json声明的validation_command_argv)"以与 schema 完全一致,不影响当前可读性。
验证
- 本地:
tests/control_plane/test_todo_completion_validation.py25 passed。 git diff --check干净;diff 无凭据、私有路径或内部链接。- 变更仅 2 个文件、+11/−5,单 commit 含 Signed-off-by,无运行时 API 或 schema 变化。
整体评价
小而准的收尾 PR:把 #3291 的契约语义写进公开文档并清理不可达防御代码,改动量与其目的完全匹配,没有引入新抽象。代码层面无阻塞项;合并前只需重跑因 GitHub 基础设施故障失败的 Frontstage Pages job 并等 pytest 绿。
English verdict: APPROVE — this PR accurately documents the user-role todo update --status done validation-gate contract (closing the #3291 P2 documentation note) and removes the unreachable try/except around normalize_todo_status with provably identical behavior; 25/25 local completion-validation tests pass, git diff --check is clean. The only failing CI check (Frontstage Pages build) is a GitHub codeload 503/429 action-download failure unrelated to this diff and must be rerun before merge; pytest is still running. Reviewed at exact head af008d216edce58278d3392d01d2d112c7285a5f. Optional P2: use the canonical stored field name validation_command_argv in the doc sentence.
Per the huangruiteng#3293 review note: the contract sentence now references the stored field `validation_command_argv` (declared via the `--validation-command-json` flag) instead of the flag-derived alias `validation_command_json`, matching the todo schema exactly. Signed-off-by: 牛瑞博 <912906590@qq.com>
huangruiteng
left a comment
There was a problem hiding this comment.
Re-review: docs(todos): state the user-role update validation gate in the contract
Reviewed exact head: b22c27062df5690f97959fd1d39262b567233d36 (docs/user-todo-update-validation-gate), base main.
动机
本 PR 在 af008d21 的基础上新增了一个 commit(b22c2706),把上轮评审中提出的 P2 措辞建议落地:文档中的 validation_command_json 改为规范存储字段名 validation_command_argv,并注明其 CLI 声明形式是 --validation-command-json。这是对 #3293 评审意见的直接收尾,改动仅限文档一行。
改动思路
docs/project-agent-todo-contract.md 的 user-role done 更新段落现在准确对应实际存储契约:todo add 时 --validation-command / --validation-command-json 分别归一化为 validation_command / validation_command_argv(loopx/todos.py:554-621),update_goal_todo 的 pre-lock gate 对两者同等执行,失败返回 typed validation_blocked_completion receipt 且不落盘。loopx/todos.py 的死 try/except 删除与上一轮完全相同,行为逐字节等价。
具体改动
关键内容讲解
新增文档段落语义核对:
validation_command:legacy shlex 形式,直接存储。validation_command_argv:--validation-command-json声明的 JSON 字符串数组,存储为该字段。- 两者任一存在即走
run_completion_validation_gate;失败ok=False、changed=False、不提交done;未声明保持快路径。
文档表述与实现一致,消除了"非终态变更"框架可能造成的绕过误读。
关键代码讲解
update_goal_todo 中 normalize_todo_status(status) == TODO_STATUS_DONE 直接比较:normalize_todo_status 对非法值返回 None(loopx/control_plane/todos/contract.py:774-778),比较结果为 False 即跳过 gate,随后 in-lock apply_todo_update_to_lines 抛出与删除 try/except 前完全相同的非法状态错误。25 个 completion-validation 测试(含 4 个 update-path 用例)本地全部通过。
对主干的风险
- 自
af008d21起仅文档变化(+2/−1),无新运行时行为;上轮评审结论在本 head 依然成立。 - CI 现状(head
b22c2706):Sign-off、dependency-review、Frontstage Pages build 通过;Release Artifacts build 失败,日志显示是 GitHub codeload 下载actions/attest时 429/502(基础设施故障,与本次 docs+死代码改动无关);pytest 仍在运行。合并前需在 GitHub 恢复后重跑 Release Artifacts job。 git diff --check干净;diff 无凭据、私有路径或内部链接。
验证
- 本地:
tests/control_plane/test_todo_completion_validation.py25 passed。 - 权威 PR diff(2 文件,+12/−5):文档 +8/−0,
loopx/todos.py+4/−5,无其他文件。 - 单 commit 增量含 Signed-off-by,DCO 通过。
整体评价
新 commit 精确落实了上轮 P2 建议,文档字段名与实现 schema 完全对齐,改动量最小且无副作用。代码层面无阻塞项;合并前只需重跑因 GitHub codeload 故障失败的 Release Artifacts job 并等 pytest 绿。
English verdict: APPROVE — the new commit b22c2706 applies the previous review's P2 wording fix by using the canonical stored field validation_command_argv (declared via --validation-command-json); docs-only delta (+2/−1) since the last reviewed head, behavior unchanged, 25/25 local completion-validation tests pass. CI: DCO, dependency-review, and Frontstage Pages build pass; the Release Artifacts build failure is a GitHub codeload 429/502 action-download infra error unrelated to this diff and needs a rerun; pytest is still running. Reviewed at exact head b22c27062df5690f97959fd1d39262b567233d36.
Addresses the two non-blocking P2 notes from the #3291 review (merged as
cca1e8ef):docs/project-agent-todo-contract.md): the completion section now states explicitly that a user-roletodo update --status doneon a todo with a declaredvalidation_command(orvalidation_command_json) runs the same completion validation gate astodo completeand fails closed with a typedvalidation_blocked_completionreceipt instead of committing; undeclared todos keep the unchanged fast path. This prevents the "usetodo updatefor lower-level, non-terminal status changes" framing earlier in the file from being misread as a gate bypass.loopx/todos.py): the pre-lock gate inupdate_goal_tododrops itstry/except ValueErroraroundnormalize_todo_status— that helper returnsNonefor an invalid status rather than raising, so the except branch was unreachable and theNone == TODO_STATUS_DONEcomparison already skips the gate with behavior unchanged. A comment now records why no guard is needed.Tests:
tests/control_plane/test_todo_completion_validation.py→ 25 passed (the gate's four update-path cases included).Refs #3291, #3082.