Separate pre-push patch gate from PR review agent

doc/architecture/SPEC/engine/steps/review.md

@@ -2,25 +2,29 @@
 
 <!-- verified-against: 2026-09-26 -->
 
-`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，
 以及它们仓库中立的 prompt 体系。
 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、只读模型调用，

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
 

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})

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

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,