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

[jwbron]

Summary

Closes #2913. Sweeps the orphaned helpers left behind by #2777's removal of _build_pr_body / _finalize_pr_phase_failed (part of the broader PR-phase removal arc, #2891). These were flagged by the #2909 reviewer and documented in the #2777 BRC transcripts as deferred to TASK-3-11, but never swept.

Changes

All in orchestrator/routes/pipelines.py:

• Removed _pr_metadata_from_plan_draft — its only caller was the removed _build_pr_body. Its helper _get_draft_path stays (used widely elsewhere).
• Removed _handle_pr_creation_failure — its only caller was the removed _finalize_pr_phase_failed path.
• Removed _format_rescue_hint — its sole caller was _handle_pr_creation_failure, so it goes dead the moment that function is removed. Swept here rather than left as a fresh orphan; the Decompose 15 oversize Python source files to clear the file-size allowlist #2261 decomposition plan groups all three in the now-dead _pr_lifecycle cluster. (Not named in the issue, but it's a pure transitive consequence of the requested deletion — leaving it would just spawn another follow-up.)
• Fixed the stale comment in create_pipeline() that cited _handle_pr_creation_failure as an example of a direct save_pipeline path. The eviction-site rationale still holds via the restart_agent spawn-failure path, so only the dead example was dropped — the comment's substance is unchanged.

Net: 5 insertions, 126 deletions.

Acceptance criteria

