Fix: Make main_pm + task_type=code impossible

tests/unit/gateway/test_pr_gate_notifies_pm.py

@@ -0,0 +1,232 @@
+"""pr_fail delivers its change-requests to the owning PM, not just to GitHub.
+
+The in-path ``pr_fail`` gate persists the reviewer's verdict to
+``notes_structured.pr_review`` and posts it on the assembled PR — but historically
+never pushed the concrete issues to any channel the owning PM reads (no a2a,
+and ``_briefing_for`` / ``build_task_handoff`` read neither ``pr_reviewer_notes``
+nor ``notes_structured.pr_review``). So the cell PM respawned into
+``needs_revision`` saw a generic "needs revision" with zero actionable issues,
+concluded there was nothing to rework, and re-submitted the same PR — an
+infinite ``pr_fail`` loop (observed live on coordination root 9980d0a0 / PR #138).
+
+The fix mirrors QA's ``fail_review`` a2a to the dev (qa.py:671-678): on
+``pr_fail`` the gate now sends an a2a to the owner the runner just re-assigned
+(the cell PM, via ``_revision_pm_for_task``) carrying the issues, so the PM
+"knows" and can ``delegate`` a rework subtask or action the change-requests
+directly. ``pr_pass`` is unaffected.
+"""
+
+from __future__ import annotations
+
+from typing import Any
+from unittest.mock import AsyncMock, MagicMock
+from uuid import uuid4
+
+import pytest
+from roboco.foundation.policy import lifecycle as spec_module
+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))
+
+
+def _stub_gate_path(
+    c: Choreographer,
+    *,
+    reviewer_id: Any,
+    t_before: Any,
+    t_after: Any,
+) -> None:
+    """Drive ``_gate_decision`` past preflight/tracing/post and into the new
+    a2a step without exercising the heavy ownership/tracing logic (those have
+    their own tests). The runner rebinding to ``t_after`` is what simulates the
+    PM re-assignment the real ``_revision_pm_for_task`` performs.
+    """
+    agent = MagicMock(role="pr_reviewer", slug="be-pr-reviewer")
+    c._gate_preflight = AsyncMock(  # type: ignore[method-assign]
+        return_value=(
+            t_before,
+            agent,
+            "pr_reviewer",
+            {},
+            spec_module.Context(actor_id=reviewer_id),
+        )
+    )
+    c._gate_tracing = AsyncMock(return_value=None)  # type: ignore[method-assign]
+    c._record_gate_verdict = MagicMock()  # type: ignore[method-assign]
+    c._post_gate_review_to_pr = AsyncMock()  # type: ignore[method-assign]
+    runner = MagicMock()
+    runner.run_intent = AsyncMock(return_value=t_after)
+    c._verb_runner = MagicMock(return_value=runner)  # type: ignore[method-assign]
+
+
+@pytest.mark.asyncio
+async def test_pr_fail_notifies_reassigned_owning_pm() -> None:
+    reviewer_id = uuid4()
+    pm_id = uuid4()
+    task_id = uuid4()
+    parent_id = uuid4()
+    t_before = MagicMock(
+        id=task_id,
+        assigned_to=reviewer_id,  # owned by the reviewer until the runner runs
+        pr_number=138,
+        parent_task_id=parent_id,
+        status="awaiting_pr_review",
+    )
+    # The runner reassigns the task to the owning PM (needs_revision owner).
+    t_after = MagicMock(
+        id=task_id,
+        assigned_to=pm_id,
+        pr_number=138,
+        parent_task_id=parent_id,
+        status="needs_revision",
+    )
+
+    c = _make_choreographer()
+    _stub_gate_path(c, reviewer_id=reviewer_id, t_before=t_before, t_after=t_after)
+
+    await c.pr_fail(reviewer_id, task_id, ["seam mismatch", "docs lag the diff"])
+
+    c.a2a.send.assert_awaited_once()
+    kwargs = c.a2a.send.await_args.kwargs
+    assert kwargs["from_agent"] == reviewer_id
+    assert kwargs["to_agent"] == pm_id
+    assert kwargs["skill"] == "code_review"
+    assert kwargs["task_id"] == task_id
+    body = kwargs["body"]
+    assert "PR review needs changes." in body
+    assert "seam mismatch" in body
+    assert "docs lag the diff" in body
+
+
+@pytest.mark.asyncio
+async def test_pr_pass_does_not_notify_anyone() -> None:
+    reviewer_id = uuid4()
+    pm_id = uuid4()
+    task_id = uuid4()
+    t_before = MagicMock(
+        id=task_id,
+        assigned_to=reviewer_id,
+        pr_number=42,
+        parent_task_id=uuid4(),
+        status="awaiting_pr_review",
+    )
+    t_after = MagicMock(
+        id=task_id, assigned_to=pm_id, pr_number=42, status="awaiting_pm_review"
+    )
+
+    c = _make_choreographer()
+    _stub_gate_path(c, reviewer_id=reviewer_id, t_before=t_before, t_after=t_after)
+
+    await c.pr_pass(reviewer_id, task_id, "Assembled root scope is clean and covered.")
+
+    c.a2a.send.assert_not_awaited()
+
+
+@pytest.mark.asyncio
+async def test_pr_fail_skips_a2a_when_no_assignee() -> None:
+    """If the runner left the task unassigned, there's nobody to notify — must
+    not raise and must not crash on ``a2a.send(None)``."""
+    reviewer_id = uuid4()
+    task_id = uuid4()
+    t_before = MagicMock(
+        id=task_id,
+        assigned_to=reviewer_id,
+        pr_number=9,
+        parent_task_id=uuid4(),
+        status="awaiting_pr_review",
+    )
+    t_after = MagicMock(
+        id=task_id, assigned_to=None, pr_number=9, status="needs_revision"
+    )
+
+    c = _make_choreographer()
+    _stub_gate_path(c, reviewer_id=reviewer_id, t_before=t_before, t_after=t_after)
+
+    env = await c.pr_fail(reviewer_id, task_id, ["one concrete issue here"])
+    c.a2a.send.assert_not_awaited()
+    assert env.status == "needs_revision"
+
+
+@pytest.mark.asyncio
+async def test_pr_fail_a2a_for_main_pm_root_steers_to_redelegate() -> None:
+    """A Main-PM branch-bearing root is an assembled cell→root / root→master PR —
+    coordination, not the Main PM's own code. The ``pr_fail`` a2a body must steer
+    the Main PM to re-delegate the fixes and NOT re-submit the unchanged root
+    (the 2026-06-27 infinite ``pr_fail`` loop), while still carrying the concrete
+    issues. ``_revision_pm_for_task`` returns main-pm for a non-cell team, so the
+    recipient stays the Main PM — correct, since the Main PM re-delegates."""
+    reviewer_id = uuid4()
+    main_pm_id = uuid4()
+    task_id = uuid4()
+    t_before = MagicMock(
+        id=task_id,
+        assigned_to=reviewer_id,
+        pr_number=139,
+        parent_task_id=uuid4(),
+        status="awaiting_pr_review",
+    )
+    t_after = MagicMock(
+        id=task_id,
+        assigned_to=main_pm_id,
+        pr_number=139,
+        parent_task_id=uuid4(),
+        status="needs_revision",
+    )
+    # Team is read off the runner-rebound task. ``getattr(team, "value", team)``
+    # must yield ``"main_pm"``; a MagicMock team would not, so set the attribute
+    # explicitly. branch_name set => an assembled root, not a branchless umbrella.
+    t_after.team = spec_module.Team.MAIN_PM
+    t_after.branch_name = "feature/main_pm/c80e19ff"
+
+    c = _make_choreographer()
+    _stub_gate_path(c, reviewer_id=reviewer_id, t_before=t_before, t_after=t_after)
+
+    await c.pr_fail(reviewer_id, task_id, ["duplicate TimeseriesChart export"])
+
+    c.a2a.send.assert_awaited_once()
+    kwargs = c.a2a.send.await_args.kwargs
+    assert kwargs["to_agent"] == main_pm_id
+    body = kwargs["body"]
+    assert "PR review needs changes." in body
+    assert "duplicate TimeseriesChart export" in body
+    assert "re-delegate" in body
+    assert "do NOT re-submit" in body
+
+
+@pytest.mark.asyncio
+async def test_pr_fail_a2a_failure_is_swallowed() -> None:
+    """The gate transition already committed; an a2a delivery failure must not
+    roll back the verdict or 500 the reviewer (same posture as the PR-post step,
+    and the inverse of the cell_pm_complete None-deref crash)."""
+    reviewer_id = uuid4()
+    pm_id = uuid4()
+    task_id = uuid4()
+    t_before = MagicMock(
+        id=task_id,
+        assigned_to=reviewer_id,
+        pr_number=7,
+        parent_task_id=uuid4(),
+        status="awaiting_pr_review",
+    )
+    t_after = MagicMock(
+        id=task_id, assigned_to=pm_id, pr_number=7, status="needs_revision"
+    )
+
+    c = _make_choreographer()
+    _stub_gate_path(c, reviewer_id=reviewer_id, t_before=t_before, t_after=t_after)
+    c.a2a.send = AsyncMock(side_effect=RuntimeError("db hiccup"))  # type: ignore[method-assign]
+
+    env = await c.pr_fail(reviewer_id, task_id, ["a concrete actionable issue"])
+    # Verdict still landed — the owning PM is in needs_revision.
+    assert env.status == "needs_revision"

