Skip to content

fix(pr-review): reconcile obsolete blockers after exact-head approval - #5425

Merged
huangruiteng merged 2 commits into
mainfrom
codex/approve-stale-review-reconciliation
Oct 1, 2026
Merged

huangruiteng merged 2 commits into
mainfrom
codex/approve-stale-review-reconciliation

Conversation

@huangruiteng

Copy link
Copy Markdown
Collaborator

Goal And Delivered Outcome

  • Outcome basis: PR #5343 demonstrated that a valid exact-head approval can coexist with another reviewer's obsolete blocking review. Publishing APPROVE alone did not finish the review workflow.
  • Before → after: the capability previously ended at published-review readback; policy revision 13 now requires post-approval reconciliation of every still-effective blocking review. A read-only CLI exposes those reviews without treating an old commit as proof that its findings were fixed.
  • Intended base: main. Complete within the implementation/proposal scope; integration and installed behavior remain pending maintainer review and merge.

Scope And Continuation

  • Add pr-review --check-approval-closeout NUMBER@HEAD_OID: require the existing capability-qualified exact-head approval, read all paginated reviews, select each reviewer's latest decisive opinion, and check the head again. Return clear, verification_required, or hold; preserve GitHub's raw aggregate decision, including null.
  • The capability-owned host procedure verifies the full review and every inline finding on the approved head, checks explicit owner authority and native GitHub permissions, rechecks the effective target immediately before a dismissal, and reads back dismissal, preserved approval, unchanged head, and remaining blockers. Unresolved findings and discussion history stay intact.
  • This is not an automatic dismissal executor. The CLI is read-only; the host uses GitHub's native dismissal API only after satisfying the contract. Neither an approval, review age, green CI, nor a resolved discussion grants dismissal or merge authority. An unresolved closeout does not revoke an earned APPROVE.
  • Owner/provider placement: existing built-in pull-request-review; TypeScript owns the pure reconciliation read model, while Python adapts GitHub transport and reuses the existing approval validator. No new capability, settings owner, provider distribution, or parallel Python decision owner.
  • Bounded future-facing pass: isolate the typed read model and reuse existing exact-head/approval/file-source owners rather than extending queue-wide scans or adding a general review mutation framework. No larger migration is needed for this outcome.
  • Related PR #5419 addresses reviewer provenance, not obsolete-review reconciliation. It may require normal merge conflict resolution in shared contract/skill files, but does not replace this fix.
  • No self-merge: this changes CLI and capability/control-plane behavior. Maintainer review and merge remain required.

Validation

  • Tested revision: e0f81e6ccd5b06fb459b5a9e85a24c25c256003b, based on 8fdc1616fc84ef158775e322cf3660cfe18a5be0.
  • Run state: finished.
  • Input classes: synthetic, public_fixture.
