diff --git a/codeframe/git/github_integration.py b/codeframe/git/github_integration.py index 4c8d5650..050b1c37 100644 --- a/codeframe/git/github_integration.py +++ b/codeframe/git/github_integration.py @@ -430,10 +430,34 @@ async def get_pr_review_status(self, pr_number: int) -> str: "GET", f"/repos/{self.owner}/{self.repo_name}/pulls/{pr_number}/reviews", ) - reviews = reviews or [] - has_changes_requested = any(r.get("state") == "CHANGES_REQUESTED" for r in reviews) - has_approved = any(r.get("state") == "APPROVED" for r in reviews) + # Guard against unexpected response shapes. + if not isinstance(reviews, list): + reviews = [] + + # Only count actionable states — exclude DISMISSED, COMMENTED, and non-dicts. + ACTIONABLE = {"APPROVED", "CHANGES_REQUESTED"} + + # Collapse per-reviewer: keep only each reviewer's latest actionable review. + # A reviewer who requested changes and later approved should be counted as + # approved, not as both. + latest_per_reviewer: dict[str, dict] = {} + for r in reviews: + if not isinstance(r, dict): + continue + if r.get("state") not in ACTIONABLE: + continue + reviewer = r.get("user", {}).get("login") if isinstance(r.get("user"), dict) else None + if reviewer is None: + reviewer = str(r.get("id", id(r))) + existing = latest_per_reviewer.get(reviewer) + if existing is None or (r.get("submitted_at") or "") >= (existing.get("submitted_at") or ""): + latest_per_reviewer[reviewer] = r + + active = list(latest_per_reviewer.values()) + + has_changes_requested = any(r.get("state") == "CHANGES_REQUESTED" for r in active) + has_approved = any(r.get("state") == "APPROVED" for r in active) if has_changes_requested: return "changes_requested" diff --git a/codeframe/ui/routers/pr_v2.py b/codeframe/ui/routers/pr_v2.py index 2c1e0fa6..d8787f7f 100644 --- a/codeframe/ui/routers/pr_v2.py +++ b/codeframe/ui/routers/pr_v2.py @@ -163,17 +163,46 @@ async def get_pr_status( Returns: PRStatusResponse with CI checks, review status, and merge state """ + # _get_github_client() raises HTTPException if GitHub isn't configured — + # no client to close in that case, so call it before the try/finally. + client = _get_github_client() try: - client = _get_github_client() - # Single call to get PR state, URL, and head SHA. pr_raw = await client._make_request( "GET", f"/repos/{client.owner}/{client.repo_name}/pulls/{pr_number}", ) - head_sha: str = pr_raw["head"]["sha"] - pr_url: str = pr_raw["html_url"] - merge_state: str = "merged" if pr_raw.get("merged_at") else pr_raw["state"] + + # Validate payload shape: non-dict responses (e.g. a list) or a dict with + # a non-dict "head" field would blow up before reaching the field checks. + if not isinstance(pr_raw, dict): + raise HTTPException( + status_code=502, + detail=api_error( + "Invalid GitHub response", + ErrorCodes.EXECUTION_FAILED, + "PR payload was not an object", + ), + ) + + # Use safe access; raise 502 if required fields are absent rather than + # letting a KeyError bubble into an unhandled 500. + head = pr_raw.get("head") + head_sha: str | None = head.get("sha") if isinstance(head, dict) else None + pr_url: str | None = pr_raw.get("html_url") + state: str | None = pr_raw.get("state") + + if not head_sha or not pr_url or not state: + raise HTTPException( + status_code=502, + detail=api_error( + "Invalid GitHub response", + ErrorCodes.EXECUTION_FAILED, + "Missing required fields (head.sha / html_url / state) in PR payload", + ), + ) + + merge_state: str = "merged" if pr_raw.get("merged_at") else state # Fetch CI checks and reviews in parallel (2 more GitHub API calls). ci_checks, review_status = await asyncio.gather( @@ -210,6 +239,8 @@ async def get_pr_status( status_code=500, detail=api_error("Failed to get PR status", ErrorCodes.EXECUTION_FAILED, str(e)), ) + finally: + await client.close() @router.get("", response_model=PRListResponse) diff --git a/tests/ui/test_pr_status.py b/tests/ui/test_pr_status.py index d3c28c6d..3e657f6a 100644 --- a/tests/ui/test_pr_status.py +++ b/tests/ui/test_pr_status.py @@ -78,6 +78,7 @@ def _make_mock_client( return_value=ci_checks if ci_checks is not None else [] ) client.get_pr_review_status = AsyncMock(return_value=review_status) + client.close = AsyncMock() # always present — called in finally block client.owner = "owner" client.repo_name = "repo" return client @@ -229,3 +230,186 @@ def test_github_api_error_propagates_status_code(self, test_client): resp = test_client.get("/api/v2/pr/status?workspace_path=/tmp&pr_number=42") assert resp.status_code == 503 + + def test_missing_required_fields_returns_502(self, test_client): + """PR payload missing head.sha / html_url / state returns 502.""" + # Simulate a partial/unexpected GitHub response. + pr_raw = {"merged_at": None} # no "head", "html_url", or "state" + mock_client = _make_mock_client(pr_raw=pr_raw) + + with patch("codeframe.ui.routers.pr_v2._get_github_client", return_value=mock_client): + resp = test_client.get("/api/v2/pr/status?workspace_path=/tmp&pr_number=42") + + assert resp.status_code == 502 + + def test_non_dict_pr_raw_returns_502(self, test_client): + """A non-dict GitHub response (e.g. a list) returns 502 rather than crashing.""" + mock_client = _make_mock_client(pr_raw=[]) # list instead of dict + + with patch("codeframe.ui.routers.pr_v2._get_github_client", return_value=mock_client): + resp = test_client.get("/api/v2/pr/status?workspace_path=/tmp&pr_number=42") + + assert resp.status_code == 502 + + def test_non_dict_head_field_returns_502(self, test_client): + """A dict pr_raw with a non-dict 'head' field returns 502 instead of AttributeError.""" + pr_raw = {"head": "not-a-dict", "html_url": "https://github.com/p/r/pull/1", "state": "open"} + mock_client = _make_mock_client(pr_raw=pr_raw) + + with patch("codeframe.ui.routers.pr_v2._get_github_client", return_value=mock_client): + resp = test_client.get("/api/v2/pr/status?workspace_path=/tmp&pr_number=42") + + assert resp.status_code == 502 + + def test_generic_exception_returns_500(self, test_client): + """An unexpected exception (e.g. ValueError) results in HTTP 500.""" + mock_client = _make_mock_client(raise_error=ValueError("unexpected")) + + with patch("codeframe.ui.routers.pr_v2._get_github_client", return_value=mock_client): + resp = test_client.get("/api/v2/pr/status?workspace_path=/tmp&pr_number=42") + + assert resp.status_code == 500 + + def test_client_close_called_on_success(self, test_client): + """Client.close() is always called, even on success.""" + mock_client = _make_mock_client() + + with patch("codeframe.ui.routers.pr_v2._get_github_client", return_value=mock_client): + test_client.get("/api/v2/pr/status?workspace_path=/tmp&pr_number=42") + + mock_client.close.assert_awaited_once() + + def test_client_close_called_on_github_error(self, test_client): + """Client.close() is always called, even when GitHub returns an error.""" + from codeframe.git.github_integration import GitHubAPIError + + mock_client = _make_mock_client(raise_error=GitHubAPIError(503, "Service Unavailable")) + + with patch("codeframe.ui.routers.pr_v2._get_github_client", return_value=mock_client): + test_client.get("/api/v2/pr/status?workspace_path=/tmp&pr_number=42") + + mock_client.close.assert_awaited_once() + + +class TestGetPrReviewStatus: + """Unit tests for GitHubIntegration.get_pr_review_status().""" + + def _make_integration(self) -> object: + """Return a GitHubIntegration instance with a mocked _make_request.""" + from codeframe.git.github_integration import GitHubIntegration + + with patch.object(GitHubIntegration, "__init__", lambda self, **kw: None): + gh = object.__new__(GitHubIntegration) + gh.owner = "owner" + gh.repo_name = "repo" + return gh + + @pytest.mark.asyncio + async def test_approved_review(self): + """Single APPROVED review → approved.""" + from codeframe.git.github_integration import GitHubIntegration + + gh = self._make_integration() + gh._make_request = AsyncMock(return_value=[{"state": "APPROVED"}]) + result = await GitHubIntegration.get_pr_review_status(gh, 1) + assert result == "approved" + + @pytest.mark.asyncio + async def test_changes_requested_beats_approved(self): + """CHANGES_REQUESTED takes priority over APPROVED.""" + from codeframe.git.github_integration import GitHubIntegration + + gh = self._make_integration() + gh._make_request = AsyncMock( + return_value=[{"state": "APPROVED"}, {"state": "CHANGES_REQUESTED"}] + ) + result = await GitHubIntegration.get_pr_review_status(gh, 1) + assert result == "changes_requested" + + @pytest.mark.asyncio + async def test_dismissed_review_ignored(self): + """DISMISSED review is not counted — falls back to pending.""" + from codeframe.git.github_integration import GitHubIntegration + + gh = self._make_integration() + gh._make_request = AsyncMock(return_value=[{"state": "DISMISSED"}]) + result = await GitHubIntegration.get_pr_review_status(gh, 1) + assert result == "pending" + + @pytest.mark.asyncio + async def test_commented_review_ignored(self): + """COMMENTED review is not counted — falls back to pending.""" + from codeframe.git.github_integration import GitHubIntegration + + gh = self._make_integration() + gh._make_request = AsyncMock(return_value=[{"state": "COMMENTED"}]) + result = await GitHubIntegration.get_pr_review_status(gh, 1) + assert result == "pending" + + @pytest.mark.asyncio + async def test_non_list_response_returns_pending(self): + """A non-list response from GitHub is treated as no reviews → pending.""" + from codeframe.git.github_integration import GitHubIntegration + + gh = self._make_integration() + gh._make_request = AsyncMock(return_value={"unexpected": "dict"}) + result = await GitHubIntegration.get_pr_review_status(gh, 1) + assert result == "pending" + + @pytest.mark.asyncio + async def test_non_dict_items_in_list_ignored(self): + """Non-dict items in the reviews list are silently skipped.""" + from codeframe.git.github_integration import GitHubIntegration + + gh = self._make_integration() + gh._make_request = AsyncMock( + return_value=["not-a-dict", None, {"state": "APPROVED"}] + ) + result = await GitHubIntegration.get_pr_review_status(gh, 1) + assert result == "approved" + + @pytest.mark.asyncio + async def test_same_reviewer_changes_then_approved(self): + """Reviewer who first requested changes and later approved counts as approved.""" + from codeframe.git.github_integration import GitHubIntegration + + gh = self._make_integration() + gh._make_request = AsyncMock( + return_value=[ + { + "state": "CHANGES_REQUESTED", + "user": {"login": "alice"}, + "submitted_at": "2024-01-01T10:00:00Z", + }, + { + "state": "APPROVED", + "user": {"login": "alice"}, + "submitted_at": "2024-01-01T12:00:00Z", + }, + ] + ) + result = await GitHubIntegration.get_pr_review_status(gh, 1) + assert result == "approved" + + @pytest.mark.asyncio + async def test_same_reviewer_approved_then_changes(self): + """Reviewer who approved and later requested changes counts as changes_requested.""" + from codeframe.git.github_integration import GitHubIntegration + + gh = self._make_integration() + gh._make_request = AsyncMock( + return_value=[ + { + "state": "APPROVED", + "user": {"login": "bob"}, + "submitted_at": "2024-01-01T10:00:00Z", + }, + { + "state": "CHANGES_REQUESTED", + "user": {"login": "bob"}, + "submitted_at": "2024-01-01T12:00:00Z", + }, + ] + ) + result = await GitHubIntegration.get_pr_review_status(gh, 1) + assert result == "changes_requested" diff --git a/web-ui/src/__tests__/components/review/PRStatusPanel.test.tsx b/web-ui/src/__tests__/components/review/PRStatusPanel.test.tsx index 0db9c2d4..c8a014c7 100644 --- a/web-ui/src/__tests__/components/review/PRStatusPanel.test.tsx +++ b/web-ui/src/__tests__/components/review/PRStatusPanel.test.tsx @@ -173,7 +173,7 @@ describe('PRStatusPanel', () => { const [key] = mockUseSWR.mock.calls[0]; expect(key).toContain(`pr_number=${PR_NUMBER}`); - expect(key).toContain(`workspace_path=${WORKSPACE}`); + expect(key).toContain(`workspace_path=${encodeURIComponent(WORKSPACE)}`); }); it('passes a refreshInterval function to SWR config', () => { diff --git a/web-ui/src/components/review/PRStatusPanel.tsx b/web-ui/src/components/review/PRStatusPanel.tsx index 3fed716d..3799e214 100644 --- a/web-ui/src/components/review/PRStatusPanel.tsx +++ b/web-ui/src/components/review/PRStatusPanel.tsx @@ -63,7 +63,7 @@ export interface PRStatusPanelProps { } export function PRStatusPanel({ prNumber, workspacePath }: PRStatusPanelProps) { - const swrKey = `/api/v2/pr/status?workspace_path=${workspacePath}&pr_number=${prNumber}`; + const swrKey = `/api/v2/pr/status?workspace_path=${encodeURIComponent(workspacePath)}&pr_number=${prNumber}`; const { data, error } = useSWR( swrKey, diff --git a/web-ui/src/types/index.ts b/web-ui/src/types/index.ts index 34860c09..21dc26e9 100644 --- a/web-ui/src/types/index.ts +++ b/web-ui/src/types/index.ts @@ -274,14 +274,14 @@ export interface CreatePRRequest { export interface CICheck { name: string; - status: string; // "queued" | "in_progress" | "completed" - conclusion: string | null; // "success" | "failure" | "neutral" | "cancelled" | etc. + status: 'queued' | 'in_progress' | 'completed' | 'waiting' | 'requested' | 'pending'; + conclusion: 'success' | 'failure' | 'neutral' | 'cancelled' | 'timed_out' | 'action_required' | 'skipped' | 'stale' | null; } export interface PRStatusResponse { ci_checks: CICheck[]; - review_status: string; // "approved" | "changes_requested" | "pending" - merge_state: string; // "open" | "merged" | "closed" + review_status: 'approved' | 'changes_requested' | 'pending'; + merge_state: 'open' | 'merged' | 'closed'; pr_url: string; pr_number: number; }