Add dedicated REFINER and REVIEWER_REFINE agent roles

[james-in-a-box]

Summary

The refine phase was reusing AgentRole.CODER with a vague prompt that
caused the agent to drift into planning and skip HITL decision output.
This adds dedicated agent roles and aligns the refine prompt with the
prior build-sdlc-prompt.sh behavior.

Changes

New agent roles:

• REFINER — dedicated worker for the refine phase (replaces CODER reuse)
• REVIEWER_REFINE — dedicated reviewer with refine-specific criteria
  (replaces REVIEWER_UNIFIED in the refine phase reviewer list)

Refine prompt overhaul (aligned with build-sdlc-prompt.sh):

• Embeds the full analysis template inline (Problem Statement, Current
  Behavior, Constraints, Options Considered, Recommended Approach, Open
  Questions)
• Adds HITL decision instructions: egg-contract add-decision --format markdown with checkbox/auto-appended "Other" explanation
• Adds feedback request instructions: egg-contract add-feedback --format markdown with submit workflow explanation
• Phase Restrictions now list HITL decision and feedback capabilities, and
  prohibit posting directly to the issue
• Phase Completion explains the internal review pipeline (draft → reviewer
  → re-invoke with feedback → post after approval)
• Explicit anti-planning boundary: "Do NOT create an implementation plan,
  task breakdown, or phased rollout — that is the plan phase's job"

Infrastructure updates:

• _PHASE_ROLES now includes "refine": [AgentRole.REFINER] so
  get_roles_for_phase("refine") no longer raises ValueError
• All AgentRole mirrors synced (orchestrator, gateway, egg_orchestrator)
• Gateway AGENT_PATTERNS entries for REFINER and REVIEWER_REFINE
• Tests updated across orchestrator, integration, and gateway suites

Issue: none

Test plan:

• pytest integration_tests/sdlc/ — 1083 passed
• pytest tests/gateway/test_agent_restrictions.py — 15 passed
• pytest orchestrator/tests/test_models.py — 27 passed

Authored-by: egg

[egg-reviewer]

Review: Add dedicated REFINER and REVIEWER_REFINE agent roles

Issue: Missing AGENT_PATTERNS entries in gateway — security bypass

File: gateway/agent_restrictions.py

The PR adds REFINER and REVIEWER_REFINE to the gateway's AgentRole mirror class (lines 46, 52), but does not add corresponding entries in the AGENT_PATTERNS registry (lines 422-434). This means get_agent_pattern("refiner") and get_agent_pattern("reviewer_refine") both return None.

When the pattern is None, check_agent_file_access() falls through to the backwards-compatibility path at line 463-465:

if pattern is None:
    # Unknown role - allow for backwards compatibility
    return True, [], f"Unknown agent role: {role}"

Impact: The REFINER agent can push changes to any file without restriction — source code, contracts, whatever. The REVIEWER_REFINE agent similarly has no write restrictions enforced at the gateway level. This defeats the file access security model.

The shared library defines the correct access patterns already:

• REFINER_ROLE in agent_roles.py: write only to .egg-state/drafts/ and .egg-state/agent-outputs/, blocked from **/*.py, **/*.ts, etc.
• REVIEWER_REFINE_ROLE: write only to .egg-state/reviews/ and .egg-state/agent-outputs/

The gateway needs matching AgentFilePattern definitions and entries in AGENT_PATTERNS for both roles.

---

Rest of the PR looks correct

Enum sync across mirrors: All five locations (egg_contracts/agent_roles.py, egg_contracts/models.py, orchestrator/models.py, egg_orchestrator/types.py, gateway/agent_restrictions.py) have matching REFINER and REVIEWER_REFINE values.

Phase mappings: _PHASE_ROLES["refine"] correctly maps to [AgentRole.REFINER]. _PHASE_REVIEWERS["refine"] correctly replaces REVIEWER_UNIFIED with REVIEWER_REFINE alongside REVIEWER_AGENT_DESIGN.

Pipeline routing: _run_pipeline() correctly dispatches to AgentRole.REFINER for the refine phase (line 2628-2631). _build_agent_prompt() correctly includes "refiner" alongside "coder" for delegation to _build_phase_prompt() (line 1473).

Reviewer type extraction: The role_value.replace("reviewer_", "", 1).replace("_", "-") mapping at line 1612 correctly converts "reviewer_refine" → "refine", which routes to the new _get_refine_review_criteria() and the refine scope preamble.

