Skip to content

fix(pr-review): read effective approvals across reviewer identities - #5447

Merged
huangruiteng merged 1 commit into
mainfrom
codex/pr-review-foreign-approval-readback
Oct 2, 2026
Merged

huangruiteng merged 1 commit into
mainfrom
codex/pr-review-foreign-approval-readback

Conversation

@huangruiteng

Copy link
Copy Markdown
Collaborator

Goal and delivered delta

Follow up #5425: pr-review --check-approval-closeout NUMBER@HEAD_OID must recognize an effective, valid exact-head approval even when another account performs the read. The existing adapter filters review history to the authenticated account, causing exact_head_approval_missing despite a valid public approval.

The source fix removes that account filter, validates each review with the existing standalone-body validator, and lets the established TypeScript closeout owner select the latest effective opinion per reviewer. For public PR #5338, the final source CLI now reads the existing approval while preserving GitHub's raw REVIEW_REQUIRED rather than inventing APPROVED or merge readiness.

Scope and boundary

  • Existing built-in pull-request-review capability and GitHub read adapter; no new capability/provider or Python decision owner.
  • Superseding dissent/dismissal prevents revival of an earlier approval. Ordinary comments do not erase an opinion. The existing titled author-owned COMMENTED fallback remains supported.
  • Complete conclusions bind to original review identities; other reviewers' blockers remain visible. Approval selection and blocker reduction share the existing typed owner.
  • Public result schema, flags, default queue, optional configuration, and review-body requirements are unchanged. The transient internal request producer and decoder deploy together; no persisted request migration is introduced.
  • This remains read-only. It grants neither dismissal nor merge authority, and does not treat review age, another approval, or CI as finding-resolution evidence.
  • No frontend/Lark companion is required: the changed entrypoint is the explicit CLI closeout read model, with no configuration or UI interaction change. Installed-runtime adoption has not been performed.

Validation

Final candidate: 79913951e8afcbe66d5e530e27686b8ce9ca4513, base 0cbeda380babc623502c2549817c78d42db947c0. Inputs: synthetic fixtures and public GitHub metadata; no private evidence is attached.

Check Result Evidence / limitation
Historical sensitivity Passed The added foreign-account CLI counterexample fails on the old adapter and passes with the fix.
Focused review contracts Passed 181 tests across body, result, contract and closeout suites on the final head. Includes foreign approval, subsequent dissent/dismissal, ordinary comments, author fallback, malformed history and identity rebinding.
Real entrypoint/backend Passed Final source CLI against unchanged public #5338 reads the foreign approval, preserves raw REVIEW_REQUIRED, and performs no GitHub write.
Static Passed Full control-plane TypeScript typecheck; Ruff on changed Python; diff hygiene; canonical public-boundary scanner: three candidate files, zero hits.
Source CLI smoke Passed examples/pr-review-command-smoke.py.
Semantic checks Passed Diff advisory followed by full semantic-vocabulary smoke; no new shared vocabulary. An empty advisory is not an equivalence proof.
Full TS suite Failed 3711 passed, 1 failed, 31 skipped. receipt-only claims require head CAS and recover a lost commit response expects changed=false, gets undefined. The identical focused test fails on both pinned main and the final candidate. The initial broad run predates the README-only rebase; the affected TS source is unchanged.
Independent baseline comparison Passed Same isolated FileAuthorityStore test/oracle at main and final head reproduces the identical failure. This does not turn the failed suite into a pass. Existing #5444 addresses the stale assertion; this PR does not duplicate it.
Risk-selected premerge checks Passed, gate held All 10 selected canaries and five direct checks passed. The overall premerge gate remains blocked by the failed strict quality receipt; passing canaries do not waive it.

Delivery hold

Draft only: the strict exact-scope quality receipt is recorded as failed because the full TS validation failed. The source fix is not a qualified delivery or installed repair. After #5444 (or the owning baseline correction) lands, update this branch, rerun affected/full validation and exact-scope qualification, then request maintainer review. Do not merge or dismiss reviews from this PR.

The bounded future-facing pass removed duplicate Python selection and retained one typed effective-opinion owner; no additional abstraction was needed. No private state, credentials, local paths, raw logs or generated dependency files are included. The sole commit has a DCO sign-off.

Signed-off-by: huangruiteng <huangrt01@163.com>
@huangruiteng
huangruiteng marked this pull request as ready for review October 2, 2026 06:38

@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:79913951e8afcbe66d5e530e27686b8ce9ca4513;不可变基线:0cbeda380babc623502c2549817c78d42db947c0。覆盖完整三文件差异,没有新的代码阻塞项;原严格质量失败和维护者合并限制仍保留。

动机

原 closeout adapter 把完整 GitHub review 历史过滤成当前认证账号的记录,导致另一操作者无法读取有效的同 head 审批。目标是复用现有 PR review capability 的有效意见 owner,正确读回跨账号结论,同时保留不同 reviewer 的反对意见、GitHub 原始聚合状态及权限边界。

独立真实 CLI 对照读取公开 #5338 的同一 head/review:正常账号时基线与当前完整结果一致;仅将 reader identity 设为合成 observer,GitHub 后端仍为真实只读 transport,基线误报 exact_head_approval_missing,当前读回 review 5390732339。这不是切换真实 GitHub 凭据或批准未读的 findings;原始状态仍是 REVIEW_REQUIRED。

