diff --git a/graphify/prs.py b/graphify/prs.py index a1ef3b6c5..1084d61e9 100644 --- a/graphify/prs.py +++ b/graphify/prs.py @@ -199,12 +199,37 @@ def _parse_ci(rollup: list) -> str: return "NONE" +_PR_JSON_FIELDS = ( + "number,title,state,headRefName,baseRefName,author,isDraft," + "reviewDecision,statusCheckRollup,updatedAt" +) + + +def _parse_pr_dict(item: dict, expected_base: str) -> PRInfo: + updated_raw = item.get("updatedAt") + if updated_raw: + updated = datetime.fromisoformat(updated_raw.replace("Z", "+00:00")) + else: + updated = datetime.now(timezone.utc) + return PRInfo( + number=item.get("number", 0), + title=item.get("title", ""), + branch=item.get("headRefName", ""), + base_branch=item.get("baseRefName", ""), + author=item["author"]["login"] if item.get("author") else "?", + is_draft=item.get("isDraft", False), + review_decision=item.get("reviewDecision") or "", + ci_status=_parse_ci(item.get("statusCheckRollup") or []), + updated_at=updated, + expected_base=expected_base, + ) + + def fetch_prs(repo: str | None = None, base: str | None = None, limit: int = 50) -> list[PRInfo]: resolved_base = base or _detect_default_branch(repo) args = [ "pr", "list", "--state", "open", "--limit", str(limit), - "--json", "number,title,headRefName,baseRefName,author,isDraft," - "reviewDecision,statusCheckRollup,updatedAt", + "--json", _PR_JSON_FIELDS, ] if repo: args += ["--repo", repo] @@ -213,22 +238,26 @@ def fetch_prs(repo: str | None = None, base: str | None = None, limit: int = 50) if raw is None: raise RuntimeError("gh CLI not found or not authenticated. Run: gh auth login") - prs = [] - for item in raw: - updated = datetime.fromisoformat(item["updatedAt"].replace("Z", "+00:00")) - prs.append(PRInfo( - number=item["number"], - title=item["title"], - branch=item["headRefName"], - base_branch=item["baseRefName"], - author=item["author"]["login"] if item.get("author") else "?", - is_draft=item.get("isDraft", False), - review_decision=item.get("reviewDecision") or "", - ci_status=_parse_ci(item.get("statusCheckRollup") or []), - updated_at=updated, - expected_base=resolved_base, - )) - return prs + return [_parse_pr_dict(item, resolved_base) for item in raw] + + +def fetch_pr(number: int, repo: str | None = None, base: str | None = None) -> PRInfo | None: + resolved_base = base or _detect_default_branch(repo) + args = [ + "pr", "view", str(number), + "--json", _PR_JSON_FIELDS, + ] + if repo: + args += ["--repo", repo] + + raw = _gh(*args) + if raw is None or not isinstance(raw, dict): + return None + if raw.get("state") != "OPEN": + return None + + return _parse_pr_dict(raw, resolved_base) + def fetch_pr_files(number: int, repo: str | None = None) -> list[str]: @@ -358,7 +387,10 @@ def build_community_labels(data: dict, top_n: int = 4) -> dict[int, list[str]]: def attach_graph_impact( - prs: list[PRInfo], graph_path: Path, repo: str | None = None + prs: list[PRInfo], + graph_path: Path, + repo: str | None = None, + include_wrong_base: bool = False, ) -> dict[int, list[str]]: """Fetch PR file lists concurrently, compute graph impact, return community labels.""" data = _load_graph_json(graph_path) @@ -381,7 +413,7 @@ def attach_graph_impact( file_to_nodes[src] += 1 # Fetch diffs concurrently — gh pr diff is the bottleneck (network I/O) - actionable = [pr for pr in prs if pr.status != "WRONG-BASE"] + actionable = [pr for pr in prs if include_wrong_base or pr.status != "WRONG-BASE"] workers = min(8, len(actionable)) if actionable else 1 with ThreadPoolExecutor(max_workers=workers) as pool: future_to_pr = { @@ -738,21 +770,27 @@ def cmd_prs(argv: list[str]) -> None: for pr in prs: pr.worktree_path = worktrees.get(pr.branch) - # Graph impact is expensive (concurrent gh pr diff calls) — only fetch when - # the user actually needs it: deep dive, triage, and conflict detection. - community_labels: dict[int, list[str]] = {} - needs_impact = graph_path.exists() and (pr_number is not None or do_triage or do_conflicts) - if needs_impact: - community_labels = attach_graph_impact(prs, graph_path, repo) - if pr_number is not None: match = next((p for p in prs if p.number == pr_number), None) + if not match: + match = fetch_pr(pr_number, repo=repo, base=base) + if match and match.branch: + match.worktree_path = worktrees.get(match.branch) if not match: print(red(f" PR #{pr_number} not found in open PRs."), file=sys.stderr) sys.exit(1) + if graph_path.exists(): + attach_graph_impact([match], graph_path, repo, include_wrong_base=True) render_pr_detail(match, repo) return + # Graph impact is expensive (concurrent gh pr diff calls) — only fetch when + # the user actually needs it: triage and conflict detection. + community_labels: dict[int, list[str]] = {} + needs_impact = graph_path.exists() and (do_triage or do_conflicts) + if needs_impact: + community_labels = attach_graph_impact(prs, graph_path, repo) + if do_triage: render_dashboard(prs, base, show_wrong_base) triage_with_opus(prs, base) diff --git a/tests/test_prs.py b/tests/test_prs.py index 2bf4e9154..1e81527df 100644 --- a/tests/test_prs.py +++ b/tests/test_prs.py @@ -21,6 +21,11 @@ from graphify.prs import ( fetch_worktrees, format_prs_text, _detect_default_branch, + _parse_pr_dict, + fetch_pr, + fetch_prs, + attach_graph_impact, + cmd_prs, ) @@ -519,3 +524,230 @@ class TestSubprocessStdinIsolation: _detect_default_branch() _args, kwargs = mock_run.call_args assert kwargs.get("stdin") == subprocess.DEVNULL + + +# ── _parse_pr_dict & fetch_pr (#3824) ────────────────────────────────────────── + +class TestParsePrDict: + def test_parse_pr_dict_complete(self): + raw = { + "number": 42, + "title": "Add feature", + "headRefName": "feat-42", + "baseRefName": "v8", + "author": {"login": "alice"}, + "isDraft": False, + "reviewDecision": "APPROVED", + "statusCheckRollup": [{"conclusion": "SUCCESS", "status": "COMPLETED"}], + "updatedAt": "2026-09-20T10:00:00Z", + } + pr = _parse_pr_dict(raw, expected_base="v8") + assert pr.number == 42 + assert pr.title == "Add feature" + assert pr.branch == "feat-42" + assert pr.base_branch == "v8" + assert pr.author == "alice" + assert pr.is_draft is False + assert pr.review_decision == "APPROVED" + assert pr.ci_status == "SUCCESS" + assert pr.status == "APPROVED" + assert pr.updated_at.year == 2026 + + def test_parse_pr_dict_defaults_on_missing_optional_fields(self): + raw = { + "number": 7, + "title": "Minimal PR", + "headRefName": "patch-1", + "baseRefName": "main", + } + pr = _parse_pr_dict(raw, expected_base="main") + assert pr.author == "?" + assert pr.is_draft is False + assert pr.review_decision == "" + assert pr.ci_status == "NONE" + + +class TestFetchPr: + def test_fetch_pr_success(self): + raw = { + "number": 105, + "title": "Fix something", + "state": "OPEN", + "headRefName": "fix-105", + "baseRefName": "v8", + "author": {"login": "bob"}, + "isDraft": False, + "reviewDecision": "", + "statusCheckRollup": [], + "updatedAt": "2026-09-24T12:00:00Z", + } + with patch("graphify.prs._gh", return_value=raw) as mock_gh: + pr = fetch_pr(105, base="v8") + assert pr is not None + assert pr.number == 105 + assert pr.title == "Fix something" + args, _ = mock_gh.call_args + assert args[0:3] == ("pr", "view", "105") + + def test_fetch_pr_with_repo(self): + with patch("graphify.prs._gh", return_value=None) as mock_gh: + fetch_pr(105, repo="owner/repo", base="v8") + args, _ = mock_gh.call_args + assert "--repo" in args + assert "owner/repo" in args + + def test_fetch_pr_not_found_returns_none(self): + with patch("graphify.prs._gh", return_value=None): + pr = fetch_pr(999, base="v8") + assert pr is None + + @pytest.mark.parametrize("state", ["CLOSED", "MERGED"]) + def test_fetch_pr_closed_or_merged_returns_none(self, state: str): + raw = { + "number": 105, + "title": "Old PR", + "state": state, + "headRefName": "feature", + "baseRefName": "v8", + "updatedAt": "2026-09-24T12:00:00Z", + } + with patch("graphify.prs._gh", return_value=raw): + pr = fetch_pr(105, base="v8") + assert pr is None + + +# ── attach_graph_impact & include_wrong_base (#3824) ────────────────────────── + +class TestAttachGraphImpact: + def _sample_graph_data(self): + return { + "nodes": [ + {"id": "n1", "source_file": "src/auth.py", "community": 0, "label": "auth"}, + {"id": "n2", "source_file": "src/auth.py", "community": 0, "label": "login"}, + {"id": "n3", "source_file": "src/api.py", "community": 1, "label": "api"}, + ] + } + + def test_attach_graph_impact_skips_wrong_base_by_default(self, tmp_path): + graph_file = tmp_path / "graph.json" + graph_file.write_text(json.dumps(self._sample_graph_data()), encoding="utf-8") + + pr_ready = make_pr(number=1, base_branch="v8", expected_base="v8") + pr_wrong = make_pr(number=2, base_branch="develop", expected_base="v8") + + with patch("graphify.prs.fetch_pr_files", return_value=["src/auth.py"]) as mock_files: + attach_graph_impact([pr_ready, pr_wrong], graph_file, include_wrong_base=False) + + mock_files.assert_called_once_with(1, None) + assert pr_ready.nodes_affected == 2 + assert pr_ready.communities_touched == [0] + assert pr_wrong.nodes_affected == 0 + assert pr_wrong.communities_touched == [] + + def test_attach_graph_impact_includes_wrong_base_when_requested(self, tmp_path): + graph_file = tmp_path / "graph.json" + graph_file.write_text(json.dumps(self._sample_graph_data()), encoding="utf-8") + + pr_wrong = make_pr(number=2, base_branch="develop", expected_base="v8") + assert pr_wrong.status == "WRONG-BASE" + + with patch("graphify.prs.fetch_pr_files", return_value=["src/auth.py"]) as mock_files: + attach_graph_impact([pr_wrong], graph_file, include_wrong_base=True) + + mock_files.assert_called_once_with(2, None) + assert pr_wrong.nodes_affected == 2 + assert pr_wrong.communities_touched == [0] + assert pr_wrong.status == "WRONG-BASE" + assert pr_wrong.blast_radius == "2 nodes / 1 community" + + +# ── cmd_prs single-PR detail mode (#3824) ───────────────────────────────────── + +class TestCmdPrsDetail: + def test_detail_only_fetches_diff_for_requested_pr(self, tmp_path, capsys): + graph_file = tmp_path / "graph.json" + graph_data = { + "nodes": [ + {"id": "n1", "source_file": "src/app.py", "community": 0, "label": "app"}, + ] + } + graph_file.write_text(json.dumps(graph_data), encoding="utf-8") + + pr1 = make_pr(number=10, base_branch="v8", expected_base="v8") + pr2 = make_pr(number=20, base_branch="v8", expected_base="v8") + pr3 = make_pr(number=30, base_branch="v8", expected_base="v8") + + with patch("graphify.prs.fetch_prs", return_value=[pr1, pr2, pr3]), \ + patch("graphify.prs.fetch_worktrees", return_value={}), \ + patch("graphify.prs.fetch_pr_files", return_value=["src/app.py"]) as mock_diff: + cmd_prs(["20", f"--graph={graph_file}"]) + + # Crucial check for #3824: only PR 20's diff must be fetched, NOT 10 or 30 + assert mock_diff.call_count == 1 + assert mock_diff.call_args[0][0] == 20 + captured = capsys.readouterr() + assert "PR #20" in captured.out + assert "Graph impact:" in captured.out + + def test_detail_calculates_impact_for_wrong_base_pr(self, tmp_path, capsys): + graph_file = tmp_path / "graph.json" + graph_data = { + "nodes": [ + {"id": "n1", "source_file": "src/app.py", "community": 0, "label": "app"}, + ] + } + graph_file.write_text(json.dumps(graph_data), encoding="utf-8") + + pr_wrong = make_pr(number=42, base_branch="feature-x", expected_base="v8") + assert pr_wrong.status == "WRONG-BASE" + + with patch("graphify.prs.fetch_prs", return_value=[pr_wrong]), \ + patch("graphify.prs.fetch_worktrees", return_value={}), \ + patch("graphify.prs.fetch_pr_files", return_value=["src/app.py"]) as mock_diff: + cmd_prs(["42", f"--graph={graph_file}"]) + + assert mock_diff.call_count == 1 + assert mock_diff.call_args[0][0] == 42 + captured = capsys.readouterr() + assert "PR #42" in captured.out + assert "WRONG-BASE" in captured.out + assert "Graph impact:" in captured.out + + def test_detail_falls_back_to_fetch_pr_when_outside_first_50(self, tmp_path, capsys): + graph_file = tmp_path / "graph.json" + graph_data = { + "nodes": [ + {"id": "n1", "source_file": "src/app.py", "community": 0, "label": "app"}, + ] + } + graph_file.write_text(json.dumps(graph_data), encoding="utf-8") + + # 50 PRs that do not include PR 99 + prs_50 = [make_pr(number=i, base_branch="v8") for i in range(1, 51)] + pr_99 = make_pr(number=99, branch="feat-99", base_branch="v8", expected_base="v8") + + with patch("graphify.prs.fetch_prs", return_value=prs_50), \ + patch("graphify.prs.fetch_worktrees", return_value={"feat-99": "/path/to/wt"}), \ + patch("graphify.prs.fetch_pr", return_value=pr_99) as mock_fetch_single, \ + patch("graphify.prs.fetch_pr_files", return_value=["src/app.py"]): + cmd_prs(["99", f"--graph={graph_file}"]) + + mock_fetch_single.assert_called_once_with(99, repo=None, base="v8") + assert pr_99.worktree_path == "/path/to/wt" + captured = capsys.readouterr() + assert "PR #99" in captured.out + assert "worktree:" in captured.out + assert "/path/to/wt" in captured.out + + def test_detail_exits_when_not_found_in_list_or_view(self, capsys): + prs_50 = [make_pr(number=i) for i in range(1, 51)] + + with patch("graphify.prs.fetch_prs", return_value=prs_50), \ + patch("graphify.prs.fetch_worktrees", return_value={}), \ + patch("graphify.prs.fetch_pr", return_value=None): + with pytest.raises(SystemExit) as exc_info: + cmd_prs(["999"]) + + assert exc_info.value.code == 1 + captured = capsys.readouterr() + assert "PR #999 not found in open PRs." in captured.err