refactor(todo): typed update intent and scope semantics - #4289
Conversation
|
Approval conclusion (author-owned PR; GitHub blocks formal self-approval) Reviewed exact head 动机本 PR 将 Todo 更新从松散字段拼装收敛为 typed update intent,并明确作用域、字段存在性和协调命令回执。目标是让规划字段、监控元数据、所有权/租约和终态校验由正确的边界负责,同时保留旧调用方的兼容行为。 改动思路改动在 Python 侧一次构建 canonical intent,再由原生 provider 处理被提升的规划字段;协调层复用共享 mutation decision 和 receipt。TypeScript 字段更新改为基于字段是否显式出现来区分 具体改动
对主干的风险我未发现阻塞性问题。精确 head 上的 focused Python 测试 3/3 通过,TypeScript 测试 20/20 通过;基线 authority/coordination 套件 48/48 通过。仓库检查中 SonarCloud 是唯一报告的非阻塞失败,不改变本次行为结论。剩余风险主要是未来新增字段需要继续声明其 intent/owner,避免重新回到隐式字段路由。 我的整体评价实现与现有控制面边界一致,行为变化有明确的 typed contract 和回执,且正负路径均有验证。建议合入该精确 head;后续可在新增 Todo 字段时继续要求 intent 分类与对应拒绝测试。 English verdict: APPROVE — no blocking finding on exact head |
huangruiteng
left a comment
There was a problem hiding this comment.
Request changes conclusion (author-owned PR; GitHub blocks formal self-review)
审阅对象:PR #4289,exact head b8345f13b0d81158d836fed5dc371cd08171f39b(base ff4c7bdeb57bdc48fc829dbb1bc00487ae22f45b)。本次按当前 policy-v3 对该 head 做独立完整评审,不继承其他 head 的结论。
动机
本 PR 将 Todo 的普通 metadata 更新统一成一份显式 planning intent,补齐 status/evidence/reason、任务声明、绑定/排除和显式清除的 typed provider 路径;终态 completion、Monitor observation 仍保留在各自 effect-owned 路径。这样可以消除 Python CLI 与 TypeScript planner 间的重复字段路由,并让空字符串、空集合、false 与省略保持不同语义。改动覆盖 todos.update_goal_todo、canonical provider update、native/public/field planners、legacy line adapter,以及对应的 native/production-scale fixtures。目标边界合理,但新增的 single-agent compatibility 分支没有保留原有 ownership/exclusion 拒绝规则,形成主干级安全回归。
改动思路
入口由 Python 的 build_canonical_update_intent 生成只包含显式请求字段的 intent;promoted Todo 进入 executeCoordinationTodoUpdate,在锁定的 canonical projection 上完成 request hash、authority admission、lease fence、typed plan、CAS commit 与 receipt replay。TypeScript 的 planNativeTodoUpdate 复用 planPublicTodoUpdate 和 planTodoFieldUpdate,不自行创建第二个存储或终态协议;Monitor 与 done 状态仍被拒绝并由原 effect-owned writer 处理。问题在 targetRejection:它改为调用 evaluateCoordinationTodoMutationDecision,而共享 authority 对 registered_agents.length <= 1 直接返回 single_agent_compatibility,不运行 registeredTodoMutationRejection。后续新增的保护只覆盖 actor 为空或 registry 为空,没有覆盖 non-null actor 的 claimed_by、excluded_agents 或 bound_agent。单 agent 兼容应只放宽“无 owner 的 actorless legacy edit”,不应把已归属或排除的 canonical Todo 当作任意注册 actor 可写。
具体改动
关键代码讲解
loopx/control_plane/coordination/todo_update.ts:188的targetRejection现在把角色、ownership、handoff 和 lease 事实送入 mutation decision。对于多 agent,现有 claim/exclusion/binding 保护仍能触发;对于单 agent,decision 返回 compatibility,随后仅检查空 registry/actor-null,于是 non-null actor 绕过 owner predicate。loopx/control_plane/todos/update_intent.py:45的build_canonical_update_intent保留显式空值和 clear switches,正确区分 omission 与 clear,并把普通 work/role metadata 交给 typed planner。loopx/control_plane/todos/native_update_plan.ts:26与public_update.ts复用 work/ownership/field planners,拒绝 terminal/Monitor escalation;field_update.ts:182以 presence 生成 scalar/collection updates,显式空 scalar 最终清除 canonical field。- Python
line_update.py、provider adapter、fixtures、mutants 与test_todo_update_intent.py补充了 legacy compatibility、production-scale matrix 和 parity coverage,但现有 negative tests 主要走双 agent 分支。
对主干的风险
[P1] Single-agent ownership/exclusion bypass(阻塞)
触发条件:canonical handoff 使用一个注册 actor,Todo 的 claimed_by 是另一个 actor 或 excluded_agents 包含当前 actor;调用普通 text/metadata update。当前 head 的 evaluateCoordinationTodoMutationDecision.authority 走 single-agent compatibility,
targetRejection 不再调用旧的 registeredTodoMutationRejection,因此 provider 返回 status=applied 并提交修改。我在 exact head 使用真实临时 FileAuthorityStore 重现:registered_agents=["agent-a"]、actor=agent-a、Todo 为 claimed_by="agent-b", excluded_agents=["agent-a"],合法 text update 的结果是 {"status":"applied"}。预期应为 typed rejection 且 provider head 不变。请在 non-null actor 路径恢复共享 ownership predicate(或让 authority 在 single-agent 下同样检查 exclusion/claim/binding),并新增单 agent 的 claimed/excluded/bound regression cases;actorless 兼容只应适用于真正 unowned Todo。
验证:exact-base 与 exact-head 的 git merge-tree --write-tree 均 clean;focused Python 121 tests、native TypeScript 66 tests、ruff 与 diff-check 通过。Remote checks 为 22 success、3 skipped,SonarCloud Code Analysis 失败(另一个 non-blocking Sonar workflow 通过);该 CI 问题也应在合入前确认,但不是本次 P1 的依据。
我的整体评价
该 PR 的 intent/planner 分层、显式 clear 语义、receipt/CAS 复用和测试矩阵总体方向正确,代码量与目标相称;不过 ownership admission 是 control-plane 的安全边界,single-agent compatibility 的 fail-open 使当前 exact head 不能安全进入主干。修复共享 predicate、补齐单 agent negative tests 后可重新 review;在此之前结论为 REQUEST_CHANGES。
English verdict: REQUEST_CHANGES — exact head b8345f13b0d81158d836fed5dc371cd08171f39b has a P1 single-agent ownership/exclusion bypass in targetRejection: a registered actor can mutate a Todo claimed by another actor or excluding that actor. Focused Python (121) and TypeScript (66) tests pass, but the real temporary FileAuthorityStore counterexample returns status=applied; restore the shared owner predicate and add single-agent regressions before re-review.
b038d23 to
83950ae
Compare
b8345f1 to
407806a
Compare
|
Self-refinement for exact head Fixed the P1 single-agent authority bypass from the earlier review: non-null single-agent actors now go through the shared ownership/exclusion/binding predicate, and actorless compatibility is limited to genuinely unowned Todo records. Added real FileAuthorityStore regressions for claimed-by-other, excluded, and bound-to-other cases; each rejects without mutating the authority head or writing a receipt. 冲突处理同时保留了生产 fixture 的 presentation/update cases,以及 journal-scan 和 receipt-replay 覆盖;没有重复带入已合并的 #4286 变更。 Validation:
The previous request-changes review targeted the superseded pre-rebase head; this exact head contains the ownership-fence repair and its regression coverage. Remote required checks are now running; proceeding with authorized admin self-merge once they finish. |
|
Remote CI found five compatibility regressions after the first review fix; they are repaired in head Root cause was at the Python-to-TypeScript authority boundary: legacy callers could omit Validation after the repair:
|
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>
f3936c3 to
bebab16
Compare
|
Final self-merge validation for exact head
Final self-merge decision: approve and squash-merge once the required GitHub checks for this rebased head finish successfully. No unresolved local blockers or manual holds remain. 最终自合并校验:当前精确 head 已 rebase 到最新主干;评论中指出的单 agent 所有权绕过已修复。完整门禁 18/18 通过,质量收据与 17 文件精确范围匹配;等待该 head 的 GitHub 必需检查完成后执行 squash merge。 |
|
Merged successfully via admin squash after all required checks and 已完成 admin squash 自合并,合并提交: |
Summary
This is a stacked follow-up to #4286 (
codex/authority-transaction-semantics). It does not change command receipts, terminal lifecycle, archive semantics, or the shared authority transaction introduced there.This PR makes the next Todo mutation seam typed and provider-owned:
noteretains its historical omission behavior);global_gate=falseis applied;Behavior changes and fixes
reason, work-declaration strings, and other scalar metadata distinguish omission from explicit clearing. Empty collections and false values remain explicit where their contract requires them.falsemarker.Validation
npm run typecheck:control-planenpm run test:control-planewith a disposable isolated PostgreSQL instance: 1482/1482 passedmypy: no issues in 22 source filesloopx canary premerge --from-git-diff --git-diff-base origin/codex/authority-transaction-semantics --goal-id loopx-meta: merge gate passed, 9 catalog + 8 risk-profile checks passed, no manual holdscqr_9442a19f7685a2f5ea49)Review notes
The original duplication was mainly duplicated knowledge at the Python/TypeScript boundary: the Python CLI assembled a second partial intent map and a truthiness-based promotion predicate, while the TypeScript planner independently owned the real field semantics. This PR leaves only the necessary cross-language transport allowlist and makes the TypeScript planner the single semantic owner. It also removes repeated actor/ownership checks by reusing the shared coordination mutation decision. No benchmark-specific or private runtime artifacts are included.