From 4135b5bd5c6c32f16d070dad9aa5a0e35851bebf Mon Sep 17 00:00:00 2001 From: egg Date: Wed, 18 Feb 2026 03:57:43 +0000 Subject: [PATCH 1/5] Fix feedback iteration limit never enforcing --- .github/workflows/on-review-feedback.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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)" From 30b31ac82071b1cbb51cfe3097393042ec133dd5 Mon Sep 17 00:00:00 2001 From: egg Date: Wed, 18 Feb 2026 04:20:18 +0000 Subject: [PATCH 2/5] Exempt reviewer agents from reviews/ readonly mount --- orchestrator/container_spawner.py | 6 ++- shared/egg_container/__init__.py | 11 +++++ .../shared/egg_container/test_phase_mounts.py | 44 +++++++++++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/orchestrator/container_spawner.py b/orchestrator/container_spawner.py index 16001cf87c..e4659d063a 100644 --- a/orchestrator/container_spawner.py +++ b/orchestrator/container_spawner.py @@ -312,7 +312,11 @@ def spawn_agent_container( 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)) + 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..e41e28c581 100644 --- a/shared/egg_container/__init__.py +++ b/shared/egg_container/__init__.py @@ -184,6 +184,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 +193,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 +207,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 +217,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..7a3c1cb583 100644 --- a/tests/shared/egg_container/test_phase_mounts.py +++ b/tests/shared/egg_container/test_phase_mounts.py @@ -278,3 +278,47 @@ 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 From f8f00f9f6e321ed7be7566b7d7c9e3d7f0a98c3f Mon Sep 17 00:00:00 2001 From: egg Date: Wed, 18 Feb 2026 20:29:44 +0000 Subject: [PATCH 3/5] Remove dangling setup-gateway symlink The symlink target (gateway/setup.sh) was deleted but the symlink in bin/ was not cleaned up. This dangling symlink breaks GitHub Actions action downloads because the runner cannot resolve it during the Set up job step. --- bin/setup-gateway | 1 - 1 file changed, 1 deletion(-) delete mode 120000 bin/setup-gateway diff --git a/bin/setup-gateway b/bin/setup-gateway deleted file mode 120000 index 4d44706107..0000000000 --- a/bin/setup-gateway +++ /dev/null @@ -1 +0,0 @@ -../gateway/setup.sh \ No newline at end of file From f5ac35816aa4e73aa8eee1108139ec5f62d452fd Mon Sep 17 00:00:00 2001 From: egg Date: Wed, 18 Feb 2026 20:34:36 +0000 Subject: [PATCH 4/5] Revert dangling symlink removal (moved to PR #823) --- bin/setup-gateway | 1 + 1 file changed, 1 insertion(+) create mode 120000 bin/setup-gateway diff --git a/bin/setup-gateway b/bin/setup-gateway new file mode 120000 index 0000000000..4d44706107 --- /dev/null +++ b/bin/setup-gateway @@ -0,0 +1 @@ +../gateway/setup.sh \ No newline at end of file From 0bca8b582cc139476f099723792b8b032697bade Mon Sep 17 00:00:00 2001 From: "egg-reviewer[bot]" <261018737+egg-reviewer[bot]@users.noreply.github.com> Date: Wed, 18 Feb 2026 21:01:57 +0000 Subject: [PATCH 5/5] Skip .egg-readonly marker in reviews/ for reviewer agents Thread agent_role through ensure_egg_state_dirs so that reviewer agents don't get a misleading .egg-readonly marker in reviews/, since that directory is not mounted readonly for them. Addresses review feedback on the reviewer readonly exemption. --- orchestrator/container_spawner.py | 23 ++++--- shared/egg_container/__init__.py | 12 +++- .../shared/egg_container/test_phase_mounts.py | 63 +++++++++++++++++-- 3 files changed, 84 insertions(+), 14 deletions(-) diff --git a/orchestrator/container_spawner.py b/orchestrator/container_spawner.py index e4659d063a..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,12 +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 e41e28c581..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" diff --git a/tests/shared/egg_container/test_phase_mounts.py b/tests/shared/egg_container/test_phase_mounts.py index 7a3c1cb583..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().""" @@ -279,10 +325,17 @@ 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", - ]) + @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: