Remove orphaned _cleanup_agent_outputs_for_pr (no production call sites since #2891) - #2909
Merged
Merged
Conversation
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Approve — clean orphan removal
Verified the central claim against the tip of the PR branch:
grep -rn _cleanup_agent_outputs_for_pracrossorchestrator/,shared/,gateway/,sandbox/, anddocs/returns zero hits after this PR. The only remaining references are in.egg-state/brc-history/and.egg-state/drafts/artifacts (out of review scope).- The deleted test file (
orchestrator/tests/test_cleanup_agent_outputs_for_pr.py) was the sole test that imported the helper. - The deletion site (
pipelines.py:8375-8378before /:8375-8378after) is mechanically clean — no orphaned import, no trailing comment, no dangling reference. _open_context_pr_at_implement_start(pipelines.py:9973) — the new plan→implement context-PR seam that replaced the deleted PR phase — does not invoke any agent-outputs cleanup. Confirmed by reading the four-step behaviour in its docstring and the body through line 10247.- CI is green on the head sha (Unit Tests, Integration Tests, Lint, Security Scan, Custom Checks all SUCCESS).
On the delete-vs-rewire trade-off
Issue #2899 frames this as an operator decision: with cleanup gone, ephemeral .egg-state/agent-outputs/ files will land in the context PR's diff (verified: .gitignore:64 keeps .egg-state/agent-outputs/ tracked; only last-known-good/, selection/, grimp-cache/, and oversight/* are ignored). That is accepted noise, not a correctness regression — the original docstring listed two motivations:
- Cross-pipeline divergence on shared paths (#1731). Already mitigated by the push-reconcile auto-resolve in
gateway_client.py:3361-3418, which treats.egg-state/agent-outputs/conflicts as "take remote" during rebase. The pre-PR-phase scrub was a second line of defence; removing it leaves the primary mechanism intact. - Diff noise. A reviewer-UX cost, not a correctness one.
Both concerns survive the deletion cleanly. Reasonable operator call.
Non-blocking — sibling orphan not addressed
The comment on #2899 flags _pr_metadata_from_plan_draft (pipelines.py:9675-9732) as a sibling orphan from the same #2891 PR-phase removal. This PR's title scopes to _cleanup_agent_outputs_for_pr, so leaving the sibling for a follow-up is fine — but worth a separate cleanup PR rather than letting it linger.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
Contributor
| egg is addressing review feedback... |
4 tasks
Contributor
|
Thanks for the review and the verification trace. Per-item disposition:
— Authored by egg |
Contributor
|
egg feedback addressed. View run logs 1 previous review(s) hidden. |
4 tasks
jwbron
added a commit
that referenced
this pull request
Jun 3, 2026
* refactor(orchestrator): remove orphaned PR-phase helpers (#2913) #2777 hard-removed `_build_pr_body` / `_finalize_pr_phase_failed` as part of the PR-phase removal arc (#2891), leaving their only callees dangling. This sweeps the remaining orphans flagged by the #2909 reviewer and the #2777 BRC transcripts (deferred to TASK-3-11 but never swept): - `_pr_metadata_from_plan_draft` — its only caller was `_build_pr_body`. - `_handle_pr_creation_failure` — its only caller was the removed `_finalize_pr_phase_failed` path. - `_format_rescue_hint` — sole caller was `_handle_pr_creation_failure`, so it goes dead with it. Removed too rather than leaving a fresh orphan (the #2261 decomposition plan groups all three in the dead `_pr_lifecycle` cluster). Also fixes the stale comment at `pipelines.py` 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 is dropped. `grep -rn` for all three names now returns zero non-historical hits across orchestrator/, shared/, gateway/, sandbox/, docs/. No tests referenced any of them (the `_build_pr_body` tests were removed in #2777). Full `make test` green: 17186 passed, 34 skipped. Closes #2913 * refactor(orchestrator): sweep stale _build_pr_body comment refs (#2913) Address review-feedback observations: two comments still pointed at the `_build_pr_body` helper that #2777 removed. - `shared/egg_restrictions/patterns.py`: rewire the `.github/` block rationale to cite `_build_github_staging_manual_step`, which is the current renderer for the staged-files manual step (issue #2508). - `gateway/gateway.py`: drop the dead cross-reference to the removed helper and its (also removed) `## Pipeline Context` markdown section. The HTML-comment metadata injection the gateway does is now described on its own terms — tooling-facing, recovers pipeline_id/agent_role/ issue from the PR body without round-tripping through state. The third observation (the `_finalize_pr_phase_failed` docstring at `orchestrator/routes/pipelines.py:9772`) is left as-is — it is appropriate historical documentation explaining why the current writer exists, per the reviewer's note. * refactor(orchestrator): delete orphan _build_github_staging_manual_step (#2913) Address review feedback on the prior commit. The patterns.py:234 comment edit cited `_build_github_staging_manual_step`, but that renderer is itself a current orphan — its only caller (`_build_pr_body`) was removed by #2777, so the auto manual-step it advertises is never emitted to PR bodies today. Five other call sites still promise the auto-emit behavior in agent-facing prompts, hints, and docs. Two options were on the table: wire the renderer back in, or delete it. Deletion matches this PR's stated 'remove orphaned PR-phase helpers' scope. Sweep: - `orchestrator/routes/pipelines.py`: delete the orphan renderer (lines 9528-9648); rewrite the coder-prompt `.github/` guidance and the task-planner role-restrictions section so they tell the agent to call staged files out in the PR body rather than promising an auto-emitted manual step. - `shared/egg_restrictions/patterns.py`: drop the `_build_github_staging_manual_step` citation; the comment now describes the human-reviewer responsibility on its own terms. - `shared/egg_restrictions/hints.py`: update both the comment and the user-visible push-denial hint so agents know to flag staged files in the PR body. - `shared/egg_contracts/plan_parser.py`: update the `validate_task_role_alignment` remediation hint with the same framing. - `shared/egg_contracts/agent_roles.py`: update the staging-dir comment in the coder role definition. - `docs/reference/agent-roles.md`: update the producer role documentation. The `.github-staging/` convention itself is unchanged — agents can still stage proposed `.github/` changes there, the gateway still allowlists the path, and the planner-side validation still steers `.github/`-touching tasks to staging. Only the now-extinct claim that the PR builder auto-emits a markdown manual step has been removed. Net: 30 insertions, 152 deletions. Supersedes #2962 (filed during the prior re-review round to track this follow-up); closing that issue once this PR merges. Refs #2913. * docs(restrictions): sweep two stale "PR builder" references (#2913) Address re-review on commit f0f59b9. Two sites outside .egg-state/ still described the deleted _build_github_staging_manual_step auto-emit as if it existed; the reviewer flagged them as in-scope for this sweep and suggested folding them in rather than spawning a follow-up: - gateway/tests/test_agent_restrictions_patterns.py:264-266 — test docstring for test_can_write_github_staging_dir. - docs/guides/concurrent-execution.md:287-288 — body of an example HANDOFF message in the Directed Coordination section. Both rewrites use the same "agent calls staged files out in its PR body so the human reviewer moves them into .github/ before merge" framing established by the five sites swept in f0f59b9. No runtime behavior changes; the .github-staging/ convention is unchanged end-to-end. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2899.
Delete the orphaned
_cleanup_agent_outputs_for_prfunction (pipelines.py:8378-8503, 126 lines) and its test file (121 lines). It had zero production call sites since #2891 removed the PR phase that used it at PR-phase entry._open_context_pr_at_implement_startdoesn't invoke it and the new context-PR seam works fine without it, so: delete rather than rewire.