diff --git a/docs/architecture/slice-dag.md b/docs/architecture/slice-dag.md index df24cf391e..bfb002ec09 100644 --- a/docs/architecture/slice-dag.md +++ b/docs/architecture/slice-dag.md @@ -622,36 +622,59 @@ team uses. The dynamic prompt builders for `task_planner` and `reviewer_plan` were extended to teach the agents the new schema and constraints: -- **Planner (`task_planner`)** — three new sections appended to the plan - phase prompt: - 1. *Slice-sizing guidance* (advisory only, never enforced; HITL - decision-6 opt-2): the planner is encouraged to keep slices ≤1,000 - LOC and informed that the plan reviewer issues escalating advisories - at >1,000 / >2,000 LOC. +- **Architect (`architect`)** — sole authority for slice composition + (#2809, inverting HITL decision-6 opt-2). The architect prompt + declares this authority explicitly and the architect emits a binding + `architect-slices.yaml` scaffold alongside its analysis JSON + (`{identifier}-architect-slices.yaml` under `.egg-state/agent-outputs/`). + The scaffold encodes slice `id` / `name` / `goal` / `parent_slice_id`; + `tasks:` is intentionally omitted (that is `task_planner`'s job). +- **Planner (`task_planner`)** — sections appended to the plan phase + prompt: + 1. *Slice composition is NOT the planner's call* (#2809): the + architect's `architect-slices.yaml` scaffold is binding. The + planner copies it verbatim into the `# yaml-tasks` appendix + (same `id` / `name` / `goal` / `parent_slice_id`, same order) and + fills in `tasks:` under each slice. The planner does not + silently re-shape slices — if a slice needs to be subdivided, + the planner raises NACK pressure on the architect (via the + plan prose, which `risk_analyst` / `reviewer_plan` pick up) + rather than re-shaping locally. 2. *Forest constraint* (HARD): every slice must have ≤1 DAG parent; the populator hard-rejects multi-parent slices with - `ForestValidationError`. - 3. *Auto-serialization rule with worked example*: when the planner - identifies a slice that would otherwise have >1 parents, it - serialises the upstream cluster into a chain and records the chosen - order on the downstream slice's `serialized_chain_order` field. The - fallback heuristic (`files_affected` Jaccard >0.3, then descending - fan-out) is documented; the planner's own ordering is the source of - truth (HITL decision-17). + `ForestValidationError`. The architect's scaffold encodes this + via `parent_slice_id`; the planner preserves it. + 3. *Auto-serialization* for would-be multi-parent slices: the + architect is responsible for serialising the upstream cluster + and populating `serialized_chain_order` on the downstream + slice. The fallback heuristic (`files_affected` Jaccard >0.3, + then descending fan-out) is documented; the architect's own + ordering is the source of truth (HITL decision-17). The + planner preserves the field verbatim from the scaffold. 4. The yaml-block key swap: `slices:` is canonical, `phases:` is backward-compat. - **Plan reviewer (`reviewer_plan`)** — two new prompt sections: 1. *Forest-violation NACK*: when the populator left a "Plan ingestion REJECTED" block on `plan_review_feedback` (or a `forest_violation` - log discriminator is present), the reviewer NACKs the planner with - the structured errors verbatim and instructs re-emission with - `serialized_chain_order` populated. - 2. *Slice-sizing advisory* (advisory only, NEVER NACK): tone scales - with magnitude — 1,000–2,000 LOC: "consider splitting"; >2,000 LOC: - "well above the soft target — strongly consider splitting". HITL - decision-6 opt-2 keeps override authority with the - refiner/operator; promoting the advisory to a hard NACK requires a - fresh HITL revision. + log discriminator is present), the reviewer NACKs the **architect** + (not the planner — slice scaffold ownership moved to architect in + #2809) with the structured errors verbatim and instructs re-emission + of the scaffold with `serialized_chain_order` populated. + 2. *Slice-sizing NACK* (hard, judgment-based; #2809 inverts HITL + decision-6 opt-2). The reviewer is empowered AND required to + hard-NACK the architect on `slice_size` when a slice is oversized + for one BRC cycle. There is no fixed LOC budget — the rubric is + judgment-based: NACK when a slice bundles >~3 distinct + file-categories, combines deletion-heavy work with new-API + introduction, would require >3–4 commit-propose-revise cycles to + converge, or contains independent task groups with no internal + dependency. The reviewer names the seam where subdivision is + appropriate so the architect's re-propose is actionable. See + `_get_plan_review_criteria()` §11 for the full rubric and worked + NACK examples. Refiner/operator override remains available when a + large slice is deliberate (e.g. atomic schema migration) — the + architect cites the override in the analysis and the reviewer + ACKs once the rationale is on the record. ## Configuration knobs diff --git a/docs/reference/agent-roles.md b/docs/reference/agent-roles.md index 0e3eae8919..364c01e59a 100644 --- a/docs/reference/agent-roles.md +++ b/docs/reference/agent-roles.md @@ -25,8 +25,8 @@ Use `get_roles_by_category(AgentCategory.REVIEW)` to dynamically query roles by | `reviewer_agent_design` | Review | Refine (egg repo only) | Yes (with `reviewer_refine`) | refiner | | `architect` | Analysis | Plan | No | — | | `task_planner` | Analysis | Plan | Yes (with `risk_analyst`) | architect | -| `risk_analyst` | Analysis | Plan | Yes (with `task_planner`) | architect | -| `reviewer_plan` | Review | Plan | No | task_planner, risk_analyst | +| `risk_analyst` | Analysis (dual-role: also reviews `architect` + `task_planner`) | Plan | Yes (with `task_planner`) | architect | +| `reviewer_plan` | Review | Plan | No | architect, task_planner, risk_analyst | | `applier` | Execution | Apply (epic-mode only) | No | — | | `coder` | Execution | Implement | No | — | | `tester` | Execution | Implement | Yes (with `documenter`) | coder | @@ -85,7 +85,7 @@ All agents within a phase run concurrently via BRC consensus. Concurrency is ena ### `architect` -**Purpose**: Analyze the task, research the codebase, and recommend a high-level implementation approach. +**Purpose**: Analyze the task, research the codebase, and recommend a high-level implementation approach. **Sole authority for slice composition** in the plan phase (#2809) — owns slice count, slice boundaries, slice DAG shape, and sub-slicing. Emits a binding `architect-slices.yaml` scaffold that `task_planner` copies verbatim into the plan document's `# yaml-tasks` appendix. **File access**: - Allowed writes: `.egg-state/drafts/`, `.egg-state/agent-outputs/` @@ -93,29 +93,38 @@ All agents within a phase run concurrently via BRC consensus. Concurrency is ena **Outputs**: - `.egg-state/agent-outputs/{identifier}-architect-output.json` — Architectural analysis +- `.egg-state/agent-outputs/{identifier}-architect-slices.yaml` — Binding slice scaffold (`id` / `name` / `goal` / `parent_slice_id`; no `tasks:` — that is `task_planner`'s job) **Prompt context**: Full issue body, refine analysis. +**Reviewed by**: `reviewer_plan` (structural lens, CRITICAL) and `risk_analyst` (risk lens, CRITICAL). + ### `task_planner` -**Purpose**: Break the work into discrete phases and tasks with acceptance criteria. Produces the plan document with a YAML appendix. +**Purpose**: Enumerate the discrete tasks (with acceptance criteria) **inside the architect's slice scaffold**. Produces the plan document with a YAML appendix; the slice shape comes from `architect-slices.yaml` verbatim (#2809). When a slice needs subdivision, the planner raises NACK pressure on the architect via the plan prose rather than silently re-shaping slices. **File access**: Same as `architect`. **Outputs**: -- `.egg-state/drafts/{identifier}-plan.md` — The plan document (includes YAML appendix) +- `.egg-state/drafts/{identifier}-plan.md` — The plan document (includes YAML appendix; slice scaffold copied verbatim from architect, `tasks:` enumerated underneath) - `.egg-state/agent-outputs/{identifier}-task_planner-output.json` — Handoff data -**Prompt context**: Full issue body, architect output. +**Prompt context**: Full issue body, architect output, architect slice scaffold. + +**Reviewed by**: `reviewer_plan` (structural lens, CRITICAL) and `risk_analyst` (risk lens, CRITICAL). ### `risk_analyst` -**Purpose**: Identify technical risks and propose mitigation strategies. +**Purpose**: Identify technical risks and propose mitigation strategies. **Dual-role in the plan phase** (#2809) — also reviews `architect` and `task_planner` through the risk lens, mirroring the implement-phase `tester` dual-role pattern (#2749). + +**Producer role**: emit a risk register (top risks, mitigations, blocking concerns). + +**Reviewer role**: CRITICAL reviewer of `architect` and `task_planner`. NACK when a risk is severe enough that shipping the plan as-proposed would invite a known-class failure (security regression, data loss, compliance break, runtime-primitive or trust-boundary mismatch). ACK when risks are real but mitigated, or low enough that the plan can ship and the risks belong in the register as forward-looking notes. Verdict / feedback is mirrored from the producer artifact into the `egg-orch consensus ack` / `nack` reason body so the upstream producer can act on it. The *Dual-Role Execution Order* banner in the BRC preamble is the authoritative ordering — `CONSENSUS_PROPOSE`-augmented producer waits wake the agent on architect's / task_planner's proposals. **File access**: Same as `architect`. **Outputs**: -- `.egg-state/agent-outputs/{identifier}-risk_analyst-output.json` — Risk analysis +- `.egg-state/agent-outputs/{identifier}-risk_analyst-output.json` — Risk analysis (producer artifact carries the verdict + `blocking_concerns` + `feedback` payload mirrored into reviewer ACK/NACK reasons) **Prompt context**: Full issue body, architect output. diff --git a/orchestrator/review_graph.py b/orchestrator/review_graph.py index 181d75fbdd..3c186068a6 100644 --- a/orchestrator/review_graph.py +++ b/orchestrator/review_graph.py @@ -236,22 +236,40 @@ def get_default_refine_graph() -> ReviewGraph: def get_default_plan_graph() -> ReviewGraph: """Get the default review graph for the plan phase. - Review adjacency per the phase-role mappings: - - reviewer_plan reviews architect (critical) - - reviewer_plan reviews task_planner (critical) - - reviewer_plan reviews risk_analyst (advisory) + Review adjacency per the phase-role mappings (issue #2809): + - reviewer_plan reviews architect (critical) — structural lens + - reviewer_plan reviews task_planner (critical) — structural lens + - reviewer_plan reviews risk_analyst (advisory) — risk register + still reviewable as a producer artifact + - risk_analyst reviews architect (critical) — risk lens, dual-role + - risk_analyst reviews task_planner (critical) — risk lens, dual-role + + Plan-phase consensus therefore requires **both** ``reviewer_plan`` + and ``risk_analyst`` to ACK every CRITICAL producer (architect, + task_planner). The two lenses catch what one wouldn't: ``reviewer_plan`` + audits structure (slice DAG shape, slice_size, role assignments, + test strategy, rollback, PR block); ``risk_analyst`` audits the + risk surface (what could go wrong with this design; blocking + concerns). ``risk_analyst`` is dual-role — it produces the risk + register and also reviews its upstream peers, mirroring the + implement-phase ``tester`` pattern (#2749). Producers: architect, task_planner, risk_analyst - Reviewers: reviewer_plan + Reviewers: reviewer_plan, risk_analyst (dual-role) """ return ReviewGraph( [ - # reviewer_plan reviews architect (critical) + # reviewer_plan reviews architect (critical) — structural lens ReviewEdge("reviewer_plan", "architect", ReviewCriticality.CRITICAL), - # reviewer_plan reviews task_planner (critical) + # reviewer_plan reviews task_planner (critical) — structural lens ReviewEdge("reviewer_plan", "task_planner", ReviewCriticality.CRITICAL), - # reviewer_plan reviews risk_analyst (advisory) + # reviewer_plan reviews risk_analyst (advisory) — risk + # register still reviewable as a producer artifact ReviewEdge("reviewer_plan", "risk_analyst", ReviewCriticality.ADVISORY), + # risk_analyst reviews architect (critical — risk lens, #2809) + ReviewEdge("risk_analyst", "architect", ReviewCriticality.CRITICAL), + # risk_analyst reviews task_planner (critical — risk lens, #2809) + ReviewEdge("risk_analyst", "task_planner", ReviewCriticality.CRITICAL), ] ) diff --git a/orchestrator/routes/pipelines.py b/orchestrator/routes/pipelines.py index 19931f5d77..51b2d2430f 100644 --- a/orchestrator/routes/pipelines.py +++ b/orchestrator/routes/pipelines.py @@ -4935,7 +4935,56 @@ def _get_plan_review_criteria() -> str: '- "task TASK-4-2 references `ScriptedProvider` from ' "`sandbox/` (or any deployed-pod path) — it is a unit-test " "double under `shared/tests/`, not a runtime-injectable " - 'provider"\n' + 'provider"\n\n' + "### 11. Slice Sizing (hard NACK, judgment-based — see #2809)\n" + "Slice sizing is owned by the **architect**, not the " + "task_planner. ``reviewer_plan`` is empowered AND required to " + "hard-NACK the architect when a slice is oversized for one " + "BRC cycle. This is a separate rubric key from the slice-DAG " + "shape checks so the NACK is unambiguously routed to the " + "architect for slice re-shaping (re-spawn ``architect`` with " + "the subdivision feedback).\n\n" + "**No fixed tasks-per-slice budget.** Use judgment. NACK when " + "any of the following holds:\n\n" + "- A single slice touches **more than ~3 distinct " + "file-categories** (e.g. orchestrator + gateway + schema + " + "tests + docs all in one slice probably wants subdivision).\n" + "- A single slice combines **deletion-heavy work** with " + "**new-API-introduction work** — these usually want different " + "review attention and ship better as separate slices.\n" + "- A single slice would require the implementing producer to " + "**commit-propose-revise more than 3–4 times** to converge " + "(typical signal: many independent commit clusters with " + "different reviewer surfaces).\n" + "- A single slice contains **independent task groups with no " + "internal dependency** — natural seams for parallel " + "sub-slices.\n\n" + "**NACK format**: name the seam where subdivision is " + "appropriate so the architect's re-propose is actionable. " + "Examples:\n\n" + '- "slice-1 bundles gateway allowlist edits, orchestrator ' + "route handlers, and shared/egg_contracts schema changes — " + "three distinct file-categories with different reviewer " + "surfaces. Subdivide along the gateway / orchestrator " + '/ schema seam."\n' + '- "slice-2 bundles ~600 LOC of removals across "' + "orchestrator/* with ~200 LOC of new gateway-Jira routes — " + "deletion-heavy + new-API in one cycle. Ship the removals " + 'as one slice and the new routes as a downstream slice."\n' + '- "slice-3 contains 9 tasks across 4 independent feature ' + "areas (search, profile, settings, notifications) with no " + "cross-area dependency — subdivide into one slice per " + 'area."\n\n' + "The architect re-proposes with the subdivision applied (the " + "existing BRC re-review loop handles convergence). " + "task_planner re-consumes the revised " + "``architect-slices.yaml`` scaffold on the next BRC cycle. " + "**Refiner / operator can override sizing concerns** if there " + "is a deliberate reason to ship a large slice (e.g. atomic " + "schema migration that cannot be split safely) — in that " + "case the architect should cite the override in the analysis " + "and the reviewer can ACK once the rationale is on the " + "record.\n" ) @@ -12252,9 +12301,15 @@ def _build_phase_prompt( "``# yaml-tasks`` block (the parser also accepts ``phases:`` for " "backward compatibility). New plans should use ``slices:``.", "", - "**Slice-sizing guidance (soft, advisory only)**: target ≤1,000 " - "LOC per slice where possible. The plan reviewer flags oversized " - "slices as advisory but does NOT reject on size.", + "**Slice-sizing NACK (hard, judgment-based — #2809)**: the plan " + "reviewer will hard-NACK an oversized slice. Use judgment when " + "shaping — no fixed LOC budget, but avoid bundling more than ~3 " + "distinct file-categories in one slice, avoid combining " + "deletion-heavy work with new-API-introduction work, avoid " + "slices that would require >3–4 commit-propose-revise cycles, " + "and avoid bundling independent task groups with no internal " + "dependency. Subdivide along those seams up front rather than " + "earning a NACK.", "", "**Forest constraint (HARD)**: every slice must have at most ONE " "DAG parent — the implement-phase pipeline ships every slice as a " @@ -13371,20 +13426,22 @@ def _build_reviewer_preparation( "rejected at plan ingestion with a " "``forest_violation`` log discriminator (or the contract's " "``plan_review_feedback`` carries a 'Plan ingestion REJECTED' " - "block), NACK the planner and cite the structured errors " - "verbatim. Instruct the planner to re-emit the plan with " - "``serialized_chain_order`` populated on the downstream " - "slice. " - "(2) **Slice-sizing advisory (advisory only — never " - "NACK)**: for each slice whose estimated LOC " - "(count of ``files_affected`` × heuristic weight) is " - ">1,000, surface a non-blocking advisory line in your ACK " - "body. Tone scales with magnitude: 1,000–2,000 LOC: " - "'consider splitting'; >2,000 LOC: 'this slice is well " - "above the soft target — strongly consider splitting'. " - "Per HITL decision-6 opt-2 the plan reviewer NEVER NACKs " - "on size — the refiner/operator retains override " - "authority." + "block), NACK the architect and cite the structured errors " + "verbatim. Instruct the architect to re-emit the slice " + "scaffold with ``serialized_chain_order`` populated on the " + "downstream slice. " + "(2) **Slice-sizing NACK (hard, judgment-based — #2809)**: " + "slice composition is owned by the **architect**, not the " + "task_planner. You ARE empowered and required to hard-NACK " + "the architect on ``slice_size`` when a slice is oversized " + "for one BRC cycle. Use judgment — no fixed tasks-per-slice " + "or LOC budget. NACK when a slice bundles more than ~3 " + "distinct file-categories, combines deletion-heavy with " + "new-API-introduction work, would require >3–4 " + "commit-propose-revise cycles, or contains independent " + "task groups with no internal dependency. Name the seam in " + "your NACK so the architect's re-propose is actionable. " + "See criteria §11 for the full rubric and examples." ) elif phase == "refine": if role_value in ("reviewer_refine", "reviewer_agent_design"): @@ -14250,9 +14307,83 @@ def _build_agent_prompt( "", f"Write your analysis to `.egg-state/agent-outputs/{_identifier}-architect-output.json`.", "", + # ---------------------------------------------------- + # #2809 — architect owns slice composition + # ---------------------------------------------------- + "## Slice composition authority (#2809)", + "", + "**You are the sole authority for slice composition in the " + "plan phase.** ``task_planner`` enumerates tasks within the " + "slices you define; ``risk_analyst`` surfaces risks that " + "feed your design. Neither owns slice shape — you do. " + "Specifically, you own:", + "", + "- **Slice count.** Treat the operator's ``cq-1`` (or " + "equivalent refine-phase complexity answer) as a coarse " + "top-level hint, not a literal slice count. Subdivide " + "further when the natural slice DAG calls for it.", + "- **Slice boundaries.** Which work goes into which slice, " + "anchored on design seams.", + "- **Slice DAG shape.** Parent/child dependencies between " + "slices. The forest constraint (every slice has at most " + "ONE DAG parent) is HARD — multi-parent slices break the " + "stacked-PR invariant. If a slice would naturally have >1 " + "parents, serialise the upstream slices into a linear " + "chain and record the chosen ordering on the downstream " + "slice's ``serialized_chain_order`` field. See " + "``docs/architecture/slice-dag.md``.", + "- **Sub-slicing.** When one slice would be too coarse, " + "subdivide it. Right-size slices for a single BRC cycle: " + "avoid bundling distinct file-category groups (e.g. " + "orchestrator + gateway + schema + tests + docs all in " + "one slice), avoid bundling deletion-heavy work with " + "new-API-introduction work, and avoid bundling task " + "groups that have no internal dependency — those are " + "natural seams for parallel sub-slices. If a slice would " + "require the implementing producer to " + "commit-propose-revise more than 3–4 times to converge, " + "subdivide it.", + "", + "Emit the slice scaffold as a YAML file alongside your " + "JSON analysis. ``task_planner`` will copy this scaffold " + "**verbatim** into the plan document's ``# yaml-tasks`` " + "appendix and fill in ``tasks:`` under each slice — the " + "scaffold is binding. If ``reviewer_plan`` NACKs on " + "``slice_size`` or the structural lens calls a " + "sub-division, you re-propose with the updated scaffold; " + "task_planner re-consumes the new scaffold on the next " + "BRC cycle.", + "", + f"Write the slice scaffold to `.egg-state/agent-outputs/{_identifier}-architect-slices.yaml`:", + "", + "```yaml", + "slices:", + " - id: 1", + " name: |-", + " ", + " goal: |-", + " ", + " parent_slice_id: null # root", + " - id: 2", + " name: |-", + " ", + " goal: |-", + " ", + " parent_slice_id: 1", + "```", + "", + "Use ``parent_slice_id: null`` for root slices and the " + "parent slice's integer ``id`` otherwise. Do NOT include " + "``tasks:`` in the scaffold — that is task_planner's job. " + "Keep ``name`` and ``goal`` concise enough that " + "task_planner can copy them without rewording.", + "", "### File Restrictions", "", - f"You MUST only write to `.egg-state/agent-outputs/{_identifier}-architect-output.json`.", + "You MUST only write to:", + f"- `.egg-state/agent-outputs/{_identifier}-architect-output.json`", + f"- `.egg-state/agent-outputs/{_identifier}-architect-slices.yaml`", + "", "Do NOT create or modify any other files. Specifically:", "- Do NOT modify analysis drafts (`.egg-state/drafts/*-analysis.md`) — " "these are finalized in the refine phase and are read-only", @@ -14264,30 +14395,43 @@ def _build_agent_prompt( ) elif role_value == "task_planner": draft_path = _get_draft_path("plan", issue_number=issue_number, pipeline_id=pipeline_id) + architect_slices_path = f".egg-state/agent-outputs/{_identifier}-architect-slices.yaml" lines.extend( [ "Decompose the architecture analysis into a slice-DAG implementation " "plan. The implement-phase pipeline ships each slice as its own " - "stacked PR — see the ``## Slice-DAG guidance (#2137)`` section below " - "for the slice-shaping rules. **Slice shape is your call.** A " - "single-slice plan is fine when the work is cohesive; pick a " - "multi-slice shape when the work has clean seams that ship " - "independently. If the refine analysis sketched a decomposition " - "(e.g. naming the components touched), treat it as **advisory " - "context** — you are free to slice differently if a better seam " - "exists. The only thing that binds your slice shape is an explicit " - "slice-DAG HITL decision recorded by the operator on the contract; " - "if you believe such a decision is wrong, raise it as an open " - "question in your plan rather than silently overriding.", + "stacked PR.", + "", + "**Slice composition is NOT your call (#2809).** ``architect`` owns " + "slice count, slice boundaries, slice DAG shape, and sub-slicing — " + f"and emits the binding scaffold at `{architect_slices_path}`. Your job " + "is to enumerate ``tasks:`` within those slices, **not to re-shape " + "them**. Copy the architect's scaffold verbatim into the " + "``# yaml-tasks`` appendix (preserving slice ``id``, ``name``, " + "``goal``, and ``parent_slice_id``) and add ``tasks:`` under each " + "slice with task IDs of the form ``TASK--``.", + "", + "If a slice has too many tasks for one BRC cycle, or you discover a " + "natural sub-seam the architect missed, that is a **slicing problem " + "the architect must fix** — surface it as NACK pressure (your peer " + "reviewer ``risk_analyst`` and the structural reviewer " + "``reviewer_plan`` will NACK ``architect`` on ``slice_size`` when " + "evidence supports it; you can also flag the concern in your plan " + "prose so the reviewers pick it up). **Do NOT silently re-shape " + "slices.** Re-propose against the architect's revised scaffold " + "once it lands.", "", "Steps:", - "1. Review the architecture analysis from the ARCHITECT agent", - "2. Break down the work into phases with discrete, actionable tasks", - "3. Define clear acceptance criteria for each task", - "4. Define dependency ordering between tasks", - "5. Identify the test strategy — what automated tests cover the changes, " - "and what manual verification is needed", - "6. Identify any manual pre-merge or post-merge steps " + f"1. Read the architecture analysis AND the slice scaffold at `{architect_slices_path}`", + "2. Copy the architect's slice scaffold verbatim into the " + "``# yaml-tasks`` appendix (same ``id`` / ``name`` / ``goal`` / " + "``parent_slice_id`` values, in the same order)", + "3. Enumerate ``tasks:`` under each slice — discrete, " + "actionable, with clear acceptance criteria and dependency ordering " + "between tasks", + "4. Identify the test strategy — what automated tests cover the " + "changes, and what manual verification is needed", + "5. Identify any manual pre-merge or post-merge steps " "(migrations, config changes, deployments)", "", "## Output Format", @@ -14420,26 +14564,26 @@ def _build_agent_prompt( "``phases:`` for backward compatibility with already-shipped " "planner prompts). New plans should use ``slices:``.", "", - "**Slice-sizing guidance (soft, advisory only)**: target " - "≤1,000 LOC per slice where possible. Slices estimated above " - "1,000 LOC will be flagged as advisory by the plan reviewer " - "but are NOT rejected. There is no hard size ceiling — the " - "refiner/operator can override sizing concerns at any point. " - "The plan reviewer never NACKs on size.", + "**Slice sizing is the architect's call (#2809).** Slice " + "count, boundaries, and DAG shape come from the architect's " + "scaffold — copy them verbatim. ``reviewer_plan`` will hard " + "NACK ``architect`` on ``slice_size`` when a slice is " + "oversized for one BRC cycle (judgment-based — see the " + "reviewer's §11 rubric); do NOT silently re-shape slices " + "to dodge a size concern. Raise it as NACK pressure on " + "architect instead (see the surfacing guidance above).", "", - "**Forest constraint (HARD)**: every slice must have at most " - "ONE DAG parent — the implement-phase pipeline ships every " - "slice as a stacked PR with exactly one base branch. " - "Multi-parent slices break the stacking invariant and are " - "rejected at plan ingestion.", + "**Forest constraint (HARD, enforced at plan ingestion)**: " + "every slice must have at most ONE DAG parent — the " + "implement-phase pipeline ships every slice as a stacked " + "PR with exactly one base branch. The architect's scaffold " + "encodes this via ``parent_slice_id``; preserve it.", "", - "**Auto-serialization rule for would-be multi-parent slices**: " - "when you identify a slice that would naturally have >1 " - "parents, you MUST serialise the upstream slices into a " - "linear chain and record your chosen ordering on the " - "downstream slice's ``serialized_chain_order`` field. The " - "list names the upstream slice IDs in their chosen " - "serialization order.", + "**Auto-serialization for would-be multi-parent slices**: " + "the architect is responsible for serialising would-be " + "multi-parent slices and populating " + "``serialized_chain_order`` on the downstream slice. " + "Preserve that field verbatim from the scaffold.", "", "Worked example: if ``slice-3`` would naturally have " "parents ``[slice-1, slice-2]``, instead emit:", @@ -14482,6 +14626,17 @@ def _build_agent_prompt( elif role_value == "risk_analyst": lines.extend( [ + "**You are dual-role (producer AND reviewer) in this phase " + "(#2809).** You produce the risk register AND you review " + "``architect`` and ``task_planner`` through the risk lens — " + "your NACK blocks plan-phase consensus until the upstream " + "producer re-proposes addressing the concern. This mirrors " + "the implement-phase ``tester`` dual-role pattern (#2749); " + "the *Dual-Role Execution Order* banner in your BRC " + "preamble is the authoritative ordering — read it first.", + "", + "## Producer role (risk register)", + "", "Assess technical risks for the proposed implementation:", "", "1. Review the architecture analysis from the ARCHITECT agent", @@ -14506,6 +14661,43 @@ def _build_agent_prompt( "", f"Write your risk assessment to `.egg-state/agent-outputs/{_identifier}-risk_analyst-output.json`.", "", + "## Reviewer role (risk lens on architect + task_planner)", + "", + "When ``architect`` or ``task_planner`` proposes (their " + "``CONSENSUS_PROPOSE`` will wake you via the dual-role " + "augmentation on your producer waits — see the banner), " + "review their work through the risk lens and emit ACK or " + "NACK. ``blocking_concerns`` are NACK-shaped: they block " + "plan-phase consensus and force the upstream producer to " + "re-propose addressing them.", + "", + "Use this verdict shape in your producer artifact " + "(risk-register JSON) **and** mirror the verdict / " + "feedback in your ``egg-orch consensus ack`` / " + "``egg-orch consensus nack`` ``--reason`` body so the " + "upstream producer can act on it:", + "", + "```json", + "{", + ' "verdict": "ACK" | "NACK",', + ' "risks": [...],', + ' "top_3_risks": [...],', + ' "blocking_concerns": [...],', + ' "feedback": "concrete revision instructions for architect / task_planner (empty on ACK)"', + "}", + "```", + "", + "NACK when a risk is severe enough that shipping the plan " + "as-proposed would invite a known-class failure (security " + "regression, data loss, compliance break, runtime-primitive " + "or trust-boundary mismatch that would surface as an " + "expensive implement-phase NACK). ACK when risks are real " + "but mitigated, or low enough that the plan can ship and " + "the risks belong in the register as forward-looking " + "notes. Be specific in ``feedback`` — name the file, " + "the slice, the missing mitigation — so the upstream " + "producer's re-propose is actionable.", + "", ] ) elif role_value.startswith("reviewer_"): @@ -18658,6 +18850,7 @@ def _synthesize_plan_draft( sections: list[str] = [] agent_files = [ ("architect-output.json", "Architecture Analysis"), + ("architect-slices.yaml", "Slice Scaffold"), ("risk_analyst-output.json", "Risk Assessment"), ] diff --git a/orchestrator/tests/test_concurrent_executor.py b/orchestrator/tests/test_concurrent_executor.py index c36bf9235e..fc539452a3 100644 --- a/orchestrator/tests/test_concurrent_executor.py +++ b/orchestrator/tests/test_concurrent_executor.py @@ -475,6 +475,16 @@ def test_plan_graph_has_correct_edges(self): assert "reviewer_plan" in graph.critical_reviewers_for("task_planner") assert "reviewer_plan" in graph.advisory_reviewers_for("risk_analyst") + # #2809 — risk_analyst is dual-role: CRITICAL reviewer of + # architect and task_planner in addition to producing the + # risk register. + assert graph.is_reviewer("risk_analyst") + assert graph.is_dual_role("risk_analyst") + assert "risk_analyst" in graph.reviewers_for("architect") + assert "risk_analyst" in graph.reviewers_for("task_planner") + assert "risk_analyst" in graph.critical_reviewers_for("architect") + assert "risk_analyst" in graph.critical_reviewers_for("task_planner") + # Phase lookup returns same structure phase_graph = get_review_graph_for_phase("plan") assert len(phase_graph.edges) == len(graph.edges) diff --git a/orchestrator/tests/test_pipeline_prompts.py b/orchestrator/tests/test_pipeline_prompts.py index 594afe8bcb..15e0b369be 100644 --- a/orchestrator/tests/test_pipeline_prompts.py +++ b/orchestrator/tests/test_pipeline_prompts.py @@ -2653,6 +2653,54 @@ def test_empty_outputs_produce_no_draft(self, tmp_path): # No meaningful content → draft not written assert not draft_path.exists() + def test_includes_architect_slices_yaml_when_present(self, tmp_path): + """architect-slices.yaml is captured in the synthesized fallback draft (#2809). + + The fallback synthesizer is the recovery path when task_planner fails + to write {id}-plan.md directly. The architect scaffold is the binding + source of slice DAG shape, so it must be visible in the synthesized + draft for the HITL gate to show the operator a complete plan. + """ + drafts_dir = tmp_path / ".egg-state" / "drafts" + drafts_dir.mkdir(parents=True) + + outputs_dir = tmp_path / ".egg-state" / "agent-outputs" + outputs_dir.mkdir(parents=True) + (outputs_dir / "871-architect-output.json").write_text( + json.dumps({"content": "Architecture analysis for issue 871"}) + ) + scaffold_yaml = ( + "slices:\n" + " - id: 1\n" + " name: |-\n" + " Bootstrap\n" + " goal: |-\n" + " Stand up the new auth route\n" + " parent_slice_id: null\n" + ) + (outputs_dir / "871-architect-slices.yaml").write_text(scaffold_yaml) + (outputs_dir / "871-risk_analyst-output.json").write_text( + json.dumps({"content": "Risk assessment for issue 871"}) + ) + + _synthesize_plan_draft( + repo_path=tmp_path, + pipeline_id="issue-871", + pipeline_mode="issue", + issue_number=871, + ) + + draft_path = tmp_path / ".egg-state" / "drafts" / "871-plan.md" + assert draft_path.exists() + content = draft_path.read_text() + # Scaffold heading and raw YAML body (YAML falls through json.loads to raw text) + assert "## Slice Scaffold" in content + assert "Bootstrap" in content + assert "parent_slice_id: null" in content + # Other agent outputs still included + assert "Architecture analysis for issue 871" in content + assert "Risk assessment for issue 871" in content + class TestBuildReviewPrompt: """Tests for _build_review_prompt verdict format, conventions, and preambles.""" @@ -3326,10 +3374,11 @@ class TestPlannerPromptTreatsRefinerSlicingAsAdvisory: Regression for #2793: the planner used to be told that "multi-slice plans are required when an upstream refine-phase work-decomposition HITL decision selected a multi-slice shape" — a now-removed contract that - locked the planner to whatever shape the refiner picked. The new - contract: the planner picks the slice shape; the refiner's sketch is - advisory only; the only thing that binds is an explicit operator-side - slice-DAG HITL decision, which the planner may still push back on. + locked the planner to whatever shape the refiner picked. + + The refiner's sketch is advisory only. Per #2809 the **architect** owns + slice composition within the plan phase (concurrent path); in the + sequential path the single planner agent picks the shape. """ @staticmethod @@ -3388,12 +3437,24 @@ def test_sequential_planner_states_slice_shape_is_planner_call(self): assert "explicit slice-DAG HITL decision" in prompt assert "raise it as an open question" in prompt - def test_concurrent_planner_states_slice_shape_is_planner_call(self): + def test_concurrent_planner_defers_slice_shape_to_architect(self): + """#2809: in the concurrent plan path, slice composition is the + architect's job — task_planner copies the architect-emitted + scaffold verbatim and only enumerates ``tasks:``.""" prompt = self._concurrent_planner_prompt() - assert "Slice shape is your call" in prompt - assert "advisory context" in prompt - assert "explicit slice-DAG HITL decision" in prompt - assert "raise it as an open question" in prompt + # The old "task_planner picks the shape" framing must be gone + # in the concurrent path (where architect now owns it). + assert "Slice shape is your call" not in prompt, ( + "concurrent task_planner still claims slice-shape authority — " + "#2809 moved that ownership to the architect" + ) + # The new contract must be explicit. + assert "architect-slices.yaml" in prompt + assert "verbatim" in prompt + assert "Slice composition is NOT your call" in prompt + # NACK pressure on architect is the documented escape valve for + # discovered slicing problems. + assert "NACK pressure" in prompt class TestPlannerPromptSliceDagFraming: @@ -5212,6 +5273,20 @@ def test_dual_role_banner_absent_for_pure_producer(self): preamble = _build_brc_preamble("coder", "implement") assert "### Dual-Role Execution Order" not in preamble + def test_dual_role_banner_present_for_risk_analyst(self): + """#2809 — risk_analyst is dual-role in the plan phase (mirrors + the implement-phase tester pattern from #2749), so its BRC + preamble must carry the same ordering banner.""" + preamble = _build_brc_preamble("risk_analyst", "plan") + assert "### Dual-Role Execution Order" in preamble + # And its pre-confirm wait must include the augmented + # `--for CONSENSUS_PROPOSE` so peer producer (architect / + # task_planner) proposals wake it for review. + respond_start = preamble.index("**RESPOND TO REVIEWS**") + respond_end = preamble.index("**CONFIRM**", respond_start) + respond_block = preamble[respond_start:respond_end] + assert "--for CONSENSUS_PROPOSE" in respond_block + def test_dual_role_banner_absent_for_pure_reviewer(self): preamble = _build_brc_preamble("reviewer_code", "implement") assert "### Dual-Role Execution Order" not in preamble @@ -5770,6 +5845,148 @@ def test_risk_analyst_prompt_flags_runtime_primitive_risks(self): assert "#2474" in prompt +class TestPlanPhaseArchitectOwnsComposition: + """Issue #2809 — within the concurrent plan phase, slice composition is + owned by ``architect`` (emits ``architect-slices.yaml`` scaffold); + ``task_planner`` consumes it verbatim and only enumerates ``tasks:``; + ``risk_analyst`` becomes a dual-role producer+reviewer; ``reviewer_plan`` + hard-NACKs oversized slices via a new ``slice_size`` rubric.""" + + @staticmethod + def _prompt(role: str, issue_number: int = 871) -> str: + return _build_agent_prompt( + role_value=role, + phase="plan", + pipeline_id="pid-1", + pipeline_mode="issue", + prompt="# Feature\n\nDetail.", + issue_number=issue_number, + concurrent=True, + ) + + def test_architect_prompt_grants_slice_composition_authority(self): + prompt = self._prompt("architect") + assert "Slice composition authority (#2809)" in prompt + assert "sole authority for slice composition" in prompt + # Architect must be told to treat cq-1 as a coarse hint, not a literal count. + assert "cq-1" in prompt + assert "coarse" in prompt + # The four dimensions of authority must be named. + assert "Slice count" in prompt + assert "Slice boundaries" in prompt + assert "Slice DAG shape" in prompt + assert "Sub-slicing" in prompt + + def test_architect_prompt_emits_slice_scaffold_yaml(self): + prompt = self._prompt("architect", issue_number=871) + # Architect must be directed to write the scaffold at the + # issue-prefixed path. + assert "871-architect-slices.yaml" in prompt + # Scaffold schema must show parent_slice_id for forest expression. + assert "parent_slice_id" in prompt + # Architect must NOT enumerate tasks in the scaffold — that's + # task_planner's role. + scaffold_start = prompt.index("Write the slice scaffold") + scaffold_end = prompt.index("### File Restrictions", scaffold_start) + scaffold_section = prompt[scaffold_start:scaffold_end] + assert "Do NOT include ``tasks:``" in scaffold_section + + def test_architect_file_restrictions_permit_scaffold_yaml(self): + prompt = self._prompt("architect", issue_number=871) + restrictions_start = prompt.index("### File Restrictions") + restrictions_section = prompt[restrictions_start:] + # Both artifacts must be explicitly permitted. + assert "871-architect-output.json" in restrictions_section + assert "871-architect-slices.yaml" in restrictions_section + + def test_task_planner_prompt_drops_slice_shape_ownership(self): + prompt = self._prompt("task_planner") + # The old "Slice shape is your call" framing must not survive + # in the concurrent path. + assert "Slice shape is your call" not in prompt + # The new contract must be present. + assert "Slice composition is NOT your call" in prompt + # And reference the architect's scaffold by path. + assert "architect-slices.yaml" in prompt + # Verbatim-copy directive must be explicit. + assert "verbatim" in prompt + + def test_task_planner_surfaces_slicing_problems_as_nack_pressure(self): + prompt = self._prompt("task_planner") + # The escape valve — task_planner discovers a slice is too big, + # surfaces it back as NACK pressure on architect (via the + # risk_analyst dual-role NACK or reviewer_plan structural NACK), + # NOT by silently re-shaping. + assert "NACK pressure" in prompt + assert "Do NOT silently re-shape slices" in prompt + + def test_task_planner_drops_never_nacks_on_size_wording(self): + prompt = self._prompt("task_planner") + # #2809 inverts the prior contract — reviewer_plan now NACKs on + # size. The old "never NACKs on size" wording must not leak into + # the task_planner's slice-DAG block. + assert "never NACKs on size" not in prompt + assert "soft, advisory only" not in prompt + + def test_risk_analyst_prompt_declares_dual_role(self): + prompt = self._prompt("risk_analyst") + # Issue reference. + assert "#2809" in prompt + # Dual-role announcement is the lede. + assert "dual-role (producer AND reviewer)" in prompt + # Pattern reference — risk_analyst mirrors tester's dual-role + # primitive (#2749). + assert "#2749" in prompt + # Both role sections must be present. + assert "## Producer role (risk register)" in prompt + assert "## Reviewer role (risk lens on architect + task_planner)" in prompt + + def test_risk_analyst_prompt_specifies_verdict_shape(self): + prompt = self._prompt("risk_analyst") + # Verdict-shape JSON keys must be documented so reviewer-role + # output is uniform. + assert "blocking_concerns" in prompt + assert "top_3_risks" in prompt + # The verdict field has only two legal values. + assert '"verdict": "ACK" | "NACK"' in prompt + + def test_plan_review_criteria_has_slice_size_section(self): + criteria = _get_plan_review_criteria() + # New §11 rubric on slice sizing. + assert "### 11." in criteria + assert "Slice Sizing" in criteria + # Hard-NACK framing (parallels §9, §10 audit framing). + assert "hard NACK" in criteria + # Issue reference. + assert "#2809" in criteria + # No fixed budget — explicitly judgment-based. + assert "judgment" in criteria.lower() + # The four canonical NACK criteria from the issue must all + # appear, scoped to the §11 section to prevent unrelated + # mentions elsewhere from masking deletion. + section_start = criteria.index("### 11.") + section = criteria[section_start:] + assert "file-categories" in section + assert "deletion-heavy" in section + assert "new-API" in section + assert "commit-propose-revise" in section + assert "independent task groups" in section + + def test_reviewer_plan_prep_drops_advisory_only_wording(self): + # _build_reviewer_preparation(reviewer_plan, plan) is what + # the reviewer sees while waiting for proposals. The old + # "advisory only — never NACK" line must not survive. + prep = _build_reviewer_preparation( + role_value="reviewer_plan", + phase="plan", + ) + assert "advisory only — never NACK" not in prep + assert "NEVER NACKs on size" not in prep + # New framing must be present. + assert "Slice-sizing NACK (hard, judgment-based" in prep + assert "#2809" in prep + + class TestRefinerOrientationSurfacesPrimitives: """Issue #2594 — refiner phase-orientation should ask the refiner to surface runtime-primitive assumptions at the phase_gate so the