diff --git a/roboco/runtime/orchestrator.py b/roboco/runtime/orchestrator.py index b013238d..750db814 100644 --- a/roboco/runtime/orchestrator.py +++ b/roboco/runtime/orchestrator.py @@ -714,8 +714,8 @@ class AgentOrchestrator: configs: dict[str, dict[str, list[str]]] = { "developer": { "allow": [ - f"Write({workspace_path}/**)", - f"Edit({workspace_path}/**)", + f"Write(/{workspace_path}/**)", + f"Edit(/{workspace_path}/**)", ], "deny": [], }, @@ -729,14 +729,14 @@ class AgentOrchestrator: }, "documenter": { "allow": [ - f"Write({cell_workspace_path}/**)", - f"Edit({cell_workspace_path}/**)", - "Write(/app/docs/**)", - "Edit(/app/docs/**)", - "Write(/app/CHANGELOG.md)", - "Edit(/app/CHANGELOG.md)", - "Write(/app/README.md)", - "Edit(/app/README.md)", + f"Write(/{cell_workspace_path}/**)", + f"Edit(/{cell_workspace_path}/**)", + "Write(//app/docs/**)", + "Edit(//app/docs/**)", + "Write(//app/CHANGELOG.md)", + "Edit(//app/CHANGELOG.md)", + "Write(//app/README.md)", + "Edit(//app/README.md)", ], "deny": [], }, @@ -769,15 +769,15 @@ class AgentOrchestrator: }, "product_owner": { "allow": [ - f"Write({workspace_path}/**)", - f"Edit({workspace_path}/**)", + f"Write(/{workspace_path}/**)", + f"Edit(/{workspace_path}/**)", ], "deny": [], }, "head_marketing": { "allow": [ - f"Write({workspace_path}/**)", - f"Edit({workspace_path}/**)", + f"Write(/{workspace_path}/**)", + f"Edit(/{workspace_path}/**)", ], "deny": [], }, @@ -845,9 +845,16 @@ class AgentOrchestrator: base_deny = [ # Block ALL native git commands - must use roboco_git_* tools "Bash(git:*)", - # Block file ops outside workspace (role-specific allows override) - "Write(*)", - "Edit(*)", + # NOTE: Write/Edit are intentionally NOT globally denied here. + # Claude Code evaluates rules deny -> ask -> allow and the first + # match wins, so a deny ALWAYS beats a more-specific allow (the + # glob syntax has no negation). A global Write(*)/Edit(*) here + # therefore unconditionally shadowed the per-role, + # workspace-scoped Write/Edit allows below — every agent (devs + # included) was unable to edit ANY file and fell back to + # destructive bash redirection (clobbering real files). Roles + # that must NOT write (qa, cell_pm, main_pm, auditor) carry + # their own Write(*)/Edit(*) deny in _get_role_permissions. # Block reads of credential stores, anywhere on the FS "Read(**/.git/config)", "Read(**/.gitconfig)", diff --git a/tests/unit/runtime/test_agent_write_permissions.py b/tests/unit/runtime/test_agent_write_permissions.py new file mode 100644 index 00000000..ff1ab149 --- /dev/null +++ b/tests/unit/runtime/test_agent_write_permissions.py @@ -0,0 +1,110 @@ +"""#167: agents could never Edit/Write any file (smoke-10..14). + +Root cause: _generate_agent_settings put ``Write(*)``/``Edit(*)`` in the +GLOBAL base_deny. Claude Code evaluates rules deny -> ask -> allow and the +first match wins, so a deny ALWAYS beats a more-specific allow (the glob +syntax has no negation). The global deny therefore unconditionally +shadowed every per-role workspace-scoped Write/Edit allow — every agent, +developers included, got "Edit exists but is not enabled in this context" +and fell back to destructive bash redirection (clobbering real files; +e.g. a 207-line README rewritten to a 3-line stub, which QA correctly +failed). + +Second defect: the workspace allow used a SINGLE leading slash +(``Write(/data/...)``). Claude Code resolves a single ``/`` against the +settings.json project root, not the container filesystem root, so even +without the global deny the allow never matched. Absolute container +paths require the ``//`` form. + +Fix: drop Write(*)/Edit(*) from base_deny (roles that must not write keep +their OWN Write(*)/Edit(*) deny); emit the workspace allow in the ``//`` +absolute form. +""" + +from __future__ import annotations + +import json +from pathlib import Path +from unittest.mock import patch + +from roboco.runtime.orchestrator import AgentOrchestrator + + +def _orch() -> AgentOrchestrator: + with patch.object(AgentOrchestrator, "__init__", return_value=None): + return AgentOrchestrator.__new__(AgentOrchestrator) + + +_WS = "/data/workspaces/roboco-api/backend/be-dev-1" +_CELL = "/data/workspaces/roboco-api/backend" + +_WRITER_ROLES = ("developer", "documenter", "product_owner", "head_marketing") +_NON_WRITER_ROLES = ("qa", "cell_pm", "main_pm", "auditor") + + +def test_generated_settings_base_deny_has_no_global_write_edit() -> None: + """The settings file a developer is spawned with must NOT globally + deny Write/Edit (that shadowed the workspace allow → unusable).""" + orch = _orch() + path = orch._generate_agent_settings( + agent_id="be-dev-1", + role="developer", + workspace_path=_WS, + cell_workspace_path=_CELL, + ) + settings = json.loads(Path(path).read_text()) + deny = settings["permissions"]["deny"] + allow = settings["permissions"]["allow"] + + assert "Write(*)" not in deny, deny + assert "Edit(*)" not in deny, deny + # The security denies that DO rely on deny-always-wins must remain. + assert "Bash(git:*)" in deny, deny + assert any(".git/config" in d for d in deny), deny + # Workspace allow present and in the // absolute form. + assert f"Write(/{_WS}/**)" in allow, allow + assert f"Edit(/{_WS}/**)" in allow, allow + + +def test_writer_roles_use_double_slash_absolute_allow() -> None: + """Every role that authors files emits Write/Edit allow rules in the + // absolute-filesystem form (single / silently never matches).""" + orch = _orch() + for role in _WRITER_ROLES: + perms = orch._get_role_permissions( + role=role, workspace_path=_WS, cell_workspace_path=_CELL + ) + write_edit = [e for e in perms["allow"] if e.startswith(("Write(", "Edit("))] + assert write_edit, f"{role} should allow some Write/Edit: {perms}" + for entry in write_edit: + inner = entry[entry.index("(") + 1 :] + assert inner.startswith("//"), ( + f"{role} allow rule must use // absolute form: {entry}" + ) + + +def test_non_writer_roles_still_deny_write_edit() -> None: + """Removing the GLOBAL deny must not let QA / PMs / auditor write — + they carry their own Write(*)/Edit(*) deny in the role config.""" + orch = _orch() + for role in _NON_WRITER_ROLES: + perms = orch._get_role_permissions( + role=role, workspace_path=_WS, cell_workspace_path=_CELL + ) + assert "Write(*)" in perms["deny"], f"{role}: {perms}" + assert "Edit(*)" in perms["deny"], f"{role}: {perms}" + + +def test_non_writer_generated_settings_block_write() -> None: + """End-to-end: a cell_pm's generated settings still deny Write/Edit + (their own role deny survives the base_deny change).""" + orch = _orch() + path = orch._generate_agent_settings( + agent_id="be-pm", + role="cell_pm", + workspace_path=_WS, + cell_workspace_path=_CELL, + ) + deny = json.loads(Path(path).read_text())["permissions"]["deny"] + assert "Write(*)" in deny, deny + assert "Edit(*)" in deny, deny diff --git a/tests/unit/runtime/test_spawn_cwd_workspace.py b/tests/unit/runtime/test_spawn_cwd_workspace.py index 92eb778f..5013cb7b 100644 --- a/tests/unit/runtime/test_spawn_cwd_workspace.py +++ b/tests/unit/runtime/test_spawn_cwd_workspace.py @@ -145,14 +145,28 @@ _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. + """Extract the workspace fs-path prefix from an Edit(path/**) rule. + + The rule MUST use the ``//`` absolute-filesystem form: Claude Code + resolves a single leading ``/`` against the settings.json project + root, not the container filesystem root, so a single-slash workspace + allow silently never matches and Edit/Write are effectively denied + (the smoke-10..14 "Edit not enabled in this context" failure). This + asserts the ``//`` invariant and returns the real fs path (one + leading slash) so it can be cross-checked against the docker ``-w``. 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) + rule_path = m.group(1) + assert rule_path.startswith("//"), ( + "Edit/Write allow rule must use the // absolute-filesystem " + "form; a single leading / resolves against the settings.json " + f"project root and silently never matches: {entry}" + ) + return rule_path[1:] raise AssertionError( f"No Edit(/**) entry found in allow list: {permissions['allow']}" )