mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(orchestrator): give pr_reviewer a spawn manifest so it can claim work
pr_reviewer was absent from GATEWAY_ENABLED_ROLES, so the spawn mounted no tool-manifest and set ROBOCO_GATEWAY_ENABLED=false. The reviewer booted with no flow verbs, could never claim its external-PR task, exited, and was respawned on the same task every tick — an endless loop that burned tokens. Add pr_reviewer to the set, plus a regression invariant asserting every spawnable seeded role has a manifest (only the never-spawned roles — prompter/secretary/ceo/system — may be absent) and a direct pr-reviewer-1 manifest test checking its claim_pr_review/post_pr_review verbs are present.
This commit is contained in:
@@ -323,7 +323,12 @@ def _resolve_project_slug_from_git_context(
|
|||||||
# SPAWN MANIFEST — per-developer tool manifest mounting (Phase 1)
|
# SPAWN MANIFEST — per-developer tool manifest mounting (Phase 1)
|
||||||
# =============================================================================
|
# =============================================================================
|
||||||
|
|
||||||
# Phase 4: every role gets a gateway manifest. The legacy briefing path is gone.
|
# Phase 4: every spawned role gets a gateway manifest. The legacy briefing path
|
||||||
|
# is gone. A role omitted here gets NO manifest and ROBOCO_GATEWAY_ENABLED=false,
|
||||||
|
# i.e. none of its flow verbs are pre-registered — so it can never claim its work
|
||||||
|
# and the dispatcher respawns it on the same task forever. The only roles that
|
||||||
|
# may be absent are the human-only ones (prompter, secretary) that the
|
||||||
|
# orchestrator never spawns as delivery agents.
|
||||||
GATEWAY_ENABLED_ROLES: frozenset[str] = frozenset(
|
GATEWAY_ENABLED_ROLES: frozenset[str] = frozenset(
|
||||||
{
|
{
|
||||||
"developer",
|
"developer",
|
||||||
@@ -334,6 +339,7 @@ GATEWAY_ENABLED_ROLES: frozenset[str] = frozenset(
|
|||||||
"product_owner",
|
"product_owner",
|
||||||
"head_marketing",
|
"head_marketing",
|
||||||
"auditor",
|
"auditor",
|
||||||
|
"pr_reviewer",
|
||||||
}
|
}
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|||||||
@@ -6,12 +6,19 @@ import json
|
|||||||
from typing import TYPE_CHECKING
|
from typing import TYPE_CHECKING
|
||||||
from unittest.mock import patch
|
from unittest.mock import patch
|
||||||
|
|
||||||
|
from roboco.agents_config import ALL_AGENTS, get_agent_role
|
||||||
from roboco.runtime.orchestrator import GATEWAY_ENABLED_ROLES, _build_manifest_for_agent
|
from roboco.runtime.orchestrator import GATEWAY_ENABLED_ROLES, _build_manifest_for_agent
|
||||||
from roboco.seeds.initial_data import AGENT_UUIDS
|
from roboco.seeds.initial_data import AGENT_UUIDS
|
||||||
|
|
||||||
if TYPE_CHECKING:
|
if TYPE_CHECKING:
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
|
||||||
|
# Roles the orchestrator never spawns as containerized delivery agents, so they
|
||||||
|
# legitimately get no spawn manifest: the human-only chat agents (prompter,
|
||||||
|
# secretary), the human CEO, and the orchestrator's own `system` sentinel. Every
|
||||||
|
# OTHER seeded role must be gateway-enabled or it boots with no flow verbs.
|
||||||
|
NON_SPAWNED_ROLES = {"prompter", "secretary", "ceo", "system"}
|
||||||
|
|
||||||
|
|
||||||
class TestGatewayEnabledRoles:
|
class TestGatewayEnabledRoles:
|
||||||
def test_all_roles_enabled(self) -> None:
|
def test_all_roles_enabled(self) -> None:
|
||||||
@@ -28,6 +35,34 @@ class TestGatewayEnabledRoles:
|
|||||||
assert "head_marketing" in GATEWAY_ENABLED_ROLES
|
assert "head_marketing" in GATEWAY_ENABLED_ROLES
|
||||||
assert "auditor" in GATEWAY_ENABLED_ROLES
|
assert "auditor" in GATEWAY_ENABLED_ROLES
|
||||||
|
|
||||||
|
def test_pr_reviewer_enabled(self) -> None:
|
||||||
|
"""The PR reviewer is a spawned delivery agent — it must be enabled.
|
||||||
|
|
||||||
|
Without this it gets ROBOCO_GATEWAY_ENABLED=false and no manifest, so
|
||||||
|
none of its flow verbs (claim_pr_review/post_pr_review) are registered;
|
||||||
|
it can never claim its task and the dispatcher respawns it forever.
|
||||||
|
"""
|
||||||
|
assert "pr_reviewer" in GATEWAY_ENABLED_ROLES
|
||||||
|
|
||||||
|
def test_every_spawnable_agent_role_is_gateway_enabled(self) -> None:
|
||||||
|
"""Invariant: every seeded agent the orchestrator spawns has a manifest.
|
||||||
|
|
||||||
|
Guards against the regression where a new spawnable role is added to the
|
||||||
|
roster but forgotten here — that role would spawn with no flow verbs and
|
||||||
|
loop. Only the never-spawned roles (prompter/secretary/ceo/system) may be
|
||||||
|
absent.
|
||||||
|
"""
|
||||||
|
missing = {
|
||||||
|
agent_id: role
|
||||||
|
for agent_id in ALL_AGENTS
|
||||||
|
if (role := get_agent_role(agent_id)) not in NON_SPAWNED_ROLES
|
||||||
|
and role not in GATEWAY_ENABLED_ROLES
|
||||||
|
}
|
||||||
|
assert not missing, (
|
||||||
|
f"spawnable agents missing from GATEWAY_ENABLED_ROLES "
|
||||||
|
f"(would loop with no flow verbs): {missing}"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
class TestBuildManifestForAgent:
|
class TestBuildManifestForAgent:
|
||||||
def test_developer_writes_file(self, tmp_path: Path) -> None:
|
def test_developer_writes_file(self, tmp_path: Path) -> None:
|
||||||
@@ -152,6 +187,26 @@ class TestBuildManifestForAgent:
|
|||||||
data = json.loads(result.read_text())
|
data = json.loads(result.read_text())
|
||||||
assert data["role"] == "main_pm"
|
assert data["role"] == "main_pm"
|
||||||
|
|
||||||
|
def test_pr_reviewer_writes_file_with_review_verbs(self, tmp_path: Path) -> None:
|
||||||
|
"""pr-reviewer-1 produces a manifest carrying its review flow verbs.
|
||||||
|
|
||||||
|
Regression guard for the respawn loop: if this returns None (role not
|
||||||
|
gateway-enabled) the reviewer spawns with no task tools.
|
||||||
|
"""
|
||||||
|
with patch("roboco.runtime.orchestrator.settings") as mock_settings:
|
||||||
|
mock_settings.manifest_host_dir = str(tmp_path)
|
||||||
|
mock_settings.workspaces_root = str(tmp_path / "workspaces")
|
||||||
|
|
||||||
|
result = _build_manifest_for_agent("pr-reviewer-1", "claude-sonnet-4-6")
|
||||||
|
|
||||||
|
assert result is not None, "pr-reviewer-1 must produce a manifest"
|
||||||
|
assert result.exists()
|
||||||
|
assert result.name == "pr-reviewer-1.json"
|
||||||
|
data = json.loads(result.read_text())
|
||||||
|
assert data["role"] == "pr_reviewer"
|
||||||
|
assert "claim_pr_review" in data["flow_tools"]
|
||||||
|
assert "post_pr_review" in data["flow_tools"]
|
||||||
|
|
||||||
def test_manifest_dir_created_if_absent(self, tmp_path: Path) -> None:
|
def test_manifest_dir_created_if_absent(self, tmp_path: Path) -> None:
|
||||||
"""manifest_host_dir is created automatically when it doesn't exist."""
|
"""manifest_host_dir is created automatically when it doesn't exist."""
|
||||||
nested = tmp_path / "new" / "nested" / "dir"
|
nested = tmp_path / "new" / "nested" / "dir"
|
||||||
|
|||||||
Reference in New Issue
Block a user