Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 77 additions & 0 deletions codeframe/git/github_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,15 @@ def __init__(
super().__init__(f"GitHub API Error ({status_code}): {message}")


@dataclass
class CICheck:
"""A single CI check run result."""

name: str
status: str # "queued" | "in_progress" | "completed"
conclusion: Optional[str] # "success" | "failure" | "neutral" | "cancelled" | etc.


@dataclass
class PRDetails:
"""Pull Request details from GitHub API."""
Expand Down Expand Up @@ -364,6 +373,74 @@ async def close_pull_request(self, pr_number: int) -> bool:
logger.info(f"Closed PR #{pr_number}")
return data.get("state") == "closed"

async def get_pr_ci_checks(
self,
pr_number: int,
head_sha: Optional[str] = None,
) -> List[CICheck]:
"""Get CI check runs for a pull request.

Args:
pr_number: PR number
head_sha: Head commit SHA (fetched from the PR if not provided)

Returns:
List of CICheck results
"""
if head_sha is None:
pr_data = await self._make_request(
"GET",
f"/repos/{self.owner}/{self.repo_name}/pulls/{pr_number}",
)
head_sha = pr_data["head"]["sha"]

data = await self._make_request(
"GET",
f"/repos/{self.owner}/{self.repo_name}/commits/{head_sha}/check-runs",
)
check_runs = data.get("check_runs", []) if isinstance(data, dict) else []
normalized: list[CICheck] = []
for run in check_runs:
if not isinstance(run, dict):
continue
name = run.get("name")
status = run.get("status")
if not name or not status:
logger.warning("Skipping malformed check-run entry: %s", run)
continue
normalized.append(
CICheck(name=name, status=status, conclusion=run.get("conclusion"))
)
return normalized

async def get_pr_review_status(self, pr_number: int) -> str:
"""Get the aggregate review status for a pull request.

Returns "changes_requested" if any reviewer requested changes,
"approved" if any reviewer approved (and none requested changes),
or "pending" if there are no actionable reviews.

Args:
pr_number: PR number

Returns:
"approved" | "changes_requested" | "pending"
"""
reviews = await self._make_request(
"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)
Comment on lines +429 to +436
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Guard review payload type before state aggregation.

At Line 430-Line 431, r.get(...) assumes each review item is a dict. If reviews is non-list or mixed, this can raise and mask the real upstream issue.

Suggested hardening
-        reviews = await self._make_request(
+        reviews_raw = await self._make_request(
             "GET",
             f"/repos/{self.owner}/{self.repo_name}/pulls/{pr_number}/reviews",
         )
-        reviews = reviews or []
+        reviews = reviews_raw if isinstance(reviews_raw, list) else []
 
-        has_changes_requested = any(r.get("state") == "CHANGES_REQUESTED" for r in reviews)
-        has_approved = any(r.get("state") == "APPROVED" for r in reviews)
+        has_changes_requested = any(
+            isinstance(r, dict) and r.get("state") == "CHANGES_REQUESTED"
+            for r in reviews
+        )
+        has_approved = any(
+            isinstance(r, dict) and r.get("state") == "APPROVED"
+            for r in reviews
+        )
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@codeframe/git/github_integration.py` around lines 424 - 431, The reviews
payload from self._make_request may not be a list of dicts, so before computing
has_changes_requested and has_approved (and before any use of reviews)
validate/coerce the response: ensure reviews is a list (default to []) and
filter its items to dicts only (e.g., keep only items where isinstance(item,
dict)); then compute has_changes_requested = any(r.get("state") ==
"CHANGES_REQUESTED" for r in filtered_reviews) and has_approved =
any(r.get("state") == "APPROVED" for r in filtered_reviews) so .get is only
called on dict objects and unexpected payload shapes are guarded.


if has_changes_requested:
return "changes_requested"
if has_approved:
return "approved"
return "pending"

async def close(self) -> None:
"""Close the HTTP client."""
await self._client.aclose()
88 changes: 88 additions & 0 deletions codeframe/ui/routers/pr_v2.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
POST /api/v2/pr/{number}/close - Close a PR without merging
"""

import asyncio
import logging
from typing import Optional

Expand Down Expand Up @@ -77,6 +78,24 @@ class MergeResponse(BaseModel):
message: str


class CICheckResponse(BaseModel):
"""A single CI check run result."""

name: str
status: str
conclusion: Optional[str]


class PRStatusResponse(BaseModel):
"""Live PR status: CI checks, review status, and merge state."""

ci_checks: list[CICheckResponse]
review_status: str # "approved" | "changes_requested" | "pending"
merge_state: str # "open" | "merged" | "closed"
pr_url: str
pr_number: int


# ============================================================================
# Helper Functions
# ============================================================================
Expand Down Expand Up @@ -124,6 +143,75 @@ def _get_github_client() -> GitHubIntegration:
# ============================================================================


@router.get("/status", response_model=PRStatusResponse)
@rate_limit_standard()
async def get_pr_status(
request: Request,
pr_number: int = Query(..., description="PR number to poll"),
workspace: Workspace = Depends(get_v2_workspace),
) -> PRStatusResponse:
"""Get live PR status: CI checks, review status, and merge state.

Polls the GitHub API for the given PR number and returns a snapshot
of all three status dimensions. The frontend polls this every 30 s
and stops when merge_state is merged or closed.

Args:
pr_number: PR number to inspect
workspace: v2 Workspace (for context)

Returns:
PRStatusResponse with CI checks, review status, and merge state
"""
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"]
Comment on lines +174 to +176
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Validate required PR fields before deriving status fields.

Line 174-Line 176 directly indexes pr_raw; malformed-but-200 responses will raise KeyError and return generic 500s. Prefer explicit validation and a controlled error.

Suggested fix
-        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"]
+        head_sha = (pr_raw.get("head") or {}).get("sha") if isinstance(pr_raw, dict) else None
+        pr_url = pr_raw.get("html_url") if isinstance(pr_raw, dict) else None
+        pr_state = pr_raw.get("state") if isinstance(pr_raw, dict) else None
+        if not head_sha or not pr_url or pr_state not in {"open", "closed"}:
+            raise HTTPException(
+                status_code=502,
+                detail=api_error(
+                    "Invalid GitHub PR payload",
+                    ErrorCodes.EXECUTION_FAILED,
+                    f"PR #{pr_number} missing required fields",
+                ),
+            )
+        merge_state: str = "merged" if pr_raw.get("merged_at") else pr_state
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@codeframe/ui/routers/pr_v2.py` around lines 174 - 176, The code directly
indexes pr_raw to derive head_sha, pr_url, and merge_state which will raise
KeyError on malformed responses; before using pr_raw["head"]["sha"],
pr_raw["html_url"], and pr_raw.get("merged_at")/["state"], explicitly validate
the required fields exist (e.g., ensure "head" is present and is a dict with
"sha", and "html_url" and "state" keys exist), or use safe lookups like
pr_raw.get("head", {}).get("sha") and pr_raw.get("html_url") and if any required
value is missing raise a clear, controlled error/HTTPException (with a helpful
message and status code) or return a validation error response rather than
letting a KeyError bubble up; make this check immediately before computing
head_sha, pr_url, and merge_state so callers receive deterministic error
handling.


# Fetch CI checks and reviews in parallel (2 more GitHub API calls).
ci_checks, review_status = await asyncio.gather(
client.get_pr_ci_checks(pr_number, head_sha=head_sha),
client.get_pr_review_status(pr_number),
)

return PRStatusResponse(
ci_checks=[
CICheckResponse(name=c.name, status=c.status, conclusion=c.conclusion)
for c in ci_checks
],
review_status=review_status,
merge_state=merge_state,
pr_url=pr_url,
pr_number=pr_number,
)

except GitHubAPIError as e:
if e.status_code == 404:
raise HTTPException(
status_code=404,
detail=api_error("PR not found", ErrorCodes.NOT_FOUND, f"No PR #{pr_number}"),
)
raise HTTPException(
status_code=e.status_code,
detail=api_error("GitHub API error", ErrorCodes.EXECUTION_FAILED, e.message),
)
except HTTPException:
raise
except Exception as e:
logger.error(f"Failed to get PR #{pr_number} status: {e}", exc_info=True)
raise HTTPException(
status_code=500,
detail=api_error("Failed to get PR status", ErrorCodes.EXECUTION_FAILED, str(e)),
)

Comment on lines +167 to +213
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Close GitHubIntegration client in this endpoint to prevent connection leaks.

At Line 167, a new client is created but never closed. Since GitHubIntegration wraps httpx.AsyncClient, repeated polling can accumulate open connections.

Suggested fix
 async def get_pr_status(
@@
 ) -> PRStatusResponse:
@@
-    try:
-        client = _get_github_client()
+    client: Optional[GitHubIntegration] = None
+    try:
+        client = _get_github_client()
@@
     except Exception as e:
         logger.error(f"Failed to get PR #{pr_number} status: {e}", exc_info=True)
         raise HTTPException(
             status_code=500,
             detail=api_error("Failed to get PR status", ErrorCodes.EXECUTION_FAILED, str(e)),
         )
+    finally:
+        if client is not None:
+            await client.close()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@codeframe/ui/routers/pr_v2.py` around lines 167 - 213, The GitHubIntegration
client returned by _get_github_client() is never closed; wrap its usage in an
async context manager or ensure it's closed in a finally block to avoid leaking
httpx connections. Replace the direct assignment client = _get_github_client()
with either "async with _get_github_client() as client:" around the logic that
calls client.get_pr_ci_checks and client.get_pr_review_status, or add a
try/finally where you call await client.aclose() in the finally; ensure this
runs for all code paths including the GitHubAPIError and generic Exception
handlers so the GitHubIntegration/httpx AsyncClient is always closed.


@router.get("", response_model=PRListResponse)
@rate_limit_standard()
async def list_pull_requests(
Expand Down
Loading
Loading