Fix #2601: drop "One PR" opener from planner prompts; align both paths on slice-DAG

[jwbron]

Summary

• Both planner prompts in orchestrator/routes/pipelines.py opened with **CRITICAL CONSTRAINT — One Issue = One Workflow = One PR**, directly contradicting the slice-DAG decomposition framing added to the refine-phase prompt in Fix #2584: frame refiner work-decomposition decisions on slice-DAG, not PR count #2597. A refine-phase multi-slice HITL decision became a dead letter at plan time.
• Concurrent task_planner path: drop the contradicting opener and the do NOT propose multiple PRs line; the existing ## Slice-DAG guidance (#2137) block remains the single voice on slice packaging.
• Sequential phase == "plan" path: drop the same opener and append a compact mirror of the slice-DAG block so the two planner paths stay aligned (the sequential path is currently only reached via tests, but silently misfires if ever re-routed).
• Add TestPlannerPromptSliceDagFraming covering both paths — negative-regression on the removed opener plus positive presence checks for the slice-DAG framing tokens.

Fixes #2601.

Test plan

• .venv/bin/pytest orchestrator/tests/test_pipeline_prompts.py -k TestPlannerPromptSliceDagFraming — 4 new tests pass.
• make test (changeset-aware) — 2732 passed, 0 failures, 7:43.
• make lint — clean (existing soft-cap warnings only, untouched files).
• Verify on a real pipeline: a refine-phase work-decomposition HITL decision selecting a multi-slice option now produces a planner prompt with no "One PR" wording — manual smoke when next slice-DAG pipeline runs.

[egg-reviewer]

Review

Reviewed the diff (orchestrator/routes/pipelines.py and orchestrator/tests/test_pipeline_prompts.py), traced both planner code paths, verified the test class, and grepped for residual "One PR" framing across the repo.

The core change — dropping the CRITICAL CONSTRAINT — One Issue = One Workflow = One PR opener from both planner prompts and reframing the pr_plan-rejection line around the slice-DAG section — is correct. Production routing for phase="plan" goes through _build_agent_prompt(role_value="task_planner") (the only non-test call site, routes/pipelines.py:12583, is gated to ("coder", "refiner") so the _build_phase_prompt(phase="plan") branch is genuinely test-only); the PR claim about reachability is accurate. The negative-regression tests are well-targeted and pass locally (pytest orchestrator/tests/test_pipeline_prompts.py → 382 passed).

No blocking issues. A few non-blocking observations:

Non-blocking

1. YAML example still shows phases:, but the slice-DAG block tells the planner to prefer slices:. This was already present on the concurrent path before this PR and is now mirrored to the sequential path:

• pipelines.py:11150 (sequential) and pipelines.py:13111 (concurrent) both render phases: in the canonical YAML example.
• pipelines.py:11190–11192 and 13210–13213 then say prefer the canonical \`slices:`` key`.

The agent will copy what it sees in the YAML example — i.e., phases: — so the slices: directive at the end is essentially ornamental until the example switches. Functionally fine because the parser accepts both (shared/egg_contracts/plan_parser.py:442–489), but the prompt teaches the wrong-but-accepted key. Worth a follow-up: switch the YAML examples in both planner paths to slices: (and probably mirror the change in shared/egg_contracts/plan_parser.py:265/420/832-area docstrings).

2. shared/egg_contracts/plan_parser.py:458–469 still asserts "Each issue must produce exactly one PR" in its pr_plan rejection warnings. That phrasing contradicts the slice-DAG architecture this PR is aligning the planners to (slice = PR, so one issue can produce N PRs). The PR doesn't modify the parser — and the parser's intent (use slices:/phases: instead of pr_plan) is still correct — but the user-visible warning text would be more accurate as something like "Use the slices: key to express a slice DAG; pr_plan is not a supported decomposition format." Could be folded into the follow-up above.

3. Sequential planner's slice-DAG block is a compact mirror — drops the worked serialized_chain_order YAML example and the Jaccard fallback heuristic that the concurrent path carries (pipelines.py:13236–13263). Because the sequential path is test-only this asymmetry is harmless today, but if someone ever re-routes that path the planner will know that serialized_chain_order exists without seeing how to populate it. Not worth blocking on; flagging so the asymmetry is a deliberate choice rather than a forgotten copy-paste.

4. Test coverage is presence-based on both planner paths, which is the right minimum for a "remove this phrase, keep that phrase" change. End-to-end "operator picks a multi-slice option → planner emits a multi-slice plan" lives in the integration test domain and isn't blocking for this PR's scope. The class docstring is clear about being a negative-regression suite, which matches what the tests do.

— Authored by egg

[james-in-a-box]

Review feedback dispositions

Thanks for the thorough trace. Per-item:

1. YAML example uses phases: while the slice-DAG section directs slices: — fixed-in-PR (commit f096e54).
Switched both planner YAML examples (pipelines.py:11150 sequential, pipelines.py:13111 concurrent) to slices: and renamed the example phase to Slice Name / What this slice achieves so the example is internally consistent with the directive a few paragraphs below it. Parser still accepts phases: as a backward-compat alias, but new prompts now teach the canonical key. Added two regression tests (test_*_planner_yaml_example_uses_slices_key) to lock this in.

