Skip to content

Repository navigation

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jwbron merged 2 commits into from
May 7, 2026
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
256ad1d
Fix #2538: slice PRs carry contract.pr narrative on every slice
jwbron May 7, 2026
0474859
Merge origin/main into egg/issue-2538-slice-pr-narrative
jwbron May 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 65 additions & 42 deletions docs/architecture/slice-dag.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,27 +441,39 @@ timeout / stuck-phase handler operates on the correct tracker.
`GatewayClient.create_slice_pr(pipeline_id, repo, *, slice_id, slice_name,
slice_tasks, head, base, program_title=None, program_description=None,
program_test_plan=None, program_manual_steps=None,
terminal_slice_id=None, ...)` opens one PR per slice with two body
shapes:

- **Non-terminal slices** (no `program_title`) — title is
`"slice {slice_id}: {slice_name}"` truncated to 70 chars; body lists
the slice's tasks (each truncated to 300 chars), optionally followed
by a one-line pointer to `terminal_slice_id` so reviewers can jump
to the umbrella PR; footer names the slice ID, pipeline, and base.
- **Terminal slice** (`program_title` set, sourced from the planner's
`# yaml-tasks` `pr` block) — title is the planner-authored
`contract.pr.title` (still capped to 70 chars); body opens with a
`> Program-level umbrella PR — terminal slice of pipeline …`
banner and renders `contract.pr.description`, then — when present —
the `## ⚠️ Pre-merge Obligations` / `## ✅ Resolved within this PR`
section from `contract.pr.deferred_actions` (threaded via
`program_deferred_actions`; same placement as the legacy
single-PR path, rendered by `orchestrator/pr_obligations.py` #2354),
then `pr.test_plan` (under `## Test Plan`) / `pr.manual_steps` (under
`## Manual Steps`) so the umbrella PR carries the program-level
reviewer narrative; the slice-context footer survives so the chain
position stays legible.
terminal_slice_id=None, ...)` opens one PR per slice. **Every** slice
PR — terminal AND non-terminal — carries the planner-authored program
narrative when `contract.pr` is populated, so reviewers see the
program rationale on whichever slice they open first (#2538). Earlier
behaviour (terminal-only narrative + a "see terminal slice's PR for
the umbrella narrative" pointer on non-terminals) buried the
description on the last-merged PR; reviewers approaching slice-1 saw
only task bullets.

- **Title.** The terminal slice gets the bare `program_title` (the
planner's program-level title from the `# yaml-tasks` `pr` block,
capped to 70 chars). Non-terminal slices get a `[<slice-id>] `
prefix (`[slice-1] <program_title>`) so the GitHub PR list stays
scannable when several stacked PRs are open at once. When
`program_title` is empty (older contracts / planner skipped the
field), every slice falls back to the deterministic
`slice {slice_id}: {slice_name}` form.
- **Body.** Program description → (terminal-only) `## ⚠️ Pre-merge
Obligations` / `## ✅ Resolved within this PR` section from
`contract.pr.deferred_actions` (rendered by
`orchestrator/pr_obligations.py` #2354) → `## This slice` (slice
name + task bullets, each truncated to 300 chars) → `## Test Plan` →
`## Manual Steps` → stack footer naming the slice ID, pipeline, and
base. The terminal slice prepends a `> Program-level umbrella PR —
terminal slice of pipeline …` merge-gate banner; non-terminals
skip the banner.

