Skip to content

fix(pr-review): require repository reuse evidence before approval - #3988

Merged
huangruiteng merged 1 commit into
mainfrom
codex/pr-review-reuse-evidence
Sep 6, 2026
Merged

huangruiteng merged 1 commit into
mainfrom
codex/pr-review-reuse-evidence

Conversation

@huangruiteng

@huangruiteng huangruiteng commented Sep 6, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Make repository reuse an explicit evidence requirement of the existing PR-review capability, following the missed parallel completed-history implementation in #3961 alongside #3970.

The review failure was not a lack of passing tests: the reviewer accepted locally correct, conflict-free coexistence without comparing the existing resource contract and state owner. Existing architecture/proportionality guidance did not require an evidenced base/head search of unchanged sibling implementations.

Changes

  • Add repository_reuse to behavior-bearing review plans, initially unverified, using the existing evidence/result/completion contract.
  • Require searched revisions, queries and paths, candidate definitions/callers, semantic comparison, reuse/separation rationale, validation and a verdict. Compare scope/filtering, ordering/pagination, authority/sanitization and state/retry ownership.
  • Project a request-changes conclusion for unjustified duplication or unproven evidence. Preserve justified independent invariants and bounded negative searches; similar code alone is not a defect.
  • Keep ordinary docs and smoke-only applicability unchanged. Thin the host skill by replacing its duplicate interpretation checklist with capability routing; do not raise its length budget.
  • Extend existing unit and real CLI coverage; correct two stale smoke expectations to the already-shipped behavior-bearing policy.

Validation

  • Focused contract, queue and GitHub scan tests: 46 passed.
  • Real pr-review-command-smoke: passed.
  • Ruff on four changed Python files and mypy on two production modules: passed.
  • Skill validator and diff hygiene: passed.
  • CLI output-budget regression including base/head differential: passed.
  • Public-boundary scan: six candidate files clean; unrelated local registry health warnings are not part of this change.
  • Exact change-quality receipt: cqr_6bdd1ddcac54ba007694, verified for fingerprint 6bdd1ddcac54ba007694dd795ad06eb14f73c26048d4d218729a27575bc5d1bb; safe fix allowed/applied once, zero blockers/warnings/advisories.
  • Final premerge gate passed with a valid exact receipt: seven catalog checks, eight risk-profile checks, one boundary check and four direct checks; zero failures or manual holds.
  • One blinded, read-only forward-test against the historical candidate independently found the parallel snapshot/offset ownership and reproduced record omission, stranded lane loading, and missing evidence. Existing historical focused tests still passed (14 tests). This single run is not an A/B model qualification; full historical browser/build validation was unavailable.

Boundaries

This is a reviewer-executed requirement, not an automatic repository search, similarity detector, or semantic validator of published prose. Contract tests do not prove model compliance. No scheduler, authority store, permission, queue-selection policy, provider call, or external write is added. The temporary evaluation artifacts and all private/runtime state are excluded.

The six-file commit is one coherent capability contract, adapter and validation change. Owner requested self-review and self-merge after qualification.

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)

No blocking finding in this capability change. Reviewed exact head 76e597c742a742e8e9d712f7b96223aaff7ae5ab against base bb71b386254540926489cd6c61bcc6a6308accef. The full CLI packet was preserved and the selected PR's evidence plan executed. This is an author self-review, not an independent review of this PR.

动机

这次修复的是审阅过程里一个具体盲点:此前把“新增代码能够运行、CI 通过、与主干无合并冲突”当成了足够强的正向依据,没有进一步验证“相同用户需求是否已有生产实现,为什么不能复用”。#3961 与 #3970 的完成任务历史就是实例:局部代码看似完整,却留下两套分页和请求状态。这个遗漏首先是审阅者的判断失误,不能归咎于作者或工具。现有 capability 虽要求解释架构和复杂度,但明确的现有覆盖搜索主要属于 smoke-only 审阅,运行时代码没有独立的复用证据项。只在某个宿主 skill 加一句提醒,会继续形成两处规则来源;本 PR 因此直接补共享审阅合同,不改调度器、不添加查重服务,也不扩大外部操作权限。

改动思路

仍沿用原链路:CLI 读取并归一化 PR,build_review_plan 根据已有 area 集合决定适用证据,build_review_execution_contract 提供证据字段和结论规则,宿主执行搜索、验证后填入结果,再用原五段模板发表。新增的 repository_reuse 从 unverified 开始,要求记录 base/head 的不可变版本、检索词与路径、已有定义和调用方,并对照资源、过滤、分页、权限/脱敏和状态所有权。

本 PR 自身也执行了该搜索:在上述 base/head 对 pr_review_queue/review_contract.py 和 change_quality/result.py 搜索 reuse|duplication|existing.coverage,并检查 loopx/pr_review.py 的未修改调用方。最近的可复用所有者是现有证据声明、plan、结果骨架和 completion policy,已经直接扩展;change-quality 的局部质量回执不是 PR 搜索执行器,未复制其 parser 或新增另一种状态。正向路径要求给出可审计的复用理由;负向路径中,即使 CI 全绿,缺少搜索仍不能升级为批准。对不同不变量或兼容边界允许有据可查的独立实现,避免把所有相似代码强行抽象。

具体改动

完整 diff 为六个文件、+161/-18,没有生成产物。生产合同/catalog 为 +52/-2,测试与既有 smoke 为 +84/-2,文档与 skill 为 +25/-14。README.md 解释适用范围、五种结论和证据局限;catalog_entry.py 更新既有能力边界,不改队列优先级、checkpoint 或权限。skills/loopx-pr-review/SKILL.md 删除重复讲解清单,转为引用共享证据,保留完整评审、正负路径和发布要求,179 行仍在原长度门槛内。

