Skip to content

fix(todo): preserve executor exclusions on idempotent add - #4356

Merged
huangruiteng merged 1 commit into
mainfrom
codex/fix-monitor-successor-metadata
Sep 15, 2026
Merged

huangruiteng merged 1 commit into
mainfrom
codex/fix-monitor-successor-metadata

Conversation

@huangruiteng

Copy link
Copy Markdown
Collaborator

Why

A material quota monitor-poll can deduplicate its generated successor against an existing independent-review Todo. The legacy add path normalized an omitted excluded_agents field to [], so that idempotent replay silently removed the author exclusion. The next quota turn could then assign the PR author its own review.

What

  • Preserve the distinction between omitted executor exclusions (None) and an explicit exclusion list during Todo creation/replay.
  • Add a direct idempotent-add regression.
  • Add an end-to-end monitor-successor replay regression proving the independent-review exclusion remains intact.

Validation

  • python -m ruff check loopx/todos.py tests/control_plane/test_todo_mutation_authority.py
  • python -m pytest -q tests/control_plane/test_todo_mutation_authority.py (63 passed)
  • git diff --check

This is a control-plane integrity fix; it does not change review authority or permit self-review.

Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>

@huangruiteng huangruiteng left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval conclusion (author-owned PR; GitHub blocks formal self-approval)

详细中文评审

Exact head: 78f6645200c3fcb5d3a44268c6bd983695650549(base main = 1c7b40ea4)

动机

add_goal_todo 过去把"没传 excluded_agents"一路规范化成 [](normalize_todo_excluded_agents(None) -> []),而 add_todo_to_lines 对已存在 Todo 的判断是"值不是 None 就写",于是幂等重放会把既有的排除名单清空。真实触发路径就在 monitor 上:write_monitor_poll_todo_state 重新生成后继 Todo 时从不传 excluded_agents(loopx/control_plane/scheduler/monitor_poll_writeback.py:187),所以一次 material-change 轮询重放就能把"PR 作者被排除在独立评审之外"这条约束抹掉,下一轮 quota 就可能把评审派回作者本人。这正是仓库明令禁止的自评审风险,而且失败是静默的:重放看上去成功,只有读了记录才发现名单没了。

我在 origin/main 上做了反事实验证:把本 PR 新增的两个用例原样应用到 detached worktree(1c7b40ea4),两个都失败,其中端到端用例报 KeyError: 'excluded_agents'——记录里整个字段消失,证明缺陷真实存在且就是这条重放分支。

改动思路

入口是 add_goal_todo(loopx/todos.py:687);权威输入是既有 Todo 块里的排除元数据加 registry 校验;决策归属本来是 add_todo_to_lines(loopx/todos.py:577)的"按 presence 写",上游却先把省略变成了显式空表。改法就是在边界上保留三态:None 保持 None(不写),给了列表才校验并写入——这正是 update_goal_todo(loopx/todos.py:1296)早就在用的写法,本 PR 让 add 与 update 共用同一条省略语义,不再各自一套。更小的替代方案(只在 monitor 写入点补传 excluded_agents)只能救一个调用方,其他任何重放仍是数据丢失,所以没有采用;也没有新增参数、字段或清除开关。

具体改动

2 个文件、+93/−2,生产代码净 5 行:

关键代码讲解

  • effective_excluded_agents(loopx/todos.py:778):把"校验"变成条件调用,excluded_agents is None 时保持 None。不变量是——只要给了值仍走 require_registered_todo_excluded_agents 的注册校验,非法 agent 在任何写入前就抛错,所以"省略"与"非法"不会被混为一谈。
  • add_todo_to_lines(loopx/todos.py:577):本身没改,但它的含义变了。它一直是"is not None 才更新",过去被喂了 [] 才写坏;现在拿到 None 直接跳过,重放记录的元数据注释保持原样,metadata_updated 也是 false。
  • test_idempotent_add_preserves_existing_executor_exclusions(tests/control_plane/test_todo_mutation_authority.py:264):直接断言 already_exists=True、metadata_updated=False、todo_id 不变、名单仍是 [AUTHOR_AGENT],同时锁定返回值与落盘两份状态。
  • test_monitor_successor_replay_preserves_independent_reviewer_exclusion(tests/control_plane/test_todo_mutation_authority.py:2056):走真实调用链 write_monitor_poll_todo_state -> add_goal_todo,断言 successor_receipts 指向原 todo_id 且作者仍被排除。它保护的是"评审独立性"这条不变量,而不是某个输出字段的形状。