tests/unit/gateway/test_pr_gate_records_verdict.py

@@ -89,3 +89,68 @@ def test_record_gate_verdict_swallows_invalid_note() -> None:
     # No exception, and the stale slot is left as-is rather than corrupted.
     assert t.notes_structured is not None
     assert t.notes_structured["pr_review"]["verdict"] == "passed"
+
+
+def test_pr_fail_stores_issues_structurally_not_summary_only() -> None:
+    """pr_fail's free-text issues must persist into the structured ``issues``
+    slot (so a reader of notes_structured.pr_review gets the concrete
+    change-requests), not be flattened into the summary string alone."""
+    c = _make_choreographer()
+    t = _TaskWithNoNotes()
+    c._record_gate_verdict(
+        t,
+        "pr_fail",
+        "Issues:\n- seam mismatch\n- docs lag the diff",
+        issues=("seam mismatch", "docs lag the diff"),
+    )
+    slot = t.notes_structured["pr_review"]
+    assert slot["verdict"] == "failed"
+    assert slot["issues"] == ["seam mismatch", "docs lag the diff"]
+    # The derived TEXT mirror surfaces them too, so pr_reviewer_notes carries
+    # the concrete change-requests for any future reader.
+    assert "seam mismatch" in t.pr_reviewer_notes
+    assert "docs lag the diff" in t.pr_reviewer_notes
+
+
+def test_pr_fail_summary_does_not_duplicate_issues() -> None:
+    """pr_fail's issues must render only under ## Issues, not also baked into
+    ## Summary — otherwise the Task Details "PR Reviewer Notes" card shows each
+    issue twice (once under Summary, once under Issues). The summary is a
+    substantive non-issues sentence; the structured ``issues`` slot carries the
+    change-requests. ``notes`` (with the issues) still drives the GitHub PR post
+    and the a2a to the owning PM — those are raw text, not rendered through
+    ``render_markdown``, so no duplication there."""
+    c = _make_choreographer()
+    t = _TaskWithNoNotes()
+    c._record_gate_verdict(
+        t,
+        "pr_fail",
+        "Issues:\n- seam mismatch\n- docs lag the diff",
+        issues=("seam mismatch", "docs lag the diff"),
+    )
+    slot = t.notes_structured["pr_review"]
+    assert slot["verdict"] == "failed"
+    # Issues live in the structured issues slot...
+    assert slot["issues"] == ["seam mismatch", "docs lag the diff"]
+    # ...NOT baked into the summary.
+    assert "seam mismatch" not in slot["summary"]
+    assert "docs lag the diff" not in slot["summary"]
+    # The rendered TEXT mirror surfaces each issue (under ## Issues)...
+    assert "seam mismatch" in t.pr_reviewer_notes
+    assert "docs lag the diff" in t.pr_reviewer_notes
+    # ...with both section headers present, and the summary is the substantive
+    # sentence (not the issues-joined string).
+    assert "## Summary" in t.pr_reviewer_notes
+    assert "## Issues" in t.pr_reviewer_notes
+    assert "requested changes" in t.pr_reviewer_notes
+
+
+def test_pr_pass_leaves_issues_slot_empty() -> None:
+    c = _make_choreographer()
+    t = _TaskWithNoNotes()
+    c._record_gate_verdict(
+        t, "pr_pass", "Assembled root scope is clean; every criterion is covered."
+    )
+    slot = t.notes_structured["pr_review"]
+    assert slot["verdict"] == "passed"
+    assert slot.get("issues", []) == []

