diff --git a/orchestrator/routes/pipelines.py b/orchestrator/routes/pipelines.py index a6f3fa0be5..c422a06d8b 100644 --- a/orchestrator/routes/pipelines.py +++ b/orchestrator/routes/pipelines.py @@ -4483,8 +4483,15 @@ def _get_refine_review_criteria() -> str: "- Are open questions specific enough for a human to answer?\n" "- Do questions address genuine ambiguities?\n" "- Are questions actionable?\n" - "- Are ALL uncertainties and assumptions surfaced? The analysis should not " - "proceed with unvalidated assumptions when it could ask the human instead.\n\n" + "- **Does each question require a human, or could the planner decide it?** " + "NACK questions that ask about work decomposition / slice-DAG shape / " + "PR packaging — those belong to the plan phase's HITL gate, not the " + "refine gate. NACK questions about implementation strategy " + "(API shape, migration approach, fallback design, detector design) " + "unless the answer is a fact only the operator knows (product intent, " + "scope boundary, external commitment, user-visible behavior). Good " + "refine questions are about *what the problem is* and *what's in/out " + "of scope*; the planner handles *how to build it*.\n\n" "### 6. Recommendation Quality\n" "- Is there a clear recommended approach?\n" "- Is the recommendation justified with specific reasons?\n" @@ -11426,8 +11433,10 @@ def _build_phase_prompt( "4. Identify constraints and dependencies", "5. Consider multiple implementation approaches", "6. Recommend an approach with justification", - "7. Surface **all** questions and uncertainties that need human input " - "(do not self-limit — raise every ambiguity)", + "7. Surface the questions and uncertainties that genuinely need a " + "human to answer — see `## How to Populate Open Questions` below for " + "the filter (slice/PR packaging, implementation strategy, and " + "API/schema details belong to the planner, not the refiner)", "", "**IMPORTANT**: Do NOT create an implementation plan, task breakdown, " "or phased rollout. That is the **plan** phase's job. Stay focused on " @@ -11498,11 +11507,38 @@ def _build_phase_prompt( "subsection at the top of `## Open Questions` (one bullet per resolved " "item, citing the answer). Only register questions that go beyond what " "`## Additional Context` covers.\n", - "Surface **all** uncertainties, ambiguities, and assumptions that need " - "human input. Do not limit yourself to a small number — every genuine " - "ambiguity, missing requirement, unstated assumption, or design choice " - "that could go multiple ways should be raised here. It is far better to " - "ask too many questions than to proceed with incorrect assumptions.\n", + "Surface uncertainties, ambiguities, and assumptions **that genuinely " + "need a human to answer**. Filter ruthlessly: a good open question is " + "one the operator must answer because the answer changes what we're " + "building. A bad open question is one the planner phase will decide on " + "its own once it sees the analysis — those waste the operator's " + "attention and pre-anchor the planner. Err toward registering questions " + "about *what the problem actually is* and *what's in or out of scope* " + "rather than *how to build it*.\n", + "**Out of scope for refine open questions** — do NOT register decisions " + "about:\n" + "- **Work decomposition / slice-DAG shape / PR packaging** — " + "**Slice / PR packaging is NOT a refine-phase decision.** The " + "plan phase owns slice-DAG construction (see " + "`docs/architecture/slice-dag.md`) and the operator approves the " + "proposed slice shape at the plan HITL gate. Do not register " + "`add-decision` items asking how the work should be sliced, how " + "many PRs to ship, or which parts should run in parallel. If " + "the task obviously spans multiple parts, name them in Problem " + "Statement or Constraints — the planner will propose a shape " + "from the analysis it reads.\n" + "- **Implementation strategy choices** that the planner can decide " + 'from Problem Statement + Constraints (e.g. "which migration ' + 'approach", "which fallback design", "which detector shape"). ' + "Surface these as Options Considered / Recommended Approach in the " + "analysis prose, not as `add-decision` items.\n" + "- **API / schema details** the planner phase will work out once it " + "starts designing. If the operator must constrain the API shape, " + "frame it as a *constraint* in `## Constraints`, not an open question.\n" + "Register questions when the answer is a fact only the human knows " + "(product intent, scope boundaries, external commitments, " + "user-visible behavior) — not when the answer is a design call the " + "planner will make.\n", "**Multiple-choice questions** — RUN this command for each question " "where the human must pick from discrete options:", "```bash", @@ -11524,34 +11560,14 @@ def _build_phase_prompt( 'They edit the comment to add their responses and check "Submit ' 'feedback" when done. The pipeline will resume with the feedback ' "available in the contract.\n", - "**Work-decomposition decisions** — when the task spans multiple " - "independently-implementable parts, the question to register is " - "**how to decompose the work into slices**, not how to package PRs. " - "In egg, slices are the decomposition primitive: each slice has its " - "own integration branch, agent team, BRC consensus, and PR, and " - "sibling slices in the same wave run in **parallel** " - "(see `docs/architecture/slice-dag.md`). Slice count = PR count by " - "construction, so frame the decision on the slice-DAG shape and " - "annotate the PR consequence in parentheses — do not frame it on " - "the PR count. Never offer " - '"N sequential PRs"-style options: that wording forces serialization ' - "that the slice scheduler does not require and teaches the operator " - "the wrong mental model. Example:", - "```bash", - "egg-contract add-decision \\", - ' --question "How should this work be decomposed into slices?" \\', - " --options \\", - ' "Single slice: all parts ship together (1 PR)" \\', - ' "Two slices in parallel: [A] || [B+C] (2 PRs)" \\', - ' "Two slices with dependency: [A] -> [B] (2 PRs)" \\', - ' "Three slices fully parallel: [A], [B], [C] (3 PRs)" \\', - " --format markdown", - "```", - "Name the parts inside the brackets so the operator can see which " - "concrete work each slice owns. If a would-be slice has more than " - "one DAG parent, note it — the plan phase rejects multi-parent " - "slices and the planner will need to serialise the upstream cluster " - "into a chain.\n", + "**Advisory seam-listing is fine** — if the task obviously spans " + "independently-implementable parts, you MAY name them in Problem " + 'Statement or Constraints (e.g. "the change touches the gateway, ' + 'the orchestrator, and the sandbox") so the planner has the seam ' + "information. Make it **explicitly advisory**: the planner is free " + "to slice differently if it sees a better seam. Do not pre-number " + "parts as `slice-1 / slice-2`, do not draw a DAG, and do not pick " + "a 1-PR-vs-3-PR shape — those choices belong to the planner.\n", "**DO NOT:**", "- Write questions as plain markdown text without running " "`egg-contract add-decision` or `egg-contract add-feedback`", @@ -11595,9 +11611,15 @@ def _build_phase_prompt( "Create a detailed implementation plan, decomposing the work into " "slices per the slice-DAG guidance at the end of this section. The " "implement-phase pipeline ships each slice as its own stacked PR. " - "A single-slice plan is fine when the work is cohesive; multi-slice " - "plans are required when an upstream refine-phase work-decomposition " - "HITL decision selected a multi-slice shape.", + "**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.", "", "Steps:", "1. Review any prior analysis", @@ -11665,8 +11687,8 @@ def _build_phase_prompt( # ---------------------------------------------------- # #2137 — slice-DAG planner guidance (mirrors the # concurrent task_planner block; keep the two paths - # aligned so a refine-phase multi-slice HITL decision - # is honoured regardless of which planner runs). + # aligned so the slice-shape rules behave the same way + # regardless of which planner runs). # ---------------------------------------------------- "## Slice-DAG guidance (#2137)", "", @@ -13695,10 +13717,16 @@ def _build_agent_prompt( "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. A single-slice plan is fine when the " - "work is cohesive; multi-slice plans are required when an upstream " - "refine-phase work-decomposition HITL decision selected a " - "multi-slice shape.", + "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.", "", "Steps:", "1. Review the architecture analysis from the ARCHITECT agent", diff --git a/orchestrator/tests/test_pipeline_prompts.py b/orchestrator/tests/test_pipeline_prompts.py index 156845d3e8..594afe8bcb 100644 --- a/orchestrator/tests/test_pipeline_prompts.py +++ b/orchestrator/tests/test_pipeline_prompts.py @@ -33,6 +33,7 @@ _get_code_review_criteria, _get_contract_review_criteria, _get_plan_review_criteria, + _get_refine_review_criteria, _get_reviewer_scope_preamble, _read_shared_criteria, _read_tester_gaps, @@ -3196,10 +3197,16 @@ def test_meta_instructions_live_outside_template_fence(self): "egg-contract add-feedback", "**DO NOT:**", "Skip already-resolved questions", - "Surface **all** uncertainties", + "Surface uncertainties, ambiguities, and assumptions", "**Multiple-choice questions**", "**Open-ended questions**", "Transcribe this `## How to Populate Open Questions` section", + # #2793 added new meta-guidance about slice/PR packaging — these + # strings must also stay outside the template fence, otherwise the + # refiner may transcribe them into its analysis document. + "Out of scope for refine open questions", + "Slice / PR packaging is NOT a refine-phase decision", + "Advisory seam-listing is fine", ): assert needle not in body, ( f"meta-instruction {needle!r} leaked into template fence — " @@ -3218,18 +3225,17 @@ def test_prompt_labels_meta_section_distinctly(self): assert "How to Populate Open Questions" in body -class TestRefinePromptSliceDagFraming: - """Refine prompt must frame work-decomposition decisions in slice-DAG terms. +class TestRefinePromptDelegatesSlicingToPlanner: + """Refine prompt must NOT invite slice-DAG / PR-packaging decisions. - Regression for #2584: refiner used to register multi-part work-decomposition - decisions with options framed as PR count ("Two PRs: E first, then A+F", - "Three sequential PRs: E -> A -> F"). In egg, slices are the - work-decomposition primitive — each slice has its own branch + BRC consensus - + PR, and sibling slices in a wave run in parallel under the slice scheduler. - Slice count = PR count by construction, so the decision should name the - slice-DAG shape and annotate the PR consequence in parentheses; "N sequential - PRs" is doubly wrong because it forces serialization the scheduler does not - require. + Regression for #2793: the refiner used to register an `add-decision` item + asking the operator how the work should be sliced (options like "Single + slice", "Two slices in parallel", etc). That decision belongs to the plan + phase, which has its own HITL gate. Observed on pipeline `issue-2777`: + the refiner's draft was dominated by slice-shape options and the only + differences between them were decomposition flavors, pre-empting and + biasing the planner. Supersedes #2584's regression suite, which had + enforced the now-removed slice-DAG-options block. """ @staticmethod @@ -3242,52 +3248,153 @@ def _refine_prompt() -> str: issue_number=100, ) - def test_prompt_introduces_work_decomposition_section(self): + def test_prompt_does_not_invite_slice_dag_decision(self): prompt = self._refine_prompt() - assert "Work-decomposition decisions" in prompt + # The exact phrasings from the removed Work-decomposition decisions + # block — none of them should reappear. + for forbidden in ( + "Work-decomposition decisions", + "How should this work be decomposed into slices?", + "Single slice: all parts ship together (1 PR)", + "Two slices in parallel: [A] || [B+C] (2 PRs)", + "Three slices fully parallel: [A], [B], [C] (3 PRs)", + '"N sequential PRs"', + ): + assert forbidden not in prompt, ( + f"refiner prompt re-introduced slice-DAG decision phrasing " + f"{forbidden!r}; that decision belongs to the plan phase (#2793)" + ) - def test_prompt_frames_decomposition_on_slices_not_pr_count(self): + def test_prompt_states_slicing_is_planner_owned(self): prompt = self._refine_prompt() - # Slice-DAG vocabulary must appear in the work-decomposition guidance. - for needle in ( - "decomposition primitive", - "decomposed into slices", - "slice-dag.md", - "in parallel", - ): - assert needle in prompt, f"slice-DAG framing token {needle!r} missing" + # Positive assertion: the prompt must explicitly delegate slice/PR + # packaging to the planner so the refiner doesn't re-derive the + # behavior from training-data priors. + assert "Slice / PR packaging is NOT a refine-phase decision" in prompt + assert "plan phase owns slice-DAG construction" in prompt - def test_prompt_provides_slice_shaped_example_options(self): + def test_prompt_marks_seam_listing_as_advisory(self): prompt = self._refine_prompt() - # The worked egg-contract add-decision example should show options - # framed on slice-DAG shape with the PR count as an annotation. - assert "Single slice: all parts ship together (1 PR)" in prompt - assert "Two slices in parallel: [A] || [B+C] (2 PRs)" in prompt - assert "Two slices with dependency: [A] -> [B] (2 PRs)" in prompt - assert "Three slices fully parallel: [A], [B], [C] (3 PRs)" in prompt - - def test_prompt_warns_against_sequential_pr_framing(self): + # Advisory seam-listing is allowed — the refiner may name the + # components touched so the planner has seam context — but it + # must be **advisory**: no pre-numbered slices, no DAG, no PR + # count picks. The planner is free to slice differently. + assert "Advisory seam-listing is fine" in prompt + assert "planner is free to slice differently" in prompt + assert "Do not pre-number" in prompt + + def test_prompt_blocks_planner_shaped_open_questions(self): prompt = self._refine_prompt() - # "N sequential PRs" framing must be explicitly called out as wrong. - assert '"N sequential PRs"' in prompt - assert "the slice scheduler does not require" in prompt + # The Open Questions guidance must name the categories of question + # the refiner is NOT allowed to register. + assert "Out of scope for refine open questions" in prompt + for category in ( + "Work decomposition / slice-DAG shape / PR packaging", + "Implementation strategy choices", + "API / schema details", + ): + assert category in prompt, ( + f"refine open-question guardrail {category!r} missing — the " + "refiner needs an explicit list of question categories that " + "belong to the planner phase" + ) - def test_decomposition_guidance_lives_outside_template_fence(self): - prompt = self._refine_prompt() - body = TestRefinePromptTemplateFenceSeparation._template_fence_body(prompt) - # The decomposition guidance is meta-protocol — it must not leak - # into the analysis-document template body that the refiner copies. - for needle in ( - "Work-decomposition decisions", - "decomposition primitive", - "Single slice: all parts ship together (1 PR)", - "the slice scheduler does not require", + +class TestRefineReviewCriteriaKicksBackPlannerQuestions: + """Refine reviewer must NACK open questions that belong to the planner. + + Regression for #2793: without this guardrail in the reviewer rubric, + the reviewer rubber-stamps planner-shaped open questions (slice-DAG + shape, implementation strategy) and the operator gets prompted for + decisions the planner should make. + """ + + def test_rubric_names_planner_shaped_question_categories(self): + criteria = _get_refine_review_criteria() + # The rubric must tell the reviewer to NACK these specific categories. + assert "work decomposition" in criteria + assert "slice-DAG shape" in criteria + assert "PR packaging" in criteria + # And it must give the reviewer the positive contrast — what a + # good refine question looks like. + assert "fact only the operator knows" in criteria + + +class TestPlannerPromptTreatsRefinerSlicingAsAdvisory: + """Planner prompts must frame any refiner-side slice sketch as advisory. + + 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. + """ + + @staticmethod + def _sequential_plan_prompt() -> str: + return _build_phase_prompt( + phase="plan", + pipeline_id="test-pipe", + pipeline_mode="issue", + prompt="Implement the change.", + issue_number=100, + ) + + @staticmethod + def _concurrent_planner_prompt() -> str: + return _build_agent_prompt( + role_value="task_planner", + phase="plan", + pipeline_id="test-pipe", + pipeline_mode="issue", + prompt="Implement the change.", + issue_number=100, + concurrent=True, + ) + + def test_sequential_planner_drops_refine_hitl_required_framing(self): + prompt = self._sequential_plan_prompt() + # The old "multi-slice plans are required when an upstream refine-phase + # work-decomposition HITL decision selected …" framing must be gone. + for forbidden in ( + "refine-phase work-decomposition HITL decision selected", + "multi-slice plans are required when an upstream", ): - assert needle not in body, ( - f"decomposition guidance {needle!r} leaked into template " - "fence — refiner may transcribe it into the analysis document" + assert forbidden not in prompt, ( + f"sequential planner still carries removed framing " + f"{forbidden!r}; the refiner no longer registers that " + "decision (#2793)" + ) + + def test_concurrent_planner_drops_refine_hitl_required_framing(self): + prompt = self._concurrent_planner_prompt() + for forbidden in ( + "refine-phase work-decomposition HITL decision selected", + "multi-slice plans are required when an upstream", + ): + assert forbidden not in prompt, ( + f"concurrent planner still carries removed framing " + f"{forbidden!r}; the refiner no longer registers that " + "decision (#2793)" ) + def test_sequential_planner_states_slice_shape_is_planner_call(self): + prompt = self._sequential_plan_prompt() + assert "Slice shape is your call" in prompt + assert "advisory context" in prompt + # The operator-HITL escape hatch must be present with push-back rights. + 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): + 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 + class TestPlannerPromptSliceDagFraming: """Planner prompts must not contradict slice-DAG decomposition (#2601). diff --git a/plugins/refine-plan/skills/refine-plan/agents/refiner.md b/plugins/refine-plan/skills/refine-plan/agents/refiner.md index 1ab6dad5f1..3afee51ca6 100644 --- a/plugins/refine-plan/skills/refine-plan/agents/refiner.md +++ b/plugins/refine-plan/skills/refine-plan/agents/refiner.md @@ -208,11 +208,27 @@ Each question must be specific enough for a human to answer in one decision. 3. For third-party libraries / APIs / integrations, use WebSearch and WebFetch. 4. Identify at least two meaningfully different options (not three flavors of the same idea). 5. Recommend one option with explicit justification grounded in the constraints. -6. Surface every uncertainty as an Open Question — do not self-limit. +6. Surface questions only where the answer is a fact only the operator knows + (product intent, scope boundaries, external commitments, user-visible + behavior). See "What you do not do" below for the categories that belong + to the planner phase, not the refiner. ## What you do not do - Do not write implementation phases, slices, or task breakdowns +- Do not register `egg-contract add-decision` items about work decomposition, + slice-DAG shape, or PR packaging — the plan phase owns slice construction + and has its own HITL gate. If the task obviously spans multiple parts, + you MAY name them in Problem Statement or Constraints as **advisory seam + information** for the planner — but do not pre-number them as + `slice-1 / slice-2`, do not draw a DAG, and do not pick a 1-PR-vs-3-PR + shape. The planner is free to slice differently if it sees a better seam. +- Do not register questions about implementation strategy (API shape, + migration approach, fallback design, detector design) unless the answer + is a fact only the operator knows (product intent, scope boundary, + external commitment, user-visible behavior). Surface those as Options + Considered / Recommended Approach in the analysis prose, not as + open-question decisions. - Do not modify source code, tests, or docs in this phase - Do not propose changes you have not verified are necessary by reading the relevant code diff --git a/plugins/refine-plan/skills/refine-plan/agents/reviewer-refine.md b/plugins/refine-plan/skills/refine-plan/agents/reviewer-refine.md index 1534a10408..8e3484d3c4 100644 --- a/plugins/refine-plan/skills/refine-plan/agents/reviewer-refine.md +++ b/plugins/refine-plan/skills/refine-plan/agents/reviewer-refine.md @@ -62,3 +62,11 @@ If the Task context includes `prior_nacks`, verify each prior cycle's NACK is no - Speculative scope: bundled rewrites, fixes for adjacent issues the brief didn't ask about - "Generic best practice" pros/cons that don't specifically engage with this codebase - Open questions that are actually decisions the refiner should have made +- **Planner-shaped open questions**: NACK questions that ask the operator about + work decomposition, slice-DAG shape, PR packaging, or implementation strategy + (API shape, migration approach, fallback design, detector design). Those + belong to the plan phase's HITL gate. Good refine questions are about *what + the problem is* and *what's in/out of scope* — facts only the operator + knows. Cite the offending decision-ID in `feedback` and tell the refiner to + either drop the decision or fold the relevant context into Problem Statement + / Constraints.