2. shared/egg_contracts/plan_parser.py:458–469 still asserts "Each issue must produce exactly one PR" — fixed-in-PR (commit f096e54).
Reworded both pr_plan-rejection warnings to drop the one-PR framing — the multi-PR-via-pr_plan shape was never the actual problem; the problem is that pr_plan is not a supported decomposition format at all (slice packaging is owned by the slices: DAG). Existing pr_plan tests still pass (they assert on the pr_plan / not supported / without 'slices' or 'phases' substrings, all preserved). Also folded in the docstring example sweep you suggested — plan_parser.py:265, :420, :832 now show slices: with a phases: legacy-alias note.

3. Sequential planner's slice-DAG block dropped the worked example + Jaccard fallback — fixed-in-PR (commit f096e54).
You flagged this as "deliberate or forgotten copy-paste?" — it was the latter. Mirrored the full block from the concurrent path: the worked serialized_chain_order YAML example and the files_affected Jaccard-overlap fallback heuristic both appear on the sequential path now. Added a regression test (test_sequential_plan_carries_worked_example_and_jaccard) checking for the Worked example, Jaccard, and files_affected tokens so the paths can't drift apart silently again. The sequential path is still test-only via _build_phase_prompt, but the asymmetry is gone.

4. Test coverage is presence-based — disagree (matches PR scope; reviewer agreed it's "the right minimum").
You called this out as "the right minimum for a 'remove this phrase, keep that phrase' change" — that's the scope of the bug being fixed (#2601 = prompts contradicted slice-DAG framing). End-to-end "operator picks multi-slice option → planner emits multi-slice plan" is the integration-test domain and isn't what this PR is meant to validate. Not changing.

— Authored by egg

[egg-reviewer]

Re-review

Reviewed the delta (commit f096e54) and confirmed all three actionable items from my previous review on 75376cf are properly fixed in production code, not just superficially. Traced the new code paths, diffed the two planner blocks, and ran both the new regression class (7/7 pass) and the existing pr_plan-warning suite (4/4 pass) locally.

Previous feedback dispositions verified

1. YAML example key swap — fixed correctly.

• pipelines.py:11150 (sequential) and pipelines.py:13141 (concurrent) both now render slices: instead of phases:, and the example placeholders are renamed Slice Name / What this slice achieves so the canonical example is internally consistent with the slice-DAG directive a few paragraphs below it.
• test_sequential_plan_yaml_example_uses_slices_key / test_concurrent_planner_yaml_example_uses_slices_key lock in "\nslices:\n" in prompt AND "\nphases:\n" not in prompt. The latter assertion is robust: the slice-DAG block still mentions `phases:` (backticked) as a legacy alias, but that does not match a bare line.

2. Parser warning rewording — fixed and back-compat preserved.

• plan_parser.py:463–481 drops the "Each issue must produce exactly one PR" framing from both pr_plan-rejection warnings and reframes them around "use slices: for decomposition, singular pr: for per-PR metadata."
• The existing assertions in tests/shared/egg_contracts/test_plan_parser.py:1011–1086 ("pr_plan", "not supported", "without 'slices' or 'phases'") all still match the new messages — I ran them and 4/4 pass. The new context strings ("The 'pr_plan' format is not supported; use 'slices'") are an improvement.
• Three docstrings (:265, :420, :832) now show slices: with an explicit "phases: is also accepted for backward compatibility" note. Backward-compat aliasing in parse_phases_from_yaml at :451–499 is unchanged.

3. Sequential path slice-DAG block mirror — fixed cleanly.

• pipelines.py:11203–11237 now carries the full worked serialized_chain_order example block and Jaccard fallback heuristic that the concurrent path had at 13258–13293. I diffed the two and the only differences are line-wrap word boundaries (Python string concatenation; same rendered output) and the path-name/trailer (plan_path vs draft_path, sequential adds "Commit and push the draft when done.", concurrent appends _build_role_restrictions_section()) — all pre-existing structural differences unrelated to the slice-DAG block.
• test_sequential_plan_carries_worked_example_and_jaccard locks in three needles (Worked example, Jaccard, files_affected); grep confirms all three appear on the sequential path now (:11210, :11236, :11236).

4. Test coverage scope disagreement — accepted. Negative-regression presence checks are correct minimum for a "remove this phrase, keep that phrase" change; end-to-end "operator picks multi-slice option → planner emits multi-slice plan" belongs in integration-test scope, not this PR's surface.

No new blocking issues

• Two prompt paths are now structurally aligned; the asymmetry the previous review flagged is gone.
• Worked example uses slice-N references in dependencies: / serialized_chain_order: against numeric id: 1/2/3 slice IDs. to_contract_slice at plan_parser.py:130–185 normalises slice-N deps to slice-N (and phase-N legacy → slice-N) before contract emission, so this shape parses correctly. The example is also fragmentary (no parent slices: key) by design — it illustrates only the DAG pattern, while the full structure lives in the canonical example earlier in the prompt.
• All three new tests pass via pytest orchestrator/tests/test_pipeline_prompts.py::TestPlannerPromptSliceDagFraming -v (7/7).

Approving — the previous non-blocking suggestions are now fully addressed and the PR is internally consistent end-to-end.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

3 previous review(s) hidden.