Improve generalized reviewer with subagent fan-out and lens reviewers - #2061
Merged
Conversation
Adds .egg-state/drafts/1965-analysis.md scoping the BRC reviewer improvements (subagent delegation in reviewer_code + new reviewer_security and reviewer_concurrency roles). Surface area is narrow because the issue body has pre-resolved most decisions; the analysis captures the residual implementation-shape questions (threshold metrics path, contract task-list plumbing, threshold composition, subagent diff scope, criteria inheritance, parallelism config site, replay-test execution mode) and registers them as HITL decisions. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Architecture analysis for "improve the generalized reviewer" — A + B small (subagent delegation inside reviewer_code, plus reviewer_security and reviewer_concurrency ADVISORY reviewers). Embeds the seven HITL-resolved technical decisions, suggested implement-phase task partition, test strategy outline, and risk hand-off list to the risk_analyst. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Decomposes the architecture analysis into a single-PR, five-phase plan shipping (A) reviewer_code subagent fan-out and (B) two new ADVISORY lens reviewers (reviewer_security, reviewer_concurrency) per the issue's resolved scope. Phase 1 wires up the new agent-role enum members, role definitions, implement-phase membership, and four ADVISORY review-graph edges plus unit tests guarding the role-mapping invariance line and the absence of attestation models (pitfalls 1 and 4). Phase 2 adds the lens criteria files (each inheriting from code-review-criteria.md per decision-5), the criteria loaders, and the scope preambles. Phase 3 adds the phase_configs.implement.reviewer_code.parallel knob to PhaseConfig (decision-6). Phase 4 attaches the fan-out block to reviewer_code's prompt — reviewer self-gates via git diff --numstat (decision-1), self-fetches phases.implement.tasks[] via mcp__sdlc__show_contract (decision-2), uses OR threshold composition (decision-3), each subagent re-runs git diff filtered by path glob (decision-4), and parallelism honours the new knob. Phase 5 closes with the PR #1964 regression replay (CI-unconditional prompt-asserts plus opt-in live-LLM run gated by RUN_REVIEWER_REPLAY=1, per decision-7) and brief doc updates. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
19 risks across security/performance/compatibility/correctness, with mitigations and rollback plans, plus 5 areas flagged for human review. 4 high-severity risks (subagent phase-gating, Task subagent SDK maturity, MCP reachability inside subagents, cross-partition findings that reproduce PR #1964's failure mode). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Amend C5 (subagent fan-out block) with a MANDATORY parent cross-partition pass that runs after subagents return: parent reviewer reads the full unfiltered diff and explicitly checks handler ↔ allowlist consistency, route ↔ schema consistency, and fixture ↔ Dockerfile reference consistency before issuing ACK/NACK. Closes the cross-file blind spot that motivated #1965 in the first place — without this, the architecture would reproduce the PR #1964 ^project$ failure mode by construction (day-1 reviewer_security is ADVISORY and cannot block consensus on its own). Non-blocking amendments: - C8: switch fixture from `1964-diff.patch` to a Python-string constant (pr_1964_diff.py) to round-trip through TESTER_ROLE file_access constraints. - C5: prefer mcp__brc__send_heartbeat / mcp__progress__emit over egg-orch message send for fan-out gate logging; specify ≤6 subagent soft cap with grouping rule; add ~5-min per-subagent wall-clock budget; document the MCP-unavailability contingency for decision-2 (3B) so the implementation has a falsifiable probe + fallback rather than a redesign mid-coding. - C9: pair the no-attestation-model positive test with a negative case asserting non-empty attestation from reviewer_security raises a clear ValueError. - task-6 / task-8: bake the cross-partition-pass requirement into acceptance criteria and the regression-replay assert set. - Risks: R-8 mitigation upgraded from 'escalate to follow-up' to 'mandatory parent cross-partition pass'; add R-9 (decision-10 load-bearing assumption + smoke-spike + fallback) and R-10 (fan-out blast radius bounded by ≤6 cap + heartbeats + 5-min budget). - Complexity: keep 'medium' rating but explicitly call out decision-10 viability as the one assumption to validate during implement-phase task-6. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds the two blocking fixes the reviewer flagged on the previous proposal and folds in the non-blocking suggestions: Blocking 1 — TASK-4-1 step 7 now requires a parent-side cross-partition consistency pass (handler ↔ allowlist, route ↔ schema, fixture ↔ Dockerfile/symlink, import-graph cycles) AFTER subagents return and BEFORE verdict emission, with the PR #1964 ^project$ pattern called out by name. TASK-4-3 and TASK-5-2 mode (a) assert the prompt markers; risks section names this as risk-4. Blocking 2 — TASK-4-1 caps fan-out at 6 subagents (groups adjacent tasks if the partition list exceeds the cap) and imposes a 5-minute / 300-second per-subagent wall-clock timeout. Both are asserted by TASK-4-3 and TASK-5-2; risk-5 names them. Non-blocking incorporated: - Pre-implementation Task subagent smoke spike documented in the Approach section - TASK-4-1 step 1 emits an mcp__brc__send_heartbeat with the fan-out gate decision so silent regressions are visible (risk-7) - TASK-4-1 step 4 names an mcp-unavailable fallback path (parent fetches contract and inlines partition specs) (risk-6) - Parallel knob threaded at call site via new reviewer_code_parallel kwarg, not loaded inside _build_review_prompt - TASK-1-3 splits the role-mapping invariant test out into a dedicated test_pipeline_role_to_reviewer_type_mapping.py - Phase 1/2/3 dependency graph relaxed — all three are now parallel-developable - TASK-5-1 fixture caps at 200 KB and documents PR # / commit SHA provenance - TASK-5-2 live-LLM mode pins model alias via EggAgentClient.default_model() (no date-pinned literal) - TASK-2-1 acceptance requires "cross-file allowlist mismatch" and "handler-vs-validator path mismatch" by name in the security-review-criteria.md content (defence in depth alongside the parent post-pass) - TASK-2-2 lens-preamble wording rewritten to "Focus ONLY on the {lens} lens; defer ... to reviewer_code" so the security preamble does not self-contradict ("Do NOT review security") - manual_steps clarifies operator-managed monitoring and notes the absence of an automated dashboard Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds documentation for the issue-1965 reviewer improvements: the `reviewer_code` subagent fan-out block on large implement-phase diffs and the two new ADVISORY lens reviewers (`reviewer_security`, `reviewer_concurrency`). - New `shared/prompts/security-review-criteria.md` and `shared/prompts/concurrency-review-criteria.md`. Both inherit from `code-review-criteria.md` (verbatim header on line 5) and enumerate lens-specific patterns. Security covers cross-file allowlist mismatches, handler-vs-validator path mismatches, info-disclosure / authz bypass, uncommitted-artifact / Dockerfile-symlink mismatches, secret leakage, and cross-file OWASP top-10 patterns. Concurrency covers race conditions, deadlocks, shared-state mutation, async-context leakage, retry storms, resource-cleanup ordering, and BRC-protocol invariants (send-wait ordering, heartbeat-stall windows, fan-out heartbeat propagation). Each file ≤ 150 line target. - `shared/prompts/REVIEWER-SYNC.md` now lists `security` and `concurrency` in the SDLC reviewer-types cell, documents the subagent fan-out and lens-reviewer SDLC-only asymmetries, and adds two modification-checklist rows (one for the fan-out block, one for adding/modifying any lens reviewer) so future PRs touching either surface have a single checklist to follow. - `docs/guides/concurrent-execution.md` adds a one-paragraph subsection "Implement-phase `reviewer_code` Subagent Fan-Out" under the BRC consensus protocol section explaining the gate threshold (>10 files OR >500 LOC), `mcp__sdlc__show_contract` self-fetch, 6-subagent cap, 5-minute timeout, no-recursion ban, parent cross-partition consistency pass, and parallelism knob; and lists the two new ADVISORY lens reviewers in the implement-phase spawned- roles table. - `docs/architecture/orchestrator.md` adds rows for Reviewer (Security) and Reviewer (Concurrency) to the implement-phase roles table with links to their criteria files, and notes the fan-out behavior on Reviewer (Code). Satisfies TASK-2-1 (criteria files), TASK-4-2 (REVIEWER-SYNC update), and TASK-5-3 (concurrent-execution + orchestrator docs). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ges (#1965) TASK-1-1 + TASK-1-2: register the two new ADVISORY lens reviewers in the canonical AgentRole enum and wire them into the implement-phase review graph as ADVISORY edges. Both new roles map to Role.REVIEWER via AGENT_ROLE_TO_CONTRACT_ROLE and appear in _PHASE_REVIEWERS["implement"] alongside reviewer_code / reviewer_contract. ADVISORY-by-default day 1 so they cannot deadlock consensus until severity-tagged NACKs land in #1997. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
TASK-3-1: introduces ReviewerCodeConfig (single field parallel: bool = True), exposes it as an optional reviewer_code field on PhaseConfig, and adds a get_reviewer_code_parallel(contract) accessor that handles every fall-through case (None contract, missing phase_configs, missing implement key, missing reviewer_code field) and defaults to True. Legacy contracts without the new field continue to round-trip through Contract.model_validate_json / model_dump_json without loss. The accessor is consumed by orchestrator/routes/pipelines.py at the reviewer-prompt construction site so the per-pipeline knob is plumbed into _build_review_prompt without the prompt builder having to load the contract itself. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
) TASK-2-2 + TASK-4-1. Lens criteria (TASK-2-2): - _get_security_review_criteria() and _get_concurrency_review_criteria() load shared/prompts/{security,concurrency}-review-criteria.md with inline fallbacks for headless test runs. The header in each shared file is "Inherits from `code-review-criteria.md`; only lens-specific rules below override or extend it." - _get_review_criteria_for_type() and _get_reviewer_scope_preamble() pick up "security" and "concurrency" cases. Each lens preamble scopes the reviewer to its lens AND defers non-lens findings to reviewer_code; we deliberately do NOT phrase the security preamble as "Do NOT review security" because that would be self-contradictory. Subagent fan-out (TASK-4-1): - _build_review_prompt() gains a reviewer_code_parallel kwarg (default True) and emits a "## Subagent Fan-Out Strategy" block ONLY for reviewer_type=="code" AND phase=="implement" (delta reviews skip the block — their git log A..HEAD --not origin/<base> command is small by construction). - Block contents: numstat-based diff sizing + STATUS heartbeat for the gate decision, files>10 OR loc>500 threshold, mcp__sdlc__show_contract partition fetch with empty-list and mcp-unavailable fallbacks, 6-subagent cap with 5-minute / 300-second per-subagent timeout, parent cross-partition consistency pass (handler ↔ allowlist, route ↔ schema, fixture ↔ Dockerfile/symlink, import-graph cycles) before verdict, parallel-vs-sequential per kwarg, and an explicit "subagents must NOT spawn their own subagents" recursion ban. - Call site at _build_agent_prompt loads the contract via egg_contracts.loader.load_contract and reads the get_reviewer_code_parallel accessor; failures fall through to the parallel default so unit tests and contractless invocations keep working. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…1965) reviewer NACK from tester: mypy was failing on the `# type: ignore[union-attr]` at shared/egg_contracts/models.py:368 because mypy infers the type correctly once `phase_configs` has been narrowed by the prior `getattr(..., None)` guard. The tester also noted (non-blocking) that the broad `except Exception` clause could swallow non-AttributeError failures silently; tightened it to `(AttributeError, TypeError)` matching the outer try/except so the fall-through behaviour is predictable. Verified: `mypy shared/egg_contracts/models.py` exits 0, `ruff check` clean, all 67 shared/egg_contracts/tests/ pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two blocking issues from reviewer_code's review of f85dfac: 1. **Wrong contract field name (`files` → `files_affected`).** The prompt's Step 3 instructed reviewers to read `task.files` but the `Task` Pydantic model field is `files_affected`. A reviewer following the prompt literally would have looked for a non-existent JSON key and either fallen back to single-pass review (defeating fan-out) or failed to partition correctly. Step 3 now reads `files_affected` (with a parenthetical noting the legacy `files` key for compatibility) AND adds an explicit per-task fallback: if `files_affected` is empty for a task, group it with an adjacent task or fall back to single-pass review. 2. **Cross-partition pass was gated on "After subagents return"** — meaning the explicit anti-PR-#1964 cross-file consistency check (handler ↔ allowlist, route ↔ schema, fixture ↔ Dockerfile / symlink) silently skipped on the single-pass paths (below-threshold solo, mcp-unavailable fallback, empty-tasks fallback). PR #1964's `^project$` allowlist bypass and `sandbox/scripts/jira` symlink would have slipped through a small PR with the same shape. Lifted the cross-partition pass into its own `## Mandatory Cross-Partition Consistency Pass` subsection that runs in ALL paths (above-threshold fan-out, below-threshold solo, both fallback paths). Fan-out steps 2 and 4 now explicitly reference the mandatory pass. Non-blocking suggestions also addressed: - Replaced "STATUS heartbeat" with "heartbeat (state=WORKING)" terminology (the schema has no STATUS state). - Narrowed the bare `except Exception` at the call site of `load_contract` to `(ImportError, FileNotFoundError, ValueError)` with a `logger.warning` so genuine contract-load failures are observable rather than silently swallowed. Verified all required prompt markers still present (`Subagent Fan-Out Strategy`, `git diff --numstat`, `files_changed > 10`, `500`, `mcp__sdlc__show_contract`, `phases.implement.tasks`, `subagents must NOT spawn their own subagents`, `cross-partition`, `handler`, `allowlist`, `capped at 6`, `5 minutes`, `300 seconds`, `fan-out: enabled`, `fan-out: skipped`, `mcp unavailable`, `no implement tasks`, `files_affected`, `Mandatory Cross-Partition Consistency Pass`), and that `STATUS heartbeat` and `task's \`files\` list` are no longer in the rendered prompt. Block remains correctly absent for non-code reviewer types and non-implement phases. Lint: ruff check + format clean. test_pipeline_prompts.py: 312 passed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Lands tester-owned tasks TASK-1-3, TASK-2-3, TASK-3-2, TASK-4-3,
TASK-5-1, and TASK-5-2 from the implement-phase plan:
- shared/egg_contracts/tests/test_agent_roles.py — TASK-1-3 (b):
reviewer_security / reviewer_concurrency resolve as enum members,
are registered in AGENT_ROLES with the canonical
_REVIEWER_BLOCKED_WRITE list, map to Role.REVIEWER, appear in
_PHASE_REVIEWERS["implement"] / get_roles_for_phase("implement")
for both egg and non-egg repos, and are NOT in EGG_ONLY_REVIEWERS.
- orchestrator/tests/test_review_graph_advisory_reviewers.py
— TASK-1-3 (a): four ADVISORY edges land on the implement graph;
pre-existing CRITICAL edges stay CRITICAL; documenter is NOT
reviewed by either lens; lens roles are absent from plan/refine.
- orchestrator/tests/test_pipeline_role_to_reviewer_type_mapping.py
— TASK-1-3 (c, d) Pitfall-1 + Pitfall-4 guards: the
`replace("reviewer_", "", 1).replace("_", "-")` invariant covers
both new role names, no redundant dict / if-elif chain shadows it,
and REVIEWER_ATTESTATION_MODELS does not register either role.
- orchestrator/tests/test_lens_reviewer_prompts.py — TASK-2-3:
shared-file load + inline-fallback parity for both criteria
loaders; dispatcher routes "security" / "concurrency" to the lens
loaders (NOT to _get_code_review_criteria); preambles are non-
empty, distinct, lens-focused, and free of the self-contradictory
"Do NOT review {security|concurrency}" phrasing; end-to-end
_build_review_prompt(reviewer_type=...) embeds the lens criteria.
- shared/egg_contracts/tests/test_phase_config_reviewer_code.py
— TASK-3-2: ReviewerCodeConfig defaults / explicit / dict
coercion; Contract round-trip with both True and False; legacy
contract round-trip without the new field; get_reviewer_code_parallel
for every fall-through path (None contract, None phase_configs,
missing implement, None reviewer_code).
- orchestrator/tests/test_reviewer_code_fan_out_prompt.py
— TASK-4-3: every fan-out marker (numstat, files_changed > 10,
500, mcp__sdlc__show_contract, phases.implement.tasks, both
fallbacks, 6-cap, 5-min/300-sec timeout, recursion ban, parent
cross-partition pass with handler/allowlist markers, STATUS
heartbeat) and the reviewer_code_parallel kwarg switching the
prompt between in-parallel and sequentially. Block correctly
absent for non-code reviewer types and non-implement phases.
- integration_tests/sdlc/test_reviewer_1964_regression.py
— TASK-5-1 + TASK-5-2 (combined): inlined PR_1964_DIFF fixture
with both motivating bug surfaces (sandbox/scripts/jira symlink,
^project$ allowlist bypass), synthesize_diff() helper with input
validation, two-mode regression test (always-on prompt-asserts
parametrized over reviewer_code_parallel True/False; opt-in
live-LLM replay gated by RUN_REVIEWER_REPLAY=1, model alias
resolved via egg_agent.client.DEFAULT_MODEL at test-collection
time so the live test cannot drift from production).
Note: a separate `integration_tests/sdlc/fixtures/pr_1964_diff.py`
module would have been cleaner but the tester role's gateway-
allowed write patterns (shared/egg_restrictions/patterns.py) cover
only test-named files, not arbitrary `.py` files under
`integration_tests/`. Inlining the fixture keeps the same
regression coverage without crossing the role boundary.
`make lint` exits 0 (Ruff check + format check + mypy +
shellcheck + yamllint + hadolint + custom checks). 506 unit and
integration prompt-assert tests pass; the 1 skip is the live-LLM
replay (RUN_REVIEWER_REPLAY not set).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Contributor
Author
Autofix tracking{"Test/Unit Tests": 2} |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…viewer_concurrency Update test_egg_restrictions.py to expect 18 roles (was 16) and include the new REVIEWER_SECURITY/REVIEWER_CONCURRENCY constants that were registered in the gateway's AGENT_PATTERNS map. Update test_full_implement_graph in test_peer_consensus_integration.py to ACK from the new advisory lens reviewers before they confirm — get_default_implement_graph() now wires reviewer_security and reviewer_concurrency as ADVISORY edges to coder and tester, and the must_have_reviewed guard requires every registered reviewer to have ACKed before confirming.
This comment has been minimized.
This comment has been minimized.
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No agent-mode design concerns.
Quick notes on what was checked and looks good:
- Specialized lens reviewers (
reviewer_security,reviewer_concurrency) match the design doc's endorsement of "one generalized review bot for broad coverage, plus specialized bots for specific concerns". The lens preambles correctly tell the agent "If the diff has no concerns, a brief approval is acceptable — verbose reports without findings are not required" rather than forcing structured output. - No diff pre-fetching. The fan-out block has the reviewer self-gate by running
git diff --numstat <base>...HEADitself rather than baking diff metadata into the prompt. The_build_review_promptbuilder doesn't compute the diff size on the orchestrator side — that's exactly the right split. - Subagents are an in-agent decision. Fan-out happens via Claude Agent SDK
Tasksubagents inside the sandbox, not via orchestrator-spawned containers parsing JSON. The parent agent emits one ACK/NACK based on subagent findings — no post-processing pipeline. - Model alias used in
_resolve_reviewer_model()— readsegg_agent.client.DEFAULT_MODELinstead of pinning a date-stamped identifier, so EGG201 stays clean. - No direct API calls. New code paths route through the Agent SDK (
run_agentin the regression test, the existing_build_agent_promptflow elsewhere).
The fan-out block is procedurally dense (9 numbered steps with specific thresholds, caps, and heartbeat strings), but this is defensible as specialized-lens domain knowledge motivated by concrete cross-file failure modes (PR #1964's ^project$ and sandbox/scripts/jira bugs). The design guide explicitly notes "specialized workflows benefit from focused guidelines".
— Authored by egg
Contributor
Author
| egg is addressing review feedback... |
Contributor
Author
|
No actionable feedback in the latest re-review pass on
The three non-blocking observations from the prior review ( — Authored by egg |
Contributor
Author
|
egg feedback addressed. View run logs 16 previous review(s) hidden. |
This was referenced Apr 25, 2026
Closed
Closed
jwbron
approved these changes
Apr 25, 2026
3 tasks
jwbron
added a commit
that referenced
this pull request
Apr 25, 2026
The sandbox container pre-installs pytest, ruff, mypy, and the rest of the dev tools globally (sandbox/Dockerfile lines 156-178), but never installs uv. The Makefile comment at lines 12-13 already documents this intent: "CI uses venv (via uv sync); the sandbox has tools installed globally." However, `test:` and `test-all:` were declared with `venv` as a prerequisite, and `venv:` hard-errors when uv is missing — so the BRC tester running inside the sandbox could never invoke `make test` and silently fell back to ad-hoc pytest invocations that miss half of pyproject.toml's testpaths (PR #2061 / CI run 24937921821). Add a `sync-venv-if-uv` target that delegates to `venv` only when uv is on PATH, and depend on it from `test:` and `test-all:` instead. Dev machines and CI continue to get the same `uv sync --extra dev` behavior; the sandbox now falls through to the existing PYTEST/PYTHON system-PATH fallbacks already wired into the Makefile. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 25, 2026
james-in-a-box Bot
pushed a commit
that referenced
this pull request
Apr 25, 2026
Main added REVIEWER_SECURITY and REVIEWER_CONCURRENCY to the local AgentRole mirror in shared/egg_restrictions/patterns.py (PR #2061). This PR deletes that mirror entirely and re-exports the canonical StrEnum from egg_contracts — which already carries those new roles — so the conflict resolves to HEAD's version. The added roles still propagate, just through the canonical enum instead of the doomed copy.
jwbron
added a commit
that referenced
this pull request
Apr 25, 2026
…producers (#2077) * Fix #2064: reject wait_loop on CONSENSUS_CONFIRMED for non-confirmed producers A producer's own consensus_confirmed is part of what generates the global CONSENSUS_CONFIRMED signal — waiting on it before the producer has reached CONFIRMED state is a circular dependency that deadlocks the pipeline until the overseer's heartbeat-stall band-aid eventually intervenes. Observed in pipeline issue-1965 (PR #2061): the documenter proposed, called consensus confirmed but got status='pending_acks' (because coder/tester hadn't yet proposed and the global_zero_proposal guard fired), then entered the post-confirm STAY ALIVE wait_loop on CONSENSUS_CONFIRMED as if confirm had succeeded. The agent sat there for ~36 minutes waking briefly on each peer's confirm but never re-attempting its own. Only the agent-heartbeat-stall OVERSEER_ALERT broke it out. This change adds a server-side guard in /messages/wait that returns HTTP 400 with an actionable error when the caller is a producer in WORKING/PROPOSED state and CONSENSUS_CONFIRMED appears in for_types. The error tells the agent to call mcp__brc__confirm and, if it returns pending_acks, to wait on the prerequisite events instead (CONSENSUS_PROPOSE, CONSENSUS_ACK, CONSENSUS_RE_REVIEW) before retrying confirm. The fix is structural — matching the project's preference for infrastructure constraints over prompt-based rules — and applies generically to every producer role (coder, tester, documenter, and any future producer), not just the documenter case that surfaced it. - orchestrator/peer_consensus.py: add is_producer_pending_confirm helper - orchestrator/routes/messages.py: add guard at the wait endpoint - orchestrator/tests/test_messages.py: 7 cases covering the documenter scenario, dual-role agents, reviewer-only roles, missing tracker, missing role, and other for_types passthrough - orchestrator/tests/test_peer_consensus_integration.py: 5 cases for the helper itself across the producer state machine - docs/reference/agent-wait-patterns.md: anti-pattern 5 documenting the deadlock and the recovery idiom Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address review feedback on PR #2077 - Drop misleading 'reviewer must confirm before producers' comment in test_producer_in_confirmed_is_not_pending and remove the unnecessary reviewer handle_confirmed call. check_confirm_guard only gates a producer's confirm on global_zero_proposal + producer_not_fully_acked, not on reviewer confirms. - Add test_dual_role_tester_in_proposed_blocked locking the helper's contract for the tester (dual-role) case the implement_tracker fixture exercises. - Simplify the peer_consensus import-shim in _check_producer_pending_confirm_guard to match the one-tier pattern already used by _apply_delphi_filter — drops the unused package-relative fallback. - Add an inline comment documenting the intentional broadness of the guard wrt the from_role query parameter. * Address second-pass review feedback on PR #2077 Three non-blocking suggestions from the re-review of commit 31d32db: 1. Drop the redundant advisory ACK and stale comment in test_producer_in_confirmed_is_not_pending — documenter has zero critical reviewers in this fixture, so it is already fully ACKed without the reviewer_code/documenter advisory ACK, and the 'must have reviewed' guard the comment cited only applies when the reviewer (not the producer) tries to confirm. 2. Add ReviewEdge(tester, coder, CRITICAL) to implement_tracker so the fixture genuinely makes tester dual-role (producer of its own artifacts + reviewer of coder, matching get_default_implement_graph). test_dual_role_tester_in_proposed_blocked now locks the dual-role contract instead of just tester-as-producer, and the docstring is updated to reflect that. All other tests using the fixture continue to pass — the new edge only activates if coder confirms (it doesn't in any of these tests). 3. Move the from_role-broadness comment from the body of _check_producer_pending_confirm_guard up into its docstring so future readers see the policy when they look at the function contract, not when they wonder why the import shim is shaped the way it is. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
jwbron
added a commit
that referenced
this pull request
Apr 25, 2026
…2082) * Fix #2066: collapse duplicate AgentRole into single source of truth shared/egg_restrictions/patterns.py defined its own AgentRole class that mirrored egg_contracts.agent_roles.AgentRole, with a comment asking readers to "keep values in sync" — enforced only by unit tests. PR #2061 surfaced the failure mode: it added REVIEWER_SECURITY and REVIEWER_CONCURRENCY to the canonical enum but not the gateway- side mirror, breaking CI. Replace the duplicate with a re-export of the canonical StrEnum so new roles propagate automatically. Trim the now-tautological cross- sync tests to a single identity assertion as a tripwire against any future re-introduction of a parallel enum. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Declare egg-contracts dep in egg-shared distribution egg_restrictions.patterns now imports AgentRole from egg_contracts. The egg-shared distribution didn't declare the dependency, so a standalone 'pip install egg-shared' would ImportError on first use of egg_restrictions.patterns. Monorepo install is unaffected (root pyproject.toml installs both packages together), so this is hygiene rather than a live regression. Addresses non-blocking observation #1 from PR #2082 review. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
jwbron
added a commit
that referenced
this pull request
Apr 25, 2026
…2085) * Fix #2078: gate ready-to-confirm nudge on full check_confirm_guard The orchestrator's "ready to confirm" STATUS used to fire on ``is_fully_acked``, which only checks critical reviewers and ignores the global zero-proposal guard. An advisory-only producer like ``documenter`` (whose sole edge is ``reviewer_code → documenter, ADVISORY``) was nudged to confirm the moment that one ADVISORY ACK landed, even though peer producers had not yet proposed — so ``handle_confirmed`` rejected the resulting confirm with ``pending_acks``. See the issue for the recorded reproducer in pipeline ``issue-1965`` (PR #2061) at 18:04:22–18:04:35. Single source of truth: ``PeerConsensusTracker.is_ready_to_confirm`` delegates to ``check_confirm_guard``. ``_collect_newly_ready_producers`` sweeps after every state-changing handler (PROPOSE, RE_PROPOSE, ACK) and returns producers whose readiness transitioned false→true, deduped by ``(role, proposal_version)`` so a re-propose naturally re-arms the nudge. ``signals.py`` emits a STATUS for each newly-ready producer the tracker reports. Sweeping on PROPOSE handles the case the original ACK-only gate missed: a producer that becomes ready *because a peer finally proposed* and unblocked the global guard. Independent of the wait_loop fix in #2077. * Address #2078 review feedback: rollback memo on send failure Six non-blocking observations from the egg-reviewer pass: 1. Roll back the per-version nudge memo when add_message raises so the producer can be re-nudged on the next state change instead of being permanently silenced at that proposal version. Adds PeerConsensusTracker.release_nudge() and wires it into _emit_ready_to_confirm_nudges via an optional tracker argument. 2. handle_consensus_producer_push_signal now also calls _emit_ready_to_confirm_nudges for symmetry with the explicit propose/re-propose handlers. The omission was benign today but would silently regress if a future guard depended on peer versions. 3. Drop is_ready_to_confirm — it had no production caller and the test assertions it backed were redundant with the existing newly_ready checks. 4. Document that _nudged_versions is in-memory only by design and a restart-time duplicate nudge is harmless under check_confirm_guard. 5. Reword the nudge body to "ready to confirm — all blocking reviews are clear" so an operator reading the bus is not misled by "has been ACKed" when the producer is advisory-only (documenter). 6. Use distinct commit SHAs across versions in test_re_propose_re_arms_nudge so the test mirrors a real auto-repropose (which short-circuits on unchanged SHA). --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
This was referenced Apr 26, 2026
4 tasks
james-in-a-box Bot
pushed a commit
that referenced
this pull request
Apr 26, 2026
Address the four issues raised by egg-reviewer on PR #2130: 1. (Blocking) Add REVIEWER_CODE_HOLISTIC to _ROLES_WITHOUT_WORKTREE in kubernetes_spawner.py — it reviews via the BRC bus and never writes code, so spawning it with repos=[] must succeed and a spawn with repos must not provision a per-agent worktree. Mirror the existing regression test for the lens reviewers (#2061). 2. Differentiate procedural-step 2 and 8 by lens. The unified block told reviewer_code_holistic to "review every changed file systematically", directly contradicting the holistic criteria's "don't verify every line; the fan-out reviewer covers that". Step 2 now skims the diff for the holistic lens and step 8 names the four mandatory passes. 3. Add an explicit guard test that reviewer_code_holistic has no attestation model — matches the lens-reviewer pattern and prevents a future contributor from silently pinning ReviewerCodeAttestation onto the holistic role (whose ACK shape is intentionally different). 4. Add reviewer_code_holistic to COMPOSITE_REVIEWER_ROLES in shared/egg_contracts/checkpoint_cli.py so operators can filter `egg-checkpoint --agent-type reviewer_code_holistic`. Authored-by: egg
jwbron
added a commit
that referenced
this pull request
Apr 26, 2026
…2130) * Add reviewer_code_holistic CRITICAL role alongside fan-out (#2126) Restores the always-on, single-pass holistic generalist that fan-out + slice subagents replaced in #2061. Addresses postmortem #2125: PR #2105 shipped with the in-pipeline BRC reviewers at full ACK consensus while GHA's egg-reviewer caught 14 architectural defects no slice owned (canonical example: the `__checkout__` synthetic-key dead-end that broke the PR's primary advertised use case end-to-end). reviewer_code_holistic runs every implement pipeline regardless of PR size, with a preamble framed as complementary to reviewer_code's fan-out work — end-to-end use-case walk, doc↔code symmetry, synthetic-key cross-module audit, silent-fallback hunt. Both reviewers feed consensus as distinct CRITICAL roles so a holistic NACK on architectural coherence is not averaged against six fan-out subagent ACKs on slice-correctness. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Fix checks: register reviewer_code_holistic in patterns + role/test counts - Add REVIEWER_CODE_HOLISTIC_PATTERNS to AGENT_PATTERNS registry (mirrors REVIEWER_CODE scope) so the gateway honors the new role. - Bump role-count assertions (18 → 19) and add the holistic role to enumerated lists in shared and orchestrator role tests. - Include reviewer_code_holistic in the short-flow default roles set so the test reflects the updated implement roster. - Add reviewer_code_holistic ACKs / re-ACKs / confirm in the full implement-graph integration test now that it is a CRITICAL edge. * Fix checks: apply automated formatting fixes * Address PR #2130 review: holistic reviewer integration gaps Address the four issues raised by egg-reviewer on PR #2130: 1. (Blocking) Add REVIEWER_CODE_HOLISTIC to _ROLES_WITHOUT_WORKTREE in kubernetes_spawner.py — it reviews via the BRC bus and never writes code, so spawning it with repos=[] must succeed and a spawn with repos must not provision a per-agent worktree. Mirror the existing regression test for the lens reviewers (#2061). 2. Differentiate procedural-step 2 and 8 by lens. The unified block told reviewer_code_holistic to "review every changed file systematically", directly contradicting the holistic criteria's "don't verify every line; the fan-out reviewer covers that". Step 2 now skims the diff for the holistic lens and step 8 names the four mandatory passes. 3. Add an explicit guard test that reviewer_code_holistic has no attestation model — matches the lens-reviewer pattern and prevents a future contributor from silently pinning ReviewerCodeAttestation onto the holistic role (whose ACK shape is intentionally different). 4. Add reviewer_code_holistic to COMPOSITE_REVIEWER_ROLES in shared/egg_contracts/checkpoint_cli.py so operators can filter `egg-checkpoint --agent-type reviewer_code_holistic`. Authored-by: egg --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: egg <egg@localhost> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
6 tasks
jwbron
added a commit
that referenced
this pull request
Apr 27, 2026
…iewers to CRITICAL (#2139) (#2152) * Reviewer roster reshape: drop reviewer_code fan-out, promote lens reviewers to CRITICAL (#2139) Two changes bundled because they touch the same surface area (`agent_roles.py`, `review_graph.py`, reviewer prompt scoping): 1. **Remove `reviewer_code` subagent fan-out** (the bulk of #2061). Slice scheduler #2137 will size implement-phase slices small enough that fan-out is obsolete; the cleanup is independent of #2137's runtime work. Clean tear-out — no salvage of `ReviewerCodeConfig.parallel`, no partition cross-pass. - Drop `ReviewerCodeConfig`, `PhaseConfig.reviewer_code`, `get_reviewer_code_parallel()` from `shared/egg_contracts/models.py`. - Drop the gated 9-step "Subagent Fan-Out Strategy" block and the "Mandatory Cross-Partition Consistency Pass" from `_build_review_prompt()` in `orchestrator/routes/pipelines.py`, plus the `reviewer_code_parallel` kwarg and resolver call site. - Delete `test_reviewer_code_fan_out_prompt.py`, `test_phase_config_reviewer_code.py`, and `test_reviewer_1964_regression.py`. 2. **Promote `reviewer_security` and `reviewer_concurrency` from ADVISORY to CRITICAL** in `get_default_implement_graph()`. A NACK from either lens now blocks consensus until the producer re-proposes. Closes #1997 (severity-tagged NACK signalling — answered by promoting lenses to CRITICAL). `reviewer_code_holistic` (added in #2126) stays as the always-on holistic CRITICAL reviewer; the prompt scoping wording that referenced "fan-out reviewer" / "slice ACKs" is updated throughout to refer to `reviewer_code`'s line-by-line review. Closes #2139. Closes #2127. Closes #1997. Closes the fan-out portions of #1965 and #2067. Subsumes #2029. * Address review: flip lens preambles to CRITICAL, drop stale slice wording Two blocking drift bugs from the ADVISORY → CRITICAL lens promotion: 1. `_get_reviewer_scope_preamble` in orchestrator/routes/pipelines.py still told security/concurrency lens reviewers their review was ADVISORY. This is the first runtime framing the agent reads, and it contradicted the review-graph edges (now CRITICAL) and the criteria docs (already updated). Flipped both branches to CRITICAL with a #2139 reference; added regression tests pinning the new wording so future prompt edits cannot quietly revert it. 2. docs/guides/concurrent-execution.md role table line 45 still tagged the lens reviewers as ADVISORY. Dropped the qualifier so it matches the same file's line 358 prose and the rest of the docs tree. Plus two non-blocking cleanups flagged in the same review: the holistic reviewer's prep instructions and the procedural-step comment both still referenced "slice work" / "slice criteria" — replaced with "line-by-line work" and "code-review criteria" for consistency with the rest of the PR. Also flipped the two stale ADVISORY labels in shared/prompts/REVIEWER-SYNC.md (Reviewer types row + criteria bullets). --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
james-in-a-box Bot
pushed a commit
that referenced
this pull request
Apr 28, 2026
…iewers to CRITICAL (#2139) (#2152) * Reviewer roster reshape: drop reviewer_code fan-out, promote lens reviewers to CRITICAL (#2139) Two changes bundled because they touch the same surface area (`agent_roles.py`, `review_graph.py`, reviewer prompt scoping): 1. **Remove `reviewer_code` subagent fan-out** (the bulk of #2061). Slice scheduler #2137 will size implement-phase slices small enough that fan-out is obsolete; the cleanup is independent of #2137's runtime work. Clean tear-out — no salvage of `ReviewerCodeConfig.parallel`, no partition cross-pass. - Drop `ReviewerCodeConfig`, `PhaseConfig.reviewer_code`, `get_reviewer_code_parallel()` from `shared/egg_contracts/models.py`. - Drop the gated 9-step "Subagent Fan-Out Strategy" block and the "Mandatory Cross-Partition Consistency Pass" from `_build_review_prompt()` in `orchestrator/routes/pipelines.py`, plus the `reviewer_code_parallel` kwarg and resolver call site. - Delete `test_reviewer_code_fan_out_prompt.py`, `test_phase_config_reviewer_code.py`, and `test_reviewer_1964_regression.py`. 2. **Promote `reviewer_security` and `reviewer_concurrency` from ADVISORY to CRITICAL** in `get_default_implement_graph()`. A NACK from either lens now blocks consensus until the producer re-proposes. Closes #1997 (severity-tagged NACK signalling — answered by promoting lenses to CRITICAL). `reviewer_code_holistic` (added in #2126) stays as the always-on holistic CRITICAL reviewer; the prompt scoping wording that referenced "fan-out reviewer" / "slice ACKs" is updated throughout to refer to `reviewer_code`'s line-by-line review. Closes #2139. Closes #2127. Closes #1997. Closes the fan-out portions of #1965 and #2067. Subsumes #2029. * Address review: flip lens preambles to CRITICAL, drop stale slice wording Two blocking drift bugs from the ADVISORY → CRITICAL lens promotion: 1. `_get_reviewer_scope_preamble` in orchestrator/routes/pipelines.py still told security/concurrency lens reviewers their review was ADVISORY. This is the first runtime framing the agent reads, and it contradicted the review-graph edges (now CRITICAL) and the criteria docs (already updated). Flipped both branches to CRITICAL with a #2139 reference; added regression tests pinning the new wording so future prompt edits cannot quietly revert it. 2. docs/guides/concurrent-execution.md role table line 45 still tagged the lens reviewers as ADVISORY. Dropped the qualifier so it matches the same file's line 358 prose and the rest of the docs tree. Plus two non-blocking cleanups flagged in the same review: the holistic reviewer's prep instructions and the procedural-step comment both still referenced "slice work" / "slice criteria" — replaced with "line-by-line work" and "code-review criteria" for consistency with the rest of the PR. Also flipped the two stale ADVISORY labels in shared/prompts/REVIEWER-SYNC.md (Reviewer types row + criteria bullets). --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The pipeline's BRC
reviewer_codehas been confirming consensus onlarge diffs while the downstream GHA
egg-reviewerthen catchesblocking issues that should have been found in-pipeline. PR #1964
shipped a broken
sandbox/scripts/jirasymlink and a^project$info-disclosure bypass — both visible cross-file mismatches that
reviewer_codemissed. This PR closes that gap.Key changes:
reviewer_codesubagent fan-out (issue A). When a diffis large enough (files > 10 OR loc > 500),
reviewer_codenow spawns Claude Agent SDK
Tasksubagents — partitionedalong the implement-phase task list pulled from
mcp__sdlc__show_contract— so every changed file is readcarefully. Fan-out is capped at 6 subagents (with a 5-minute
per-subagent wall-clock timeout), each subagent reviews only
its path-glob slice and explicitly cannot spawn its own
subagents, and the parent reviewer runs a cross-partition
consistency pass (handler ↔ allowlist, route ↔ schema,
fixture ↔ Dockerfile/symlink, import-graph cycles) before
emitting the verdict — closing the cross-file blind spot
that let PR Add Jira gateway wrapper with credential injection (v1 read-only) #1964's
^project$allowlist bypass slipthrough. Below the threshold, on empty implement-phase task
lists, or when
mcp__sdlc__show_contractis unreachablefrom the subagent, the reviewer falls back to its current
single-pass behaviour with a telemetry heartbeat. Parallelism
is configurable per pipeline via the new
phase_configs.implement.reviewer_code.parallelfield onthe contract (default
true).reviewer_securityandreviewer_concurrencyroles shipwith dedicated
shared/prompts/security-review-criteria.mdand
concurrency-review-criteria.md(each inheriting fromcode-review-criteria.md). The security lens enumeratescross-file allowlist mismatches and handler-vs-validator
path mismatches by name. Both reviewers enter as ADVISORY
edges in
get_default_implement_graph()so they cannotdeadlock consensus on day 1; promotion to CRITICAL waits
for Specialised reviewer findings need severity-based flagging in BRC consensus #1997's severity-tagged NACK signalling.
fixture (Python string constant;
.patchis not in thetester's gateway-allowed paths) and a two-mode replay test
(always-on prompt-text asserts + opt-in live-LLM replay
gated by
RUN_REVIEWER_REPLAY=1, with the model aliaspinned to production via the module-level
DEFAULT_MODELconstant inshared/egg_agent/client.py)keep the two motivating bugs detectable on every CI run.
Impact: pipeline-produced PRs should converge toward zero
blocking GHA-reviewer findings over the next handful of issues,
since the BRC reviewer now reads large diffs at the subagent
granularity, performs an explicit parent-side cross-file pass,
and runs them through three lenses (general, security,
concurrency). No existing pipeline behaviour changes for diffs
below the threshold or for contracts without the new
phase_configsfield.Test Plan
_PHASE_REVIEWERSextension; new ADVISORY edges inget_default_implement_graph();replace("reviewer_", "").replace("_", "-")mappinginvariant for both new role names (in dedicated
test_pipeline_role_to_reviewer_type_mapping.py); absenceof attestation models for both new roles;
criteria-loader shared-file + inline-fallback paths;
dispatcher and scope-preamble dispatch; lens preambles do
NOT contain the self-contradictory phrasing
"Do NOT review {security|concurrency}";
PhaseConfig.reviewer_code.paralleldefault / override /unset round-trip and the
get_reviewer_code_parallel()accessor.
_build_review_prompt(reviewer_type="code", phase="implement", ...)contains the fan-out block (numstat command, threshold
rule, contract-fetch instruction with empty-list and
mcp-unavailable fallbacks, partition-by-path-glob
instruction, 6-subagent cap, 5-minute / 300-second timeout,
parallel-vs-sequential per
reviewer_code_parallelkwarg,no-recursion clause, parent cross-partition consistency
pass with
handler/allowlistmarkers,STATUS-heartbeat instrumentation); block absent for
non-code reviewer types and non-implement phases.
test_reviewer_1964_regression.pyasserts the same set ofmarkers for a 12-file / 800-LOC fixture and asserts they
are absent for a 3-file / 50-LOC fixture, both
TrueandFalseparallelism paths exercised viapytest.mark.parametrize.RUN_REVIEWER_REPLAY=1 pytest integration_tests/sdlc/test_reviewer_1964_regression.pyruns the actual reviewer prompt (model alias pinned to
production via the module-level
DEFAULT_MODELconstant inshared/egg_agent/client.py) againstthe cached PR Add Jira gateway wrapper with credential injection (v1 read-only) #1964 patch and asserts the review mentions
both
sandbox/scripts/jira(uncommitted file) and^project$(allowlist bypass).trigger an SDLC pipeline on a < 10-file issue and a
Manual Steps
Pre-merge: none — new ADVISORY edges cannot deadlock existing
pipelines, criteria files have inline fallbacks, and the
phase_configsknob is fully optional.Post-merge (operator-managed; no automated dashboard yet): the
issue's workflow owner monitors the GHA blocking-issue trend on
the next 5–10 pipeline-produced PRs (decision-9 acceptance is
trend-based) by spot-checking pipeline-produced PRs and reading
the GHA
egg-reviewercomments. If the blocking-issue countdoes not decline, open a follow-up issue revisiting partition
strategy and threshold tuning. A future improvement can wire
this into a structured dashboard, but day-1 monitoring is
operator eyes on the next handful of merged PRs.
Pipeline Context
Pipeline:
issue-1965Issue: #1965
Per-phase BRC transcripts:
refine,plan,implement.Authored-by: egg