diff --git a/CHANGELOG.md b/CHANGELOG.md index 79526ef1..d88798b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), - **Pitch auto-provisioning is now idempotent — a re-approval no longer collides.** When a pitch's approval partially failed and its DB writes rolled back while the created GitHub repos survived, re-approving it tried to re-create the repos and re-insert the product → project cell mappings, hitting a duplicate-key crash on `(product_id, team)` and leaving an orphaned product that could not be cleaned up. Provisioning now reuses an existing Project (by slug) and an existing Product (by slug, refreshing its cell map with delete-before-insert ordering) instead of re-creating them, so a re-approval converges cleanly. First-time provisioning is unchanged. - **The `mypy roboco/ tests/` quality gate is green again.** A batch of test files carried type errors that turned the gate red (SQLAlchemy `.id` passed where `uuid.UUID` was expected, a couple of missing return annotations, an invariant-`list` argument, and a `None`-attribute access). Each is now typed correctly so the full gate passes. (The deeper cause — many ORM columns annotated `Mapped[UUID]` against SQLAlchemy's `UUID` type rather than `uuid.UUID` — is noted for a separate, dedicated cleanup.) +- **An external-PR review can no longer record a verdict that contradicts its own summary.** The inbound-PR reviewer verb (`post_pr_review`) derived both the recorded verdict and the posted GitHub review event solely from its `event` argument, which defaults to `REQUEST_CHANGES` — and, unlike the in-path gate's `pr_fail`, it never required any findings. So a reviewer that concluded "approve" in the summary but left `event` at its default filed (and posted to the contributor's PR) a blocking "changes requested" with nothing cited. The verb now enforces a verdict↔findings invariant before any record or post: `REQUEST_CHANGES` must cite at least one finding (almost always a forgotten `event='APPROVE'`), and `APPROVE` may not carry a blocker/major finding — rejected with a clear remediation otherwise. ## [0.12.0] - 2026-06-25 diff --git a/roboco/api/schemas/v1/flow.py b/roboco/api/schemas/v1/flow.py index 17de9468..17d48824 100644 --- a/roboco/api/schemas/v1/flow.py +++ b/roboco/api/schemas/v1/flow.py @@ -133,7 +133,14 @@ class ClaimPrReviewRequest(BaseModel): class PostPrReviewRequest(BaseModel): task_id: UUID body: str = Field(..., min_length=1) - event: str = "REQUEST_CHANGES" + event: str = Field( + default="REQUEST_CHANGES", + description=( + "APPROVE, REQUEST_CHANGES, or COMMENT. The verdict must match the " + "findings: REQUEST_CHANGES needs >=1 finding; APPROVE may not carry a " + "blocker/major finding. Pass APPROVE explicitly to approve a clean PR." + ), + ) findings: list[dict[str, Any]] = Field( default_factory=list, description=( diff --git a/roboco/foundation/policy/content/__init__.py b/roboco/foundation/policy/content/__init__.py index ef048597..ca683442 100644 --- a/roboco/foundation/policy/content/__init__.py +++ b/roboco/foundation/policy/content/__init__.py @@ -20,6 +20,7 @@ from .models import ( ResumptionNote, TaskDescription, WorkUnit, + pr_review_conflict, required_shape, validate_content, ) @@ -42,6 +43,7 @@ __all__ = [ "TaskDescription", "Verdict", "WorkUnit", + "pr_review_conflict", "required_shape", "validate_content", ] diff --git a/roboco/foundation/policy/content/models.py b/roboco/foundation/policy/content/models.py index 6c73abb4..bbbb1894 100644 --- a/roboco/foundation/policy/content/models.py +++ b/roboco/foundation/policy/content/models.py @@ -178,6 +178,52 @@ class PrReviewContent(_Content): return _join(parts) +# GitHub review events the PR-reviewer verb accepts. +_EVENT_APPROVE = "APPROVE" +_EVENT_REQUEST_CHANGES = "REQUEST_CHANGES" +_BLOCKING_SEVERITIES = frozenset({Severity.BLOCKER, Severity.MAJOR}) + + +def pr_review_conflict( + event: str, findings: list[dict[str, Any]] | None +) -> tuple[str, str] | None: + """Reason a PR-review ``(event, findings)`` pair is self-contradictory. + + Returns ``(message, remediate)`` when the verdict the event implies cannot + be reconciled with the findings, else ``None``. The recorded + ``notes_structured.pr_review.verdict`` and the posted GitHub review event + both derive from ``event``, so this is what stops an approving review from + being filed — or posted to a contributor's PR — as a blocking + "changes requested": + + - ``REQUEST_CHANGES`` with no findings blocks a PR without stating why + (parity with the in-path gate's ``pr_fail`` "at least one issue" rule); + it is almost always a forgotten ``event='APPROVE'``. + - ``APPROVE`` over a ``blocker``/``major`` finding approves a known + significant defect. + + A neutral ``COMMENT`` carries no verdict and is never in conflict. + """ + items = findings or [] + if event == _EVENT_APPROVE: + if any( + str(f.get("severity", "")).lower() in _BLOCKING_SEVERITIES for f in items + ): + return ( + "cannot APPROVE a PR that has blocker/major findings", + "resolve the blocking findings (or lower their severity), or " + "post event='REQUEST_CHANGES'", + ) + return None + if event == _EVENT_REQUEST_CHANGES and not items: + return ( + "a changes-requested review must cite at least one finding", + "pass findings=[{file, severity, expected, actual}, ...] to request " + "changes, or event='APPROVE' to approve a clean PR", + ) + return None + + class TaskDescription(_Content): """A well-formed task description (shared by PM delegate + Intake draft).""" diff --git a/roboco/mcp/flow_server.py b/roboco/mcp/flow_server.py index c5ba0162..166000bb 100644 --- a/roboco/mcp/flow_server.py +++ b/roboco/mcp/flow_server.py @@ -376,8 +376,11 @@ def post_pr_review( {file, line?, severity (blocker|major|minor|nit), expected, actual}. When findings are given, the GitHub comment is GENERATED in the RoboCo format (summary + a findings table + verdict) — do not hand-format it in body. - event: REQUEST_CHANGES (default), APPROVE, or COMMENT. Requires a - journal:learning entry first. + event: REQUEST_CHANGES (default), APPROVE, or COMMENT. The verdict must + match the findings: to APPROVE a clean PR pass event='APPROVE' (do not rely + on the default); REQUEST_CHANGES must cite at least one finding, and APPROVE + may not carry a blocker/major finding. Requires a journal:learning entry + first. """ return _post( _role_path("post_pr_review"), diff --git a/roboco/services/gateway/choreographer/pr_review.py b/roboco/services/gateway/choreographer/pr_review.py index e2a828dc..af9bca81 100644 --- a/roboco/services/gateway/choreographer/pr_review.py +++ b/roboco/services/gateway/choreographer/pr_review.py @@ -21,7 +21,11 @@ import structlog from roboco.foundation.policy import lifecycle as spec_module from roboco.foundation.policy import tracing as _tr -from roboco.foundation.policy.content import ContentValidationError, validate_content +from roboco.foundation.policy.content import ( + ContentValidationError, + pr_review_conflict, + validate_content, +) from roboco.services.content_notes import apply_structured_note from roboco.services.gateway.envelope import Envelope @@ -215,6 +219,21 @@ class PRReviewerMixin(_Base): if isinstance(pre, Envelope): return pre agent, role_str, briefing, spec_ctx = pre + # Refuse a verdict that contradicts the findings BEFORE anything is + # recorded or posted to the contributor's PR (e.g. a forgotten + # event='APPROVE' that defaults to a blocking REQUEST_CHANGES with no + # findings cited). + conflict = await self._verdict_consistency_gate( + t, + reviewer_agent_id, + task_id, + role_str, + briefing, + event=event, + findings=findings, + ) + if conflict is not None: + return conflict slug = await self._project_slug_for(t) pr_number = t.pr_number post_body = self._resolve_post_body(t, body, findings, event) @@ -299,6 +318,39 @@ class PRReviewerMixin(_Base): return gate return (agent, role_str, briefing, spec_ctx) + async def _verdict_consistency_gate( + self, + t: Any, + reviewer_agent_id: UUID, + task_id: UUID, + role_str: str, + briefing: dict[str, Any], + *, + event: str, + findings: list[dict[str, Any]] | None, + ) -> Envelope | None: + """Reject a self-contradicting (event, findings) pair, else None. + + The recorded ``pr_review`` verdict and the posted GitHub review event + both derive from ``event``; ``pr_review_conflict`` is the pure invariant + that keeps them honest. Runs before any side effect so a contradictory + review never reaches the task record or the PR. + """ + conflict = pr_review_conflict(event, findings) + if conflict is None: + return None + message, remediate = conflict + return await self._emit_rejection( + Envelope.invalid_state( + message=message, + remediate=remediate, + context_briefing=briefing, + ).with_introspection(task=t, role=role_str), + agent_id=reviewer_agent_id, + task_id=task_id, + verb="post_pr_review", + ) + async def _resolve_role( self, t: Any, diff --git a/tests/unit/foundation/policy/content/test_pr_review_conflict.py b/tests/unit/foundation/policy/content/test_pr_review_conflict.py new file mode 100644 index 00000000..3ff07f26 --- /dev/null +++ b/tests/unit/foundation/policy/content/test_pr_review_conflict.py @@ -0,0 +1,67 @@ +"""The PR-review verdict<->findings invariant. + +A reviewer's recorded verdict and the GitHub review event both derive from the +``event`` argument, so they must agree with the findings the reviewer cites. +``pr_review_conflict`` is the pure predicate the gateway enforces before any +review is recorded or posted — so an approving review can never be filed (or +posted to a contributor's PR) as a blocking "changes requested", and an +approval can never sail over a blocker. +""" + +from __future__ import annotations + +from roboco.foundation.policy.content import pr_review_conflict + +_BLOCKER = {"file": "a.py", "severity": "blocker", "expected": "x", "actual": "y"} +_MAJOR = {"file": "a.py", "severity": "major", "expected": "x", "actual": "y"} +_MINOR = {"file": "a.py", "severity": "minor", "expected": "x", "actual": "y"} +_NIT = {"file": "a.py", "severity": "nit", "expected": "x", "actual": "y"} + + +def test_request_changes_with_no_findings_conflicts() -> None: + # The reported bug: REQUEST_CHANGES (the default) with zero findings — a + # blocking verdict with nothing cited, contradicting an approving summary. + conflict = pr_review_conflict("REQUEST_CHANGES", []) + assert conflict is not None + message, remediate = conflict + assert "finding" in message.lower() + assert "APPROVE" in remediate + + +def test_request_changes_with_a_finding_is_ok() -> None: + assert pr_review_conflict("REQUEST_CHANGES", [_MINOR]) is None + + +def test_approve_over_a_blocker_conflicts() -> None: + conflict = pr_review_conflict("APPROVE", [_BLOCKER]) + assert conflict is not None + message, _ = conflict + assert "approve" in message.lower() + + +def test_approve_over_a_major_conflicts() -> None: + assert pr_review_conflict("APPROVE", [_MAJOR]) is not None + + +def test_approve_with_only_nits_is_ok() -> None: + assert pr_review_conflict("APPROVE", [_NIT, _MINOR]) is None + + +def test_approve_with_no_findings_is_ok() -> None: + assert pr_review_conflict("APPROVE", []) is None + + +def test_comment_is_never_in_conflict() -> None: + # A neutral COMMENT carries no verdict, so it is never a contradiction. + assert pr_review_conflict("COMMENT", []) is None + assert pr_review_conflict("COMMENT", [_BLOCKER]) is None + + +def test_findings_none_is_treated_as_empty() -> None: + assert pr_review_conflict("REQUEST_CHANGES", None) is not None + assert pr_review_conflict("APPROVE", None) is None + + +def test_severity_match_is_case_insensitive() -> None: + upper = {"file": "a.py", "severity": "BLOCKER", "expected": "x", "actual": "y"} + assert pr_review_conflict("APPROVE", [upper]) is not None diff --git a/tests/unit/gateway/test_pr_review_verdict_consistency.py b/tests/unit/gateway/test_pr_review_verdict_consistency.py new file mode 100644 index 00000000..ed72c62a --- /dev/null +++ b/tests/unit/gateway/test_pr_review_verdict_consistency.py @@ -0,0 +1,104 @@ +"""post_pr_review refuses a self-contradicting verdict before it is recorded/posted. + +The recorded ``notes_structured.pr_review.verdict`` and the GitHub review event +both derive from the verb's ``event`` argument. A reviewer who concludes +"approve" in the body but leaves ``event`` at its ``REQUEST_CHANGES`` default +(with no findings) would otherwise file — and post to the contributor's PR — a +blocking "changes requested" that contradicts the approving summary. The gate +catches that at the choreographer before any side effect runs. +""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import AsyncMock, MagicMock +from uuid import uuid4 + +import pytest +from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps + + +def _make_choreographer() -> Choreographer: + base: dict[str, Any] = { + "task": AsyncMock(), + "work_session": AsyncMock(), + "git": AsyncMock(), + "a2a": AsyncMock(), + "journal": AsyncMock(), + "audit": AsyncMock(), + "evidence_repo": AsyncMock(), + } + return Choreographer(ChoreographerDeps(**base)) + + +_CLEAN_FINDING = { + "file": "a.py", + "severity": "minor", + "expected": "x", + "actual": "y", +} +_BLOCKER = {"file": "a.py", "severity": "blocker", "expected": "x", "actual": "y"} + + +@pytest.mark.asyncio +async def test_request_changes_without_findings_is_rejected() -> None: + c = _make_choreographer() + env = await c._verdict_consistency_gate( + MagicMock(), + uuid4(), + uuid4(), + "pr_reviewer", + {}, + event="REQUEST_CHANGES", + findings=[], + ) + assert env is not None + body = env.as_dict() + assert body["error"] == "invalid_state" + assert "finding" in body["message"].lower() + + +@pytest.mark.asyncio +async def test_approve_over_a_blocker_is_rejected() -> None: + c = _make_choreographer() + env = await c._verdict_consistency_gate( + MagicMock(), + uuid4(), + uuid4(), + "pr_reviewer", + {}, + event="APPROVE", + findings=[_BLOCKER], + ) + assert env is not None + assert env.as_dict()["error"] == "invalid_state" + + +@pytest.mark.asyncio +async def test_request_changes_with_findings_passes_gate() -> None: + c = _make_choreographer() + env = await c._verdict_consistency_gate( + MagicMock(), + uuid4(), + uuid4(), + "pr_reviewer", + {}, + event="REQUEST_CHANGES", + findings=[_CLEAN_FINDING], + ) + assert env is None + + +@pytest.mark.asyncio +async def test_clean_approve_passes_gate() -> None: + c = _make_choreographer() + env = await c._verdict_consistency_gate( + MagicMock(), + uuid4(), + uuid4(), + "pr_reviewer", + {}, + event="APPROVE", + findings=[], + ) + assert env is None