Skip to content

fix(pr-review): suppress unchanged merge-readiness work - #4881

Merged
huangruiteng merged 2 commits into
mainfrom
codex/pr-review-readiness-observation
Sep 22, 2026
Merged

huangruiteng merged 2 commits into
mainfrom
codex/pr-review-readiness-observation

Conversation

@huangruiteng

@huangruiteng huangruiteng commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

Problem

pr-review discarded merge-readiness outcomes after each run, so a valid exact-head approval was selected again on every heartbeat even when no material PR state had changed. Re-reading the same merged/open PRs consumed queue capacity and made the automation appear stuck.

Change

  • persist a bounded, Goal-scoped, public-safe merge-readiness observation keyed by repository and exact head;
  • fingerprint the material readiness inputs: head/base, PR and draft state, review conclusion, CI, merge state, and review threads;
  • suppress only an unchanged exact-head readiness action and surface material transitions for a fresh decision;
  • require --goal-id when recording merge-readiness state;
  • keep changed-head PRs unsuppressed and parallelize only the observed thread readbacks.

This remains observation/cache state; the existing review conclusion and merge-readiness builder stay authoritative.

Validation

  • 102 focused pytest cases passed.
  • PR-review public command smoke passed.
  • Ruff passed for every changed Python file.
  • Semantic vocabulary and maintainability ratchets passed at the existing budgets.
  • Full standard premerge canary passed: 19/19 selected checks, no warnings, skips, manual holds, or boundary findings.
  • Change-quality receipt: cqr_0eceb424d347d33a98fa.

Merge decision

This changes control-plane queue behavior, so the review is recorded on the exact head before merge.

Review follow-up

The first review requested changes on two points; both are addressed here. Verified head: e1293cc09d1a6228f22991d5d088193c023a701b.

  1. Canonical guidance still showed the bare readiness invocation. The mandatory --goal-id makes the previous command fail deterministically, right before the observation this PR exists to persist. AGENTS.md (the canonical self-merge gate) and the capability README's review rubric now pass --goal-id GOAL, and examples/pr-review-command-smoke.py gained a durable assertion that every canonical guidance file mentioning --check-merge-readiness also carries --goal-id. The assertion was mutation-checked by reverting AGENTS.md to the bare form and confirming the smoke fails.
  2. The branch conflicted with main and its required CI was red. The branch is rebased onto the current main, and the one real conflict in loopx/cli_commands/pr_review.py keeps both behaviours: main's default-open --state resolution (resolved_state_filter) and this branch's Goal readiness observation load/writeback.

Re-validated after the rebase: 76 focused queue/github-scan/configuration tests, examples/pr-review-command-smoke.py, Ruff (CI scope and every changed Python file), mypy, git diff --check, and loopx canary premerge (19 selected checks, 0 failures).

@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.

Request changes conclusion (author-owned PR; GitHub blocks formal self-review)

审阅对象:4881@42fdbc04c6baaadf2a0edc020c8eb9818b356e22

动机

这个 PR 解决的是一个真实且反复出现的队列问题:某个 exact head 已经做过 merge-readiness qualification,但只要后续 heartbeat 没有可复用的持久 receipt,队列就会再次生成同一条 qualify_pull_request_merge_readiness,占用审阅注意力并表现成“卡在同一 PR”。

目标边界是合理的:第一次 readiness 检查记录一个 Goal-scoped、public-safe、bounded 的 observation;之后只有 exact head、base、PR/draft/merge state、独立审阅结论、required checks、unresolved threads 或 wait_for_ci 发生变化时才重新打开 qualification。这个 observation 只能抑制重复工作,不能替代 readiness 决策,更不能赋予 approve/merge 权限。

改动思路

整体架构方向正确:

  1. readiness_material_state 把现有 readiness 输入投影为稳定、可比较的 material state;
  2. record_readiness_observation 在 Goal 下原子写入 observation,按 repository/PR/exact head 去重并将总量限制为 200;
  3. materialize_review_execution 只在 action 是 qualify_pull_request_merge_readiness 且 fingerprint 完全一致时抑制动作;缺 observation、字段变化或状态不完整时都 fail-open,继续 qualification;
  4. readiness 的真正判断仍由已有 build_merge_readiness / review-conclusion 逻辑负责,没有形成第二套权威;
  5. 为了让 observation 有明确归属,--check-merge-readiness 新增了强制 --goal-id 约束。