`program_deferred_actions` is **terminal-only** by convention — the
merge gate is the last-to-merge PR in the stack, so obligations live
on exactly one PR across the chain. `create_slice_pr` asserts that a
caller passing `program_deferred_actions` also passes `program_title`
(or `terminal_slice_id=None`); wiring obligations through to a
non-terminal slice is a caller bug and fails fast (#2354 review nit).

The implement-phase run loop (`_run_implement_phase_slices` in
`orchestrator/routes/pipelines.py`) selects the terminal slice and
Expand All @@ -471,34 +483,45 @@ threads the kwargs:
2. `terminal_ids = [s.id for s in contract.slices if s.id not in depended_on]`.
3. `chosen_terminal = terminal_ids[-1]` (last in declared order — see
the multi-terminal forest note below).
4. For the terminal slice: pass `program_*` from `contract.pr`
(including `program_deferred_actions` collected via
`_collect_pre_merge_obligations`), `terminal_slice_id=None`.
5. For every non-terminal slice: pass `program_*=None`
(`program_deferred_actions=None` — obligations belong on the
umbrella only),
`terminal_slice_id=chosen_terminal` **only if** `contract.pr.title`
is non-empty. When the contract has no program block (older
contracts, or `_populate_contract_from_plan` did not run), the
pointer is suppressed so reviewers are not directed to a PR with
no umbrella content.
4. For **every** slice (terminal and non-terminal): pass `program_*`
from `contract.pr` (so the narrative reaches every slice PR).
5. For the terminal slice only: pass `program_deferred_actions`
collected via `_collect_pre_merge_obligations` (live tracker
fallback included for parity with the legacy single-PR path); set
`terminal_slice_id=None`. The `None` pointer is the gateway's
signal for "this slice IS the terminal — bare title, umbrella
banner."
6. For non-terminal slices: `program_deferred_actions=None` (the
merge gate is elsewhere); set `terminal_slice_id=chosen_terminal`
**only if** `contract.pr.title` is non-empty. When the contract
has no program block (older contracts, or
`_populate_contract_from_plan` did not run), the pointer is
suppressed and the slice falls back to the deterministic title
shape. The `terminal_slice_id` value itself is no longer rendered
into the body (every slice carries its own narrative now); it
serves purely as the gateway's title-shape signal.

### Multi-terminal-forest pointer caveat

The slice DAG is a forest (≤1 DAG parent per slice — see
[Forest Validation](#plan-parser--forest-validation)) and a
multi-tree forest can have multiple terminal slices, one per tree.
The current behaviour picks `terminal_ids[-1]` (last declared) as
`chosen_terminal` and points **every** non-terminal across **every**
tree at it. This means non-terminal leaves in non-chosen trees carry
a pointer to a PR that lives in a different subtree of the chain. The
choice is deliberate (arbitrary but stable, deterministic across
parallel slice runs) and matches the simplification the issue asks
for, but operators reviewing a multi-tree pipeline should not be
surprised by the cross-tree pointer. A future refinement could
choose per-tree terminals and emit `terminal_slice_id` only within
the slice's own tree; until then the umbrella PR remains a single
roll-up at `chosen_terminal`.
`chosen_terminal` — that's the slice that gets the bare
`program_title` (no `[<slice-id>]` prefix) and the merge-gate
umbrella banner; the per-merge obligations section also lives on
exactly that PR. Other terminal leaves in non-chosen trees are
treated as non-terminals from the gateway's perspective: they
receive `terminal_slice_id=chosen_terminal`, get the
`[<slice-id>] <program_title>` title shape, and skip the banner
and obligations section. The choice is deliberate (arbitrary but
stable, deterministic across parallel slice runs) and matches the
simplification the issue asks for, but operators reviewing a
multi-tree pipeline should not be surprised that the merge-gate
PR sits in `chosen_terminal`'s subtree. Since #2538 the
narrative itself is on every slice PR, so cross-subtree
discoverability is no longer a concern — only the merge-gate
marker is centralised.

## Stacked-PR rebase reconciler

Expand Down
183 changes: 105 additions & 78 deletions orchestrator/gateway_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1224,71 +1224,100 @@ def create_slice_pr(
) -> str | None:
"""Open a PR for one slice in a stacked-PR chain.

Two body shapes:

* **Terminal slice** (caller passes ``program_title`` from the
contract's ``pr`` block): the PR carries the planner-authored
title / description / test plan / manual steps, plus a
banner marking it as the program-level umbrella for the
chain. When ``program_deferred_actions`` is non-empty, the
body also includes the ``## ⚠️ Pre-merge Obligations`` (and,
when applicable, ``## ✅ Resolved within this PR``) section
carrying conditional-ACK obligations (#2354). The section
is rendered immediately after the program description and
*before* ``## Test Plan`` so the merge-blocking banner is
visible without scrolling past pipeline metadata — same
placement as the legacy ``_auto_create_pr`` path. The
caller is expected to pass the *normalized* obligation
shape produced by
``routes.pipelines._collect_pre_merge_obligations`` (a list
of ``{reviewer, condition, resolved_in_diff}`` dicts), which
carries both the contract source and the live-tracker
fallback so this path stays at parity with the legacy
single-PR renderer.
* **Non-terminal slice** (no ``program_title``): deterministic
``slice {slice_id}: {slice_name}`` title, bulleted task list
body. When ``terminal_slice_id`` is supplied, the body also
carries a pointer to the terminal slice so reviewers can
jump to the umbrella PR. ``program_deferred_actions`` MUST
be ``None`` on this shape — obligations belong on the
umbrella, and the assertion below fails fast on caller
mistakes (#2354 review nit).

Both shapes always end with a footer naming the slice and the
branch it stacks onto, so the slice's role in the chain stays
legible in the PR view.
Every slice — terminal or not — carries the planner-authored
program narrative (title, description, test plan, manual steps)
when ``contract.pr`` is populated, so reviewers see the program
rationale on whichever slice PR they open first (#2538).

* **Title.** The terminal slice gets the bare ``program_title``;
non-terminal slices get a ``[<slice-id>] `` prefix so the
GitHub PR list stays scannable when several stacked PRs are
open at once. When ``program_title`` is empty (older contracts
/ planner skipped the field), every slice falls back to the
deterministic ``slice {slice_id}: {slice_name}`` form.
* **Body.** Program description → ``## This slice`` (slice name
and task bullets) → ``## Test Plan`` → ``## Manual Steps`` →
stack footer. The terminal slice prepends a "merge gate /
umbrella" banner and (when ``program_deferred_actions`` is
non-empty) injects the ``## ⚠️ Pre-merge Obligations`` /
``## ✅ Resolved within this PR`` section before
``## Test Plan`` — same placement as the legacy
``_auto_create_pr`` path. ``program_deferred_actions`` is
terminal-only by convention (the merge gate is the
last-to-merge PR in the stack); the assertion below fails
fast if a caller wires obligations through to a non-terminal
slice (#2354 review nit).

The caller passes the *normalized* obligation shape produced
by ``routes.pipelines._collect_pre_merge_obligations`` (a list
of ``{reviewer, condition, resolved_in_diff}`` dicts) so this
path stays at parity with the legacy single-PR renderer.

``terminal_slice_id`` is the caller's signal for "this is a
non-terminal slice; the terminal slice's id is X." It's no
longer rendered into the body (every slice carries its own
narrative now), but it still selects the title shape: the
terminal slice gets the bare ``program_title``, non-terminal
slices get the ``[<slice-id>] `` prefix.
"""
has_program_block = bool(program_title and program_title.strip())

if has_program_block:
assert program_title is not None # implied by has_program_block
title = program_title.strip()
has_program_title = bool(program_title and program_title.strip())
# ``pipelines.py`` sets ``terminal_slice_id`` only for
# non-terminal slices that point at an existing umbrella. So
# ``terminal_slice_id is None`` combined with a populated
# ``program_title`` identifies the terminal slice (or a
# single-slice pipeline, which is also terminal).
is_terminal_slice = has_program_title and terminal_slice_id is None

if has_program_title:
assert program_title is not None # implied by has_program_title
program_title_clean = program_title.strip()
if is_terminal_slice:
title = program_title_clean
else:
title = f"[{slice_id}] {program_title_clean}"
else:
# Defensive: obligations belong only on the umbrella. Failing
# fast here catches a caller wiring ``program_deferred_actions``
# through to a non-terminal slice (#2354 review nit) instead
# of silently dropping it.
#
# Guarded on ``program_title is None`` rather than
# ``has_program_title`` so a whitespace-only ``program_title``
# (which ``PRMetadata.title`` allows under its current
# ``min_length=1`` validator) doesn't masquerade as a slice
# routing error here — that's a different bug and should
# surface as such, not as a spurious AssertionError on the
# umbrella PR creation path (#2354 review observation B).
if program_title is None:
assert program_deferred_actions is None, (
"program_deferred_actions must be None on non-terminal slices; "
"obligations belong on the umbrella PR only"
)
title = f"slice {slice_id}: {slice_name}".strip()
if len(title) > 70:
title = title[:67] + "..."

body_lines: list[str] = []

if has_program_block:
body_lines.append(
f"> **Program-level umbrella PR — terminal slice of pipeline `{pipeline_id}`.**"
)
body_lines.append(
"> Roll-up of the slice-PR chain; the planner's narrative below covers "
"the whole program, not just this slice."
)
body_lines.append("")
if has_program_title:
if is_terminal_slice:
body_lines.append(
f"> **Program-level umbrella PR — terminal slice of pipeline `{pipeline_id}`.**"
)
body_lines.append(
"> Roll-up of the slice-PR chain; this PR is the merge gate "
"for the program. Pre-merge obligations (when present) live here."
)
body_lines.append("")
if program_description and program_description.strip():
body_lines.append(program_description.strip())
body_lines.append("")
# Render Pre-merge Obligations / Resolved-within-PR *before*
# ``## Test Plan`` so the merge-blocking banner is visible
# without scrolling past plan/steps. Same placement as the
# legacy ``_auto_create_pr`` path (see
# ``routes/pipelines.py::_build_pr_body``); the renderer is
# the same shared module so both paths stay byte-identical
# (#2354 review).
# Render Pre-merge Obligations / Resolved-within-PR right
# after the program description so the merge-blocking
# banner stays high in the body — the original visibility
# intent from #2354 (banner must be visible without
# scrolling past plan/steps). Same placement as the legacy
# ``_auto_create_pr`` path.
if program_deferred_actions:
try:
from pr_obligations import render_obligations_section_from_normalized
Expand All @@ -1302,6 +1331,26 @@ def create_slice_pr(
if obligations_section:
body_lines.append(obligations_section)
body_lines.append("")
# Per-slice scope: slice name + task bullets. Sits between
# obligations and the test plan so reviewers see "what does
# the program do (and what's blocking merge)" → "what does
# this slice contribute" → "how do we verify the program"
# in reading order.
body_lines.append("## This slice")
body_lines.append("")
body_lines.append(slice_name)
if slice_tasks:
body_lines.append("")
body_lines.append("Tasks:")
for task in slice_tasks:
desc = task.get("description") or task.get("id") or ""
desc = " ".join(desc.split()) # collapse whitespace
if len(desc) > 300:
desc = desc[:297] + "..."
task_id = task.get("id") or ""
bullet_prefix = f"- {task_id}: " if task_id else "- "
body_lines.append(f"{bullet_prefix}{desc}")
body_lines.append("")
if program_test_plan and program_test_plan.strip():
body_lines.append("## Test Plan")
body_lines.append("")
Expand All @@ -1313,23 +1362,8 @@ def create_slice_pr(
body_lines.append(program_manual_steps.strip())
body_lines.append("")
else:
# Defensive: obligations belong only on the umbrella. Failing
# fast here catches a caller wiring ``program_deferred_actions``
# through to a non-terminal slice (#2354 review nit) instead
# of silently dropping it.
#
# Guarded on ``program_title is None`` rather than
# ``has_program_block`` so a whitespace-only ``program_title``
# (which ``PRMetadata.title`` allows under its current
# ``min_length=1`` validator) doesn't masquerade as a slice
# routing error here — that's a different bug and should
# surface as such, not as a spurious AssertionError on the
# umbrella PR creation path (#2354 review observation B).
if program_title is None:
assert program_deferred_actions is None, (
"program_deferred_actions must be None on non-terminal slices; "
"obligations belong on the umbrella PR only"
)
# Fallback: contract.pr missing or program_title empty.
# Render the deterministic per-slice body with no narrative.
body_lines.append(slice_name)
if slice_tasks:
body_lines.append("")
Expand All @@ -1342,13 +1376,6 @@ def create_slice_pr(
task_id = task.get("id") or ""
bullet_prefix = f"- {task_id}: " if task_id else "- "
body_lines.append(f"{bullet_prefix}{desc}")
if terminal_slice_id:
body_lines.append("")
body_lines.append(
f"Part of pipeline `{pipeline_id}`; the terminal slice "
f"`{terminal_slice_id}`'s PR carries the program-level "
"narrative (description, test plan, manual steps)."
)
body_lines.append("")

body_lines.append(
Expand Down
Loading
Loading