mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(orchestrator): spawn the owning PM for 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. The dispatch side
(_dispatch_revision_coordination_roots) re-spawns the owning PM for any
PM-owned needs_revision task, but the readiness gate
(_readiness_check_role_for_status) only waived the dev/doc role restriction for
coordination tasks — and a gate-failed assembled PR has a project + branch, so
it is NOT coordination. Result: dispatch routed the cell PM, readiness refused
it ("state=needs_revision requires role in {developer,documenter} but agent
be-pm is cell_pm"), and the task deadlocked. Pass owner_is_pm (derived from the
task's assignee, like the dispatch side) so the readiness waiver also fires for
a PM-owned revision, not just coordination roots. QA stays excluded.
This commit is contained in:
@@ -2559,7 +2559,12 @@ class AgentOrchestrator:
|
|||||||
|
|
||||||
@staticmethod
|
@staticmethod
|
||||||
def _readiness_check_role_for_status(
|
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:
|
) -> str | None:
|
||||||
"""Verify agent role matches the role expected for the task status.
|
"""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
|
developer/documenter to defang the bug where QA got
|
||||||
respawned on a `needs_revision` task via the crash-restart path
|
respawned on a `needs_revision` task via the crash-restart path
|
||||||
and immediately hit ``role 'qa' may not claim from status
|
and immediately hit ``role 'qa' may not claim from status
|
||||||
'needs_revision'`` at the gateway. A coordination root (no code; product
|
'needs_revision'`` at the gateway. The exception is a PM-OWNED revision:
|
||||||
fan-out owned by a PM) is the exception: it has no dev, so a CEO-rejected
|
a coordination root (no code; product fan-out owned by a PM, a CEO-reject
|
||||||
one returns to its PM — the dev-owned states also accept the PM roles for
|
returning to its PM) AND a gate-failed assembled PR (the PR-review gate's
|
||||||
it (a pure widening; nothing currently allowed is blocked).
|
``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]] = {
|
role_mismatch: dict[str, str | set[str]] = {
|
||||||
"awaiting_qa": "qa",
|
"awaiting_qa": "qa",
|
||||||
@@ -2587,7 +2596,9 @@ class AgentOrchestrator:
|
|||||||
required = role_mismatch.get(status)
|
required = role_mismatch.get(status)
|
||||||
if required is None:
|
if required is None:
|
||||||
return 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"}
|
required = set(required) | {"cell_pm", "main_pm"}
|
||||||
ok = role in required if isinstance(required, set) else role == required
|
ok = role in required if isinstance(required, set) else role == required
|
||||||
if ok:
|
if ok:
|
||||||
@@ -2614,8 +2625,14 @@ class AgentOrchestrator:
|
|||||||
# the readiness and stuck-detection paths agree.
|
# the readiness and stuck-detection paths agree.
|
||||||
if _branch_is_expected(task) and not task.get("branch_name"):
|
if _branch_is_expected(task) and not task.get("branch_name"):
|
||||||
return f"state={status} but branch_name is unset"
|
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(
|
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
|
@staticmethod
|
||||||
|
|||||||
@@ -125,8 +125,48 @@ def test_qa_on_needs_revision_coordination_still_blocked() -> None:
|
|||||||
|
|
||||||
|
|
||||||
def test_pm_on_needs_revision_noncoordination_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(
|
reason = AgentOrchestrator._readiness_check_role_for_status(
|
||||||
agent_id="be-pm", role="cell_pm", status="needs_revision", is_coordination=False
|
agent_id="be-pm", role="cell_pm", status="needs_revision", is_coordination=False
|
||||||
)
|
)
|
||||||
assert reason is not None
|
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
|
||||||
|
|||||||
Reference in New Issue
Block a user