Check kind Result Public-safe evidence / limitation
unit, regression_parity passed Eight focused PR-review Python test files: 253 passed, 33 conditional skips. New invariant tests cover per-reviewer blockers, same-head reviews, pagination, malformed/incomplete sources, changed heads, missing approval, and aggregate conflicts; public CLI tests prove no CI read or GitHub write. Existing review/configuration paths retain parity.
unit, integration passed npm run -s test:control-plane: 3,642 passed, 31 conditional skips. Run before the documentation-only base rebase; changed product sources are identical. Focused Python, typecheck, and native premerge were rerun on the final head.
static passed npm run -s typecheck:control-plane; Ruff on changed Python and the repository's declared lint surfaces; Mypy on the 19 repository-declared files (not a claim of full-tree Mypy coverage); git diff --check.
real_entrypoint, real_backend passed Checkout CLI against public GitHub PR #5343 at 3a06669967e691780610d8f68c1ea7e048066cc8: repeated read-only closeout returned clear, retained the existing approval, and performed no dismissal or CI read. No mutation probe was run against a live PR.
integration passed pr-review-command-smoke.py, full semantic vocabulary/registry-I/O census, and CLI output budget smoke passed. The development-time semantic advisory was also run; GitHub's external review vocabulary and the local closeout read model have explicit owners. No budget was increased.
integration, static passed Native canary premerge --from-git-diff --git-diff-base 8fdc1616fc84ef158775e322cf3660cfe18a5be0: all 19 selected checks and 5 direct checks passed; zero manual holds, runtime failures, or quality-receipt failures. Public/private scan clean for all nine changed paths. Validation passing grants no self-merge authority.
  • Coverage/gaps: no full Python suite, model calls, benchmark jobs, or live dismissal mutation test. Finding resolution and dismissal remain host-owned permission-gated operations. PostgreSQL authority, scheduler, persisted Goal state, and packaged frontend behavior are unchanged, so their real-backend/UI gates do not apply to this slice.
  • Change-quality qualification: state=valid; scope_fingerprint=4e437c2da28881e12a59f797296d958231e458926315af5a0a54249bca8b9649; base_ref=base_commit=8fdc1616fc84ef158775e322cf3660cfe18a5be0; head_commit=e0f81e6ccd5b06fb459b5a9e85a24c25c256003b; receipt_id=cqr_4e437c2da28881e12a59; requalification_required=false; previous_receipt_id=null; previous_scope_fingerprint=null; previous_base_commit=null.

Frontend / Visual Evidence

  • UI impact: none. The new mode is an explicit CLI readback and post-publication host contract, not a configuration change. Existing settings/capability editors need no companion control. No dashboard, Lark, opening navigation, or public first viewport changes.
  • Before/after/viewports/attention review: N/A; source data: none.

Type of Change

  • Bug fix
  • Documentation update
  • Test update

LoopX Area

  • Capability or extension (providers, adapters, skills)
  • Control plane (goals, todos, quota, scheduler, registry, runtime)

Technical Direction

  • Existing pull-request-review delivery acceptance and TypeScript control-plane ownership. This is a bounded approval-closeout fix, not a migration/promotion claim.
  • Shared-authority RFC fixture impact: N/A; no provider routing, authority-store contract, or compatibility projection promotion.

Boundary Checklist

  • Diff and public artifacts contain no private Goal state, credentials, raw logs, internal links, or local machine paths.
  • No duplicated benchmark work or new benchmark jobs.
  • Single-purpose approval-closeout scope.
  • UI impact marked none.
  • Both commits carry DCO Signed-off-by trailers.

Signed-off-by: huangruiteng <huangrt01@163.com>
Signed-off-by: huangruiteng <huangrt01@163.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:e0f81e6ccd5b06fb459b5a9e85a24c25c256003b;按 pull-request-review capability policy revision 13 执行。未发现阻塞问题。

动机

PR #5343 已证明:自己的有效批准不会覆盖另一账号仍生效的旧阻塞评审。单纯多发一次 APPROVE 不能完成用户想要的收尾,也不应把旧 commit、另一个账号的批准或绿色 CI 当成问题已修复的证据。本改动补齐 capability 的批准后收尾流程;源代码提案完成且可独立验证,实际安装行为仍等待维护者合并与发布。

改动思路

保留 GitHub 评审历史作为唯一事实源,Python 只负责读取完整文件、分页评审和调用既有批准校验,TypeScript 根据每位 reviewer 最新的决定性意见生成只读结果。没有增加重复同步的 Todo 状态、权限授予或自动撤销执行器。成功路径是读回批准、定位实际阻塞评审、逐条验证正文及 inline findings、检查 owner 授权和 GitHub 权限、临近操作再次确认精确 head,再走原生 dismissal 并读回。未解决的意见必须保留;收尾失败也不反向撤销已经得到证据支持的 APPROVE。相比只改 skill 提醒,这个有界读模型能确定性处理分页和历史;相比自动清除所有红评审,它保留必要判断与权限边界。

具体改动

关键代码讲解

