From 10be97fd5acbaca14ae2b4d8481d02a3910017ff Mon Sep 17 00:00:00 2001 From: Renn F Date: Tue, 12 May 2026 03:13:11 +0200 Subject: [PATCH] =?UTF-8?q?refactor(orchestrator):=20A2+A3=20follow-ups=20?= =?UTF-8?q?=E2=80=94=20extract=20workspace-path=20helpers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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(/**) 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). --- roboco/runtime/orchestrator.py | 56 +++-- .../unit/runtime/test_spawn_cwd_workspace.py | 192 ++++++++++++++++-- 2 files changed, 224 insertions(+), 24 deletions(-) diff --git a/roboco/runtime/orchestrator.py b/roboco/runtime/orchestrator.py index 88632291..63c2e86e 100644 --- a/roboco/runtime/orchestrator.py +++ b/roboco/runtime/orchestrator.py @@ -169,6 +169,44 @@ def _resolve_agent_cli_model(provider_type: str, model: str) -> str: 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///. 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) # ============================================================================= @@ -1239,7 +1277,7 @@ class AgentOrchestrator: """Build AgentConfig + AgentInstance and surface per-agent settings path.""" blueprint_path = self._generate_composed_prompt(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` # wins (dispatcher overrides, tests). Otherwise the routing service @@ -1252,8 +1290,8 @@ class AgentOrchestrator: model = route.model_name project_slug = self._resolve_project_slug(git_context, agent_id, task_id) - workspace_path = f"/data/workspaces/{project_slug}/{team}/{agent_id}" - cell_workspace_path = f"/data/workspaces/{project_slug}/{team}" + workspace_path = _agent_workspace_path(project_slug, team, agent_id) + cell_workspace_path = _cell_workspace_path(project_slug, team) agent_settings_path = self._generate_agent_settings( agent_id, canonical_role, workspace_path, cell_workspace_path @@ -1574,19 +1612,13 @@ class AgentOrchestrator: # so the container falls back to /app (Dockerfile WORKDIR). _role = get_agent_role(config.agent_id) or "developer" _team = get_agent_team(config.agent_id) or "" - _project = ( - 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}" + _project = _resolve_project_slug_from_git_context(config.git_context) _roles_with_agent_workspace = {"developer", "product_owner", "head_marketing"} _roles_with_cell_workspace = {"documenter"} 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: - 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 return cmd diff --git a/tests/unit/runtime/test_spawn_cwd_workspace.py b/tests/unit/runtime/test_spawn_cwd_workspace.py index 1de63d68..92eb778f 100644 --- a/tests/unit/runtime/test_spawn_cwd_workspace.py +++ b/tests/unit/runtime/test_spawn_cwd_workspace.py @@ -9,6 +9,7 @@ the agent's task workspace. from __future__ import annotations +import re from pathlib import Path 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]: """Minimal host-paths dict that satisfies _build_mount_args without real FS.""" return { @@ -100,6 +127,37 @@ def _build_cmd(container_name: str, config: OrchestratorAgentConfig) -> list[str 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(/**) entry found in allow list: {permissions['allow']}" + ) + + class TestDeveloperSpawnCwdWorkspace: """Developer container must start in the agent's task workspace.""" @@ -114,20 +172,42 @@ class TestDeveloperSpawnCwdWorkspace: # Developer workspace: /data/workspaces/// expected = "/data/workspaces/roboco-api/backend/be-dev-1" assert workdir == expected, ( - f"Expected workdir '{expected}' but got '{workdir}'. " - f"Full cmd: {cmd}" + f"Expected workdir '{expected}' but got '{workdir}'. Full cmd: {cmd}" ) def test_workdir_matches_edit_allowlist_path(self) -> None: - """The -w value matches the Edit({workspace_path}/**) allowlist prefix.""" - config = _make_dev_config(project_slug="my-project") + """The -w value matches the Edit({workspace_path}/**) allowlist prefix. + + 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) - w_idx = cmd.index("-w") - workdir = cmd[w_idx + 1] - # Allowlist in _get_role_permissions: Edit({workspace_path}/**) - # workdir must equal that workspace_path - assert workdir == "/data/workspaces/my-project/backend/be-dev-1" + workdir = _extract_workdir_from_cmd(cmd) + assert workdir is not None, f"'-w' flag missing from docker run cmd: {cmd}" + assert workdir == edit_prefix, ( + f"_build_mount_args -w value '{workdir}' does not match " + f"_get_role_permissions Edit allowlist prefix '{edit_prefix}'. " + "These two sites must use the same workspace-path formula." + ) class TestCellPmSpawnCwdNoWorkdir: @@ -154,9 +234,7 @@ class TestDocumenterSpawnCwdCellWorkspace: # Documenter allowlist scopes to cell_workspace_path: # /data/workspaces// - assert "-w" in cmd, ( - f"'-w' flag missing from documenter docker run cmd: {cmd}" - ) + assert "-w" in cmd, f"'-w' flag missing from documenter docker run cmd: {cmd}" w_idx = cmd.index("-w") workdir = cmd[w_idx + 1] expected = "/data/workspaces/roboco-api/backend" @@ -164,3 +242,93 @@ class TestDocumenterSpawnCwdCellWorkspace: f"Expected documenter workdir '{expected}' but got '{workdir}'. " 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 .""" + 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 .""" + 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}'." + )