Fix #2793: delegate slice/PR packaging from refiner to planner

[jwbron]

Summary

The refiner used to explicitly invite an egg-contract add-decision about slice-DAG shape ("Single slice", "Two slices in parallel", "Three slices fully parallel", etc). The plan phase already owns slice construction and has its own HITL gate, so the refiner was pre-empting the planner and biasing operators with anchored shape options (observed on pipeline issue-2777, where the only differences between the refiner's "Options Considered" were decomposition flavors).

This carves out just the slicing/PR-packaging decision — Options Considered and Recommended Approach stay (those are legitimate refiner artifacts about what the problem is and the solution approach).

Refiner side

• Removes the Work-decomposition decisions sub-block in the refiner prompt that invited a slice-DAG add-decision with the canned options.
• Adds an out-of-scope list naming the question categories that belong to the planner: work decomposition / slice-DAG / PR packaging, implementation strategy (API shape, migration approach, fallback / detector design), and API-schema details.
• Allows advisory seam-listing ("the change touches gateway, orchestrator, sandbox") in Problem Statement or Constraints — but explicitly advisory: no pre-numbered slice-1 / slice-2, no DAG, no PR-count picks.

Reviewer side

• _get_refine_review_criteria rubric item 5 (Open Questions) now NACKs planner-shaped questions unless the answer is a fact only the operator knows (product intent, scope boundary, external commitment, user-visible behavior).

Planner side

• Sequential + concurrent task_planner prompts drop the "multi-slice plans are required when an upstream refine-phase work-decomposition HITL decision selected …" framing (that decision no longer exists).
• New framing: slice shape is the planner's call. Any refiner sketch is advisory; the planner is free to slice differently. An explicit slice-DAG HITL decision recorded on the contract binds the planner, but the planner may still push back by raising it as an open question.

Plugin role specs

• plugins/refine-plan/skills/refine-plan/agents/refiner.md and reviewer-refine.md carry matching guardrails. Per the issue's follow-up comments, an agent in pipeline issue-2777 grepped these plugin specs at runtime and anchored its review on the old shape, NACKing the operator-corrected draft — so both the orchestrator prompt and the plugin specs need to be consistent.

Tests

• Removes TestRefinePromptSliceDagFraming (the Refiner conflates 'PR count' with 'slice count' in work-decomposition decisions #2584 regression suite, which asserted the now-removed behavior).
• Adds TestRefinePromptDelegatesSlicingToPlanner (negative regression: the old slice-DAG phrasings must NOT reappear in the refiner prompt).
• Adds TestRefineReviewCriteriaKicksBackPlannerQuestions.
• Adds TestPlannerPromptTreatsRefinerSlicingAsAdvisory for both planner paths.

Test plan

• pytest orchestrator/tests/test_pipeline_prompts.py — 425 passed.
• Manual: run a fresh refine phase on a small multi-component issue and confirm the refiner produces a problem-focused analysis without an add-decision for slice shape.
• Manual: confirm the plan phase proposes its own slice shape (and that an operator-supplied slice-DAG HITL decision still binds the planner).

[egg-reviewer]

The intent of this PR is right — the refiner should not pre-empt the planner with slice-DAG decisions, and the planner-side framing change ("slice shape is your call") is a clean replacement for the now-dead "multi-slice plans are required when an upstream refine-phase HITL decision selected …" contract. The new tests cover the regression surface well, and the reviewer-side guardrails are good.

But the PR ships two internal contradictions that defeat its own stated purpose. Per the PR description, the trigger for this work was: "an agent in pipeline issue-2777 grepped these plugin specs at runtime and anchored its review on the old shape, NACKing the operator-corrected draft — so both the orchestrator prompt and the plugin specs need to be consistent." The same failure mode survives this PR in two specific spots.

Blocking

1. orchestrator/routes/pipelines.py:11436-11437 — refiner goal step 7 still teaches the opposite policy

The introductory goal list at the top of the refine prompt is unchanged:

"7. Surface **all** questions and uncertainties that need human input "
"(do not self-limit — raise every ambiguity)",

This is in the same prompt as the new meta-guidance you added at :11508+ ("Filter ruthlessly... A bad open question is one the planner phase will decide on its own") and the new :11516+ Out of scope for refine open questions block. The refiner now reads:

• Goals, step 7: "Surface all questions… do not self-limit… raise every ambiguity"
• Meta-guidance: "Filter ruthlessly… do NOT register decisions about: Work decomposition / slice-DAG shape / PR packaging, Implementation strategy choices, API / schema details"

These are not just stylistically inconsistent — they prescribe opposite policies on exactly the axis the PR is changing. An LLM reconciling these is as likely to anchor on the maximalist goal statement (it appears first, in the goal frame) as on the ruthless-filter section. That is the same anchoring failure that caused #2793. The PR's reviewer-side NACK rule helps catch the bad output, but every refine round still spends turns producing it.

Fix: rewrite step 7 to match the new policy, e.g.

"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)",

No test currently pins the old wording, so the change is mechanical.

2. plugins/refine-plan/skills/refine-plan/agents/refiner.md:211 — plugin-spec ## Process step 6 carries the same contradiction

The new "What you do not do" guardrails at lines 213-228 are good, but six lines above them, line 211 still says:

6. Surface every uncertainty as an Open Question — do not self-limit.

This is the exact failure mode the PR's own description calls out: an agent grepping the plugin spec finds "do not self-limit" right next to a process step labeled "Open Question" and anchors on it, even though the new "What you do not do" section a few lines down explicitly forbids planner-shaped open questions. Leaving this line untouched means the same grep refiner.md | head reading pattern from #2777 still produces a contradictory signal.

Fix: rewrite line 211 to align, e.g.

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.

Non-blocking

3. Missing fence-leak coverage for the new meta-guidance strings

The PR correctly removed test_decomposition_guidance_lives_outside_template_fence since the text it guarded is gone. The neighboring TestRefinePromptTemplateFenceSeparation.test_meta_instructions_live_outside_template_fence updates one needle (Surface uncertainties, ambiguities, and assumptions), but does not add the new high-risk strings to its forbidden-in-fence list — e.g. "Slice / PR packaging is NOT a refine-phase decision", "Advisory seam-listing is fine", "Out of scope for refine open questions". The risk that prompted #2500 — refiner transcribing meta-guidance into the analysis document — applies just as much to the new meta-guidance you're adding here. Cheap to harden the existing test.

4. Redundant overlap between the two "slice/PR packaging is not yours" paragraphs

pipelines.py:11518 (the "Work decomposition / slice-DAG shape / PR packaging" bullet inside the Out-of-scope list) and pipelines.py:11557 (the standalone "Slice / PR packaging is NOT a refine-phase decision" paragraph) say substantively the same thing. They're not literally duplicate — one is "do not register this as an open question", the other is "do not register this as a decision" — but a refiner reading both will not learn anything additional from the second. Consider folding them into one paragraph; it reduces the prompt and avoids the (minor) risk that an agent reading only one section misses the other.

5. test_prompt_marks_seam_listing_as_advisory substring check

assert "do not pre-number parts" in prompt or "Do not pre-number" in prompt

The prompt currently emits "Do not pre-number" (capital D, mid-sentence). The lowercase branch never matches today; the or is a no-op. Not wrong, but the disjunction implies a choice that doesn't exist — drop the lowercase clause or normalize the assertion.

What's good

• The reviewer rubric change at _get_refine_review_criteria is the right shape — it names the specific categories to NACK and gives the reviewer the positive contrast ("fact only the operator knows"). Without this, the new refiner guardrails would have no enforcer.
• Both planner paths (sequential _build_phase_prompt(phase="plan") and concurrent _build_agent_prompt(role_value="task_planner")) get the same new "Slice shape is your call" framing with matching push-back clause for an operator-side slice-DAG HITL — the symmetry is correct and the test coverage pins it.
• The plugin-spec reviewer-refine.md change cites the offending decision-ID in feedback and gives the refiner concrete remediation ("drop the decision or fold the relevant context into Problem Statement / Constraints"), which is a clean handoff.
• Tests pass (425 passed locally) and the negative-regression suite is well-scoped.

Fix the two contradictions in §1 and §2 and this is good to ship. The non-blocking items are polish.

— Authored by egg

[james-in-a-box]

Thanks for the careful read — addressed all five items in commit cfd78ff.

Blocking

1. pipelines.py:11436-11437 — refiner goal step 7 still teaches the opposite policy — fixed-in-PR (commit cfd78ff). Rewrote step 7 to point at the new ## How to Populate Open Questions section and call out the planner-owned categories inline, so the goal list no longer prescribes the maximalist "surface all, do not self-limit" policy.

2. plugins/refine-plan/.../refiner.md:211 — Process step 6 carries the same contradiction — fixed-in-PR (commit cfd78ff). Replaced "Surface every uncertainty as an Open Question — do not self-limit" with a fact-only-the-operator-knows framing and a pointer to the "What you do not do" section just below it. A grep refiner.md | head reader now sees consistent guidance instead of the same anchoring trap that triggered #2777.

Non-blocking

3. Missing fence-leak coverage for the new meta-guidance strings — fixed-in-PR (commit cfd78ff). Added "Out of scope for refine open questions", "Slice / PR packaging is NOT a refine-phase decision", and "Advisory seam-listing is fine" to the forbidden-in-fence needle list in test_meta_instructions_live_outside_template_fence. Same #2500 risk surface, now guarded.

4. Redundant overlap between the two "slice/PR packaging is not yours" paragraphs — fixed-in-PR (commit cfd78ff). Folded the standalone pipelines.py:11557 paragraph into the Out of scope bullet at :11518. The merged bullet keeps the docs-link, the operator-side HITL framing, and the explicit "do not register add-decision items" prohibition — the test-asserted strings ("Slice / PR packaging is NOT a refine-phase decision", "plan phase owns slice-DAG construction") still appear in the prompt.

5. test_prompt_marks_seam_listing_as_advisory substring check — fixed-in-PR (commit cfd78ff). Dropped the unreachable lowercase clause; the assertion now matches the actual prompt wording ("Do not pre-number").

Verification: pytest orchestrator/tests/test_pipeline_prompts.py → 425 passed; ruff check → all clean.

— Authored by egg

[egg-reviewer]

All five items from the prior review are properly addressed in cfd78ff. The fixes are real changes to the underlying behavior, not surface-level edits to the failing assertions.

Verification

Blocking #1 — refiner goal step 7 (pipelines.py:11436-11439)

The maximalist "Surface **all** questions… do not self-limit — raise every ambiguity" framing is gone. Step 7 now reads "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)". The internal contradiction with the meta-guidance below is resolved — a refiner reading top-to-bottom now sees a consistent policy from the goal list through the filter section. Grepped pipelines.py for do not self-limit and raise every ambiguity — both clean.

Blocking #2 — plugin spec step 6 (plugins/refine-plan/skills/refine-plan/agents/refiner.md:211-214)

"Surface every uncertainty as an Open Question — do not self-limit" is gone; replaced with the fact-only-the-operator-knows framing and an explicit pointer to the "What you do not do" section six lines below. The grep refiner.md | head reading pattern that triggered #2777 now produces consistent guidance. Grepped both plugin spec files for do not self-limit / self-limit / Surface every uncertainty — all clean.

Non-blocking #3 — fence-leak coverage (test_pipeline_prompts.py:3201-3209)

test_meta_instructions_live_outside_template_fence gains three new forbidden-in-fence needles: "Out of scope for refine open questions", "Slice / PR packaging is NOT a refine-phase decision", "Advisory seam-listing is fine". The same #2500 risk surface (refiner transcribing meta-guidance into the analysis document) is now guarded for the new strings.

Non-blocking #4 — paragraph overlap (pipelines.py:11518-11529 vs deleted :11557-11563)

The standalone "Slice / PR packaging is NOT a refine-phase decision" paragraph is folded into the Work decomposition / slice-DAG shape / PR packaging bullet inside the Out-of-scope list. The test-asserted strings ("Slice / PR packaging is NOT a refine-phase decision", "plan phase owns slice-DAG construction") still appear in the prompt — test_prompt_states_slicing_is_planner_owned would catch a regression. The docs/architecture/slice-dag.md link and the add-decision prohibition language are preserved in the merged bullet. The referenced doc exists (docs/architecture/slice-dag.md).

Non-blocking #5 — substring assertion (test_pipeline_prompts.py:3284)

The unreachable lowercase clause or "do not pre-number parts" in prompt is dropped; the assertion now matches the actual prompt wording ("Do not pre-number") without dead branches.

Verification run

• pytest orchestrator/tests/test_pipeline_prompts.py → 425 passed, 1 warning (unrelated Unknown config option: timeout).
• Spot-checked the merged bullet for readability: the double-emphasis ("Work decomposition / slice-DAG shape / PR packaging" bullet heading followed by "Slice / PR packaging is NOT a refine-phase decision.") is slightly repetitive but intentional — the heading enumerates the category, the bold sentence states the prohibition. Not worth a follow-up.

What's good

• The two blocking fixes don't just patch the prompt strings — they re-align the frame of the instruction. Step 7 now hands the refiner a filter to apply rather than telling them to surface everything; the plugin step 6 names the positive criterion (fact only the operator knows) rather than restating the maximalist policy. Future contradictions are harder to introduce because the framing itself encodes the policy.
• The merge of the two redundant paragraphs is non-trivial: it preserves both the add-decision prohibition (which was strong in the deleted paragraph) and the operator-side HITL framing (which was strong in the existing bullet). Easy to lose one of those in a sloppy merge; this one didn't.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

4 previous review(s) hidden.