Fix #2538: slice PRs carry contract.pr narrative on every slice

[jwbron]

Summary

• Every slice PR — terminal and non-terminal — now renders the planner-authored contract.pr narrative (title, description, test plan, manual steps), so reviewers see program rationale on whichever slice PR they open first. Previously the narrative lived only on the terminal slice — the last-to-merge PR — leaving reviewers of slice-1 (the canonical merge entry point) with no context.
• Non-terminal slice titles get a [<slice-id>] prefix (e.g. [slice-1] Wire integration tests into PR CI…) so the GitHub PR list stays scannable. The terminal slice keeps the bare program_title and an "umbrella / merge gate" banner.
• Per-merge obligations remain terminal-only — the merge gate is the last-to-merge PR — so the existing slice-DAG umbrella PR doesn't surface contract.pr.deferred_actions #2354 invariant and fail-fast assertion are preserved.
• Drops the old "see terminal slice's PR for the program-level narrative" pointer; the narrative is right there now.

Fixes #2538.

Body shape

[terminal-only] > Program-level umbrella PR — merge gate banner

<program description>

[terminal-only, when present] ## ⚠️ Pre-merge Obligations / ## ✅ Resolved within this PR

## This slice
<slice name>

Tasks:
- task-N-1: …

## Test Plan
<program test plan>

## Manual Steps
<program manual steps>

Slice <id> of pipeline <pipeline_id>. Stacked on top of `<base>`.