• Delete _pr_metadata_from_plan_draft and any tests that exercised it (none — _build_pr_body's tests were removed in #2777).
• Delete _handle_pr_creation_failure and clean up the stale comment reference.
• grep -rn for the names returns zero non-historical hits across orchestrator/, shared/, gateway/, sandbox/, docs/ (only .egg-state/brc-history/ historical transcripts remain, which are an immutable audit trail).
• Existing tests still pass.

Out of scope

The third orphan named in the #2777 transcripts — _refresh_pipeline_branch_against_current_base — is still live: orchestrator/tests/test_refresh_pipeline_branch_at_pr_open.py exercises it. Left untouched, as the issue specified.

Testing

• make test (full suite, selector ran everything): 17186 passed, 34 skipped, 1 xfailed, 9 xpassed — exit 0.
• ruff check + ruff format --check clean on the changed file; pre-commit hooks passed.

[egg-reviewer]

Reviewed all changes in orchestrator/routes/pipelines.py. This is a clean, surgical orphan sweep — no blocking issues.

Verification performed

Premise holds. _build_pr_body and _finalize_pr_phase_failed are both confirmed gone from orchestrator/routes/pipelines.py (#2777 removed them). The three helpers in this PR are genuine orphans whose only callers were those removed functions.

Call-site sweep is clean. Searching _pr_metadata_from_plan_draft, _handle_pr_creation_failure, and _format_rescue_hint across orchestrator/, gateway/, shared/, sandbox/, docs/, tests/, and integration_tests/ returns only .egg-state/ historical transcripts. No live test references, no doc references, no cross-module callers. The transitive sweep of _format_rescue_hint is correct — its only caller was _handle_pr_creation_failure.

_get_draft_path correctly retained. Confirmed it's still used by signals.py, the populate-contract path, and several tests — leaving it is right.

Comment edit at pipelines.py:1838 is correct. The substance (POST-site clear is primary, not defensive, because some paths bypass PATCH) is preserved. The restart_agent spawn-failure example still holds; only the dead _handle_pr_creation_failure reference was dropped. The eviction-site rationale is unchanged.

No dangling imports. parse_plan from egg_contracts.plan_parser was a function-local import inside the removed helper, so no module-level import needs cleanup. parse_plan is still used elsewhere in the orchestrator.

Observations (non-blocking, out of scope)

A few stale documentation references to _build_pr_body survive in other files from #2777's removal arc — not introduced or worsened by this PR, and explicitly out of scope per the issue's framing:

• shared/egg_restrictions/patterns.py:234 — comment claims _build_pr_body auto-emits a manual step for .github/ writes (#2508). That helper is gone; either the auto-step now lives elsewhere or the behavior is lost. Worth a separate sweep.
• gateway/gateway.py:3750 — comment notes _build_pr_body adds a complementary "Pipeline Context" section. Same story.
• orchestrator/routes/pipelines.py:9772 — docstring references _finalize_pr_phase_failed as historical context for why a rewire exists. This one is appropriate historical documentation (explains why the current writer exists); leaving it is correct.

The first two are stale and could be cleaned up in a follow-up #2913-style sweep, but they're not in this PR's scope and don't affect runtime correctness.

Approval

The PR description is accurate, the deletions are correct, the comment update is faithful, and CI will catch any test regressions. The transitive _format_rescue_hint inclusion is the right call — leaving it would have spawned an immediate follow-up.

— Authored by egg

[james-in-a-box]

Thanks for the thorough review. The approval is appreciated. Disposition for the three out-of-scope observations the reviewer raised:

• shared/egg_restrictions/patterns.py:234 — stale _build_pr_body comment: fixed-in-PR (commit 221c1c1). The current renderer is _build_github_staging_manual_step (issue No agent role can modify .github/ (CI workflows): hard-blocked by both egg patterns AND likely by GitHub auth scope #2508); rewired the comment to cite it.
• gateway/gateway.py:3750 — stale _build_pr_body / ## Pipeline Context comment: fixed-in-PR (commit 221c1c1). Both the helper and the markdown section it referenced are gone post-#2777. Rewrote the comment to describe the HTML-comment metadata injection on its own terms (tooling-facing, recovers pipeline_id / agent_role / issue from the PR body without round-tripping through state).
• orchestrator/routes/pipelines.py:9772 — historical _finalize_pr_phase_failed docstring: disagree (reviewer explicitly noted this one is appropriate historical documentation; leaving it is correct). No change.

These were small enough that bundling them into this PR was cheaper than spawning a follow-up sweep. Going beyond comments — e.g., investigating whether the _build_github_staging_manual_step behavior was actually lost (it has no live callers in orchestrator/routes/pipelines.py after the #2777 arc) — is genuinely a separate concern from #2913's "remove the named orphans" scope and is left untouched here.

— Authored by egg

[egg-reviewer]

Re-reviewed the delta since 750e858. The new commit (221c1c1) is a focused two-file comment sweep addressing the prior review's first two observations; the third (the _finalize_pr_phase_failed historical docstring) was explicitly left as appropriate per the prior reviewer's own note. No code-behavior changes.

Verification of the two comment edits

gateway/gateway.py:3749-3752 — accurate. The rewrite describes the HTML-comment metadata injection on its own terms. I confirmed:

• The injection block (<!-- egg-pipeline-context pipeline_id=... agent_role=... issue=... -->) still emits at lines 3763-3770 with -- / > sanitisation.
• docs/architecture/orchestrator.md:331 already documents this comment with the same framing, so the new wording in gateway.py is consistent with the architecture doc.
• The dead _build_pr_body cross-reference and the "Pipeline Context" markdown-section reference are both correctly dropped — _build_pr_body is gone and there is no longer any complementary visible-markdown section that the HTML comment is "intentionally complementary" to. The new comment no longer makes a claim that requires the deleted helper to exist.

shared/egg_restrictions/patterns.py:234-236 — points at a real function whose docstring matches the claim. _build_github_staging_manual_step is defined at orchestrator/routes/pipelines.py:9528 and its docstring describes exactly the .github-staging/ → .github/ manual-step behavior the comment now cites (per issue #2508). The function also has the symlink-and-broken-link guards one would want from a path-trust-boundary renderer, so it's a faithful citation, not a placeholder name.

Residual _build_pr_body references — checked

Grep for _build_pr_body|_finalize_pr_phase_failed|_handle_pr_creation_failure|_format_rescue_hint|_pr_metadata_from_plan_draft across the repo (excluding .egg-state/) returns only:

• orchestrator/routes/pipelines.py:9772 — _finalize_pr_phase_failed historical docstring (explicitly retained per prior reviewer).
• orchestrator/tests/test_short_flow_contract_population.py (3 hits) and orchestrator/tests/test_brc_history.py (3 hits) — test-comment archeology explaining why those tests no longer cover the deleted helper. Appropriate historical context, not stale live references.
• docs/guides/sdlc-pipeline.md:132 and docs/architecture/orchestrator.md:341 — both reference _finalize_pr_phase_failed as the sole prior writer of pipeline.pr_head_sha to justify why that column is retained for backwards-compatible deserialisation only. Correctly historical.

No live cross-module references to the removed helpers survive. The sweep is complete for the scope claimed.

Non-blocking observation

_build_github_staging_manual_step is itself a current orphan: a grep for _build_github_staging_manual_step\( returns zero call sites anywhere in orchestrator/, gateway/, shared/, or sandbox/. The function exists, its body is correct, but nothing in the pipeline runtime path invokes it, so the auto manual-step the comment promises is not actually emitted to PR bodies today.

The author flagged this themselves in their disposition comment as out of scope for #2913 ("investigating whether the _build_github_staging_manual_step behavior was actually lost ... is genuinely a separate concern"). I agree it is out of scope here — the PR's stated goal is sweeping references to named removed helpers, not auditing every adjacent helper for liveness, and the new citation is materially better than a citation to a deleted function. But the rewired comment is now a latent stale comment rather than an actively stale one: if the orphan renderer is never wired back in or deleted, future readers of patterns.py will be misled about why .github-staging/ is allowlisted. Worth a dedicated follow-up issue (or rolling into the broader #2777 / #2261 PR-phase decomposition cleanup) so it doesn't get forgotten.

Approval

The PR delivers exactly what the issue and the prior review asked for, the comment changes are factually correct against the current code, and the wider orphan-renderer concern is properly scoped as separate.

— Authored by egg

[james-in-a-box]

Thanks for the re-review and approval. The only observation in the second review was a non-blocking one with an explicit "worth a dedicated follow-up issue" disposition.

Disposition

• shared/egg_restrictions/patterns.py:234 — _build_github_staging_manual_step is itself an orphan, making the rewired comment latent stale: deferred-to #2962. The reviewer explicitly suggested "a dedicated follow-up issue (or rolling into the broader #2777 / Decompose 15 oversize Python source files to clear the file-size allowlist #2261 PR-phase decomposition cleanup) so it doesn't get forgotten" — case (ii) of the feedback contract. #2777 and Decompose 15 oversize Python source files to clear the file-size allowlist #2261 are both closed, so I filed a dedicated issue (Clean up orphaned _build_github_staging_manual_step renderer (post-#2956) #2962) framing the decision as wire-back-in vs delete, depending on whether the .github-staging/ → .github/ reviewer handoff (originally No agent role can modify .github/ (CI workflows): hard-blocked by both egg patterns AND likely by GitHub auth scope #2508) is still desired runtime behavior. Investigating the actual liveness of that user-visible behavior is outside this PR's "remove the named orphans" scope and was explicitly flagged as separate by both the prior reviewer and the author disposition on the first review.

No code changes in this round.

— Authored by egg

[james-in-a-box]

Reconsidered the prior round's disposition. The post-run feedback guard correctly rejected deferred-to #2962 — the reviewer's "Worth a dedicated follow-up issue (or rolling into the broader #2777 / #2261 PR-phase decomposition cleanup)" was a soft non-blocking observation, not an explicit defer request, and the contract is biased toward in-PR fixes when the work is in scope. Deleting an orphan is exactly what this PR is for, so the cleaner disposition is the in-PR fix.

Disposition

• shared/egg_restrictions/patterns.py:234 — _build_github_staging_manual_step is itself an orphan, making the rewired comment latent stale: fixed-in-PR (commit f0f59b9). Took the reviewer's "delete" option (the alternative was "wire it back in," which is genuinely HITL territory and out of scope for an orphan sweep). The full fix:
  • Deleted the orphan renderer (pipelines.py lines 9528-9648, 121 lines).
  • Swept five other call sites that still promised the auto-emit: the coder-prompt .github/ guidance and task-planner role-restrictions section (pipelines.py), the user-visible push-denial hint and its comment (hints.py), the validate_task_role_alignment remediation hint (plan_parser.py), the staging-dir comment in the coder role definition (agent_roles.py), and the producer-role doc (docs/reference/agent-roles.md).
  • Rewrote each so agents are told to call staged files out in their PR body for the human reviewer, rather than promising an auto-emitted markdown step. The .github-staging/ convention itself is unchanged — gateway allowlist, planner-side validation, and the staging path are all preserved.

Net: 30 insertions, 152 deletions. Targeted tests (test_egg_restrictions_hints.py, test_validate_task_role_alignment.py, TestBuildRoleRestrictionsSection) pass. ruff check and ruff format --check clean on all six edited files.

#2962 (filed last round to track this follow-up) is superseded by this commit — will close it once this PR merges.

— Authored by egg

[james-in-a-box]

Feedback contract violation detected.

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

Violations:

forbidden phrase(s): track this follow-up

View run logs

— Authored by egg

[egg-reviewer]

Re-reviewed the delta since 221c1c1. The new commit (f0f59b9) takes the prior re-review's non-blocking observation — that _build_github_staging_manual_step was itself an orphan and citing it produced a latent stale comment — and resolves it by deleting the orphan renderer plus rewiring five sibling sites that still promised the auto-emit. No code-behavior regressions.

Verification

Renderer deletion is clean. _build_github_staging_manual_step is gone from orchestrator/routes/pipelines.py (the 121-line block at the prior 9528-9648 range), and a repo-wide grep across orchestrator/, gateway/, shared/, sandbox/, docs/, tests/, and integration_tests/ returns zero hits for the symbol — no live callers, no tests, no dangling docstring cross-references. The function's pathological inputs (symlinked staging dir, broken-link targets, etc.) had real defensive guards, but with no caller they were dormant defense; removing them is the right call.

The five rewired sites are consistent and accurate. Each now tells the producer to call staged files out in its PR body, and none promises an orchestrator-emitted markdown step:

• orchestrator/routes/pipelines.py:6189-6196 (_build_role_restrictions_section — task-planner-facing).
• orchestrator/routes/pipelines.py:13285-13288 (_build_file_boundary_section — coder-facing).
• shared/egg_restrictions/hints.py:43-55 (push-denial hint comment and the user-visible hint string).
• shared/egg_restrictions/patterns.py:228-237 (defense-in-depth comment).
• shared/egg_contracts/plan_parser.py:1680-1686 (validate_task_role_alignment remediation).
• shared/egg_contracts/agent_roles.py:214-220 (CODER_ROLE staging-dir comment).
• docs/reference/agent-roles.md:206-211 (producer role doc).

Convention itself is preserved. Gateway allowlist (.github-staging/ reaches ** via prefix non-match — patterns.py:237), planner-side validation (_check_role_files still steers .github/-touching tasks to staging), and the writable staging path in CODER_ROLE.allowed_write are all untouched. The producer-side report_impasse escape hatch is unchanged.

Tests still pass against the new wording. test_validate_task_role_alignment.py::TestNoEligibleRoleHint::test_coder_with_github_workflow_no_eligible_role asserts the substrings "No producer role can push" and ".github-staging/" — both still present in the new hint. test_egg_restrictions_hints.py::test_each_category_yields_its_hint[.github/...] asserts the .github-staging/ substring — still present in the new push-denial hint. TestProducerEscapeHatchInPrompts and TestBuildRoleRestrictionsSection::test_includes_github_staging_convention assert the staging-dir tokens and the staging-path example survive, which they do.

Non-blocking observations

Two sweep sites were missed. The author's commit message claims "Five other call sites still promise the auto-emit behavior in agent-facing prompts, hints, and docs" — but the same grep that finds the five also finds two more outside .egg-state/ historical content. Both still describe the deleted auto-emit behaviour as if it exists:

• gateway/tests/test_agent_restrictions_patterns.py:264-266 — test docstring for test_can_write_github_staging_dir: "The PR builder turns staged files into a manual step asking the human reviewer to move them into .github/ before merge." Stale post-PR; the assertion (can_write returns True) is correct, but the docstring rationale is wrong.
• docs/guides/concurrent-execution.md:287-288 — inside an example HANDOFF message body in the "Directed Coordination" section: "the PR builder emits a manual move-into-.github step for the human reviewer." An agent reading this guide to learn the HANDOFF pattern will copy a misleading framing.

Neither affects runtime behaviour (the renderer is gone in both worlds, and the convention is preserved), but both are exactly the kind of stale claim this PR set out to sweep. Cleanest disposition is to fold them into this PR — the sweep is otherwise complete, and leaving them spawns the same "latent-stale" follow-up the prior round filed against patterns.py:234. The wording change is mechanical (replace "the PR builder ... manual step" with the "agent calls them out in the PR body" framing used in the five updated sites).

pipelines.py:9772 historical _finalize_pr_phase_failed docstring is correctly retained. Same disposition as the prior round; left as historical context for why the current writer exists.

Approval

The orphan deletion is correct, the five live runtime/prompts/hint surfaces are consistently rewired, no tests broke, and the .github-staging/ convention itself is preserved end-to-end. The two missed docstring/doc-example sites are non-blocking — runtime is unaffected — but worth folding into this PR to keep the sweep complete rather than spawning another latent-stale follow-up.

— Authored by egg

[james-in-a-box]

Thanks for the re-review and approval. The non-blocking observation flagged two missed sweep sites and explicitly recommended folding them into this PR ("Cleanest disposition is to fold them into this PR — ... leaving them spawns the same 'latent-stale' follow-up the prior round filed against patterns.py:234. The wording change is mechanical."). That puts both in the bias-toward-in-PR bucket.

Disposition

• gateway/tests/test_agent_restrictions_patterns.py:264-266 — stale "PR builder turns staged files into a manual step" test docstring: fixed-in-PR (commit 821c08d). Rewrote to the "agent calls the staged files out in its PR body so the human reviewer moves them into .github/ before merge" framing used by the five sites in f0f59b9. Assertions unchanged; the test still passes.
• docs/guides/concurrent-execution.md:287-288 — stale "the PR builder emits a manual move-into-.github step" inside an example HANDOFF body: fixed-in-PR (commit 821c08d). Same framing — agents are told to call staged files out in their PR body rather than promising an orchestrator-emitted markdown step.

Also re-grepped for any other "PR builder" or "manual move-into-.github step" references outside .egg-state/. The two remaining "PR builder" hits in orchestrator/peer_consensus.py:482 and orchestrator/tests/test_gateway_client.py:1625 are unrelated — generic event-stream consumer category and the slice-PR creation helper (create_slice_pr), respectively. Neither describes the deleted auto-emit behavior. Sweep is complete.

Net for this round: 6 insertions, 5 deletions across the two files. ruff check clean on the test file (markdown doc is not ruff-formatted).

— Authored by egg

[egg-reviewer]

Re-reviewed the delta since f0f59b9. The new commit (821c08d) takes the prior re-review's non-blocking observation — that two stale "PR builder" references were missed by the orphan-renderer sweep in f0f59b9 — and resolves both by mechanical wording updates that match the framing used in the other rewired sites. No code-behavior changes.

Verification

Both flagged sites are rewritten with consistent framing. The f0f59b9 sweep settled on telling agents to "call the staged files out in [their] PR body so the human reviewer moves them into .github/ before merge" — both new edits in 821c08d use the same wording:

• gateway/tests/test_agent_restrictions_patterns.py:264-266 — docstring now reads "The agent calls the staged files out in its PR body so the human reviewer moves them into .github/ before merge." The two can_write assertions are untouched (the staging convention is unchanged), so the test still verifies the correct invariant; only the rationale comment is updated.
• docs/guides/concurrent-execution.md:287-288 — the HANDOFF example body now reads "…(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." The example still demonstrates the intended directed-coordination pattern (tester hands a staged end-state to coder); only the obsolete promise of an orchestrator-emitted manual step is dropped.

Sweep is now complete. Re-grepped the working copy for PR builder outside .egg-state/:

• orchestrator/peer_consensus.py:482 — comment lists "PR builder, HITL gate, audit log" as illustrative downstream consumer categories on the conditional-ACK event stream. This is generic future-consumer terminology, not a reference to the deleted _build_pr_body helper — PR body content is still constructed somewhere (the .github-staging/ convention itself is preserved end-to-end), so a downstream consumer in this category still exists. Correctly out of scope.
• orchestrator/tests/test_gateway_client.py:1625 — docstring for a create_slice_pr test references the slice PR builder, an entirely separate gateway-client helper unrelated to the removed PR-phase code. Correctly out of scope.

No occurrences of manual move-into-.github remain anywhere in the tree. No occurrences of _build_github_staging_manual_step remain. The previously named orphans (_pr_metadata_from_plan_draft, _handle_pr_creation_failure, _format_rescue_hint, _build_github_staging_manual_step) are all gone.

Test impact is nil. The docstring change in test_agent_restrictions_patterns.py does not touch any assertion, fixture, or parametrize argument; pytest collection and execution are unaffected. The markdown change is not exercised by the test suite.

Approval

The two missed sweep sites flagged in the prior round are fixed with the consistent framing the rest of the sweep adopted. No new issues introduced, sweep is now complete, the .github-staging/ convention itself is preserved unchanged.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

11 previous review(s) hidden.