diff --git a/docs/architecture/sdlc-pipeline.md b/docs/architecture/sdlc-pipeline.md index 00fd5811e0..1a949162c3 100644 --- a/docs/architecture/sdlc-pipeline.md +++ b/docs/architecture/sdlc-pipeline.md @@ -195,6 +195,22 @@ the agent reads to recover the task: > links are branch-qualified absolute URLs > (`https://github.com//blob//...`) because GitHub > resolves relative links in PR bodies against the default branch. +> +> **Body reflow + stack cross-links (#3122).** Prose fields +> (`pr.description` / `pr.test_plan` / `pr.manual_steps`, and the +> per-slice `goal` lead on slice PRs) are treated as markdown source, +> not preformatted text: `egg_contracts.markdown.unwrap_soft_breaks` +> joins the ~75-char YAML block-scalar hard wraps back into paragraphs +> (lists, headings, tables, code fences, and explicit hard breaks are +> preserved) so GitHub renders prose instead of a choppy column. After +> each slice PR opens, the run loop records its number/URL on the +> contract slice (`Slice.pr_number` / `Slice.pr_url`) and re-composes + +> pushes the context-PR body via the gateway's `gh pr edit` REST route, +> so the slice table gains a `— #N` link to each slice PR as the stack +> materialises. Pipeline-generated PR bodies are **machine-owned**: the +> refresh fully regenerates the body and clobbers manual edits. The +> refresh is best-effort/cosmetic — failures log and never fail the +> slice. ### Schema v1.1 → v1.2 migration note (#2777) diff --git a/docs/architecture/slice-dag.md b/docs/architecture/slice-dag.md index d3b407209c..cafccfa27e 100644 --- a/docs/architecture/slice-dag.md +++ b/docs/architecture/slice-dag.md @@ -539,7 +539,18 @@ every slice PR is purely slice-scoped. criteria behind a `
` fold) → `## Stack` (position, base PR, base branch). Program-level test plan, manual steps and pre-merge obligations live on the up-front context PR (#2777), not - on any slice PR. + on any slice PR. Prose fields (`goal`, inlined program narrative) + have their YAML block-scalar hard wraps joined back into paragraphs + before rendering (`unwrap_soft_breaks`, #3122). +- **Reverse linkage (#3122).** After a slice PR opens, the run loop + parses the PR number from the returned URL, persists it on the + contract slice (`Slice.pr_number` / `Slice.pr_url` — also on the + idempotent already-open path, so resumes recover the linkage), and + refreshes the machine-owned context-PR body so its slice table links + the new PR (`— #N`). The refresh routes through + `GatewayClient.update_pr_body` (synthetic session → + `/api/v1/gh/pr/edit`, same seam as `rebase_onto`'s base retarget) + and is strictly best-effort: failures log and never fail the slice. - **No `context_pr_number` — should not occur under #2777.** Because the context PR is opened up-front, hard-required and idempotent at the plan→implement boundary, every slice PR sees a populated diff --git a/docs/guides/sdlc-pipeline.md b/docs/guides/sdlc-pipeline.md index 9703c20eb5..a4fc7168bc 100644 --- a/docs/guides/sdlc-pipeline.md +++ b/docs/guides/sdlc-pipeline.md @@ -871,7 +871,7 @@ Template sections: - [TASK-1-2] Add role validation — Acceptance: Unauthorized mutations rejected ``` -**PR Metadata**: The plan should include a `pr:` section in the YAML appendix with a title, description, test plan, and manual steps for the pull request. The `test_plan` field is required — describe both automated test coverage and manual verification steps. The `manual_steps` field lists any pre- or post-merge actions (migrations, config changes, deployments); use an empty string if none. The context PR's body renders all of these: the description verbatim, then `## Test Plan` / `## Manual Steps` sections, then a generated `## Pipeline context` footer linking the originating issue, the slice table, and the analysis/plan drafts and BRC transcripts committed on the work branch (#3115). Each slice's `goal` field is likewise rendered verbatim as that slice's PR-body lead paragraph, so write it as a reviewer-facing summary. If not provided, the orchestrator falls back to the issue title (or a generic stub) and opens the PR as a **draft** with a warning banner in the body that lists any parse errors from the plan draft, so reviewers cannot silently merge a PR whose planner metadata is missing (see #1975). +**PR Metadata**: The plan should include a `pr:` section in the YAML appendix with a title, description, test plan, and manual steps for the pull request. The `test_plan` field is required — describe both automated test coverage and manual verification steps. The `manual_steps` field lists any pre- or post-merge actions (migrations, config changes, deployments); use an empty string if none. The context PR's body renders all of these: the description, then `## Test Plan` / `## Manual Steps` sections, then a generated `## Pipeline context` footer linking the originating issue, the slice table, and the analysis/plan drafts and BRC transcripts committed on the work branch (#3115). Each slice's `goal` field is likewise rendered as that slice's PR-body lead paragraph, so write it as a reviewer-facing summary. All of these prose fields are treated as markdown source, not preformatted text: single newlines inside paragraphs (YAML block-scalar hard wraps) are joined back into prose before rendering, while lists, headings, code fences, and blank-line paragraph breaks are preserved (#3122) — so wrap freely when authoring. If not provided, the orchestrator falls back to the issue title (or a generic stub) and opens the PR as a **draft** with a warning banner in the body that lists any parse errors from the plan draft, so reviewers cannot silently merge a PR whose planner metadata is missing (see #1975). ### Phase Completion Comments @@ -991,6 +991,7 @@ Default checks for each phase are defined in `shared/egg_contracts/phase_default **Context PR (opened at implement-phase start, #2777):** - The terminal "PR phase" was **deleted** as a separate pipeline stage; `IMPLEMENT` is now terminal. The context PR (`egg//work → main`) is opened by the orchestrator at the plan→implement boundary via `_open_context_pr_at_implement_start` — no separate phase or agent is spawned for this. The open is hard-required and idempotent (`GatewayClient.lookup_open_pr` pre-flight with a server-side `gh pr list --head … --base … --limit 1` filter). Per-slice PRs are opened inline by `create_slice_pr`, which uses the same `GatewayClient.lookup_open_pr` primitive (#2777 cq-8 / #2934 unified both PR-idempotency paths onto it). - The PR title is sourced from `contract.pr.title` (populated by the plan agent); the body is composed by `_compose_context_pr_body` (#3115) from `contract.pr.description`, rendered `## Test Plan` / `## Manual Steps` sections, and a generated `## Pipeline context` footer (pipeline id, originating issue, slice table, branch-qualified absolute-URL links to the refine analysis draft / plan draft, and the per-phase BRC transcripts committed on the work branch). The `## Pipeline context` header is suppressed when only the bare pipeline-id line would be emitted. The BRC-history transcript line is rendered inline inside the footer (the helper calls `_build_brc_history_link_line` while building the footer body) rather than appended as a separate paragraph; see [Concurrent Execution — BRC History Link in PR Body](concurrent-execution.md#brc-history-link-in-pr-body) for the link-line shape. +- **Stack cross-links + machine-owned body (#3122)**: after each slice PR opens, the run loop persists its number/URL on the contract slice (`Slice.pr_number` / `Slice.pr_url`) and re-composes + pushes the context-PR body through the gateway (`GatewayClient.update_pr_body` → `/api/v1/gh/pr/edit`), so the footer's slice table links each slice PR (`— #N`) as the stack materialises. The context-PR body is machine-owned: each refresh fully regenerates it, clobbering manual edits. The refresh is best-effort — a failure logs a warning and never fails the slice. - **Draft preservation**: Pipeline-specific draft files (`.egg-state/drafts/{id}-analysis.md`, `.egg-state/drafts/{id}-plan.md`) are **preserved** on the PR branch as artifacts of the pipeline's reasoning. Reviewers can compare the planned approach against the shipped code, and post-hoc debugging has the analysis and plan available as a baseline (#1713). - If the up-front context-PR open fails (after the idempotent pre-flight returns no existing PR and `gh pr create` itself fails), the pipeline is marked **FAILED** immediately — there is no terminal back-stop because the open is hard-required up-front. diff --git a/orchestrator/gateway_client.py b/orchestrator/gateway_client.py index 46ced11f8a..9af652427a 100644 --- a/orchestrator/gateway_client.py +++ b/orchestrator/gateway_client.py @@ -16,6 +16,7 @@ import subprocess import sys import time +import uuid from collections.abc import Callable from dataclasses import dataclass from datetime import datetime, timedelta @@ -50,6 +51,15 @@ def get_logger(name: str, **kwargs) -> logging.Logger: # type: ignore[misc] GATEWAY_CONTAINER_NAME = "egg-gateway" GATEWAY_PORT = 9848 # noqa: EGG002 +try: + from egg_contracts.markdown import unwrap_soft_breaks +except ImportError: + # Fallback: render prose verbatim when egg_contracts is unavailable + # (matches the egg_logging/egg_config degradation above). + def unwrap_soft_breaks(text: str | None) -> str: # type: ignore[misc] + return text or "" + + logger = get_logger("orchestrator.gateway_client") T = TypeVar("T") @@ -1830,9 +1840,12 @@ def create_slice_pr( # description just below (the blurb would duplicate its first # sentence). inline_program_narrative = has_program_title and not has_base_pr - lead = (slice_goal or "").strip() + # Soft-break unwrapping (#3122): the goal / description reach us + # as YAML block scalars hard-wrapped at ~75 chars, and GitHub + # renders every newline in a PR body as a line break. + lead = unwrap_soft_breaks(slice_goal).strip() if not lead and program_description and not inline_program_narrative: - lead = _first_sentence(program_description) + lead = _first_sentence(unwrap_soft_breaks(program_description)) if lead: body_lines.append(lead) body_lines.append("") @@ -1851,7 +1864,7 @@ def create_slice_pr( # ``/work``. The stack is structurally unmergeable in this # state — fixing the body here is a UX backstop, not a fix. if program_description and program_description.strip(): - body_lines.append(program_description.strip()) + body_lines.append(unwrap_soft_breaks(program_description).strip()) body_lines.append("") _append_diff_summary_section(body_lines, diffstat, commit_subjects) @@ -1861,12 +1874,12 @@ def create_slice_pr( if program_test_plan and program_test_plan.strip(): body_lines.append("## Test Plan") body_lines.append("") - body_lines.append(program_test_plan.strip()) + body_lines.append(unwrap_soft_breaks(program_test_plan).strip()) body_lines.append("") if program_manual_steps and program_manual_steps.strip(): body_lines.append("## Manual Steps") body_lines.append("") - body_lines.append(program_manual_steps.strip()) + body_lines.append(unwrap_soft_breaks(program_manual_steps).strip()) body_lines.append("") # ``## Stack`` block — parent PR + base PR + position. Replaces @@ -1926,6 +1939,120 @@ def create_slice_pr( draft=draft, ) + def update_pr_body( + self, + pipeline_id: str, + repo: str, + *, + pr_number: int, + body: str, + issue_number: int | None = None, + agent_role: str | None = None, + mode: Literal["public", "private"] = "public", + ) -> bool: + """Replace an existing PR's body via the gateway (#3122). + + Routes through the per-agent ``/api/v1/gh/pr/edit`` endpoint + (``gh api repos//pulls/ -X PATCH -f body=...``) under a + synthetic phase-less session, exactly like :meth:`create_pr` / + :meth:`rebase_onto` — no new privileged orchestrator endpoint. + The gateway's PR-ownership policy still applies, which is the + desired bound: the orchestrator only rewrites PRs the egg bot + user authored. + + Sole production caller is the run loop's context-PR refresh: + after a slice PR opens, the context PR body is recomposed with a + link to it. Pipeline-generated PR bodies are machine-owned — + each call fully replaces the body, clobbering manual edits. + + Returns ``True`` on success, ``False`` on any failure. Unlike + :meth:`create_pr` this method does NOT propagate errors: a body + refresh is cosmetic, and no caller should fail a slice over it. + """ + if ( + not repo + or pr_number is None + or isinstance(pr_number, bool) + or not isinstance(pr_number, int) + or pr_number < 1 + ): + logger.warning( + "update_pr_body: invalid repo/pr_number", + pipeline_id=pipeline_id, + repo=repo, + pr_number=pr_number, + ) + return False + + # The gateway's ``gh pr edit`` rejects an empty payload (#3431 + # in gateway/gateway.py) — short-circuit before burning a + # synthetic-session create+delete round-trip on a guaranteed + # 400. + if not body: + logger.warning( + "update_pr_body: empty body", + pipeline_id=pipeline_id, + repo=repo, + pr_number=pr_number, + ) + return False + + # Suffix the container id with a short random tag so two + # concurrent refreshes for the same pipeline (two slices in + # the same wave finishing within ms of each other) don't share + # a session-table key in the gateway. Matches the + # per-slice-id uniqueness create_pr / rebase_onto already get + # for free. + temp_container_id = f"{pipeline_id}-pr-body-update-{uuid.uuid4().hex[:8]}" + session_token: str | None = None + try: + session = self.register_session( + container_id=temp_container_id, + container_ip=self.self_ip, + mode=mode, + pipeline_id=pipeline_id, + repos=[repo], + issue_number=issue_number, + agent_role=agent_role, + synthetic=True, + ) + session_token = session.session_token + + self._make_request( + "/api/v1/gh/pr/edit", + method="POST", + data={ + "repo": repo, + "pr_number": int(pr_number), + "body": body, + }, + bearer_token=session_token, + ) + logger.info( + "Updated PR body via gateway", + pipeline_id=pipeline_id, + repo=repo, + pr_number=pr_number, + ) + return True + except Exception as exc: # noqa: BLE001 + # Session registration + single gh-pr-edit HTTP call. + # Catches GatewayError and OSError (DNS / socket). + logger.warning( + "update_pr_body: gateway request failed", + pipeline_id=pipeline_id, + repo=repo, + pr_number=pr_number, + error=str(exc), + ) + return False + finally: + if session_token: + try: + self.delete_session(session_token) + except Exception: + pass + def rebase_onto( self, pipeline_id: str, diff --git a/orchestrator/routes/pipelines.py b/orchestrator/routes/pipelines.py index 9250afb127..ce77f10d4d 100644 --- a/orchestrator/routes/pipelines.py +++ b/orchestrator/routes/pipelines.py @@ -307,6 +307,7 @@ def get_repo_checks(repo: str) -> list[dict[str, str]]: # type: ignore[misc] get_state_store, ) +from egg_contracts.markdown import unwrap_soft_breaks from egg_contracts.orchestrator import load_agent_output, save_agent_output from egg_git.default_branch import get_default_branch from lifecycle_auth import require_lifecycle_secret @@ -9474,15 +9475,19 @@ def _compose_context_pr_body( pr = contract.pr sections: list[str] = [] - description = (pr.description or "").strip() if pr else "" + # Soft-break unwrapping (#3122): the ``pr:`` block fields arrive as + # YAML block scalars hard-wrapped at ~75 chars, and GitHub renders + # every newline in a PR body as a line break — join the wraps back + # into paragraphs, leaving real markdown structure alone. + description = unwrap_soft_breaks(pr.description if pr else None).strip() if description: sections.append(description) - test_plan = (pr.test_plan or "").strip() if pr else "" + test_plan = unwrap_soft_breaks(pr.test_plan if pr else None).strip() if test_plan: sections.append(f"## Test Plan\n\n{test_plan}") - manual_steps = (pr.manual_steps or "").strip() if pr else "" + manual_steps = unwrap_soft_breaks(pr.manual_steps if pr else None).strip() if manual_steps: sections.append(f"## Manual Steps\n\n{manual_steps}") @@ -9508,7 +9513,14 @@ def _compose_context_pr_body( # ``_migrate_phases_to_slices`` only rewrites it on JSON # load, so a directly-constructed Slice can still carry it. number = s.id.removeprefix("slice-").removeprefix("phase-") - body_lines.append(f" {number}. {name} (`{s.id}`)") + line = f" {number}. {name} (`{s.id}`)" + # Cross-link the stack (#3122): once the slice's PR is open + # its number is persisted on the contract and the run loop + # re-composes this body, so the entry gains a link. Bare + # ``#N`` autolinks within the repo the context PR lives in. + if getattr(s, "pr_number", None): + line += f" — #{s.pr_number}" + body_lines.append(line) has_meaningful_content = True link_base: str | None = None @@ -9702,6 +9714,99 @@ def _persist_context_pr_number( ) from save_err +def _refresh_context_pr_body( + pipeline_id: str, + *, + pipeline: Any, + spawner: Any, + worktree_repo_path: Path, + identifier: int | str, + gateway_mode: str = "public", +) -> bool: + """Re-compose and push the context PR's body to GitHub (#3122). + + Called by the run loop after a slice PR opens and its number is + persisted on the contract, so the context PR's slice table gains a + link to each slice PR as the stack materialises + (:func:`_compose_context_pr_body` renders ``— #N`` for every slice + with a recorded ``pr_number``). + + The context PR body is machine-owned: the refresh fully regenerates + it from contract + pipeline state through the same composer the + opener used, clobbering any manual edits. Best-effort by design — + a body refresh is cosmetic, so every failure (contract load, + composition, gateway) logs a warning and returns ``False`` without + raising; no slice outcome may depend on it. + + **Concurrency contract**: the caller must hold + ``get_pipeline_state_lock(pipeline_id)`` for the entire load + + compose + push sequence — without it, two slices completing in the + same wave can interleave so the slice whose refresh lands later + clobbers a body that already included both links. Because no + later slice fires a refresh after the last one, the final slice's + ``— #N`` link would stay missing forever if the race fired on it. + Serializing inside the per-pipeline lock eliminates the race; the + sole production caller (``_run_implement_phase_slices``) already + holds it. + """ + if not pipeline.repo: + return False + + try: + from egg_contracts.loader import load_contract + + contract = load_contract(identifier, worktree_repo_path) + except Exception as load_err: # noqa: BLE001 + # Lazy import + contract load: ImportError, loader validation + # errors, OSError on the contract file read. + logger.warning( + "Context PR body refresh: contract load failed (skipping)", + pipeline_id=pipeline_id, + error=str(load_err), + ) + return False + + context_pr_number = ( + contract.pr.context_pr_number if contract.pr else None + ) or pipeline.pr_number + if not context_pr_number: + # No context PR to refresh — reachable on #3100-degraded + # contracts where the opener never persisted linkage. + return False + + try: + body = _compose_context_pr_body( + contract=contract, + pipeline=pipeline, + worktree_repo_path=worktree_repo_path, + identifier=identifier, + ) + except Exception as compose_err: # noqa: BLE001 + # Pure string composition over loaded state; a raise here is a + # programming error, but the cosmetic-refresh contract still + # holds — log and skip rather than fail the slice. + logger.warning( + "Context PR body refresh: composition failed (skipping)", + pipeline_id=pipeline_id, + pr_number=context_pr_number, + error=str(compose_err), + ) + return False + + return spawner.gateway.update_pr_body( + pipeline_id, + pipeline.repo, + pr_number=context_pr_number, + body=body, + issue_number=pipeline.issue_number, + # Attribute the action in the gateway audit log; matches + # sibling orchestrator-driven PR mutations (create_slice_pr, + # rebase_onto). + agent_role="orchestrator", + mode=gateway_mode, + ) + + def _open_context_pr_at_implement_start( pipeline_id: str, repo_path: Path | None = None ) -> int | None: @@ -15927,7 +16032,12 @@ def _contract_loader() -> Any: except ImportError: from state_store import get_pipeline_state_lock # type: ignore[no-redef] - def _persist_slice_status_complete(slice_id: str) -> None: + def _persist_slice_status_complete( + slice_id: str, + *, + pr_number: int | None = None, + pr_url: str | None = None, + ) -> None: """Mark ``slice_id`` as ``SliceStatus.COMPLETE`` on the contract. Durable signal so the bootstrap reconciliation pass below and @@ -15936,6 +16046,23 @@ def _persist_slice_status_complete(slice_id: str) -> None: on save failure the in-memory scheduler state still reflects completion for this pass and the next ``start_pipeline`` re-detects via the merged-detection helper. + + When the caller just opened the slice's PR it passes + ``pr_number`` / ``pr_url`` so the linkage lands in the same + contract write (#3122) — the context-PR body refresh and any + later stack consumer read them from ``Slice.pr_number``. + ``None`` (the merged-skip and bootstrap callers) leaves any + previously recorded linkage untouched. + + TODO(#3122): the three ``None`` callers — bootstrap layer-A + (contract-recorded COMPLETE), bootstrap layer-B (merged on + origin), and the run-loop merged-skip — do not recover the + slice PR number from GitHub (`gh pr list --head … --state + merged`), so on a resume past those points the slice-table + entries for merged slices stay unlinked. Acceptable for v1 + because the per-slice ``— #N`` link is most useful while the + stack is live, but worth backfilling if reviewers ask for + complete cross-linkage on archived stacks. """ try: with get_pipeline_state_lock(pipeline_id): @@ -15943,6 +16070,10 @@ def _persist_slice_status_complete(slice_id: str) -> None: for s in contract_local.slices: if s.id == slice_id: s.status = SliceStatus.COMPLETE + if pr_number is not None: + s.pr_number = pr_number + if pr_url is not None: + s.pr_url = pr_url break save_contract(contract_local, worktree_repo_path) except Exception as save_err: # noqa: BLE001 @@ -16744,6 +16875,8 @@ def _probe_parent_branch_exists(parent_branch: str) -> bool: ) pr_created = True + slice_pr_url: str | None = None + slice_pr_number: int | None = None if slice_pr_data is not None and pipeline.repo: # Best-effort real-diff summary for the PR body # (#3115) — commit subjects + diffstat from the @@ -16758,7 +16891,7 @@ def _probe_parent_branch_exists(parent_branch: str) -> bool: gateway_mode=gateway_mode, # type: ignore[arg-type] ) try: - spawner.gateway.create_slice_pr( + slice_pr_url = spawner.gateway.create_slice_pr( pipeline_id=pipeline_id, repo=pipeline.repo, slice_id=slice_id, @@ -16801,8 +16934,57 @@ def _probe_parent_branch_exists(parent_branch: str) -> bool: f"base={parent_branch})" ) - scheduler.record_complete(slice_id) - _persist_slice_status_complete(slice_id) + # Parse the slice PR number from the returned URL + # (#3122) — same trailing-boundary pattern the context- + # PR opener uses, narrowed to ``[1-9]\d*`` so a + # malformed ``/pull/0/...`` URL doesn't make it as far + # as ``Slice.pr_number``'s ``ge=1`` validator (which + # would silently downgrade to a warning log via the + # save try/except in ``_persist_slice_status_complete``). + # Best-effort: an unparseable URL just means the + # linkage isn't recorded this pass; the idempotent + # ``create_slice_pr`` re-yields it on a resume. + if slice_pr_url: + pr_match = re.search(r"/pull/([1-9]\d*)(?:[/?#]|$)", slice_pr_url) + if pr_match: + slice_pr_number = int(pr_match.group(1)) + + # Hold the per-pipeline state lock across both the + # contract-write (``_persist_slice_status_complete`` + # itself reacquires this RLock) and the context-PR + # body refresh (load + compose + push). Without the + # outer lock, two slices in the same wave could + # interleave between persist and push so the slice + # whose refresh starts earlier but lands later + # clobbers the body that already included both links + # — and because no later slice fires a refresh, the + # final slice's ``— #N`` link would stay missing + # forever. Serializing here bounds the per-slice tail + # latency by one gateway PATCH per concurrent slice + # rather than racing them. + with get_pipeline_state_lock(pipeline_id): + scheduler.record_complete(slice_id) + _persist_slice_status_complete( + slice_id, + pr_number=slice_pr_number, + pr_url=slice_pr_url if slice_pr_number else None, + ) + + # Refresh the context PR body so its slice table + # links the PR that just opened (#3122). Strictly + # cosmetic and best-effort: every failure path + # inside logs + returns False without raising, and + # the slice outcome below never depends on it. + if slice_pr_number: + _refresh_context_pr_body( + pipeline_id, + pipeline=pipeline, + spawner=spawner, + worktree_repo_path=worktree_repo_path, + identifier=_pipeline_identifier(pipeline.issue_number, pipeline_id), + gateway_mode=gateway_mode, + ) + try: remove_peer_consensus_tracker(pipeline_id, slice_id) except Exception: # noqa: BLE001 diff --git a/orchestrator/tests/test_gateway_client.py b/orchestrator/tests/test_gateway_client.py index f6aa25acbd..048c478229 100644 --- a/orchestrator/tests/test_gateway_client.py +++ b/orchestrator/tests/test_gateway_client.py @@ -76,6 +76,8 @@ def do_POST(self): self._handle_git_fetch(data) elif self.path == "/api/v1/gh/pr/create": self._handle_pr_create(data) + elif self.path == "/api/v1/gh/pr/edit": + self._handle_pr_edit(data) elif self.path.startswith("/api/v1/sessions/by-container/") and self.path.endswith( "/heartbeat" ): @@ -301,6 +303,24 @@ def _handle_pr_create(self, data): } ) + def _handle_pr_edit(self, data): + """Handle PR edit (POST /api/v1/gh/pr/edit) — records the payload (#3122).""" + auth_header = self.headers.get("Authorization", "") + if not auth_header.startswith("Bearer ") or not auth_header[7:]: + self._send_error(401, "Unauthorized") + return + + # Expose the last edit payload for assertions. + MockGatewayHandler.last_pr_edit = data + + self._send_json( + { + "success": True, + "message": "PR updated", + "data": {"stdout": "{}"}, + } + ) + def _send_json(self, data, status=200): """Send JSON response.""" self.send_response(status) @@ -1493,6 +1513,83 @@ def test_create_pr_registers_synthetic_session_without_phase( assert call_kwargs.kwargs.get("phase") is None +class TestUpdatePrBody: + """Tests for update_pr_body (#3122) — best-effort context-PR body refresh.""" + + def test_update_pr_body_success(self, gateway_client, mock_gateway_server): + result = gateway_client.update_pr_body( + pipeline_id="issue-42", + repo="owner/repo", + pr_number=4242, + body="## Pipeline context\n\n- Slices (1):\n 1. Foundation (`slice-1`) — #4243", + ) + assert result is True + payload = MockGatewayHandler.last_pr_edit + assert payload["repo"] == "owner/repo" + assert payload["pr_number"] == 4242 + assert "— #4243" in payload["body"] + # Body-only edit: never touches title or base. + assert "title" not in payload + assert "base" not in payload + + def test_update_pr_body_gateway_unreachable_returns_false(self): + """No raise on failure — a body refresh is cosmetic by contract.""" + client = GatewayClient( + gateway_host="localhost", + gateway_port=19999, + launcher_secret="test-secret", + timeout=1, + ) + result = client.update_pr_body( + pipeline_id="issue-42", + repo="owner/repo", + pr_number=4242, + body="body", + ) + assert result is False + + @pytest.mark.parametrize("bad_number", [None, 0, -1, True, "123", 1.0]) + def test_update_pr_body_invalid_pr_number_returns_false(self, gateway_client, bad_number): + """Locks in the contract: any non-int (string, float, bool) or + ``< 1`` int returns False without burning a synthetic-session + round-trip. ``"123"`` would previously have crashed on + ``"123" < 1`` (Python 3 TypeError) — review feedback on + #3128.""" + with patch.object(gateway_client, "_make_request") as mock_req: + result = gateway_client.update_pr_body( + pipeline_id="issue-42", + repo="owner/repo", + pr_number=bad_number, + body="body", + ) + assert result is False + mock_req.assert_not_called() + + def test_update_pr_body_empty_body_short_circuits(self, gateway_client): + """The gateway's ``gh pr edit`` rejects an empty payload — short- + circuit before burning a synthetic-session round-trip (review + feedback on #3128).""" + with patch.object(gateway_client, "_make_request") as mock_req: + result = gateway_client.update_pr_body( + pipeline_id="issue-42", + repo="owner/repo", + pr_number=4242, + body="", + ) + assert result is False + mock_req.assert_not_called() + + def test_update_pr_body_cleans_up_session(self, gateway_client, mock_gateway_server): + with patch.object(gateway_client, "delete_session") as mock_delete: + gateway_client.update_pr_body( + pipeline_id="issue-42", + repo="owner/repo", + pr_number=4242, + body="body", + ) + mock_delete.assert_called_once_with("test-token-12345") + + class TestCreateSlicePR: """Body / title composition for create_slice_pr (#2340, #2538, #2745).""" @@ -1580,6 +1677,59 @@ def test_slice_goal_leads_body_over_program_blurb(self, gateway_client): # The blurb fallback must not ALSO render. assert "The lint added in #2250" not in body + def test_slice_goal_soft_breaks_unwrapped(self, gateway_client): + """#3122: the goal arrives as a YAML block scalar hard-wrapped at + ~75 chars; the lead paragraph joins the wraps so GitHub renders + prose, not a choppy column.""" + captured, ctx = self._capture(gateway_client) + with ctx: + gateway_client.create_slice_pr( + pipeline_id="issue-42", + repo="owner/repo", + slice_id="slice-1", + slice_name="Pattern adoption", + slice_tasks=[{"id": "task-1-1", "description": "do X"}], + head="egg/issue-42/slice-1", + base="egg/issue-42/work", + program_title="Decompose oversize files", + slice_index=1, + slice_count=3, + context_pr_number=99, + slice_goal=( + "Adopt the barrel re-export pattern\nacross the orchestrator so each\n" + "module fits the size cap." + ), + ) + assert captured["body"].startswith( + "Adopt the barrel re-export pattern across the orchestrator " + "so each module fits the size cap." + ) + + def test_inline_program_narrative_soft_breaks_unwrapped(self, gateway_client): + """#3122: the no-base-PR backstop's inlined program description / + test plan / manual steps get the same unwrapping.""" + captured, ctx = self._capture(gateway_client) + with ctx: + gateway_client.create_slice_pr( + pipeline_id="issue-42", + repo="owner/repo", + slice_id="slice-1", + slice_name="Pattern adoption", + slice_tasks=[{"id": "task-1-1", "description": "do X"}], + head="egg/issue-42/slice-1", + base="egg/issue-42/work", + program_title="Decompose oversize files", + program_description="A description wrapped\nby the block scalar.", + program_test_plan="- automated tests\n- manual check of\n the rendered body", + program_manual_steps="Redeploy after\nmerge.", + slice_index=1, + slice_count=3, + ) + body = captured["body"] + assert "A description wrapped by the block scalar." in body + assert "- automated tests\n- manual check of the rendered body" in body + assert "Redeploy after merge." in body + def test_diff_summary_section_renders_commits_and_diffstat(self, gateway_client): """#3115: ``## What's in this PR`` renders caller-supplied commit subjects (capped at 20 with a remainder line) and the diffstat in diff --git a/orchestrator/tests/test_open_context_pr_at_implement_start.py b/orchestrator/tests/test_open_context_pr_at_implement_start.py index df3d7388e4..d085d3f457 100644 --- a/orchestrator/tests/test_open_context_pr_at_implement_start.py +++ b/orchestrator/tests/test_open_context_pr_at_implement_start.py @@ -54,6 +54,7 @@ _compose_context_pr_body, _open_context_pr_at_implement_start, _persist_context_pr_number, + _refresh_context_pr_body, ) # ---------------------------------------------------------------------- @@ -377,6 +378,169 @@ def test_phase_n_slice_id_renders_clean_number(self, tmp_path): assert "1. Foundation (`phase-1`)" in body assert "phase-1. Foundation" not in body + def test_slice_with_pr_number_renders_link(self, tmp_path): + """#3122: once a slice PR opens its number is persisted on the + contract and the slice-table entry gains a ``— #N`` autolink.""" + from egg_contracts.models import Slice + + body = _compose_context_pr_body( + contract=self._contract( + slices=[ + Slice(id="slice-1", name="Foundation", tasks=[], pr_number=4243), + Slice(id="slice-2", name="Rollout", tasks=[]), + ], + ), + pipeline=_make_pipeline(), + worktree_repo_path=tmp_path, + identifier=2777, + ) + assert "1. Foundation (`slice-1`) — #4243" in body + assert "2. Rollout (`slice-2`)" in body + assert "2. Rollout (`slice-2`) — #" not in body + + def test_soft_breaks_unwrapped_in_prose_fields(self, tmp_path): + """#3122: YAML block-scalar hard wraps in description / + test_plan / manual_steps are joined back into paragraphs; + markdown structure (lists) survives.""" + from egg_contracts.models import Contract, IssueInfo, PRMetadata + + contract = Contract( + issue=IssueInfo(number=2777, title="t", url=""), + pipeline_id="issue-2777", + pr=PRMetadata( + title="Add feature X", + description=( + "This paragraph was wrapped\nat an arbitrary column by\n" + "the YAML block scalar.\n\nSecond paragraph stays\nseparate." + ), + test_plan="- run make test\n- check the rendered\n body manually", + manual_steps="Redeploy the orchestrator\nafter merge.", + ), + ) + body = _compose_context_pr_body( + contract=contract, + pipeline=_make_pipeline(), + worktree_repo_path=tmp_path, + identifier=2777, + ) + assert "This paragraph was wrapped at an arbitrary column by the YAML block scalar." in body + assert "Second paragraph stays separate." in body + # List structure preserved; wrapped list-item tail joined. + assert "- run make test\n- check the rendered body manually" in body + assert "Redeploy the orchestrator after merge." in body + + +class TestRefreshContextPrBody: + """#3122: best-effort context-PR body refresh after a slice PR opens.""" + + def _contract(self, *, context_pr_number=4242, slices=()): + from egg_contracts.models import Contract, IssueInfo, PRMetadata + + return Contract( + issue=IssueInfo(number=2777, title="t", url=""), + pipeline_id="issue-2777", + pr=PRMetadata( + title="Add feature X", + description="The narrative.", + context_pr_number=context_pr_number, + ), + slices=list(slices), + ) + + def test_happy_path_pushes_recomposed_body(self, tmp_path, spawner_factory): + from egg_contracts.models import Slice + + spawner = spawner_factory() + spawner.gateway.update_pr_body.return_value = True + contract = self._contract( + slices=[Slice(id="slice-1", name="Foundation", tasks=[], pr_number=4243)] + ) + from egg_contracts import loader + + with patch.object(loader, "load_contract", return_value=contract): + ok = _refresh_context_pr_body( + "issue-2777", + pipeline=_make_pipeline(), + spawner=spawner, + worktree_repo_path=tmp_path, + identifier=2777, + gateway_mode="public", + ) + assert ok is True + kwargs = spawner.gateway.update_pr_body.call_args.kwargs + assert kwargs["pr_number"] == 4242 + assert "1. Foundation (`slice-1`) — #4243" in kwargs["body"] + # The gateway audit log attributes the action to ``orchestrator``, + # matching sibling orchestrator-driven PR mutations + # (create_slice_pr, rebase_onto) — review feedback on #3128. + assert kwargs["agent_role"] == "orchestrator" + + def test_no_context_pr_number_skips(self, tmp_path, spawner_factory): + spawner = spawner_factory() + contract = self._contract(context_pr_number=None) + pipeline = _make_pipeline() + pipeline.pr_number = None + from egg_contracts import loader + + with patch.object(loader, "load_contract", return_value=contract): + ok = _refresh_context_pr_body( + "issue-2777", + pipeline=pipeline, + spawner=spawner, + worktree_repo_path=tmp_path, + identifier=2777, + ) + assert ok is False + spawner.gateway.update_pr_body.assert_not_called() + + def test_pipeline_pr_number_fallback(self, tmp_path, spawner_factory): + """#3100-degraded contracts: linkage missing on the contract but + mirrored on the pipeline — the refresh still targets the PR.""" + spawner = spawner_factory() + spawner.gateway.update_pr_body.return_value = True + contract = self._contract(context_pr_number=None) + pipeline = _make_pipeline() + pipeline.pr_number = 4242 + from egg_contracts import loader + + with patch.object(loader, "load_contract", return_value=contract): + ok = _refresh_context_pr_body( + "issue-2777", + pipeline=pipeline, + spawner=spawner, + worktree_repo_path=tmp_path, + identifier=2777, + ) + assert ok is True + assert spawner.gateway.update_pr_body.call_args.kwargs["pr_number"] == 4242 + + def test_contract_load_failure_returns_false(self, tmp_path, spawner_factory): + spawner = spawner_factory() + from egg_contracts import loader + + with patch.object(loader, "load_contract", side_effect=OSError("disk gone")): + ok = _refresh_context_pr_body( + "issue-2777", + pipeline=_make_pipeline(), + spawner=spawner, + worktree_repo_path=tmp_path, + identifier=2777, + ) + assert ok is False + spawner.gateway.update_pr_body.assert_not_called() + + def test_no_repo_skips(self, tmp_path, spawner_factory): + spawner = spawner_factory() + ok = _refresh_context_pr_body( + "issue-2777", + pipeline=_make_pipeline(repo=""), + spawner=spawner, + worktree_repo_path=tmp_path, + identifier=2777, + ) + assert ok is False + spawner.gateway.update_pr_body.assert_not_called() + class TestOpenContextPRAtImplementStartTypedErrors: """Each closed ``ContextPrCreationReason`` (or a representative diff --git a/orchestrator/tests/test_slice_run_loop_integration.py b/orchestrator/tests/test_slice_run_loop_integration.py index b116ed315a..75203c428b 100644 --- a/orchestrator/tests/test_slice_run_loop_integration.py +++ b/orchestrator/tests/test_slice_run_loop_integration.py @@ -532,6 +532,91 @@ def test_single_root_slice_uses_pipeline_branch_as_parent(self) -> None: assert pr_kwargs["head"] == f"egg/{pipeline.id}/slice-1" assert pr_kwargs["slice_id"] == "slice-1" + def test_slice_pr_linkage_persisted_and_context_pr_body_refreshed(self) -> None: + """#3122: the run loop records the opened slice PR's number/URL on + the contract slice (same write as status=COMPLETE) and then + refreshes the context PR body so its slice table links the PR.""" + from egg_contracts.models import PRMetadata + + pipeline = _make_pipeline() + slice_obj = _make_slice("slice-1", tasks=[_make_task("task-1-1")]) + contract = _make_contract(slices=[slice_obj]) + contract.pr = PRMetadata( + title="Add feature X", + description="The narrative.", + context_pr_number=4242, + ) + load_mock, save_mock = self._make_loader_save_pair(contract) + + with ( + patch("egg_contracts.loader.load_contract", load_mock), + patch("egg_contracts.loader.save_contract", save_mock), + patch("routes.pipelines._start_stacked_pr_reconciler") as mock_start_recon, + patch("routes.pipelines._run_concurrent_phase", return_value=(0, "ok")), + patch("orchestrator.peer_consensus.remove_peer_consensus_tracker"), + ): + mock_start_recon.return_value = (MagicMock(), threading.Event()) + spawner = self._make_spawner() + spawner.gateway.create_slice_pr.return_value = "https://github.com/owner/repo/pull/4243" + spawner.gateway.update_pr_body.return_value = True + exit_code, _logs = _run_implement_phase_slices( + pipeline_id=pipeline.id, + pipeline=pipeline, + spawner=spawner, + repo_volumes={}, + gateway_mode="public", + repos=["owner/repo"], + sandbox_env={}, + store=MagicMock(), + certs_volume=None, + worktree_repo_path=Path("/tmp/x"), + ) + assert exit_code == 0 + # PR linkage persisted on the contract slice (#3122). + assert slice_obj.pr_number == 4243 + assert slice_obj.pr_url == "https://github.com/owner/repo/pull/4243" + # Context PR body refreshed with the slice link. + spawner.gateway.update_pr_body.assert_called_once() + refresh_kwargs = spawner.gateway.update_pr_body.call_args.kwargs + assert refresh_kwargs["pr_number"] == 4242 + assert "1. Slice slice-1 (`slice-1`) — #4243" in refresh_kwargs["body"] + + def test_unparseable_slice_pr_url_skips_linkage_and_refresh(self) -> None: + """#3122: a slice PR URL without ``/pull/`` (e.g. a stub) means + no linkage is recorded and no body refresh fires — the slice + still completes normally.""" + pipeline = _make_pipeline() + slice_obj = _make_slice("slice-1", tasks=[_make_task("task-1-1")]) + contract = _make_contract(slices=[slice_obj]) + load_mock, save_mock = self._make_loader_save_pair(contract) + + with ( + patch("egg_contracts.loader.load_contract", load_mock), + patch("egg_contracts.loader.save_contract", save_mock), + patch("routes.pipelines._start_stacked_pr_reconciler") as mock_start_recon, + patch("routes.pipelines._run_concurrent_phase", return_value=(0, "ok")), + patch("orchestrator.peer_consensus.remove_peer_consensus_tracker"), + ): + mock_start_recon.return_value = (MagicMock(), threading.Event()) + spawner = self._make_spawner() # returns "https://example/pr/1" + exit_code, _logs = _run_implement_phase_slices( + pipeline_id=pipeline.id, + pipeline=pipeline, + spawner=spawner, + repo_volumes={}, + gateway_mode="public", + repos=["owner/repo"], + sandbox_env={}, + store=MagicMock(), + certs_volume=None, + worktree_repo_path=Path("/tmp/x"), + ) + assert exit_code == 0 + assert slice_obj.pr_number is None + assert slice_obj.pr_url is None + spawner.gateway.update_pr_body.assert_not_called() + assert slice_obj.status == SliceStatus.COMPLETE + def test_child_slice_targets_parent_integration_branch(self) -> None: pipeline = _make_pipeline() root = _make_slice("slice-1", tasks=[_make_task("task-1-1")]) diff --git a/shared/egg_contracts/markdown.py b/shared/egg_contracts/markdown.py new file mode 100644 index 0000000000..d2adea5dee --- /dev/null +++ b/shared/egg_contracts/markdown.py @@ -0,0 +1,141 @@ +"""Markdown reflow helpers for pipeline-generated PR bodies (#3122). + +The planner authors ``pr:`` block fields (``description``, ``test_plan``, +``manual_steps``) and per-slice ``goal`` text as YAML ``|`` literal block +scalars, naturally hard-wrapped at ~75 characters. GitHub renders every +newline in a PR/issue body as a visible line break, so wrapped prose +comes out as a choppy column of short lines instead of paragraphs. + +:func:`unwrap_soft_breaks` joins those soft wraps back into paragraphs +while leaving real markdown structure alone. The heuristic is +conservative: when a line *could* be structural, it is never joined. + +**Known limitations of the conservative heuristic.** The unwrapper is +intentionally line-local — it does not look ahead to disambiguate. Two +corners worth knowing about: + +* **Setext heading without a preceding blank line.** ``Heading\n===`` + is recognised as a setext H1 because ``===`` looks structural on its + own line, but ``some prose\nHeading\n===`` joins ``Heading`` onto + ``some prose`` (both look like prose), leaving ``===`` as an orphan + thematic break. Planners writing setext headings should always + precede them with a blank line — or just use ATX (``# Heading``), + which is unambiguous and always preserved. +* **HTML block opens are carved out**, but only on their *opening* + tag. A line beginning with ``
`` / ``
`` / etc. is + treated as block-structural and never joined; the matching close + tag is also opaque (joining onto a non-prose previous line is + blocked by ``_is_prose``). Inline HTML mid-paragraph is unaffected. +""" + +from __future__ import annotations + +import re + +# A line that *starts* a markdown block element must never be appended +# to the previous line (joining would swallow the element), and must +# never have the following line appended to it (the element ends at the +# newline). Covers: ATX headings, bullet / ordered list items, +# blockquotes, table rows, fences, thematic breaks and setext-heading +# underlines (``---`` / ``===``), footnote/link-reference definitions +# (``[label]: ...``), and HTML block opens (``
``, ``
``, +# etc. — GitHub renders raw HTML in PR bodies, and joining ``
`` +# onto a prose tail would corrupt the block). +_BLOCK_MARKER = re.compile( + r"""^\s{0,3}( + \#{1,6}(\s|$) # ATX heading + | [-*+]\s # bullet list item + | \d{1,9}[.)]\s # ordered list item + | > # blockquote + | \| # table row + | (`{3,}|~{3,}) # code fence + | (-\s*){3,}$ # thematic break / setext h2 underline + | (\*\s*){3,}$ # thematic break (asterisks) + | (_\s*){3,}$ # thematic break (underscores) + | =+\s*$ # setext h1 underline + | \[[^\]]+\]: # link-reference definition + | bool: + """True when ``line`` is plain paragraph text (joinable).""" + if not line.strip(): + return False + if _BLOCK_MARKER.match(line): + return False + if _INDENTED_CODE.match(line): + return False + return True + + +def unwrap_soft_breaks(text: str | None) -> str: + """Join hard-wrapped prose lines back into paragraphs. + + Single newlines between two plain-prose lines are replaced with a + space; everything else — blank-line paragraph breaks, list items, + headings, blockquotes, tables, fenced and indented code, thematic + breaks, and explicit hard breaks (trailing double-space or + backslash) — is preserved verbatim. + + A prose line is also joined *into* a preceding list-item or + blockquote line (lazy continuation): the wrapped tail of a long + bullet belongs to the bullet. It is never joined into a heading, + table row, fence, or thematic break, where the block ends at the + newline. + + Idempotent: running the function over its own output is a no-op. + Returns ``""`` for ``None`` / empty input. + """ + if not text: + return "" + + out: list[str] = [] + in_fence = False + fence_marker = "" + + for line in text.splitlines(): + fence_match = _FENCE.match(line) + if in_fence: + out.append(line) + # The closing fence must use the same character as the + # opener (``` vs ~~~) and be at least as long. + if ( + fence_match + and fence_match.group(1)[0] == fence_marker[0] + and len(fence_match.group(1)) >= len(fence_marker) + ): + in_fence = False + continue + if fence_match: + in_fence = True + fence_marker = fence_match.group(1) + out.append(line) + continue + + if out and _is_prose(line): + prev = out[-1] + prev_stripped = prev.strip() + # Join onto plain prose, list items, and blockquote lines + # (markdown lazy continuation) — never onto blanks, + # headings, tables, fences, or thematic breaks, and never + # past an explicit hard break. + prev_is_joinable = bool(prev_stripped) and ( + _is_prose(prev) or re.match(r"^\s{0,3}([-*+]\s|\d{1,9}[.)]\s|>)", prev) + ) + hard_break = prev.endswith(" ") or prev_stripped.endswith("\\") + if prev_is_joinable and not hard_break: + out[-1] = f"{prev.rstrip()} {line.strip()}" + continue + + out.append(line) + + return "\n".join(out) diff --git a/shared/egg_contracts/models.py b/shared/egg_contracts/models.py index b630920265..986d46297e 100644 --- a/shared/egg_contracts/models.py +++ b/shared/egg_contracts/models.py @@ -412,6 +412,26 @@ class Slice(EggContractBaseModel): pattern=r"^[a-f0-9]{7,40}$", description="Git commit SHA linked to this slice", ) + pr_number: int | None = Field( + default=None, + ge=1, + description=( + "GitHub PR number of this slice's stacked PR (#3122). " + "Recorded by the run loop when ``create_slice_pr`` returns " + "(including the idempotent already-open hit), so the context " + "PR body can be regenerated with a link to each slice PR as " + "it opens. ``None`` until the slice PR exists, and for " + "contracts written before the field existed." + ), + ) + pr_url: str | None = Field( + default=None, + description=( + "Canonical URL of this slice's PR (#3122). Written together " + "with ``pr_number``; kept separately so consumers don't have " + "to re-derive the repo to build a link." + ), + ) review_feedback: list[ReviewFeedback] = Field( default_factory=list, description="Feedback from reviewer" ) diff --git a/tests/shared/egg_contracts/test_markdown.py b/tests/shared/egg_contracts/test_markdown.py new file mode 100644 index 0000000000..df1fde6b8f --- /dev/null +++ b/tests/shared/egg_contracts/test_markdown.py @@ -0,0 +1,120 @@ +"""Tests for the soft-break unwrapper (#3122).""" + +from egg_contracts.markdown import unwrap_soft_breaks + + +class TestUnwrapSoftBreaks: + def test_empty_and_none(self): + assert unwrap_soft_breaks(None) == "" + assert unwrap_soft_breaks("") == "" + + def test_joins_wrapped_paragraph(self): + text = "This sentence was wrapped by the\nYAML block scalar at an\narbitrary column." + assert unwrap_soft_breaks(text) == ( + "This sentence was wrapped by the YAML block scalar at an arbitrary column." + ) + + def test_preserves_paragraph_breaks(self): + text = "First paragraph line one\nline two.\n\nSecond paragraph line one\nline two." + assert unwrap_soft_breaks(text) == ( + "First paragraph line one line two.\n\nSecond paragraph line one line two." + ) + + def test_preserves_bullet_list_items(self): + text = "- first item\n- second item\n- third item" + assert unwrap_soft_breaks(text) == text + + def test_joins_wrapped_bullet_continuation(self): + text = "- a long bullet that was\n wrapped onto a second line\n- next bullet" + assert unwrap_soft_breaks(text) == ( + "- a long bullet that was wrapped onto a second line\n- next bullet" + ) + + def test_preserves_ordered_list_items(self): + text = "1. first\n2. second\n10. tenth" + assert unwrap_soft_breaks(text) == text + + def test_preserves_headings(self): + text = "## Heading\nprose right after the heading\nwrapped once." + assert unwrap_soft_breaks(text) == ( + "## Heading\nprose right after the heading wrapped once." + ) + + def test_does_not_join_heading_into_prose(self): + text = "some prose\n## Heading" + assert unwrap_soft_breaks(text) == text + + def test_preserves_fenced_code(self): + text = "Run this:\n\n```bash\nmake test\nmake lint\n```\n\nthen push\nand wait." + assert unwrap_soft_breaks(text) == ( + "Run this:\n\n```bash\nmake test\nmake lint\n```\n\nthen push and wait." + ) + + def test_fence_close_requires_matching_char(self): + text = "```\n~~~\nstill code\n```\nprose after\nwrapped." + assert unwrap_soft_breaks(text) == "```\n~~~\nstill code\n```\nprose after wrapped." + + def test_preserves_indented_code(self): + text = "Example:\n\n indented code line one\n indented code line two" + assert unwrap_soft_breaks(text) == text + + def test_preserves_table(self): + text = "| a | b |\n|---|---|\n| 1 | 2 |" + assert unwrap_soft_breaks(text) == text + + def test_preserves_blockquote_markers(self): + text = "> quoted line one\n> quoted line two" + assert unwrap_soft_breaks(text) == text + + def test_joins_blockquote_lazy_continuation(self): + text = "> a quote that was\nwrapped lazily" + assert unwrap_soft_breaks(text) == "> a quote that was wrapped lazily" + + def test_preserves_hard_break_trailing_spaces(self): + text = "line with explicit break \nnext line" + assert unwrap_soft_breaks(text) == text + + def test_preserves_hard_break_backslash(self): + text = "line with explicit break\\\nnext line" + assert unwrap_soft_breaks(text) == text + + def test_preserves_thematic_break(self): + text = "prose above\n---\nprose below" + assert unwrap_soft_breaks(text) == text + + def test_preserves_setext_underline(self): + text = "Heading text\n===\nprose below" + assert unwrap_soft_breaks(text) == text + + def test_preserves_link_reference_definition(self): + text = "see the docs\n[docs]: https://example.com" + assert unwrap_soft_breaks(text) == text + + def test_preserves_html_block_open(self): + """#3122 review: GitHub renders raw HTML in PR bodies, so a + ``
`` (or any HTML block open) must not be folded into + the previous prose line — joining would corrupt the block.""" + text = "prose above\n
\nhidden\nbody\n
" + assert unwrap_soft_breaks(text) == text + + def test_setext_heading_without_blank_line_known_limitation(self): + """#3122 review: the conservative line-local heuristic cannot + disambiguate a setext heading from prose when no blank line + precedes it — ``Heading`` looks like prose and is joined onto + ``Some prose``, leaving ``===`` as an orphan thematic break. + Planners should always blank-line-separate setext headings or + use ATX (``# Heading``), which is unambiguous and preserved.""" + text = "Some prose\nHeading text\n===\nprose below" + # Known limitation: the setext underline is *preserved* as a + # standalone ``===`` line (which GitHub renders as a thematic + # break), but the heading text itself is joined into the prose + # above it. We assert the current behaviour to lock it in. + assert unwrap_soft_breaks(text) == "Some prose Heading text\n===\nprose below" + + def test_idempotent(self): + text = ( + "A wrapped\nparagraph here.\n\n- bullet one\n wrapped\n- bullet two\n\n" + "```\ncode\n```\n\n| a |\n| - |\n" + ) + once = unwrap_soft_breaks(text) + assert unwrap_soft_breaks(once) == once