From 8eba6d316ef9ef0c099f02d00896ed892e45a992 Mon Sep 17 00:00:00 2001 From: vitali87 Date: Sun, 30 Aug 2026 15:02:33 +0100 Subject: [PATCH 1/2] fix: exit 1 when a PR's state could not be fetched during merge A get_pr_state failure was recorded as a skip and the run still ended with exit 0 and 'Merge complete', so a broken gh session or transient API error looked like success to scripts and webhooks. Report the affected groups, exit 1, and send exit_reason=fetch_error. --- pr_split/cli.py | 13 +++++++++++-- tests/test_cli_new_features.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/pr_split/cli.py b/pr_split/cli.py index a678a01..86317bf 100644 --- a/pr_split/cli.py +++ b/pr_split/cli.py @@ -1139,6 +1139,7 @@ def merge_all( skipped: list[str] = [] skipped_ids: set[str] = set() blocked: list[str] = [] + fetch_errors: list[str] = [] failed: list[str] = [] stopped = False @@ -1157,6 +1158,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 @@ -1245,6 +1247,11 @@ 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" @@ -1253,6 +1260,8 @@ def merge_all( if exited_early else "unmerged_dependency" if blocked + else "fetch_error" + if fetch_errors else "success" ) skipped_structured = [{"id": s.split(" (")[0], "reason": s} for s in skipped] @@ -1263,11 +1272,11 @@ 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 2d1b74d..979f60a 100644 --- a/tests/test_cli_new_features.py +++ b/tests/test_cli_new_features.py @@ -485,3 +485,37 @@ def test_child_of_merged_parent_merges( assert [c.args[0] for c in mock_merge.call_args_list] == [1, 2] 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" From cee40beef3dc26ecfcec6c4da75924a6c57d8cf3 Mon Sep 17 00:00:00 2001 From: vitali87 Date: Sun, 30 Aug 2026 15:06:05 +0100 Subject: [PATCH 2/2] fix: report polling fetch errors and rank fetch_error above unmerged_dependency --- README.md | 2 +- pr_split/cli.py | 19 +++++++++--- tests/test_cli_new_features.py | 56 ++++++++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 406e052..3b61675 100644 --- a/README.md +++ b/README.md @@ -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 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 86317bf..5edf477 100644 --- a/pr_split/cli.py +++ b/pr_split/cli.py @@ -1067,7 +1067,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 @@ -1086,6 +1090,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) @@ -1218,8 +1224,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: @@ -1258,10 +1269,10 @@ def merge_all( if stopped else "incomplete_batch" if exited_early - else "unmerged_dependency" - if blocked else "fetch_error" if fetch_errors + else "unmerged_dependency" + if blocked else "success" ) skipped_structured = [{"id": s.split(" (")[0], "reason": s} for s in skipped] diff --git a/tests/test_cli_new_features.py b/tests/test_cli_new_features.py index 979f60a..44dd3c6 100644 --- a/tests/test_cli_new_features.py +++ b/tests/test_cli_new_features.py @@ -519,3 +519,59 @@ def test_unfetchable_pr_state_fails_the_run( 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"]