From 72fb0aaadcbf3f2df10ce57c486dd16402d9bce8 Mon Sep 17 00:00:00 2001 From: vitali87 Date: Sun, 30 Aug 2026 19:16:51 +0100 Subject: [PATCH 01/14] fix: prune stale tracking refs before pushing so re-split branches are not rejected as stale --- pr_split/cli.py | 6 ++ pr_split/git_ops/__init__.py | 3 + pr_split/git_ops/branches.py | 27 +++++ pr_split/logs.py | 4 + tests/test_cli_helpers.py | 16 +++ tests/test_cli_new_features.py | 2 + tests/test_git_branches.py | 184 +++++++++++++++++++++++++++++++-- 7 files changed, 236 insertions(+), 6 deletions(-) diff --git a/pr_split/cli.py b/pr_split/cli.py index 2e65e0c..52a2eea 100644 --- a/pr_split/cli.py +++ b/pr_split/cli.py @@ -63,6 +63,7 @@ fetch_fork_pr, is_worktree_clean, merge_base, + prune_remote_tracking_refs, push_branch, remove_worktree, ) @@ -463,6 +464,11 @@ def _push_and_create_prs( record_map = {r.group_id: r for r in branch_records} errors: list[tuple[str, Exception]] = [] + # Branch names are reused across runs; drop tracking refs left by an + # earlier split whose remote branch was merged or deleted since, or + # force-with-lease rejects every push as stale. + prune_remote_tracking_refs() + # Children target parent branches, so every branch is pushed before any PR opens. with ThreadPoolExecutor(max_workers=_PUSH_MAX_WORKERS) as executor: push_futures = { diff --git a/pr_split/git_ops/__init__.py b/pr_split/git_ops/__init__.py index 7bd6dfb..545af0b 100644 --- a/pr_split/git_ops/__init__.py +++ b/pr_split/git_ops/__init__.py @@ -28,6 +28,9 @@ from .branches import ( merge_base as merge_base, ) +from .branches import ( + prune_remote_tracking_refs as prune_remote_tracking_refs, +) from .branches import ( push_branch as push_branch, ) diff --git a/pr_split/git_ops/branches.py b/pr_split/git_ops/branches.py index 0861dfb..ff7ce6e 100644 --- a/pr_split/git_ops/branches.py +++ b/pr_split/git_ops/branches.py @@ -1,5 +1,6 @@ from __future__ import annotations +import contextlib import os import re import subprocess @@ -110,10 +111,36 @@ def delete_branch(branch: str, *, remote: bool = False) -> None: if _REMOTE_REF_MISSING not in str(exc): raise logger.info(logs.BRANCH_ALREADY_GONE.format(branch=branch, where=" on origin")) + # Either way the remote branch is gone; drop the local tracking ref + # too, or the next `push --force-with-lease` of a reused branch name + # is rejected as "stale info" against a ref origin no longer has. + forget_remote_tracking_ref(branch) if local_error is not None: raise local_error +def forget_remote_tracking_ref(branch: str) -> None: + with contextlib.suppress(GitOperationError): + run_git("update-ref", "-d", f"refs/remotes/origin/{branch}") + + +def prune_remote_tracking_refs() -> None: + """Drop tracking refs whose remote branch no longer exists. + + Split branch names are reused across runs; after a merge or `clean` the + remote branch is gone but `refs/remotes/origin/pr-split/...` may still + point at the old head, and every `push --force-with-lease` would be + rejected as stale. Only *gone* refs are dropped (`git remote prune`, no + fetch): refreshing the refs that still exist would make the lease + compare against origin's current tip and silently force over commits + someone else pushed to a reused branch. + """ + try: + run_git("remote", "prune", "origin") + except GitOperationError as exc: + logger.warning(logs.PRUNE_FAILED.format(error=exc)) + + def merge_base(ref_a: str, ref_b: str) -> str: return run_git("merge-base", ref_a, ref_b) diff --git a/pr_split/logs.py b/pr_split/logs.py index 6e773c9..e3bace2 100644 --- a/pr_split/logs.py +++ b/pr_split/logs.py @@ -74,3 +74,7 @@ PR_SKIPPED_BASE_NOT_PUSHED = ( "Skipping PR for group '{group}': its base branch '{base}' was not pushed" ) +PRUNE_FAILED = ( + "Could not prune origin's tracking refs before pushing ({error}); " + "a reused branch name may be rejected as stale" +) diff --git a/tests/test_cli_helpers.py b/tests/test_cli_helpers.py index 911bfe2..a9cea8b 100644 --- a/tests/test_cli_helpers.py +++ b/tests/test_cli_helpers.py @@ -627,3 +627,19 @@ def push(branch: str) -> None: with pytest.raises(PRSplitError): _push_and_create_prs(groups, records) assert mock_create.call_count == 0 + + +class TestPushPrunesFirst: + @patch("pr_split.cli.create_pr", return_value=(1, "https://github.com/pr/1")) + @patch("pr_split.cli.push_branch") + @patch("pr_split.cli.prune_remote_tracking_refs") + def test_tracking_refs_are_refreshed_before_any_push( + self, mock_prune: MagicMock, mock_push: MagicMock, mock_create: MagicMock + ) -> None: + order: list[str] = [] + mock_prune.side_effect = lambda: order.append("prune") + mock_push.side_effect = lambda branch: order.append("push") + _push_and_create_prs( + [_group("pr-1", "feat: a")], [_branch_record("pr-1", "pr-split/ns/pr-1")] + ) + assert order == ["prune", "push"] diff --git a/tests/test_cli_new_features.py b/tests/test_cli_new_features.py index 81716ed..f050fe2 100644 --- a/tests/test_cli_new_features.py +++ b/tests/test_cli_new_features.py @@ -401,6 +401,7 @@ def test_close_failure_warning_includes_the_reason( class TestCleanupAfterMergeDeletedBranches: + @patch("pr_split.git_ops.branches.forget_remote_tracking_ref") @patch("pr_split.cli.get_pr_state", return_value={"state": "MERGED"}) @patch("pr_split.cli.shutil.rmtree") @patch("pr_split.cli.Path") @@ -413,6 +414,7 @@ def test_fully_merged_split_cleans_up_completely( mock_path: MagicMock, mock_rmtree: MagicMock, mock_state: MagicMock, + mock_forget: MagicMock, ) -> None: mock_path.return_value.exists.return_value = True # merge --delete-branch already removed both local and remote branches diff --git a/tests/test_git_branches.py b/tests/test_git_branches.py index 11f9cfd..51744c8 100644 --- a/tests/test_git_branches.py +++ b/tests/test_git_branches.py @@ -125,14 +125,18 @@ def test_local_only(self, mock_git: MagicMock) -> None: delete_branch("pr-split/pr-1") mock_git.assert_called_once_with("branch", "-D", "pr-split/pr-1") + @patch("pr_split.git_ops.branches.forget_remote_tracking_ref") @patch("pr_split.git_ops.branches.run_git") - def test_with_remote(self, mock_git: MagicMock) -> None: + def test_with_remote(self, mock_git: MagicMock, mock_forget: MagicMock) -> None: mock_git.return_value = "" delete_branch("pr-split/pr-1", remote=True) assert mock_git.call_count == 2 + @patch("pr_split.git_ops.branches.forget_remote_tracking_ref") @patch("pr_split.git_ops.branches.run_git") - def test_local_failure_still_deletes_remote(self, mock_git: MagicMock) -> None: + def test_local_failure_still_deletes_remote( + self, mock_git: MagicMock, mock_forget: MagicMock + ) -> None: mock_git.side_effect = [GitOperationError("checked out"), ""] with pytest.raises(GitOperationError, match="checked out"): delete_branch("pr-split/pr-1", remote=True) @@ -145,15 +149,21 @@ def test_local_failure_without_remote_raises_immediately(self, mock_git: MagicMo delete_branch("pr-split/pr-1") mock_git.assert_called_once() + @patch("pr_split.git_ops.branches.forget_remote_tracking_ref") @patch("pr_split.git_ops.branches.run_git") - def test_branch_already_deleted_locally_counts_as_deleted(self, mock_git: MagicMock) -> None: + def test_branch_already_deleted_locally_counts_as_deleted( + self, mock_git: MagicMock, mock_forget: MagicMock + ) -> None: # `pr-split merge` deletes the local branch; cleanup must not fail on it. mock_git.side_effect = [GitOperationError("error: branch 'pr-split/pr-1' not found."), ""] delete_branch("pr-split/pr-1", remote=True) mock_git.assert_any_call("push", "origin", "--delete", "pr-split/pr-1") + @patch("pr_split.git_ops.branches.forget_remote_tracking_ref") @patch("pr_split.git_ops.branches.run_git") - def test_branch_already_deleted_on_origin_counts_as_deleted(self, mock_git: MagicMock) -> None: + def test_branch_already_deleted_on_origin_counts_as_deleted( + self, mock_git: MagicMock, mock_forget: MagicMock + ) -> None: mock_git.side_effect = [ "", GitOperationError( @@ -162,8 +172,11 @@ def test_branch_already_deleted_on_origin_counts_as_deleted(self, mock_git: Magi ] delete_branch("pr-split/pr-1", remote=True) + @patch("pr_split.git_ops.branches.forget_remote_tracking_ref") @patch("pr_split.git_ops.branches.run_git") - def test_branch_gone_everywhere_counts_as_deleted(self, mock_git: MagicMock) -> None: + def test_branch_gone_everywhere_counts_as_deleted( + self, mock_git: MagicMock, mock_forget: MagicMock + ) -> None: mock_git.side_effect = [ GitOperationError("error: branch 'pr-split/pr-1' not found."), GitOperationError( @@ -177,8 +190,11 @@ def test_missing_local_branch_without_remote_is_fine(self, mock_git: MagicMock) mock_git.side_effect = GitOperationError("error: branch 'pr-split/pr-1' not found.") delete_branch("pr-split/pr-1") + @patch("pr_split.git_ops.branches.forget_remote_tracking_ref") @patch("pr_split.git_ops.branches.run_git") - def test_other_remote_failure_still_raises(self, mock_git: MagicMock) -> None: + def test_other_remote_failure_still_raises( + self, mock_git: MagicMock, mock_forget: MagicMock + ) -> None: mock_git.side_effect = ["", GitOperationError("fatal: could not read from remote")] with pytest.raises(GitOperationError, match="could not read from remote"): delete_branch("pr-split/pr-1", remote=True) @@ -399,3 +415,159 @@ def test_localised_git_still_reports_already_deleted( monkeypatch.setenv("LANGUAGE", "de") # never existed: must be treated as already deleted, not as an error delete_branch("pr-split/ns/never") + + +class TestStaleRemoteTrackingRefs: + def _repo_with_origin(self, tmp_path: Path) -> Path: + origin = tmp_path / "origin.git" + subprocess.run(["git", "init", "-q", "--bare", str(origin)], check=True) + repo = tmp_path / "repo" + subprocess.run(["git", "clone", "-q", str(origin), str(repo)], check=True) + subprocess.run( + [ + "git", + "-c", + "user.name=t", + "-c", + "user.email=t@x", + "commit", + "-q", + "--allow-empty", + "-m", + "base", + ], + cwd=repo, + check=True, + ) + subprocess.run(["git", "push", "-q", "-u", "origin", "HEAD:main"], cwd=repo, check=True) + return repo + + def test_reused_branch_name_pushes_again_after_remote_deletion( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + from pr_split.git_ops.branches import prune_remote_tracking_refs, push_branch + + repo = self._repo_with_origin(tmp_path) + monkeypatch.chdir(repo) + subprocess.run(["git", "branch", "pr-split/ns/pr-1"], cwd=repo, check=True) + push_branch("pr-split/ns/pr-1") + # Simulate `gh pr merge --delete-branch` / GitHub deleting the head + # branch behind our back: the remote branch goes away but the local + # tracking ref still points at the old head. + subprocess.run( + ["git", "update-ref", "-d", "refs/heads/pr-split/ns/pr-1"], + cwd=tmp_path / "origin.git", + check=True, + ) + subprocess.run( + [ + "git", + "-c", + "user.name=t", + "-c", + "user.email=t@x", + "commit", + "-q", + "--allow-empty", + "-m", + "v2", + ], + cwd=repo, + check=True, + ) + subprocess.run(["git", "branch", "-f", "pr-split/ns/pr-1", "HEAD"], cwd=repo, check=True) + with pytest.raises(GitOperationError, match="stale info"): + push_branch("pr-split/ns/pr-1") + + prune_remote_tracking_refs() + push_branch("pr-split/ns/pr-1") + + def test_prune_keeps_the_lease_against_third_party_pushes( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + from pr_split.git_ops.branches import prune_remote_tracking_refs, push_branch + + repo = self._repo_with_origin(tmp_path) + monkeypatch.chdir(repo) + subprocess.run(["git", "branch", "pr-split/ns/pr-9"], cwd=repo, check=True) + push_branch("pr-split/ns/pr-9") + # A reviewer pushes a fixup to the split branch from another clone. + other = tmp_path / "other" + subprocess.run( + ["git", "clone", "-q", str(tmp_path / "origin.git"), str(other)], check=True + ) + subprocess.run(["git", "checkout", "-q", "pr-split/ns/pr-9"], cwd=other, check=True) + subprocess.run( + [ + "git", + "-c", + "user.name=r", + "-c", + "user.email=r@x", + "commit", + "-q", + "--allow-empty", + "-m", + "fixup", + ], + cwd=other, + check=True, + ) + subprocess.run(["git", "push", "-q", "origin", "pr-split/ns/pr-9"], cwd=other, check=True) + subprocess.run( + [ + "git", + "-c", + "user.name=t", + "-c", + "user.email=t@x", + "commit", + "-q", + "--allow-empty", + "-m", + "resplit", + ], + cwd=repo, + check=True, + ) + subprocess.run(["git", "branch", "-f", "pr-split/ns/pr-9", "HEAD"], cwd=repo, check=True) + + prune_remote_tracking_refs() + + # The lease must still protect the reviewer's commit. + with pytest.raises(GitOperationError, match="stale info"): + push_branch("pr-split/ns/pr-9") + + def test_remote_delete_drops_the_tracking_ref( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + from pr_split.git_ops.branches import push_branch + + repo = self._repo_with_origin(tmp_path) + monkeypatch.chdir(repo) + subprocess.run(["git", "branch", "pr-split/ns/pr-2"], cwd=repo, check=True) + push_branch("pr-split/ns/pr-2") + assert branch_exists("refs/remotes/origin/pr-split/ns/pr-2") + + delete_branch("pr-split/ns/pr-2", remote=True) + + assert not branch_exists("refs/remotes/origin/pr-split/ns/pr-2") + + def test_already_deleted_remote_still_drops_the_tracking_ref( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + from pr_split.git_ops.branches import push_branch + + repo = self._repo_with_origin(tmp_path) + monkeypatch.chdir(repo) + subprocess.run(["git", "branch", "pr-split/ns/pr-3"], cwd=repo, check=True) + push_branch("pr-split/ns/pr-3") + subprocess.run( + ["git", "update-ref", "-d", "refs/heads/pr-split/ns/pr-3"], + cwd=tmp_path / "origin.git", + check=True, + ) + + delete_branch("pr-split/ns/pr-3", remote=True) + + assert not branch_exists("refs/remotes/origin/pr-split/ns/pr-3") From de40416d3ab4e79bbe4a2c241ca18582a7c6804d Mon Sep 17 00:00:00 2001 From: vitali87 Date: Sun, 30 Aug 2026 20:35:59 +0100 Subject: [PATCH 02/14] fix: retry execute after a failed push instead of forcing a full re-plan A push/PR-creation failure leaves the plan with branches recorded and no PRs. execute then refused to run ('use clean first') and clean deletes the plan file entirely, so recovering from a transient network failure meant regenerating the whole plan - for the LLM backend, paying for planning again and possibly getting a different split. Branch creation is idempotent (add_worktree recreates an existing branch), so execute now retries when the saved state has branches but no PRs, and only refuses when PRs already exist. --- pr_split/cli.py | 60 +++++++++++- tests/test_cli_coverage.py | 186 ++++++++++++++++++++++++++++++++++++- 2 files changed, 241 insertions(+), 5 deletions(-) diff --git a/pr_split/cli.py b/pr_split/cli.py index 1a16831..d543c4d 100644 --- a/pr_split/cli.py +++ b/pr_split/cli.py @@ -45,7 +45,13 @@ merge_chain_assignments, parse_diff, ) -from .exceptions import ErrorMsg, PlanValidationError, PRCreationError, PRSplitError +from .exceptions import ( + ErrorMsg, + GitOperationError, + PlanValidationError, + PRCreationError, + PRSplitError, +) from .git_ops import ( add_worktree, branch_exists, @@ -967,6 +973,44 @@ def clean() -> None: logger.success(logs.CLEAN_COMPLETE.format(branches=deleted_branches, prs=closed_prs)) +def _delete_stale_recorded_branches( + records: list[BranchRecord], groups: list[Group], namespace: str +) -> None: + """Remove branches from a previous failed run that the plan no longer contains. + + A retried execute recreates only the current groups and then overwrites + git_state; without this, a branch created before the plan was edited would + survive locally (and on origin, if its push succeeded) with no record left + for `pr-split clean` to delete. + """ + expected = {f"{BRANCH_PREFIX}{namespace}/{group.id}" for group in groups} + for record in records: + if record.branch_name in expected: + continue + if branch_exists(record.branch_name): + try: + delete_branch(record.branch_name) + except GitOperationError as exc: + console.print( + f"[yellow]Could not delete stale branch" + f" {escape(record.branch_name)}: {escape(str(exc))}[/yellow]" + ) + # The remote copy is handled independently: the local branch may be + # gone (manual prune, an earlier retry) while the pushed one survives, + # and delete_branch would raise before reaching the remote delete. + try: + run_git("ls-remote", "--exit-code", "origin", f"refs/heads/{record.branch_name}") + except GitOperationError: + continue # never pushed (or remote unreachable): nothing to delete + try: + run_git("push", "origin", "--delete", record.branch_name) + except GitOperationError as exc: + console.print( + f"[yellow]Could not delete stale remote branch" + f" {escape(record.branch_name)}: {escape(str(exc))}[/yellow]" + ) + + @app.command( help="Execute a previously saved dry-run plan, creating branches and PRs.", ) @@ -999,9 +1043,18 @@ def execute( if draft and not plan.draft: plan = plan.model_copy(update={"draft": True}) - if plan_file.git_state.branches or plan_file.git_state.prs: - console.print("[red]This plan already has branches/PRs. Use 'pr-split clean' first.[/red]") + if plan_file.git_state.prs: + console.print("[red]This plan already has PRs. Use 'pr-split clean' first.[/red]") raise typer.Exit(1) + if plan_file.git_state.branches: + # A previous execute created branches but no PRs - a failed push or PR + # creation. Branch creation is idempotent (add_worktree recreates an + # existing branch), so retry instead of forcing 'clean' + a full + # re-plan, which for the LLM backend means paying for planning again. + console.print( + "[yellow]A previous run created branches but no PRs" + " (the push or PR creation failed). Recreating them and retrying.[/yellow]" + ) if not plan.raw_diff: console.print( @@ -1046,6 +1099,7 @@ def execute( typer.confirm("Proceed with creating branches and PRs?", abort=True) namespace = derive_split_namespace(plan.dev_branch_arg or plan.dev_branch) + _delete_stale_recorded_branches(plan_file.git_state.branches, plan.groups, namespace) try: branch_records = _create_branches_and_commits( plan.groups, diff --git a/tests/test_cli_coverage.py b/tests/test_cli_coverage.py index 371d3ac..5db3818 100644 --- a/tests/test_cli_coverage.py +++ b/tests/test_cli_coverage.py @@ -490,13 +490,14 @@ def test_execute_no_plan(self, mock_pe: MagicMock) -> None: @patch("pr_split.cli.load_plan") @patch("pr_split.cli.plan_exists", return_value=True) - def test_execute_already_has_git_state(self, mock_pe: MagicMock, mock_load: MagicMock) -> None: + def test_execute_already_has_prs(self, mock_pe: MagicMock, mock_load: MagicMock) -> None: mock_plan_file = MagicMock() mock_plan_file.git_state.branches = [MagicMock()] - mock_plan_file.git_state.prs = [] + mock_plan_file.git_state.prs = [MagicMock()] mock_load.return_value = mock_plan_file result = runner.invoke(app, ["execute"]) assert result.exit_code != 0 + assert "already has PRs" in result.output @patch("pr_split.cli.load_plan") @patch("pr_split.cli.plan_exists", return_value=True) @@ -599,6 +600,187 @@ def test_clean_no_plan(self, mock_pe: MagicMock) -> None: assert result.exit_code == 0 +class TestExecuteRetriesAfterFailedPush: + """Branches without PRs mean a failed push; execute must retry, not refuse. + + Forcing 'clean' + re-plan would delete the plan and require paying for + LLM planning again after a transient network failure. + """ + + @patch("pr_split.cli.typer.confirm", return_value=True) + @patch("pr_split.cli.save_plan") + @patch("pr_split.cli._push_and_create_prs", return_value=[]) + @patch("pr_split.cli._create_branches_and_commits", return_value=[]) + @patch("pr_split.cli.commit_exists", return_value=True) + @patch("pr_split.cli.parse_diff") + @patch("pr_split.cli.is_worktree_clean", return_value=True) + @patch("pr_split.cli.check_gh_auth", return_value=True) + @patch("pr_split.cli.branch_exists", return_value=True) + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_branches_without_prs_is_retried( + self, + mock_pe: MagicMock, + mock_load: MagicMock, + mock_be: MagicMock, + mock_auth: MagicMock, + mock_clean: MagicMock, + mock_parse: MagicMock, + mock_commit: MagicMock, + mock_create: MagicMock, + mock_push: MagicMock, + mock_save: MagicMock, + mock_confirm: MagicMock, + ) -> None: + from pr_split.constants import Priority + from pr_split.schemas import BranchRecord, GitState, PlanFile, SplitPlan + + plan_file = PlanFile( + plan=SplitPlan( + dev_branch="feature-branch", + base_branch="main", + max_loc=400, + priority=Priority.ORTHOGONAL, + merge_base_sha="0123456789abcdef", + raw_diff="some diff", + groups=[_group("pr-1", "feat: auth", files=["a.py"])], + ), + git_state=GitState( + branches=[ + BranchRecord( + group_id="pr-1", + branch_name="pr-split/feature-branch/pr-1", + base_branch="main", + commit_sha="abc123", + ) + ] + ), + ) + mock_load.return_value = plan_file + + result = runner.invoke(app, ["execute"]) + + assert result.exit_code == 0 + assert "Recreating them and retrying" in result.output.replace("\n", " ") + mock_create.assert_called_once() + mock_push.assert_called_once() + mock_save.assert_called_once() + + @staticmethod + def _plan_with_stale_pr2() -> object: + from pr_split.constants import Priority + from pr_split.schemas import BranchRecord, GitState, PlanFile, SplitPlan + + return PlanFile( + plan=SplitPlan( + dev_branch="feature-branch", + base_branch="main", + max_loc=400, + priority=Priority.ORTHOGONAL, + merge_base_sha="0123456789abcdef", + raw_diff="some diff", + groups=[_group("pr-1", "feat: auth", files=["a.py"])], + ), + git_state=GitState( + branches=[ + BranchRecord( + group_id="pr-1", + branch_name="pr-split/feature-branch/pr-1", + base_branch="main", + commit_sha="abc123", + ), + # pr-2 was merged into pr-1 by an edit between the runs. + BranchRecord( + group_id="pr-2", + branch_name="pr-split/feature-branch/pr-2", + base_branch="main", + commit_sha="def456", + ), + ] + ), + ) + + @patch("pr_split.cli.run_git") + @patch("pr_split.cli.delete_branch") + @patch("pr_split.cli.typer.confirm", return_value=True) + @patch("pr_split.cli.save_plan") + @patch("pr_split.cli._push_and_create_prs", return_value=[]) + @patch("pr_split.cli._create_branches_and_commits", return_value=[]) + @patch("pr_split.cli.commit_exists", return_value=True) + @patch("pr_split.cli.parse_diff") + @patch("pr_split.cli.is_worktree_clean", return_value=True) + @patch("pr_split.cli.check_gh_auth", return_value=True) + @patch("pr_split.cli.branch_exists", return_value=True) + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_branches_dropped_from_an_edited_plan_are_deleted( + self, + mock_pe: MagicMock, + mock_load: MagicMock, + mock_be: MagicMock, + mock_auth: MagicMock, + mock_clean: MagicMock, + mock_parse: MagicMock, + mock_commit: MagicMock, + mock_create: MagicMock, + mock_push: MagicMock, + mock_save: MagicMock, + mock_confirm: MagicMock, + mock_delete: MagicMock, + mock_rg: MagicMock, + ) -> None: + """A retry must not orphan branches the edited plan no longer contains.""" + mock_load.return_value = self._plan_with_stale_pr2() + + result = runner.invoke(app, ["execute"]) + + assert result.exit_code == 0 + mock_delete.assert_called_once_with("pr-split/feature-branch/pr-2") + mock_rg.assert_any_call( + "ls-remote", "--exit-code", "origin", "refs/heads/pr-split/feature-branch/pr-2" + ) + mock_rg.assert_any_call("push", "origin", "--delete", "pr-split/feature-branch/pr-2") + + @patch("pr_split.cli.run_git") + @patch("pr_split.cli.delete_branch") + @patch("pr_split.cli.typer.confirm", return_value=True) + @patch("pr_split.cli.save_plan") + @patch("pr_split.cli._push_and_create_prs", return_value=[]) + @patch("pr_split.cli._create_branches_and_commits", return_value=[]) + @patch("pr_split.cli.commit_exists", return_value=True) + @patch("pr_split.cli.parse_diff") + @patch("pr_split.cli.is_worktree_clean", return_value=True) + @patch("pr_split.cli.check_gh_auth", return_value=True) + @patch("pr_split.cli.branch_exists") + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_remote_only_stale_branch_is_still_deleted_on_origin( + self, + mock_pe: MagicMock, + mock_load: MagicMock, + mock_be: MagicMock, + mock_auth: MagicMock, + mock_clean: MagicMock, + mock_parse: MagicMock, + mock_commit: MagicMock, + mock_create: MagicMock, + mock_push: MagicMock, + mock_save: MagicMock, + mock_confirm: MagicMock, + mock_delete: MagicMock, + mock_rg: MagicMock, + ) -> None: + """A stale branch pruned locally must still be removed from origin.""" + mock_be.side_effect = lambda name: name != "pr-split/feature-branch/pr-2" + mock_load.return_value = self._plan_with_stale_pr2() + + result = runner.invoke(app, ["execute"]) + + assert result.exit_code == 0 + mock_delete.assert_not_called() + mock_rg.assert_any_call("push", "origin", "--delete", "pr-split/feature-branch/pr-2") + + class TestPRCreationErrorIsACleanMessage: """A failed push/PR creation must end with a red message, not a traceback.""" From 3ba90352cddc596a9ba7b65c5bdb75ad1f6d17d9 Mon Sep 17 00:00:00 2001 From: vitali87 Date: Sun, 30 Aug 2026 13:50:19 +0100 Subject: [PATCH 03/14] fix: preserve CRLF line endings in reconstructed files git diff and git show were run with text=True, whose universal-newline translation strips the CR from CRLF files, and the worktree write used the platform newline. A one-line change to a CRLF file therefore became a whole-file line-ending rewrite in every sub-PR. Capture git output as bytes and decode it, and write reconstructed files with newline="". --- pr_split/cli.py | 3 +- pr_split/diff_ops/parser.py | 8 ++-- pr_split/diff_ops/reconstructor.py | 7 ++-- tests/test_cli_helpers.py | 25 ++++++++++++ tests/test_diff_parser_extended.py | 20 ++++++++-- tests/test_reconstructor.py | 62 ++++++++++++++++++++++++++++-- 6 files changed, 111 insertions(+), 14 deletions(-) diff --git a/pr_split/cli.py b/pr_split/cli.py index 2770ca9..1d33689 100644 --- a/pr_split/cli.py +++ b/pr_split/cli.py @@ -225,7 +225,8 @@ def _create_single_branch_and_commit( p = Path(worktree_path) / file_path if content is not None: p.parent.mkdir(parents=True, exist_ok=True) - p.write_text(content, encoding="utf-8") + # newline="" keeps CRLF from the reconstructed content intact. + p.write_text(content, encoding="utf-8", newline="") elif p.exists(): p.unlink() diff --git a/pr_split/diff_ops/parser.py b/pr_split/diff_ops/parser.py index b2a4574..adb020e 100644 --- a/pr_split/diff_ops/parser.py +++ b/pr_split/diff_ops/parser.py @@ -34,14 +34,16 @@ def extract_diff(dev_branch: str, base_branch: str) -> str: logger.info(logs.EXTRACTING_DIFF.format(base=base_branch, dev=dev_branch)) + # Capture bytes: text mode applies universal-newline translation, which + # turns CRLF into LF and would make every sub-PR rewrite the file's + # line endings. result = subprocess.run( ["git", "diff", *DIFF_ARGS, f"{base_branch}...{dev_branch}"], capture_output=True, - text=True, ) if result.returncode != 0: - raise GitOperationError(result.stderr.strip()) - return result.stdout + raise GitOperationError(result.stderr.decode("utf-8", errors="replace").strip()) + return result.stdout.decode("utf-8") def parse_diff(raw_diff: str) -> ParsedDiff: diff --git a/pr_split/diff_ops/reconstructor.py b/pr_split/diff_ops/reconstructor.py index 74d57a4..1ea7a6b 100644 --- a/pr_split/diff_ops/reconstructor.py +++ b/pr_split/diff_ops/reconstructor.py @@ -60,14 +60,15 @@ def merge_chain_assignments( def _get_base_file_content(file_path: str, ref: str) -> str: + # Bytes, not text mode: universal newlines would strip the CR from CRLF + # files and the reconstructed file would come out with LF endings. result = subprocess.run( ["git", "show", f"{ref}:{file_path}"], capture_output=True, - text=True, ) if result.returncode != 0: - raise GitOperationError(result.stderr.strip()) - return result.stdout + raise GitOperationError(result.stderr.decode("utf-8", errors="replace").strip()) + return result.stdout.decode("utf-8") NO_NEWLINE_MARKER = "\\" diff --git a/tests/test_cli_helpers.py b/tests/test_cli_helpers.py index 01b6d8b..2da1f9b 100644 --- a/tests/test_cli_helpers.py +++ b/tests/test_cli_helpers.py @@ -2,6 +2,7 @@ import contextlib import threading +from pathlib import Path from unittest.mock import MagicMock, patch import pytest @@ -574,3 +575,27 @@ def push(branch: str) -> None: with pytest.raises(PRSplitError): _push_and_create_prs(groups, records) assert mock_create.call_count == 0 + + +class TestWorktreeWritesPreserveCrlf: + @patch("pr_split.cli.commit_files_in_dir", return_value="sha1") + @patch("pr_split.cli.materialize_group_files", return_value={"c.txt": "a\r\nb\r\n"}) + @patch("pr_split.cli.remove_worktree") + @patch("pr_split.cli.add_worktree") + def test_crlf_content_written_verbatim( + self, + mock_add: MagicMock, + mock_remove: MagicMock, + mock_mat: MagicMock, + mock_commit: MagicMock, + ) -> None: + written: dict[str, bytes] = {} + + def capture(cwd: str, file_paths: list[str], message: str, **kwargs: object) -> str: + for file_path in file_paths: + written[file_path] = (Path(cwd) / file_path).read_bytes() + return "sha1" + + mock_commit.side_effect = capture + _create_branches_and_commits([_group("pr-1", "t")], MagicMock(), "main", "sha", "ns") + assert written["c.txt"] == b"a\r\nb\r\n" diff --git a/tests/test_diff_parser_extended.py b/tests/test_diff_parser_extended.py index 7745048..77d9aba 100644 --- a/tests/test_diff_parser_extended.py +++ b/tests/test_diff_parser_extended.py @@ -97,7 +97,7 @@ class TestExtractDiffSubprocess: @patch("pr_split.diff_ops.parser.subprocess.run") def test_extract_diff_success(self, mock_run: MagicMock) -> None: mock_run.return_value = subprocess.CompletedProcess( - args=["git", "diff"], returncode=0, stdout=EXTRACT_DIFF_SAMPLE, stderr="" + args=["git", "diff"], returncode=0, stdout=EXTRACT_DIFF_SAMPLE.encode(), stderr=b"" ) result = extract_diff("feature", "main") assert result == EXTRACT_DIFF_SAMPLE @@ -115,7 +115,6 @@ def test_extract_diff_success(self, mock_run: MagicMock) -> None: "main...feature", ], capture_output=True, - text=True, ) @patch("pr_split.diff_ops.parser.subprocess.run") @@ -123,8 +122,8 @@ def test_extract_diff_failure(self, mock_run: MagicMock) -> None: mock_run.return_value = subprocess.CompletedProcess( args=["git", "diff"], returncode=1, - stdout="", - stderr="fatal: bad revision", + stdout=b"", + stderr=b"fatal: bad revision", ) with pytest.raises(GitOperationError, match="bad revision"): extract_diff("bad-branch", "main") @@ -134,3 +133,16 @@ class TestRawDiffPreserved: def test_raw_diff_preserved(self) -> None: parsed = parse_diff(EXTRACT_DIFF_SAMPLE) assert parsed.raw_diff == EXTRACT_DIFF_SAMPLE + + +class TestExtractDiffPreservesCarriageReturns: + @patch("pr_split.diff_ops.parser.subprocess.run") + def test_crlf_diff_is_returned_verbatim(self, mock_run: MagicMock) -> None: + raw = b"--- a/c.txt\n+++ b/c.txt\n@@ -1 +1 @@\n-old\r\n+new\r\n" + mock_run.return_value = subprocess.CompletedProcess( + args=["git", "diff"], returncode=0, stdout=raw, stderr=b"" + ) + result = extract_diff("feature", "main") + assert "\r\n" in result + assert result == raw.decode() + assert mock_run.call_args.kwargs.get("text") is not True diff --git a/tests/test_reconstructor.py b/tests/test_reconstructor.py index 71b4480..f8fe9e2 100644 --- a/tests/test_reconstructor.py +++ b/tests/test_reconstructor.py @@ -115,13 +115,13 @@ def test_preserves_untouched_lines(self) -> None: class TestGetBaseFileContent: @patch("pr_split.diff_ops.reconstructor.subprocess.run") def test_success(self, mock_run: MagicMock) -> None: - mock_run.return_value = MagicMock(returncode=0, stdout="file content\n", stderr="") + mock_run.return_value = MagicMock(returncode=0, stdout=b"file content\n", stderr=b"") result = _get_base_file_content("foo.py", "abc123") assert result == "file content\n" @patch("pr_split.diff_ops.reconstructor.subprocess.run") def test_failure_raises(self, mock_run: MagicMock) -> None: - mock_run.return_value = MagicMock(returncode=1, stdout="", stderr="not found") + mock_run.return_value = MagicMock(returncode=1, stdout=b"", stderr=b"not found") with pytest.raises(GitOperationError): _get_base_file_content("missing.py", "abc123") @@ -183,7 +183,7 @@ def test_file_not_in_diff_skipped(self) -> None: class TestGetBaseFileContentExtended: @patch("pr_split.diff_ops.reconstructor.subprocess.run") def test_empty_file_returns_empty(self, mock_run: MagicMock) -> None: - mock_run.return_value = MagicMock(returncode=0, stdout="", stderr="") + mock_run.return_value = MagicMock(returncode=0, stdout=b"", stderr=b"") result = _get_base_file_content("empty.py", "abc123") assert result == "" @@ -479,3 +479,59 @@ def test_form_feed_in_earlier_line_does_not_shift_hunk(self) -> None: diff = "--- a/f.txt\n+++ b/f.txt\n@@ -7,5 +7,5 @@\n h\n i\n j\n-k\n+K\n l\n" pf = PatchSet(diff)[0] assert apply_hunks(base, pf, [0]) == dev + + +class TestCrlfPreserved: + @patch("pr_split.diff_ops.reconstructor.subprocess.run") + def test_base_content_keeps_crlf(self, mock_run: MagicMock) -> None: + mock_run.return_value = MagicMock(returncode=0, stdout=b"a\r\nb\r\n", stderr=b"") + assert _get_base_file_content("c.txt", "main") == "a\r\nb\r\n" + + @patch("pr_split.diff_ops.reconstructor.subprocess.run") + def test_partial_change_to_crlf_file_keeps_endings(self, mock_run: MagicMock) -> None: + base = "".join(f"line{i}\r\n" for i in range(1, 8)) + mock_run.return_value = MagicMock(returncode=0, stdout=base.encode(), stderr=b"") + diff = ( + "diff --git a/c.txt b/c.txt\n--- a/c.txt\n+++ b/c.txt\n" + "@@ -1,6 +1,6 @@\n line1\r\n line2\r\n-line3\r\n+LINE3\r\n" + " line4\r\n line5\r\n line6\r\n" + ) + parsed = parse_diff(diff) + group = Group( + id="pr-1", + title="t", + description="d", + assignments=[ + GroupAssignment( + file_path="c.txt", + assignment_type=AssignmentType.WHOLE_FILE, + hunk_indices=[0], + ) + ], + ) + result = materialize_group_files(parsed, group, "main") + assert result["c.txt"] == base.replace("line3\r\n", "LINE3\r\n") + + @patch("pr_split.diff_ops.reconstructor.subprocess.run") + def test_bare_carriage_return_does_not_shift_hunks(self, mock_run: MagicMock) -> None: + # A lone CR is not a line break for git; with base content no longer + # newline-translated, splitting on it would misplace every later hunk. + mock_run.return_value = MagicMock(returncode=0, stdout=b"x\ry\nz\n", stderr=b"") + diff = ( + "diff --git a/c.txt b/c.txt\n--- a/c.txt\n+++ b/c.txt\n" + "@@ -1,2 +1,2 @@\n x\ry\n-z\n+Z\n" + ) + parsed = parse_diff(diff) + group = Group( + id="pr-1", + title="t", + description="d", + assignments=[ + GroupAssignment( + file_path="c.txt", + assignment_type=AssignmentType.WHOLE_FILE, + hunk_indices=[0], + ) + ], + ) + assert materialize_group_files(parsed, group, "main")["c.txt"] == "x\ry\nZ\n" From 03957070a2c64ccf67bc7a18dd74887d4f35f7e2 Mon Sep 17 00:00:00 2001 From: vitali87 Date: Sun, 30 Aug 2026 14:15:03 +0100 Subject: [PATCH 04/14] test: assert newline='' on worktree writes and cover a bare CR base file --- tests/test_cli_helpers.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/test_cli_helpers.py b/tests/test_cli_helpers.py index 2da1f9b..3bc2047 100644 --- a/tests/test_cli_helpers.py +++ b/tests/test_cli_helpers.py @@ -597,5 +597,10 @@ def capture(cwd: str, file_paths: list[str], message: str, **kwargs: object) -> return "sha1" mock_commit.side_effect = capture - _create_branches_and_commits([_group("pr-1", "t")], MagicMock(), "main", "sha", "ns") + # On POSIX write_text only translates "\n" (a no-op), so also assert the + # newline="" argument that keeps CRLF intact on Windows. + with patch.object(Path, "write_text", autospec=True, side_effect=Path.write_text) as wt: + _create_branches_and_commits([_group("pr-1", "t")], MagicMock(), "main", "sha", "ns") assert written["c.txt"] == b"a\r\nb\r\n" + content_writes = [c for c in wt.call_args_list if c.args[1] == "a\r\nb\r\n"] + assert content_writes and all(c.kwargs.get("newline") == "" for c in content_writes) From 5d2f16b70d8f648fad0a7385cc2a2242a891d9b5 Mon Sep 17 00:00:00 2001 From: vitali87 Date: Sun, 30 Aug 2026 14:51:53 +0100 Subject: [PATCH 05/14] fix: keep non-ASCII paths unescaped in the extracted diff git quotes and octal-escapes non-ASCII paths by default (core.quotePath). unidiff then fails on new/deleted files ('Target without source') and, for modified files, records the literal quoted string as the path so materialization asks git for a path that does not exist. Run git diff with core.quotePath=false. --- pr_split/diff_ops/parser.py | 5 ++++- tests/test_diff_parser_extended.py | 35 ++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/pr_split/diff_ops/parser.py b/pr_split/diff_ops/parser.py index adb020e..7ef94ba 100644 --- a/pr_split/diff_ops/parser.py +++ b/pr_split/diff_ops/parser.py @@ -38,7 +38,10 @@ def extract_diff(dev_branch: str, base_branch: str) -> str: # turns CRLF into LF and would make every sub-PR rewrite the file's # line endings. result = subprocess.run( - ["git", "diff", *DIFF_ARGS, f"{base_branch}...{dev_branch}"], + # core.quotePath=false: otherwise git octal-escapes and quotes any + # non-ASCII path ("b/caf\303\251.txt"), which unidiff cannot parse for + # new/deleted files and mis-reports as a literal quoted path otherwise. + ["git", "-c", "core.quotePath=false", "diff", *DIFF_ARGS, f"{base_branch}...{dev_branch}"], capture_output=True, ) if result.returncode != 0: diff --git a/tests/test_diff_parser_extended.py b/tests/test_diff_parser_extended.py index 77d9aba..f797a0f 100644 --- a/tests/test_diff_parser_extended.py +++ b/tests/test_diff_parser_extended.py @@ -1,6 +1,8 @@ from __future__ import annotations +import os import subprocess +from pathlib import Path from unittest.mock import MagicMock, patch import pytest @@ -104,6 +106,8 @@ def test_extract_diff_success(self, mock_run: MagicMock) -> None: mock_run.assert_called_once_with( [ "git", + "-c", + "core.quotePath=false", "diff", "--no-color", "--no-ext-diff", @@ -146,3 +150,34 @@ def test_crlf_diff_is_returned_verbatim(self, mock_run: MagicMock) -> None: assert "\r\n" in result assert result == raw.decode() assert mock_run.call_args.kwargs.get("text") is not True + + +class TestNonAsciiPaths: + def test_unicode_paths_survive_extraction_and_parsing(self, tmp_path: Path) -> None: + def git(*args: str) -> str: + return subprocess.run( + ["git", "-c", "user.name=t", "-c", "user.email=t@x", *args], + cwd=tmp_path, + capture_output=True, + text=True, + check=True, + ).stdout + + git("init", "-q", "-b", "main") + (tmp_path / "ünï.txt").write_text("one\n", encoding="utf-8") + git("add", "-A") + git("commit", "-qm", "base") + git("checkout", "-qb", "dev") + (tmp_path / "ünï.txt").write_text("two\n", encoding="utf-8") + (tmp_path / "new file ü.txt").write_text("x\n", encoding="utf-8") + git("add", "-A") + git("commit", "-qm", "dev") + + cwd = os.getcwd() + os.chdir(tmp_path) + try: + parsed = parse_diff(extract_diff("dev", "main")) + finally: + os.chdir(cwd) + + assert sorted(pf.path for pf in parsed.patch_set) == ["new file ü.txt", "ünï.txt"] From fde97b3baa6fd591dd6dd2577e0791dc8b0e1cc7 Mon Sep 17 00:00:00 2001 From: vitali87 Date: Sun, 30 Aug 2026 15:19:41 +0100 Subject: [PATCH 06/14] fix: decode C-quoted paths from git diff so files are not written under a quoted name --- pr_split/diff_ops/parser.py | 45 +++++++++++++++++++ tests/test_diff_parser_extended.py | 72 +++++++++++++++++++++++++++++- 2 files changed, 116 insertions(+), 1 deletion(-) diff --git a/pr_split/diff_ops/parser.py b/pr_split/diff_ops/parser.py index 7ef94ba..282977a 100644 --- a/pr_split/diff_ops/parser.py +++ b/pr_split/diff_ops/parser.py @@ -1,5 +1,6 @@ from __future__ import annotations +import re import subprocess from dataclasses import dataclass from functools import cached_property @@ -49,11 +50,55 @@ def extract_diff(dev_branch: str, base_branch: str) -> str: return result.stdout.decode("utf-8") +_C_ESCAPES = { + "a": b"\a", + "b": b"\b", + "f": b"\f", + "n": b"\n", + "r": b"\r", + "t": b"\t", + "v": b"\v", + "\\": b"\\", + '"': b'"', +} +_C_ESCAPE_RE = re.compile(r"\\([0-7]{1,3}|.)", re.DOTALL) + + +def unquote_git_path(path: str) -> str: + """Decode a path git printed in its C-quoted form, e.g. `"a/we\\"ird.py"`. + + Even with core.quotePath=false git quotes any path containing `"`, `\\` + or a control character. unidiff keeps the quotes and escapes verbatim, so + without this the a/ b/ prefix is never stripped and the file would be + written under a literally quoted name. + """ + if len(path) < 2 or path[0] != '"' or path[-1] != '"': + return path + out = bytearray() + pos = 0 + body = path[1:-1] + for match in _C_ESCAPE_RE.finditer(body): + out += body[pos : match.start()].encode("utf-8") + escape = match.group(1) + if escape[0] in "01234567": + # Octal escapes are single bytes; consecutive ones form one + # multi-byte UTF-8 character, which the final decode reassembles. + out.append(int(escape, 8)) + else: + out += _C_ESCAPES.get(escape, escape.encode("utf-8")) + pos = match.end() + out += body[pos:].encode("utf-8") + return bytes(out).decode("utf-8", errors="surrogateescape") + + def parse_diff(raw_diff: str) -> ParsedDiff: try: patch_set = PatchSet(raw_diff) except Exception as exc: raise DiffParseError(str(exc)) from exc + for patch_file in patch_set: + patch_file.source_file = unquote_git_path(patch_file.source_file) + patch_file.target_file = unquote_git_path(patch_file.target_file) return ParsedDiff(patch_set=patch_set, raw_diff=raw_diff) diff --git a/tests/test_diff_parser_extended.py b/tests/test_diff_parser_extended.py index f797a0f..4036697 100644 --- a/tests/test_diff_parser_extended.py +++ b/tests/test_diff_parser_extended.py @@ -7,7 +7,7 @@ import pytest -from pr_split.diff_ops.parser import extract_diff, parse_diff +from pr_split.diff_ops.parser import extract_diff, parse_diff, unquote_git_path from pr_split.exceptions import GitOperationError SAMPLE_DIFF = ( @@ -181,3 +181,73 @@ def git(*args: str) -> str: os.chdir(cwd) assert sorted(pf.path for pf in parsed.patch_set) == ["new file ü.txt", "ünï.txt"] + + +class TestUnquoteGitPath: + @pytest.mark.parametrize( + ("quoted", "expected"), + [ + ("plain/path.py", "plain/path.py"), + ('"a/we\\"ird.py"', 'a/we"ird.py'), + ('"b/tab\\tname.py"', "b/tab\tname.py"), + ('"b/back\\\\slash.py"', "b/back\\slash.py"), + ('"b/caf\\303\\251.txt"', "b/café.txt"), + ('"b/bell\\a\\1.txt"', "b/bell\a\x01.txt"), + ('""', ""), + ('"', '"'), + ], + ) + def test_decodes_c_style_quoting(self, quoted: str, expected: str) -> None: + assert unquote_git_path(quoted) == expected + + def test_quoted_headers_are_unquoted_after_parse(self) -> None: + raw = ( + 'diff --git "a/we\\"ird.py" "b/we\\"ird.py"\n' + "new file mode 100644\n" + "--- /dev/null\n" + '+++ "b/we\\"ird.py"\n' + "@@ -0,0 +1 @@\n" + "+x\n" + 'diff --git "a/tab\\tname.py" "b/tab\\tname.py"\n' + "deleted file mode 100644\n" + '--- "a/tab\\tname.py"\n' + "+++ /dev/null\n" + "@@ -1 +0,0 @@\n" + "-y\n" + ) + parsed = parse_diff(raw) + assert [pf.path for pf in parsed.patch_set] == ['we"ird.py', "tab\tname.py"] + assert parsed.patch_set[0].source_file == "/dev/null" + assert parsed.patch_set[1].target_file == "/dev/null" + assert parsed.patch_set[1].is_removed_file + + def test_special_paths_survive_real_git_diff(self, tmp_path: Path) -> None: + def git(*args: str) -> str: + return subprocess.run( + ["git", "-c", "user.name=t", "-c", "user.email=t@x", *args], + cwd=tmp_path, + capture_output=True, + text=True, + check=True, + ).stdout + + git("init", "-q", "-b", "main") + (tmp_path / "keep.txt").write_text("one\n", encoding="utf-8") + git("add", "-A") + git("commit", "-qm", "base") + git("checkout", "-qb", "dev") + names = ['we"ird.py', "tab\tname.py", "back\\slash.py"] + for name in names: + (tmp_path / name).write_text("x\n", encoding="utf-8") + git("add", "-A") + git("commit", "-qm", "dev") + + cwd = os.getcwd() + os.chdir(tmp_path) + try: + parsed = parse_diff(extract_diff("dev", "main")) + finally: + os.chdir(cwd) + + assert sorted(pf.path for pf in parsed.patch_set) == sorted(names) + assert all(pf.is_added_file for pf in parsed.patch_set) From e1accdcc65975158a4383c39cf562c2284a998d2 Mon Sep 17 00:00:00 2001 From: vitali87 Date: Sun, 30 Aug 2026 15:42:45 +0100 Subject: [PATCH 07/14] fix: parse C-quoted paths that contain spaces without phantom files or parse errors --- pr_split/diff_ops/parser.py | 36 +++++++++- tests/test_diff_parser_extended.py | 106 ++++++++++++++++++++++++++++- 2 files changed, 140 insertions(+), 2 deletions(-) diff --git a/pr_split/diff_ops/parser.py b/pr_split/diff_ops/parser.py index 282977a..1368d41 100644 --- a/pr_split/diff_ops/parser.py +++ b/pr_split/diff_ops/parser.py @@ -91,9 +91,43 @@ def unquote_git_path(path: str) -> str: return bytes(out).decode("utf-8", errors="surrogateescape") +_QUOTED_PATH_RE = re.compile(r'"(?:[^"\\]|\\.)*"') +_HEADER_PREFIXES = ("diff --git ", "--- ", "+++ ") + + +def _escape_spaces_in_quoted_paths(line: str) -> str: + return _QUOTED_PATH_RE.sub(lambda m: m.group(0).replace(" ", "\\040"), line) + + +def _normalize_quoted_headers(raw_diff: str) -> str: + """Make C-quoted paths containing spaces parseable by unidiff. + + unidiff splits `diff --git ` on the last space, so a quoted + path with a space (`"a/my \\"notes\\".md"`) is cut in the wrong place and + no longer matches the `---`/`+++` lines: unidiff then keeps a phantom + 0-hunk file for a modification and raises "Target without source" for an + addition or deletion. Spaces inside quoted paths are rewritten as the + octal escape `\\040` -- still valid C-quoting, which unquote_git_path + decodes -- on the header lines only (between `diff --git` and the first + `@@`), so hunk content is never touched. + """ + if '"' not in raw_diff: + return raw_diff + lines = raw_diff.split("\n") + in_header = False + for i, line in enumerate(lines): + if line.startswith("diff --git "): + in_header = True + elif line.startswith("@@"): + in_header = False + if in_header and '"' in line and line.startswith(_HEADER_PREFIXES): + lines[i] = _escape_spaces_in_quoted_paths(line) + return "\n".join(lines) + + def parse_diff(raw_diff: str) -> ParsedDiff: try: - patch_set = PatchSet(raw_diff) + patch_set = PatchSet(_normalize_quoted_headers(raw_diff)) except Exception as exc: raise DiffParseError(str(exc)) from exc for patch_file in patch_set: diff --git a/tests/test_diff_parser_extended.py b/tests/test_diff_parser_extended.py index 4036697..c8316e5 100644 --- a/tests/test_diff_parser_extended.py +++ b/tests/test_diff_parser_extended.py @@ -7,7 +7,12 @@ import pytest -from pr_split.diff_ops.parser import extract_diff, parse_diff, unquote_git_path +from pr_split.diff_ops.parser import ( + _normalize_quoted_headers, + extract_diff, + parse_diff, + unquote_git_path, +) from pr_split.exceptions import GitOperationError SAMPLE_DIFF = ( @@ -251,3 +256,102 @@ def git(*args: str) -> str: assert sorted(pf.path for pf in parsed.patch_set) == sorted(names) assert all(pf.is_added_file for pf in parsed.patch_set) + + +class TestQuotedPathsWithSpaces: + def test_header_spaces_are_escaped_but_hunk_content_is_not(self) -> None: + raw = ( + 'diff --git "a/my \\"notes\\".md" "b/my \\"notes\\".md"\n' + "index 1..2 100644\n" + '--- "a/my \\"notes\\".md"\t\n' + '+++ "b/my \\"notes\\".md"\t\n' + "@@ -1 +1 @@\n" + '-a "quoted line" here\n' + '+--- "not a header" line\n' + ) + normalized = _normalize_quoted_headers(raw) + assert normalized.splitlines()[0] == ( + 'diff --git "a/my\\040\\"notes\\".md" "b/my\\040\\"notes\\".md"' + ) + assert normalized.splitlines()[2] == '--- "a/my\\040\\"notes\\".md"\t' + assert normalized.splitlines()[5:] == raw.splitlines()[5:] + + def test_diff_without_quotes_is_returned_unchanged(self) -> None: + raw = ( + "diff --git a/plain name.md b/plain name.md\n" + "--- a/plain name.md\n" + "+++ b/plain name.md\n" + ) + assert _normalize_quoted_headers(raw) is raw + + def test_modified_quoted_path_with_space_parses_to_one_file(self) -> None: + raw = ( + 'diff --git "a/my \\"notes\\".md" "b/my \\"notes\\".md"\n' + "index 1..2 100644\n" + '--- "a/my \\"notes\\".md"\n' + '+++ "b/my \\"notes\\".md"\n' + "@@ -1 +1 @@\n" + "-a\n" + "+b\n" + ) + parsed = parse_diff(raw) + assert [(pf.path, len(pf)) for pf in parsed.patch_set] == [('my "notes".md', 1)] + assert parsed.stats["total_files"] == 1 + + def test_added_and_deleted_quoted_paths_with_spaces_parse(self) -> None: + raw = ( + 'diff --git "a/new \\"file\\".txt" "b/new \\"file\\".txt"\n' + "new file mode 100644\n" + "--- /dev/null\n" + '+++ "b/new \\"file\\".txt"\n' + "@@ -0,0 +1 @@\n" + "+x\n" + 'diff --git "a/old \\"file\\".txt" "b/old \\"file\\".txt"\n' + "deleted file mode 100644\n" + '--- "a/old \\"file\\".txt"\n' + "+++ /dev/null\n" + "@@ -1 +0,0 @@\n" + "-y\n" + ) + parsed = parse_diff(raw) + assert [pf.path for pf in parsed.patch_set] == ['new "file".txt', 'old "file".txt'] + assert parsed.patch_set[0].is_added_file + assert parsed.patch_set[1].is_removed_file + + def test_real_git_diff_with_quoted_spaced_paths(self, tmp_path: Path) -> None: + def git(*args: str) -> str: + return subprocess.run( + ["git", "-c", "user.name=t", "-c", "user.email=t@x", *args], + cwd=tmp_path, + capture_output=True, + text=True, + check=True, + ).stdout + + git("init", "-q", "-b", "main") + modified = 'my "notes".md' + deleted = 'old "file".txt' + (tmp_path / modified).write_text("a\n", encoding="utf-8") + (tmp_path / deleted).write_text("y\n", encoding="utf-8") + (tmp_path / "plain name.md").write_text("p\n", encoding="utf-8") + git("add", "-A") + git("commit", "-qm", "base") + git("checkout", "-qb", "dev") + (tmp_path / modified).write_text("b\n", encoding="utf-8") + (tmp_path / deleted).unlink() + (tmp_path / "plain name.md").write_text("q\n", encoding="utf-8") + (tmp_path / 'new "file".txt').write_text("x\n", encoding="utf-8") + git("add", "-A") + git("commit", "-qm", "dev") + + cwd = os.getcwd() + os.chdir(tmp_path) + try: + parsed = parse_diff(extract_diff("dev", "main")) + finally: + os.chdir(cwd) + + assert sorted((pf.path, len(pf)) for pf in parsed.patch_set) == sorted( + [(modified, 1), (deleted, 1), ("plain name.md", 1), ('new "file".txt', 1)] + ) + assert parsed.stats["total_files"] == 4 From 494f537cc5a534bc25837ad3b7b8096a8b46d1ea Mon Sep 17 00:00:00 2001 From: vitali87 Date: Sun, 30 Aug 2026 18:16:10 +0100 Subject: [PATCH 08/14] fix: round-trip non-UTF-8 text files instead of crashing with UnicodeDecodeError --- pr_split/cli.py | 2 +- pr_split/diff_ops/parser.py | 5 ++- pr_split/diff_ops/reconstructor.py | 2 +- pr_split/plan_store.py | 9 +++-- pr_split/planner/client.py | 12 +++++++ tests/test_client.py | 21 ++++++++++++ tests/test_diff_parser_extended.py | 55 ++++++++++++++++++++++++++++++ tests/test_plan_store.py | 29 ++++++++++++++++ 8 files changed, 130 insertions(+), 5 deletions(-) diff --git a/pr_split/cli.py b/pr_split/cli.py index 1d33689..3b2e2c3 100644 --- a/pr_split/cli.py +++ b/pr_split/cli.py @@ -226,7 +226,7 @@ def _create_single_branch_and_commit( if content is not None: p.parent.mkdir(parents=True, exist_ok=True) # newline="" keeps CRLF from the reconstructed content intact. - p.write_text(content, encoding="utf-8", newline="") + p.write_text(content, encoding="utf-8", errors="surrogateescape", newline="") elif p.exists(): p.unlink() diff --git a/pr_split/diff_ops/parser.py b/pr_split/diff_ops/parser.py index 1368d41..a54ec75 100644 --- a/pr_split/diff_ops/parser.py +++ b/pr_split/diff_ops/parser.py @@ -47,7 +47,10 @@ def extract_diff(dev_branch: str, base_branch: str) -> str: ) if result.returncode != 0: raise GitOperationError(result.stderr.decode("utf-8", errors="replace").strip()) - return result.stdout.decode("utf-8") + # Git diffs any NUL-free file as text, so legacy latin-1 sources reach us + # too; surrogateescape keeps their bytes intact so they round-trip when + # the worker writes them back with the same error handler. + return result.stdout.decode("utf-8", errors="surrogateescape") _C_ESCAPES = { diff --git a/pr_split/diff_ops/reconstructor.py b/pr_split/diff_ops/reconstructor.py index 1ea7a6b..e163905 100644 --- a/pr_split/diff_ops/reconstructor.py +++ b/pr_split/diff_ops/reconstructor.py @@ -68,7 +68,7 @@ def _get_base_file_content(file_path: str, ref: str) -> str: ) if result.returncode != 0: raise GitOperationError(result.stderr.decode("utf-8", errors="replace").strip()) - return result.stdout.decode("utf-8") + return result.stdout.decode("utf-8", errors="surrogateescape") NO_NEWLINE_MARKER = "\\" diff --git a/pr_split/plan_store.py b/pr_split/plan_store.py index f7bfd99..4f3e587 100644 --- a/pr_split/plan_store.py +++ b/pr_split/plan_store.py @@ -1,3 +1,4 @@ +import json from pathlib import Path from loguru import logger @@ -12,7 +13,11 @@ def save_plan(plan_file: PlanFile) -> None: path = Path(PLAN_DIR) path.mkdir(parents=True, exist_ok=True) plan_path = Path(PLAN_FILE) - plan_path.write_text(plan_file.model_dump_json(indent=2)) + # The raw diff may carry surrogate-escaped bytes from non-UTF-8 files; + # json.dumps escapes those as \udcXX and loads them back losslessly, + # which pydantic's own JSON writer refuses to do. + payload = json.dumps(plan_file.model_dump(mode="json"), indent=2, ensure_ascii=True) + plan_path.write_text(payload, encoding="utf-8") logger.info(logs.SAVING_PLAN.format(path=plan_path)) @@ -20,7 +25,7 @@ def load_plan() -> PlanFile: plan_path = Path(PLAN_FILE) if not plan_path.exists(): raise PRSplitError(ErrorMsg.NO_PLAN()) - plan_file = PlanFile.model_validate_json(plan_path.read_text()) + plan_file = PlanFile.model_validate(json.loads(plan_path.read_text(encoding="utf-8"))) logger.info(logs.PLAN_LOADED.format(count=len(plan_file.plan.groups), path=plan_path)) return plan_file diff --git a/pr_split/planner/client.py b/pr_split/planner/client.py index 0c933c8..258ed3e 100644 --- a/pr_split/planner/client.py +++ b/pr_split/planner/client.py @@ -107,7 +107,18 @@ def _count_tokens_openai(texts: list[str], *, model: str) -> int: return sum(len(enc.encode(t)) for t in texts) +def _utf8_safe(text: str) -> str: + """Replace surrogate-escaped bytes so the text can be sent as JSON. + + Diff text keeps undecodable bytes as surrogates so files round-trip on + disk; the HTTP clients encode request bodies as strict UTF-8, so the + prompt copy gets U+FFFD instead. + """ + return text.encode("utf-8", errors="surrogateescape").decode("utf-8", errors="replace") + + def _count_tokens(system: str, user: str, *, settings: Settings) -> int: + system, user = _utf8_safe(system), _utf8_safe(user) match settings.provider: case Provider.ANTHROPIC: return _count_tokens_anthropic(system, user, settings=settings) @@ -169,6 +180,7 @@ def _call_openai(system: str, user: str, *, settings: Settings) -> RawToolOutput def _call_llm(system: str, user: str, *, settings: Settings) -> RawToolOutput: + system, user = _utf8_safe(system), _utf8_safe(user) match settings.provider: case Provider.ANTHROPIC: return _call_anthropic(system, user, settings=settings) diff --git a/tests/test_client.py b/tests/test_client.py index 1bdfffa..fade584 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -986,3 +986,24 @@ def test_large_diff_uses_chunking( result = _plan_split_with_llm(parsed, settings) assert len(result) == 1 mock_chunked.assert_called_once() + + +class TestSurrogateSafePrompts: + @patch("pr_split.planner.client._call_anthropic") + def test_call_llm_replaces_surrogates_before_the_request(self, mock_call: MagicMock) -> None: + from pr_split.planner.client import _call_llm + + user = b"diff caf\xe9".decode("utf-8", errors="surrogateescape") + _call_llm("sys", user, settings=_make_settings()) + sent_user = mock_call.call_args.args[1] + assert "\udce9" not in sent_user + assert sent_user == "diff caf\ufffd" + sent_user.encode("utf-8") # must be JSON-serialisable + + @patch("pr_split.planner.client._count_tokens_anthropic", return_value=3) + def test_count_tokens_replaces_surrogates(self, mock_count: MagicMock) -> None: + from pr_split.planner.client import _count_tokens + + user = b"caf\xe9".decode("utf-8", errors="surrogateescape") + assert _count_tokens("sys", user, settings=_make_settings()) == 3 + mock_count.call_args.args[1].encode("utf-8") diff --git a/tests/test_diff_parser_extended.py b/tests/test_diff_parser_extended.py index c8316e5..ffc0ae0 100644 --- a/tests/test_diff_parser_extended.py +++ b/tests/test_diff_parser_extended.py @@ -355,3 +355,58 @@ def git(*args: str) -> str: [(modified, 1), (deleted, 1), ("plain name.md", 1), ('new "file".txt', 1)] ) assert parsed.stats["total_files"] == 4 + + +class TestNonUtf8FileContent: + def test_latin1_file_round_trips_byte_for_byte(self, tmp_path: Path) -> None: + from pr_split.constants import AssignmentType + from pr_split.diff_ops.reconstructor import materialize_group_files + from pr_split.git_ops.branches import merge_base + from pr_split.schemas import Group, GroupAssignment + + def git(*args: str) -> str: + return subprocess.run( + ["git", "-c", "user.name=t", "-c", "user.email=t@x", *args], + cwd=tmp_path, + capture_output=True, + text=True, + check=True, + ).stdout + + git("init", "-q", "-b", "main") + base_bytes = "caf\xe9 one\nkeep\n".encode("latin-1") + dev_bytes = "caf\xe9 two\nkeep\n".encode("latin-1") + (tmp_path / "legacy.txt").write_bytes(base_bytes) + git("add", "-A") + git("commit", "-qm", "base") + git("checkout", "-qb", "dev") + (tmp_path / "legacy.txt").write_bytes(dev_bytes) + git("add", "-A") + git("commit", "-qm", "dev") + + cwd = os.getcwd() + os.chdir(tmp_path) + try: + parsed = parse_diff(extract_diff("dev", "main")) + group = Group( + id="pr-1", + title="t", + description="d", + assignments=[ + GroupAssignment( + file_path="legacy.txt", + assignment_type=AssignmentType.WHOLE_FILE, + hunk_indices=[0], + ) + ], + ) + materialized = materialize_group_files(parsed, group, merge_base("main", "dev")) + out = tmp_path / "out.txt" + content = materialized["legacy.txt"] + assert content is not None + out.write_text(content, encoding="utf-8", errors="surrogateescape", newline="") + finally: + os.chdir(cwd) + + assert [pf.path for pf in parsed.patch_set] == ["legacy.txt"] + assert out.read_bytes() == dev_bytes diff --git a/tests/test_plan_store.py b/tests/test_plan_store.py index 98dd939..41df2d5 100644 --- a/tests/test_plan_store.py +++ b/tests/test_plan_store.py @@ -106,3 +106,32 @@ def test_saved_file_is_valid_json( assert raw["plan"]["priority"] == "logical" assert raw["plan"]["min_loc"] == 25 assert raw["plan"]["strict_loc_bounds"] is True + + +class TestPlanStoreSurrogates: + def test_raw_diff_with_undecodable_bytes_round_trips( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + from pr_split.constants import Priority + from pr_split.plan_store import load_plan, save_plan + from pr_split.schemas import PlanFile, SplitPlan + + monkeypatch.chdir(tmp_path) + raw = b"--- a/x\n+++ b/x\n@@ -1 +1 @@\n-caf\xe9\n+caf\xe9!\n".decode( + "utf-8", errors="surrogateescape" + ) + plan = SplitPlan( + dev_branch="dev", + base_branch="main", + max_loc=400, + priority=Priority.ORTHOGONAL, + raw_diff=raw, + ) + save_plan(PlanFile(plan=plan)) + + loaded = load_plan() + + assert loaded.plan.raw_diff == raw + assert loaded.plan.raw_diff.encode("utf-8", errors="surrogateescape").endswith( + b"+caf\xe9!\n" + ) From fa37abffba41dec57b9c009ba981daad33db43c5 Mon Sep 17 00:00:00 2001 From: vitali87 Date: Sun, 30 Aug 2026 20:03:25 +0100 Subject: [PATCH 09/14] fix: report an Anthropic token-count failure as an LLM error instead of an SDK traceback --- pr_split/exceptions.py | 1 + pr_split/planner/client.py | 18 ++++++++++++------ tests/test_client.py | 11 +++++++++++ 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/pr_split/exceptions.py b/pr_split/exceptions.py index d7d9c2d..8c97956 100644 --- a/pr_split/exceptions.py +++ b/pr_split/exceptions.py @@ -22,6 +22,7 @@ class ErrorMsg(StrEnum): MERGE_CONFLICT = "Groups '{a}' and '{b}' modify overlapping regions in '{file}'" NO_PLAN = "No split plan found; run 'pr-split split' first" LLM_PARSE_ERROR = "Failed to parse LLM response: {detail}" + LLM_TOKEN_COUNT_FAILED = "Could not count prompt tokens with the LLM API: {detail}" BRANCH_CREATE_FAILED = "Failed to create branch '{branch}': {detail}" PR_CREATE_FAILED = "Failed to create PR for group '{group}': {detail}" MERGE_FAILED = "Merge of '{source}' into '{target}' failed: {detail}" diff --git a/pr_split/planner/client.py b/pr_split/planner/client.py index 258ed3e..532e70c 100644 --- a/pr_split/planner/client.py +++ b/pr_split/planner/client.py @@ -90,12 +90,18 @@ def _extract_raw_output(block_input: dict[str, object]) -> list[_RawGroup]: def _count_tokens_anthropic(system: str, user: str, *, settings: Settings) -> int: client = anthropic.Anthropic(api_key=settings.api_key) - response = client.messages.count_tokens( - model=settings.model, - system=system, - messages=[{"role": "user", "content": user}], - tools=[_ANTHROPIC_TOOL_DEF], - ) + # This is the first API call of a run, so a bad key, a network blip or a + # rate limit surfaces here; it must be an LLMError like every other + # provider failure rather than an SDK traceback. + try: + response = client.messages.count_tokens( + model=settings.model, + system=system, + messages=[{"role": "user", "content": user}], + tools=[_ANTHROPIC_TOOL_DEF], + ) + except anthropic.APIError as exc: + raise LLMError(ErrorMsg.LLM_TOKEN_COUNT_FAILED(detail=str(exc))) from exc return response.input_tokens diff --git a/tests/test_client.py b/tests/test_client.py index fade584..3b7c210 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -4,6 +4,7 @@ from types import SimpleNamespace from unittest.mock import MagicMock, patch +import anthropic import pytest from pr_split.config import Settings @@ -547,6 +548,16 @@ def test_returns_input_tokens(self, mock_cls: MagicMock) -> None: assert result == 42 mock_client.messages.count_tokens.assert_called_once() + @patch("pr_split.planner.client.anthropic.Anthropic") + def test_api_failure_is_an_llm_error(self, mock_cls: MagicMock) -> None: + mock_client = mock_cls.return_value + mock_client.messages.count_tokens.side_effect = anthropic.APIConnectionError( + request=MagicMock() + ) + settings = _make_settings(Provider.ANTHROPIC) + with pytest.raises(LLMError, match="Could not count prompt tokens"): + _count_tokens_anthropic("sys", "usr", settings=settings) + # --------------------------------------------------------------------------- # _count_tokens_openai From 3f7810789e036a8c4295135872b58fa6dedc6a4b Mon Sep 17 00:00:00 2001 From: vitali87 Date: Thu, 24 Sep 2026 20:22:55 +0100 Subject: [PATCH 10/14] fix: resume execute past groups whose PRs already exist, and retry transient push errors A push that failed with a transient server error left some groups with PRs and the rest without, and execute then refused to run at all, so the only way on was clean (closing good PRs) and a full re-run. push_branch now retries known transient server errors, and execute keeps the groups that already have PRs (their branches untouched) and creates only the rest. --- pr_split/cli.py | 50 +++++++++++++++++++---- pr_split/git_ops/branches.py | 29 +++++++++++++- pr_split/logs.py | 1 + tests/test_cli_coverage.py | 78 ++++++++++++++++++++++++++++++++++++ tests/test_cli_helpers.py | 24 +++++++++++ tests/test_git_branches.py | 34 ++++++++++++++++ 6 files changed, 207 insertions(+), 9 deletions(-) diff --git a/pr_split/cli.py b/pr_split/cli.py index d543c4d..f66df2d 100644 --- a/pr_split/cli.py +++ b/pr_split/cli.py @@ -283,7 +283,15 @@ def _create_branches_and_commits( *, author: str | None = None, stacked: bool = False, + keep: dict[str, BranchRecord] | None = None, ) -> list[BranchRecord]: + """Create a branch and commit per group; groups in ``keep`` reuse their record. + + ``keep`` holds groups whose PRs already exist from an earlier, partly + failed run: their branches are left as they are (recreating them would + rewrite the open PR's head). + """ + kept = keep or {} worktree_base = Path(tempfile.mkdtemp(prefix="pr-split-worktrees-")) if stacked: @@ -298,9 +306,10 @@ def _create_branches_and_commits( batches = iter([[(group, base_branch, merge_base_ref) for group in groups]]) try: - results: dict[str, BranchRecord] = {} + results: dict[str, BranchRecord] = dict(kept) errors: list[tuple[str, Exception]] = [] for batch_args in batches: + batch_args = [args for args in batch_args if args[0].id not in kept] with ThreadPoolExecutor(max_workers=_WORKTREE_MAX_WORKERS) as executor: future_to_group_id = { executor.submit( @@ -327,7 +336,9 @@ def _create_branches_and_commits( break if errors: - for record in results.values(): + for gid, record in results.items(): + if gid in kept: + continue try: delete_branch(record.branch_name) except PRSplitError as exc: @@ -428,7 +439,10 @@ def _push_and_create_prs( branch_records: list[BranchRecord], *, draft: bool = False, + existing_prs: dict[str, PRRecord] | None = None, ) -> list[PRRecord]: + """Push each group's branch and open its PR; groups in ``existing_prs`` are done.""" + done = existing_prs or {} record_map = {r.group_id: r for r in branch_records} errors: list[tuple[str, Exception]] = [] @@ -437,8 +451,9 @@ def _push_and_create_prs( push_futures = { executor.submit(push_branch, record_map[group.id].branch_name): group.id for group in groups + if group.id not in done } - pushed: set[str] = set() + pushed: set[str] = set(done) for future in as_completed(push_futures): group_id = push_futures[future] try: @@ -473,9 +488,9 @@ def _base_pushed(group: Group) -> bool: _create_single_pr, group, record_map[group.id], groups, draft=draft ): group.id for group in groups - if group.id in pushed and _base_pushed(group) + if group.id not in done and group.id in pushed and _base_pushed(group) } - results: dict[str, PRRecord] = {} + results: dict[str, PRRecord] = dict(done) for future in as_completed(future_to_group_id): group_id = future_to_group_id[future] try: @@ -1043,10 +1058,26 @@ def execute( if draft and not plan.draft: plan = plan.model_copy(update={"draft": True}) - if plan_file.git_state.prs: + existing_prs = {r.group_id: r for r in plan_file.git_state.prs} + kept_branches = { + r.group_id: r for r in plan_file.git_state.branches if r.group_id in existing_prs + } + group_ids = {g.id for g in plan.groups} + if existing_prs and ( + set(existing_prs) >= group_ids + or not set(existing_prs) <= group_ids + or set(kept_branches) != set(existing_prs) + ): + # Every group already has a PR, or the recorded PRs no longer match + # the plan's groups: there is nothing safe to resume. console.print("[red]This plan already has PRs. Use 'pr-split clean' first.[/red]") raise typer.Exit(1) - if plan_file.git_state.branches: + if existing_prs: + console.print( + f"[yellow]A previous run opened {len(existing_prs)} of {len(plan.groups)} PR(s)." + " Keeping those and creating the rest.[/yellow]" + ) + elif plan_file.git_state.branches: # A previous execute created branches but no PRs - a failed push or PR # creation. Branch creation is idempotent (add_worktree recreates an # existing branch), so retry instead of forcing 'clean' + a full @@ -1109,12 +1140,15 @@ def execute( namespace, author=plan.author, stacked=plan.stacked, + keep=kept_branches, ) except PRSplitError as exc: console.print(f"[red]{exc}[/red]") raise typer.Exit(1) from exc try: - pr_records = _push_and_create_prs(plan.groups, branch_records, draft=plan.draft) + pr_records = _push_and_create_prs( + plan.groups, branch_records, draft=plan.draft, existing_prs=existing_prs + ) except PRCreationError as exc: save_plan( PlanFile( diff --git a/pr_split/git_ops/branches.py b/pr_split/git_ops/branches.py index e7bc096..170b62e 100644 --- a/pr_split/git_ops/branches.py +++ b/pr_split/git_ops/branches.py @@ -2,6 +2,7 @@ import re import subprocess +import time from loguru import logger @@ -74,9 +75,35 @@ def commit_files(file_paths: list[str], message: str, *, author: str | None = No return run_git("rev-parse", "HEAD") +# Server-side failures GitHub reports for a push that can succeed on retry. +_TRANSIENT_PUSH_ERRORS = ( + "fatal error in commit_refs", + "the remote end hung up unexpectedly", + "internal server error", + "http 500", + "http 502", + "http 503", + "connection reset", + "operation timed out", +) +_PUSH_ATTEMPTS = 3 +_PUSH_RETRY_DELAY = 2.0 + + def push_branch(branch: str) -> None: logger.info(logs.PUSHING_BRANCH.format(branch=branch)) - run_git("push", "--force-with-lease", "-u", "origin", branch) + for attempt in range(1, _PUSH_ATTEMPTS + 1): + try: + run_git("push", "--force-with-lease", "-u", "origin", branch) + return + except GitOperationError as exc: + transient = any(marker in str(exc).lower() for marker in _TRANSIENT_PUSH_ERRORS) + if not transient or attempt == _PUSH_ATTEMPTS: + raise + logger.warning( + logs.PUSH_RETRY.format(branch=branch, attempt=attempt, error=str(exc).strip()) + ) + time.sleep(_PUSH_RETRY_DELAY * attempt) def delete_branch(branch: str, *, remote: bool = False) -> None: diff --git a/pr_split/logs.py b/pr_split/logs.py index ec302ce..139fa9c 100644 --- a/pr_split/logs.py +++ b/pr_split/logs.py @@ -69,3 +69,4 @@ PR_SKIPPED_BASE_NOT_PUSHED = ( "Skipping PR for group '{group}': its base branch '{base}' was not pushed" ) +PUSH_RETRY = "Push of {branch} failed (attempt {attempt}): {error}; retrying" diff --git a/tests/test_cli_coverage.py b/tests/test_cli_coverage.py index 5db3818..6097778 100644 --- a/tests/test_cli_coverage.py +++ b/tests/test_cli_coverage.py @@ -666,6 +666,84 @@ def test_branches_without_prs_is_retried( mock_push.assert_called_once() mock_save.assert_called_once() + @staticmethod + def _plan_with_one_of_two_prs(pr_groups: list[str]) -> object: + from pr_split.constants import Priority + from pr_split.schemas import BranchRecord, GitState, PlanFile, PRRecord, SplitPlan + + return PlanFile( + plan=SplitPlan( + dev_branch="feature-branch", + base_branch="main", + max_loc=400, + priority=Priority.ORTHOGONAL, + merge_base_sha="0123456789abcdef", + raw_diff="some diff", + groups=[ + _group("pr-1", "feat: a", files=["a.py"]), + _group("pr-2", "feat: b", files=["b.py"]), + ], + ), + git_state=GitState( + branches=[ + BranchRecord( + group_id=gid, + branch_name=f"pr-split/feature-branch/{gid}", + base_branch="main", + commit_sha="abc123", + ) + for gid in ("pr-1", "pr-2") + ], + prs=[PRRecord(group_id=gid, pr_number=10, pr_url="u") for gid in pr_groups], + ), + ) + + @patch("pr_split.cli.typer.confirm", return_value=True) + @patch("pr_split.cli.save_plan") + @patch("pr_split.cli._push_and_create_prs", return_value=[]) + @patch("pr_split.cli._create_branches_and_commits", return_value=[]) + @patch("pr_split.cli.commit_exists", return_value=True) + @patch("pr_split.cli.parse_diff") + @patch("pr_split.cli.validate_coverage") + @patch("pr_split.cli.is_worktree_clean", return_value=True) + @patch("pr_split.cli.check_gh_auth", return_value=True) + @patch("pr_split.cli.branch_exists", return_value=True) + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_partial_prs_are_kept_and_the_rest_created( + self, + mock_pe: MagicMock, + mock_load: MagicMock, + mock_be: MagicMock, + mock_auth: MagicMock, + mock_clean: MagicMock, + mock_validate: MagicMock, + mock_parse: MagicMock, + mock_commit: MagicMock, + mock_create: MagicMock, + mock_push: MagicMock, + mock_save: MagicMock, + mock_confirm: MagicMock, + ) -> None: + mock_load.return_value = self._plan_with_one_of_two_prs(["pr-1"]) + + result = runner.invoke(app, ["execute"]) + + assert result.exit_code == 0, result.output + assert "opened 1 of 2 PR(s)" in result.output.replace("\n", " ") + assert set(mock_create.call_args.kwargs["keep"]) == {"pr-1"} + assert set(mock_push.call_args.kwargs["existing_prs"]) == {"pr-1"} + + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_every_group_with_a_pr_is_refused( + self, mock_pe: MagicMock, mock_load: MagicMock + ) -> None: + mock_load.return_value = self._plan_with_one_of_two_prs(["pr-1", "pr-2"]) + result = runner.invoke(app, ["execute"]) + assert result.exit_code == 1 + assert "already has PRs" in result.output + @staticmethod def _plan_with_stale_pr2() -> object: from pr_split.constants import Priority diff --git a/tests/test_cli_helpers.py b/tests/test_cli_helpers.py index 01b6d8b..0470095 100644 --- a/tests/test_cli_helpers.py +++ b/tests/test_cli_helpers.py @@ -178,6 +178,30 @@ def test_returns_records_for_all_groups( assert result[0].group_id == "pr-1" assert result[1].group_id == "pr-2" + @patch("pr_split.cli.create_pr", return_value=(2, "https://github.com/pr/2")) + @patch("pr_split.cli.push_branch") + def test_groups_with_existing_prs_are_not_pushed_or_reopened( + self, mock_push: MagicMock, mock_create: MagicMock + ) -> None: + groups = [_group("pr-1", "feat: a"), _group("pr-2", "feat: b", ["pr-1"])] + records = [ + _branch_record("pr-1", "pr-split/ns/pr-1"), + BranchRecord( + group_id="pr-2", + branch_name="pr-split/ns/pr-2", + base_branch="pr-split/ns/pr-1", + commit_sha="abc123", + ), + ] + existing = PRRecord(group_id="pr-1", pr_number=1, pr_url="https://github.com/pr/1") + + result = _push_and_create_prs(groups, records, existing_prs={"pr-1": existing}) + + assert [call.args[0] for call in mock_push.call_args_list] == ["pr-split/ns/pr-2"] + mock_create.assert_called_once() + assert result[0] is existing + assert result[1].pr_number == 2 + @patch("pr_split.cli.create_pr", return_value=(1, "https://github.com/pr/1")) @patch("pr_split.cli.push_branch") def test_pushes_all_branches(self, mock_push: MagicMock, mock_create: MagicMock) -> None: diff --git a/tests/test_git_branches.py b/tests/test_git_branches.py index 7cc4793..99953c4 100644 --- a/tests/test_git_branches.py +++ b/tests/test_git_branches.py @@ -118,6 +118,40 @@ def test_calls_push(self, mock_git: MagicMock) -> None: ) +class TestPushBranchRetries: + @patch("pr_split.git_ops.branches.time.sleep") + @patch("pr_split.git_ops.branches.run_git") + def test_transient_server_error_is_retried( + self, mock_git: MagicMock, mock_sleep: MagicMock + ) -> None: + mock_git.side_effect = [ + GitOperationError("remote: fatal error in commit_refs"), + "", + ] + push_branch("pr-split/pr-5") + assert mock_git.call_count == 2 + mock_sleep.assert_called_once() + + @patch("pr_split.git_ops.branches.time.sleep") + @patch("pr_split.git_ops.branches.run_git") + def test_rejection_is_not_retried(self, mock_git: MagicMock, mock_sleep: MagicMock) -> None: + mock_git.side_effect = GitOperationError("! [rejected] (stale info)") + with pytest.raises(GitOperationError, match="stale info"): + push_branch("pr-split/pr-5") + assert mock_git.call_count == 1 + mock_sleep.assert_not_called() + + @patch("pr_split.git_ops.branches.time.sleep") + @patch("pr_split.git_ops.branches.run_git") + def test_gives_up_after_the_last_attempt( + self, mock_git: MagicMock, mock_sleep: MagicMock + ) -> None: + mock_git.side_effect = GitOperationError("fatal: the remote end hung up unexpectedly") + with pytest.raises(GitOperationError, match="hung up"): + push_branch("pr-split/pr-5") + assert mock_git.call_count == 3 + + class TestDeleteBranch: @patch("pr_split.git_ops.branches.run_git") def test_local_only(self, mock_git: MagicMock) -> None: From 744bcdb93799f816189bf1974b3fd917055b908e Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 26 Sep 2026 21:19:56 +0000 Subject: [PATCH 11/14] fix: log execute's completion once; the main merge doubled the line Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015rnEEjXWDBdePRhKMmZr1w --- pr_split/cli.py | 1 - 1 file changed, 1 deletion(-) diff --git a/pr_split/cli.py b/pr_split/cli.py index 181bac0..94b9b9a 100644 --- a/pr_split/cli.py +++ b/pr_split/cli.py @@ -1503,7 +1503,6 @@ def execute( if plan.stacked: _link_stacks(PlanDAG(plan.groups), pr_records) logger.success(f"Execute complete: {len(plan.groups)} PRs created from saved plan") - logger.success(f"Execute complete: {len(plan.groups)} PRs created from saved plan") _AUTO_MERGE_POLL_INTERVAL = 10 From 467d8970cc629ff8763f5d886c4e19ad93b6b366 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 26 Sep 2026 22:24:01 +0000 Subject: [PATCH 12/14] test: patch plan validation in execute-retry tests that feed a mocked diff main's execute validates the plan against the parsed diff; these tests mock parse_diff, so the check would reject the plan before the retry logic runs. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015rnEEjXWDBdePRhKMmZr1w --- tests/test_cli_coverage.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/test_cli_coverage.py b/tests/test_cli_coverage.py index eeeccfb..d217cae 100644 --- a/tests/test_cli_coverage.py +++ b/tests/test_cli_coverage.py @@ -1030,7 +1030,8 @@ def test_branches_without_prs_is_retried( ) mock_load.return_value = plan_file - result = runner.invoke(app, ["execute"]) + with patch("pr_split.cli.validate_coverage"): + result = runner.invoke(app, ["execute"]) assert result.exit_code == 0 assert "Recreating them and retrying" in result.output.replace("\n", " ") @@ -1104,7 +1105,8 @@ def test_branches_dropped_from_an_edited_plan_are_deleted( """A retry must not orphan branches the edited plan no longer contains.""" mock_load.return_value = self._plan_with_stale_pr2() - result = runner.invoke(app, ["execute"]) + with patch("pr_split.cli.validate_coverage"): + result = runner.invoke(app, ["execute"]) assert result.exit_code == 0 mock_delete.assert_called_once_with("pr-split/feature-branch/pr-2") @@ -1146,7 +1148,8 @@ def test_remote_only_stale_branch_is_still_deleted_on_origin( mock_be.side_effect = lambda name: name != "pr-split/feature-branch/pr-2" mock_load.return_value = self._plan_with_stale_pr2() - result = runner.invoke(app, ["execute"]) + with patch("pr_split.cli.validate_coverage"): + result = runner.invoke(app, ["execute"]) assert result.exit_code == 0 mock_delete.assert_not_called() From 2d23ac0f2161d8087e11aa9e7421b8ed992e4ad9 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 26 Sep 2026 22:38:05 +0000 Subject: [PATCH 13/14] fix: keep a kept layer's parents on a stacked resume, and accept a push that landed - execute: when some layers already have PRs, a stacked resume now also keeps the recorded branches of those layers' ancestors. Rebuilding an ancestor gave it a new commit and force-pushed it, so the kept child's open PR (which targets it) showed the ancestor's changes again. The kept ancestors still get their PRs opened. - push_branch: after a transient error (hung-up remote, timeout) the push may have landed; the retry is then rejected as a stale lease. If the remote branch already holds the local head, the push counts as done. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015rnEEjXWDBdePRhKMmZr1w --- pr_split/cli.py | 14 +++++++--- pr_split/git_ops/branches.py | 15 +++++++++++ tests/test_cli_coverage.py | 48 ++++++++++++++++++++++++++++++++++ tests/test_git_branches.py | 50 ++++++++++++++++++++++++++++++++++++ 4 files changed, 124 insertions(+), 3 deletions(-) diff --git a/pr_split/cli.py b/pr_split/cli.py index 94b9b9a..ad13190 100644 --- a/pr_split/cli.py +++ b/pr_split/cli.py @@ -1384,9 +1384,8 @@ def execute( plan = plan.model_copy(update={"draft": True}) existing_prs = {r.group_id: r for r in plan_file.git_state.prs} - kept_branches = { - r.group_id: r for r in plan_file.git_state.branches if r.group_id in existing_prs - } + recorded = {r.group_id: r for r in plan_file.git_state.branches} + kept_branches = {gid: recorded[gid] for gid in existing_prs if gid in recorded} group_ids = {g.id for g in plan.groups} if existing_prs and ( set(existing_prs) >= group_ids @@ -1461,6 +1460,15 @@ def execute( console.print(f"[red]{exc}[/red]") raise typer.Exit(1) from exc + if plan.stacked: + # A kept layer's branch sits on its ancestors' current commits; rebuilding + # an ancestor would rewrite the base under the kept layer's open PR, which + # would then show the ancestor's changes again. Their PRs are still opened. + for gid in list(kept_branches): + for ancestor in dag.ancestors(gid): + if ancestor in recorded: + kept_branches.setdefault(ancestor, recorded[ancestor]) + _present_plan(plan.groups) typer.confirm("Proceed with creating branches and PRs?", abort=True) diff --git a/pr_split/git_ops/branches.py b/pr_split/git_ops/branches.py index 7cc57ff..b481518 100644 --- a/pr_split/git_ops/branches.py +++ b/pr_split/git_ops/branches.py @@ -70,16 +70,31 @@ def is_worktree_clean() -> bool: _PUSH_RETRY_DELAY = 2.0 +def _remote_has_local_head(branch: str) -> bool: + try: + remote = run_git("ls-remote", "origin", f"refs/heads/{branch}").split() + return bool(remote) and remote[0] == run_git("rev-parse", branch) + except GitOperationError: + return False + + def push_branch(branch: str) -> None: logger.info(logs.PUSHING_BRANCH.format(branch=branch)) + retried = False for attempt in range(1, _PUSH_ATTEMPTS + 1): try: run_git("push", "--force-with-lease", "-u", "origin", branch) return except GitOperationError as exc: transient = any(marker in str(exc).lower() for marker in _TRANSIENT_PUSH_ERRORS) + # A push reported as failed (hung-up remote, timeout) may still have + # landed; the retry is then rejected as a stale lease although the + # branch is already on the remote. + if retried and not transient and _remote_has_local_head(branch): + return if not transient or attempt == _PUSH_ATTEMPTS: raise + retried = True logger.warning( logs.PUSH_RETRY.format(branch=branch, attempt=attempt, error=str(exc).strip()) ) diff --git a/tests/test_cli_coverage.py b/tests/test_cli_coverage.py index c3b02f4..562a53d 100644 --- a/tests/test_cli_coverage.py +++ b/tests/test_cli_coverage.py @@ -1107,6 +1107,54 @@ def test_partial_prs_are_kept_and_the_rest_created( assert set(mock_create.call_args.kwargs["keep"]) == {"pr-1"} assert set(mock_push.call_args.kwargs["existing_prs"]) == {"pr-1"} + @pytest.mark.parametrize("stacked", [True, False]) + @patch("pr_split.cli._link_stacks") + @patch("pr_split.cli._require_gh_stack") + @patch("pr_split.cli.typer.confirm", return_value=True) + @patch("pr_split.cli.save_plan") + @patch("pr_split.cli._push_and_create_prs", return_value=[]) + @patch("pr_split.cli._create_branches_and_commits", return_value=[]) + @patch("pr_split.cli.commit_exists", return_value=True) + @patch("pr_split.cli.parse_diff") + @patch("pr_split.cli.validate_coverage") + @patch("pr_split.cli.is_worktree_clean", return_value=True) + @patch("pr_split.cli.check_gh_auth", return_value=True) + @patch("pr_split.cli.branch_exists", return_value=True) + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_stacked_resume_keeps_the_parents_of_a_layer_with_a_pr( + self, + mock_pe: MagicMock, + mock_load: MagicMock, + mock_be: MagicMock, + mock_auth: MagicMock, + mock_clean: MagicMock, + mock_validate: MagicMock, + mock_parse: MagicMock, + mock_commit: MagicMock, + mock_create: MagicMock, + mock_push: MagicMock, + mock_save: MagicMock, + mock_confirm: MagicMock, + mock_stack: MagicMock, + mock_link: MagicMock, + stacked: bool, + ) -> None: + """pr-2's PR opened but its parent pr-1's did not: rebuilding pr-1 would + rewrite the base under pr-2's open PR, so a stacked resume keeps it.""" + plan_file = self._plan_with_one_of_two_prs(["pr-2"]) + plan_file.plan.groups[1].depends_on = ["pr-1"] + plan_file.plan.stacked = stacked + mock_load.return_value = plan_file + + result = runner.invoke(app, ["execute"]) + + assert result.exit_code == 0, result.output + kept = set(mock_create.call_args.kwargs["keep"]) + assert kept == ({"pr-1", "pr-2"} if stacked else {"pr-2"}) + # pr-1 still gets its PR opened. + assert set(mock_push.call_args.kwargs["existing_prs"]) == {"pr-2"} + @patch("pr_split.cli.load_plan") @patch("pr_split.cli.plan_exists", return_value=True) def test_every_group_with_a_pr_is_refused( diff --git a/tests/test_git_branches.py b/tests/test_git_branches.py index 3ef71f4..abea01f 100644 --- a/tests/test_git_branches.py +++ b/tests/test_git_branches.py @@ -121,6 +121,56 @@ def test_gives_up_after_the_last_attempt( assert mock_git.call_count == 3 +class TestPushThatLandedDespiteAnError: + @patch("pr_split.git_ops.branches.time.sleep") + @patch("pr_split.git_ops.branches.run_git") + def test_stale_lease_after_a_landed_push_is_success( + self, mock_git: MagicMock, mock_sleep: MagicMock + ) -> None: + def git(*args: str) -> str: + if args[0] == "push": + if git.pushes == 0: + git.pushes += 1 + raise GitOperationError("fatal: the remote end hung up unexpectedly") + raise GitOperationError("! [rejected] pr-split/pr-5 (stale info)") + if args[0] == "ls-remote": + return "abc123\trefs/heads/pr-split/pr-5" + return "abc123" # rev-parse + + git.pushes = 0 # type: ignore[attr-defined] + mock_git.side_effect = git + push_branch("pr-split/pr-5") + + @patch("pr_split.git_ops.branches.time.sleep") + @patch("pr_split.git_ops.branches.run_git") + def test_stale_lease_is_still_an_error_when_the_remote_differs( + self, mock_git: MagicMock, mock_sleep: MagicMock + ) -> None: + def git(*args: str) -> str: + if args[0] == "push": + if git.pushes == 0: + git.pushes += 1 + raise GitOperationError("fatal: the remote end hung up unexpectedly") + raise GitOperationError("! [rejected] pr-split/pr-5 (stale info)") + if args[0] == "ls-remote": + return "fff999\trefs/heads/pr-split/pr-5" + return "abc123" + + git.pushes = 0 # type: ignore[attr-defined] + mock_git.side_effect = git + with pytest.raises(GitOperationError, match="stale info"): + push_branch("pr-split/pr-5") + + @patch("pr_split.git_ops.branches.run_git") + def test_a_first_attempt_rejection_does_not_consult_the_remote( + self, mock_git: MagicMock + ) -> None: + mock_git.side_effect = GitOperationError("! [rejected] (stale info)") + with pytest.raises(GitOperationError): + push_branch("pr-split/pr-5") + assert [c.args[0] for c in mock_git.call_args_list] == ["push"] + + class TestDeleteBranch: @patch("pr_split.git_ops.branches.run_git") def test_local_only(self, mock_git: MagicMock) -> None: From e8d00e157ddbce21622e10c308827188c4a02de5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 08:45:21 +0000 Subject: [PATCH 14/14] Keep a resumed layer's ancestors whenever the plan has dependencies Since execute lays out any plan with dependency edges along its DAG, stacked or not, an unstacked child also sits on its parent's branch; rebuilding the parent on resume would rewrite the base under the child's open PR. Keep recorded ancestors whenever the plan has dependencies. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015rnEEjXWDBdePRhKMmZr1w --- pr_split/cli.py | 9 +++++---- tests/test_cli_coverage.py | 5 +++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/pr_split/cli.py b/pr_split/cli.py index 354674c..ad690a4 100644 --- a/pr_split/cli.py +++ b/pr_split/cli.py @@ -1972,10 +1972,11 @@ def execute( console.print(f"[red]{exc}[/red]") raise typer.Exit(1) from exc - if plan.stacked: - # A kept layer's branch sits on its ancestors' current commits; rebuilding - # an ancestor would rewrite the base under the kept layer's open PR, which - # would then show the ancestor's changes again. Their PRs are still opened. + # Any plan with dependency edges is laid out along its DAG (stacked or not), + # so a kept layer's branch sits on its ancestors' current commits; rebuilding + # an ancestor would rewrite the base under the kept layer's open PR, which + # would then show the ancestor's changes again. Their PRs are still opened. + if plan.stacked or any(g.depends_on for g in plan.groups): for gid in list(kept_branches): for ancestor in dag.ancestors(gid): if ancestor in recorded: diff --git a/tests/test_cli_coverage.py b/tests/test_cli_coverage.py index b5fd62c..cb5bfe0 100644 --- a/tests/test_cli_coverage.py +++ b/tests/test_cli_coverage.py @@ -1143,7 +1143,8 @@ def test_stacked_resume_keeps_the_parents_of_a_layer_with_a_pr( stacked: bool, ) -> None: """pr-2's PR opened but its parent pr-1's did not: rebuilding pr-1 would - rewrite the base under pr-2's open PR, so a stacked resume keeps it.""" + rewrite the base under pr-2's open PR, so the resume keeps it. A plan + with dependencies is laid out along its DAG with or without --stack.""" plan_file = self._plan_with_one_of_two_prs(["pr-2"]) plan_file.plan.groups[1].depends_on = ["pr-1"] plan_file.plan.stacked = stacked @@ -1153,7 +1154,7 @@ def test_stacked_resume_keeps_the_parents_of_a_layer_with_a_pr( assert result.exit_code == 0, result.output kept = set(mock_create.call_args.kwargs["keep"]) - assert kept == ({"pr-1", "pr-2"} if stacked else {"pr-2"}) + assert kept == {"pr-1", "pr-2"} # pr-1 still gets its PR opened. assert set(mock_push.call_args.kwargs["existing_prs"]) == {"pr-2"}