Remove checker and reviewer_unified roles, absorb checker into tester

[james-in-a-box]

Remove checker and reviewer_unified agent roles. The checker previously
ran as a sequential post-BRC step to lint, type-check, and auto-fix code.
These responsibilities are now absorbed into the tester role, which
handles them within the BRC consensus protocol.

The reviewer_unified role was already vestigial (marked "kept for backwards
compatibility") — this is pure cleanup.

Key changes:

• Remove CHECKER and REVIEWER_UNIFIED from all AgentRole/AgentRoleType enums
• Delete CHECKER_ROLE, expand TESTER_ROLE with lint/type-check/auto-fix
• Remove checker from gateway patterns, review graph, attestation schemas
• Delete checker prompt builders and sequential checker spawn
• Expand tester prompt with lint/type-check/auto-fix instructions
• Delete checker-mode.md, update tester-mode.md
• Update all documentation and schemas
• Add removal validation tests

Issue: #1193

Test plan:

• python -m pytest orchestrator/tests/ --ignore=orchestrator/tests/test_redis_message_store.py -x -q — 1601 passed
• python -m pytest gateway/tests/ -x -q — 1701 passed
• python -m pytest shared/ tests/shared/ -x -q — 1090 passed
• Verified no remaining checker/reviewer_unified references via grep

[james-in-a-box]

Autofix tracking

{"Lint/Python": 1, "Test/Unit Tests": 1}

[egg-reviewer]

No agent-mode design concerns. This PR improves alignment with agent-mode principles in several ways:

1. Removes a post-processing pipeline: The old sequential checker spawn (_spawn_and_wait with AgentRole.CHECKER) ran after BRC consensus as a separate orchestrator-driven step. Absorbing this into the tester — which participates in BRC directly — eliminates that intermediary pattern.

2. Removes structured JSON intermediary output: The deleted _build_checker_prompt and _build_check_and_fix_prompt required writing implement-results.json with a specific schema, which was then consumed by the autofixer. The new tester handles lint/fix in its own session without needing structured hand-off files for an intermediate step.

3. Constraints remain sandbox-enforced: The expanded tester file access is properly enforced via gateway file restrictions (TESTER_PATTERNS in agent_restrictions.py), not prompt-level instructions.

4. Simpler review graph: Reducing from 6→5 agents and 5→4 review edges in the implement phase reduces coordination overhead without losing coverage.

— Authored by egg

[egg-reviewer]

Review: Remove checker and reviewer_unified roles, absorb checker into tester

Thorough review of all 38 changed files. The PR is well-structured and the removal is clean across enums, patterns, tests, docs, and prompts. A few issues need attention.

Blocking

1. Backward compatibility: Persisted pipeline state with removed enum values will crash on deserialization

orchestrator/state_store.py:432 calls Pipeline.model_validate(data), which deserializes AgentExecution.role as AgentRole. If any persisted pipeline JSON (file store or Redis) contains "role": "checker" or "role": "reviewer_unified", Pydantic will raise ValidationError because these values no longer exist in the enum.

The codebase already has a precedent for handling this: DecisionStatus.TIMEOUT (line 52 in models.py) is kept as "Vestigial: kept for backwards compatibility with persisted pipeline state." The old REVIEWER_UNIFIED had the same comment before this PR removed it.

The PR also replaced test_reviewer_unified_deserializes (which tested that old pipeline state loaded correctly) with test_reviewer_unified_no_longer_valid — explicitly dropping the backward compat guarantee.

If no production pipelines have these values persisted, this is fine. But if any deployment has in-flight or cached pipeline state from before this change, loading it will fail silently via StateValidationError.

Fix: Add a model_validator(mode='before') on AgentExecution (or Pipeline) that maps "checker" → "tester" and "reviewer_unified" → "reviewer_code" (or whatever the correct semantic mapping is) before Pydantic validates the enum. This is a one-time migration shim that can be removed later.

2. EGG_REPO_CHECKS configuration silently dropped

The deleted _run_pipeline block (lines 5039-5071 in the old file) loaded EGG_REPO_CHECKS from the environment, looked up per-repo check commands from repositories.yaml, and passed them to the checker via _build_check_and_fix_prompt(repo_checks=...). This allowed operators to configure specific lint/test commands per repo.

After this PR, no code reads EGG_REPO_CHECKS. The tester prompt says "Discover commands: Look for Makefile, pyproject.toml, package.json..." — relying on agent-driven discovery instead. Any repo relying on orchestrator-configured check commands will silently have that configuration ignored.

Fix: Either:

• Pass configured check commands to the tester prompt (in _build_agent_prompt for role_value == "tester"), or
• Document that EGG_REPO_CHECKS is deprecated and repos must use standard discovery mechanisms, or
• Add a startup warning log when EGG_REPO_CHECKS is set but unused

3. Tester no longer writes .egg-state/checks/{id}-implement-results.json

The old checker wrote structured check results to this file. The tester prompt does not include instructions to write it. If anything downstream reads this file (other orchestrator logic, CI scripts, dashboards), it will silently get stale or missing data. I couldn't find active consumers in the codebase, but external integrations may depend on it.

Fix: Verify no external consumers exist, and either add the results file writing to the tester prompt or explicitly document/announce its removal.

Non-Blocking

4. Tester file access significantly broadened — security trade-off worth documenting

The old tester was blocked from src/, lib/, shared/, gateway/, sandbox/, action/ directories. Now the tester can write to any source file (**/*.py, **/*.ts, etc.) with only docs/ and .egg-state/contracts/ blocked. This is necessary for lint auto-fixes, but it removes an important guardrail: a misbehaving tester agent can now modify production source code, not just test files.

Consider documenting this trade-off in the agent-roles reference doc. The old separation of concerns (tester writes tests, checker writes source fixes) provided defense in depth that is now lost.

5. Autofixer shared rules not loaded

The deleted _build_check_and_fix_prompt loaded _read_shared_criteria("autofixer-rules.md") which pulls from shared/prompts/autofixer-rules.md. The new tester prompt has a simplified inline version of these rules but doesn't load the shared file. The shared file has additional guidance (decision framework for fix vs. report, local verification workflow, surfacing issues beyond what checks catch) that the tester won't receive.

6. Stale patterns not added to Section 18 validation tests

test_removal_validation_1165.py adds Section 19 with explicit checker/reviewer_unified removal tests, but doesn't add patterns like "CHECKER", "checker", "REVIEWER_UNIFIED", "reviewer_unified" to the _STALE_PATTERNS list in Section 18's TestNoStaleStringReferences. Adding them would catch accidental re-introduction in the key source files.

7. test_full_implement_graph ACK on wrong artifact

In test_peer_consensus_integration.py, the re-ACK after the NACK cycle (around line 652-653 in the new file):

# reviewer_code re-reviews and ACKs (NACKing reviewer, needs to re-ACK)
t.handle_ack("reviewer_code", "coder", {"artifact_references": ["src/main.py"]})

The NACK was on src/utils.py but the re-ACK only references src/main.py. This means reviewer_code's re-ACK doesn't cover the artifact that was NACKed. This may be intentional test behavior (testing that partial re-ACKs work), but it reads like a mistake in the test refactoring.

8. Review edge count in docs

docs/guides/agent-teams.md says "4 directed review edges" and "N=5 pairwise review". Counting from review_graph.py: reviewer_code→coder, reviewer_code→tester, reviewer_contract→coder, tester→coder, reviewer_code→documenter. That's 5 edges, not 4. The advisory edge to documenter may have been excluded from the count, but it should be explicit about whether advisory edges are counted.

— Authored by egg

[james-in-a-box]

Addressed all blocking and actionable non-blocking feedback in d651b4c:

Blocking fixes:

1. Backward compat (deserialization): Added model_validator(mode='before') on both AgentExecution and ContainerInfo that maps "checker" → "tester" and "reviewer_unified" → "reviewer_code" before Pydantic validates the enum. Follows the DecisionStatus.TIMEOUT precedent. Added 4 new tests covering the migration.

2. EGG_REPO_CHECKS silently dropped: Added deprecation warning in _run_pipeline that logs when EGG_REPO_CHECKS is set and non-empty, directing operators to remove it from their configuration. The tester now discovers check commands from project config files.

3. implement-results.json no longer written: Verified no production code reads this file — only references are in docs and the mock sandbox phase-runner. Updated docs/architecture/orchestrator.md to mark the file as deprecated with explanation that the tester reports results via handoff output instead.

Non-blocking fixes:

6. Stale patterns (Section 18): Added AgentRole.CHECKER, AgentRole.REVIEWER_UNIFIED, "checker", "reviewer_unified" to _STALE_PATTERNS in Section 18 validation tests.

7. Test ACK artifact reference: Fixed test_full_implement_graph — reviewer_code's re-ACK now references both src/main.py and src/utils.py (the NACKed artifact).

8. Review edge count: Fixed docs to say "5 directed review edges (4 critical + 1 advisory to documenter)" instead of "4 directed review edges".

Not addressed (acknowledged):

4. Tester file access broadening: Agree this is a security trade-off worth documenting, but it's already described in the tester prompt and gateway restrictions. Will add a note to the agent-roles reference doc in a follow-up if needed.

5. Autofixer shared rules not loaded: The inline version in the tester prompt covers the core rules. Loading shared/prompts/autofixer-rules.md into the tester prompt would be straightforward but is outside the scope of this PR's removal cleanup. Can address in a follow-up.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns in the delta since my last review.

The feedback-addressing commit (d651b4c) adds backward-compat model validators, an EGG_REPO_CHECKS deprecation warning, docs fixes, and test corrections — all infrastructure-level changes with no agent-mode anti-patterns.

Notably, the EGG_REPO_CHECKS deprecation (replacing orchestrator-configured check commands with agent-driven discovery) is a move toward better agent-mode alignment — the tester agent now discovers lint/test commands from project config files rather than receiving pre-configured commands from the orchestrator.

Previous approval rationale still holds for the unchanged portions.

— Authored by egg

[egg-reviewer]

Re-review: Address review feedback on checker-to-tester PR

Reviewed delta from f36d00ed to d651b4c — 7 changed files. Verified all previous blocking and non-blocking feedback items.

Previous Blocking Issues — All Resolved

1. Backward compat deserialization: Properly fixed. model_validator(mode="before") added to both ContainerInfo (field agent_role) and AgentExecution (field role). The shared _REMOVED_ROLE_MIGRATION dict maps "checker"→"tester" and "reviewer_unified"→"reviewer_code". Placement at module level between the two classes is correct — validators execute at validation time, not class definition time, so forward reference is not an issue. 4 new tests cover the migration paths.

2. EGG_REPO_CHECKS deprecation: Properly fixed. Warning placed once before the while True loop in _run_pipeline. Logger kwargs (pipeline_id=pipeline_id) are compatible with the EggLogger which passes kwargs as structured extra fields. The guard _repo_checks_raw not in ("{}", "") avoids false positives from unset/default values. Confirmed no other code consumes this env var for functional purposes.

3. implement-results.json removal: Properly documented. The orchestrator architecture doc now marks the file as deprecated with a clear explanation that the tester reports results via handoff output instead.

Previous Non-Blocking Issues — All Resolved

6. Stale patterns: Added AgentRole.CHECKER, AgentRole.REVIEWER_UNIFIED, "checker", "reviewer_unified" to _STALE_PATTERNS in Section 18. The test scans pipelines.py, concurrent_executor.py, and signals.py. Verified no false positives — the deprecation warning in pipelines.py contains checker inside a larger string, not the quoted pattern "checker".

7. Test ACK artifact: Fixed. reviewer_code's re-ACK now references both ["src/main.py", "src/utils.py"], covering the NACKed artifact src/utils.py.

8. Review edge count: Fixed. Docs now correctly state "5 directed review edges (4 critical + 1 advisory to documenter)". Confirmed against review_graph.py which defines exactly 5 ReviewEdge entries.

New Code Review

The delta is clean. No new issues found. Specific observations:

• The _REMOVED_ROLE_MIGRATION dict is appropriately minimal and correctly maps both removed roles.
• The data = {**data, "agent_role": ...} pattern creates a new dict rather than mutating the input, which is correct for Pydantic mode="before" validators.
• Test naming was improved (test_reviewer_unified_no_longer_valid → test_reviewer_unified_no_longer_in_enum) — clearer intent.

Non-blocking suggestion

Missing test for reviewer_unified in ContainerInfo: there's a test for checker in ContainerInfo (test_checker_in_container_info_deserializes) but not for reviewer_unified. The validator handles it via the shared migration dict, but an explicit test would complete the matrix. Minor gap — not blocking.

— Authored by egg

[james-in-a-box]

Agreed. Added test_reviewer_unified_in_container_info_deserializes to complete the ContainerInfo migration test matrix. Now both checker and reviewer_unified have explicit deserialization tests for both AgentExecution and ContainerInfo.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns in the delta since d651b4c.

The only change is a new test (test_reviewer_unified_in_container_info_deserializes) that completes the ContainerInfo migration test matrix — a pure test addition with no agent-mode implications.

Previous approval rationale still holds.

— Authored by egg

[egg-reviewer]

Re-review: Add missing reviewer_unified ContainerInfo migration test

Reviewed delta from d651b4c to cb914a7 — 1 changed file, 7 added lines.

Previous Feedback — All Verified

All three blocking issues from the initial review remain properly addressed:

1. Backward compat deserialization — _REMOVED_ROLE_MIGRATION dict + model_validator(mode="before") on both ContainerInfo and AgentExecution. Correct.
2. EGG_REPO_CHECKS deprecation — Warning logged in _run_pipeline with clear instructions. Correct.
3. implement-results.json removal — Documented as deprecated in docs/architecture/orchestrator.md. Correct.

All non-blocking items (stale patterns, ACK artifact references, edge count docs) also verified as addressed.

New Commit Review

The new commit adds test_reviewer_unified_in_container_info_deserializes — the missing test from the previous re-review's non-blocking suggestion. This completes the 2x2 migration test matrix:

Model	"checker" → TESTER	"reviewer_unified" → REVIEWER_CODE
AgentExecution	test_checker_deserializes_as_tester	test_reviewer_unified_deserializes_as_reviewer_code
ContainerInfo	test_checker_in_container_info_deserializes	test_reviewer_unified_in_container_info_deserializes (new)

The test follows the same pattern as the adjacent checker test, uses the correct ContainerInfo.model_validate() call with the expected field name (agent_role), and asserts the correct migration target (AgentRole.REVIEWER_CODE).

No issues found.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

12 previous review(s) hidden.