From 1a166d0f8213821f4d071c9e8ed48425436f34ba Mon Sep 17 00:00:00 2001 From: Test User Date: Mon, 13 Apr 2026 08:24:22 -0700 Subject: [PATCH 1/2] fix(pr-status): address CodeRabbit and claude-review feedback post-merge Five issues identified in review of #570 that were not addressed before merge: 1. get_pr_review_status: guard against non-list response, filter non-dict items, and exclude DISMISSED/COMMENTED states (only APPROVED and CHANGES_REQUESTED are actionable). 2. pr_v2.py get_pr_status: use safe .get() access on pr_raw (head.sha, html_url, state); raise 502 with a clear message if required fields are absent rather than letting KeyError surface as unhandled 500. 3. pr_v2.py get_pr_status: wrap GitHub calls in try/finally and call await client.close() to prevent httpx AsyncClient file-descriptor leak. 4. PRStatusPanel: encodeURIComponent(workspacePath) in SWR cache key to prevent cache mismatches for paths with spaces or special characters. 5. types/index.ts: narrow CICheck.status/conclusion and PRStatusResponse.review_status/merge_state to union literal types so typos are caught at compile time rather than failing silently at runtime. Tests: +10 backend (502 path, generic 500, client-close assertions, six direct unit tests for get_pr_review_status corner cases). All 20 pass. --- codeframe/git/github_integration.py | 16 ++- codeframe/ui/routers/pr_v2.py | 28 ++++- tests/ui/test_pr_status.py | 119 ++++++++++++++++++ .../components/review/PRStatusPanel.test.tsx | 2 +- .../src/components/review/PRStatusPanel.tsx | 2 +- web-ui/src/types/index.ts | 8 +- 6 files changed, 161 insertions(+), 14 deletions(-) diff --git a/codeframe/git/github_integration.py b/codeframe/git/github_integration.py index 4c8d5650..34518c23 100644 --- a/codeframe/git/github_integration.py +++ b/codeframe/git/github_integration.py @@ -430,10 +430,20 @@ 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"} + active = [ + r for r in reviews + if isinstance(r, dict) and r.get("state") in ACTIONABLE + ] + + 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..46674491 100644 --- a/codeframe/ui/routers/pr_v2.py +++ b/codeframe/ui/routers/pr_v2.py @@ -163,17 +163,33 @@ 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"] + + # Use safe access; raise 502 if required fields are absent rather than + # letting a KeyError bubble into an unhandled 500. + head_sha: str | None = pr_raw.get("head", {}).get("sha") + 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 +226,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..1d07c3f9 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,121 @@ 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_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" 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..c6a4eb83 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'; + conclusion: 'success' | 'failure' | 'neutral' | 'cancelled' | 'timed_out' | 'action_required' | 'skipped' | 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; } From edf08e9ac2dd3250c9e3f5ec8990160f79644b60 Mon Sep 17 00:00:00 2001 From: Test User Date: Mon, 13 Apr 2026 11:11:06 -0700 Subject: [PATCH 2/2] fix(pr-status): address CodeRabbit follow-up feedback on PR #580 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - github_integration.py: collapse per-reviewer review history before aggregating — a reviewer who CHANGES_REQUESTed then APPROVEd now correctly counts as approved (was double-counted before) - pr_v2.py: validate pr_raw is a dict before .get() access; non-dict head field also raises 502 rather than AttributeError - types/index.ts: add missing GitHub API values to CICheck unions (status: waiting/requested/pending; conclusion: stale) - tests: add 4 new cases (same-reviewer sequence ×2, non-dict pr_raw, non-dict head field) — 24/24 pass --- codeframe/git/github_integration.py | 22 ++++++++-- codeframe/ui/routers/pr_v2.py | 15 ++++++- tests/ui/test_pr_status.py | 65 +++++++++++++++++++++++++++++ web-ui/src/types/index.ts | 4 +- 4 files changed, 99 insertions(+), 7 deletions(-) diff --git a/codeframe/git/github_integration.py b/codeframe/git/github_integration.py index 34518c23..050b1c37 100644 --- a/codeframe/git/github_integration.py +++ b/codeframe/git/github_integration.py @@ -437,10 +437,24 @@ async def get_pr_review_status(self, pr_number: int) -> str: # Only count actionable states — exclude DISMISSED, COMMENTED, and non-dicts. ACTIONABLE = {"APPROVED", "CHANGES_REQUESTED"} - active = [ - r for r in reviews - if isinstance(r, dict) and r.get("state") in ACTIONABLE - ] + + # 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) diff --git a/codeframe/ui/routers/pr_v2.py b/codeframe/ui/routers/pr_v2.py index 46674491..d8787f7f 100644 --- a/codeframe/ui/routers/pr_v2.py +++ b/codeframe/ui/routers/pr_v2.py @@ -173,9 +173,22 @@ async def get_pr_status( f"/repos/{client.owner}/{client.repo_name}/pulls/{pr_number}", ) + # 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_sha: str | None = pr_raw.get("head", {}).get("sha") + 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") diff --git a/tests/ui/test_pr_status.py b/tests/ui/test_pr_status.py index 1d07c3f9..3e657f6a 100644 --- a/tests/ui/test_pr_status.py +++ b/tests/ui/test_pr_status.py @@ -242,6 +242,25 @@ def test_missing_required_fields_returns_502(self, test_client): 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")) @@ -348,3 +367,49 @@ async def test_non_dict_items_in_list_ignored(self): ) 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/types/index.ts b/web-ui/src/types/index.ts index c6a4eb83..21dc26e9 100644 --- a/web-ui/src/types/index.ts +++ b/web-ui/src/types/index.ts @@ -274,8 +274,8 @@ export interface CreatePRRequest { export interface CICheck { name: string; - status: 'queued' | 'in_progress' | 'completed'; - conclusion: 'success' | 'failure' | 'neutral' | 'cancelled' | 'timed_out' | 'action_required' | 'skipped' | null; + status: 'queued' | 'in_progress' | 'completed' | 'waiting' | 'requested' | 'pending'; + conclusion: 'success' | 'failure' | 'neutral' | 'cancelled' | 'timed_out' | 'action_required' | 'skipped' | 'stale' | null; } export interface PRStatusResponse {