Fix feedback iteration limit never enforcing

.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)"
 

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(

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,16 +147,23 @@ 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.
         uid: Owner UID for created directories (default: current user).
         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,19 +217,26 @@ 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.
     """
     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}"

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