fix(pr-review): owe merge readiness for every approved open head - #4765
Conversation
GitHub blocks self-approval, so an author-owned approval is necessarily recorded as a `COMMENTED` review with the titled fallback. The queue keyed the merge-readiness qualification on the formal `APPROVED` state, so those heads counted as `current_head_concluded`: of 39 open heads on 2026-09-20, five held a valid approval and were behind or blocked, and the queue reported three actionable rows instead of eight. `pull_request_merge_readiness_v0` already decides this from the typed verdict, so the queue now agrees with the gate it routes to and an approved open head keeps owing `qualify_pull_request_merge_readiness`. An offline fixture may now declare `reviewer_login`, because a fixture without a reviewer identity cannot express an author-owned conclusion at all; the public smoke covers an approved head that degraded, one that is still clean, and a request-changes fallback that stays inventory-only. Validation: the queue packet on the live window (attention 3 -> 8, the five approved heads now in `review_sequence`), `--check-merge-readiness` for a degraded and a clean approved head, `examples/pr-review-command-smoke.py`, and `pytest -k pr_review` (164 passed, 12 skipped). Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
huangruiteng
left a comment
There was a problem hiding this comment.
Approval conclusion (author-owned PR; GitHub blocks formal self-approval)
Reviewed exact head: dfecb2849fde779d1d95aa3a7e2b63a662dd30e4.
动机
PR 队列的存在意义是回答"下一个该做的评审动作是什么"。current_head_concluded 这条 lane 的语义是"已有完整结论,只允许回读、不授权再评审",所以被错放进这条 lane 的头等于从操作者视野里消失。GitHub 禁止自我 approve,本仓库的产出又几乎全部是作者自有 PR,于是作者自有的 approval 只能以 COMMENTED + 固定标题落库;而分类规则读的是 review state:
if state == "OPEN" and str(conclusion.get("state") or "").upper() == "APPROVED":
return "qualify_pull_request_merge_readiness"COMMENTED 永远不等于 APPROVED,所以每一个作者自有 approval 都被判成"已结论"。这不是理论风险:2026-09-20 实测窗口 39 个 open head,队列报 3 个 attention,其中 5 个 head 持有有效 APPROVE 却分别是 4×BEHIND + 1×BLOCKED(4748/4755/4756/4758/4759)。一个 head 可以同时"已批准""不可合并""不可见"。
同一能力里的 build_merge_readiness(pull_request_merge_readiness_v0)早就是按 typed verdict 判定的,并且已经输出 author_owned_commented_approval。也就是说队列与它自己路由到的门禁结论不一致,这才是要修的 gap,而不是缺一个新的 recheck surface。
交付判定:这是一个完整、可独立评审、可回滚的行为切片——队列分类与已存在的 fail-closed 门禁对齐。它不新增第二份 "degraded" 定义,也不授予任何写权限。
改动思路
- 入口与决策边界:
materialize_review_execution→review_action_kind(分类的唯一 owner),下游core._candidate_action由它派生 lane、attention、候选 Todo 与review_sequence。 - 权威状态:
_review_conclusion已经产出 typed 结论(valid/state/verdict),作者自有回退还带固定标题。判定改用verdict,与build_merge_readiness同源。 - 边界保持最小:只有
state == "OPEN"且 valid 结论verdict == APPROVE才回到qualify_pull_request_merge_readiness;valid 的非 approval 结论仍是 inventory-only。 - 被否掉的方案(写进 todos 的原始设想):新增一个队列级 "drift set",只列出退化了的已结论头。它会把 "degraded" 的第二种定义放到 fail-closed 门禁旁边,而且仍然会让"已批准且干净"的头从唯一能告诉操作者"可以合了"的界面上消失。复用既有 action kind 只保留一条规则、一扇门。
- 离线 fixture 需要能表达作者自有结论:没有 reviewer 身份就无法构造该真实场景,因此 fixture 允许声明
reviewer_login,公共 smoke 才可能覆盖这个契约。
具体改动
关键代码讲解
review_action_kind(loopx/capabilities/pr_review_queue/selection_execution.py:24):判定条件由conclusion.state == "APPROVED"改为conclusion.verdict == "APPROVE",并写明平台禁止 self-approval 这一事实来源。行为差异只author-owned approval 一类:它们不再被计为已结论。_review_why_now(loopx/pr_review.py:1315):同一规则,使行的why_now与它的 action kind 一致(此前作者自有批准会显示"已有完整独立结论",与它现在要走的门禁矛盾)。load_pr_fixture(loopx/pr_review.py:341):返回值增加 fixture 声明的reviewer_login;两个 CLI 调用点同步更新。只有显式声明该字段的 fixture 行为改变,未声明的 fixture 与之前逐字节等价。examples/pr-review-command-smoke.py:新增 approved-open-heads 段——一个退化(BEHIND+ 失败检查)、一个仍然干净、一个 request-changes 反例,并对前两个头直接跑--check-merge-readiness断言 fail-closed 结果。tests/capabilities/test_pr_review_queue.py:新增 typed-verdict 表驱动用例(作者自有批准 / 正式批准 / 作者自有 request changes / MERGED / draft / invalid 回退路径);tests/test_pr_review_github_scan.py中两处编码旧默认的断言更新为断言新规则,其中"后续普通 COMMENTED 评论不得抹掉早先批准"这条语义被显式写进注释。
对主干的风险
- 失败路径追因:attention 3 → 8 是本 PR 的直接后果。多出的 5 行不是噪声:它们是需要动作的 head(4 个要 update-from-base,1 个需要 admin bypass 授权路径)。正式 approval 的行为没有变化,只有作者自有 approval 从"已结论"变成"欠门禁"。
- 误分类风险与最小修复:若某天出现 valid 且非 approval 的结论被判成 approval,只会来自
_review_conclusion放宽;该函数同时强制 state/verdict 对齐,并要求作者自有回退带精确标题,因此这里的verdict是可信的 typed 输入。 load_pr_fixture是签名变更(2 元组 → 3 元组)。调用点只有两个 CLI 位置与本仓库 smoke,已全部更新;没有改动 CLI 参数、持久化状态、协议字段或缓存。外部若直接 import 该 helper 会 breaking——它是loopx.pr_review的内部 helper,不在 capability 契约里。- default-off 对等:
examples/fixtures/pr-review.public.json未声明reviewer_login,其 packet 断言(3 attention、unmerged actionable 2 / no_action 1)未改动即通过,证明未声明时行为不变。 - 权限面:改动只影响只读分类与渲染,不授予 review/comment/merge/bypass 权限;
write_authority_granted仍为 false。
我的整体评价
- 可观察语义:同一实测窗口,base 与 head 对比 attention
3 -> 8、review_sequence长度3 -> 8、上述 5 个具体 head 换 lane;packet schema、字段名与 lane 枚举不变,因此下游消费者不需要迁移。 - 残余风险:队列不代替门禁——它只把
qualify_pull_request_merge_readiness交给 agent,真正的 fail-closed 判定仍由--check-merge-readiness在合前重跑;head 漂移由既有 exact-head 绑定处理。 - 验证证据:live packet(
--state open --limit 100);--check-merge-readiness 4758@c6d78c46→ready=false/merge_state_requires_update,4759@552a848d→ready=true/admin_bypass_required=true;python examples/pr-review-command-smoke.pyok;pytest -q tests -k pr_review164 passed / 12 skipped;loopx check --scan-path .→ public boundary scan clean, 3809 files。 - 这是 control-plane 行为变更(
loopx/**),因此由维护者合入;作者侧只提供已发布在精确 head 上的评审记录与验证证据。
English verdict: APPROVE at exact head dfecb28. The queue now agrees with the merge gate it routes to: an approved open head keeps owing the pre-merge qualification whether its approval is formal or the author-owned COMMENTED fallback. Merge authority remains with the maintainer because this is a control-plane behavior change.
Publication note: this review was published while the head's Full Public Smokes / shard jobs were still running (Sign-off and dependency-review had passed, the focused pytest selection, the public smoke, the live packet and the boundary scan had all been run locally on this exact head). CI completion on the unchanged head is a maintainer merge gate, not a substitute for this record.
Why
GitHub blocks self-approval, so an author-owned approval can only be recorded as a
COMMENTEDreview with the titled fallback. The PR-review queue decided merge readiness from GitHub's formal review state, so every author-owned approval fell into thecurrent_head_concludedlane and the queue reported0attention for heads that were not mergeable at all.Census on 2026-09-20 (
loopx pr-review --state open --limit 100): 39 open heads, 3 actionable; five of them carried a validAPPROVEconclusion and were 4xBEHIND+ 1xBLOCKED(for example#4758behind atc6d78c46,#4759blocked at552a848d). The head could be approved, unmergeable, and invisible at the same time.pull_request_merge_readiness_v0already decides this from the typed verdict and treats the author-ownedCOMMENTEDapproval as an approval (author_owned_commented_approval). The queue did not agree with the gate it routes to.What changed
review_action_kindqualifies merge readiness from the typedverdictinstead of the reviewstate, so an approved open head keeps owingqualify_pull_request_merge_readinesswhether the approval is formal or the author-ownedCOMMENTEDfallback. A valid non-approval conclusion stays inventory-only._review_why_nowuses the same rule, so the row text matches the action.reviewer_login. Without a reviewer identity a fixture cannot represent an author-owned conclusion at all, so the public smoke could not cover this contract.BEHINDplus a failing check), an approved head that is still clean, and a request-changes fallback as the negative control; it also checks the fail-closed gate for both approved heads.Behavior change
Author-owned approvals on open heads are now actionable (
authenticated_developer_owned, tier 2) instead of concluded. Formal approvals are unchanged. Request-changes and other valid conclusions are unchanged. Fixtures that do not declarereviewer_loginbehave exactly as before.Validation
uv run --extra test loopx --format json pr-review --state open --limit 100: attention3 -> 8; the five approved heads (4748,4755,4756,4758,4759) move intoreview_sequencewithqualify_pull_request_merge_readiness.loopx pr-review --check-merge-readiness 4758@c6d78c46…->ready=false,merge_state_requires_update;4759@552a848d…->ready=true,admin_bypass_required=true.uv run --extra test python examples/pr-review-command-smoke.py-> ok.uv run --extra test pytest -q tests -k pr_review-> 164 passed, 12 skipped.Boundary
Control-plane behavior change (
loopx/**): proposed for maintainer review and merge, not self-merged. The change grants no review, comment, merge or bypass authority; it only reports.