diff --git a/roboco/runtime/orchestrator.py b/roboco/runtime/orchestrator.py index 28ee602b..f63f47eb 100644 --- a/roboco/runtime/orchestrator.py +++ b/roboco/runtime/orchestrator.py @@ -2559,7 +2559,12 @@ class AgentOrchestrator: @staticmethod def _readiness_check_role_for_status( - agent_id: str, role: str, status: str, *, is_coordination: bool = False + agent_id: str, + role: str, + status: str, + *, + is_coordination: bool = False, + owner_is_pm: bool = False, ) -> str | None: """Verify agent role matches the role expected for the task status. @@ -2568,10 +2573,14 @@ class AgentOrchestrator: developer/documenter to defang the bug where QA got respawned on a `needs_revision` task via the crash-restart path and immediately hit ``role 'qa' may not claim from status - 'needs_revision'`` at the gateway. A coordination root (no code; product - fan-out owned by a PM) is the exception: it has no dev, so a CEO-rejected - one returns to its PM — the dev-owned states also accept the PM roles for - it (a pure widening; nothing currently allowed is blocked). + 'needs_revision'`` at the gateway. The exception is a PM-OWNED revision: + a coordination root (no code; product fan-out owned by a PM, a CEO-reject + returning to its PM) AND a gate-failed assembled PR (the PR-review gate's + ``pr_fail`` sends a cell->root / root->master PR back to needs_revision, + still owned by the cell/main PM). In both the owner is a PM, so the + dev-owned states also accept the PM roles when ``owner_is_pm`` — matching + ``_dispatch_revision_coordination_roots``, which re-spawns exactly those. + A pure widening; nothing currently allowed is blocked, and QA stays out. """ role_mismatch: dict[str, str | set[str]] = { "awaiting_qa": "qa", @@ -2587,7 +2596,9 @@ class AgentOrchestrator: required = role_mismatch.get(status) if required is None: return None - if is_coordination and status in ("needs_revision", "verifying"): + if status in ("needs_revision", "verifying") and ( + is_coordination or owner_is_pm + ): required = set(required) | {"cell_pm", "main_pm"} ok = role in required if isinstance(required, set) else role == required if ok: @@ -2614,8 +2625,14 @@ class AgentOrchestrator: # the readiness and stuck-detection paths agree. if _branch_is_expected(task) and not task.get("branch_name"): return f"state={status} but branch_name is unset" + owner = task.get("assigned_to") or task.get("claimed_by") + owner_role = get_agent_role(self._resolve_agent_slug(owner)) if owner else None return self._readiness_check_role_for_status( - agent_id, role, status, is_coordination=_is_coordination_task(task) + agent_id, + role, + status, + is_coordination=_is_coordination_task(task), + owner_is_pm=owner_role in ("cell_pm", "main_pm"), ) @staticmethod diff --git a/tests/unit/runtime/test_readiness_role_status_match.py b/tests/unit/runtime/test_readiness_role_status_match.py index 33e07ec8..4b46617c 100644 --- a/tests/unit/runtime/test_readiness_role_status_match.py +++ b/tests/unit/runtime/test_readiness_role_status_match.py @@ -125,8 +125,48 @@ def test_qa_on_needs_revision_coordination_still_blocked() -> None: def test_pm_on_needs_revision_noncoordination_still_blocked() -> None: - """A normal (code) needs_revision task is still dev/doc-only for a PM.""" + """A normal (code) needs_revision task is still dev/doc-only for a PM when it + is NOT PM-owned (owner_is_pm unset) — the raw default behaviour.""" reason = AgentOrchestrator._readiness_check_role_for_status( agent_id="be-pm", role="cell_pm", status="needs_revision", is_coordination=False ) assert reason is not None + + +# --------------------------------------------------------------------------- +# Gate-failed assembled PRs — the PR-review gate's pr_fail sends a cell->root / +# root->master PR back to needs_revision, still owned by the PM. It is NOT a +# coordination task (it has a project + branch), so the readiness waiver relies +# on owner_is_pm (mirroring _dispatch_revision_coordination_roots). Regression +# for the production deadlock: "spawn refused for be-pm ... state=needs_revision +# requires role in {'documenter','developer'} but agent be-pm is 'cell_pm'". +# --------------------------------------------------------------------------- + + +def test_pm_on_needs_revision_owner_is_pm_allowed() -> None: + """A gate-failed assembled PR is PM-owned but NOT coordination; the owning + cell/main PM must be spawnable to revise it.""" + for role in ("main_pm", "cell_pm"): + assert ( + AgentOrchestrator._readiness_check_role_for_status( + agent_id="be-pm", role=role, status="needs_revision", owner_is_pm=True + ) + is None + ) + + +def test_pm_on_verifying_owner_is_pm_allowed() -> None: + assert ( + AgentOrchestrator._readiness_check_role_for_status( + agent_id="be-pm", role="cell_pm", status="verifying", owner_is_pm=True + ) + is None + ) + + +def test_qa_on_needs_revision_owner_is_pm_still_blocked() -> None: + """owner_is_pm widens to the PM roles only — QA is still a misroute.""" + reason = AgentOrchestrator._readiness_check_role_for_status( + agent_id="be-qa", role="qa", status="needs_revision", owner_is_pm=True + ) + assert reason is not None