diff --git a/README.md b/README.md index 124af7e..f467ba4 100644 --- a/README.md +++ b/README.md @@ -121,7 +121,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 or any PR was left blocked, so re-run once the blocking PRs are ready. +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. 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 ad690a4..47215a2 100644 --- a/pr_split/cli.py +++ b/pr_split/cli.py @@ -2034,7 +2034,11 @@ def execute( _AUTO_MERGE_POLL_TIMEOUT = 600 -def _poll_for_merged(group_ids: list[str], pr_map: dict[str, PRRecord]) -> set[str]: +def _poll_for_merged( + group_ids: list[str], + pr_map: dict[str, PRRecord], + fetch_errors: list[str] | None = None, +) -> set[str]: pending = set(group_ids) actually_merged: set[str] = set() deadline = time.monotonic() + _AUTO_MERGE_POLL_TIMEOUT @@ -2053,6 +2057,8 @@ def _poll_for_merged(group_ids: list[str], pr_map: dict[str, PRRecord]) -> set[s logger.warning( f"PR #{pr_record.pr_number} ({gid}) {reason} while polling, aborting wait" ) + if state == "" and fetch_errors is not None: + fetch_errors.append(gid) pending.discard(gid) if pending: remaining = ", ".join(pending) @@ -2126,6 +2132,7 @@ def merge_all( skipped: list[str] = [] skipped_ids: set[str] = set() blocked: list[str] = [] + fetch_errors: list[str] = [] failed: list[str] = [] stopped = False @@ -2144,6 +2151,7 @@ def merge_all( f"PR #{pr_record.pr_number} ({group_id}) state could not be fetched, skipping" ) skipped_ids.add(group_id) + fetch_errors.append(group_id) skipped.append(f"{group_id} (fetch error)") continue @@ -2203,8 +2211,13 @@ def merge_all( queued = [gid for gid in batch if gid not in merged and gid not in skipped_ids] if queued: logger.info(f"Waiting for auto-merge to complete: {', '.join(queued)}") - actually_merged = _poll_for_merged(queued, pr_map) + poll_fetch_errors: list[str] = [] + actually_merged = _poll_for_merged(queued, pr_map, poll_fetch_errors) merged.extend(actually_merged) + for gid in poll_fetch_errors: + fetch_errors.append(gid) + skipped_ids.add(gid) + skipped.append(f"{gid} (fetch error)") if stopped or any(gid not in merged and gid not in skipped_ids for gid in batch): if not stopped: @@ -2232,12 +2245,19 @@ def merge_all( f"[yellow]Blocked by unmerged dependencies ({len(blocked)}): " f"{', '.join(blocked)}. Re-run once those PRs are merged.[/yellow]" ) + if fetch_errors: + console.print( + f"[red]Could not fetch PR state for ({len(fetch_errors)}): " + f"{', '.join(fetch_errors)}. Check 'gh auth status' and re-run.[/red]" + ) if notify: exit_reason = ( "merge_error" if stopped else "incomplete_batch" if exited_early + else "fetch_error" + if fetch_errors else "unmerged_dependency" if blocked else "success" @@ -2250,12 +2270,12 @@ def merge_all( "merged": merged, "skipped": skipped_structured, "failed": failed, - "success": not (failed or stopped or exited_early or blocked), + "success": not (failed or stopped or exited_early or blocked or fetch_errors), "exit_reason": exit_reason, }, ) - if failed or stopped or exited_early or blocked: + if failed or stopped or exited_early or blocked or fetch_errors: raise typer.Exit(1) logger.success(f"Merge complete: {len(merged)} PRs merged") diff --git a/tests/test_cli_new_features.py b/tests/test_cli_new_features.py index 4987110..31f7177 100644 --- a/tests/test_cli_new_features.py +++ b/tests/test_cli_new_features.py @@ -863,6 +863,96 @@ def test_child_of_merged_parent_merges( assert result.exit_code == 0 +class TestMergeFetchErrors: + @patch("pr_split.cli._send_webhook") + @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_unfetchable_pr_state_fails_the_run( + self, + mock_exists: MagicMock, + mock_load: MagicMock, + mock_state: MagicMock, + mock_merge: MagicMock, + mock_webhook: MagicMock, + ) -> None: + groups = [ + _group("pr-1", "ok", files=["a.py"]), + _group("pr-2", "unfetchable", files=["b.py"]), + ] + mock_load.return_value = _plan_with_prs(groups) + mock_state.side_effect = lambda n: ( + {"state": "OPEN", "isDraft": False, "reviewDecision": None} if n == 1 else {} + ) + + result = runner.invoke(app, ["merge", "--notify", "https://example.invalid/hook"]) + + mock_merge.assert_called_once_with(1, auto=False) + assert result.exit_code == 1 + assert "Could not fetch PR state" in result.output + assert "Merge complete" not in result.output + payload = mock_webhook.call_args[0][1] + assert payload["success"] is False + assert payload["exit_reason"] == "fetch_error" + + @patch("pr_split.cli._send_webhook") + @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_fetch_error_on_parent_is_reported_as_root_cause( + self, + mock_exists: MagicMock, + mock_load: MagicMock, + mock_state: MagicMock, + mock_merge: MagicMock, + mock_webhook: MagicMock, + ) -> None: + groups = [ + _group("pr-1", "unfetchable", files=["a.py"]), + _group("pr-2", "child", depends_on=["pr-1"], files=["b.py"]), + ] + mock_load.return_value = _plan_with_prs(groups) + mock_state.side_effect = lambda n: ( + {} if n == 1 else {"state": "OPEN", "isDraft": False, "reviewDecision": None} + ) + + result = runner.invoke(app, ["merge", "--notify", "https://example.invalid/hook"]) + + assert result.exit_code == 1 + assert mock_webhook.call_args[0][1]["exit_reason"] == "fetch_error" + + @patch("pr_split.cli.time.sleep") + @patch("pr_split.cli._send_webhook") + @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_fetch_error_while_polling_auto_merge_is_reported( + self, + mock_exists: MagicMock, + mock_load: MagicMock, + mock_state: MagicMock, + mock_merge: MagicMock, + mock_webhook: MagicMock, + mock_sleep: MagicMock, + ) -> None: + groups = [_group("pr-1", "only", files=["a.py"])] + mock_load.return_value = _plan_with_prs(groups) + mock_state.side_effect = [{"state": "OPEN", "isDraft": False, "reviewDecision": None}, {}] + + result = runner.invoke( + app, ["merge", "--auto", "--notify", "https://example.invalid/hook"] + ) + + assert result.exit_code == 1 + assert "Could not fetch PR state" in result.output + payload = mock_webhook.call_args[0][1] + assert payload["exit_reason"] == "fetch_error" + assert [s["id"] for s in payload["skipped"]] == ["pr-1"] + + class TestMergeRejectsMalformedSavedPlan: def _plan_file(self, groups: list[Group]) -> PlanFile: from pr_split.constants import Priority