From 27458634ae201c8d35fa4f825c180fa2871da46a Mon Sep 17 00:00:00 2001 From: vitali87 Date: Sun, 30 Aug 2026 19:18:34 +0100 Subject: [PATCH] fix: retarget a stacked child PR at the plan base before merging it --- README.md | 4 +- pr_split/cli.py | 13 ++++- pr_split/git_ops/prs.py | 20 ++++++++ pr_split/logs.py | 4 ++ tests/test_cli_new_features.py | 94 ++++++++++++++++++++++++++++++++++ tests/test_git_prs.py | 27 ++++++++++ 6 files changed, 159 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 3b61675..b254398 100644 --- a/README.md +++ b/README.md @@ -92,7 +92,7 @@ pr-split split feature-branch --base main --dry-run pr-split split feature-branch --base main --stack ``` -Without `--stack`, every sub-PR branch is cut from the merge base and targets the base branch, so a sub-PR that depends on code from another group only goes green once its dependency merges. With `--stack`, each dependent group's branch is cut from its parent group's branch and carries the parent's hunks for shared files, and its PR targets the parent's branch. Every PR shows only its own diff, compiles standalone, and GitHub retargets children automatically as parents merge. +Without `--stack`, every sub-PR branch is cut from the merge base and targets the base branch, so a sub-PR that depends on code from another group only goes green once its dependency merges. With `--stack`, each dependent group's branch is cut from its parent group's branch and carries the parent's hunks for shared files, and its PR targets the parent's branch. Every PR shows only its own diff and compiles standalone. When `pr-split merge` reaches a stacked child it retargets the PR at the base branch (`gh pr edit --base`) right before merging it, since GitHub only does that itself for native stacks or when the parent's head branch is deleted (which `gh pr merge --auto` skips). Linear chains in the plan are also registered as [native GitHub stacks](https://github.blog/changelog/2026-07-30-stacked-pull-requests-are-now-in-public-preview/) via the [`gh-stack` extension](https://github.com/github/gh-stack) (`gh extension install github/gh-stack`). If the extension is missing the linking step is skipped with a warning — the PRs are already correctly chained without it. Groups that depend on more than one group target the base branch directly, since native stacks are strictly linear; their branch carries every ancestor's changes so it still builds standalone, and those extra changes drop out of the diff as the ancestor PRs merge. @@ -110,7 +110,7 @@ Shows a table with each sub-PR's ID, title, branch, PR number, live state (OPEN/ pr-split merge ``` -Walks the dependency DAG and merges each PR in topological order. Skips already-merged, closed, draft, review-required, or changes-requested PRs, and every PR whose dependency was not merged in this run (independent subtrees still proceed). Stops if a merge fails. Exits 1 whenever a merge failed, any PR was left blocked, or a PR's state could not be fetched from GitHub (check `gh auth status`), so re-run once the cause is resolved. +Walks the dependency DAG and merges each PR in topological order. For plans created with `split --stack` (or `execute --stack`), each child PR is retargeted at the base branch right before it is merged (native-stack PRs are left to GitHub). Skips already-merged, closed, draft, review-required, or changes-requested PRs, and every PR whose dependency was not merged in this run (independent subtrees still proceed). Stops if a merge fails. Exits 1 whenever a merge failed, any PR was left blocked, or a PR's state could not be fetched from GitHub (check `gh auth status`), so re-run once the cause is resolved. Use `--auto` to queue merges behind CI checks (uses `gh pr merge --auto`): diff --git a/pr_split/cli.py b/pr_split/cli.py index 501a7b9..ec892db 100644 --- a/pr_split/cli.py +++ b/pr_split/cli.py @@ -60,7 +60,7 @@ remove_worktree, ) from .git_ops.branches import run_git -from .git_ops.prs import close_pr, create_pr, get_pr_state, link_stack, merge_pr +from .git_ops.prs import close_pr, create_pr, get_pr_state, link_stack, merge_pr, retarget_pr from .graph import PlanDAG from .plan_store import load_plan, plan_exists, save_plan from .planner import plan_split, validate_coverage, validate_plan @@ -1138,6 +1138,10 @@ def merge_all( plan = plan_file.plan git_state = plan_file.git_state pr_map = {r.group_id: r for r in git_state.prs} + # Stacked children were opened against their parent's split branch. + stacked_children = { + r.group_id for r in git_state.branches if r.base_branch != plan.base_branch + } if not pr_map: console.print("[yellow]No PRs found in plan. Nothing to merge.[/yellow]") @@ -1215,6 +1219,13 @@ def merge_all( continue try: + if group_id in stacked_children: + # Its parent has merged by now (iter_ready + the guard + # above), but the PR still targets the parent's branch. + # Merging there -- which `--auto` does silently, since gh + # skips deleting the head branch in auto mode -- would + # never reach the base branch. + retarget_pr(pr_record.pr_number, plan.base_branch) merge_pr(pr_record.pr_number, auto=auto) if not auto: merged.append(group_id) diff --git a/pr_split/git_ops/prs.py b/pr_split/git_ops/prs.py index 9129796..eecc115 100644 --- a/pr_split/git_ops/prs.py +++ b/pr_split/git_ops/prs.py @@ -74,6 +74,26 @@ def merge_pr(pr_number: int, *, auto: bool = False) -> None: logger.info(f"{'Queued' if auto else 'Merged'} PR #{pr_number}") +_STACKED_PR_EDIT_REFUSED = "part of a stack" + + +def retarget_pr(pr_number: int, base: str) -> bool: + """Point a stacked child PR at ``base`` once its parent has merged. + + Returns False when GitHub refuses because the PR is in a native stack; + GitHub retargets those itself when the parent PR merges. + """ + try: + _run_gh("pr", "edit", str(pr_number), "--base", base) + except GitOperationError as exc: + if _STACKED_PR_EDIT_REFUSED in str(exc).lower(): + logger.info(logs.PR_RETARGET_NATIVE_STACK.format(number=pr_number)) + return False + raise + logger.info(logs.PR_RETARGETED.format(number=pr_number, base=base)) + return True + + def close_pr(pr_number: int) -> None: _run_gh("pr", "close", str(pr_number)) logger.info(logs.PR_CLOSED.format(number=pr_number)) diff --git a/pr_split/logs.py b/pr_split/logs.py index ec302ce..8909fb1 100644 --- a/pr_split/logs.py +++ b/pr_split/logs.py @@ -69,3 +69,7 @@ PR_SKIPPED_BASE_NOT_PUSHED = ( "Skipping PR for group '{group}': its base branch '{base}' was not pushed" ) +PR_RETARGETED = "Retargeted PR #{number} at {base} before merging" +PR_RETARGET_NATIVE_STACK = ( + "PR #{number} is in a native stack; GitHub retargets it when its parent merges" +) diff --git a/tests/test_cli_new_features.py b/tests/test_cli_new_features.py index 2f7c18e..e986ec5 100644 --- a/tests/test_cli_new_features.py +++ b/tests/test_cli_new_features.py @@ -645,3 +645,97 @@ def test_closed_leaf_during_auto_wait_exits_nonzero( assert payload["exit_reason"] == "pr_closed" assert payload["success"] is False assert payload["merged"] == [] + + +class TestStackedMergeRetargetsBase: + def _stacked_plan(self) -> MagicMock: + groups = [ + _group("pr-1", "root", files=["a.py"]), + _group("pr-2", "child", depends_on=["pr-1"], files=["b.py"]), + ] + plan_file = _plan_with_prs(groups) + plan_file.plan.base_branch = "main" + plan_file.git_state.branches = [ + BranchRecord(group_id="pr-1", branch_name="pr-split/ns/pr-1", base_branch="main"), + BranchRecord( + group_id="pr-2", branch_name="pr-split/ns/pr-2", base_branch="pr-split/ns/pr-1" + ), + ] + return plan_file + + @patch("pr_split.cli.retarget_pr") + @patch("pr_split.cli.merge_pr") + @patch("pr_split.cli.get_pr_state") + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_child_is_retargeted_at_the_plan_base_before_merging( + self, + mock_exists: MagicMock, + mock_load: MagicMock, + mock_state: MagicMock, + mock_merge: MagicMock, + mock_retarget: MagicMock, + ) -> None: + mock_load.return_value = self._stacked_plan() + mock_state.return_value = {"state": "OPEN", "isDraft": False, "reviewDecision": None} + calls: list[str] = [] + mock_retarget.side_effect = lambda n, base: calls.append(f"retarget {n} -> {base}") + mock_merge.side_effect = lambda n, *, auto=False: calls.append(f"merge {n}") + + result = runner.invoke(app, ["merge"]) + + assert result.exit_code == 0, result.output + assert calls == ["merge 1", "retarget 2 -> main", "merge 2"] + + @patch("pr_split.cli.retarget_pr") + @patch("pr_split.cli.merge_pr") + @patch("pr_split.cli.get_pr_state") + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_unstacked_prs_are_not_retargeted( + self, + mock_exists: MagicMock, + mock_load: MagicMock, + mock_state: MagicMock, + mock_merge: MagicMock, + mock_retarget: MagicMock, + ) -> None: + plan_file = self._stacked_plan() + plan_file.git_state.branches[1] = BranchRecord( + group_id="pr-2", branch_name="pr-split/ns/pr-2", base_branch="main" + ) + mock_load.return_value = plan_file + mock_state.return_value = {"state": "OPEN", "isDraft": False, "reviewDecision": None} + + result = runner.invoke(app, ["merge"]) + + assert result.exit_code == 0, result.output + mock_retarget.assert_not_called() + assert mock_merge.call_count == 2 + + @patch("pr_split.cli.time.sleep") + @patch("pr_split.cli.retarget_pr") + @patch("pr_split.cli.merge_pr") + @patch("pr_split.cli.get_pr_state") + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_auto_mode_retargets_before_queueing( + self, + mock_exists: MagicMock, + mock_load: MagicMock, + mock_state: MagicMock, + mock_merge: MagicMock, + mock_retarget: MagicMock, + mock_sleep: MagicMock, + ) -> None: + mock_load.return_value = self._stacked_plan() + open_state = {"state": "OPEN", "isDraft": False, "reviewDecision": None} + mock_state.side_effect = [open_state, {"state": "MERGED"}, open_state, {"state": "MERGED"}] + calls: list[str] = [] + mock_retarget.side_effect = lambda n, base: calls.append(f"retarget {n}") + mock_merge.side_effect = lambda n, *, auto=False: calls.append(f"queue {n}") + + result = runner.invoke(app, ["merge", "--auto"]) + + assert result.exit_code == 0, result.output + assert calls == ["queue 1", "retarget 2", "queue 2"] diff --git a/tests/test_git_prs.py b/tests/test_git_prs.py index 25d6031..a9047cc 100644 --- a/tests/test_git_prs.py +++ b/tests/test_git_prs.py @@ -149,3 +149,30 @@ def test_ready_by_default(self, mock_gh: MagicMock) -> None: mock_gh.return_value = "https://github.com/org/repo/pull/7" create_pr("head", "main", "Title", "Body") assert "--draft" not in mock_gh.call_args.args + + +class TestRetargetPr: + @patch("pr_split.git_ops.prs._run_gh", return_value="") + def test_edits_the_base(self, mock_gh: MagicMock) -> None: + from pr_split.git_ops.prs import retarget_pr + + assert retarget_pr(7, "main") is True + mock_gh.assert_called_once_with("pr", "edit", "7", "--base", "main") + + @patch( + "pr_split.git_ops.prs._run_gh", + side_effect=GitOperationError( + "Cannot change the base branch because the pull request is part of a stack" + ), + ) + def test_native_stack_refusal_is_tolerated(self, mock_gh: MagicMock) -> None: + from pr_split.git_ops.prs import retarget_pr + + assert retarget_pr(7, "main") is False + + @patch("pr_split.git_ops.prs._run_gh", side_effect=GitOperationError("rate limited")) + def test_other_failures_raise(self, mock_gh: MagicMock) -> None: + from pr_split.git_ops.prs import retarget_pr + + with pytest.raises(GitOperationError, match="rate limited"): + retarget_pr(7, "main")