Unwrap PR-body soft breaks + cross-link slice PRs from the context PR (#3122)

[jwbron]

Closes #3122 (remaining scope after #3118 — see the rescope comment).

Two gaps survived the #3115/#3118 body composer: prose fields still rendered their YAML block-scalar hard wraps as choppy line breaks on GitHub, and the stack was only cross-linked in one direction (slice → context) — the context PR's slice table never learned which PRs actually opened, and slice PR numbers weren't persisted anywhere (create_slice_pr's return value was discarded).

Changes

Soft-break unwrapping (egg_contracts.markdown.unwrap_soft_breaks, new):

• Joins single newlines inside paragraphs back into prose while preserving real markdown structure: blank-line paragraph breaks, bullet/ordered list items (wrapped list-item tails join into the bullet — lazy continuation), headings, blockquotes, tables, fenced + indented code, thematic breaks/setext underlines, link-reference definitions, and explicit hard breaks (trailing double-space / backslash). Idempotent; conservative on anything that could be structural.
• Applied in _compose_context_pr_body (pr.description / test_plan / manual_steps) and create_slice_pr (the slice_goal lead, the first-sentence blurb fallback, and the no-base-PR inline program narrative). Planner guidance docs updated to say "wrap freely".

Context→slice cross-links:

• Slice.pr_number / Slice.pr_url added to the contract model. The run loop now captures create_slice_pr's returned URL (including the idempotent already-open hit, so resumes recover the linkage), parses the number with the same trailing-boundary /pull/(\d+) pattern the context-PR opener uses, and persists both in the same contract write as status=COMPLETE.
• The context PR body's slice-table entries render — #N for every slice with a recorded PR number, and a new _refresh_context_pr_body re-composes + pushes the body after each slice PR opens. Strictly best-effort: every failure path (contract load, composition, gateway) logs and returns False; no slice outcome depends on it.
• Transport is a new GatewayClient.update_pr_body — synthetic phase-less session → existing /api/v1/gh/pr/edit route (gh api ... -X PATCH -f body=...), the same seam rebase_onto uses for base retargets. No new gateway surface; the PR-ownership policy still bounds it to bot-authored PRs.

Body-ownership semantics: pipeline-generated PR bodies are machine-owned — each refresh fully regenerates the body and clobbers manual edits. Documented in docs/architecture/sdlc-pipeline.md, docs/architecture/slice-dag.md, and docs/guides/sdlc-pipeline.md rather than building delimited-section merging.

Test plan

• New: tests/shared/egg_contracts/test_markdown.py (20 cases over the reflow heuristic), TestUpdatePrBody + goal/narrative-unwrap cases in test_gateway_client.py (mock gateway server grew a /api/v1/gh/pr/edit handler), TestRefreshContextPrBody + slice-link/unwrap composer cases in test_open_context_pr_at_implement_start.py, linkage-persisted/body-refreshed + unparseable-URL run-loop cases in test_slice_run_loop_integration.py.
• make lint green. Targeted suites all green: test_markdown.py (20), test_gateway_client.py (133), test_open_context_pr_at_implement_start.py (34), test_context_pr_opener.py + contracts model/parser suites (234), test_slice_run_loop_integration.py (42). A partial make test run (13,368 passed before being cut short) showed only the pre-existing tests/scripts/test_reap_stale_egg_images.py host failures also noted in Compose context-PR body + restructure slice-PR body (#3115) #3118, untouched by this change.

[egg-reviewer]

No agent-mode design concerns.

This is deterministic orchestrator code (markdown reflow + a gateway-routed PR body update) for rendering machine-generated PR bodies on GitHub. No new agent prompts, no pre-fetched diffs/logs baked into prompts, no structured-output-for-humans, no post-processing of agent output, and no direct LLM API calls. The planner-facing doc change ("wrap freely when authoring") is orienting, not constraining.

— Authored by egg

[egg-reviewer]

Review: PR #3128 — Soft-break unwrap + slice cross-links

I traced the full path from contract write through _refresh_context_pr_body → GatewayClient.update_pr_body → gateway's /api/v1/gh/pr/edit, examined the markdown unwrapper heuristic against CommonMark edge cases, and stepped through the new tests against the production paths. The design is sound, the framing as "best-effort / machine-owned" is honest, and the tests exercise the production code paths rather than building bypassing fixtures. No blocking issues. A handful of non-blocking concerns and edge cases below — the most consequential is the concurrent-refresh race because it can permanently drop the last slice's link.

Non-blocking — concurrent-refresh race can drop the final slice's link

_run_implement_phase_slices runs ready slices through ThreadPoolExecutor (orchestrator/routes/pipelines.py:16981-16988), so two slices in the same wave can be in the post-PR-creation block in parallel. The completion sequence is:

1. _persist_slice_status_complete(...) writes Slice.pr_number under get_pipeline_state_lock(pipeline_id) and returns.
2. _refresh_context_pr_body(...) runs without the lock — it loads the contract, composes the body, and pushes it to GitHub.

Interleaving two slices A and B with the second _refresh_context_pr_body started before but completing after the other:

T1  A: persist (lock A's pr_number, release)
T2  A: refresh start — load contract (sees A only)
T3  B: persist (lock B's pr_number, release)
T4  B: refresh start — load contract (sees A and B)
T5  B: refresh composes body with A's link + B's link, pushes
T6  A: refresh composes body with A's link only, pushes        ← clobbers T5

The final body on GitHub now has A's link but not B's. Because there is no subsequent slice completion (B was the last), no later refresh fires and B's — #N stays missing forever. The pipeline state and the slice PR itself are correct; only the context-PR slice-table entry for the last-completing slice is silently wrong.

This is observable, not theoretical — GitHub PATCH latency varies enough that A's push completing after B's is plausible whenever two slices finish within a few hundred ms of each other.

Suggested fixes, in increasing cost:

• Run the compose + push inside get_pipeline_state_lock(pipeline_id). The lock then bounds the latency of the slice-completion path, but it eliminates the race.
• Replace the inline refresh with a single-worker queue per pipeline (drop-coalesce duplicates), so refreshes serialize regardless of slice-worker concurrency.
• Add a final "stack settled" refresh in the run loop's terminal block after all slices have completed — even if interim refreshes race, the final one is sequential.

The framing as "best-effort / cosmetic" justifies not blocking on this, but the docstring should call out the last-slice failure mode so the next reader knows to expect it.

Non-blocking — update_pr_body container_id collides on concurrent refreshes

orchestrator/gateway_client.py:1980 hard-codes temp_container_id = f"{pipeline_id}-pr-body-update". Two concurrent refreshes for the same pipeline (the same wave that triggers the race above) hit register_session with the same container_id. create_pr, rebase_onto, and the rest of the synthetic-session sites have the same shape, so this isn't a regression — but if the gateway's session table is keyed by container_id, the second registration races against the first's delete_session in the finally. Worth either:

• Suffixing the container id with uuid.uuid4().hex[:8] (matches the slice-id-uniqueness the rest of the synthetic-session sites get for free), or
• Documenting the assumption that the gateway tolerates concurrent same-container_id registrations.

Non-blocking — agent_role lost on the refresh path

_refresh_context_pr_body (orchestrator/routes/pipelines.py:9785-9792) calls update_pr_body without agent_role. Sibling orchestrator-driven PR mutations (create_slice_pr callsite at 16878, rebase_onto callsites at 16518/16589) consistently pass agent_role="orchestrator" so the gateway's audit log can attribute the action. Pass agent_role="orchestrator" here too for parity — the audit trail otherwise has a hole where context-PR body edits land as agent_role=None.

Non-blocking — string pr_number would crash instead of returning False

update_pr_body short-circuits on bad input at gateway_client.py:1971:

if not repo or pr_number is None or isinstance(pr_number, bool) or pr_number < 1:

If a future caller passes pr_number="123" the pr_number < 1 clause raises TypeError (Python 3 comparison) — the function never reaches the return False. Today's sole caller passes the int from contract.pr.context_pr_number or pipeline.pr_number, both int | None, so this is theoretical. But the contract advertised by the method is "returns False on any failure," and TypeError is reachable. Cheap fix: not isinstance(pr_number, int) ahead of pr_number < 1.

The test parametrization at test_gateway_client.py:1551 covers [None, 0, -1, True] but not a non-int — worth adding "123" to lock in the contract.

Non-blocking — body="" would burn a session

update_pr_body validates repo and pr_number but not body. The gateway's gh_pr_edit requires "at least one of title, body, or base" (gateway/gateway.py:3431), so an empty body produces a 400, the except Exception returns False, and we burn a synthetic-session create+delete round-trip per call. Composing produces "" only on a degenerate contract (no pr, no issue_number, no slices, no link_base), which the type system makes hard to reach, but a if not body: return False ahead of the session-registration block is essentially free.

Non-blocking — unwrap_soft_breaks heuristic gaps

The heuristic is intentionally conservative, but two corners aren't covered today:

Setext heading without preceding blank line. A planner writing:

Some prose
Heading text
===
prose below

Heading text is joined into Some prose (both _is_prose), so the output becomes Some prose Heading text\n===\nprose below. The === is now an orphan thematic break, not a heading underline. ATX headings (##) are by far more common, but a single test asserting unwrap_soft_breaks("Some prose\nHeading\n===\n") does not corrupt the heading would either lock in the current (broken) behavior or catch a future fix.

HTML block opens get joined. Lines starting with <details>, <div>, etc. are treated as prose (_BLOCK_MARKER has no HTML block clause), so they fold into preceding prose. Rare in PR bodies but worth a one-line carve-out (^\s{0,3}<[a-zA-Z]) for safety since GitHub renders raw HTML.

Both are limitations of the conservative design rather than bugs; mention them in the docstring so the next reviewer knows where the heuristic stops.

Non-blocking — bootstrap / merged-skip paths never record pr_number

_persist_slice_status_complete is called without pr_number from three sites:

• Bootstrap layer-A (pipelines.py:16158) — slices the contract already had as COMPLETE.
• Bootstrap layer-B (pipelines.py:16222) — slices detected as merged on origin.
• Merged-skip in the run loop (pipelines.py:16541-16542) — slice's branch already merged into parent.

None of these query gh pr list --head … --state merged to recover the PR number, so on a pipeline resumed past those points the slice-table entries for merged slices stay unlinked. Acceptable for v1 — the per-slice link is most useful while the stack is live — but worth a TODO comment so the next reader knows it's a deliberate omission, not an oversight.

Non-blocking — pr_url written without validating pr_number ≥ 1

re.search(r"/pull/(\d+)(?:[/?#]|$)", slice_pr_url) captures \d+, which includes 0. Slice.pr_number has ge=1, and EggContractBaseModel sets validate_assignment=True, so s.pr_number = 0 would raise inside the _persist_slice_status_complete try/except and silently downgrade to a warning log. GitHub PR numbers start at 1, so this is theoretical — but if you wanted to be tidy, change the regex to /pull/([1-9]\d*)(?:[/?#]|$) and skip the implicit validate_assignment rescue.

What's good

• The unwrap heuristic correctly handles fence open/close character matching, lazy continuation into list items and blockquotes, hard breaks, and the idempotent property.
• Tests exercise the real composer (_compose_context_pr_body) rather than asserting against hand-built strings — a regression in the unwrapper would break them.
• The "machine-owned body" call-out in three doc files is the right tradeoff to surface — better than building a delimited-section merger.
• update_pr_body correctly uses gh api -f (raw-field, not -F), so a body starting with @ is sent literally and not interpreted as a file ref.
• The PR number parse uses the trailing-boundary pattern (?:[/?#]|$) so /pull/12345/files parses but /pulled-files/12345 doesn't smuggle a wrong number — matches the existing opener's pattern.

— Authored by egg

[james-in-a-box]

Thanks for the thorough trace through _refresh_context_pr_body → update_pr_body → /api/v1/gh/pr/edit. Addressed all eight non-blocking items in b1ae69b:

• Concurrent-refresh race can drop the final slice's link — fixed-in-PR (commit b1ae69b). Took option 1 from your suggestion list: wrapped the _persist_slice_status_complete + _refresh_context_pr_body pair in get_pipeline_state_lock(pipeline_id) at the run-loop call site. The lock is an RLock so the inner reacquire inside _persist_slice_status_complete is fine, and serialising compose+push eliminates the interleaving you described — A always sees its own write before B can start composing, and the last-slice path is no longer special. Bounded the tail latency by one gateway PATCH per concurrent slice rather than racing them. Recorded the concurrency contract on _refresh_context_pr_body's docstring so the next reader knows the caller must hold the lock.
• update_pr_body container_id collides on concurrent refreshes — fixed-in-PR (commit b1ae69b). Suffixed temp_container_id with uuid.uuid4().hex[:8]. Matches the per-slice-id uniqueness create_pr / rebase_onto already get for free.
• agent_role lost on the refresh path — fixed-in-PR (commit b1ae69b). _refresh_context_pr_body now passes agent_role="orchestrator" to update_pr_body, matching create_slice_pr / rebase_onto. Added an assertion in test_happy_path_pushes_recomposed_body to lock it in.
• String pr_number would crash instead of returning False — fixed-in-PR (commit b1ae69b). Added not isinstance(pr_number, int) ahead of pr_number < 1. Extended the parametrized test to include "123" and 1.0.
• body="" would burn a session — fixed-in-PR (commit b1ae69b). Added a not body short-circuit before the session-registration block. New test test_update_pr_body_empty_body_short_circuits pins it.
• unwrap_soft_breaks heuristic gaps — fixed-in-PR (commit b1ae69b). Added an HTML block carve-out to _BLOCK_MARKER (</?[a-zA-Z]) so <details> / <div> / etc. never fold into preceding prose. New test_preserves_html_block_open locks it in. For the setext-without-preceding-blank-line case I went with the docstring-call-out + a regression test (test_setext_heading_without_blank_line_known_limitation) that pins the current behaviour — the conservative line-local heuristic can't disambiguate Heading text\n=== from prose without lookahead, and a setext fix risks regressing the more-common ATX path. Documented the workaround (blank line or ATX) in the module docstring.
• Bootstrap / merged-skip paths never record pr_number — fixed-in-PR (commit b1ae69b). Added a TODO(Compose structured PR bodies for context and slice PRs (use test_plan/manual_steps, unwrap soft breaks, cross-link the stack) #3122) block to _persist_slice_status_complete's docstring documenting the three pr_number=None callers as a deliberate v1 omission (per-slice link is most useful while the stack is live), with the backfill path (gh pr list --head … --state merged) called out for the next reader.
• pr_url written without validating pr_number ≥ 1 — fixed-in-PR (commit b1ae69b). Tightened the run-loop parse from /pull/(\d+) to /pull/([1-9]\d*). A malformed /pull/0/... URL now skips linkage instead of falling through to the Slice.pr_number ge=1 validator's silent-rescue path.

make lint clean; targeted tests green (test_markdown.py 22, TestUpdatePrBody 10, TestRefreshContextPrBody 5, test_slice_run_loop_integration.py 42).

— Authored by egg

[egg-reviewer]

No agent-mode design concerns in the delta since 0e732bf.

The re-review commit (b1ae69b) addresses the eight non-blocking items from the prior general-code review: serializing _persist_slice_status_complete + _refresh_context_pr_body under the per-pipeline state lock, suffixing the synthetic-session container id with uuid.uuid4().hex[:8], passing agent_role="orchestrator" on the refresh path, tightening update_pr_body input validation (non-int pr_number, empty body), narrowing the slice-PR URL parse to [1-9]\d*, carving out HTML block opens in _BLOCK_MARKER, documenting the setext-without-blank-line limitation, and a TODO for merged-slice backfill.

All of this is deterministic orchestrator + markdown-reflow code. No new agent prompts, no pre-fetched diffs/logs baked into prompts, no structured-output-for-humans, no post-processing of agent output, and no direct LLM API calls. Original approval stands.

— Authored by egg

[egg-reviewer]

Re-review: PR #3128 — Eight review items addressed in b1ae69b

Traced the delta against the previous review (0e732bf..b1ae69b). Every non-blocking item from the prior pass has been addressed cleanly; no new issues introduced.

Verified fixes

1. Concurrent-refresh race (orchestrator/routes/pipelines.py:16965-16986). The record_complete + _persist_slice_status_complete + _refresh_context_pr_body sequence now runs inside get_pipeline_state_lock(pipeline_id). The lock is a threading.RLock (orchestrator/state_store.py:1521), so the inner reacquire in _persist_slice_status_complete:16068 is safe. _refresh_context_pr_body's docstring (pipelines.py:9741-9750) records the concurrency contract: caller must hold the lock for the entire load + compose + push sequence. The single production caller (the run loop) honors it.

The lock now wraps a gateway PATCH call; that's an intentional tail-latency tradeoff (one PATCH per concurrent slice, no race). update_pr_body inherits the gateway-client timeout, so the lock has a bounded hold time.

2. container_id collision (gateway_client.py:2006). temp_container_id = f"{pipeline_id}-pr-body-update-{uuid.uuid4().hex[:8]}". 4 bytes of entropy is plenty against same-pipeline concurrent refreshes, and no other code in the orchestrator or gateway depends on the deterministic container_id (grep confirms).

3. agent_role on refresh (pipelines.py:9805). _refresh_context_pr_body now passes agent_role="orchestrator" to update_pr_body, matching create_slice_pr / rebase_onto. test_happy_path_pushes_recomposed_body asserts kwargs["agent_role"] == "orchestrator".

4. Non-int pr_number (gateway_client.py:1972-1985). Guard order is correct: bool check first (since bool is an int subclass and True < 1 is False, the int check alone wouldn't catch it), then not isinstance(pr_number, int), then pr_number < 1. Parametrized test extended to [None, 0, -1, True, "123", 1.0] — "123" would previously have raised TypeError; now it returns False without burning a session.

5. Empty body short-circuit (gateway_client.py:1991-1998). if not body: return False before any session registration. test_update_pr_body_empty_body_short_circuits pins it.

6. HTML block carve-out (shared/egg_contracts/markdown.py:57). Added </?[a-zA-Z] to _BLOCK_MARKER. Traced the test_preserves_html_block_open case: <details> and <summary>hidden</summary> both match the new clause; body doesn't join into </summary> because _is_prose returns False on a block-marker line; </details> also stays opaque. Output equals input. ✓

The clause is broader than CommonMark's HTML block rules — any line beginning with <letter> or </letter> is treated as non-prose, so a prose line like <command> runs the thing is preserved unjoined. That's the conservative direction (no corruption risk) and is called out in the module docstring.

The setext-without-blank-line corner is documented as a known limitation, with test_setext_heading_without_blank_line_known_limitation locking in the current behaviour so a future fix is a deliberate change, not an accident. Reasonable tradeoff — fixing it correctly requires lookahead, and the workaround (blank line or ATX) is what every planner should be doing anyway.

7. TODO on bootstrap/merged-skip (pipelines.py:16057-16065). Docstring on _persist_slice_status_complete now records the three pr_number=None callsites as a deliberate v1 omission, with the backfill path (gh pr list --head … --state merged) called out for the next reader. Acceptable scope deferral.

8. Slice PR URL parse tightening (pipelines.py:16948). Regex narrowed from /pull/(\d+) to /pull/([1-9]\d*). /pull/0 no longer matches, so Slice.pr_number's ge=1 validator never sees an invalid input. The existing test_unparseable_slice_pr_url_skips_linkage_and_refresh covers the "no match → skip linkage" branch; a /pull/0 URL falls into that branch under the new regex. No regression risk on real URLs (PR numbers are ≥1 by GitHub design).

CI

Unit Tests, Lint, Integration Tests all SUCCESS on b1ae69b.

Verdict

Approved. The follow-up commit cleanly closes every item from the prior review with appropriate test coverage. No new blocking issues, no new suggestions.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

5 previous review(s) hidden.