refactor(todos): unify continuation evidence and completion readback - #4761
Conversation
huangruiteng
left a comment
There was a problem hiding this comment.
Reviewed head f0aa16c8690f1442a8119eae578b72661b90ea82.
Verdict: PASS for this bounded continuation-readback slice; maintainer merge required.
The typed Todo boundary is the only owner of successor resolution, handoff state and closure decisions. The existing Python succession presentation adapter and authority providers consume it. No new maintained Python/TS module pair is added. The public behavior changes are explicit: missing/self successors no longer close work, archive/filter selection preserves valid lineage, and query subsets cannot certify whole-source closure. No optional feature is activated and no provider default, grant, claim, lease or active Goal is promoted by this change.
Future-facing refactor applied: archive capture and readback share the inferred-edge index, and duplicate Python state/closure rules are removed. Derived evaluations are kept out of stored authority and public hot-path payloads. A digest verifies consistency, not authorization. This does not claim transitive acceptance or cycle freedom.
One advisory remains: the historical stale/handoff/closeout prose hint can overmatch narrative. Explicit booleans take precedence; writer migration and retirement remain in the existing RFC. The adjacent receipt/head race is fixed by a matching-receipt recheck, not by accepting caller validation on a state-only replay. Deterministic positive and negative cases cover all four providers. No actionable blocker found in the final scope.
Validation: before the final bounded replay fix, full TS suite 2,289 passed, zero failures; the service-environment skip separately qualified with 10/10 real PostgreSQL service tests. Python consumer suite 94 passed; real snapshot readback covered legacy/File/SQLite and authenticated PostgreSQL. Typecheck, configured mypy, Ruff and unchanged payload budgets passed. The earlier SQLite subprocess failure was followed by a passing isolated test and complete lower-concurrency rerun. Expanded-import mypy has identical base/head errors and is not a newly clean full-repository gate.
Quality receipt cqr_41ed5dbc90976d220cb2 verifies the final 24-file scope, fingerprint 41ed5dbc90976d220cb23da39e69975404f4dd331719de5bb8935eb09f9c6743; one bounded safe-fix pass applied. Long-term soak, complete projection recovery and default cutover remain unqualified by this PR.
Final delta qualification: PostgreSQL store suite 174/174 (zero skips), focused terminal/consumer tests 21/21, graph-provider readback 8/8 and deterministic receipt/head-race plus missing-receipt guards 4/4. Maintained Python/TS pairs remain 43/43 without raising the budget.
Final premerge: all 19 selected risk/catalog/public-boundary checks passed; exact-head quality enforcement is valid. No manual validation hold remains for this slice; maintainer merge and the separate cutover/durability gates remain.
huangruiteng
left a comment
There was a problem hiding this comment.
Request changes conclusion (author-owned PR; GitHub blocks formal self-review)
Exact head reviewed: f0aa16c8690f1442a8119eae578b72661b90ea82
动机
把 continuation evidence、completion readback 和 archive capture 收敛到一个 typed TypeScript succession owner,方向是对的。现状中 Python 与 TypeScript 分别推导相近的 succession 语义,长期会让 completion、handoff、claim/lease 与公开 Todo readback 出现不一致。
但这个 PR 的目标是重构并保持现有可观察语义,不是借重构改变公开 row schema,或者禁止仓库当前已经支持的 archive → recreate 生命周期。当前 exact head 在这两点上都有可复现的回归,因此不能批准。
改动思路
我沿真实调用链而不是只看新测试检查了这次收敛:
archive capture / active-state parse → succession_warning Python adapter → succession.ts typed decision → compaction / public summary → task lease、authority projection、handoff 和 completion consumer
设计中正确的部分是:succession 决策归 loopx/control_plane/todos/succession.ts,Python 只做输入输出适配,避免第二套规则权威。
当前问题出在集成边界:
- typed decision 把所有重复
todo_id都判成非法,却没有区分“同一逻辑 Todo 的归档 generation + 当前 active generation”和“真正冲突的双重 authority”; - Python adapter 直接在共享 item dictionary 上写入内部
succession_evaluation,而todo_summary又把这些 item 作为公开 parser rows 返回。
最小修复不是退回双重实现,而是保留 TS owner,同时补上 record generation/active precedence,并把 evaluation 放到 internal sidecar(或在每个公开边界稳定剥离)。
具体改动
我完整阅读了 succession.ts、succession_warning.py、archive_capture.ts、handoff_gate.py,以及 todo_summary.py 的相关变更、effect-runtime handler 注册、runtime shadow capture、terminal lifecycle 修复、新增测试和文档。24 个文件共 +958/-279。
新路径本身的聚焦验证通过:
tests/control_plane/test_succession_provider_readback.py与tests/control_plane/test_todo_succession_read_model.py:15 passed;tests/control_plane_ts/todo_succession.test.ts与tests/control_plane_ts/archive_capture.test.ts:22 个测试通过。
但真实既有 caller 的 base/head 对照揭示了两个 P1 blocker:
-
合法 archive + active identity reuse 会让后续读取/lease 崩溃。 既有
test_enabled_todo_public_facades_emit_post_commit_evidence完成 Todo、归档、再创建同 deterministic id 的 replacement,随后 lease read 经过list_goal_todos → compact_todo_group → evaluate_succession → evaluateTodoSuccession,在succession.ts:77-82抛出EffectRuntimeRejected: duplicate succession identity。succession_warning.py:187-206明确保留 archive/role 的重复 id,随后又把它交给“任何重复即拒绝”的决策,两端契约冲突。 -
内部 evaluation 泄漏到公开 parser row。
todo_summary.py:968-970对共享 items 调用evaluate_succession,succession_warning.py:204-206写入succession_evaluation,随后todo_summary.py:1112直接返回这些 items。既有 canonical-successor 流把 parser rows 交给authorityProjectionFixture后得到:AuthorityStoreProtocolError: fixture Todo domain record has unversioned fields: succession_evaluation。
同一组既有回归在 exact head 为 2 failed, 2 passed,在 exact base 916763e2c7fc9c19274f877dd234f7db6a65390c 为 4 passed in 93.07s;第二个错误还通过直接 Node 路径再次复现。这是 head 引入的语义差异,不是远程 CI 噪声。
对主干的风险
两个 blocker 都位于默认热路径:第一个会阻断 task lease acquisition,第二个会破坏使用稳定 Todo row contract 的现有 consumer。它们的危险之处正是“新测试全绿但真实 caller 失败”:新增 fixture 没有覆盖 deterministic id 的 generation reuse,也没有跨越公开 parser → canonical authority consumer 的边界。
最低修复要求:
- 为 succession input 提供稳定 record identity / generation,或定义 active/archive precedence;合法生命周期应通过,同时补一个真正 ambiguous duplicate authority 的负例,不能简单删除 duplicate guard。
- 不在共享公开 row 上持久添加
succession_evaluation;使用 internal sidecar,或确保parse_active_state_todos(...)[agent_todos][items]等所有公开边界都剥离它。 - 保留并跑通上述两个既有回归;增加直接断言公开 parser rows 不包含
succession_evaluation的测试。
修复后还应跑一轮较宽的 Todo/list/lease/handoff suite,因为这次重构横跨多个读路径。未来导向检查没有发现需要新增框架:正确的小重构就是进一步收紧 TS decision owner 与 public projection owner 的边界。
我的整体评价
结论:REQUEST_CHANGES。统一 typed succession authority 的架构方向值得保留,新聚焦测试也证明了核心 happy path;但当前实现尚未保持既有可观察语义。两个基线通过、head 失败的真实 caller 回归都必须在本 PR 内修复,不能把它们归为无关 CI 或留给后续。
English verdict: REQUEST_CHANGES - head f0aa16c regresses a valid archived-plus-active Todo identity lifecycle and leaks internal succession_evaluation into the public parser row contract; repair both boundaries and rerun the reproduced base/head callers before approval.
…ypeScript Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
…oviders Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
…oints Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
f0aa16c to
1dcc712
Compare
|
已 rebase 到 main 上一轮两项 P1 已在拥有规则的边界修复:
相同原始 caller 测试在旧 PR head 为 2 failed, 2 passed,基线 4 passed,修复版全部通过。最终 Python 相关 suite 263 passed;TS 全套 2333 passed / 1 failed / 0 skipped,唯一失败确认是本机空闲磁盘低于 capacity rehearsal 的 5 GiB 保留阈值,同一 capacity suite 在独立 8 GiB RAM-backed filesystem 上 5/5 passed。阈值未修改,这不作为耐久性或完整 D2 资格。真实隔离 PostgreSQL store/service 与其他 provider conformance 已通过;typecheck、mypy、Ruff、catalog 10/10、premerge selected 19/19 和公开边界检查通过。 当前仍等待本 head 的必需远端 CI;尚未作出合并决定。后续 exact-head 评审会覆盖整个 PR,而非只确认两个旧 finding 消失。 |
Completed work could appear closed with a nonexistent successor, or appear unfinished after its real successor was archived or filtered out. Handoff used a different resolver and ignored explicit successor lists. This PR makes continuation and closure readback use one typed policy across legacy, File, SQLite and PostgreSQL authority reads.
todo_done; version the internal capture request so old runtimes cannot silently omit the new edges.The existing Todo control plane owns this read policy; no capability or provider is added. CLI list, status/quota and manager detail entrypoints keep their existing configuration. Python reuses the existing succession presentation adapter rather than adding another maintained Python/TS module pair; the documented legacy prose replan hint remains until its writers emit the explicit flag. No provider default, Goal promotion, grant, claim or lease is changed. An existing successor establishes lineage, not Goal acceptance or cycle freedom.
Exact-head review and qualification are recorded below before handoff. Long-term durability soak, full projection-delivery recovery and default cutover remain separate acceptance gates; #4754 remains an independent transaction change. This is a complete continuation-readback slice, not completion of R5 or D1–D3.
Refs #4574, #3225, #3245.
Validation:
Final delta qualification: PostgreSQL store suite 174/174 (zero skips), focused terminal/consumer tests 21/21, graph-provider readback 8/8 and deterministic receipt/head-race plus missing-receipt guards 4/4. Maintained Python/TS pairs remain 43/43 without raising the budget.
Final premerge: all 19 selected risk/catalog/public-boundary checks passed; exact-head quality enforcement is valid. No manual validation hold remains for this slice; maintainer merge and the separate cutover/durability gates remain.