refactor(orchestrator): remove orphaned PR-phase helpers (#2913)

docs/guides/concurrent-execution.md

@@ -284,8 +284,9 @@ egg-orch message send --to coder --type HANDOFF \
   --subject "CI needs a step for the new regression suite" \
   --body "tests/test_auth_regression.py won't run in CI. The workflow needs a
 step invoking it. Please stage the end-state under .github-staging/workflows/
-ci.yml (add a 'pytest tests/test_auth_regression.py' step); the PR builder
-emits a manual move-into-.github step for the human reviewer."
+ci.yml (add a 'pytest tests/test_auth_regression.py' step) and call the
+staged file out in your PR body so the human reviewer moves it into
+.github/ before merge."
 
 # 2. Coder receives the HANDOFF on its next poll cycle:
 egg-orch message poll --wait 30

docs/reference/agent-roles.md

@@ -205,8 +205,9 @@ worktree branch.
   config — branch-protection invariant). To propose `.github/` changes,
   write the end-state to `.github-staging/` mirroring the `.github/`
   structure (e.g. stage `.github/workflows/ci.yml` as
-  `.github-staging/workflows/ci.yml`); the PR builder auto-emits a
-  manual step asking the human reviewer to move the files before merge
+  `.github-staging/workflows/ci.yml`); the agent must call the staged
+  files out in its PR body so the human reviewer moves them into
+  `.github/` before merge
   ([#2508](https://github.com/jwbron/egg/issues/2508)).
   `sandbox/scripts/` is **writable** — the gateway is the sole egress
   chokepoint, so credential-shim modifications are reviewed by

gateway/gateway.py

@@ -3746,10 +3746,10 @@ def gh_pr_create() -> tuple[Response, int] | Response:
     if policy_result.details and policy_result.details.get("force_draft"):
         draft = True
 
-    # Inject machine-parseable pipeline metadata as an HTML comment.
-    # Note: _build_pr_body (in the orchestrator) also adds a human-readable
-    # "## Pipeline Context" section. The two formats are intentionally
-    # complementary — visible for humans, hidden comment for tooling.
+    # Inject machine-parseable pipeline metadata as an HTML comment so
+    # downstream tooling (status reporters, audit scrapers) can recover
+    # the pipeline_id / agent_role / issue from the PR body without
+    # round-tripping through the orchestrator state store.
     session = getattr(g, "session", None)
     session_pipeline_id = getattr(session, "pipeline_id", None) if session else None
     if session_pipeline_id:

gateway/tests/test_agent_restrictions_patterns.py

@@ -261,9 +261,9 @@ def test_can_write_github_staging_dir(self, pattern):
 
         The `.github/` blocked prefix is matched via ``startswith(".github/")``,
         which doesn't match `.github-staging/...`, so the catch-all
-        ``**`` allowlist reaches it. The PR builder turns staged files
-        into a manual step asking the human reviewer to move them
-        into `.github/` before merge.
+        ``**`` allowlist reaches it. The agent calls the staged files
+        out in its PR body so the human reviewer moves them into
+        `.github/` before merge.
         """
         assert pattern.can_write(".github-staging/workflows/ci.yml") is True
         assert pattern.can_write(".github-staging/CODEOWNERS") is True

orchestrator/routes/pipelines.py

@@ -1835,12 +1835,11 @@ def create_pipeline() -> tuple[Response, int]:
         #
         # This is the *primary* eviction site for auto-FAILED prior runs,
         # not just a defensive backstop: paths like restart_agent spawn
-        # failure and _handle_pr_creation_failure call
-        # store.update_pipeline / store.save_pipeline directly (bypassing
-        # PATCH), so the PATCH-site clear never fires for them. Without
-        # this POST-site clear, those auto-FAILED pipelines would leak
-        # consensus + message-store state into the next run that reuses
-        # the id.
+        # failure call store.update_pipeline / store.save_pipeline directly
+        # (bypassing PATCH), so the PATCH-site clear never fires for them.
+        # Without this POST-site clear, those auto-FAILED pipelines would
+        # leak consensus + message-store state into the next run that
+        # reuses the id.
         _clear_pipeline_runtime_state(pipeline.id, reason="pipeline_create")
 
         logger.info(
@@ -6187,15 +6186,14 @@ def _build_role_restrictions_section(repo: str | None = None) -> str:
         "end-state to top-level `.github-staging/`, mirroring the "
         "`.github/` structure (e.g. a proposed change to "
         "`.github/workflows/ci.yml` is staged at "
-        "`.github-staging/workflows/ci.yml`). The orchestrator's PR "
-        "builder auto-detects `.github-staging/` and emits a manual "
-        "step asking the human reviewer to move the staged files into "
-        "place before merge. Assign such tasks to `role: coder` and "
-        "make the staging path explicit in the task's "
-        "`files_affected`. `.github-staging/` must remain tracked by "
-        "git (do not add it to `.gitignore`); otherwise the staged "
-        "files won't be in the PR commit and the reviewer's `git mv` "
-        "will fail."
+        "`.github-staging/workflows/ci.yml`). The producing agent must "
+        "call the staged files out in the PR body so the human "
+        "reviewer moves them into `.github/` before merge. Assign such "
+        "tasks to `role: coder` and make the staging path explicit in "
+        "the task's `files_affected`. `.github-staging/` must remain "
+        "tracked by git (do not add it to `.gitignore`); otherwise the "
+        "staged files won't be in the PR commit and the reviewer's "
+        "`git mv` will fail."
     )
     lines.append("")
 
@@ -8730,66 +8728,6 @@ def _fetch_pr_state(pr_number: int, repo: str | None = None) -> dict[str, Any]:
     }
 
 
-def _handle_pr_creation_failure(
-    pipeline_id: str,
-    current_phase: str,
-    store,
-    reason: str | None = None,
-) -> None:
-    """Mark a pipeline as FAILED after PR creation returns no URL.
-
-    Extracted from ``_health_monitor_poll`` so this state-transition logic can
-    be tested independently of the full polling loop.
-
-    The error message attached to the pipeline tells the user exactly what
-    happened and how to rescue the work.  The agents' commits are on
-    ``origin/<pipeline.branch>`` regardless of the failure mode, so the
-    rescue is always "open the PR manually against that branch" — we
-    surface the exact ``gh pr create`` invocation to avoid forcing users
-    to dig through orchestrator logs (see #1731).
-
-    ``reason`` is a short phrase explaining *why* PR creation failed (e.g.
-    ``"fetch+rebase reconcile failed"``).  When omitted, the generic
-    ``"no PR URL returned"`` message is used for back-compat.
-    """
-    reason_text = reason or "no PR URL returned"
-    error_msg = f"Auto PR creation failed: {reason_text}"
-    logger.error(error_msg, pipeline_id=pipeline_id, reason=reason_text)
-    with get_pipeline_state_lock(pipeline_id):
-        pipeline = store.load_pipeline(pipeline_id)
-        phase_execution = pipeline.get_phase_execution(current_phase)
-        # Compose a user-facing message that includes the rescue hint,
-        # using pipeline state we only have access to inside the lock.
-        rescue_hint = _format_rescue_hint(pipeline)
-        full_error = f"{error_msg}\n{rescue_hint}" if rescue_hint else error_msg
-        phase_execution.status = PipelineStatus.FAILED
-        phase_execution.error = full_error
-        phase_execution.completed_at = datetime.now(UTC)
-        pipeline.status = PipelineStatus.FAILED
-        pipeline.error = full_error
-        store.save_pipeline(pipeline)
-
-
-def _format_rescue_hint(pipeline) -> str:
-    """Build a user-facing rescue hint for a pipeline whose PR couldn't be auto-created.
-
-    Returns an empty string when we don't have enough state to compose a
-    useful hint (no repo or no branch on the pipeline) — in that case the
-    error log + pipeline ID are the user's only handholds.
-    """
-    repo = getattr(pipeline, "repo", None)
-    branch = getattr(pipeline, "branch", None)
-    if not repo or not branch:
-        return ""
-    base = getattr(pipeline, "base_branch", None) or "main"
-    return (
-        f"Agent work is on origin/{branch} in {repo}. "
-        f"To open the PR manually:\n"
-        f"  gh pr create --repo '{repo}' --head '{branch}' --base '{base}' "
-        f'--title "..." --body "..."'
-    )
-
-
 BRC_HISTORY_TYPES = frozenset(
     {
         "CONSENSUS_PROPOSE",
@@ -9586,189 +9524,6 @@ def _sort_key(name: str) -> tuple[int, int, str]:
     return f"_Per-phase BRC transcripts: {links}._"
 
 
-def _pr_metadata_from_plan_draft(
-    worktree_repo_path: Path,
-    issue_number: int | None,
-    pipeline_id: str,
-) -> tuple[str | None, str, str, str, list[str], str | None]:
-    """Parse PR metadata from the plan draft on disk.
-
-    Used as a fallback in ``_build_pr_body`` when ``contract.pr`` is not
-    populated — e.g. when the plan-phase contract write did not reach the
-    branch tip (see #1829). The plan draft itself is reliably on the
-    branch even when the contract is not.
-
-    Returns ``(title, description, test_plan, manual_steps, warnings,
-    draft_rel_path)``. ``title`` is ``None`` when the draft is missing,
-    unparseable, or has no ``pr:`` block, signalling the caller to fall
-    through to the next tier. ``warnings`` is a list of
-    human-readable parse warning strings collected from ``parse_plan``
-    (empty when the parse was clean or the draft was absent); it is
-    surfaced in the PR body when the caller falls through to the stub
-    tier so reviewers can see what went wrong (see #1975).
-    ``draft_rel_path`` is the relative path to the draft that was
-    parsed, or ``None`` if no draft was attempted.
-    """
-    warnings_out: list[str] = []
-    draft_rel = _get_draft_path("plan", issue_number=issue_number, pipeline_id=pipeline_id)
-    if not draft_rel:
-        return None, "", "", "", warnings_out, None
-    plan_path = worktree_repo_path / draft_rel
-    if not plan_path.exists():
-        warnings_out.append(f"Plan draft not found at {draft_rel}")
-        return None, "", "", "", warnings_out, draft_rel
-    try:
-        from egg_contracts.plan_parser import parse_plan
-
-        result = parse_plan(plan_path.read_text())
-    except Exception as e:
-        logger.debug(
-            "Could not parse plan draft for PR metadata fallback",
-            path=str(plan_path),
-            error=str(e),
-        )
-        warnings_out.append(f"parse_plan raised: {e}")
-        return None, "", "", "", warnings_out, draft_rel
-    for w in result.warnings:
-        msg = w.message
-        if w.context:
-            msg = f"{msg} ({w.context})"
-        warnings_out.append(msg)
-    if not result.pr_title:
-        return None, "", "", "", warnings_out, draft_rel
-    return (
-        result.pr_title,
-        result.pr_description or "",
-        result.pr_test_plan or "",
-        result.pr_manual_steps or "",
-        warnings_out,
-        draft_rel,
-    )
-
-
-def _build_github_staging_manual_step(worktree_repo_path: Path) -> str:
-    """Render the auto manual-step for `.github-staging/` files (issue #2508).
-
-    Producer agents (coder, etc.) cannot push to `.github/` because the
-    gateway blocks the path as a branch-protection invariant.  When a
-    plan calls for CI workflow or CODEOWNERS changes, the agent instead
-    writes the proposed end-state to top-level `.github-staging/`,
-    mirroring the `.github/` structure.  This helper scans that
-    directory and returns a markdown step the human reviewer must
-    complete before merge: review the staged files, move them into
-    `.github/`, delete the staging dir, and push the resulting commit.
-
-    Returns an empty string when `.github-staging/` is absent or empty.
-    """
-    staging_dir = worktree_repo_path / ".github-staging"
-    # Drop the whole step when ``.github-staging`` itself is a symlink:
-    # ``Path.is_dir()`` follows symlinks, so without this guard a
-    # ``.github-staging -> /etc`` (or any other host path) would let
-    # ``rglob`` enumerate the link target's files into the manual-step
-    # file list, polluting the PR body with arbitrary host-filesystem
-    # paths. Mirrors the per-entry symlink guard below.
-    if staging_dir.is_symlink():
-        return ""
-    if not staging_dir.is_dir():
-        return ""
-
-    staged_paths: list[str] = []
-    for path in sorted(staging_dir.rglob("*")):
-        # Skip symlinks: ``Path.is_file()`` follows them, so without this
-        # guard a staged ``.github-staging/evil.yml`` → ``/etc/passwd``
-        # would be surfaced in the manual-step file list, the reviewer's
-        # ``git mv`` would preserve it, and ``.github/evil.yml`` would
-        # land in the repo as a symlink. The reviewer's only mitigation
-        # would be the diff (where a symlink shows as a small mode
-        # change that's easy to skim past). Drop staged symlinks here so
-        # the helper is the choke point.
-        if path.is_symlink():
-            continue
-        if not path.is_file():
-            continue
-        try:
-            rel = path.relative_to(worktree_repo_path).as_posix()
-        except ValueError:
-            continue
-        staged_paths.append(rel)
-
-    if not staged_paths:
-        return ""
-
-    # Compute concrete move commands per staged file, choosing
-    # ``git mv`` vs ``git rm`` + ``git mv`` based on whether the target
-    # ``.github/<rest>`` already exists.  ``git mv`` refuses to
-    # overwrite an existing destination, so a template that always
-    # emits the plain form breaks for replacement scenarios (e.g.
-    # restaging an existing workflow).
-    staging_prefix = ".github-staging/"
-    target_prefix = ".github/"
-    mkdir_dirs: list[str] = []
-    move_cmds: list[str] = []
-    for rel in staged_paths:
-        if not rel.startswith(staging_prefix):
-            continue
-        rest = rel[len(staging_prefix) :]
-        target_rel = f"{target_prefix}{rest}"
-        target_dir = target_rel.rsplit("/", 1)[0] if "/" in rest else target_prefix.rstrip("/")
-        if target_dir and target_dir not in mkdir_dirs:
-            mkdir_dirs.append(target_dir)
-        target_abs = worktree_repo_path / target_rel
-        # ``Path.exists()`` follows symlinks and returns False for a
-        # broken link, so an existing-but-broken symlink would slip
-        # through the existence check and ``git mv`` would still refuse
-        # to overwrite it. ``Path.is_symlink()`` returns True regardless
-        # of whether the target resolves, so the disjunction catches
-        # regular files, valid symlinks, and broken symlinks.
-        if target_abs.is_symlink() or target_abs.exists():
-            move_cmds.append(f"git rm {target_rel}  # target exists; remove before mv")
-        move_cmds.append(f"git mv {rel} {target_rel}")
-
-    lines = [
-        "### Move staged `.github/` changes (auto-generated, issue #2508)",
-        "",
-        "This PR includes proposed `.github/` changes under `.github-staging/`. "
-        "Agent roles cannot push to `.github/` directly (CI workflow / CODEOWNERS "
-        "branch-protection invariant), so the agent staged the proposed "
-        "end-state for human review.",
-        "",
-        "Staged files:",
-    ]
-    for rel in staged_paths:
-        lines.append(f"- `{rel}`")
-    lines.extend(
-        [
-            "",
-            "Before merging:",
-            "",
-            "1. Review each staged file for correctness — these are proposed "
-            "CI / repo-config changes that bypass the agent's normal sandbox.",
-            "2. Run the following to move each staged file into `.github/` "
-            "(commands below are pre-computed for this PR; replacement targets "
-            "are handled via `git rm` + `git mv` since `git mv` refuses to "
-            "overwrite an existing destination):",
-            "   ```",
-        ]
-    )
-    for d in mkdir_dirs:
-        lines.append(f"   mkdir -p {d}")
-    for cmd in move_cmds:
-        lines.append(f"   {cmd}")
-    lines.extend(
-        [
-            "   ```",
-            "   After the moves, `.github-staging/` is no longer tracked "
-            "by git (git doesn't track empty directories). Run "
-            "`rm -rf .github-staging` locally if you want to clear any "
-            "leftover empty subdirectories from your worktree.",
-            "3. Commit the move and push from a context with the GitHub "
-            "`workflow` scope (a normal user push works; the bot token may "
-            "not — see issue #2508 layer 2).",
-        ]
-    )
-    return "\n".join(lines)
-
-
 def _derive_producer_roles_with_tasks(
     pipeline_id: str,
     slice_id: str | None,
@@ -13527,9 +13282,10 @@ def _build_file_boundary_section(role_value: str, repo: str | None = None) -> st
             "end-state to top-level `.github-staging/` instead, mirroring "
             "the `.github/` structure (e.g. stage "
             "`.github/workflows/test-e2e.yml` as "
-            "`.github-staging/workflows/test-e2e.yml`). The PR builder "
-            "auto-emits a manual step asking the human reviewer to move "
-            "the staged files into place before merge — see issue #2508."
+            "`.github-staging/workflows/test-e2e.yml`). Call out the "
+            "staged files explicitly in your PR body so the human reviewer "
+            "knows to move them into `.github/` before merge — see issue "
+            "#2508."
         )
     lines.append("")
     return "\n".join(lines)

shared/egg_contracts/agent_roles.py

@@ -214,8 +214,8 @@ def depends_on(self, other: AgentRole) -> bool:
             ".egg-state/agent-outputs/",  # For handoff data
             # Issue #2508: staging dir for proposed `.github/` changes.
             # `.github/` itself is blocked below; the agent stages the
-            # proposed end-state here and the PR builder emits a manual
-            # step asking the human reviewer to move the files into
+            # proposed end-state here and calls the staged files out
+            # in its PR body so the human reviewer moves them into
             # `.github/` before merge.
             ".github-staging/",
         ],

shared/egg_contracts/plan_parser.py

@@ -1681,8 +1681,8 @@ def _check_role_files(task: Task, slice_id: str, repo: str | None = None) -> str
             "No producer role can push every file in this task. Either "
             "split the task so each subtask falls within a single "
             "role's scope, or — for `.github/` files — stage them "
-            "under top-level `.github-staging/` and let the PR "
-            "builder emit a manual reviewer step (issue #2508)."
+            "under top-level `.github-staging/` and call them out in "
+            "the PR body for the human reviewer (issue #2508)."
         )
     return (
         f"Task '{task.id}' (slice '{slice_id}') is assigned role "