diff --git a/doc/architecture/SPEC/app/core.md b/doc/architecture/SPEC/app/core.md index 035aa041..20ac6611 100644 --- a/doc/architecture/SPEC/app/core.md +++ b/doc/architecture/SPEC/app/core.md @@ -3,8 +3,9 @@ `Copilot` is the headless workflow application. It resolves a TaskSpec, -enforces plan review and confirmation, executes or resumes a playbook, and -reserves/executes durable runs. The CLI imports it through `cli.copilot` for +delegates reviewer verdict policy to `review.plan_gate`, confirms when needed, +executes or resumes a playbook, and reserves/executes durable runs. The CLI +imports it through `cli.copilot` for compatibility; the application never imports CLI or MCP transports. The pre-execution gate runs before a normal CLI run directory is created. @@ -14,5 +15,7 @@ binding; a run ID must stay contained under the configured run root. Existing checkpoint keys, run IDs, and blocked exit codes remain stable. `GateOutcome`, `_mode_review_context`, and `Copilot` retain their import path -through the compatibility module. Only orchestration belongs here; command -argument parsing and stdio protocol registration remain transport concerns. +through the compatibility module. The application passes its reviewer to the +plan gate, preserving the no-human fail-closed rule. Only orchestration belongs +here; command argument parsing and stdio protocol registration remain +transport concerns. diff --git a/doc/architecture/SPEC/review/plan_gate.md b/doc/architecture/SPEC/review/plan_gate.md new file mode 100644 index 00000000..2f7d4a6b --- /dev/null +++ b/doc/architecture/SPEC/review/plan_gate.md @@ -0,0 +1,20 @@ +# review/plan_gate.py — plan-review policy + + + +This component builds the authoritative mode-aware context for a planned +playbook and interprets the injected plan reviewer's verdict. Exact reused +plans that do not require review pass without a reviewer call. A `block` +verdict always stops; `revise` or `unavailable` stops an unattended `--yes` +run because no person remains to inspect the plan. Interactive runs surface +those verdicts and leave final confirmation to `app.core`. + +The mode context uses the executor's `when` evaluation and resolved rebase +mode, so inactive write/push steps are not presented as active. It does not +execute a plan, print notices, prompt for confirmation, create a run directory, +or own a transport. It returns a `PlanGateResult` with the decision and notices; +`app.core` renders them and keeps `_mode_review_context` and +`_plan_review_gate` compatibility entry points while supplying the reviewer. + +Verification: `test_planner_playbooks.py`, `test_curator.py`, and +`test_phase_b.py` cover verdicts, mode context, and the application gate. diff --git a/src/infermatrix_copilot/app/core.py b/src/infermatrix_copilot/app/core.py index 45be44ee..465ca315 100644 --- a/src/infermatrix_copilot/app/core.py +++ b/src/infermatrix_copilot/app/core.py @@ -17,8 +17,6 @@ from pathlib import Path from typing import NamedTuple -import yaml - from .. import idempotency as idem from .. import run_status as rs from ..config import Settings, TierNotConfiguredError @@ -31,6 +29,7 @@ from ..notify import BLOCKED_EXIT, Notifier from ..playbooks.store import PlaybookStore, parse_playbook, playbook_to_doc from ..push import PushPolicy +from ..review.plan_gate import _mode_review_context, review_plan_gate from ..review.reviewer import run_plan_review from ..run_trace import RunTrace from ..task_spec import TaskSpec @@ -57,43 +56,6 @@ def stop(cls, exit_code: int) -> "GateOutcome": return cls(proceed=False, exit_code=exit_code) -def _mode_review_context(playbook, spec) -> str: - """Plan-review context for MODE-AWARE playbooks: the reviewer sees the - raw yaml's FULL step list, but the `when:` gates resolve against the - ALREADY-RESOLVED mode (`resolve_effective_mode` runs before the review - gate) — without this context a report-only plan looks like it runs its - write/push steps and gets spuriously blocked. The active-step set is - the same mechanical truth the executor computes, never prose.""" - if not getattr(playbook, "mode_aware", False): - return "" - mode = str((getattr(spec, "params", None) or {}) - .get("rebase_mode", "") or "") - if not mode: - return "" - from ..engine.executor import _eval_when - from ..rebase_engine.modes import mode_state_flags - - flags = {"task_spec": {}, **mode_state_flags(mode)} - active = [s.get("id", s.get("step", "?")) - for s in playbook_to_doc(playbook).get("steps", []) - if "when" not in s or _eval_when(s["when"], flags)] - repo = str(getattr(spec, "repo", "") or "") - repo_line = (f"\nTarget repo (authoritative): {repo!r} — bound at " - "runtime from the TaskSpec; the yaml `repos:` list is a " - "planner RECALL FILTER where empty means repo-neutral, " - "never untargeted." if repo else "") - return (f"\n\nResolved mode context (authoritative): " - f"rebase_mode={mode}. Under this mode the `when:` gates run " - f"ONLY these steps: {active}. Every other listed step is " - "statically gated OFF for this run — judge the plan for THIS " - "mode's step set." - + repo_line + - "\nWrite/push governance (authoritative): the mode's own " - "push/CI steps are governed at runtime by the push-gate " - "ruling, guard_push, and the ALLOW_PUSH env double-gate — " - "the task tier does not forbid steps this mode activates.") - - class Copilot: """Orchestration core: resolves a TaskSpec to a playbook (reuse > adapt > generate), runs it through the plan-review gate + confirmation into the @@ -165,42 +127,14 @@ def resolve(self, spec: TaskSpec) -> Resolution: def _plan_review_gate(self, resolution: Resolution, spec: TaskSpec, assume_yes: bool) -> bool: - """Inline Plan-Review for adapted/generated plans. LLM verdict shown in - the session; block stops. A non-`lgtm` verdict is only ever *surfaced* - — the human `[y/N]` is what actually gates it — so when `--yes` removes - that human, the same verdict must stop the run instead - (`ReviewVerdict.passing`, SPEC C6: only `lgtm` passes).""" - if not resolution.requires_review: - return True - doc = yaml.safe_dump(playbook_to_doc(resolution.playbook), sort_keys=False) - task_text = spec.describe() + _mode_review_context( - resolution.playbook, spec) - verdict = run_plan_review(self.llm, playbook_doc=doc, - task=task_text, - model=self.settings.reviewer) - if verdict.verdict != "unavailable": - print(f" plan review: {verdict.verdict}" - + (f" — {verdict.critiques}" if verdict.critiques else "")) - if verdict.verdict == "block": - print("✋ plan blocked by reviewer.") - return False - if verdict.passing: - return True - # Everything below is non-passing: `revise`, or `unavailable`. Both used - # to return True on the strength of a confirmation that `--yes` had - # already deleted, so an unattended run executed an unvetted plan on an - # unread verdict. Measured on the release matrix: an unparseable review - # let three of four backends run a pr-rebase plan through to its push - # gate, while the one backend whose reviewer parsed cleanly BLOCKED the - # very same plan. - if assume_yes: - reason = ("no reviewer LLM" if verdict.verdict == "unavailable" - else f"plan review returned {verdict.verdict}") - print(f"✋ {reason} and --yes leaves no human to gate it — blocked.") - return False - if verdict.verdict == "unavailable": - print(" ⚠ no reviewer LLM — your confirmation is the plan-review gate") - return True # revise/unavailable, surfaced to the user before their confirm + """Apply the reviewer verdict policy before human confirmation.""" + result = review_plan_gate( + resolution, spec, self.llm, self.settings.reviewer, assume_yes, + review_fn=run_plan_review, + ) + for notice in result.notices: + print(notice) + return result.allowed def _gate_and_confirm(self, resolution: Resolution, spec: TaskSpec, assume_yes: bool, *, prompt: str = "Proceed?", diff --git a/src/infermatrix_copilot/review/plan_gate.py b/src/infermatrix_copilot/review/plan_gate.py new file mode 100644 index 00000000..9f96a46c --- /dev/null +++ b/src/infermatrix_copilot/review/plan_gate.py @@ -0,0 +1,85 @@ +"""Plan-review verdict policy and mode-aware context for generated playbooks. + +The application supplies its reviewer and retains the human confirmation gate. +""" + +from __future__ import annotations + +from dataclasses import dataclass + +import yaml + +from ..playbooks.store import playbook_to_doc + + +def _mode_review_context(playbook, spec) -> str: + """Plan-review context for MODE-AWARE playbooks: the reviewer sees the + raw yaml's FULL step list, but the `when:` gates resolve against the + ALREADY-RESOLVED mode (`resolve_effective_mode` runs before the review + gate) — without this context a report-only plan looks like it runs its + write/push steps and gets spuriously blocked. The active-step set is + the same mechanical truth the executor computes, never prose.""" + if not getattr(playbook, "mode_aware", False): + return "" + mode = str((getattr(spec, "params", None) or {}) + .get("rebase_mode", "") or "") + if not mode: + return "" + from ..engine.executor import _eval_when + from ..rebase_engine.modes import mode_state_flags + + flags = {"task_spec": {}, **mode_state_flags(mode)} + active = [s.get("id", s.get("step", "?")) + for s in playbook_to_doc(playbook).get("steps", []) + if "when" not in s or _eval_when(s["when"], flags)] + repo = str(getattr(spec, "repo", "") or "") + repo_line = (f"\nTarget repo (authoritative): {repo!r} — bound at " + "runtime from the TaskSpec; the yaml `repos:` list is a " + "planner RECALL FILTER where empty means repo-neutral, " + "never untargeted." if repo else "") + return (f"\n\nResolved mode context (authoritative): " + f"rebase_mode={mode}. Under this mode the `when:` gates run " + f"ONLY these steps: {active}. Every other listed step is " + "statically gated OFF for this run — judge the plan for THIS " + "mode's step set." + + repo_line + + "\nWrite/push governance (authoritative): the mode's own " + "push/CI steps are governed at runtime by the push-gate " + "ruling, guard_push, and the ALLOW_PUSH env double-gate — " + "the task tier does not forbid steps this mode activates.") + + +@dataclass(frozen=True) +class PlanGateResult: + allowed: bool + notices: tuple[str, ...] = () + + +def review_plan_gate( + resolution, spec, llm, model, assume_yes, *, review_fn +) -> PlanGateResult: + """Interpret a reviewer verdict without owning confirmation or output.""" + if not resolution.requires_review: + return PlanGateResult(True) + doc = yaml.safe_dump(playbook_to_doc(resolution.playbook), sort_keys=False) + task_text = spec.describe() + _mode_review_context(resolution.playbook, spec) + verdict = review_fn(llm, playbook_doc=doc, task=task_text, model=model) + notices = [] + if verdict.verdict != "unavailable": + notices.append(f" plan review: {verdict.verdict}" + + (f" — {verdict.critiques}" if verdict.critiques else "")) + if verdict.verdict == "block": + notices.append("✋ plan blocked by reviewer.") + return PlanGateResult(False, tuple(notices)) + if verdict.passing: + return PlanGateResult(True, tuple(notices)) + # A non-passing verdict can only proceed through explicit human review. + # --yes removes that confirmation, so revise/unavailable must block. + if assume_yes: + reason = ("no reviewer LLM" if verdict.verdict == "unavailable" + else f"plan review returned {verdict.verdict}") + notices.append(f"✋ {reason} and --yes leaves no human to gate it — blocked.") + return PlanGateResult(False, tuple(notices)) + if verdict.verdict == "unavailable": + notices.append(" ⚠ no reviewer LLM — your confirmation is the plan-review gate") + return PlanGateResult(True, tuple(notices))