关键代码讲解

  1. review_contract.py:105 的 build_review_execution_contract:原先没有独立仓库复用项,现在声明七个证据字段、五个语义对照维度与五种结论;552 行把 unjustified_duplication、not_yet_proven 接入原 blocking map。返回值仍由 build_agent_response_contract 经 loopx/pr_review.py:1287 投影,不产生 I/O 或自动判重。
  2. review_contract.py:604 的 build_review_plan:沿用 CODE_AREAS 与 BEHAVIORAL_POLICY_AREAS,仅在 behavior-bearing 分支加入复用项及适用标志;结果仍由已有 comprehension 初始化为 unverified。未修改的生产调用点是 loopx/pr_review.py:1079,所以不是只供测试调用的新模块。
  3. review_contract.py:61 的 build_review_template:改动思路段引用新证据,模板仍是空白输出结构,不能代替查代码。测试新增六类行为变更正例、普通文档/测试两类反例和 verdict/schema 覆盖;现有真实 CLI smoke 同时验证序列化与适用性。两条 smoke 旧断言修正为主干早已使用的 behavior_bearing_change,没有放宽产品门禁。

对主干的风险

主要风险不是运行时数据损坏,而是审阅要求过重或模型只写漂亮结论却没有执行搜索。前者通过复用旧 area 判断、普通 docs/test-only 反例,以及明确允许 justified separation 来控制;后者不能靠字段测试完全解决,因此文档明确这属于审阅者执行合同,不是自动搜索或公开评论的语义验证器。枚举结论和现有集合分支继续承载分类,不增加 substring 判断、持久状态或新的 actor/authority 含义。

这是对显式 PR-review 工作流的有意加强,不声称新规则单独 default-off;既有自主队列 default-off/选择逻辑未改。skill 安装入口在原 pyproject.toml,仅在相关任务路由使用,未向所有 Goal 注入规则。新增要求和失去批准依据的情形均在 capability README 披露。无数据库、provider、权限迁移;回滚可通过单提交 revert PR 完成。

验证:46 个 focused tests、实际 CLI smoke、四文件 Ruff、两生产模块 mypy、skill validator、CLI output budget regression、diff hygiene 均通过;premerge 的 7 个 catalog、8 个 risk-profile、1 个 boundary 检查与四项 direct checks 均通过,零失败、零 manual hold。精确质量回执 cqr_6bdd1ddcac54ba007694 为 valid,fingerprint 6bdd1ddcac54ba007694dd795ad06eb14f73c26048d4d218729a27575bc5d1bb,safe-fix allowed/applied 一次,零 blocker/warning/advisory。远端完整 CI 在审阅时仍有 pending,没有观察到失败;合并前再次回读。本变更采用仓库允许的本地 risk-based 等价验证,不把 pending 报成通过。

我的整体评价

结论为批准:复用已有审阅基础设施,增加一个明确证据责任,规模与已证实问题相称;没有为一次漏审制造新框架。独立审阅者仅得到旧 base/head、新 packet 和最小原始材料,未获得问题提示,实际发现重复 snapshot/offset 所有者,并复现并发移除漏项、lane loading 卡死和 evidence 丢失。旧代码 14 个 focused Python tests 仍全绿,恰好说明只看既有测试不足。该盲测有价值,但只有一次、没有 A/B 对照;完整旧 UI/browser 构建也未验证,不能宣称模型命中率提升或今后保证不漏。按 owner 的自合并请求,在 exact-head、质量回执和最终 checks 回读后执行。

English verdict: APPROVE at 76e597c742a742e8e9d712f7b96223aaff7ae5ab. No blocking finding: this extends existing evidence/plan/completion ownership instead of introducing a duplicate search or validator framework. Forty-six focused tests, real CLI smoke, lint/type checks, output-budget regression, exact quality verification and the 16 selected premerge checks passed. One blinded historical forward-test found actionable reuse-related defects; it is not an A/B model qualification or guarantee. Remote CI was still pending at review time and is not represented as passing.

@huangruiteng
huangruiteng merged commit 52c34cd into main Sep 6, 2026
11 checks passed
@huangruiteng
huangruiteng deleted the codex/pr-review-reuse-evidence branch September 6, 2026 05:46
@huangruiteng

Copy link
Copy Markdown
Collaborator Author

Self-merged under the owner's explicit authorization after exact-head review and passing local risk-based qualification.

  • Reviewed head: 76e597c742a742e8e9d712f7b96223aaff7ae5ab.
  • Squash merge: 52c34cda50ce2a8156adbf9fc50b75de324d9ee2.
  • Exact quality receipt: cqr_6bdd1ddcac54ba007694, valid; 16 selected premerge checks and four direct checks passed, no manual holds.
  • Remote readback immediately before merge: no failed checks. DCO, dependency review, release build, frontstage build, Windows validation and SonarCloud Code Analysis passed. Full pytest and the non-blocking SonarCloud workflow were still pending, not counted as passed. Release publishing/deployment jobs were skipped as expected for a PR.
  • This is the repository-authorized admin-bypass path with focused local validation, not a claim that full remote CI had completed.

The bounded historical forward-test validates this one review outcome only; no guarantee of future model compliance is asserted. Temporary probe artifacts and private runtime state are not part of the PR.

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