diff --git a/orchestrator/routes/pipelines.py b/orchestrator/routes/pipelines.py index 0a3f778f85..a76accdb50 100644 --- a/orchestrator/routes/pipelines.py +++ b/orchestrator/routes/pipelines.py @@ -8375,134 +8375,6 @@ def _commit_statefiles_to_worktree( return True -def _cleanup_agent_outputs_for_pr( - worktree_path: Path, - pipeline_id: str, -) -> None: - """Remove ``.egg-state/agent-outputs/`` from the PR branch at PR-phase entry. - - Files under ``.egg-state/agent-outputs/`` are coder→tester handoff - artifacts (e.g. ``coder-test-changes.patch``) that the tester consumes - and re-emits as real source/test files. They are ephemeral: once the - implement phase closes, nothing on the PR branch should reference them. - - Leaving them on the branch causes two problems: - - 1. Concurrent pipelines can write different contents to the same path - (e.g. two coder runs producing divergent patches), making the - orchestrator's PR-phase worktree and ``origin/`` diverge in - a way that merge/rebase reconcile cannot auto-resolve (see #1731). - 2. The PR itself then ships throwaway artifacts that add noise to - reviewers' diffs. - - This helper runs once at PR-phase entry, unstages/removes any tracked - agent-outputs, and commits the cleanup. If nothing is tracked, it - no-ops. All subprocess errors are swallowed with a warning — cleanup - is best-effort. - """ - state_dir = worktree_path / ".egg-state" / "agent-outputs" - logger.info( - "_cleanup_agent_outputs_for_pr: entering", - worktree_path=str(worktree_path), - pipeline_id=pipeline_id, - agent_outputs_exists=state_dir.exists(), - ) - - git_base = [ - "git", - "-c", - "core.hooksPath=/dev/null", - "-c", - f"safe.directory={worktree_path}", - "-C", - str(worktree_path), - ] - - try: - # Remove from both the index and the working tree. ``--ignore-unmatch`` - # makes this a no-op when nothing is tracked under that path. - # ``-r`` recurses; ``-f`` forces removal even if files were modified. - subprocess.run( - [ - *git_base, - "rm", - "-rf", - "--ignore-unmatch", - "--", - ".egg-state/agent-outputs", - ], - capture_output=True, - text=True, - check=True, - timeout=30, - ) - except subprocess.CalledProcessError as rm_err: - logger.warning( - "_cleanup_agent_outputs_for_pr: git rm failed — continuing", - pipeline_id=pipeline_id, - stderr=rm_err.stderr, - ) - return - except subprocess.TimeoutExpired: - logger.warning( - "_cleanup_agent_outputs_for_pr: git rm timed out — continuing", - pipeline_id=pipeline_id, - ) - return - - # Only commit when the index actually changed (idempotent on re-runs). - try: - diff_result = subprocess.run( - [*git_base, "diff", "--cached", "--quiet"], - capture_output=True, - text=True, - check=False, - timeout=30, - ) - except subprocess.TimeoutExpired: - logger.warning( - "_cleanup_agent_outputs_for_pr: git diff --cached timed out — continuing", - pipeline_id=pipeline_id, - ) - return - if diff_result.returncode == 0: - logger.info( - "_cleanup_agent_outputs_for_pr: nothing tracked — skipping commit", - pipeline_id=pipeline_id, - ) - return - - try: - subprocess.run( - [ - *git_base, - "commit", - "--no-verify", - "-m", - "Remove ephemeral agent-output handoff artifacts (#1731)", - ], - capture_output=True, - text=True, - check=True, - timeout=30, - ) - logger.info( - "_cleanup_agent_outputs_for_pr: commit succeeded", - pipeline_id=pipeline_id, - ) - except subprocess.CalledProcessError as commit_err: - logger.warning( - "_cleanup_agent_outputs_for_pr: commit failed — continuing", - pipeline_id=pipeline_id, - stderr=commit_err.stderr, - ) - except subprocess.TimeoutExpired: - logger.warning( - "_cleanup_agent_outputs_for_pr: commit timed out — continuing", - pipeline_id=pipeline_id, - ) - - def _ensure_statefiles_on_branch( worktree_repo_path: Path, pipeline: Pipeline, diff --git a/orchestrator/tests/test_cleanup_agent_outputs_for_pr.py b/orchestrator/tests/test_cleanup_agent_outputs_for_pr.py deleted file mode 100644 index 1c1ef0ff21..0000000000 --- a/orchestrator/tests/test_cleanup_agent_outputs_for_pr.py +++ /dev/null @@ -1,121 +0,0 @@ -""" -Tests for ``_cleanup_agent_outputs_for_pr``. - -This helper runs at PR-phase entry to drop the ephemeral coder→tester -handoff artifacts in ``.egg-state/agent-outputs/`` before the PR is -created. See jwbron/egg#1731. -""" - -import subprocess -from unittest.mock import MagicMock, patch - - -def _run_result(returncode=0, stdout="", stderr=""): - """Build a CompletedProcess stand-in for subprocess.run mocks.""" - result = MagicMock(spec=subprocess.CompletedProcess) - result.returncode = returncode - result.stdout = stdout - result.stderr = stderr - return result - - -class TestCleanupAgentOutputsForPr: - def test_noop_when_agent_outputs_missing(self, tmp_path): - """When .egg-state/agent-outputs doesn't exist, still runs git rm - --ignore-unmatch (cheap) and exits cleanly without committing.""" - from routes.pipelines import _cleanup_agent_outputs_for_pr - - # No directory created — the helper should still run git rm - # --ignore-unmatch (which no-ops) and skip the commit via the - # diff --cached --quiet check. - with patch( - "routes.pipelines.subprocess.run", - side_effect=[ - _run_result(), # git rm -rf --ignore-unmatch - _run_result(returncode=0), # diff --cached --quiet → empty index - ], - ) as mock_run: - _cleanup_agent_outputs_for_pr(tmp_path, "issue-42") - - # No commit attempted - all_cmds = [c.args[0] for c in mock_run.call_args_list] - assert not any("commit" in c for c in all_cmds) - - def test_removes_and_commits_when_tracked(self, tmp_path): - """When agent-outputs files are tracked, git rm stages removal and we commit.""" - from routes.pipelines import _cleanup_agent_outputs_for_pr - - (tmp_path / ".egg-state" / "agent-outputs").mkdir(parents=True) - (tmp_path / ".egg-state" / "agent-outputs" / "coder-test-changes.patch").write_text( - "diff --git a/... b/...\n" - ) - - with patch( - "routes.pipelines.subprocess.run", - side_effect=[ - _run_result(), # git rm -rf - _run_result(returncode=1), # diff --cached --quiet → staged changes present - _run_result(), # commit - ], - ) as mock_run: - _cleanup_agent_outputs_for_pr(tmp_path, "issue-42") - - all_cmds = [c.args[0] for c in mock_run.call_args_list] - # git rm was called with --ignore-unmatch and targeted at agent-outputs - rm_cmd = all_cmds[0] - assert "rm" in rm_cmd - assert "--ignore-unmatch" in rm_cmd - assert ".egg-state/agent-outputs" in rm_cmd - # Commit was called with the canonical message - commit_cmd = all_cmds[-1] - assert "commit" in commit_cmd - assert any( - "Remove ephemeral agent-output handoff artifacts" in str(arg) for arg in commit_cmd - ) - - def test_swallows_rm_failure(self, tmp_path): - """A failing git rm is logged and swallowed — cleanup is best-effort.""" - from routes.pipelines import _cleanup_agent_outputs_for_pr - - (tmp_path / ".egg-state" / "agent-outputs").mkdir(parents=True) - - with patch( - "routes.pipelines.subprocess.run", - side_effect=subprocess.CalledProcessError( - returncode=1, cmd="git rm", stderr="fatal: unable to remove" - ), - ) as mock_run: - # Must not raise. - _cleanup_agent_outputs_for_pr(tmp_path, "issue-42") - - # Only the rm attempt — no diff check, no commit. - assert mock_run.call_count == 1 - - def test_swallows_rm_timeout(self, tmp_path): - """A timed-out git rm is logged and swallowed.""" - from routes.pipelines import _cleanup_agent_outputs_for_pr - - with patch( - "routes.pipelines.subprocess.run", - side_effect=subprocess.TimeoutExpired(cmd="git rm", timeout=30), - ): - _cleanup_agent_outputs_for_pr(tmp_path, "issue-42") - - def test_swallows_commit_failure(self, tmp_path): - """A failing commit is logged and swallowed; helper returns normally.""" - from routes.pipelines import _cleanup_agent_outputs_for_pr - - (tmp_path / ".egg-state" / "agent-outputs").mkdir(parents=True) - - with patch( - "routes.pipelines.subprocess.run", - side_effect=[ - _run_result(), # rm - _run_result(returncode=1), # diff: has staged - subprocess.CalledProcessError( - returncode=1, cmd="git commit", stderr="nothing to commit" - ), - ], - ): - # Must not raise. - _cleanup_agent_outputs_for_pr(tmp_path, "issue-42")