mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
refactor(orchestrator): A2+A3 follow-ups — extract workspace-path helpers
Fixes 2 important + 1 minor issue from the code-quality review of 5adb4ff:
1. Formula duplication: the workspace path string was inlined at two
sites in orchestrator.py (the canonical _prepare_agent_spawn and the
new _build_mount_args -w logic). Extracted to module-level helpers
_agent_workspace_path(project, team, agent_id) and
_cell_workspace_path(project, team) so both callers share the same
formula. Future path changes only land in one place.
Also extracted _resolve_project_slug_from_git_context() as the
module-level counterpart to the instance method, called by the static
_build_mount_args site that cannot access self.
2. Test consistency: test_workdir_matches_edit_allowlist_path now
extracts the Edit(<prefix>/**) value from _get_role_permissions and
asserts the spawn cmd's -w value equals that prefix. The test would
actually catch a drift where _build_mount_args and _get_role_permissions
use different formulas — previously it just compared two copies of
the same string.
3. Test coverage: added test cases for product_owner and head_marketing
spawns (both share the per-agent workspace path), so all roles that
_get_role_permissions distinguishes are covered.
Spec ref: docs/superpowers/specs/2026-05-12-post-smoke-3-fixes-design.md
A2+A3 (re-scoped 2026-05-12).
This commit is contained in:
@@ -169,6 +169,44 @@ def _resolve_agent_cli_model(provider_type: str, model: str) -> str:
|
|||||||
return model
|
return model
|
||||||
|
|
||||||
|
|
||||||
|
def _agent_workspace_path(project_slug: str, team: str, agent_id: str) -> str:
|
||||||
|
"""Per-agent workspace path inside the container.
|
||||||
|
|
||||||
|
Mirrors the bind-mount layout: the host's workspaces dir is mounted at
|
||||||
|
/data/workspaces (orchestrator.py mount args), so each agent's clone lives
|
||||||
|
at /data/workspaces/<project>/<team>/<agent>. Used by both
|
||||||
|
_get_role_permissions (Edit/Write allowlist) and _build_mount_args
|
||||||
|
(docker ``-w`` flag) so the cwd matches the allowlist scope.
|
||||||
|
"""
|
||||||
|
return f"/data/workspaces/{project_slug}/{team}/{agent_id}"
|
||||||
|
|
||||||
|
|
||||||
|
def _cell_workspace_path(project_slug: str, team: str) -> str:
|
||||||
|
"""Cell-level workspace path (documenter scope).
|
||||||
|
|
||||||
|
Same rationale as ``_agent_workspace_path``; documenters work at the cell
|
||||||
|
branch, not a per-agent dev branch.
|
||||||
|
"""
|
||||||
|
return f"/data/workspaces/{project_slug}/{team}"
|
||||||
|
|
||||||
|
|
||||||
|
def _resolve_project_slug_from_git_context(
|
||||||
|
git_context: "SpawnGitContext | None",
|
||||||
|
) -> str:
|
||||||
|
"""Extract project_slug from git_context, falling back to 'default'.
|
||||||
|
|
||||||
|
Module-level counterpart to the instance method ``_resolve_project_slug``.
|
||||||
|
Called by static / classmethod contexts (e.g. ``_build_mount_args``) that
|
||||||
|
cannot access ``self``. The fallback warning is omitted here because the
|
||||||
|
instance method already logs it when the full spawn path runs; this helper
|
||||||
|
is only for the mount-args path where the agent_id/task_id context is not
|
||||||
|
available.
|
||||||
|
"""
|
||||||
|
if git_context and git_context.project_slug:
|
||||||
|
return git_context.project_slug
|
||||||
|
return "default"
|
||||||
|
|
||||||
|
|
||||||
# =============================================================================
|
# =============================================================================
|
||||||
# SPAWN MANIFEST — per-developer tool manifest mounting (Phase 1)
|
# SPAWN MANIFEST — per-developer tool manifest mounting (Phase 1)
|
||||||
# =============================================================================
|
# =============================================================================
|
||||||
@@ -1239,7 +1277,7 @@ class AgentOrchestrator:
|
|||||||
"""Build AgentConfig + AgentInstance and surface per-agent settings path."""
|
"""Build AgentConfig + AgentInstance and surface per-agent settings path."""
|
||||||
blueprint_path = self._generate_composed_prompt(agent_id)
|
blueprint_path = self._generate_composed_prompt(agent_id)
|
||||||
canonical_role = get_agent_role(agent_id)
|
canonical_role = get_agent_role(agent_id)
|
||||||
team = get_agent_team(agent_id)
|
team = get_agent_team(agent_id) or "backend"
|
||||||
|
|
||||||
# Resolve the provider route for this agent. Caller-supplied `model`
|
# Resolve the provider route for this agent. Caller-supplied `model`
|
||||||
# wins (dispatcher overrides, tests). Otherwise the routing service
|
# wins (dispatcher overrides, tests). Otherwise the routing service
|
||||||
@@ -1252,8 +1290,8 @@ class AgentOrchestrator:
|
|||||||
model = route.model_name
|
model = route.model_name
|
||||||
|
|
||||||
project_slug = self._resolve_project_slug(git_context, agent_id, task_id)
|
project_slug = self._resolve_project_slug(git_context, agent_id, task_id)
|
||||||
workspace_path = f"/data/workspaces/{project_slug}/{team}/{agent_id}"
|
workspace_path = _agent_workspace_path(project_slug, team, agent_id)
|
||||||
cell_workspace_path = f"/data/workspaces/{project_slug}/{team}"
|
cell_workspace_path = _cell_workspace_path(project_slug, team)
|
||||||
|
|
||||||
agent_settings_path = self._generate_agent_settings(
|
agent_settings_path = self._generate_agent_settings(
|
||||||
agent_id, canonical_role, workspace_path, cell_workspace_path
|
agent_id, canonical_role, workspace_path, cell_workspace_path
|
||||||
@@ -1574,19 +1612,13 @@ class AgentOrchestrator:
|
|||||||
# so the container falls back to /app (Dockerfile WORKDIR).
|
# so the container falls back to /app (Dockerfile WORKDIR).
|
||||||
_role = get_agent_role(config.agent_id) or "developer"
|
_role = get_agent_role(config.agent_id) or "developer"
|
||||||
_team = get_agent_team(config.agent_id) or ""
|
_team = get_agent_team(config.agent_id) or ""
|
||||||
_project = (
|
_project = _resolve_project_slug_from_git_context(config.git_context)
|
||||||
config.git_context.project_slug
|
|
||||||
if config.git_context and config.git_context.project_slug
|
|
||||||
else "default"
|
|
||||||
)
|
|
||||||
_workspace_path = f"/data/workspaces/{_project}/{_team}/{config.agent_id}"
|
|
||||||
_cell_workspace_path = f"/data/workspaces/{_project}/{_team}"
|
|
||||||
_roles_with_agent_workspace = {"developer", "product_owner", "head_marketing"}
|
_roles_with_agent_workspace = {"developer", "product_owner", "head_marketing"}
|
||||||
_roles_with_cell_workspace = {"documenter"}
|
_roles_with_cell_workspace = {"documenter"}
|
||||||
if _role in _roles_with_agent_workspace:
|
if _role in _roles_with_agent_workspace:
|
||||||
cmd.extend(["-w", _workspace_path])
|
cmd.extend(["-w", _agent_workspace_path(_project, _team, config.agent_id)])
|
||||||
elif _role in _roles_with_cell_workspace:
|
elif _role in _roles_with_cell_workspace:
|
||||||
cmd.extend(["-w", _cell_workspace_path])
|
cmd.extend(["-w", _cell_workspace_path(_project, _team)])
|
||||||
# else: qa / cell_pm / main_pm / auditor — omit -w, fall back to /app
|
# else: qa / cell_pm / main_pm / auditor — omit -w, fall back to /app
|
||||||
|
|
||||||
return cmd
|
return cmd
|
||||||
|
|||||||
@@ -9,6 +9,7 @@ the agent's task workspace.
|
|||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import re
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
from unittest.mock import patch
|
from unittest.mock import patch
|
||||||
|
|
||||||
@@ -56,6 +57,32 @@ def _make_documenter_config(
|
|||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def _make_product_owner_config(
|
||||||
|
*, project_slug: str = "roboco-api"
|
||||||
|
) -> OrchestratorAgentConfig:
|
||||||
|
"""Minimal AgentConfig for product-owner (product_owner role)."""
|
||||||
|
return OrchestratorAgentConfig(
|
||||||
|
agent_id="product-owner",
|
||||||
|
blueprint_path=Path("/app/agents/blueprints/product-owner.md"),
|
||||||
|
model="sonnet",
|
||||||
|
mcp_config_path=Path("/app/mcp-config.json"),
|
||||||
|
git_context=SpawnGitContext(project_slug=project_slug),
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def _make_head_marketing_config(
|
||||||
|
*, project_slug: str = "roboco-api"
|
||||||
|
) -> OrchestratorAgentConfig:
|
||||||
|
"""Minimal AgentConfig for head-marketing (head_marketing role)."""
|
||||||
|
return OrchestratorAgentConfig(
|
||||||
|
agent_id="head-marketing",
|
||||||
|
blueprint_path=Path("/app/agents/blueprints/head-marketing.md"),
|
||||||
|
model="sonnet",
|
||||||
|
mcp_config_path=Path("/app/mcp-config.json"),
|
||||||
|
git_context=SpawnGitContext(project_slug=project_slug),
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
def _minimal_hosts() -> dict[str, str | None]:
|
def _minimal_hosts() -> dict[str, str | None]:
|
||||||
"""Minimal host-paths dict that satisfies _build_mount_args without real FS."""
|
"""Minimal host-paths dict that satisfies _build_mount_args without real FS."""
|
||||||
return {
|
return {
|
||||||
@@ -100,6 +127,37 @@ def _build_cmd(container_name: str, config: OrchestratorAgentConfig) -> list[str
|
|||||||
return AgentOrchestrator._build_mount_args(container_name, config, hosts)
|
return AgentOrchestrator._build_mount_args(container_name, config, hosts)
|
||||||
|
|
||||||
|
|
||||||
|
def _make_minimal_orchestrator() -> AgentOrchestrator:
|
||||||
|
"""Instantiate AgentOrchestrator with all constructor I/O mocked out."""
|
||||||
|
with patch.object(AgentOrchestrator, "__init__", return_value=None):
|
||||||
|
orch = AgentOrchestrator.__new__(AgentOrchestrator)
|
||||||
|
return orch
|
||||||
|
|
||||||
|
|
||||||
|
def _extract_workdir_from_cmd(cmd: list[str]) -> str | None:
|
||||||
|
"""Return the value after -w in a docker run cmd list, or None."""
|
||||||
|
if "-w" not in cmd:
|
||||||
|
return None
|
||||||
|
return cmd[cmd.index("-w") + 1]
|
||||||
|
|
||||||
|
|
||||||
|
_EDIT_ALLOWLIST_RE = re.compile(r"^Edit\((.+)/\*\*\)$")
|
||||||
|
|
||||||
|
|
||||||
|
def _extract_edit_allowlist_prefix(permissions: dict[str, list[str]]) -> str:
|
||||||
|
"""Extract the workspace path prefix from an Edit(path/**) allowlist entry.
|
||||||
|
|
||||||
|
Raises AssertionError if no matching entry is found.
|
||||||
|
"""
|
||||||
|
for entry in permissions.get("allow", []):
|
||||||
|
m = _EDIT_ALLOWLIST_RE.match(entry)
|
||||||
|
if m:
|
||||||
|
return m.group(1)
|
||||||
|
raise AssertionError(
|
||||||
|
f"No Edit(<path>/**) entry found in allow list: {permissions['allow']}"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
class TestDeveloperSpawnCwdWorkspace:
|
class TestDeveloperSpawnCwdWorkspace:
|
||||||
"""Developer container must start in the agent's task workspace."""
|
"""Developer container must start in the agent's task workspace."""
|
||||||
|
|
||||||
@@ -114,20 +172,42 @@ class TestDeveloperSpawnCwdWorkspace:
|
|||||||
# Developer workspace: /data/workspaces/<project>/<team>/<agent>
|
# Developer workspace: /data/workspaces/<project>/<team>/<agent>
|
||||||
expected = "/data/workspaces/roboco-api/backend/be-dev-1"
|
expected = "/data/workspaces/roboco-api/backend/be-dev-1"
|
||||||
assert workdir == expected, (
|
assert workdir == expected, (
|
||||||
f"Expected workdir '{expected}' but got '{workdir}'. "
|
f"Expected workdir '{expected}' but got '{workdir}'. Full cmd: {cmd}"
|
||||||
f"Full cmd: {cmd}"
|
|
||||||
)
|
)
|
||||||
|
|
||||||
def test_workdir_matches_edit_allowlist_path(self) -> None:
|
def test_workdir_matches_edit_allowlist_path(self) -> None:
|
||||||
"""The -w value matches the Edit({workspace_path}/**) allowlist prefix."""
|
"""The -w value matches the Edit({workspace_path}/**) allowlist prefix.
|
||||||
config = _make_dev_config(project_slug="my-project")
|
|
||||||
|
This test derives the expected path from _get_role_permissions, not
|
||||||
|
from a hard-coded duplicate of the formula. If _build_mount_args and
|
||||||
|
_get_role_permissions drift to different formulas, this test catches it.
|
||||||
|
"""
|
||||||
|
project_slug = "my-project"
|
||||||
|
# Workspace paths that _prepare_agent_spawn would compute for be-dev-1.
|
||||||
|
# be-dev-1 resolves to team=backend (agents_config); we use the same
|
||||||
|
# values the real code uses so the cross-check is meaningful.
|
||||||
|
workspace_path = f"/data/workspaces/{project_slug}/backend/be-dev-1"
|
||||||
|
cell_workspace_path = f"/data/workspaces/{project_slug}/backend"
|
||||||
|
|
||||||
|
orch = _make_minimal_orchestrator()
|
||||||
|
permissions = orch._get_role_permissions(
|
||||||
|
role="developer",
|
||||||
|
workspace_path=workspace_path,
|
||||||
|
cell_workspace_path=cell_workspace_path,
|
||||||
|
)
|
||||||
|
edit_prefix = _extract_edit_allowlist_prefix(permissions)
|
||||||
|
|
||||||
|
# Now build the docker cmd for the same agent/project.
|
||||||
|
config = _make_dev_config(project_slug=project_slug)
|
||||||
cmd = _build_cmd("roboco-agent-be-dev-1", config)
|
cmd = _build_cmd("roboco-agent-be-dev-1", config)
|
||||||
|
|
||||||
w_idx = cmd.index("-w")
|
workdir = _extract_workdir_from_cmd(cmd)
|
||||||
workdir = cmd[w_idx + 1]
|
assert workdir is not None, f"'-w' flag missing from docker run cmd: {cmd}"
|
||||||
# Allowlist in _get_role_permissions: Edit({workspace_path}/**)
|
assert workdir == edit_prefix, (
|
||||||
# workdir must equal that workspace_path
|
f"_build_mount_args -w value '{workdir}' does not match "
|
||||||
assert workdir == "/data/workspaces/my-project/backend/be-dev-1"
|
f"_get_role_permissions Edit allowlist prefix '{edit_prefix}'. "
|
||||||
|
"These two sites must use the same workspace-path formula."
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
class TestCellPmSpawnCwdNoWorkdir:
|
class TestCellPmSpawnCwdNoWorkdir:
|
||||||
@@ -154,9 +234,7 @@ class TestDocumenterSpawnCwdCellWorkspace:
|
|||||||
|
|
||||||
# Documenter allowlist scopes to cell_workspace_path:
|
# Documenter allowlist scopes to cell_workspace_path:
|
||||||
# /data/workspaces/<project>/<team>
|
# /data/workspaces/<project>/<team>
|
||||||
assert "-w" in cmd, (
|
assert "-w" in cmd, f"'-w' flag missing from documenter docker run cmd: {cmd}"
|
||||||
f"'-w' flag missing from documenter docker run cmd: {cmd}"
|
|
||||||
)
|
|
||||||
w_idx = cmd.index("-w")
|
w_idx = cmd.index("-w")
|
||||||
workdir = cmd[w_idx + 1]
|
workdir = cmd[w_idx + 1]
|
||||||
expected = "/data/workspaces/roboco-api/backend"
|
expected = "/data/workspaces/roboco-api/backend"
|
||||||
@@ -164,3 +242,93 @@ class TestDocumenterSpawnCwdCellWorkspace:
|
|||||||
f"Expected documenter workdir '{expected}' but got '{workdir}'. "
|
f"Expected documenter workdir '{expected}' but got '{workdir}'. "
|
||||||
f"Full cmd: {cmd}"
|
f"Full cmd: {cmd}"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
class TestProductOwnerSpawnCwdWorkspace:
|
||||||
|
"""product_owner container must start in the per-agent workspace path."""
|
||||||
|
|
||||||
|
def test_cmd_contains_workdir_flag(self) -> None:
|
||||||
|
"""docker run for a product_owner includes -w <per-agent-workspace>."""
|
||||||
|
config = _make_product_owner_config(project_slug="roboco-api")
|
||||||
|
cmd = _build_cmd("roboco-agent-product-owner", config)
|
||||||
|
|
||||||
|
assert "-w" in cmd, (
|
||||||
|
f"'-w' flag missing from product_owner docker run cmd: {cmd}"
|
||||||
|
)
|
||||||
|
workdir = _extract_workdir_from_cmd(cmd)
|
||||||
|
expected = "/data/workspaces/roboco-api/board/product-owner"
|
||||||
|
assert workdir == expected, (
|
||||||
|
f"Expected product_owner workdir '{expected}' but got '{workdir}'. "
|
||||||
|
f"Full cmd: {cmd}"
|
||||||
|
)
|
||||||
|
|
||||||
|
def test_workdir_matches_edit_allowlist_path(self) -> None:
|
||||||
|
"""The product_owner -w value matches its Edit allowlist prefix."""
|
||||||
|
project_slug = "roboco-api"
|
||||||
|
workspace_path = f"/data/workspaces/{project_slug}/board/product-owner"
|
||||||
|
cell_workspace_path = f"/data/workspaces/{project_slug}/board"
|
||||||
|
|
||||||
|
orch = _make_minimal_orchestrator()
|
||||||
|
permissions = orch._get_role_permissions(
|
||||||
|
role="product_owner",
|
||||||
|
workspace_path=workspace_path,
|
||||||
|
cell_workspace_path=cell_workspace_path,
|
||||||
|
)
|
||||||
|
edit_prefix = _extract_edit_allowlist_prefix(permissions)
|
||||||
|
|
||||||
|
config = _make_product_owner_config(project_slug=project_slug)
|
||||||
|
cmd = _build_cmd("roboco-agent-product-owner", config)
|
||||||
|
workdir = _extract_workdir_from_cmd(cmd)
|
||||||
|
|
||||||
|
assert workdir is not None, (
|
||||||
|
f"'-w' flag missing from product_owner docker run cmd: {cmd}"
|
||||||
|
)
|
||||||
|
assert workdir == edit_prefix, (
|
||||||
|
f"_build_mount_args -w value '{workdir}' != "
|
||||||
|
f"_get_role_permissions Edit prefix '{edit_prefix}'."
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
class TestHeadMarketingSpawnCwdWorkspace:
|
||||||
|
"""head_marketing container must start in the per-agent workspace path."""
|
||||||
|
|
||||||
|
def test_cmd_contains_workdir_flag(self) -> None:
|
||||||
|
"""docker run for a head_marketing includes -w <per-agent-workspace>."""
|
||||||
|
config = _make_head_marketing_config(project_slug="roboco-api")
|
||||||
|
cmd = _build_cmd("roboco-agent-head-marketing", config)
|
||||||
|
|
||||||
|
assert "-w" in cmd, (
|
||||||
|
f"'-w' flag missing from head_marketing docker run cmd: {cmd}"
|
||||||
|
)
|
||||||
|
workdir = _extract_workdir_from_cmd(cmd)
|
||||||
|
expected = "/data/workspaces/roboco-api/board/head-marketing"
|
||||||
|
assert workdir == expected, (
|
||||||
|
f"Expected head_marketing workdir '{expected}' but got '{workdir}'. "
|
||||||
|
f"Full cmd: {cmd}"
|
||||||
|
)
|
||||||
|
|
||||||
|
def test_workdir_matches_edit_allowlist_path(self) -> None:
|
||||||
|
"""The head_marketing -w value matches its Edit allowlist prefix."""
|
||||||
|
project_slug = "roboco-api"
|
||||||
|
workspace_path = f"/data/workspaces/{project_slug}/board/head-marketing"
|
||||||
|
cell_workspace_path = f"/data/workspaces/{project_slug}/board"
|
||||||
|
|
||||||
|
orch = _make_minimal_orchestrator()
|
||||||
|
permissions = orch._get_role_permissions(
|
||||||
|
role="head_marketing",
|
||||||
|
workspace_path=workspace_path,
|
||||||
|
cell_workspace_path=cell_workspace_path,
|
||||||
|
)
|
||||||
|
edit_prefix = _extract_edit_allowlist_prefix(permissions)
|
||||||
|
|
||||||
|
config = _make_head_marketing_config(project_slug=project_slug)
|
||||||
|
cmd = _build_cmd("roboco-agent-head-marketing", config)
|
||||||
|
workdir = _extract_workdir_from_cmd(cmd)
|
||||||
|
|
||||||
|
assert workdir is not None, (
|
||||||
|
f"'-w' flag missing from head_marketing docker run cmd: {cmd}"
|
||||||
|
)
|
||||||
|
assert workdir == edit_prefix, (
|
||||||
|
f"_build_mount_args -w value '{workdir}' != "
|
||||||
|
f"_get_role_permissions Edit prefix '{edit_prefix}'."
|
||||||
|
)
|
||||||
|
|||||||
Reference in New Issue
Block a user