diff --git a/.github/workflows/on-review-feedback.yml b/.github/workflows/on-review-feedback.yml index 76c305f23a..94f48a4e25 100644 --- a/.github/workflows/on-review-feedback.yml +++ b/.github/workflows/on-review-feedback.yml @@ -369,11 +369,11 @@ jobs: # this for most cases, but a brief window remains. This is acceptable since # the worst case is one extra round, and the human notification still fires. # - # Uses --paginate to handle PRs with >30 comments. The --slurp combines - # paginated results into a nested array, hence the .[][] in jq. + # Uses --paginate --slurp to handle PRs with >30 comments, piped to + # external jq (gh api does not support --slurp combined with --jq). feedback_count=$(gh api "repos/${{ github.repository }}/issues/${{ env.PR_NUMBER }}/comments" \ --paginate --slurp \ - --jq '[.[][] | select(.body | test("egg-feedback-addressing"))] | length' 2>/dev/null || echo "0") + | jq '[.[][] | select(.body | test("egg-feedback-addressing"))] | length' 2>/dev/null || echo "0") echo "Found $feedback_count previous feedback-addressing run(s)" diff --git a/orchestrator/container_spawner.py b/orchestrator/container_spawner.py index 16001cf87c..f2abb006df 100644 --- a/orchestrator/container_spawner.py +++ b/orchestrator/container_spawner.py @@ -106,9 +106,7 @@ def _host_to_local_volumes(repo_volumes: dict[str, str]) -> dict[str, str]: if not host_home or host_home == container_home: return repo_volumes return { - name: path.replace(host_home, container_home, 1) - if path.startswith(host_home) - else path + name: path.replace(host_home, container_home, 1) if path.startswith(host_home) else path for name, path in repo_volumes.items() } @@ -311,8 +309,21 @@ def spawn_agent_container( # (the orchestrator can't access host paths like /home/jwies/...). if phase: local_volumes = _host_to_local_volumes(repo_volumes) - ensure_egg_state_dirs(local_volumes, uid=host_uid, gid=host_gid, phase=phase) - mounts.extend(phase_readonly_mounts(repo_volumes, phase, local_volumes=local_volumes)) + ensure_egg_state_dirs( + local_volumes, + uid=host_uid, + gid=host_gid, + phase=phase, + agent_role=agent_role.value, + ) + mounts.extend( + phase_readonly_mounts( + repo_volumes, + phase, + local_volumes=local_volumes, + agent_role=agent_role.value, + ) + ) if certs_volume: mounts.append( MountSpec( diff --git a/shared/egg_container/__init__.py b/shared/egg_container/__init__.py index 2dc4dc1458..ede09e7bb3 100644 --- a/shared/egg_container/__init__.py +++ b/shared/egg_container/__init__.py @@ -137,6 +137,7 @@ def ensure_egg_state_dirs( uid: int | None = None, gid: int | None = None, phase: str | None = None, + agent_role: str | None = None, ) -> None: """Ensure ``.egg-state/`` subdirectories exist in each repo worktree. @@ -146,6 +147,8 @@ def ensure_egg_state_dirs( When ``phase`` is ``"implement"``, ``.egg-readonly`` marker files are placed in each readonly directory to explain the restriction to agents. + Reviewer agents are exempted from the ``reviews/`` marker since that + directory is not mounted readonly for them. Args: repo_volumes: Mapping of repo_name -> host_path. @@ -153,9 +156,14 @@ def ensure_egg_state_dirs( gid: Owner GID for created directories (default: current group). phase: Current SDLC phase. When ``"implement"``, marker files are written into readonly directories. + agent_role: Agent role string (e.g., "reviewer_code"). Reviewer + roles (starting with "reviewer") are exempted from the + ``reviews/`` marker file. """ import os + is_reviewer = agent_role is not None and agent_role.startswith("reviewer") + for _repo_name, host_path in repo_volumes.items(): egg_state = Path(host_path) / ".egg-state" for dirname in _IMPLEMENT_READONLY_DIRS: @@ -165,7 +173,9 @@ def ensure_egg_state_dirs( os.chown(str(target), uid, gid) # Place marker files in readonly directories during implement phase. - if phase == "implement": + # Skip the reviews/ marker for reviewer agents since reviews/ is + # not mounted readonly for them. + if phase == "implement" and not (dirname == "reviews" and is_reviewer): marker = target / ".egg-readonly" marker.write_text( f"This directory is readonly during the '{phase}' phase.\n" @@ -184,6 +194,7 @@ def phase_readonly_mounts( phase: str | None, container_base: str = "/home/egg/repos", local_volumes: dict[str, str] | None = None, + agent_role: str | None = None, ) -> list[MountSpec]: """Create readonly overlay mounts for phase-protected directories. @@ -192,6 +203,9 @@ def phase_readonly_mounts( ``.egg-state/reviews/`` are mounted readonly to prevent agents from modifying plan/contract artifacts via direct filesystem writes. + Reviewer agents are exempted from the ``reviews/`` readonly mount + because they need to write verdict files there. + Args: repo_volumes: Mapping of repo_name -> host_path. These paths are used as Docker mount sources and may be host-absolute paths @@ -203,6 +217,9 @@ def phase_readonly_mounts( for ``is_dir()`` filesystem checks when ``repo_volumes`` contains host paths inaccessible to the current process. Mount sources still come from ``repo_volumes``. + agent_role: Agent role string (e.g., "reviewer_code"). Reviewer + roles (starting with "reviewer") are exempted from the + ``reviews/`` readonly mount so they can write verdict files. Returns: List of MountSpec for readonly overlay mounts. @@ -210,12 +227,16 @@ def phase_readonly_mounts( if phase != "implement": return [] + is_reviewer = agent_role is not None and agent_role.startswith("reviewer") + check_volumes = local_volumes if local_volumes is not None else repo_volumes mounts: list[MountSpec] = [] for repo_name, host_path in repo_volumes.items(): check_path = check_volumes.get(repo_name, host_path) for dirname in _IMPLEMENT_READONLY_DIRS: + if dirname == "reviews" and is_reviewer: + continue host_dir = Path(host_path) / ".egg-state" / dirname check_dir = Path(check_path) / ".egg-state" / dirname container_dir = f"{container_base}/{repo_name}/.egg-state/{dirname}" diff --git a/tests/shared/egg_container/test_phase_mounts.py b/tests/shared/egg_container/test_phase_mounts.py index 4d29a59a95..ebfde41c23 100644 --- a/tests/shared/egg_container/test_phase_mounts.py +++ b/tests/shared/egg_container/test_phase_mounts.py @@ -137,9 +137,55 @@ def test_marker_files_chowned_when_uid_gid_provided(self, tmp_path): repo_volumes = {"repo": str(tmp_path)} with patch("os.chown") as mock_chown: ensure_egg_state_dirs(repo_volumes, uid=1000, gid=1000, phase="implement") - # 3 directory chowns + 3 marker file chowns + # 4 directory chowns + 4 marker file chowns assert mock_chown.call_count == 2 * len(_IMPLEMENT_READONLY_DIRS) + @pytest.mark.parametrize( + "role", + [ + "reviewer_code", + "reviewer_contract", + "reviewer_agent_design", + "reviewer_refine", + "reviewer_plan", + "reviewer", + ], + ) + def test_reviewer_skips_reviews_marker(self, role, tmp_path): + """Reviewer agents don't get .egg-readonly marker in reviews/.""" + repo_volumes = {"repo": str(tmp_path)} + ensure_egg_state_dirs(repo_volumes, phase="implement", agent_role=role) + + # reviews/ should NOT have the marker + reviews_marker = tmp_path / ".egg-state" / "reviews" / ".egg-readonly" + assert not reviews_marker.exists() + + # Other dirs still get markers + for dirname in _IMPLEMENT_READONLY_DIRS: + if dirname == "reviews": + continue + marker = tmp_path / ".egg-state" / dirname / ".egg-readonly" + assert marker.exists(), f"Missing marker in {dirname}" + + @pytest.mark.parametrize("role", ["coder", "tester", "integrator", "documenter"]) + def test_non_reviewer_keeps_reviews_marker(self, role, tmp_path): + """Non-reviewer agents still get .egg-readonly marker in reviews/.""" + repo_volumes = {"repo": str(tmp_path)} + ensure_egg_state_dirs(repo_volumes, phase="implement", agent_role=role) + + for dirname in _IMPLEMENT_READONLY_DIRS: + marker = tmp_path / ".egg-state" / dirname / ".egg-readonly" + assert marker.exists(), f"Missing marker in {dirname}" + + def test_no_role_keeps_reviews_marker(self, tmp_path): + """No agent_role (default) keeps .egg-readonly marker in reviews/.""" + repo_volumes = {"repo": str(tmp_path)} + ensure_egg_state_dirs(repo_volumes, phase="implement", agent_role=None) + + for dirname in _IMPLEMENT_READONLY_DIRS: + marker = tmp_path / ".egg-state" / dirname / ".egg-readonly" + assert marker.exists(), f"Missing marker in {dirname}" + class TestPhaseReadonlyMounts: """Tests for phase_readonly_mounts().""" @@ -278,3 +324,54 @@ def test_local_volumes_missing_dir_skipped(self, tmp_path): mounts = phase_readonly_mounts(repo_volumes, "implement", local_volumes=local_volumes) assert mounts == [] + + @pytest.mark.parametrize( + "role", + [ + "reviewer_code", + "reviewer_contract", + "reviewer_agent_design", + "reviewer_refine", + "reviewer_plan", + "reviewer", + ], + ) + def test_reviewer_roles_skip_reviews_readonly(self, role, tmp_path): + """Reviewer agents are exempted from the reviews/ readonly mount.""" + for dirname in _IMPLEMENT_READONLY_DIRS: + (tmp_path / ".egg-state" / dirname).mkdir(parents=True) + + repo_volumes = {"myrepo": str(tmp_path)} + mounts = phase_readonly_mounts(repo_volumes, "implement", agent_role=role) + + destinations = {m.destination for m in mounts} + assert "/home/egg/repos/myrepo/.egg-state/reviews" not in destinations + # Other dirs still readonly + assert "/home/egg/repos/myrepo/.egg-state/drafts" in destinations + assert "/home/egg/repos/myrepo/.egg-state/contracts" in destinations + assert "/home/egg/repos/myrepo/.egg-state/pipelines" in destinations + assert len(mounts) == len(_IMPLEMENT_READONLY_DIRS) - 1 + + @pytest.mark.parametrize("role", ["coder", "tester", "integrator", "documenter"]) + def test_non_reviewer_roles_keep_reviews_readonly(self, role, tmp_path): + """Non-reviewer agents still get reviews/ mounted readonly.""" + for dirname in _IMPLEMENT_READONLY_DIRS: + (tmp_path / ".egg-state" / dirname).mkdir(parents=True) + + repo_volumes = {"myrepo": str(tmp_path)} + mounts = phase_readonly_mounts(repo_volumes, "implement", agent_role=role) + + destinations = {m.destination for m in mounts} + assert "/home/egg/repos/myrepo/.egg-state/reviews" in destinations + assert len(mounts) == len(_IMPLEMENT_READONLY_DIRS) + + def test_no_role_keeps_reviews_readonly(self, tmp_path): + """No agent_role (default) keeps reviews/ readonly.""" + for dirname in _IMPLEMENT_READONLY_DIRS: + (tmp_path / ".egg-state" / dirname).mkdir(parents=True) + + repo_volumes = {"myrepo": str(tmp_path)} + mounts = phase_readonly_mounts(repo_volumes, "implement", agent_role=None) + + destinations = {m.destination for m in mounts} + assert "/home/egg/repos/myrepo/.egg-state/reviews" in destinations