改动思路

Python 读取全部分页 review、逐条调用原 standalone/body validator,并附上原 review id;既有 TS owner 按 reviewer 的有效最新意见选审批、列 blockers。读者认证用于授权读取,不用于限定 reviewer。没有增加 Python 选择规则、公共 flag、配置、持久字段或新的 provider。

具体改动

三文件增加 112 行、删除 29 行;Python adapter 13/-17、TS owner 33/-6、测试 66/-6。新增内容针对真实跨账号入口、后来反对/撤销、普通 comment、author fallback、非法历史和身份绑定,未扩展到 merge/dismiss executor。

关键代码讲解

  • read_github_approval_closeout(approval_closeout.py:51):保留完整 file/review 分页、读前读后 PR identity/lifecycle/head 校验;每条 review 使用原 _review_conclusion,再将 conclusion 绑定原始 id。输出仍不带 review body。
  • planPrReviewApprovalCloseout(pr_review_approval_closeout.ts:19):完整 history 和 conclusions 必须一一绑定唯一 id;有效 conclusion 不能改写 state/reviewer。大小写归一的 reviewer key 统一减法处理最新有效意见,ordinary comment/pending 不擦除先前意见;有效 author-owned titled COMMENTED fallback 继续支持。
  • approval/blocker reduction(同 TS 文件:73):只选当前 head 的有效 APPROVE;后来 CHANGES_REQUESTED 或 DISMISSED 不能复活旧 approval。其他人的最新 formal blocker 仍单独列出,reviewDecision 原值保留。dismissal_authorized、merge_authorized、github_write_performed 始终 false。

对主干的风险

最大风险是扩大读取后把旧审批、后来撤销的审批或别人的 blocker 擦掉。我的独立 CLI fixture oracle 在基线/当前跑十一种完整历史:foreign approval、后来 dissent、dismissal、ordinary comment、另一 reviewer blocker、pending、旧 head、读回 head 变化、已关闭 PR、重复 id、malformed user。当前全部符合独立契约;旧适配器的 foreign approval/comment/pending 反例确实失败。另有 blocker 时结果是 verification_required,错误/不完整范围维持 hold 或 input error,所有路线零写入、零授权。

逐条 validator 和 typed reducer 共用既有规则;新增 conclusion 是从完整 GitHub review 派生的临时读模型,不是手工维护的新资格。旧 private request 与 producer/decoder 同部署,源码指纹隔离各 runtime;调用方搜索只发现该 adapter、handler 和直接 tests,没有独立升级 consumer 或持久化请求需要 legacy decoder。公共输出、旧 review history 及 titled fallback 仍兼容。

语义与 CI 对齐

181 项 focused body/result/contract/closeout 检查通过,source CLI smoke、Ruff、control-plane TS typecheck、advisory 后全树 semantic、diff/DCO 检查通过。初始 11 个 CLI fixture 失败源于主机两个 implicit runtime root 都有状态;相同基线有 10 个对应入口失败,相同路径在 adapter 执行前拒绝。给合成 registry 指定其合成 runtime 后,完整 181 项通过;没有更改或迁移活动用户状态。

严格质量门禁仍有独立 hold: receipt-only claims require head CAS and recover a lost commit response 在基线和最终 head 的同一 FileAuthorityStore 真实测试都期待 changed=false、得到 undefined,同一断言、同一行、同一细节;该 PR 没改该 store/claim 路径。未把作者报告的全 TS 失败重新标为通过,也未重新执行整套 3711 项。本评审的代码 APPROVE 不豁免原 exact-scope quality receipt 或允许合并。按当前策略没有读取、轮询或等待远程 CI。

未引入共享 vocab 或 substring 分类;GitHub enum 沿用原 exact closed set,机器强制的 body/head/completeness 义务未称为 guidance。没有 default-off 设定变化;普通 queue 仍由原 selector 处理,只有明确 --check-approval-closeout 入口修正跨账号读取。没有配置/产品交互变化,CLI 是唯一受影响入口,不需要新增 frontend/Lark editor;已安装 runtime 采用仍未验证。未来重构检查已落实删除重复 Python selection,保留一个 typed effective-opinion owner,没有必要再加抽象。

我的整体评价

APPROVE。完整差异解决了有证据的 source CLI 读取错误,负例证明没有扩大审批、dismissal 或 merge 权限。保留原严格质量失败供原 owner 修复/重新资格;已分发 runtime 采用与最终合并由维护者处理。本次没有 dismiss 任意 review,也没有 merge。

English verdict: APPROVE - head 7991395. Complete real CLI historical counterexamples and eleven same-input history branches confirm foreign approval readback while preserving subsequent dissent/dismissal, other blockers, exact-head scope and raw REVIEW_REQUIRED. 181 focused tests and source/static/semantic checks passed after explicit synthetic runtime routing. The unchanged claim-CAS assertion fails identically on immutable base and head; retain the existing strict quality/merge hold. No dismissal, merge authority or installed-runtime adoption is implied. Maintainer merge required.

@huangruiteng
huangruiteng merged commit 1d8e20f into main Oct 2, 2026
35 of 61 checks passed
@huangruiteng
huangruiteng deleted the codex/pr-review-foreign-approval-readback branch October 2, 2026 12:13
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