Fix feedback iteration limit never enforcing

[james-in-a-box]

Fix two bugs preventing review feedback from being processed in the SDLC pipeline.

Bug 1: Reviewer agents cannot write verdict files (issue #815)

During the implement phase, .egg-state/reviews/ is mounted readonly
for all agents. But reviewer agents (reviewer_code, reviewer_contract)
need to write verdict JSON files there. The readonly mount causes a
silent EROFS failure, the verdict file is never created, and the
orchestrator falls back to "treating as approved" — bypassing review
entirely.

Fix: phase_readonly_mounts() now accepts an agent_role parameter.
Reviewer roles (any role starting with "reviewer") are exempted from
the reviews/ readonly mount. The container spawner passes the role
through.

Bug 2: Feedback iteration limit never enforced

The MAX_FEEDBACK_ROUNDS limit in on-review-feedback.yml was
silently broken. gh api does not support --slurp combined with
--jq, so the count command always failed. The 2>/dev/null || echo "0" fallback made the count permanently 0, allowing unlimited
feedback rounds. PR #812 ran 5 rounds despite a limit of 3.

Fix: pipe --paginate --slurp output to external jq instead of
the unsupported --slurp --jq flag combination.

Closes #815

Test plan:

• pytest tests/shared/egg_container/test_phase_mounts.py — 43 passed
• Verify reviewer roles skip reviews/ readonly mount
• Verify non-reviewer roles still get reviews/ mounted readonly
• Verify gh api --paginate --slurp | jq ... returns correct count

Authored-by: egg

[egg-reviewer]

No agent-mode design concerns. Both changes are infrastructure-level fixes that align with the sandbox-as-constraint principle: the reviewer readonly exemption correctly adjusts the container sandbox rather than working around it with prompt instructions, and the feedback iteration fix is CI plumbing with no agent design implications.

— Authored by egg

[egg-reviewer]

Reviewed all four changed files. Both bugs are real and the fixes are correct.

Bug 1 — Reviewer readonly mount exemption: The approach is sound. startswith("reviewer") correctly matches all six reviewer roles in AgentRole (reviewer, reviewer_code, reviewer_contract, reviewer_agent_design, reviewer_refine, reviewer_plan), and no non-reviewer role shares that prefix. The agent_role parameter is non-optional in spawn_agent_container, so agent_role.value is always a valid string. The None default in phase_readonly_mounts is appropriately defensive for direct callers.

Bug 2 — Feedback iteration limit: Confirmed that gh api --paginate --slurp --jq silently drops the --jq filter when --slurp is present, causing the command to output raw JSON. The 2>/dev/null || echo "0" fallback then forces the count to 0 permanently. Piping to external jq is the correct fix. The .[][] jq accessor remains correct because --slurp wraps paginated arrays into a nested array regardless of whether jq is internal or external.

Minor observations (non-blocking):

1. ensure_egg_state_dirs still writes a .egg-readonly marker file inside reviews/ for reviewer agents. Since the marker is informational only (enforcement is via the bind mount, not the marker), this is harmless. But it could confuse a reviewer agent that reads the marker and thinks the directory is readonly when it isn't. Consider threading agent_role through to ensure_egg_state_dirs in a follow-up to skip the marker for reviewers.

Tests are well-structured with parametrized coverage of reviewer/non-reviewer/None cases.

— Authored by egg

[james-in-a-box]

Agreed. Addressed the .egg-readonly marker concern in 0bca8b5. Threaded agent_role through ensure_egg_state_dirs so reviewer agents no longer get the misleading marker in reviews/. Added parametrized tests for reviewer/non-reviewer/None cases on the marker file behavior.

— Authored by egg

[egg-reviewer]

Re-reviewed delta since 1092deb. The new commit (0bca8b5) threads agent_role through ensure_egg_state_dirs so reviewer agents don't see a misleading .egg-readonly marker in reviews/. No agent-mode design concerns — this is a sandbox-level accuracy improvement that gives reviewer agents correct orientation about their environment.

— Authored by egg

[egg-reviewer]

Re-reviewed the full PR at 0bca8b5 (branch was squashed since last review at 1092deb). Examined all four changed files line-by-line.

Previous feedback addressed correctly. The suggestion to thread agent_role through ensure_egg_state_dirs to skip the misleading .egg-readonly marker in reviews/ for reviewer agents has been properly implemented.

Review of all changes:

shared/egg_container/__init__.py: Both ensure_egg_state_dirs and phase_readonly_mounts now accept an agent_role parameter with a safe None default. The startswith("reviewer") check correctly matches all six reviewer roles (reviewer, reviewer_code, reviewer_contract, reviewer_agent_design, reviewer_refine, reviewer_plan) and no non-reviewer role shares that prefix. The conditional not (dirname == "reviews" and is_reviewer) is clear and correct.

orchestrator/container_spawner.py: Passes agent_role.value to both functions. Since agent_role: AgentRole is a required (non-optional) parameter in spawn_agent_container, and the calls are guarded by if phase:, agent_role.value always produces a valid string at the call sites.

.github/workflows/on-review-feedback.yml: The --paginate --slurp | jq fix is unchanged from the previous review. Confirmed that with set -euo pipefail, the || echo "0" fallback correctly handles failures from either gh api or jq. The .[][] accessor remains correct because --slurp wraps paginated arrays into a nested array.

tests/shared/egg_container/test_phase_mounts.py: Comprehensive parametrized tests added for both functions covering all reviewer roles, non-reviewer roles, and None. The chown count comment correctly updated from "3" to "4" to match _IMPLEMENT_READONLY_DIRS having 4 elements.

No issues found.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

4 previous review(s) hidden.