fix(pr-review): read effective approvals across reviewer identities - #5447
Conversation
Signed-off-by: huangruiteng <huangrt01@163.com>
huangruiteng
left a comment
There was a problem hiding this comment.
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.
Goal and delivered delta
Follow up #5425:
pr-review --check-approval-closeout NUMBER@HEAD_OIDmust recognize an effective, valid exact-head approval even when another account performs the read. The existing adapter filters review history to the authenticated account, causingexact_head_approval_missingdespite 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_REQUIREDrather than inventingAPPROVEDor merge readiness.Scope and boundary
pull-request-reviewcapability and GitHub read adapter; no new capability/provider or Python decision owner.Validation
Final candidate:
79913951e8afcbe66d5e530e27686b8ce9ca4513, base0cbeda380babc623502c2549817c78d42db947c0. Inputs: synthetic fixtures and public GitHub metadata; no private evidence is attached.REVIEW_REQUIRED, and performs no GitHub write.examples/pr-review-command-smoke.py.receipt-only claims require head CAS and recover a lost commit responseexpectschanged=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.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.