这比进程内 memoization 更可靠,也比把 cache 塞进 Todo 更符合 authority 边界。代码量虽不小,但与“跨 heartbeat 持久化 + 并发安全 + compaction + material-change reopening”所需机制基本相称。

具体改动

关键代码讲解

  • loopx/capabilities/pr_review_queue/readiness_observation.py:42readiness_material_state 只纳入会改变 readiness 结论的结构化字段,不持久化 PR body、原始日志、凭据或本机路径。canonical JSON + fingerprint 适合做 derived observation,而不是新的状态权威。
  • loopx/capabilities/pr_review_queue/readiness_observation.py:183record_readiness_observation 使用现有 lock/atomic-write 原语,替换同一 PR 的旧 head observation,并做 200 条上限 compaction。缺失 observation 时不会误判“已完成”。
  • loopx/capabilities/pr_review_queue/selection_execution.py:70:只抑制 readiness qualification;其他 review action 不受影响。匹配条件包含 repository、PR/exact head 和完整 material fingerprint。
  • loopx/cli_commands/pr_review.py:287--check-merge-readiness 在没有 --goal-id 时直接失败,随后在完成 exact-head qualification 后写入 observation。这是正确的 state ownership 要求,但也是一个需要完整迁移调用方/说明面的强制 CLI 变更。

我实际跑了以下验证:

  • tests/capabilities/test_pr_review_queue.pytests/test_pr_review_github_scan.py 的新行为覆盖通过;和选定 lifecycle case 合跑时为 69 passed / 1 failed
  • uv run --extra test python examples/pr-review-command-smoke.py 通过。
  • changed Python/test/smoke paths 的 Ruff 检查通过。
  • 上述唯一 lifecycle failure 在 PR 的 merge base 4c0a0174 上同样复现,而当前 main 已通过,因此它不是这组 readiness 代码直接引入的回归,但说明该分支需要 rebase 获取主干修复。
  • git merge-tree --write-tree HEAD origin/main 确认 loopx/cli_commands/pr_review.py 有真实 content conflict;GitHub 同样报告 mergeStateStatus=DIRTY

对主干的风险

Findings

  1. [P1] 强制 --goal-id 后,自动加载的 maintainer 指令仍会发出必然失败的命令。

    loopx/cli_commands/pr_review.py:287-290 现在会在 --check-merge-readiness 缺少 --goal-id 时抛出 merge readiness requires --goal-id。但仓库根 AGENTS.md:125 仍要求执行:

    loopx pr-review --check-merge-readiness NUMBER@HEAD_OID
    

    loopx/capabilities/pr_review_queue/README.md:840 也保留了同样的 bare invocation。AGENTS.md 是自动加载的权威操作面;如果当前 head 合入,agent 按 canonical self-merge gate 操作会在写 observation 前确定性失败,恰好使本 PR 要解决的重复 readiness 工作无法收敛。

    **最小修复:**更新所有 canonical/installed invocation,明确传入 --goal-id GOAL;再补一个耐久 smoke/assertion,扫描或实际执行这些指令,防止 CLI obligation 与自动加载说明再次漂移。修复后至少重跑 rg -n -- '--check-merge-readiness' AGENTS.md loopx/capabilities/pr_review_queue/README.md skillsexamples/pr-review-command-smoke.py

  2. [P1] 当前 exact head 与最新 main 存在真实冲突,required CI 仍为红色。

    当前主干已经通过 PR #4878 在同一个 loopx/cli_commands/pr_review.py 中加入默认只扫描 open PR 的 _resolve_pr_review_state_filter 逻辑;本分支基于更早的 merge base。GitHub 报告 DIRTY,本地 merge-tree 也定位到同文件冲突。目前 test-shard (3)、aggregate pytestmerge-gate 失败。

    **最小修复:**rebase 到最新 main,在冲突处理时同时保留 main 的 default-open/state-filter 行为和本 PR 的 Goal observation load/writeback;然后重跑 focused queue tests、command smoke 和全部 required CI。不要把 branch-base 已修复的 lifecycle failure 标成“可忽略”,正确结果应是新 exact head 实际继承主干修复并变绿。

语义与 CI 对齐