Obligations sit immediately after the program description (preserves the original #2354 visibility intent — merge-blocking banner stays high in the body).

Test plan

• Updated unit tests pass: TestCreateSlicePR in orchestrator/tests/test_gateway_client.py (renamed test_non_terminal_slice_uses_auto_generated_title_and_body → test_no_contract_pr_falls_back_to_auto_generated_title_and_body; replaced test_non_terminal_slice_with_terminal_pointer_includes_pointer_line with test_non_terminal_slice_carries_program_narrative_with_slice_id_prefix exercising the new shape; updated test_terminal_slice_uses_program_title_and_planner_authored_body to assert per-slice section).
• Updated integration test passes: test_terminal_slice_pr_carries_program_metadata_non_terminal_does_not → test_every_slice_pr_carries_program_metadata in orchestrator/tests/test_slice_run_loop_integration.py.
• Existing terminal-only obligations tests still pass (the renderer still gates program_deferred_actions on the terminal slice; test_non_terminal_slice_with_program_deferred_actions_raises still triggers the assertion when program_title is None).
• Manual verification once a slice-DAG pipeline runs end-to-end: every slice PR (slice-1 through slice-N) shows the program description, test plan, manual steps, and the per-slice scope. Only the terminal slice carries the obligations section + umbrella banner.

Out of scope

• agent:coder mislabeling on slice PRs (Slice PRs mislabeled as agent:coder and tagged agent_role=coder in pipeline-context comment #2537).
• slice slice-1 title duplication when the planner skips pr.title — the fallback path still produces slice {slice_id}: {slice_name}. Tracked separately under whatever issue replaces the #YYYY placeholder in Slice PRs don't use contract's pr.title / pr.description; narrative buried on terminal slice #2538.

[egg-reviewer]

No agent-mode design concerns. This PR is concerned with rendering markdown PR bodies for human reviewers (slice-PR title prefixing and per-slice body sections); it doesn't touch agent prompts, model identifiers, the Agent SDK boundary, or any pre-fetching into LLM context.

— Authored by egg

[egg-reviewer]

The change correctly delivers the stated goal: every slice PR now carries the planner-authored narrative (description / test plan / manual steps), the terminal slice keeps the umbrella banner + obligations as the merge gate, and non-terminal slices get a [<slice-id>] title prefix so the GitHub PR list stays scannable. The integration test (test_every_slice_pr_carries_program_metadata) and the gateway-level body-shape tests cover the main cases, the doc updates are in sync, and CI is green.

I did find one defense-in-depth gap that's worth fixing while you're in this code, plus a few smaller observations. None of these block the core feature, but the assertion gap below is the kind of thing that turns into a regression a year from now if left.

Non-blocking findings

1. Assertion guard for "obligations are terminal-only" is now incomplete

File: orchestrator/gateway_client.py:1271-1296 and 1321-1333

The docstring on create_slice_pr and the inline comment claim:

program_deferred_actions is terminal-only by convention … the assertion below fails fast if a caller wires obligations through to a non-terminal slice (#2354 review nit).

That was true under the old code, where has_program_block (the old name) implied terminal. The new code introduces a third state — non-terminal with program_title set (has_program_title and terminal_slice_id is not None) — and the assertion was not extended to cover it.

Trace, with program_title="X", terminal_slice_id="slice-3", program_deferred_actions=[…]:

• has_program_title = True
• is_terminal_slice = True and ("slice-3" is None) = False
• The upper branch (if has_program_title:) executes — no assertion on program_deferred_actions in this branch.
• if program_deferred_actions: is truthy → the ## ⚠️ Pre-merge Obligations section is rendered into a non-terminal PR body, without the merge-gate / umbrella banner (the banner is gated on is_terminal_slice).

That's exactly the failure mode the assertion was added to catch. The production caller in _run_one_slice_inner correctly gates obligations on is_terminal, so this isn't a live bug today — but the defensive guard the author advertises in the docstring no longer catches one of two non-terminal entry shapes.

Suggested fix — hoist the assertion above the branch so it covers both shapes uniformly:

has_program_title = bool(program_title and program_title.strip())
is_terminal_slice = has_program_title and terminal_slice_id is None

# Obligations are terminal-only by convention. Catch all caller
# mistakes regardless of which title shape they use.
if not is_terminal_slice:
    assert program_deferred_actions is None, (
        "program_deferred_actions must be None on non-terminal slices; "
        "obligations belong on the umbrella PR only"
    )

This collapses the existing guard and covers the new state, and lets you delete the duplicated comment block in the else branch. (You'd lose the whitespace-title carve-out from observation B in #2354, but that observation is now structurally moot — is_terminal_slice is False for a whitespace title regardless of the routing intent, so the assertion firing is the correct outcome, not a spurious one.)

If you'd rather keep the current shape, please at least add a parallel assertion at the top of the if has_program_title: branch (if not is_terminal_slice: assert program_deferred_actions is None, …) and add a regression test for the non-terminal+program_title+obligations shape — symmetric to the existing test_non_terminal_slice_with_program_deferred_actions_raises.

2. Whitespace-only program_title silently drops the entire planner narrative and obligations

Files: orchestrator/gateway_client.py:1263, 1278-1296, shared/egg_contracts/models.py:374

PRMetadata.title is declared with min_length=1 and no .strip() validator, so title=" " is a structurally valid contract value. With that in the contract:

• has_program_title = bool(" " and "") = False
• Falls to the deterministic else branch
• program_title is None is False, so the assertion is skipped (per the comment, deliberately)
• Title becomes slice slice-N: <slice_name> and the body is task bullets only — program_description, program_test_plan, program_manual_steps, and any program_deferred_actions are silently dropped

The author's comment defends this as "the whitespace-title bug, if any, is for PRMetadata to catch — not the slice PR builder." That's a defensible split of responsibilities, but PRMetadata.title's validator does not in fact catch it. So the net effect today is silent data loss for the planner's intended PR body.

This is pre-existing (the prior implementation had the same drop), but since this PR is touching the surrounding logic and explicitly calls out the whitespace case in a comment, the natural place to follow up is to tighten PRMetadata.title (e.g. add @field_validator("title") that rejects strings that strip to empty, or change to min_length=1, strip_whitespace=True). Worth filing as a small follow-up if you don't want to touch it here.

3. No multi-tree-forest test exercises the documented behavior

File: docs/architecture/slice-dag.md:504-524

The doc was updated to call out: in a forest with multiple terminal leaves, only terminal_ids[-1] (chosen_terminal) gets bare program_title + umbrella banner + obligations; other terminal leaves are treated as non-terminals and get [<slice-id>] <program_title> and skip the banner. There's no integration test that exercises this — test_every_slice_pr_carries_program_metadata uses a linear slice-1 → slice-2 → slice-3 chain with a single terminal.

Since #2538 documents this as deliberate, a small forest test (e.g. two trees with separate leaves slice-2 and slice-3, asserting only slice-3 gets terminal_slice_id=None and the bare title shape) would lock the behavior in. Pre-existing gap, not a #2538 regression, but you're updating the section that makes the claim.

4. No truncation test for the non-terminal [<slice-id>] title shape

File: orchestrator/tests/test_gateway_client.py

test_terminal_slice_truncates_long_program_title_to_70_chars covers the bare-title case. The [<slice-id>] prefix eats ~10 chars from the 70-char budget on non-terminals, and the truncation happens on the post-prefix concatenation. Worth a parallel regression guard so a future renaming of slice IDs (longer prefix) doesn't quietly regress title length. The truncation logic is shared, so this is a low-risk gap, but adding the test costs almost nothing.

5. Minor: task-bullet rendering is duplicated across the two body branches

File: orchestrator/gateway_client.py:1342-1352, 1368-1378

The desc = task.get("description") or task.get("id") or "" / whitespace-collapse / 300-char truncation / bullet-prefix block is copy-pasted between the program-narrative branch (under Tasks:) and the deterministic-fallback branch (under Tasks in this slice:). Different headers, identical body logic. Not worth a refactor right now — but if a future change needs to alter the bullet rendering, please extract the helper at that point rather than touching both copies.

6. Minor: Tasks: vs Tasks in this slice: label divergence between branches

File: orchestrator/gateway_client.py:1344, 1370

Same data, different headings depending on whether the program block is rendered. Cosmetic, but easy to unify by using Tasks in this slice: (the more descriptive form) in both, or Tasks: under the H2 ## This slice (which is already a scope marker). No strong preference; flagging in case it bothers anyone reviewing two-up.

What looks good

• Production caller in routes/pipelines.py:12488-12552 correctly threads program_pr to every slice and gates program_deferred_actions on is_terminal, so the locality invariant is preserved at the call site.
• terminal_pointer suppression when umbrella_has_program_block is False (line 12504-12511) is the right defensive call — non-terminals don't get pointed at a contentless terminal.
• Body section ordering on the terminal slice is unchanged from the legacy _auto_create_pr path: banner → description → obligations → (new) ## This slice → ## Test Plan → ## Manual Steps → footer. The merge-blocking banner stays high in the body per the original #2354 visibility intent. The new ## This slice section sits after obligations and before the test plan, which matches the reading-order rationale in the comment.
• Test coverage updates are in sync with the rename / renumbering — test_every_slice_pr_carries_program_metadata exercises the cross-slice fan-out cleanly, and the gateway-level tests assert both the title-shape disambiguation and the section ordering.
• Doc updates in docs/architecture/slice-dag.md accurately describe the new behavior, including the multi-terminal-forest caveat (caveat #3 above is about test coverage of that paragraph, not the prose itself).

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with main (commit 0474859). Conflicts came from #2540 (Fix #2539: drop duplicate slice prefix in non-terminal slice PR titles) landing on main while this PR was open. The PR's "Out of scope" note flagged the duplicate-prefix bug as tracked separately under what became #2539, so the merge integrates main's fix into this PR's structure rather than reverting it.

File	Category	Resolution
orchestrator/gateway_client.py	Semantic (additive)	Kept this PR's narrative-on-every-slice docstring + assertion-guarded fallback structure; adopted main's {slice_id}: {slice_name} fallback title (was slice {slice_id}: {slice_name}) and updated docstring reference to match.
orchestrator/tests/test_gateway_client.py	Semantic (additive)	Kept this PR's renamed/replaced tests (test_no_contract_pr_falls_back_to_auto_generated_title_and_body, test_non_terminal_slice_carries_program_narrative_with_slice_id_prefix); auto-merge picked main's slice-1: Pattern adoption assertion (the buggy form was unchanged on this side). Updated docstring reference to <slice_id>: <name>.
docs/architecture/slice-dag.md	Semantic (additive)	Kept this PR's "every slice carries the narrative" rewrite; updated the fallback-title reference from slice {slice_id}: {slice_name} to {slice_id}: {slice_name} (with #2539 attribution).

Verification:

• pytest orchestrator/tests/test_gateway_client.py::TestCreateSlicePR — 9/9 passed
• pytest orchestrator/tests/test_gateway_client.py orchestrator/tests/test_slice_run_loop_integration.py — 127/127 passed (including test_every_slice_pr_carries_program_metadata)
• ruff check clean on the two Python files

Please review: the docstring + doc updates now cite #2539 next to the {slice_id}: {slice_name} fallback to make the lineage clear to future readers; let me know if you'd rather leave the cite off.

— Authored by egg

[egg-reviewer]

Re-review at 0474859e — no new agent-mode design concerns since my prior approval at 256ad1dd.

The delta is the merge of origin/main into the PR branch (picking up #2526 / #2540 / #2536) plus the conflict resolution for #2540. The resolution only adjusted the fallback-title format (slice {slice_id}: {slice_name} → {slice_id}: {slice_name}) and the corresponding doc/test references — still rendering markdown PR bodies for human reviewers, no agent prompt / model-id / Agent SDK / pre-fetching surface touched.

— Authored by egg

[egg-reviewer]

Re-review at 0474859e (delta from 256ad1dd: merge of origin/main + conflict resolution for #2540).

The merge integration is clean. #2540 dropped the literal slice prefix from the deterministic fallback title; the resolution adopted that change in the right places — gateway_client.py:1296 (f"{slice_id}: {slice_name}"), the docstring at gateway_client.py:1237 now cites #2539 next to the {slice_id}: {slice_name} form, the gateway-level test docstring at test_gateway_client.py:1359-1361 and assertion at :1373 ("slice-1: Pattern adoption") match, and docs/architecture/slice-dag.md:469 was updated. TestCreateSlicePR (9/9) and the three integration tests (test_every_slice_pr_carries_program_metadata, test_terminal_slice_pr_carries_program_deferred_actions, test_non_terminal_pointer_suppressed_when_contract_pr_missing) all pass locally on the merged head.

The two title shapes that survive into the merged code are:

• Terminal: bare program_title (or {slice_id}: {slice_name} fallback)
• Non-terminal: [<slice-id>] <program_title> (or {slice_id}: {slice_name} fallback)

The [<slice-id>] non-terminal prefix is unaffected by #2540 (which only touched the fallback path), so there's no semantic conflict between #2538 and #2540 — they live on disjoint code paths.

Non-blocking findings

1. Stale comment in routes/pipelines.py:12569 references the old fallback shape

# otherwise both terminal and non-terminal
# fall back to the deterministic
# ``slice {id}: {name}`` form.

After the merge, the actual fallback is {slice_id}: {slice_name} (no leading slice ). The merge resolution explicitly updated the same wording in gateway_client.py's docstring and in slice-dag.md, but missed this companion comment in the production caller. Readers diffing this PR against #2540 will see one of the two surviving "slice " references and wonder which is canonical.

Trivial follow-up: drop slice here too, with a (#2539) cite to mirror the gateway-side comment.

2. Prior-review findings carry over unchanged

The previous review (commit 256ad1dd) flagged five non-blocking items that the merge did not touch:

1. The "obligations are terminal-only" assertion only fires when program_title is None; it does not catch the new third state (has_program_title=True, terminal_slice_id is not None, program_deferred_actions is not None). Defense-in-depth gap; the production caller in routes/pipelines.py:12607-12617 correctly gates obligations on is_terminal, so this is not a live bug.
2. Whitespace-only program_title silently drops the entire planner narrative and obligations — PRMetadata.title validates with min_length=1 but no strip, and the gateway falls through to the deterministic shape.
3. No multi-tree-forest integration test for the documented "non-chosen terminal leaf gets [<slice-id>] shape" behavior.
4. No truncation regression test for the [<slice-id>] non-terminal title shape (the prefix eats ~10 chars of the 70-char budget).
5. Task-bullet rendering and section-header label (Tasks: vs Tasks in this slice:) are duplicated across the program-narrative and fallback branches.

None of these are merge-blocking. They are still worth a follow-up issue once this lands.

3. Stale test docstring (pre-existing)

test_slice_run_loop_integration.py:862-868: the docstring on test_non_terminal_pointer_suppressed_when_contract_pr_missing still references the removed "the terminal slice 's PR carries the program-level narrative" pointer line. The test name and body are also misleading now — the test is really asserting "every slice gets terminal_slice_id=None when the umbrella has no narrative." Pre-existing at 256ad1dd, not introduced by the merge, but worth cleaning up while the area is open.

What looks good in the merge

• Conflict-resolution summary in the PR comment names the three files touched and the change in each, matching what's actually in the diff.
• The integration-side caller at routes/pipelines.py:12552-12618 correctly preserves the locality invariants from the original review: every slice gets program_pr (universal narrative), only the terminal slice gets program_deferred_actions (if program_pr and is_terminal else None), and terminal_pointer is suppressed when the umbrella has no program block (is_terminal or not umbrella_has_program_block).
• Test renames (test_no_contract_pr_falls_back_to_auto_generated_title_and_body, test_non_terminal_slice_carries_program_narrative_with_slice_id_prefix) and assertions stay in sync with the new behavior across both files.
• No new files touched; no expansion of scope under cover of the merge.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

5 previous review(s) hidden.