From b68f52bee566c290382ac644a08c407053b92e02 Mon Sep 17 00:00:00 2001 From: jwbron <8340608+jwbron@users.noreply.github.com> Date: Sun, 26 Apr 2026 22:13:54 +0000 Subject: [PATCH 1/2] docs: Add reviewer_code_holistic to agent docs [doc-updater] Update documentation to reflect changes from #2126 (44567193e): - agent-roles.md: Add reviewer_code_holistic to REVIEW category table, Role Overview table, and new Implement Phase section describing its holistic lens scope and four mandatory passes - concurrent-execution.md: Add reviewer_code_holistic to implement-phase spawn table and clarify its relationship to the reviewer_code fan-out - checkpoint-browser.md: Add reviewer_code_holistic to composite BRC role filtering table - README.md: Update implement-phase ASCII diagram to show the new role Triggered by: https://github.com/jwbron/egg/pull/2130 Authored-by: egg --- README.md | 3 ++- docs/guides/concurrent-execution.md | 4 ++-- docs/reference/agent-roles.md | 30 +++++++++++++++++++++++----- docs/reference/checkpoint-browser.md | 3 ++- 4 files changed, 31 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index bfe882ac19..d2d45da164 100644 --- a/README.md +++ b/README.md @@ -132,7 +132,8 @@ Within each phase, specialized agents run concurrently via BRC (enabled by defau │ complete │ │ ✓ coder │ │ ✓ tester ✓ documenter │ -│ ✓ reviewer_code ✓ reviewer_contract │ +│ ✓ reviewer_code ✓ reviewer_code_holistic │ +│ ✓ reviewer_contract │ │ ✓ reviewer_security ✓ reviewer_concurrency │ │ [1h11m] │ ╚═══════════════════════════════════════════════╝ diff --git a/docs/guides/concurrent-execution.md b/docs/guides/concurrent-execution.md index 633cf7b7ab..90116a0428 100644 --- a/docs/guides/concurrent-execution.md +++ b/docs/guides/concurrent-execution.md @@ -42,7 +42,7 @@ When concurrent execution starts, the `ConcurrentPhaseExecutor` (in `orchestrato |-------|--------------| | `refine` | `refiner`, `reviewer_refine`, `reviewer_agent_design` (egg repo only) | | `plan` | `architect`, `task_planner`, `risk_analyst`, `reviewer_plan` | -| `implement` | `coder`, `tester`, `documenter`, `reviewer_code`, `reviewer_contract`, `reviewer_security` (ADVISORY), `reviewer_concurrency` (ADVISORY) | +| `implement` | `coder`, `tester`, `documenter`, `reviewer_code`, `reviewer_code_holistic`, `reviewer_contract`, `reviewer_security` (ADVISORY), `reviewer_concurrency` (ADVISORY) | **Shared branch**: All agents operate on the pipeline's shared branch (e.g., `egg/issue-123`). Agents coordinate commits via the message bus to sequence their work and avoid conflicts. @@ -355,7 +355,7 @@ A reviewer has three outcomes on a proposal: ### Implement-phase `reviewer_code` Subagent Fan-Out -On the implement phase, `reviewer_code` self-gates on diff size and partitions large diffs across Claude Agent SDK `Task` subagents so every changed file is read carefully ([#1965](https://github.com/jwbron/egg/issues/1965)). The reviewer first runs `git diff --numstat` against the resolved base ref; when the diff exceeds **~10 changed files OR ~500 lines of change**, it self-fetches `phases.implement.tasks[]` via `mcp__sdlc__show_contract` and spawns one subagent per task partition (capped at 6 subagents per review with a 5-minute / 300-second per-subagent wall-clock timeout). Each subagent re-runs `git diff` filtered by its assigned path globs, reviews only its slice, and is forbidden from spawning subagents of its own. The parent reviewer then performs a **cross-partition consistency pass** (handler ↔ allowlist, route ↔ schema, fixture ↔ Dockerfile/symlink, import-graph cycles) before emitting a single ACK / NACK on behalf of the whole review — closing the cross-file blind spot that let [PR #1964](https://github.com/jwbron/egg/pull/1964)'s `^project$` allowlist bypass slip through. Below the threshold, on empty implement-phase task lists (custom-phase invocations, contractless `babysit_pr`), or when MCP is unreachable from the subagent context, the reviewer falls back to single-pass review with a STATUS heartbeat noting the gate decision (`fan-out: enabled / skipped`). Parallelism is configurable per pipeline via `phase_configs.implement.reviewer_code.parallel` (default `true`); the fan-out block lives in `_build_review_prompt()` in `orchestrator/routes/pipelines.py` and is described in detail in [`shared/prompts/REVIEWER-SYNC.md`](../../shared/prompts/REVIEWER-SYNC.md). The two new ADVISORY lens reviewers `reviewer_security` and `reviewer_concurrency` (criteria in [`security-review-criteria.md`](../../shared/prompts/security-review-criteria.md) and [`concurrency-review-criteria.md`](../../shared/prompts/concurrency-review-criteria.md)) run alongside `reviewer_code` on the same change set; their NACKs are recorded but do not deadlock consensus until severity-tagged NACK signalling lands in [#1997](https://github.com/jwbron/egg/issues/1997). +On the implement phase, `reviewer_code` self-gates on diff size and partitions large diffs across Claude Agent SDK `Task` subagents so every changed file is read carefully ([#1965](https://github.com/jwbron/egg/issues/1965)). The reviewer first runs `git diff --numstat` against the resolved base ref; when the diff exceeds **~10 changed files OR ~500 lines of change**, it self-fetches `phases.implement.tasks[]` via `mcp__sdlc__show_contract` and spawns one subagent per task partition (capped at 6 subagents per review with a 5-minute / 300-second per-subagent wall-clock timeout). Each subagent re-runs `git diff` filtered by its assigned path globs, reviews only its slice, and is forbidden from spawning subagents of its own. The parent reviewer then performs a **cross-partition consistency pass** (handler ↔ allowlist, route ↔ schema, fixture ↔ Dockerfile/symlink, import-graph cycles) before emitting a single ACK / NACK on behalf of the whole review — closing the cross-file blind spot that let [PR #1964](https://github.com/jwbron/egg/pull/1964)'s `^project$` allowlist bypass slip through. Below the threshold, on empty implement-phase task lists (custom-phase invocations, contractless `babysit_pr`), or when MCP is unreachable from the subagent context, the reviewer falls back to single-pass review with a STATUS heartbeat noting the gate decision (`fan-out: enabled / skipped`). Parallelism is configurable per pipeline via `phase_configs.implement.reviewer_code.parallel` (default `true`); the fan-out block lives in `_build_review_prompt()` in `orchestrator/routes/pipelines.py` and is described in detail in [`shared/prompts/REVIEWER-SYNC.md`](../../shared/prompts/REVIEWER-SYNC.md). `reviewer_code_holistic` ([#2126](https://github.com/jwbron/egg/issues/2126)) runs alongside `reviewer_code` as a distinct CRITICAL reviewer focused on cross-module coherence — it skims the full diff once and runs four holistic passes (end-to-end use case, doc↔code symmetry, synthetic-key/sentinel audit, silent-fallback hunt) rather than reviewing every file line-by-line. Its NACK gates consensus independently and is not averaged with fan-out slice ACKs. The two ADVISORY lens reviewers `reviewer_security` and `reviewer_concurrency` (criteria in [`security-review-criteria.md`](../../shared/prompts/security-review-criteria.md) and [`concurrency-review-criteria.md`](../../shared/prompts/concurrency-review-criteria.md)) also run alongside `reviewer_code` on the same change set; their NACKs are recorded but do not deadlock consensus until severity-tagged NACK signalling lands in [#1997](https://github.com/jwbron/egg/issues/1997). ### Pre-Proposal ACK Protection diff --git a/docs/reference/agent-roles.md b/docs/reference/agent-roles.md index eb419ff2a2..68d0e69c6f 100644 --- a/docs/reference/agent-roles.md +++ b/docs/reference/agent-roles.md @@ -10,7 +10,7 @@ Every agent role belongs to one of five categories. Categories enable dynamic te |----------|---------|-------| | **EXECUTION** | Produce artifacts (code, tests, docs) | `coder`, `tester`, `documenter` | | **ANALYSIS** | Analyze tasks and plan work | `refiner`, `architect`, `task_planner`, `risk_analyst` | -| **REVIEW** | Validate quality and correctness | `reviewer_code`, `reviewer_contract`, `reviewer_refine`, `reviewer_plan`, `reviewer_agent_design`, `reviewer_security`, `reviewer_concurrency` | +| **REVIEW** | Validate quality and correctness | `reviewer_code`, `reviewer_code_holistic`, `reviewer_contract`, `reviewer_refine`, `reviewer_plan`, `reviewer_agent_design`, `reviewer_security`, `reviewer_concurrency` | | **UTILITY** | Cross-cutting support tasks | `autofixer`, `conflict_resolver` | | **INTERFACE** | Pipeline health and monitoring | `inspector`, `overseer` | @@ -30,10 +30,11 @@ Use `get_roles_by_category(AgentCategory.REVIEW)` to dynamically query roles by | `coder` | Execution | Implement | No | — | | `tester` | Execution | Implement | Yes (with `documenter`) | coder | | `documenter` | Execution | Implement | Yes (with `tester`) | coder | -| `reviewer_code` | Review | Implement | Yes (with `reviewer_contract`, `reviewer_security`, `reviewer_concurrency`) | coder, tester | -| `reviewer_contract` | Review | Implement | Yes (with `reviewer_code`, `reviewer_security`, `reviewer_concurrency`) | coder, tester | -| `reviewer_security` | Review | Implement | Yes (with `reviewer_code`, `reviewer_contract`, `reviewer_concurrency`) | coder, tester | -| `reviewer_concurrency` | Review | Implement | Yes (with `reviewer_code`, `reviewer_contract`, `reviewer_security`) | coder, tester | +| `reviewer_code` | Review | Implement | Yes (with `reviewer_code_holistic`, `reviewer_contract`, `reviewer_security`, `reviewer_concurrency`) | coder, tester | +| `reviewer_code_holistic` | Review | Implement | Yes (with `reviewer_code`, `reviewer_contract`, `reviewer_security`, `reviewer_concurrency`) | coder, tester | +| `reviewer_contract` | Review | Implement | Yes (with `reviewer_code`, `reviewer_code_holistic`, `reviewer_security`, `reviewer_concurrency`) | coder, tester | +| `reviewer_security` | Review | Implement | Yes (with `reviewer_code`, `reviewer_code_holistic`, `reviewer_contract`, `reviewer_concurrency`) | coder, tester | +| `reviewer_concurrency` | Review | Implement | Yes (with `reviewer_code`, `reviewer_code_holistic`, `reviewer_contract`, `reviewer_security`) | coder, tester | | `autofixer` | Utility | Any | Yes | — | | `conflict_resolver` | Utility | Any | Yes | — | | `inspector` | Interface | Any | — | — (health checks) | @@ -245,6 +246,25 @@ each surface so reviewers know to keep them in sync. **Outputs**: - `.egg-state/reviews/{identifier}-implement-reviewer_code-review.json` — Verdict file +### `reviewer_code_holistic` + +**Purpose**: Single-pass holistic code review focused on cross-module coherence. Runs alongside `reviewer_code`'s slice-by-slice fan-out — its job is the architectural-coherence question no fan-out slice owns. + +**Criticality**: CRITICAL — NACKs block consensus on their own and are not averaged against `reviewer_code`'s fan-out ACKs. + +**Focus areas** (four mandatory passes): +1. Walk the primary advertised use case end-to-end across the full diff. +2. Cross-check doc-claimed behaviour against what the code actually does. +3. Audit synthetic keys, sentinels, and magic values for cross-module agreement. +4. Hunt silent fallbacks that swallow operator-visible misconfiguration. + +**File access**: +- Allowed writes: `.egg-state/reviews/`, `.egg-state/agent-outputs/` +- Blocked: All source, docs, tests, contracts, drafts + +**Outputs**: +- `.egg-state/reviews/{identifier}-implement-reviewer_code_holistic-review.json` — Verdict file + ### `reviewer_contract` **Purpose**: Verify acceptance criteria are met and all tasks are marked complete in the contract. diff --git a/docs/reference/checkpoint-browser.md b/docs/reference/checkpoint-browser.md index e09b441626..3956847c6c 100644 --- a/docs/reference/checkpoint-browser.md +++ b/docs/reference/checkpoint-browser.md @@ -63,7 +63,8 @@ The `--agent-type` flag accepts both coarse agent types (e.g., `reviewer`) and c | Composite Role | Description | |----------------|-------------| -| `reviewer_code` | Code quality reviewer | +| `reviewer_code` | Code quality reviewer (fan-out, slice-by-slice) | +| `reviewer_code_holistic` | Holistic code reviewer (cross-module coherence) | | `reviewer_contract` | Contract compliance reviewer | | `reviewer_agent_design` | Agent design reviewer | | `reviewer_refine` | Refinement reviewer | From f353ab924c255b0071a8b34f3ec6165c94339cc6 Mon Sep 17 00:00:00 2001 From: "egg-reviewer[bot]" <261018737+egg-reviewer[bot]@users.noreply.github.com> Date: Sun, 26 Apr 2026 22:29:01 +0000 Subject: [PATCH 2/2] docs: Fix reviewer verdict file path format in agent-roles The documented output paths used 'reviewer_' as the filename component, but pipelines.py:9229 strips the 'reviewer_' prefix and converts '_' -> '-' before building the path at pipelines.py:3717. Verified against on-disk filenames in .egg-state/reviews/ (e.g. '507-implement-code-review.json', '*-refine-agent-design-review.json', '*-plan-plan-review.json'). Fixes all eight occurrences in docs/reference/agent-roles.md: refine-refine, refine-agent-design, plan-plan, implement-code, implement-code-holistic, implement-contract, implement-security, implement-concurrency. --- docs/reference/agent-roles.md | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/reference/agent-roles.md b/docs/reference/agent-roles.md index 68d0e69c6f..82deb02eee 100644 --- a/docs/reference/agent-roles.md +++ b/docs/reference/agent-roles.md @@ -67,7 +67,7 @@ All agents within a phase run concurrently via BRC consensus. Concurrency is ena - Blocked: All source code, contracts, drafts **Outputs**: -- `.egg-state/reviews/{identifier}-refine-reviewer_refine-review.json` — Verdict file +- `.egg-state/reviews/{identifier}-refine-refine-review.json` — Verdict file ### `reviewer_agent_design` @@ -78,7 +78,7 @@ All agents within a phase run concurrently via BRC consensus. Concurrency is ena **File access**: Same as `reviewer_refine`. **Outputs**: -- `.egg-state/reviews/{identifier}-refine-reviewer_agent_design-review.json` — Verdict file +- `.egg-state/reviews/{identifier}-refine-agent-design-review.json` — Verdict file ## Plan Phase @@ -125,7 +125,7 @@ All agents within a phase run concurrently via BRC consensus. Concurrency is ena **File access**: Same as `reviewer_refine`. **Outputs**: -- `.egg-state/reviews/{identifier}-plan-reviewer_plan-review.json` — Verdict file +- `.egg-state/reviews/{identifier}-plan-plan-review.json` — Verdict file ## Implement Phase @@ -244,7 +244,7 @@ each surface so reviewers know to keep them in sync. **Subagent fan-out**: On large diffs (`files_changed > 10` OR `loc_added + loc_removed > 500`), `reviewer_code` fans out into Claude Agent SDK subagents — one per implement-phase task partition (capped at 6, with a 5-minute / 300-second per-subagent wall-clock timeout that NACKs the partition on overrun). Each subagent reviews its slice; the parent aggregates findings and emits the single ACK/NACK. A mandatory cross-partition consistency pass runs regardless of whether fan-out fires. Fan-out can be forced sequential via `phase_configs.implement.reviewer_code.parallel = false` (default: `true`). **Outputs**: -- `.egg-state/reviews/{identifier}-implement-reviewer_code-review.json` — Verdict file +- `.egg-state/reviews/{identifier}-implement-code-review.json` — Verdict file ### `reviewer_code_holistic` @@ -263,7 +263,7 @@ each surface so reviewers know to keep them in sync. - Blocked: All source, docs, tests, contracts, drafts **Outputs**: -- `.egg-state/reviews/{identifier}-implement-reviewer_code_holistic-review.json` — Verdict file +- `.egg-state/reviews/{identifier}-implement-code-holistic-review.json` — Verdict file ### `reviewer_contract` @@ -274,7 +274,7 @@ each surface so reviewers know to keep them in sync. - Blocked: All source, docs, tests, drafts **Outputs**: -- `.egg-state/reviews/{identifier}-implement-reviewer_contract-review.json` — Verdict file +- `.egg-state/reviews/{identifier}-implement-contract-review.json` — Verdict file ### `reviewer_security` @@ -287,7 +287,7 @@ each surface so reviewers know to keep them in sync. - Blocked: All source, docs, tests, contracts, drafts **Outputs**: -- `.egg-state/reviews/{identifier}-implement-reviewer_security-review.json` — Verdict file +- `.egg-state/reviews/{identifier}-implement-security-review.json` — Verdict file ### `reviewer_concurrency` @@ -300,7 +300,7 @@ each surface so reviewers know to keep them in sync. - Blocked: All source, docs, tests, contracts, drafts **Outputs**: -- `.egg-state/reviews/{identifier}-implement-reviewer_concurrency-review.json` — Verdict file +- `.egg-state/reviews/{identifier}-implement-concurrency-review.json` — Verdict file ## Utility Roles