handle_pr_review_command 在 loopx/cli_commands/pr_review.py:267 增加独立只读模式,混用扫描、fixture、result 或 readiness 参数会在远端读取之前拒绝,旧的直接构造 Namespace 调用用 getattr 保持兼容。read_github_approval_closeout 在 approval_closeout.py:44 复用 exact-head 规范化、文件完整性恢复和 _review_conclusion,只将当前账号的评审交给现有批准校验,再把完整跨账号历史交给 TS;正文只用于临时校验,不进入结果。planPrReviewApprovalCloseout 在 TS 文件第 18 行校验完整来源、批准和两次 head,使用 reviewer-keyed Map:COMMENTED/PENDING 不清除阻塞意见,另一账号的批准也不会清除它,最新 DISMISSED 不会复活旧历史。aggregate 与完整历史冲突时返回 hold,并保留原始 null,而不是虚构 APPROVED。

其余改动是 effect-runtime 注册、review policy 12→13 和 capability-owned closeout 组合点;README 与打包 managed skill 明示新收尾义务,skill 保持 180 行。新增测试覆盖历史、分页、同 head 评审、畸形输入、缺少批准、head 变化和禁止副作用;registry-I/O manifest 仅更新一个已有调用点行号,没有新增 registry 读取。

对主干的风险

主要风险是把“候选旧评审”错当成“已解决评审”并删掉有效反对意见。这里三个 effect/authority 标志始终为 false,候选只进入验证流程;真正撤销前必须读全 review 与 inline comments,逐项映射到当前代码和决定性验证,再由 native GitHub 权限约束操作。没有撤销 executor,因此没有对活跃 PR 做写入探针;真实 GitHub 入口使用 PR #5343 重复只读验证 clear、批准保留、无 CI 请求和无重复副作用。全量 Python 与原生 dismissal 权限没有被冒充为已验证。

语义与 CI 对齐

GitHub review states 是外部既有词汇,closeout 三种状态属于本地只读模型,不代表 merge readiness。普通 review packet 在不可变 base/head 上使用同一 public fixture 实跑,除声明的 policy/closeout 指令及生成时间、历史 fixture 的时钟年龄外逐字段一致,ranking/admission 未被归一化;独立批准后 routing oracle 在 base 缺失、head 通过。当前 Goal 设置不读 CI,评审据本地验证:253 项 Python 通过、33 项条件跳过;TS 3,642 项通过、31 项条件跳过,文档-only rebase 后产品源码不变;最终 typecheck、semantic/CLI budget/public-private 扫描和 premerge 的 19 个选中检查加 5 个直接检查通过,精确 diff CQA 有效。未提高预算或把无关红 CI 归罪于本 PR。

我的整体评价

这是现有 review owner 中一个完整、可回滚的收尾增量。长程执行改善在于重复调用从真实评审历史重新推导,不重复发布批准或维护第二份事实;用户体验改善在于给出真实阻塞 reviewer、目标和明确恢复条件。已做有界前瞻重构:隔离小 TS 读模型、复用原批准与 transport owner;不需要更大迁移或另一套 review mutation framework。默认行为新增批准后必做步骤,文档明确披露,并非 default-off 隐藏指令。残余边界是每次实际旧 finding 的人工证据判断及原生权限,而不是此 read-only CLI 自动认证。结论为 APPROVE;作者账号使用 COMMENTED fallback,既不是 GitHub 正式自批准,也不授权合并,维护者仍需独立评审合并。

English verdict: APPROVE - e0f81e6. No blocking finding; capability-owned post-approval reconciliation preserves unresolved reviews and explicit authority. Exact-head local premerge, focused Python, TS, base/head public-entrypoint characterization and real GitHub read-only validation passed; no live dismissal mutation or self-merge.

@huangruiteng
huangruiteng merged commit da5b7b1 into main Oct 1, 2026
27 of 29 checks passed
@huangruiteng
huangruiteng deleted the codex/approve-stale-review-reconciliation branch October 1, 2026 16:09
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