本 PR 新增的是 compatibility-level 的 persisted observation,不需要扩展 approve/merge authority vocabulary;这部分语义是对齐的。当前违反的是调用契约与主干集成契约:机器强制 goal_id,而 canonical guidance 没有传;分支修改 CLI 时又没有包含主干已经建立的 default-open 行为。两处都属于交付 blocker,而不是文案润色或可延后清理。

我的整体评价

方案本身扎实:derived observation 与 readiness authority 分离,fail-open、exact-head/material-change reopening、bounded persistence 和 public/private boundary 都处理得比较好;我没有发现 observation 会自行授予 merge readiness 或隐藏已覆盖 material change 的证据。

但当前 exact head 还不能批准:一个 canonical instruction 会确定性调用失败,且分支无法干净集成最新 main、required CI 仍红。完成上述两项最小修复并在新 exact head 重跑验证后,这个 PR 应该可以快速复审。

English verdict: REQUEST_CHANGES - update every authoritative --check-merge-readiness invocation for the mandatory Goal id, then rebase and preserve current-main default-open behavior so the exact head is conflict-free and required CI passes.

@huangruiteng
huangruiteng force-pushed the codex/pr-review-readiness-observation branch from 42fdbc0 to 4c964e4 Compare September 22, 2026 10:05
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
…ocation

`--check-merge-readiness` now mandates `--goal-id`, but the canonical
self-merge gate in `AGENTS.md` and the capability README still showed the bare
invocation. An agent following those instructions would fail deterministically
before writing the readiness observation, which is exactly the repeated work
this branch exists to suppress.

Update both remaining invocations so every authoritative command carries the
Goal id, and extend the pr-review command smoke to fail when any canonical
guidance file mentions `--check-merge-readiness` without it. The new assertion
was mutation-checked by reverting AGENTS.md to the bare form.

Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
@huangruiteng
huangruiteng force-pushed the codex/pr-review-readiness-observation branch from 4c964e4 to e1293cc Compare September 22, 2026 10:06

@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)

Reviewed exact head: e1293cc09d1a6228f22991d5d088193c023a701b

动机

loopx pr-review --check-merge-readiness 是 LoopX 自合并前的规范门禁,但它在每次运行时都会重新材料化一遍 merge-readiness 审查工作。当一个 PR 的 head、base、CI 观察和授权状态都没有变化时,重复的 qualify_pull_request_merge_readiness 仍会被当作“新工作”调度,产生重复的 spend 和重复的唤醒,也让 readiness 的历史无法回答“这一次和上一次相比到底变了什么”。

本 PR 的目标是:让 merge-readiness 的重复调用在材料状态未变时变成一次可观测的“unchanged”观察,而不是一次新的资格化工作;同时在材料发生真实跃迁时(head/base 变化、CI 结果变化、外部投递确认变化、授权变化)保持原有语义不变。

改动思路

核心判断是:readiness 的“是否需要重新工作”取决于材料状态(material state),而不是调用次数。因此新增一个持久化的 observation 层,把每次 readiness 调用的材料指纹记录下来,并让选择执行器在指纹未变时短路。

三个边界被刻意守住:

  1. readiness 的权威(谁能判定 merge-ready)没有改变,仍然是原来的 qualify_pull_request_merge_readiness 路径与 receipt;本 PR 只增加 observation 与抑制,不新增第二套判定。
  2. 抑制只发生在 qualify_pull_request_merge_readiness 这一个 capability 上,其他审查工作不受影响。
  3. observation 的写入是加锁 + 原子替换 + 有界(200 行上限、同仓库同 head 覆盖),并且只接受同一 Goal 的 observation,外来 Goal 或不支持的 schema 会被拒绝读取,避免跨 Goal 污染。

具体改动

  • loopx/capabilities/pr_review_queue/readiness_observation.py(新增,248 行):readiness_material_state(第 42 行)从 head OID、base OID、CI 观察、授权/投递确认等构成材料状态;readiness_material_fingerprint(第 102 行)做稳定摘要;record_readiness_observation(第 183 行)在独占锁下原子写入,按仓库 + head 覆盖旧行并保留 200 行;read_readiness_observations(第 151 行)在读取时校验 Goal 归属与 schema 版本。
  • loopx/capabilities/pr_review_queue/selection_execution.pymaterialize_review_execution(第 70 行)只在 readiness capability 上比较观察结果,输出 observed_unchangedmaterial_transition,其余路径保持原行为。
  • loopx/capabilities/pr_review_queue/merge_readiness.py:在既有 receipt 上补充 material_state / material_fingerprint 字段,便于事后核对“为什么这一次被抑制”。
  • loopx/capabilities/pr_review_queue/github_source.py:补采 baseRefOid,使 base 变化能进入材料状态,而不是被静默漏掉。
  • loopx/cli_commands/pr_review.py(第 304 行):--check-merge-readiness 现在要求 --goal-id,缺失时以 merge readiness requires --goal-id 失败;资格化完成后写回 observation。
  • loopx/pr_review.pyloopx/pr_review_merge_readiness.py:把新的材料状态接入既有调用链,不改变对外契约的含义。