tests/unit/gateway/test_pr_review_hand_format_guard.py

@@ -0,0 +1,183 @@
+"""post_pr_review refuses a hand-formatted verdict body with no findings.
+
+The tool's contract (flow_server.post_pr_review docstring) is explicit: ``body``
+is a one-paragraph summary; when ``findings`` are given the GitHub comment is
+GENERATED in the RoboCo format (summary + findings table + verdict) — "do not
+hand-format it in body". Nothing enforced that, so a reviewer could pass
+``findings=[]`` and dump a self-formatted ``## Summary`` / ``## Issues`` /
+``## Verdict`` markdown blob into ``body`` — which the system posts verbatim
+(``_resolve_post_body`` returns ``body`` as-is when there are no findings). The
+deployed renderer emits ``## Findings`` (never ``## Issues``), so a ``## Issues``
+section on the PR is proof the agent hand-formatted. Observed live: the reviewer
+posted a body that listed the issues under BOTH ``## Summary`` and ``## Issues``
+and then repeated the entire block twice — a duplicated, self-redundant
+hand-formatted blob the contributor sees.
+
+The guard: when ``findings`` is empty AND ``body`` carries verdict/section
+markdown headers, reject with ``invalid_state`` and a remediation that points the
+reviewer at the structured-findings path. The system never posts a hand-formatted
+verdict, so the duplication cannot recur. A clean plain-note ``COMMENT`` with no
+findings is still allowed (only verdict-shaped bodies are blocked), and a review
+with structured findings is unaffected (the system generates the comment).
+"""
+
+from __future__ import annotations
+
+from typing import Any
+from unittest.mock import AsyncMock, MagicMock
+from uuid import uuid4
+
+import pytest
+from roboco.foundation.policy import lifecycle as spec_module
+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))
+
+
+def _stub_post_path(c: Choreographer, *, reviewer_id: Any, t: Any) -> None:
+    """Drive ``post_pr_review`` past preflight + the verdict-consistency gate so
+    the hand-format guard is the thing under test. The runner / side-effects are
+    stubbed so a passing case does not hit GitHub or the DB transition."""
+    agent = MagicMock(role="pr_reviewer", slug="be-pr-reviewer")
+    c._post_pr_review_preflight = AsyncMock(  # type: ignore[method-assign]
+        return_value=(
+            agent,
+            "pr_reviewer",
+            {},
+            spec_module.Context(actor_id=reviewer_id),
+        )
+    )
+    c._verdict_consistency_gate = AsyncMock(return_value=None)  # type: ignore[method-assign]
+    c._project_slug_for = AsyncMock(return_value="proj")  # type: ignore[method-assign]
+    c._resolve_post_body = MagicMock(return_value="generated body")  # type: ignore[method-assign]
+    runner = MagicMock()
+    runner.run_intent = AsyncMock(return_value=t)
+    c._verb_runner = MagicMock(return_value=runner)  # type: ignore[method-assign]
+    c._post_review_side_effects = AsyncMock()  # type: ignore[method-assign]
+
+
+def _task() -> Any:
+    return MagicMock(
+        id=uuid4(),
+        pr_number=200,
+        status="in_progress",
+        notes_structured=None,
+        pr_reviewer_notes="",
+    )
+
+
+@pytest.mark.asyncio
+async def test_hand_formatted_verdict_body_with_no_findings_is_rejected() -> None:
+    reviewer_id = uuid4()
+    task_id = uuid4()
+    c = _make_choreographer()
+    _stub_post_path(c, reviewer_id=reviewer_id, t=_task())
+
+    hand_formatted = (
+        "## Summary\nIssues:\n- [BLOCKER] seam mismatch\n"
+        "## Issues\n- [BLOCKER] seam mismatch\n## Verdict\nfailed"
+    )
+    env = await c.post_pr_review(reviewer_id, task_id, hand_formatted, "COMMENT")
+
+    body = env.as_dict()
+    assert body["error"] == "invalid_state"
+    assert "hand-formatted" in body["message"].lower()
+    assert "findings" in body["remediate"].lower()
+    # Nothing posted / transitioned — the guard fired before any side effect.
+    c.git.post_pr_review.assert_not_awaited()
+    c._verb_runner().run_intent.assert_not_awaited()  # type: ignore[union-attr]
+
+
+@pytest.mark.asyncio
+async def test_hand_formatted_body_rejected_even_for_request_changes() -> None:
+    # pr_review_conflict already blocks REQUEST_CHANGES + no findings, but the
+    # guard is independent of event — a hand-formatted verdict must never post,
+    # whatever event the agent picked.
+    reviewer_id = uuid4()
+    task_id = uuid4()
+    c = _make_choreographer()
+    _stub_post_path(c, reviewer_id=reviewer_id, t=_task())
+
+    env = await c.post_pr_review(
+        reviewer_id, task_id, "## Verdict\nfailed\nbad", "REQUEST_CHANGES"
+    )
+    assert env.as_dict()["error"] == "invalid_state"
+
+
+@pytest.mark.asyncio
+async def test_clean_plain_note_with_no_findings_is_allowed() -> None:
+    # A genuine plain COMMENT note (no verdict headers, no findings) is a legit
+    # use of event=COMMENT — the guard must not block it.
+    reviewer_id = uuid4()
+    task_id = uuid4()
+    t = _task()
+    c = _make_choreographer()
+    _stub_post_path(c, reviewer_id=reviewer_id, t=t)
+
+    env = await c.post_pr_review(
+        reviewer_id,
+        task_id,
+        "Left a note for the contributor: the CI flake on job X is tracked separately.",
+        "COMMENT",
+    )
+    body = env.as_dict()
+    assert body.get("error") is None
+    assert body["status"] == "in_progress"
+
+
+@pytest.mark.asyncio
+async def test_structured_findings_with_summary_body_is_allowed() -> None:
+    # With structured findings the system generates the comment; the guard
+    # (scoped to empty findings) does not fire even if the summary body happens
+    # to contain a header-shaped word.
+    reviewer_id = uuid4()
+    task_id = uuid4()
+    t = _task()
+    c = _make_choreographer()
+    _stub_post_path(c, reviewer_id=reviewer_id, t=t)
+
+    env = await c.post_pr_review(
+        reviewer_id,
+        task_id,
+        "The 422 path is unguarded.",
+        "REQUEST_CHANGES",
+        findings=[
+            {
+                "file": "roboco/services/git.py",
+                "line": 42,
+                "severity": "blocker",
+                "expected": "retry as COMMENT",
+                "actual": "raises",
+            }
+        ],
+    )
+    body = env.as_dict()
+    assert body.get("error") is None
+
+
+@pytest.mark.asyncio
+async def test_envelope_invalid_state_has_introspection_role() -> None:
+    # The rejection must carry role introspection like the other gates.
+    reviewer_id = uuid4()
+    task_id = uuid4()
+    c = _make_choreographer()
+    _stub_post_path(c, reviewer_id=reviewer_id, t=_task())
+
+    env = await c.post_pr_review(
+        reviewer_id, task_id, "## Summary\nstuff\n## Verdict\nfailed", "COMMENT"
+    )
+    assert env.error == "invalid_state"
+    # with_introspection(role="pr_reviewer") populated the introspection fields.
+    assert env.current_state is not None
+    assert env.valid_next_verbs is not None