Compose context-PR body + restructure slice-PR body (#3115)

[jwbron]

Closes #3115.

Pipeline-generated PR descriptions were poor on both surfaces (seen on Khan/webapp#40099 / #40098): the context PR body was contract.pr.description verbatim — silently dropping the preflight-required test_plan / manual_steps and linking none of the artifacts the orchestrator knows about — and slice PR bodies were a wall of planning-consensus task prose with no reviewer-facing summary, no view of what the branch actually contains, and (on degraded contracts) no link back to the base PR.

Changes

Context PR body composer (_compose_context_pr_body, used by _open_context_pr_at_implement_start):

• Renders the planner narrative, then ## Test plan / ## Manual steps (dropped on the floor since #2777 removed the PR phase), then a generated ## Pipeline context footer: pipeline id, originating issue, slice table, and links to the refine analysis draft, plan draft, and per-phase BRC transcripts committed on the work branch.
• Links are branch-qualified absolute URLs (https://github.com/<repo>/blob/<work-branch>/...) — GitHub resolves relative PR-body links against the default branch, where .egg-state/ doesn't exist. _build_brc_history_link_line (production-orphaned since #2777) gains a link_base param and a caller again.

Slice PR body restructure (create_slice_pr):

• Leads with the planner's reviewer-facing per-slice goal — already prompted for and parsed, but dropped before reaching the contract until now (Slice.goal field added; to_contract_slice carries it; planner prompts updated to say the goal is rendered verbatim as the PR lead, plain language only).
• New ## What's in this PR section: commit subjects + diffstat computed from the pushed integration branch by _build_slice_diff_summary (best-effort: fetch via gateway, git log/git diff --stat vs the parent; (None, None) on any failure).
• Full task dump + acceptance criteria moves behind a <details> fold (traceability preserved, wall of text gone).
• **Base PR:** renders on every branch that knows the number, and the run loop falls back to pipeline.pr_number when contract linkage is missing (#3100-degraded contracts).
• Titles truncate at a word boundary (no more claim-che...); the legacy plain-text stack footer is dropped (repo-wide search found no consumer).

Out of scope

#3100 (derived PR-metadata fallback + preflight enforcement on implement-start submits) remains the root-cause fix for contracts arriving with no pr: block at all.

Test plan

• New: test_slice_diff_summary.py (real git repo), TestComposeContextPrBody, goal-lead/details/diffstat/base-PR-fallback/title-truncation cases in test_gateway_client.py, link_base case in test_brc_history.py, goal round-trip in test_plan_parser.py.
• Updated: opener happy-path body assertion, slice-PR body shape assertions.
• make lint green; make test (changeset-aware): 17,286 passed; the single failure (tests/scripts/test_reap_stale_egg_images.py::TestReapScriptSafetyGuard::test_proceeds_with_reap_when_all_four_kept_refs_present, exit 127) is pre-existing on a pristine main checkout on this host and untouched by this change.

[egg-reviewer]

No agent-mode design concerns.

This PR is deterministic, orchestrator-side PR-body composition (context PR and slice PR) from already-known contract/pipeline state plus real git output. No agent prompts are being constrained, no large diffs are being baked into a prompt, no structured output is being demanded from an agent for downstream parsing.

The planner-facing guidance added in two places — the slice goal schema description (.egg/schemas/yaml-tasks.schema.json) and the planner-prompt YAML template comments (_build_phase_prompt / _build_agent_prompt) — tells the planner what the field is used for ("rendered verbatim as the slice PR-body lead paragraph, write it as a reviewer-facing summary") rather than micromanaging how to write it. That's appropriate orientation about downstream consumption, not procedural over-specification.

— Authored by egg

[egg-reviewer]

Reviewed the diff end-to-end. The shape of the change is sound — restoring test_plan / manual_steps to the context PR body, leading slice PRs with the planner-written slice_goal, and rendering real-git diff summaries are all clear improvements. Found one blocking correctness issue and several non-blocking notes.

Blocking

_build_slice_diff_summary defaults fetch_branch to public mode, breaking the feature in private-repo pipelines

orchestrator/routes/pipelines.py:11020-11026

spawner.gateway.fetch_branch(
    pipeline_id,
    str(worktree_repo_path),
    args=[f"+refs/heads/{branch}:refs/remotes/origin/{branch}"],
)

This is the only fetch_branch call in the codebase that doesn't pass mode=gateway_mode. Every other caller threads the pipeline-computed mode through (see _commit_slice_brc_history_to_integration_branch at pipelines.py:10849-10854, _read_source_branch_artifacts at :5484-5489, and the surrounding create_slice_pr call at :16705 — they all use mode=gateway_mode). The default here is mode="public".

_compute_gateway_mode (pipelines.py:2226) auto-selects private for any repo whose visibility is private/internal. In those pipelines:

1. The helper requests a public-mode session against a private repo.
2. The gateway refuses or the session lacks credentials → fetch_branch logs "Best-effort fetch failed" and returns False.
3. origin/<parent_branch> and origin/<integration_branch> aren't refreshed (or origin/<parent> doesn't exist locally at all because nothing else has fetched it in this mode).
4. git log / git diff either return empty output or non-zero, the helper returns (None, None), and _append_diff_summary_section drops the whole section.

Net effect: in every pipeline that targets a private/internal repo, the new ## What's in this PR section silently never renders. This is the canonical cross-module silent no-op — each file is internally consistent, the wiring across modules dead-ends, and only the new feature breaks (slice PR creation itself still works because create_slice_pr is mode-aware).

Fix: add a gateway_mode parameter to _build_slice_diff_summary and pass it through to fetch_branch. gateway_mode is already in scope at the call site (pipelines.py:16687).

def _build_slice_diff_summary(
    pipeline,
    spawner: "ContainerSpawner",
    worktree_repo_path: Path,
    integration_branch: str,
    parent_branch: str,
    gateway_mode: Literal["public", "private"] = "public",
) -> tuple[list[str] | None, str | None]:
    ...
    spawner.gateway.fetch_branch(
        pipeline_id,
        str(worktree_repo_path),
        args=[f"+refs/heads/{branch}:refs/remotes/origin/{branch}"],
        mode=gateway_mode,
    )

Worth adding a private-mode test (a fixture that asserts fetch_branch.call_args.kwargs["mode"] == "private") so this doesn't regress.

Non-blocking

_truncate_title only word-boundary-cuts on spaces — still produces ugly trailing punctuation

orchestrator/gateway_client.py:277-292

For the very title the test was added to fix (Foundation: source-of-truth library + verifier + claim-check + drift CLI), the cut lands at the trailing space after the +, producing …[slice-1] Foundation: source-of-truth library +.... Not mid-word, but +... is just a different kind of ugly. Consider stripping trailing punctuation / symbols (+, -, :, etc.) before appending ..., so the result is …library... instead.

Separately, when max_len <= 3 the function returns a string longer than max_len (e.g. _truncate_title("a a", max_len=2) returns "a..."). Currently no caller uses such a value, but a guard like if max_len <= 3: return title[:max_len] would prevent surprise.

Section header case inconsistency between context PR and slice PR bodies

orchestrator/routes/pipelines.py:9481, 9485 use ## Test plan / ## Manual steps (lowercase), while orchestrator/gateway_client.py:1852, 1857 and the global PR template in ~/.claude/CLAUDE.md use ## Test Plan / ## Manual Steps (Title Case). Pick one and apply uniformly. The PR template convention is Title Case, so that's probably the right choice.

_compose_context_pr_body always emits the ## Pipeline context header

orchestrator/routes/pipelines.py:9487. Even when no slice table, no docs, no BRC transcripts, and no issue link render — only the bare - Pipeline: <id> line — the header still appears. Minor cosmetic; consider gating the header on whether anything below it actually renders.

Dead-code try/except around fetch_branch in _build_slice_diff_summary

orchestrator/routes/pipelines.py:11020-11034. GatewayClient.fetch_branch catches everything internally and returns bool (gateway_client.py:3150-3157). The except Exception branch is unreachable in production — only the test's RuntimeError mock can hit it. Not harmful, but the test is exercising a non-existent code path. Either drop the try/except (and the test) or check the bool return value instead.

Legacy phase-N slice IDs render awkwardly in the slice table

orchestrator/routes/pipelines.py:9499 uses s.id.removeprefix('slice-'), which leaves phase-1 untouched. The Slice model still permits the phase- pattern (models.py:336), though _migrate_phases_to_slices rewrites legacy JSON to slice- on load. So in practice in-memory contracts always carry slice-N and you'd only hit the awkward path if a Slice is constructed directly with a phase- ID. Cheap one-line robustness: s.id.removeprefix('slice-').removeprefix('phase-').

_compose_context_pr_body test coverage gaps

orchestrator/tests/test_open_context_pr_at_implement_start.py:240 covers happy path + minimal contract, but not:

• pipeline.repo or pipeline.branch is None → no docs/BRC links (the link_base guard).
• pipeline.issue_number is None (prompt-driven pipelines) → no - Issue: line.

Worth one or two more parametrized cases to lock down the conditional rendering.

PR body size near GitHub's 65,536-byte limit

PRMetadata.description has no max_length. Pre-#3115 the body was the bare description; post-#3115 the composed footer adds several hundred bytes. A description that previously squeaked under the limit could now hit it. Edge case, unlikely in practice, but worth knowing.

— Authored by egg

[james-in-a-box]

Thanks for the thorough review. Per-item disposition:

Blocking

• _build_slice_diff_summary defaults fetch_branch to public mode — fixed-in-PR (commit 0d8b811). Added a gateway_mode: Literal["public", "private"] = "public" parameter, threaded the pipeline-computed mode through at the call site (gateway_mode=gateway_mode # type: ignore[arg-type]), and added a test_threads_gateway_mode_to_fetch_branch test that asserts every fetch_branch.call_args.kwargs["mode"] == "private". Docstring updated to call out the silent-no-op failure mode.

Non-blocking

• _truncate_title trailing punctuation + max_len <= 3 guard — fixed-in-PR (commit 0d8b811). Added a _TITLE_TRAILING_PUNCT set covering +, -, :, /, etc. and rstrip it before appending .... Added an early-return when max_len <= 3 so we never produce a string longer than max_len. Strengthened the existing word-boundary test to assert the head-before-ellipsis ends on an alphanumeric, plus a new _truncate_title helper test covering both regressions directly.

• Section header case inconsistency — fixed-in-PR (commit 0d8b811). Picked Title Case (matches the global PR template). Renamed ## Test plan / ## Manual steps → ## Test Plan / ## Manual Steps in _compose_context_pr_body, the corresponding tests, and both architecture/guide docs that quoted the old form.

• ## Pipeline context header always emitted — fixed-in-PR (commit 0d8b811). Track has_meaningful_content across the issue / slices / docs / BRC branches; suppress the entire footer (header + bare pipeline-id line) when none of them fire. Added a test_no_issue_number_omits_issue_line case that exercises this path.

• Dead-code try/except around fetch_branch — fixed-in-PR (commit 0d8b811). Dropped the inner try/except entirely — fetch_branch swallows internally and returns bool (gateway_client.py:3150-3157), so the only path that ever hit the except was the test's RuntimeError mock. Updated test_fetch_failure_is_nonfatal to use return_value=False (the real failure shape) and cite the bool-return line range in its docstring.

• Legacy phase-N slice IDs in slice table — fixed-in-PR (commit 0d8b811). Strip both prefixes: s.id.removeprefix("slice-").removeprefix("phase-"). Added test_phase_n_slice_id_renders_clean_number to lock it in.

• _compose_context_pr_body test coverage gaps — fixed-in-PR (commit 0d8b811). Added test_no_repo_or_branch_skips_artifact_links (empty repo/branch → docs and BRC suppressed, rest of footer still renders) and test_no_issue_number_omits_issue_line (issue_number=None → no - Issue: line, and with no other content the whole footer is omitted).

• PR body size near GitHub's 65,536-byte limit — disagree (informational, no actionable change requested in-PR). As the review note says, this is an edge case "unlikely in practice" — the footer adds a few hundred bytes against a 65k limit, and a description that was already that close to the cap would have been at risk before #3115 as well. The right place to enforce a cap is at the PRMetadata model layer if it ever becomes a real problem, not as a defensive truncation deep in the composer; adding one preemptively trades complexity for an edge case neither the review nor the data flag as live. Happy to file a follow-up if you'd prefer that surface explicitly, but per the feedback contract I'm reading "worth knowing" as FYI rather than a fix request.

— Authored by egg

[james-in-a-box]

Feedback contract violation detected.

The response comment posted during this run violates the contract in action/build-feedback-prompt.sh. Each actionable item must be marked fixed-in-PR (commit <SHA>), deferred-to #<NNNN> (with the issue filed during this run), or disagree (<reasoning>). Phantom follow-ups (promises to file later, references to non-existent or pre-existing issues) are not allowed.

Violations:

forbidden phrase(s): file a follow-up

View run logs

— Authored by egg

[egg-reviewer]

No agent-mode design concerns.

Re-reviewed the delta since 3db69157. The follow-up commit is purely the corrections requested by the general-review bot: trailing-punctuation strip + max_len <= 3 guard in _truncate_title, Title Case header alignment with the global PR template, ## Pipeline context footer gated on has_meaningful_content, phase- prefix stripped alongside slice-, and gateway_mode threaded through _build_slice_diff_summary → fetch_branch. Nothing here changes prompt shape, structured-output contracts, or the agent interaction surface — the slice-diff summary remains real git output composed into a human-facing PR body, not anything routed back into an agent.

— Authored by egg

[egg-reviewer]

Re-reviewed the delta (commit 0d8b811) against my prior review at 3db69157. Every blocking and non-blocking item from the prior review is addressed cleanly; the one disposition flagged disagree (PR-body-size cap) is acceptable given the defense-in-depth nature of the concern.

Verification of prior-review items

Item	Severity	Status	Notes
_build_slice_diff_summary defaulted fetch_branch to public mode	Blocking	✅ Fixed	gateway_mode: Literal["public", "private"] = "public" added at pipelines.py:11013, threaded into both fetch_branch calls at :11049. Call site at :16707 passes gateway_mode=gateway_mode (in-scope from _run_implement_phase_slices at :15730). Docstring at :11029-11033 calls out the silent-no-op failure mode. New test test_threads_gateway_mode_to_fetch_branch (test_slice_diff_summary.py:103-119) asserts both calls use mode="private".
_truncate_title trailing punctuation + max_len <= 3 guard	Non-blocking	✅ Fixed	_TITLE_TRAILING_PUNCT = " \t\r\n.,;:!?-+/\|&*=~^<>" at gateway_client.py:277; max_len <= 3 early-return at :296-297; rstrip(_TITLE_TRAILING_PUNCT) at :302. New helper test at test_gateway_client.py:1664-1683. Verified pre-fix output [slice-1] Foundation: source-of-truth library + verifier +... becomes …verifier... post-fix.
Section header case inconsistency	Non-blocking	✅ Fixed	All four call sites unified to Title Case: pipelines.py:9483, 9487, gateway_client.py:1862, 1867, and the two docs at docs/architecture/sdlc-pipeline.md:188 and docs/guides/sdlc-pipeline.md:874.
## Pipeline context always emitted	Non-blocking	✅ Fixed	has_meaningful_content flag at pipelines.py:9494, set True only when issue / slices / docs / BRC adds content; header emitted only when True at :9535-9536. Confirmed by test_no_issue_number_omits_issue_line (test_open_context_pr_at_implement_start.py:342-363).
Dead try/except around fetch_branch	Non-blocking	✅ Fixed	Inner try/except removed at pipelines.py:11041-11050. Confirmed fetch_branch swallows internally and returns bool (gateway_client.py:3159-3167). test_fetch_failure_is_nonfatal updated from side_effect=RuntimeError to return_value=False — matches the real failure shape.
Legacy phase-N slice IDs	Non-blocking	✅ Fixed	s.id.removeprefix("slice-").removeprefix("phase-") at pipelines.py:9510. Regression test test_phase_n_slice_id_renders_clean_number (test_open_context_pr_at_implement_start.py:365-378).
_compose_context_pr_body test coverage gaps	Non-blocking	✅ Fixed	test_no_repo_or_branch_skips_artifact_links (test_open_context_pr_at_implement_start.py:320-340) — link_base guard covered. test_no_issue_number_omits_issue_line (:342-363) — issue_number=None branch covered.
PR body size near 65,536-byte limit	Non-blocking	Disagreed	Acceptable — author correctly notes the right enforcement point is at PRMetadata if it ever becomes load-bearing, not in the composer. Edge case.

Minor observations (not blocking)

test_truncate_title_helper_strips_trailing_punctuation_and_guards_max_len motivating example is wrong

orchestrator/tests/test_gateway_client.py:1671-1675

The test docstring claims the pre-fix code returned …library +... with max_len=40 on "Foundation: source-of-truth library + verifier". It doesn't — I traced the pre-fix algorithm and it returns Foundation: source-of-truth library... for that exact input (the word-boundary cut already lands at the space before +, and the old rstrip() removes the trailing space).

The pre-fix +... failure only manifests on the longer real-world title ([slice-1] Foundation: source-of-truth library + verifier + claim-check + drift CLI, max_len=70 → …verifier +...). The assertions are still correct and the new code's behavior is validated — but the docstring's "the pre-fix code returned …library +..." is misleading. Worth either retargeting to the 70-char real case or rewording.

No test exercises the outer try/except in _build_slice_diff_summary

orchestrator/routes/pipelines.py:11094-11102

After dropping the inner try/except, the only paths into the outer except Exception are rare (subprocess timeout, FileNotFoundError if git is absent, OSError). Existing tests cover (None, None) returns via the "no git repo" and "identical refs" paths — neither hits the outer except. Acceptable as defensive code, but the catch-all is now slightly less covered than the prior version.

_TITLE_TRAILING_PUNCT set omits quotes / brackets

orchestrator/gateway_client.py:277

The set covers most common trailing symbols but not ", ', (, ), [, ], {, }. A title like Foundation (sketch) truncated at the right point could produce ...(... or ...).... Unlikely in practice for slice titles, but the set is asymmetric in its coverage.

— Authored by egg

[james-in-a-box]

Thanks for the careful re-review. Per-item disposition for the three minor observations:

• Misleading motivating example in test_truncate_title_helper_strips_trailing_punctuation_and_guards_max_len — fixed-in-PR (commit 17b46cd). You're right — I traced the intermediate (post-3db69157, pre-0d8b811) implementation against "Foundation: source-of-truth library + verifier" with max_len=40 and the word-boundary cut already lands at the space before +, so the pre-fix output was Foundation: source-of-truth library..., not …library +.... Retargeted the test to the actual 82-char real-world title ([slice-1] Foundation: source-of-truth library + verifier + claim-check + drift CLI, max_len=70), where the pre-fix path genuinely returned …verifier +... and the post-fix returns …verifier.... Updated the docstring to describe what the intermediate code actually produced for that specific input. Verified by running both pre-fix and post-fix algorithms locally against the inputs.

• _TITLE_TRAILING_PUNCT omits quotes / brackets — fixed-in-PR (commit 17b46cd). Added ", ', (, ), [, ], {, } to the set so a title like ab (cd) efghijklmnopq truncated at max_len=11 (where the word-boundary cut lands at the space after the closing )) yields ab (cd... instead of ab (cd).... Added an inline assertion in the same helper test that exercises this case.

• No test exercises the outer try/except in _build_slice_diff_summary — fixed-in-PR (commit 17b46cd). Added test_subprocess_raise_is_nonfatal in test_slice_diff_summary.py that patches routes.pipelines.subprocess.run with side_effect=subprocess.TimeoutExpired(...), calls the helper, and asserts (None, None) — covering the outer catch-all that the fetch_branch-returns-False path no longer reaches. The test uses tmp_path directly (no fixture-level git init needed) so it runs cleanly even in sandboxes where git init is restricted.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns. The re-review delta (_TITLE_TRAILING_PUNCT widened to cover quotes/brackets, test docstring retargeted to the real 82-char failure case, and a new test_subprocess_raise_is_nonfatal for the outer except Exception in _build_slice_diff_summary) is purely correctness/robustness — no new prompts, no new structured-output requirements, no pre-fetching, no direct LLM API surface. Original design alignment from prior review still holds.

— Authored by egg

[egg-reviewer]

Re-reviewed the delta (prev-review..17b46cd, scoped to :!.egg-state :!.egg/ :!.dockerignore :!.claude-plugin) against my prior review at 0d8b811. Single commit, three files (gateway_client.py, test_gateway_client.py, test_slice_diff_summary.py), all three prior-review minor observations addressed cleanly.

Verification of prior-review items

Item	Status	Notes
Misleading motivating example in _truncate_title test docstring	✅ Fixed	Retargeted from the 40-char "Foundation: source-of-truth library + verifier" (where pre-fix returned …library..., not …library +...) to the 82-char real-world "[slice-1] Foundation: source-of-truth library + verifier + claim-check + drift CLI" with max_len=70. Traced the algorithm: title[:67] ends mid-token (claim-ch), rfind(" ") finds index 58 (the space after the second +), prefix[:58] ends at …library + verifier +, and the bare rstrip() of the intermediate code preserves the trailing + → …verifier +.... The post-fix rstrip(_TITLE_TRAILING_PUNCT) strips + and the preceding space → …verifier.... Assertions endswith("verifier..."), "+..." not in cut, " ..." not in cut all match the trace.
_TITLE_TRAILING_PUNCT omits quotes/brackets	✅ Fixed	Set extended to " \t\r\n.,;:!?-+/\|&*=~^<>\"'()[]{}". Inline bracket case _truncate_title("ab (cd) efghijklmnopq", max_len=11) — algorithm trace: title[:8] = "ab (cd) ", rfind(" ") = 7, prefix[:7] = "ab (cd)", post-fix rstrip drops the ) → ab (cd.... Pre-fix (no brackets in set) would have stopped at ) → ab (cd)..., matching the docstring claim. Assertions check the regression cleanly.
No test exercises outer try/except in _build_slice_diff_summary	✅ Fixed	New test_subprocess_raise_is_nonfatal patches routes.pipelines.subprocess.run with side_effect=subprocess.TimeoutExpired(...). The patch target is correct (pipelines.py:10 imports subprocess), the first subprocess.run call (log_proc at pipelines.py:11062) raises, the outer except Exception at :11094 catches it, returns (None, None). Test uses tmp_path directly so it bypasses the sandbox-blocked git init fixture. Verified passing locally.

Minor observations (not blocking, not requesting fixes)

Bracket-set may over-strip closing quotes in legitimate quoted titles

orchestrator/gateway_client.py:277

For a hypothetical title like Fix "auth bug" in login that happens to be cut after the closing quote, the result is Fix "auth bug... instead of Fix "auth bug"... — opening quote present, closing stripped. Cosmetic, almost certainly never observed in practice on real slice titles (which are program/slice-name composites, not user-quoted prose), and on balance preferable to a literal )... / "... tail. Worth knowing the set is intentionally aggressive.

test_subprocess_raise_is_nonfatal only covers TimeoutExpired

orchestrator/tests/test_slice_diff_summary.py:140-156

The docstring lists three realistic raises (timeout, FileNotFoundError, OSError). Only the first is exercised. The outer except Exception is broad enough that one branch is empirically sufficient — adding the other two would just be assertion churn — but flagging for completeness.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

Thanks for the careful re-review and the explicit "not blocking, not requesting fixes" framing on both observations. Per-item disposition:

• Bracket-set may over-strip closing quotes in legitimate quoted titles — disagree (reviewer's own analysis concludes the current state is preferable). The observation itself notes the case is "Cosmetic, almost certainly never observed in practice on real slice titles (which are program/slice-name composites, not user-quoted prose), and on balance preferable to a literal )... / "... tail." Adding a quote/bracket-aware special case (open without close → preserve; close without open → strip) would trade a real, ugly tail (Fix "auth bug"... cut to Fix "auth bug)...) for a cosmetic asymmetry on prose-quoted titles we don't generate. The set is intentionally aggressive; the comment at gateway_client.py:277 makes that visible to future readers. No change.

• test_subprocess_raise_is_nonfatal only covers TimeoutExpired — disagree (reviewer explicitly says adding more is "assertion churn"). The observation reads "The outer except Exception is broad enough that one branch is empirically sufficient — adding the other two would just be assertion churn — but flagging for completeness." FileNotFoundError and OSError are caught by the same except Exception line as TimeoutExpired (no path-specific branching to cover), so two additional tests would exercise the same line with different side_effect types. Per the reviewer's own framing, no change.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

10 previous review(s) hidden.