关键代码讲解

最值得读的是 readiness_material_state 的字段选择。抑制的正确性完全依赖“指纹是否真的覆盖了所有会改变 readiness 结论的输入”。本 PR 覆盖 head OID、base OID、CI 观察与投递确认;因此只要其中任何一项变化,指纹就变化,抑制就不会触发。反过来,如果未来新增一种会改变 readiness 结论的输入却没有进入材料状态,就会产生“该重算却观测为 unchanged”的漏抑制(false suppression)。这是本改动的主要风险点,也是 baseRefOid 被补进来的原因。

第二个关键是写入路径的幂等与隔离:独占锁 + 原子替换保证并发调用不会互相截断;同仓库同 head 覆盖保证历史不会因重复调用无限增长;200 行上限保证容量有界;读取时拒绝外来 Goal 与不支持 schema,保证一个 Goal 的 observation 不能被另一个 Goal 当作自己的状态使用。

第三是 --goal-id 的强制。observation 必须归属到某个 Goal 才有意义,因此缺失 Goal 时直接失败而不是写入一条无主记录,是刻意选择的 fail-fast,而不是兼容性回退。

对主干的风险

  • 抑制逻辑是新增分支,默认行为是“指纹未变则抑制”。如果材料状态的覆盖不完整(见上),会出现漏抑制或误抑制;本 PR 用 baseRefOid 补齐已知缺口,并把材料状态写进 receipt 以便事后审计。
  • --check-merge-readiness 现在强制 --goal-id:这是一处对调用方的行为收紧。为消除既有指引与实现的漂移,本 PR 同步更新了 AGENTS.md 的规范调用、capability README 的 rubric 与契约条目,并新增 examples/pr-review-command-smoke.py 中的 assert_merge_readiness_invocations_require_goal_id() guard(对 AGENTS.md 做变异验证时该 guard 会以 AGENTS.md must pass --goal-id to --check-merge-readiness 失败),确保文档不会再退回裸调用形式。
  • observation 引入了一个新的持久化文件。它是有界、加锁、原子写入的,且只在 readiness 路径被读写;失败的写入不会影响 readiness 的既有判定结果。

我的整体评价

本 PR 解决的是一个真实的浪费:把“重复运行 readiness 门禁”从“重复工作”降级为“可观测的 unchanged 观察”,同时保持 readiness 的判定权威不变。抑制条件被收敛到单一 capability,并且材料状态的组成被写进 receipt,使“为什么这一次没有重算”可以直接核对,而不是靠推断。

上一轮 review 提出的两个 P1 都已在当前 head(e1293cc09)上修复:规范指引与实现漂移已通过文档更新加持久化 guard 关闭;分支已 rebase 到当前 main,此前的 CI 红灯来自 main 上的 fixture 漂移,已在 main 修复后不再复现。

验证记录:76 个聚焦测试(tests/capabilities/test_pr_review_queue.pytests/test_pr_review_github_scan.pytests/capabilities/test_pr_review_configuration.py)、examples/pr-review-command-smoke.py、CI 范围内的 Ruff 与 mypy、git diff --check、以及 loopx canary premerge --from-git-diff --git-diff-base origin/main(selected=19, failures=0)均通过。

建议在所需 CI 全绿后合并。

English verdict: APPROVE - The observation layer correctly limits suppression to the merge-readiness capability, keeps the existing readiness authority and receipt as the single source of truth, makes every material transition explicit in the receipt, and both prior P1 findings (guidance drift now guarded by a durable smoke, and the stale main-derived CI failures) are resolved on head e1293cc.

@huangruiteng
huangruiteng merged commit fa15378 into main Sep 22, 2026
21 of 22 checks passed
@huangruiteng
huangruiteng deleted the codex/pr-review-readiness-observation branch September 22, 2026 10:55
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