Fix #2809: architect owns slice composition; risk_analyst becomes dual-role plan reviewer

[jwbron]

Summary

Restructures the plan-phase BRC graph per #2809: composition
becomes a single role's job (architect), risk_analyst joins
reviewer_plan as a critical reviewer, and reviewer_plan can
now hard-NACK oversized slices on a new judgment-based
slice_size rubric.

Review graph (get_default_plan_graph)

• New risk_analyst → architect CRITICAL edge
• New risk_analyst → task_planner CRITICAL edge
• risk_analyst becomes dual-role automatically via the existing
  is_producer and is_reviewer derivation, picking up the
  CONSENSUS_PROPOSE-augmented producer waits and the Dual-Role
  Execution Order banner for free (mirrors the implement-phase
  tester pattern from #2749)

Prompts

• architect — new "Slice composition authority (Plan phase: architect owns slice composition; risk_analyst becomes dual-role reviewer; reviewer_plan NACKs oversized slices #2809)"
  section; emits architect-slices.yaml scaffold alongside the
  existing analysis JSON; file restrictions extended
• task_planner — drops "Slice shape is your call" and the
  "soft, advisory only ... never NACKs on size" guidance; now
  copies the architect scaffold verbatim and raises slicing
  problems as NACK pressure on architect rather than silently
  re-shaping
• risk_analyst — declared dual-role with documented verdict
  shape; reviewer-role text explains when to NACK on the risk
  lens vs ACK with forward-looking register entries
• single-agent plan prompt — slice-sizing wording aligned to
  the new hard-NACK contract (the planner is the architect in
  that mode, so "Slice shape is your call" stays)

Reviewer rubric

• _get_plan_review_criteria gains §11 Slice Sizing (hard NACK,
  judgment-based) with the four canonical sub-division triggers
  and worked NACK examples
• _build_reviewer_preparation(reviewer_plan, plan) replaces the
  "advisory only — never NACK" guidance with the new judgment-
  based NACK directive that points at §11

Tests

• test_plan_graph_has_correct_edges asserts the two new
  CRITICAL edges and the dual-role flag on risk_analyst
• New TestPlanPhaseArchitectOwnsComposition covers: architect
  authority, scaffold path + schema, file restrictions,
  task_planner verbatim consumption, NACK-pressure escape valve,
  risk_analyst dual-role + verdict shape, §11 rubric, and
  reviewer_plan prep wording
• test_dual_role_banner_present_for_risk_analyst pins the BRC
  preamble derivation
• Updated TestPlannerPromptTreatsRefinerSlicingAsAdvisory so
  the concurrent path asserts the architect-owns-composition
  contract instead of the old "Slice shape is your call" framing

Test plan

• make lint (ruff + ruff format + extras) passes
• CI runs the full make test-all suite
• Manual: re-run #2777's plan phase against this branch to
  verify the architect now NACK-rejects the original
  multi-category bundling instead of letting it through

Closes #2809.

[egg-reviewer]

Review of #2821 — architect ownership of slice composition + risk_analyst dual-role.

Verdict

Core change is correct. The graph edge additions auto-derive risk_analyst as dual-role through the existing is_producer and is_reviewer predicate; the dual-role banner and wait-loop augmentations fire from that derivation without further wiring. Approval matrix (seed_auto_ack_for_empty_pure_producers skips dual-role) and check_confirm_guard (applies both producer and reviewer guards) both handle the new dual-role agent correctly. §11 rubric is well-scoped — judgment-based with concrete NACK patterns and an architect-routed re-propose path. Tests cover the new graph edges, the dual-role banner, the architect scaffold contract, the §11 rubric presence, and the reviewer-prep wording inversion.

Three non-blocking issues below.

Findings

1. Documentation drift in docs/architecture/slice-dag.md (non-blocking)

docs/architecture/slice-dag.md:625-654 still describes slice-sizing as advisory-only, anchored to HITL decision-6 opt-2:

Slice-sizing guidance (advisory only, never enforced; HITL decision-6 opt-2)
Slice-sizing advisory (advisory only, NEVER NACK) ... promoting the advisory to a hard NACK requires a fresh HITL revision.

This PR is exactly that promotion. The doc now contradicts the contract: §11 in _get_plan_review_criteria() makes slice-sizing a hard NACK rubric, the reviewer-prep wording in _build_reviewer_preparation instructs reviewer_plan to NACK on slice sizing, and the architect prompt asserts sole authority over slice composition. The architecture doc should be updated in the same PR — agents reading slice-dag.md to understand the contract will get the wrong story.

Suggested edits:

• §"Planner & plan-reviewer prompt updates" → reframe item 1 under planner as "architect owns slice composition, task_planner copies the scaffold verbatim"; remove "advisory only, never enforced" framing.
• §"Plan reviewer" item 2 → replace the "NEVER NACK" advisory text with the new §11 hard-NACK rubric pointer (cite #2809 alongside the existing HITL decision-6 reference; note the inversion).

2. Documentation drift in docs/reference/agent-roles.md (non-blocking)

docs/reference/agent-roles.md:28-29 doesn't reflect risk_analyst's new dual-role status:

• Line 28: risk_analyst row only mentions Analysis category — it's now also a REVIEW-category agent in plan phase (mirrors how tester is documented as dual-role in implement).
• Line 29: reviewer_plan "Depends On" lists only task_planner, risk_analyst — should also include architect (architect is now a reviewed producer per the new CRITICAL edges).
• Line 111-120: risk_analyst section under "Plan Phase" describes only its producer role; add a "Reviewer role" subsection paralleling how the architect/task_planner descriptions handle the new contract.

3. _synthesize_plan_draft fallback ignores the new scaffold (non-blocking)

orchestrator/routes/pipelines.py:18851-18854 — the fallback synthesizer's agent_files list still only enumerates:

agent_files = [
    ("architect-output.json", "Architecture Analysis"),
    ("risk_analyst-output.json", "Risk Assessment"),
]

This path fires only when task_planner fails to write {id}-plan.md directly (the comment at 18820-18824 makes this explicit), so it's an edge-case recovery — but in that path the architect's architect-slices.yaml scaffold (which is now the binding source of slice DAG shape) is silently dropped from the synthesized draft. Either:

• Append ("architect-slices.yaml", "Slice Scaffold") to agent_files (note: it's YAML not JSON; the existing json.loads branch will fall through to raw text, which is acceptable since the file is small and human-readable), or
• Document that the fallback is best-effort and the scaffold is recoverable from agent-outputs/ directly.

The synthesized draft is read by _populate_contract_from_plan() for the HITL gate, so losing the slice DAG in the fallback path means the operator sees an incomplete plan in a recovery scenario.

What I verified end-to-end

• risk_analyst resolves to dual-role via existing is_dual_role derivation in concurrent_executor.py once both new CRITICAL edges are present.
• _brc_preconfirm_wait_line and _brc_stay_alive_wait_line augmentations fire on dual-role producers; no additional wiring needed.
• Approval matrix skips seeding auto-ACK for dual-role producers (seed_auto_ack_for_empty_pure_producers) — risk_analyst will not get a phantom ACK from itself.
• check_confirm_guard applies producer + reviewer guards in sequence for dual-role agents.
• Gateway file restrictions: ARCHITECT_ROLE.allowed_write includes .egg-state/agent-outputs/, which covers the new architect-slices.yaml write path.
• Test coverage matches the contract: test_dual_role_banner_present_for_risk_analyst, TestPlanPhaseArchitectOwnsComposition (10 tests), and the updated test_plan_graph_has_correct_edges.

— Authored by egg

[james-in-a-box]

Addressed all three non-blocking findings in this PR — no follow-up issues opened (none required HITL input).

Per-item disposition

1. Documentation drift in docs/architecture/slice-dag.md — fixed-in-PR (commit e75ce78)

Replaced the "Planner & plan-reviewer prompt updates" section's "advisory only, never enforced" framing with the new contract:

• New Architect bullet declaring sole authority for slice composition (Plan phase: architect owns slice composition; risk_analyst becomes dual-role reviewer; reviewer_plan NACKs oversized slices #2809) and the binding architect-slices.yaml scaffold output.
• Planner bullets now describe verbatim consumption of the architect scaffold, with the "raise NACK pressure on architect" escape valve replacing local re-shaping.
• Plan reviewer bullets now point at the §11 hard-NACK rubric, cite Plan phase: architect owns slice composition; risk_analyst becomes dual-role reviewer; reviewer_plan NACKs oversized slices #2809 alongside HITL decision-6 (noting the inversion), and explain the seam-naming NACK contract. Refiner/operator override is documented for the deliberate-large-slice case (e.g. atomic schema migration).

2. Documentation drift in docs/reference/agent-roles.md — fixed-in-PR (commit e75ce78)

Three updates to the Plan Phase docs:

• Role Overview table: risk_analyst's Category column now reads Analysis (dual-role: also reviews architect + task_planner); reviewer_plan's Depends On now lists architect, task_planner, risk_analyst.
• architect section: documents slice composition authority (Plan phase: architect owns slice composition; risk_analyst becomes dual-role reviewer; reviewer_plan NACKs oversized slices #2809) and adds architect-slices.yaml to Outputs; adds a Reviewed by: line.
• task_planner section: reframes purpose as "enumerate tasks inside the architect's slice scaffold"; updates prompt context to include the scaffold.
• risk_analyst section: split into Producer role (risk register) and Reviewer role (CRITICAL on architect + task_planner), paralleling the implement-phase tester dual-role pattern (#2749) the reviewer called out.

3. _synthesize_plan_draft fallback ignores the new scaffold — fixed-in-PR (commit e75ce78)

orchestrator/routes/pipelines.py — appended ("architect-slices.yaml", "Slice Scaffold") to agent_files so the binding scaffold is captured by the recovery synthesizer when task_planner fails to write {id}-plan.md directly. As the reviewer noted, YAML falls through the existing json.loads branch to raw text, which is acceptable since the file is small and human-readable.

Added test_includes_architect_slices_yaml_when_present to orchestrator/tests/test_pipeline_prompts.py::TestSynthesizePlanDraftNamespaced pinning:

• the new ## Slice Scaffold heading is emitted,
• the raw YAML body (Bootstrap, parent_slice_id: null) survives the fallback,
• the existing architect-output + risk-analyst sections are still included alongside it.

All 8 tests in the class pass locally (pytest orchestrator/tests/test_pipeline_prompts.py::TestSynthesizePlanDraftNamespaced). make lint is green.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns.

The structured outputs in this PR (architect-slices.yaml scaffold, risk_analyst verdict JSON) are legitimate inter-agent contracts with real downstream consumers — task_planner copies the scaffold verbatim, _synthesize_plan_draft reads it as a HITL fallback, and the upstream producer acts on blocking_concerns/feedback. The BRC consensus protocol mandates ACK/NACK structure regardless. This is not "structured output for humans."

Other checks: no large diffs/file contents pre-fetched into prompts; the new slice-sizing rubric is explicitly judgment-based (no fixed LOC budget, four heuristic triggers) rather than lockstep procedure; the "copy verbatim" directive to task_planner is a clean separation of concerns with a documented escape valve (NACK pressure on architect) rather than silent re-shaping; no LLM API calls, no hardcoded model identifiers, no prompt-level security boundaries.

— Authored by egg

[egg-reviewer]

Re-review of #2821 at e75ce78e767bdbc0d9e43fb39cb468b7403c5f94 (delta from prior review at 8c150e93bd2a74ac9a28058d0a9556cfb96a10ce).

Verdict

All three non-blocking findings from the prior review are properly addressed. No new issues found.

Per-finding verification

1. docs/architecture/slice-dag.md documentation drift — ✅ Fixed

docs/architecture/slice-dag.md:625-677 is now consistent with the new contract:

• New Architect bullet declares sole authority for slice composition (#2809) and the binding architect-slices.yaml scaffold output.
• Planner bullets reframe item 1 around verbatim consumption of the architect scaffold, with the "raise NACK pressure on architect" escape valve replacing local re-shaping. Items 2 (Forest constraint) and 3 (Auto-serialization) correctly attribute origination to the architect and explicitly state the planner preserves the field from the scaffold.
• Plan reviewer item 1 (Forest-violation NACK) now correctly NACKs the architect rather than the planner, with a clear "slice scaffold ownership moved to architect in #2809" inline note.
• Plan reviewer item 2 now describes the §11 hard-NACK rubric, cites #2809 alongside HITL decision-6 with the "inverts" framing, documents the seam-naming requirement, points at _get_plan_review_criteria() §11, and preserves the refiner/operator override path for deliberate large slices.

I grepped docs/ for stale framing — advisory only, NEVER NACK, Slice shape is your call, soft target, never enforced, Slice-sizing advisory, Slice-sizing guidance — and the only remaining HITL decision-6 opt-2 references are the two intentional "inverting" mentions in slice-dag.md:626,664. No drift left in docs/.

2. docs/reference/agent-roles.md documentation drift — ✅ Fixed

docs/reference/agent-roles.md:28-29:

• Line 28: risk_analyst's Category now reads Analysis (dual-role: also reviews architect + task_planner).
• Line 29: reviewer_plan's Depends On now lists architect, task_planner, risk_analyst (architect added).

agent-roles.md:86-129:

• architect section (line 86) documents sole authority for slice composition, architect-slices.yaml in Outputs, and adds a Reviewed by: line naming reviewer_plan (CRITICAL, structural) and risk_analyst (CRITICAL, risk).
• task_planner section (line 102) reframes purpose around enumerating tasks inside the architect's scaffold and adds the scaffold to prompt context.
• risk_analyst section (line 116) is split into Producer role + Reviewer role subsections; the Reviewer role text correctly cites the Dual-Role Execution Order banner and CONSENSUS_PROPOSE-augmented producer waits, matching the tester (#2749) dual-role pattern. Verified tester → coder is the analogous edge in orchestrator/review_graph.py:312.

3. _synthesize_plan_draft fallback ignores the scaffold — ✅ Fixed

orchestrator/routes/pipelines.py:18851-18855 — ("architect-slices.yaml", "Slice Scaffold") is appended to agent_files between architect-output and risk_analyst-output, which is a sensible ordering (slice scaffold sits next to its author's analysis JSON).

YAML fall-through behavior is correct: json.loads(raw_yaml) raises JSONDecodeError for the standard scaffold shape (multiline list under slices:, unquoted keys), and the except json.JSONDecodeError: content = raw branch (line 18872-18874) preserves the raw YAML as the section body. This matches the reviewer's prior observation that the YAML fall-through is acceptable for this small, human-readable file.

The new test_includes_architect_slices_yaml_when_present (orchestrator/tests/test_pipeline_prompts.py:2656-2702) exercises the production code path directly (no mocks of the function under test), uses a representative scaffold YAML structure (slices:, id, name, goal, parent_slice_id), and pins:

• the new ## Slice Scaffold heading is emitted,
• raw YAML body content survives the fallback (Bootstrap, parent_slice_id: null),
• the existing architect-output and risk_analyst-output sections are still included alongside it.

Test name matches assertions (no name-vs-behavior contradiction). Fixture is hand-built but the production code path under test is _synthesize_plan_draft itself, which is what we want to verify.

What I re-verified end-to-end

• risk_analyst continues to resolve to dual-role via is_producer and is_reviewer derivation in concurrent_executor.py — no change in this delta.
• §11 rubric in _get_plan_review_criteria() and the reviewer-prep wording in _build_reviewer_preparation are unchanged from the previous review; the doc updates now match the code.
• Architect prompt at orchestrator/routes/pipelines.py:14347-14395 is internally consistent with the new doc framing (scaffold output, file restrictions, verbatim consumption contract).
• _synthesize_plan_draft is a recovery-only path (fires when task_planner fails to write the plan draft directly per the docstring at pipelines.py:18820-18824); the synthesized output is best-effort and feeds the HITL gate, not the structured slice populator. Including the raw scaffold YAML is an improvement over the prior behavior where the operator saw only architecture analysis + risk assessment.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

5 previous review(s) hidden.