mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
[F044] pr_pass gate remediation points the reviewer at pr_fail, not i_am_blocked
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.
This commit is contained in:
@@ -1855,7 +1855,7 @@ class Choreographer:
|
|||||||
)
|
)
|
||||||
|
|
||||||
async def _toolchain_broken_guard(
|
async def _toolchain_broken_guard(
|
||||||
self, agent_id: UUID, task: Any
|
self, agent_id: UUID, task: Any, *, reviewer: bool = False
|
||||||
) -> Envelope | None:
|
) -> Envelope | None:
|
||||||
"""Refuse a delivery gate when the acting agent's workspace cannot run
|
"""Refuse a delivery gate when the acting agent's workspace cannot run
|
||||||
the project's suite (interpreter mismatch).
|
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
|
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
|
flag is off; only a recorded ``broken`` status blocks — a missing or
|
||||||
``unknown`` status never strands a task (fail-open).
|
``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
|
from roboco.config import settings as _settings
|
||||||
|
|
||||||
@@ -1887,17 +1892,27 @@ class Choreographer:
|
|||||||
)
|
)
|
||||||
if status != "broken":
|
if status != "broken":
|
||||||
return None
|
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(
|
return Envelope.invalid_state(
|
||||||
message=(
|
message=(
|
||||||
"the project's test suite cannot be executed in this workspace "
|
"the project's test suite cannot be executed in this workspace "
|
||||||
"(interpreter mismatch) — verifying by reading source is hollow"
|
"(interpreter mismatch) — verifying by reading source is hollow"
|
||||||
),
|
),
|
||||||
remediate=(
|
remediate=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"
|
|
||||||
),
|
|
||||||
context_briefing={},
|
context_briefing={},
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -1951,31 +1966,47 @@ class Choreographer:
|
|||||||
A ``block`` finding (a misplaced definition, a lint suppression) or a
|
A ``block`` finding (a misplaced definition, a lint suppression) or a
|
||||||
validator that could not run returns a rejection with the offending
|
validator that could not run returns a rejection with the offending
|
||||||
``file:line`` + fix hint. ``warn`` findings never block. Inert when the
|
``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
|
from roboco.config import settings as _settings
|
||||||
|
|
||||||
if not _settings.conventions_enabled:
|
if not _settings.conventions_enabled:
|
||||||
return None
|
return None
|
||||||
result = await self.git.conventions_check_for_task(agent_id, task)
|
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
|
@staticmethod
|
||||||
def _conventions_rejection(
|
def _conventions_rejection(
|
||||||
result: dict[str, Any], briefing: dict[str, Any]
|
result: dict[str, Any], briefing: dict[str, Any], *, reviewer: bool = False
|
||||||
) -> Envelope | None:
|
) -> 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 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(
|
return Envelope.invalid_state(
|
||||||
message=(
|
message=(
|
||||||
"the architectural-conventions validator could not run on "
|
"the architectural-conventions validator could not run on "
|
||||||
"your changed files — this blocks rather than passing silently"
|
"your changed files — this blocks rather than passing silently"
|
||||||
),
|
),
|
||||||
remediate=(
|
remediate=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"
|
|
||||||
),
|
|
||||||
context_briefing=briefing,
|
context_briefing=briefing,
|
||||||
)
|
)
|
||||||
blocks = [f for f in result.get("findings", []) if f.get("level") == "block"]
|
blocks = [f for f in result.get("findings", []) if f.get("level") == "block"]
|
||||||
|
|||||||
@@ -54,7 +54,7 @@ class ChoreographerHelpers:
|
|||||||
raise NotImplementedError
|
raise NotImplementedError
|
||||||
|
|
||||||
async def _toolchain_broken_guard(
|
async def _toolchain_broken_guard(
|
||||||
self, agent_id: UUID, task: Any
|
self, agent_id: UUID, task: Any, *, reviewer: bool = False
|
||||||
) -> Envelope | None:
|
) -> Envelope | None:
|
||||||
raise NotImplementedError
|
raise NotImplementedError
|
||||||
|
|
||||||
|
|||||||
@@ -340,7 +340,7 @@ class PRGateMixin(_Base):
|
|||||||
None to proceed. Both guards are inert when their flag is off.
|
None to proceed. Both guards are inert when their flag is off.
|
||||||
"""
|
"""
|
||||||
guards = (
|
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),
|
lambda: self._conventions_guard(reviewer_agent_id, t, briefing),
|
||||||
)
|
)
|
||||||
for guard in guards:
|
for guard in guards:
|
||||||
|
|||||||
@@ -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
|
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
|
@pytest.mark.asyncio
|
||||||
async def test_pr_pass_guard_inert_when_flag_off(
|
async def test_pr_pass_guard_inert_when_flag_off(
|
||||||
monkeypatch: pytest.MonkeyPatch,
|
monkeypatch: pytest.MonkeyPatch,
|
||||||
|
|||||||
@@ -93,3 +93,37 @@ async def test_guard_silent_when_no_marker(monkeypatch: pytest.MonkeyPatch) -> N
|
|||||||
env = await c._toolchain_broken_guard(uuid4(), MagicMock())
|
env = await c._toolchain_broken_guard(uuid4(), MagicMock())
|
||||||
assert env is None
|
assert env is None
|
||||||
assert not any(e.get("event") == "toolchain.unverified_gate_pass" for e in logs)
|
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"]
|
||||||
|
|||||||
Reference in New Issue
Block a user