对主干的风险

最强回归场景就是本 PR 修的那个:material 轮询重放后继 Todo 时清空排除名单,导致作者被派回自评审;现在由上面的端到端用例钉住,回退会立刻变红。行为变化只有一条且必须说清楚:调用方若故意用省略来表示"清空",语义从"清空"改为"保持原值"——显式清空仍由 update 的 clear_excluded_agents 提供,仓库内没有任何生产调用方依赖旧的省略即清空。

我还单独验证了这条修复的边界,避免把它说大:promoted(canonical)create 适配器在 normalize_todo_metadata_for_write(loopx/control_plane/todos/contract.py:1223)里对 None 和 [] 一视同仁地丢弃,所以晋升路径从来不会因为这个默认值丢字段——我在 promoted fixture 上跑了一次幂等重放探针,返回 no_change 且排除名单保留。也就是说,缺陷与修复都属于 legacy writer,本 PR 不改变晋升路径行为。

残留风险按 P3 记录(不阻塞):promoted 重放目前没有断言钉住这一点,将来若有人把空表透传到 canonical create intent,同类丢失可能在没有测试报警的情况下回归;建议在适配器附近补一条 promoted 重放断言,或在实现里注明该字段的"省略即不可写"约定。

我的整体评价

observable_semantics 判定为 intentional_change_validated:新建 Todo、显式列表、非法列表三条分支都没变,唯一差异是"已存在 Todo + 省略"从清空改为保留";对照行和执行证据都在上面。authority_semantics 为 aligned:改动只影响一个字段的写入与否,不授予任何评审权限、不转移 claim、不跨 goal,excluded_agents 是约束而非授权。typed_state_rule 为 verified:判定用 presence 而不是 truthiness,没有新引入子串/散文式分类,false-positive 风险为零,残留的是一种误用假设(空表=清空),且该拼写仍由 update 显式提供。default_off_isolation 为 not_applicable:这是默认路径上的缺陷修复,没有可选开关,所以"关闭态"不存在;有效的反事实就是同一组测试在 origin/main 上失败。change_proportionality 为 proportionate:五行生产代码删掉一个默认值,换来 add 与 update 语义一致,可两行回退。

验证(同一 exact head):pytest -q tests/control_plane/test_todo_mutation_authority.py 63 通过;目标用例 2 通过;反事实基线 2 失败(证明缺陷);git diff --check 干净;该 head 上 CI 23 项通过、3 项按发布门跳过(presentation/publish-pypi/upload-release,与本次改动无关);边界扫描无本地路径、凭据或私有上下文。任何 head 变更都会重启本评审。

English verdict: APPROVE — exact head 78f6645200c3fcb5d3a44268c6bd983695650549 (base 1c7b40ea4). The add path no longer normalises an omitted excluded_agents to [], so an idempotent replay cannot erase the exclusion list that keeps a PR author out of their own independent review; the monitor-successor replay regression fails at origin/main and passes here, the full touched test file reports 63 passed, and the promoted create adapter was probed and is unaffected. One P3 advisory remains: the promoted replay behaviour has no assertion pinning it.

@huangruiteng
huangruiteng merged commit 56d2b05 into main Sep 15, 2026
26 checks passed
@huangruiteng
huangruiteng deleted the codex/fix-monitor-successor-metadata branch September 15, 2026 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant