From 05a5852bb5130a0fcb6e70041d9d4c6634c32b98 Mon Sep 17 00:00:00 2001 From: Renn F Date: Sun, 28 Jun 2026 11:30:55 +0200 Subject: [PATCH] [F044] pr_pass gate remediation points the reviewer at pr_fail, not i_am_blocked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pr_pass gate runs the toolchain + conventions guards on the REVIEWER's workspace, but their remediation text said 'call i_am_blocked' — a verb the PR reviewer does not have. The reviewer would chase a verb they cannot call instead of rejecting the PR. Make the guards reviewer-aware: a reviewer=True flag (passed by _pr_pass_blocked) switches the remediation to pr_fail(issues=[...]) — the reviewer's reject lever, sending the PR back to needs_revision for the dev to fix the environment / validator. The dev (i_am_done) path keeps i_am_blocked, which a dev does have. _conventions_guard (the pr_pass path) now passes reviewer=True through to _conventions_rejection. --- .../services/gateway/choreographer/_impl.py | 63 ++++++++++++++----- .../gateway/choreographer/_protocol.py | 2 +- .../services/gateway/choreographer/pr_gate.py | 2 +- .../gateway/test_conventions_gate_pr_pass.py | 16 +++++ tests/unit/gateway/test_toolchain_guard.py | 34 ++++++++++ 5 files changed, 99 insertions(+), 18 deletions(-) diff --git a/roboco/services/gateway/choreographer/_impl.py b/roboco/services/gateway/choreographer/_impl.py index 6e24462b..d5db00b1 100644 --- a/roboco/services/gateway/choreographer/_impl.py +++ b/roboco/services/gateway/choreographer/_impl.py @@ -1855,7 +1855,7 @@ class Choreographer: ) async def _toolchain_broken_guard( - self, agent_id: UUID, task: Any + self, agent_id: UUID, task: Any, *, reviewer: bool = False ) -> Envelope | None: """Refuse a delivery gate when the acting agent's workspace cannot run the project's suite (interpreter mismatch). @@ -1864,6 +1864,11 @@ class Choreographer: source", which the QA + PR-review gates exist to prevent. Inert when the flag is off; only a recorded ``broken`` status blocks — a missing or ``unknown`` status never strands a task (fail-open). + + ``reviewer=True`` for the pr_pass gate: a PR reviewer has no + ``i_am_blocked`` verb, so the remediation points at ``pr_fail`` (their + reject lever, sending the PR back to needs_revision for the dev to fix + the environment) instead of a verb they cannot call (F044). """ from roboco.config import settings as _settings @@ -1887,17 +1892,27 @@ class Choreographer: ) if status != "broken": return None + if reviewer: + remediate = ( + "the workspace Python does not match the project's requirement, " + "so the suite cannot be executed to verify this PR. call " + "pr_fail(issues=['toolchain: interpreter mismatch — suite " + "cannot run']) so the PR returns to needs_revision and the dev " + "rebuilds the environment — do NOT pr_pass on a source read" + ) + else: + remediate = ( + "the workspace Python does not match the project's requirement; " + "call i_am_blocked(reason='toolchain') so the environment is " + "rebuilt against the right interpreter — do NOT pass on a " + "source read" + ) return Envelope.invalid_state( message=( "the project's test suite cannot be executed in this workspace " "(interpreter mismatch) — verifying by reading source is hollow" ), - remediate=( - "the workspace Python does not match the project's requirement; " - "call i_am_blocked(reason='toolchain') so the environment is " - "rebuilt against the right interpreter — do NOT pass on a " - "source read" - ), + remediate=remediate, context_briefing={}, ) @@ -1951,31 +1966,47 @@ class Choreographer: A ``block`` finding (a misplaced definition, a lint suppression) or a validator that could not run returns a rejection with the offending ``file:line`` + fix hint. ``warn`` findings never block. Inert when the - flag is off. Shared by the i_am_done and pr_pass gates. + flag is off. This is the pr_pass (reviewer) path — the remediation is + reviewer-aware (``pr_fail``, not ``i_am_blocked`` which a reviewer + lacks) via ``_conventions_rejection(..., reviewer=True)`` (F044). """ from roboco.config import settings as _settings if not _settings.conventions_enabled: return None result = await self.git.conventions_check_for_task(agent_id, task) - return self._conventions_rejection(result, briefing) + return self._conventions_rejection(result, briefing, reviewer=True) @staticmethod def _conventions_rejection( - result: dict[str, Any], briefing: dict[str, Any] + result: dict[str, Any], briefing: dict[str, Any], *, reviewer: bool = False ) -> Envelope | None: - """Turn a validator result into a rejection Envelope, or None to pass.""" + """Turn a validator result into a rejection Envelope, or None to pass. + + ``reviewer=True`` for the pr_pass gate: a reviewer has no + ``i_am_blocked`` verb, so the could_not_run remediation points at + ``pr_fail`` instead (F044). + """ if result.get("could_not_run"): + if reviewer: + remediate = ( + "the validator failed to analyze the diff (a parse or grammar " + "error). call pr_fail(issues=['conventions: validator could " + "not run on the changed files']) so the PR returns to " + "needs_revision and the dev resolves it — do NOT pr_pass" + ) + else: + remediate = ( + "the validator failed to analyze the diff (a parse or grammar " + "error). resolve it and call the verb again; if it persists, " + "call i_am_blocked" + ) return Envelope.invalid_state( message=( "the architectural-conventions validator could not run on " "your changed files — this blocks rather than passing silently" ), - remediate=( - "the validator failed to analyze the diff (a parse or grammar " - "error). resolve it and call the verb again; if it persists, " - "call i_am_blocked" - ), + remediate=remediate, context_briefing=briefing, ) blocks = [f for f in result.get("findings", []) if f.get("level") == "block"] diff --git a/roboco/services/gateway/choreographer/_protocol.py b/roboco/services/gateway/choreographer/_protocol.py index 1806e95e..97534fd5 100644 --- a/roboco/services/gateway/choreographer/_protocol.py +++ b/roboco/services/gateway/choreographer/_protocol.py @@ -54,7 +54,7 @@ class ChoreographerHelpers: raise NotImplementedError async def _toolchain_broken_guard( - self, agent_id: UUID, task: Any + self, agent_id: UUID, task: Any, *, reviewer: bool = False ) -> Envelope | None: raise NotImplementedError diff --git a/roboco/services/gateway/choreographer/pr_gate.py b/roboco/services/gateway/choreographer/pr_gate.py index a580f98d..3aaa4322 100644 --- a/roboco/services/gateway/choreographer/pr_gate.py +++ b/roboco/services/gateway/choreographer/pr_gate.py @@ -340,7 +340,7 @@ class PRGateMixin(_Base): None to proceed. Both guards are inert when their flag is off. """ guards = ( - lambda: self._toolchain_broken_guard(reviewer_agent_id, t), + lambda: self._toolchain_broken_guard(reviewer_agent_id, t, reviewer=True), lambda: self._conventions_guard(reviewer_agent_id, t, briefing), ) for guard in guards: diff --git a/tests/unit/gateway/test_conventions_gate_pr_pass.py b/tests/unit/gateway/test_conventions_gate_pr_pass.py index 30bda5fc..f505c450 100644 --- a/tests/unit/gateway/test_conventions_gate_pr_pass.py +++ b/tests/unit/gateway/test_conventions_gate_pr_pass.py @@ -78,6 +78,22 @@ async def test_pr_pass_guard_blocks_when_validator_cannot_run( assert await c._conventions_guard(uuid4(), MagicMock(), {}) is not None +@pytest.mark.asyncio +async def test_pr_pass_guard_could_not_run_remediation_uses_pr_fail( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # F044: _conventions_guard is the pr_pass (reviewer) path. A reviewer has no + # i_am_blocked verb, so the could_not_run remediation must point at pr_fail + # (the reviewer's reject lever) — not tell them to call a verb they lack. + monkeypatch.setattr(settings, "conventions_enabled", True) + c = _make_choreographer(check_result={"findings": [], "could_not_run": True}) + env = await c._conventions_guard(uuid4(), MagicMock(), {}) + assert env is not None + body = env.as_dict() + assert "i_am_blocked" not in body["remediate"] + assert "pr_fail" in body["remediate"] + + @pytest.mark.asyncio async def test_pr_pass_guard_inert_when_flag_off( monkeypatch: pytest.MonkeyPatch, diff --git a/tests/unit/gateway/test_toolchain_guard.py b/tests/unit/gateway/test_toolchain_guard.py index e112d3aa..7ce4fdb8 100644 --- a/tests/unit/gateway/test_toolchain_guard.py +++ b/tests/unit/gateway/test_toolchain_guard.py @@ -93,3 +93,37 @@ async def test_guard_silent_when_no_marker(monkeypatch: pytest.MonkeyPatch) -> N env = await c._toolchain_broken_guard(uuid4(), MagicMock()) assert env is None assert not any(e.get("event") == "toolchain.unverified_gate_pass" for e in logs) + + +@pytest.mark.asyncio +async def test_guard_reviewer_remediation_uses_pr_fail_not_i_am_blocked( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # F044: the pr_pass gate runs this guard on the REVIEWER's workspace. A PR + # reviewer has no i_am_blocked verb, so the dev-path remediation ("call + # i_am_blocked(reason='toolchain')") sends them to a verb they cannot call. + # The reviewer's reject lever is pr_fail — the remediation must point there + # so the PR goes back to needs_revision for the dev to fix the environment. + monkeypatch.setattr(settings, "toolchain_match_enabled", True) + c = _make_choreographer(status="broken") + env = await c._toolchain_broken_guard(uuid4(), MagicMock(), reviewer=True) + assert env is not None + body = env.as_dict() + assert body["error"] == "invalid_state" + assert "i_am_blocked" not in body["remediate"] + assert "pr_fail" in body["remediate"] + + +@pytest.mark.asyncio +async def test_guard_dev_remediation_still_uses_i_am_blocked( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # F044: the dev (i_am_done) path keeps i_am_blocked — a dev DOES have that + # verb, so the original remediation is correct there. The reviewer flag must + # not change the dev-path wording. + monkeypatch.setattr(settings, "toolchain_match_enabled", True) + c = _make_choreographer(status="broken") + env = await c._toolchain_broken_guard(uuid4(), MagicMock()) + assert env is not None + body = env.as_dict() + assert "i_am_blocked" in body["remediate"]