From c2971e3ef9e87f9d84e3e0d005a6fb875a812184 Mon Sep 17 00:00:00 2001 From: tzhouam Date: Sat, 26 Sep 2026 06:21:56 +0800 Subject: [PATCH] Separate pre-push patch gate from PR review agent --- doc/architecture/SPEC/engine/steps/review.md | 16 +-- .../engine/steps/review/__init__.py | 9 +- .../engine/steps/review/patch_gate.py | 97 +++++++++++++++++++ .../engine/steps/review/steps.py | 87 +---------------- test/test_pr_steps.py | 4 +- 5 files changed, 114 insertions(+), 99 deletions(-) create mode 100644 src/infermatrix_copilot/engine/steps/review/patch_gate.py diff --git a/doc/architecture/SPEC/engine/steps/review.md b/doc/architecture/SPEC/engine/steps/review.md index 9c56e3be9..1f720f47d 100644 --- a/doc/architecture/SPEC/engine/steps/review.md +++ b/doc/architecture/SPEC/engine/steps/review.md @@ -2,7 +2,7 @@ -`LOC ~900（6 个文件） · step 库（评审） · refactor-status: ok` +`patch gate + PR review + quality（8 个源文件） · step 库（评审） · refactor-status: ok` ## 职责 条件式 patch 门 + PR 评审 agent step + 有界的 PR review-readiness 质量 step， @@ -10,17 +10,21 @@ Pre-push patch 门必须审查显式 base 到当前 HEAD 的已提交变更；空范围、脏的 tracked checkout、不可解析的 ref 或超过 reviewer 容量的 diff 都阻塞推送。 通过后记录批准的 base/head，后续推送必须验证当前 HEAD 仍匹配。 -它曾是一个 341 行的模块；现在是一个把评测调优过的 prompt 数据、handler、 -确定性 helper 三者分开的包。 +它曾是一个 341 行的模块；现在将提交范围的 mutation gate、只读 PR 评审 +agent、评测调优过的 prompt 数据和确定性 helper 分开。 ## 包内布局（一个文件一个关注点） -- `__init__.py` —— import `steps` 以触发 `@step` 注册副作用；再导出公开契约（见下）。无逻辑。 +- `__init__.py` —— import `patch_gate`、`quality` 和 `steps` 以触发各自的 + `@step` 注册副作用；再导出公开契约（见下）。无逻辑。 - `prompts.py` —— 由评测得出的 prompt 数据：`_REVIEW_SYSTEM`、`_REVIEW_LENSES`、 `_REVIEW_MERGE`。约 120 行文本，**移出控制流之外**。 - `utils.py` —— 确定性、无 LLM 的 helper：`_sweep_targets`、`_render_review_md`、 `_SEVERITY_ORDER`。 -- `steps.py` —— 两个 `@step` handler：`review.patch_gate`（validation/read）、 - `agent.review_diff`（agent/read）。 +- `patch_gate.py` —— `review.patch_gate`（validation/read）：捕获已提交的 + base/head 范围、计算触发条件、审查精确 diff 并记录批准的 head；不依赖 + PR review agent 的 prompt/render 控制流。 +- `steps.py` —— `agent.review_diff`（agent/read）及其评审覆盖、二轮检查、 + 评论核验控制流；不批准或推送 mutation。 - `anchor.py` —— 基于代码片段的评论锚定（2026-08 新增）。 - `repo_tools.py` —— 只读的变更考古工具组（2026-08 新增）。 - `quality.py` —— `agent.assess_pr_quality`：一次 tool-less、只读模型调用， diff --git a/src/infermatrix_copilot/engine/steps/review/__init__.py b/src/infermatrix_copilot/engine/steps/review/__init__.py index 2e36fa8b4..4e019f082 100644 --- a/src/infermatrix_copilot/engine/steps/review/__init__.py +++ b/src/infermatrix_copilot/engine/steps/review/__init__.py @@ -1,16 +1,15 @@ """Review steps package: the conditional patch gate and the PR-review agent step (`review.patch_gate`, `agent.review_diff`). -Importing this package imports `steps` for its `@step` registration side -effects. This was one 341-line module; the prompt data now lives in `prompts`, -the deterministic sweep/render helpers in `utils`, and the two handlers in -`steps`. The public contract below (spec: `engine/steps/review`) is re-exported +Importing this package imports `patch_gate`, `quality`, and `steps` for +their `@step` registration side effects. The committed-range mutation gate +lives apart from the read-only review agent and its prompt/render helpers. The public contract below (spec: `engine/steps/review`) is re-exported so existing `from ..steps.review import X` importers are unchanged. """ from __future__ import annotations -from . import quality, steps # noqa: F401 (side-effect: registers steps) +from . import patch_gate, quality, steps # noqa: F401 (side-effect: registers steps) from .prompts import _REVIEW_LENSES # noqa: F401 from .utils import _render_review_md, _sweep_targets # noqa: F401 diff --git a/src/infermatrix_copilot/engine/steps/review/patch_gate.py b/src/infermatrix_copilot/engine/steps/review/patch_gate.py new file mode 100644 index 000000000..1b22f54e0 --- /dev/null +++ b/src/infermatrix_copilot/engine/steps/review/patch_gate.py @@ -0,0 +1,97 @@ +"""Committed-range patch approval before a PR mutation is pushed. + +This step owns only the pre-push review gate. The PR review agent in +``steps.py`` is a separate, read-only review workflow. +""" + +from __future__ import annotations + +import subprocess + +from ....review.change_set import ChangeSet, ChangeSetError +from ....review.diff_summary import build_diff_summary +from ....review.reviewer import run_patch_review +from ....review.triggers import evaluate_triggers +from ...step import FailureKind, StepContext, StepResult +from .._common import repo_path as _repo_path +from .._common import step + + +@step("review.patch_gate", "validation", "read", + "Conditional patch review; fail-closed before pushes.") +async def _patch_gate(ctx: StepContext) -> StepResult: + """Conditional Patch Review: cheap summary always; LLM review only on triggers.""" + repo = _repo_path(ctx) + if repo is None: + return StepResult(False, FailureKind.BLOCKED, "no repo path") + pre_push = bool(ctx.params.get("pre_push", False)) + change_set = None + base_ref = str(ctx.params.get("base_ref") or "HEAD") + if pre_push: + base_ref = str( + ctx.params.get("base_ref") or ctx.state.get("rebase_base_sha") + or ctx.state.get("pr_initial_head_sha") or "" + ) + try: + change_set = ChangeSet.capture(repo, base_ref) + except ChangeSetError as exc: + return StepResult(False, FailureKind.BLOCKED, str(exc)) + if not change_set.diff_text: + return StepResult(False, FailureKind.BLOCKED, + "no committed changes in the pre-push review range") + base_ref = change_set.base_sha + summary = build_diff_summary( + repo, base_ref=base_ref, + primary_files=tuple(ctx.state.get("primary_files", ())), trace=ctx.trace, + ) + if not summary.changed_files and not summary.full_file_writes: + if pre_push: + return StepResult(False, FailureKind.BLOCKED, + "no committed changes in the pre-push review range") + return StepResult(True, summary="no diff to review", + outputs={"fired": [], "verdict": "not_required"}) + fired = evaluate_triggers( + summary, ctx.settings, + touched_modules=tuple(ctx.state.get("touched_modules", ())), + pre_push=pre_push, + knowledge_edit=bool(ctx.state.get("knowledge_edit", False)), + high_risk_modules=ctx.state.get("high_risk_modules"), + ) + ctx.trace.record("patch_review_triggers", fired=fired) + if not fired: + return StepResult(True, summary="no review triggers fired", + outputs={"fired": [], "verdict": "not_required"}) + if change_set is not None: + diff = change_set.diff_text + if len(diff) > 60_000: + return StepResult(False, FailureKind.ESCALATE, + "committed diff exceeds the patch reviewer's 60000-character budget") + else: + diff_result = subprocess.run(["git", "diff", base_ref], + cwd=str(repo), capture_output=True, text=True, + encoding="utf-8", errors="replace", timeout=60) + if diff_result.returncode != 0: + return StepResult(False, FailureKind.BLOCKED, + f"cannot read patch diff: {diff_result.stderr[:300]}") + diff = diff_result.stdout + verdict = run_patch_review(ctx.llm, diff_text=diff, summary=summary, + fired_rules=fired, model=ctx.settings.reviewer) + ctx.trace.record("patch_review", fired=fired, verdict=verdict.verdict, + critiques=verdict.critiques) + if verdict.passing: + if change_set is not None: + approval = {"base_sha": change_set.base_sha, + "head_sha": change_set.head_sha} + ctx.state["approved_change_set"] = approval + return StepResult(True, summary=f"patch review lgtm (rules: {fired})", + outputs={"fired": fired, "verdict": "lgtm", + "state_updates": {"approved_change_set": approval}}) + return StepResult(True, summary=f"patch review lgtm (rules: {fired})", + outputs={"fired": fired, "verdict": "lgtm"}) + if verdict.verdict == "revise": + return StepResult(False, FailureKind.REPLAN, + f"patch review requests revision: {verdict.critiques[:3]}", + outputs={"verdict": "revise", "critiques": verdict.critiques}) + return StepResult(False, FailureKind.ESCALATE, + f"patch review verdict={verdict.verdict}: {verdict.critiques[:3]}", + outputs={"verdict": verdict.verdict, "critiques": verdict.critiques}) diff --git a/src/infermatrix_copilot/engine/steps/review/steps.py b/src/infermatrix_copilot/engine/steps/review/steps.py index a37d5dcfc..7f80cd930 100644 --- a/src/infermatrix_copilot/engine/steps/review/steps.py +++ b/src/infermatrix_copilot/engine/steps/review/steps.py @@ -1,5 +1,4 @@ -"""The two review step handlers: the conditional patch gate and the PR-review -agent step. +"""The read-only PR-review agent step. PR review is eval-informed (see eval/ANALYSIS.md): deterministic gate checks catch the merge-state/CI issue class no diff-only model caught; evidence- @@ -21,11 +20,7 @@ from ....llm import parse_json_reply -from ....review.diff_summary import build_diff_summary -from ....review.change_set import ChangeSet, ChangeSetError from ....review.planner import DEFAULT_STANDARD_LENSES, DEPTHS, plan_review -from ....review.reviewer import run_patch_review -from ....review.triggers import evaluate_triggers from ...step import FailureKind, StepContext, StepResult from .._common import gh_read_tools as _gh_read_tools from .._common import repo_path as _repo_path @@ -58,86 +53,6 @@ _EXPECTED_PLANNER_CAUSES = re.compile(r"\A(unavailable|rejected_depth:)") -@step("review.patch_gate", "validation", "read", - "Conditional patch review; fail-closed before pushes.") -async def _patch_gate(ctx: StepContext) -> StepResult: - """Conditional Patch Review: cheap summary always; LLM review only on triggers.""" - repo = _repo_path(ctx) - if repo is None: - return StepResult(False, FailureKind.BLOCKED, "no repo path") - pre_push = bool(ctx.params.get("pre_push", False)) - change_set = None - base_ref = str(ctx.params.get("base_ref") or "HEAD") - if pre_push: - base_ref = str( - ctx.params.get("base_ref") or ctx.state.get("rebase_base_sha") - or ctx.state.get("pr_initial_head_sha") or "" - ) - try: - change_set = ChangeSet.capture(repo, base_ref) - except ChangeSetError as exc: - return StepResult(False, FailureKind.BLOCKED, str(exc)) - if not change_set.diff_text: - return StepResult(False, FailureKind.BLOCKED, - "no committed changes in the pre-push review range") - base_ref = change_set.base_sha - summary = build_diff_summary( - repo, base_ref=base_ref, - primary_files=tuple(ctx.state.get("primary_files", ())), trace=ctx.trace, - ) - if not summary.changed_files and not summary.full_file_writes: - if pre_push: - return StepResult(False, FailureKind.BLOCKED, - "no committed changes in the pre-push review range") - return StepResult(True, summary="no diff to review", - outputs={"fired": [], "verdict": "not_required"}) - fired = evaluate_triggers( - summary, ctx.settings, - touched_modules=tuple(ctx.state.get("touched_modules", ())), - pre_push=pre_push, - knowledge_edit=bool(ctx.state.get("knowledge_edit", False)), - high_risk_modules=ctx.state.get("high_risk_modules"), - ) - ctx.trace.record("patch_review_triggers", fired=fired) - if not fired: - return StepResult(True, summary="no review triggers fired", - outputs={"fired": [], "verdict": "not_required"}) - if change_set is not None: - diff = change_set.diff_text - if len(diff) > 60_000: - return StepResult(False, FailureKind.ESCALATE, - "committed diff exceeds the patch reviewer's 60000-character budget") - else: - diff_result = subprocess.run(["git", "diff", base_ref], - cwd=str(repo), capture_output=True, text=True, - encoding="utf-8", errors="replace", timeout=60) - if diff_result.returncode != 0: - return StepResult(False, FailureKind.BLOCKED, - f"cannot read patch diff: {diff_result.stderr[:300]}") - diff = diff_result.stdout - verdict = run_patch_review(ctx.llm, diff_text=diff, summary=summary, - fired_rules=fired, model=ctx.settings.reviewer) - ctx.trace.record("patch_review", fired=fired, verdict=verdict.verdict, - critiques=verdict.critiques) - if verdict.passing: - if change_set is not None: - approval = {"base_sha": change_set.base_sha, - "head_sha": change_set.head_sha} - ctx.state["approved_change_set"] = approval - return StepResult(True, summary=f"patch review lgtm (rules: {fired})", - outputs={"fired": fired, "verdict": "lgtm", - "state_updates": {"approved_change_set": approval}}) - return StepResult(True, summary=f"patch review lgtm (rules: {fired})", - outputs={"fired": fired, "verdict": "lgtm"}) - if verdict.verdict == "revise": - return StepResult(False, FailureKind.REPLAN, - f"patch review requests revision: {verdict.critiques[:3]}", - outputs={"verdict": "revise", "critiques": verdict.critiques}) - return StepResult(False, FailureKind.ESCALATE, - f"patch review verdict={verdict.verdict}: {verdict.critiques[:3]}", - outputs={"verdict": verdict.verdict, "critiques": verdict.critiques}) - - def _risk_paths(adapter, settings) -> tuple[str, ...]: """High-risk path prefixes for the depth planner: the adapter's `risk: high` modules' local_paths when available, else the settings module-name diff --git a/test/test_pr_steps.py b/test/test_pr_steps.py index 495b2c05c..a24bc5555 100644 --- a/test/test_pr_steps.py +++ b/test/test_pr_steps.py @@ -81,7 +81,7 @@ def test_checkout_report_only_disallows_push(registry, settings, trace, tmp_path def test_patch_gate_reviews_committed_range_and_push_binds_head( registry, settings, trace, tmp_path, git_repo, monkeypatch, ): - from infermatrix_copilot.engine.steps.review import steps as review_steps + from infermatrix_copilot.engine.steps.review import patch_gate from infermatrix_copilot.review.reviewer import ReviewVerdict base = _git(git_repo, "rev-parse", "HEAD") @@ -92,7 +92,7 @@ def test_patch_gate_reviews_committed_range_and_push_binds_head( def approve(_llm, *, diff_text, **_kwargs): seen.append(diff_text) return ReviewVerdict("lgtm") - monkeypatch.setattr(review_steps, "run_patch_review", approve) + monkeypatch.setattr(patch_gate, "run_patch_review", approve) state = { "repo_path": str(git_repo), "task_spec": {"kind": "pr_debug"}, "pr_initial_head_sha": base,