fix(pr-status): address review feedback missed before merge (#570 follow-up)

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"

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)

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"

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', () => {

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<PRStatusResponse>(
     swrKey,

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;
 }