Skip to content

fix(pr-review): attribute unrelated red checks before verdict - #5037

Merged
huangruiteng merged 3 commits into
mainfrom
codex/pr-review-failure-attribution
Sep 25, 2026
Merged

huangruiteng merged 3 commits into
mainfrom
codex/pr-review-failure-attribution

Conversation

@huangruiteng

@huangruiteng huangruiteng commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

Outcome

Required red checks no longer automatically mean REQUEST_CHANGES for an unrelated PR. The PR review contract now distinguishes a PR-caused or unresolved failure from an independently evidenced unchanged baseline failure or external outage. An unrelated red check can support APPROVE of the reviewed change while the separate merge-readiness gate remains on hold.

The concrete regression was a review of #5016 that identified the same unrelated semantic-smoke failure on base and head but still requested changes solely because the check was red.

Boundary and behavior

  • Add typed failure-attribution dispositions and evidence requirements to the existing review contract; bump its policy revision.
  • Check attribution in the existing result validator, including negative cases for new/worsened failures, missing evidence, invalid status, and REQUEST_CHANGES with no actual blocker.
  • Update the capability README and managed review skill. No merge-readiness, CI, quota, scheduler, or authority rule is relaxed. Feature-off behavior is unaffected because the change is confined to PR review judgment.
  • Future-facing pass: reused the existing contract/checker owners and kept merge authority separate; no new abstraction or follow-up refactor needed.

Validation

  • Focused PR review tests: 136 passed, including a case where two red checks have equal counts but different failing identities.
  • Ruff on the four changed Python files: passed.
  • PR review command smoke and semantic-vocabulary smoke: passed.
  • Public/private boundary scan of the six proposed files: passed; no private state, credentials, raw logs, or local artifacts included.
  • Change-quality exact-diff receipt: passed.
  • Premerge canary: passed, 17 selected checks, 0 failures, valid exact-diff quality receipt. An initial run encountered a missing local Node dependency for the semantic smoke; dependencies were prepared and the complete final canary passed.

Merge gate

This modifies loopx/** PR-review control-plane behavior. Per repository policy, the exact-head self-review may be published, but the author must leave the PR for maintainer merge rather than self-merge, even when validation passes.

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>

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

动机

这次评审针对精确 head 3cbdf587dff7fd01a05e2400050bf5272e0fa27a。#5016 的公开评审已经确认同一个语义检查在基线与 PR head 上都为红,却仍单凭红灯给出 REQUEST_CHANGES。用户需要的是区分“这个 PR 引入的问题”和“独立于这个 PR 的既有故障”,避免无关 PR 反复背锅;同时不能把 review 认可偷换成可合并许可。本 PR 是一个可独立验证的规则修复,未改动 CI 或 merge-readiness 的权限边界。

改动思路

入口仍是 loopx pr-review 生成的评审契约与 --check-result 验证器。原有 validation_matrix 已记录检查状态,因此沿用这个 owner 加入结构化失败归因,而没有另建 CI 判定器。基线和精确 head 必须使用同一命令、同一失败项及细节;外部故障则要独立证据、恢复责任人和改动不变量的独立通过证据。新规则只改变 PR 评审结论:满足归因条件时可以 APPROVE,但红 CI 是否允许合并仍交给现有 merge-readiness gate。相同总数本身不构成同因,未知或恶化的失败继续阻止认可。

具体改动

关键代码讲解

  1. review_contract.VALIDATION_FAILURE_ATTRIBUTION 定义 pr_regression、pre_existing_unrelated、external_unrelated、unresolved 四种归因以及各自证据要求;REVIEW_POLICY_REVISION 从 10 升到 11,迫使旧结果重做,不会静默沿用旧口径。
  2. build_review_execution_contract 把归因要求放进既有 validation_matrix,并明确 review verdict 与合并状态分离;build_agent_response_contract 将同一义务投射给执行评审的 agent,Goal 的 wait_for_ci=false 仍不要求读取远端 CI。
  3. result_check._check_validation_failures 对 required 失败、跳过、待定、非法状态逐项处理;基线型只有在两个不同 revision 的失败签名相同且证据字段齐全时才不阻断。check_review_result 还拒绝“没有 blocker 却 REQUEST_CHANGES”的自相矛盾结论。
  4. 测试覆盖既有同因红灯、相同红灯计数但不同失败项、恶化签名、外部故障、缺失证据与非法状态。README 和 skill 同步解释评审与合并的分界;没有新模块、持久状态、CLI 参数或独立权限模型。

对主干的风险

最强反例是 agent 仅凭两个 44/43 总数就声称“基线一致”,从而放过新失败。修订后的规则明确要求同一失败项与细节;同一测试入口对未修改基线的两个相关断言确实失败,对精确 head 则通过,其中等计数但不同失败项会被阻断。另一个风险是把被认可的 PR 当成可合并:此 PR 没改 merge-readiness,canary 也明确 self_merge_allowed=false。--check-result 只核对结构一致性,不能替代审阅者对命令、SHA 与实际日志的核实;这是保留的人工真实性风险。136 个聚焦测试、Ruff、PR review 命令 smoke、语义 smoke、公共边界扫描和精确 diff canary 已通过;最初缺少本地 Node 依赖的 smoke 失败已在补齐依赖后完整重跑并通过。没有发现阻断性代码问题。

语义与 CI 对齐

本 PR 扩展现有 validation_matrix 词汇,没有创建第二套 CI 权威。红灯归因决定的是“是否要求这个 PR 改代码”,不是“能否忽略必需检查合并”;新引入、恶化或未证实的失败仍需 REQUEST_CHANGES,独立的合并门槛照常保留。

我的整体评价

精确 head 的修复使长期 review 队列不再因无关红灯重复停滞,也给用户一个可核查的归因和恢复责任边界。默认评审行为有意改变,并已在契约、技能和文档公开说明;未启用 PR review 的路径不受影响。相邻边界的重构检查结论是复用现有 contract/checker,比增加第二个 CI 决策层更容易维护。完整本地验证与公共内容扫描通过,尚无阻断发现;但这仍是 loopx/** 控制面变更,作者不自合并,交维护者在独立合并门槛下处理。

English verdict: APPROVE - exact head 3cbdf58; unrelated red checks require causal evidence, 136 focused tests and final canary passed, merge authority remains separate.

@huangruiteng
huangruiteng merged commit cc1d46d into main Sep 25, 2026
17 of 24 checks passed
@huangruiteng
huangruiteng deleted the codex/pr-review-failure-attribution branch September 25, 2026 07:27
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