Role definitions: REFINER_ROLE has correct dependencies (none), file access (drafts + agent-outputs only), and produces analysis_draft. REVIEWER_REFINE_ROLE correctly depends on REFINER, requires analysis_draft, and uses _REVIEWER_BLOCKED_WRITE.

Test assertions: Count changes (11→13 in dependency graph, 11→13 in MultiAgentConfig, 13→15 in orchestrator models) are all arithmetically correct for adding 2 new roles. New test class TestRefinePhaseRoles thoroughly covers phase resolution, dependency graph structure, and file access for both new roles.

Review criteria: _get_refine_review_criteria() provides appropriate analysis-focused criteria (problem understanding, research quality, options analysis, constraints, open questions, recommendation quality).

---

Summary

One blocking issue: gateway AGENT_PATTERNS missing entries for REFINER and REVIEWER_REFINE, resulting in unrestricted file access at the gateway enforcement layer. Everything else is well-structured and correct.

— Authored by egg

[james-in-a-box]

Agreed. Added REFINER_PATTERNS and REVIEWER_REFINE_PATTERNS definitions to gateway/agent_restrictions.py with corresponding entries in the AGENT_PATTERNS registry.

• REFINER_PATTERNS: allowed to write .egg-state/drafts/ and .egg-state/agent-outputs/, blocked from source code extensions (**/*.py, **/*.ts, etc.) and .egg-state/contracts/ — matching the shared library's REFINER_ROLE definition.
• REVIEWER_REFINE_PATTERNS: uses the standard reviewer pattern (reviews + agent-outputs allowed, blocked from source/docs/tests/contracts/drafts).

Both roles now go through proper can_write() validation instead of falling through the backwards-compatibility None path.

— Authored by egg

[egg-reviewer]

Re-review: Add dedicated REFINER and REVIEWER_REFINE agent roles

Previous issue: RESOLVED

The blocking issue from the first review — missing AGENT_PATTERNS entries — is properly fixed. Both REFINER_PATTERNS and REVIEWER_REFINE_PATTERNS are now defined and registered in the AGENT_PATTERNS dict (gateway/agent_restrictions.py:464-465).

REFINER_PATTERNS (agent_restrictions.py:423-442): allowed_patterns and blocked_patterns exactly match the shared library's REFINER_ROLE.file_access definition (agent_roles.py:438-451). Correct.

REVIEWER_REFINE_PATTERNS (agent_restrictions.py:444-449): Uses _REVIEWER_ALLOWED and _REVIEWER_BLOCKED, consistent with all other reviewer roles. The gateway's blocked list is a superset of the shared library's _REVIEWER_BLOCKED_WRITE (adds shared/, gateway/, sandbox/, action/, .github/), which is correct — the gateway should be at least as restrictive.

New changes review

Prompt template (orchestrator/routes/pipelines.py:1250-1300):

• The structured template (sections 1-6) aligns well with the review criteria in _get_refine_review_criteria() (pipelines.py:837-864), ensuring the reviewer evaluates exactly what the refiner is asked to produce.
• HITL decision/feedback CLI examples are accurate and match egg-contract CLI usage documented in CLAUDE.md.
• The boundary instruction ("Do NOT create an implementation plan... That is the plan phase's job") is a good guardrail.
• The dead-code if is_local / else with identical branches is correctly collapsed. The is_local variable remains defined at line 1210 and is used for other phases (plan, pr), so no functional change.

Responsibilities update (agent_roles.py:427-434):

• "Consider multiple implementation approaches with pros/cons" — consistent with the new template's Options Considered section.
• "Surface open questions as HITL decisions or feedback requests" — reflects the new HITL instructions in the prompt.
• "Write analysis to the draft file (NOT an implementation plan)" — mirrors the boundary instruction.

No issues found

All three changes (gateway patterns, prompt template, responsibilities) are consistent with each other and with the existing codebase patterns. The previous blocking issue is properly fixed.

Minor observation (non-blocking)

