diff --git a/docs/architecture/slice-dag.md b/docs/architecture/slice-dag.md index 547d73f435..e5cafcb65b 100644 --- a/docs/architecture/slice-dag.md +++ b/docs/architecture/slice-dag.md @@ -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_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 `[] ` + prefix (`[slice-1] `) 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_id}: {slice_name}` form (#2539). +- **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 @@ -471,17 +483,23 @@ 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 @@ -489,16 +507,21 @@ 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 `[]` 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 +`[] ` 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 diff --git a/orchestrator/gateway_client.py b/orchestrator/gateway_client.py index ce5ed7b5e8..ace2761200 100644 --- a/orchestrator/gateway_client.py +++ b/orchestrator/gateway_client.py @@ -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_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 ``[] `` 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_id}: {slice_name}`` form (#2539). + * **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 ``[] `` 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_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 @@ -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("") @@ -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("") @@ -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( diff --git a/orchestrator/routes/pipelines.py b/orchestrator/routes/pipelines.py index 770fab4743..eb3806d02f 100644 --- a/orchestrator/routes/pipelines.py +++ b/orchestrator/routes/pipelines.py @@ -12528,18 +12528,19 @@ def _run_one_slice_inner(slice_id: str, parent_slice_id: str | None) -> tuple[in # Identify the terminal slice(s) of the # forest: any slice no other slice lists # as a dependency. Pick a single terminal - # so non-terminal slices can point at it - # ("see 's PR for the - # umbrella narrative") and so the - # planner-authored ``contract.pr`` block - # lands on exactly one PR. When the - # forest has multiple terminal slices we - # take the last one in declared order — - # arbitrary but stable. For multi-tree - # forests this means non-terminal slices - # in non-chosen trees point at a leaf - # outside their own subtree; see the - # "Stacked-PR creation" section of + # so non-terminal slices can flag it via + # ``terminal_slice_id`` (the gateway uses + # that signal to switch the title shape: + # bare ``program_title`` for the terminal + # vs. ``[] `` for + # non-terminals). When the forest has + # multiple terminal slices we take the + # last one in declared order — arbitrary + # but stable. For multi-tree forests this + # means non-terminal slices in non-chosen + # trees flag a leaf outside their own + # subtree; see the "Stacked-PR creation" + # section of # ``docs/architecture/slice-dag.md`` for # the rationale. depended_on: set[str] = { @@ -12550,21 +12551,24 @@ def _run_one_slice_inner(slice_id: str, parent_slice_id: str | None) -> tuple[in ] chosen_terminal = terminal_ids[-1] if terminal_ids else None is_terminal = slice_id == chosen_terminal - program_pr = contract_post.pr if is_terminal else None - # Only point non-terminal slices at the - # terminal when the umbrella will actually - # carry a program-level narrative. - # ``create_slice_pr`` upgrades to the - # planner-authored shape only when - # ``program_title`` is non-empty, so a - # ``contract.pr`` with a missing/empty - # title yields the auto-generated body on - # the terminal — and the pointer would - # mislead reviewers who follow it. + # #2538: every slice — terminal or not — + # carries the planner-authored narrative on + # its PR so reviewers see context on whichever + # slice they open first. Per-merge obligations + # remain terminal-only (the merge gate is the + # last-to-merge PR in the stack). + program_pr = contract_post.pr + # ``terminal_slice_id`` is the gateway's + # title-shape signal: None on the terminal + # itself (or when no umbrella narrative + # exists), the terminal id otherwise. We + # only flag non-terminals when the umbrella + # actually has a planner-authored title; + # otherwise both terminal and non-terminal + # fall back to the deterministic + # ``slice {id}: {name}`` form. umbrella_has_program_block = bool( - contract_post.pr - and contract_post.pr.title - and contract_post.pr.title.strip() + program_pr and program_pr.title and program_pr.title.strip() ) terminal_pointer = ( None @@ -12601,14 +12605,16 @@ def _run_one_slice_inner(slice_id: str, parent_slice_id: str | None) -> tuple[in # the legacy ``_auto_create_pr`` path # (#2354 review item 2). "program_deferred_actions": ( - _collect_pre_merge_obligations( - pipeline_id, - list(program_pr.deferred_actions), + ( + _collect_pre_merge_obligations( + pipeline_id, + list(program_pr.deferred_actions), + ) + or None ) - or None - ) - if program_pr - else None, + if program_pr and is_terminal + else None + ), "terminal_slice_id": terminal_pointer, } except Exception as load_err: # noqa: BLE001 diff --git a/orchestrator/tests/test_gateway_client.py b/orchestrator/tests/test_gateway_client.py index 97042bdb01..63aef32d09 100644 --- a/orchestrator/tests/test_gateway_client.py +++ b/orchestrator/tests/test_gateway_client.py @@ -1355,9 +1355,10 @@ def _fake_create_pr(*, title: str, body: str, **_kwargs: object) -> str: return captured, patch.object(gateway_client, "create_pr", side_effect=_fake_create_pr) - def test_non_terminal_slice_uses_auto_generated_title_and_body(self, gateway_client): - """Without program_title the helper still emits the deterministic - ``: `` title and the bulleted task body.""" + def test_no_contract_pr_falls_back_to_auto_generated_title_and_body(self, gateway_client): + """When ``contract.pr`` is missing (no ``program_title``), every slice + — terminal or not — falls back to the deterministic ``: + `` title and a bulleted task body. No narrative to render.""" captured, ctx = self._capture(gateway_client) with ctx: gateway_client.create_slice_pr( @@ -1374,14 +1375,18 @@ def test_non_terminal_slice_uses_auto_generated_title_and_body(self, gateway_cli assert "Tasks in this slice:" in captured["body"] assert "- task-1-1: Add the barrel re-export" in captured["body"] assert "Slice slice-1 of pipeline issue-42" in captured["body"] - # No program-umbrella banner / pointer when neither field is set. + # No program-umbrella banner / per-slice section when neither field is set. assert "Program-level umbrella PR" not in captured["body"] - assert "terminal slice" not in captured["body"] + assert "## This slice" not in captured["body"] - def test_non_terminal_slice_with_terminal_pointer_includes_pointer_line(self, gateway_client): - """When ``terminal_slice_id`` is supplied (and program_title is not) - the body adds a pointer to the umbrella PR's slice without - upgrading to the planner-authored shape.""" + def test_non_terminal_slice_carries_program_narrative_with_slice_id_prefix( + self, gateway_client + ): + """#2538: every slice — including non-terminal ones — carries the + planner-authored narrative on its PR so reviewers see program + context on whichever slice they open first. Non-terminal slices + get a ``[] `` title prefix to disambiguate in the + GitHub PR list.""" captured, ctx = self._capture(gateway_client) with ctx: gateway_client.create_slice_pr( @@ -1389,20 +1394,48 @@ def test_non_terminal_slice_with_terminal_pointer_includes_pointer_line(self, ga repo="owner/repo", slice_id="slice-1", slice_name="Pattern adoption", - slice_tasks=None, + slice_tasks=[{"id": "task-1-1", "description": "Add the barrel re-export"}], head="egg/issue-42/slice-1", base="egg/issue-42", + program_title="Decompose oversize files; ratchet allowlist", + program_description="The lint added in #2250 caps Python files at 1500 lines.", + program_test_plan="- Automated: make lint and make test-all green.", + program_manual_steps="Pre-merge (terminal slice only): verify seam tables.", terminal_slice_id="slice-3", ) - assert captured["title"] == "slice-1: Pattern adoption" - assert "`slice-3`" in captured["body"] - assert "program-level narrative" in captured["body"] - assert "Program-level umbrella PR" not in captured["body"] + assert captured["title"] == "[slice-1] Decompose oversize files; ratchet allowlist" + body = captured["body"] + # No "umbrella" banner on non-terminals — that's the merge-gate marker + # and belongs only on the terminal slice. + assert "Program-level umbrella PR" not in body + # Program narrative is present. + assert "The lint added in #2250" in body + assert "## Test Plan" in body + assert "make lint" in body + assert "## Manual Steps" in body + assert "seam tables" in body + # Per-slice scope section appears after the program description and + # carries the slice name + task bullets. + assert "## This slice" in body + assert "Pattern adoption" in body + assert "- task-1-1: Add the barrel re-export" in body + # Section ordering: description → This slice → Test Plan → Manual Steps. + assert body.index("The lint added in #2250") < body.index("## This slice") + assert body.index("## This slice") < body.index("## Test Plan") + assert body.index("## Test Plan") < body.index("## Manual Steps") + # Stack footer survives. + assert "Slice slice-1 of pipeline issue-42" in body + # Pointer line from the old non-terminal shape is gone — narrative is + # right here, no need to send reviewers elsewhere. + assert "program-level narrative" not in body + assert "carries the program-level" not in body def test_terminal_slice_uses_program_title_and_planner_authored_body(self, gateway_client): - """When ``program_title`` is set the helper emits the planner-authored - title + description / test plan / manual steps body, prefixed with - the umbrella banner so reviewers can tell the PR is program-level.""" + """When ``program_title`` is set and ``terminal_slice_id`` is None + (the terminal slice itself), the helper emits the planner-authored + title + description / per-slice / test plan / manual steps body, + prefixed with the umbrella banner so reviewers can tell the PR is + the program merge gate.""" captured, ctx = self._capture(gateway_client) with ctx: gateway_client.create_slice_pr( @@ -1418,11 +1451,16 @@ def test_terminal_slice_uses_program_title_and_planner_authored_body(self, gatew program_test_plan="- Automated: make lint and make test-all green.", program_manual_steps="Pre-merge (terminal slice only): verify seam tables.", ) + # Terminal title is bare program_title (no slice-id prefix). assert captured["title"] == "Decompose oversize files; ratchet allowlist" body = captured["body"] assert "Program-level umbrella PR" in body assert "issue-42" in body assert "The lint added in #2250" in body + # Per-slice scope section + slice name/tasks present on terminal too. + assert "## This slice" in body + assert "Apply the ratchet" in body + assert "- task-3-1: Bump the allowlist" in body assert "## Test Plan" in body assert "make lint" in body assert "## Manual Steps" in body diff --git a/orchestrator/tests/test_slice_run_loop_integration.py b/orchestrator/tests/test_slice_run_loop_integration.py index d74a21df50..7c186f0fa1 100644 --- a/orchestrator/tests/test_slice_run_loop_integration.py +++ b/orchestrator/tests/test_slice_run_loop_integration.py @@ -703,19 +703,25 @@ def test_reconciler_started_and_stopped(self) -> None: # Thread.join is invoked with a bounded timeout to avoid blocking. fake_thread.join.assert_called_once() - def test_terminal_slice_pr_carries_program_metadata_non_terminal_does_not( + def test_every_slice_pr_carries_program_metadata( self, ) -> None: - """#2340: program-level ``contract.pr`` is threaded only into the - terminal slice's PR; non-terminal slices stay deterministic and - carry a pointer to the terminal slice's PR. + """#2538: program-level ``contract.pr`` is threaded into every + slice's PR — terminal AND non-terminal — so reviewers see + program rationale on whichever slice they open first. The + original #2340 behaviour (terminal-only narrative + pointer on + non-terminals) buried the program description on the + last-merged PR, leaving slice-1 reviewers with no context. Forest: slice-1 (root) → slice-2 (intermediate) → slice-3 (terminal). - slice-3 is the unique slice no other slice depends on; it should - receive ``program_title`` / ``program_description`` / + slice-3 is the unique slice no other slice depends on. All three + slices receive ``program_title`` / ``program_description`` / ``program_test_plan`` / ``program_manual_steps`` from - ``contract.pr``. slice-1 and slice-2 should receive ``None`` for - each program-* kwarg and ``terminal_slice_id="slice-3"``. + ``contract.pr``. The terminal gets ``terminal_slice_id=None`` + (signalling "this is the merge gate"); non-terminals get + ``terminal_slice_id="slice-3"`` so the gateway switches the + title shape to ``[] `` and skips the + umbrella banner. """ pipeline = _make_pipeline() root = _make_slice("slice-1", tasks=[_make_task("task-1-1")]) @@ -756,20 +762,22 @@ def test_terminal_slice_pr_carries_program_metadata_non_terminal_does_not( } assert set(pr_calls_by_slice) == {"slice-1", "slice-2", "slice-3"} - terminal_kwargs = pr_calls_by_slice["slice-3"] - assert terminal_kwargs["program_title"] == "Decompose oversize files; ratchet allowlist" - assert terminal_kwargs["program_description"].startswith("The lint added in #2250") - assert "make lint" in terminal_kwargs["program_test_plan"] - assert "seam tables" in terminal_kwargs["program_manual_steps"] - assert terminal_kwargs["terminal_slice_id"] is None - + # Every slice — terminal and non-terminal — carries the program + # narrative. Title-shape disambiguation happens inside + # ``create_slice_pr`` based on ``terminal_slice_id``. + for slice_id in ("slice-1", "slice-2", "slice-3"): + kwargs = pr_calls_by_slice[slice_id] + assert kwargs["program_title"] == "Decompose oversize files; ratchet allowlist" + assert kwargs["program_description"].startswith("The lint added in #2250") + assert "make lint" in kwargs["program_test_plan"] + assert "seam tables" in kwargs["program_manual_steps"] + + # The terminal slice gets terminal_slice_id=None (it IS the + # merge gate); non-terminals get the terminal id so the + # gateway prefixes their title with []. + assert pr_calls_by_slice["slice-3"]["terminal_slice_id"] is None for non_terminal_id in ("slice-1", "slice-2"): - kwargs = pr_calls_by_slice[non_terminal_id] - assert kwargs["program_title"] is None - assert kwargs["program_description"] is None - assert kwargs["program_test_plan"] is None - assert kwargs["program_manual_steps"] is None - assert kwargs["terminal_slice_id"] == "slice-3" + assert pr_calls_by_slice[non_terminal_id]["terminal_slice_id"] == "slice-3" def test_terminal_slice_pr_carries_program_deferred_actions( self,