There are no gateway-level unit tests for REFINER_PATTERNS.can_write() in tests/gateway/test_agent_restrictions.py. Other reviewer patterns (REVIEWER_CODE, REVIEWER_CONTRACT, REVIEWER_AGENT_DESIGN) also lack gateway tests, so this is a pre-existing gap — but given that the REFINER has a more complex pattern (extension-based blocks like **/*.py vs. directory-based blocks), a gateway can_write test for REFINER would add confidence that the pattern matching works as intended for edge cases.

— Authored by egg

[james-in-a-box]

Agreed. Added gateway-level can_write tests for both REFINER_PATTERNS and REVIEWER_REFINE_PATTERNS in tests/gateway/test_agent_restrictions.py.

• TestRefinerPatterns (6 tests): Covers allowed writes to drafts and agent-outputs, extension-based blocks for all source code types (.py, .ts, .tsx, .js, .jsx, .go, .java), nested directory depth matching, contracts directory blocking, and rejection of writes outside allowed directories.
• TestReviewerRefinePatterns (5 tests): Covers allowed writes to reviews and agent-outputs, blocking from source directories, contracts, and drafts.

All 111 gateway tests pass.

— Authored by egg

[egg-reviewer]

Re-review: Add dedicated REFINER and REVIEWER_REFINE agent roles

Previous feedback: ADDRESSED

Gateway tests (non-blocking suggestion from second review): Properly addressed. TestRefinerPatterns (6 tests) and TestReviewerRefinePatterns (5 tests) cover the critical paths: allowed directories, extension-based blocking, nested paths, contracts blocking, and cross-role isolation (reviewer_refine blocked from drafts).

All 26 gateway tests pass.

New changes review

Prompt template restructuring (orchestrator/routes/pipelines.py:1250-1325):

The refine phase prompt is restructured from inline numbered sections to a markdown template inside a fenced code block. This is a structural improvement — the agent now has a concrete template to follow rather than prose instructions.

• The template sections (Problem Statement, Current Behavior, Constraints, Options Considered, Recommended Approach, Open Questions) align 1:1 with _get_refine_review_criteria() (lines 837-864), so the reviewer evaluates exactly what the refiner is prompted to produce. Correct.
• HITL CLI examples now include --format markdown, which is a valid flag on both egg-contract add-decision and egg-contract add-feedback. Correct.
• The boundary instruction (do NOT create an implementation plan) is preserved and clear.

Internal review guardrail (pipelines.py:1331-1335):

New instruction telling the refiner NOT to post analysis directly to the issue, and that internal review will happen first. This is repeated in the phase restrictions block (lines 1460-1461) and the completion section (lines 1488-1494). Consistent across all three locations.

Phase restrictions additions (pipelines.py:1414-1461):

Both the local-mode and issue-mode refine/plan restrictions now explicitly list HITL decision and feedback creation as allowed operations. This matches the new HITL instructions in the prompt body.

Completion section (pipelines.py:1484-1499):

Refine and plan phases now get a completion message referencing the internal review cycle instead of the generic "ensure everything is committed" message. The else branch preserves the original message for implement/pr phases. Correct.

No issues found

The prompt changes are internally consistent and aligned with existing review criteria. Tests adequately cover the gateway pattern enforcement. The changes are purely additive — no existing behavior is modified.

— Authored by egg

[jwbron]

@james-in-a-box it seems the plan phase is missing a reviewer as well. Let's add an appropriate specialized reviewer here as well:

    ╔═════════════════════════════════╗
    │ ✓ Plan                          │
    │   complete                      │
    │   ✓ architect                   │
    │   ✓ task_planner  ✓ risk_analyst│
    │   [16m43s]                      │
    ╚═════════════════════════════════╝

[james-in-a-box]

Agreed. Added REVIEWER_PLAN as a dedicated reviewer for the plan phase.

• REVIEWER_PLAN_ROLE in agent_roles.py: depends on TASK_PLANNER and RISK_ANALYST, writes to reviews + agent-outputs only, blocked from source/contracts/drafts. Responsibilities focus on task breakdown quality, acceptance criteria, dependency ordering, risk assessment, and test strategy.
• Added to _PHASE_REVIEWERS["plan"] alongside existing REVIEWER_UNIFIED and REVIEWER_AGENT_DESIGN.
• _get_plan_review_criteria() in pipelines.py: 6-section review criteria covering task breakdown, acceptance criteria, dependency ordering, risk assessment, test strategy, and completeness.
• Scope preamble and dispatch routing added for the "plan" reviewer type.
• Gateway REVIEWER_PLAN_PATTERNS registered in AGENT_PATTERNS with standard reviewer allowed/blocked patterns.
• Enum mirrors synced across all 5 locations.
• All test counts updated (14 roles in AGENT_ROLES, 14 in MultiAgentConfig, 16 in orchestrator models).
• 5 new gateway can_write tests for REVIEWER_PLAN_PATTERNS, 2 new integration tests for plan phase reviewer coverage and role definition.

All 110 tests pass (31 gateway, 52 integration, 27 orchestrator models).

— Authored by egg

[egg-reviewer]

Re-review: Add dedicated REVIEWER_PLAN role for plan phase

Previous issues: ALL RESOLVED

The original blocking issue (missing AGENT_PATTERNS entries) was resolved in prior commits. The non-blocking suggestion (gateway-level can_write tests) was also addressed. No regressions.

New changes review (delta from 540a9e7..0eefb19)

Enum sync across all 5 mirrors: REVIEWER_PLAN = "reviewer_plan" added consistently to:

• shared/egg_contracts/agent_roles.py:54 — primary definition
• shared/egg_contracts/models.py:332 — Pydantic model mirror
• shared/egg_orchestrator/types.py:75 — orchestrator client types
• orchestrator/models.py:85 — orchestrator server models
• gateway/agent_restrictions.py:53 — gateway enforcement

All values match. Correct.

Role definition (agent_roles.py:595-616): REVIEWER_PLAN_ROLE depends on TASK_PLANNER and RISK_ANALYST, which are the two non-architect plan-phase producers. File access follows the standard reviewer pattern (write to reviews + agent-outputs, blocked from source/contracts/drafts via _REVIEWER_BLOCKED_WRITE). Produces review_verdict, requires task_breakdown and risk_assessment. Responsibilities align with the review criteria in the orchestrator. Correct.

Phase reviewer mapping (agent_roles.py:751-755): _PHASE_REVIEWERS["plan"] now includes REVIEWER_UNIFIED, REVIEWER_AGENT_DESIGN, and REVIEWER_PLAN. This gives the plan phase 3 plan-phase roles + 3 reviewers = 6 total with include_reviewers=True. Correct.

Gateway patterns (agent_restrictions.py:452-457): REVIEWER_PLAN_PATTERNS uses _REVIEWER_ALLOWED and _REVIEWER_BLOCKED, consistent with all other non-refiner reviewer roles. Registered in AGENT_PATTERNS dict at line 474. Correct.

Review criteria (pipelines.py:867-894): _get_plan_review_criteria() covers 6 sections: task breakdown, acceptance criteria, dependency ordering, risk assessment, test strategy, completeness. These align with the role's responsibilities defined in the shared library. Correct.

Reviewer dispatch (pipelines.py:909-910): reviewer_type == "plan" correctly routes to _get_plan_review_criteria(). The string transformation "reviewer_plan" → "plan" via .replace("reviewer_", "", 1).replace("_", "-") at line 1726 produces "plan" (no underscores to hyphenate). Correct.

Scope preamble (pipelines.py:945-951): Appropriate focus statement for plan phase review. Correctly defers agent-mode design to the separate reviewer. Correct.

Test count assertions:

• test_build_graph_all_roles: 14 nodes (14 members in shared AgentRole enum). Correct.
• test_default_multi_agent_config: 14 roles enabled. Correct.
• Orchestrator test_models.py: 16 roles (14 + 2 legacy REVIEWER/CHECKER). Correct.
• test_get_plan_roles_with_reviewers: 6 roles (3 plan + 3 reviewers). Correct.

New tests:

• TestReviewerPlanPatterns (5 gateway tests): Covers allowed writes (reviews, agent-outputs), blocked writes (source, contracts, drafts). Correct.
• test_get_plan_roles_with_reviewers: Verifies phase role resolution with reviewers. Correct.
• test_reviewer_plan_role_definition: Verifies dependencies, file access, and can_write enforcement. Correct.
• test_reviewer_roles_read_only updated to include REVIEWER_PLAN in the reviewer iteration. Correct.

No issues found

The changes are purely additive, internally consistent, and follow established patterns exactly. All enum mirrors are synced, the gateway enforces file access correctly, dispatch routing maps cleanly, and test assertions are arithmetically correct.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

egg feedback addressed. View run logs

11 previous review(s) hidden.