From 1cc94d1daa1daffe7d55a87f8685a0d61636dd50 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Thu, 16 Jul 2026 23:04:01 -0700 Subject: [PATCH 1/2] fix: surface GitHub rate limits from find_pr_by_branch find_pr_by_branch lacked the 403/429 -> handle_github_rate_limit check that get_latest_release and fetch_pr_info both have. A rate-limited 403 raised requests.HTTPError from raise_for_status(), which the function's own `except requests.RequestException: return None` swallowed, so callers reported "PR not found" instead of the real rate-limit message. Extract the shared request logic into a module-private _github_get() helper that performs the token auth, the rate-limit check and raise_for_status() in one place, plus _pr_info_from_github() for the PRInfo construction duplicated between fetch_pr_info and find_pr_by_branch. Each caller keeps its own try/except so their divergent error semantics are unchanged. _github_get attaches the user's GITHUB_TOKEN, so it rejects any URL outside https://api.github.com/ to keep the token from leaking to another host. GitHubRateLimitError is a plain Exception, so it now propagates out of find_pr_by_branch; both call sites already wrap the call in `except Exception` and print the message correctly. --- comfy_cli/command/install.py | 97 ++++++++++------------- tests/comfy_cli/command/github/test_pr.py | 21 +++++ 2 files changed, 63 insertions(+), 55 deletions(-) diff --git a/comfy_cli/command/install.py b/comfy_cli/command/install.py index ed5a0c27..ac5a10b0 100755 --- a/comfy_cli/command/install.py +++ b/comfy_cli/command/install.py @@ -410,6 +410,30 @@ def handle_github_rate_limit(response): raise GitHubRateLimitError(message) +def _github_get(url: str, *, params: dict | None = None, timeout: int) -> dict | list: + """GET a GitHub REST API URL with optional GITHUB_TOKEN auth. + + SECURITY: must only ever be called with https://api.github.com/ URLs — + it attaches the user's GITHUB_TOKEN as a Bearer header. Keep module-private. + Raises GitHubRateLimitError on 403/429 rate limits, requests.HTTPError on + other non-2xx, requests.RequestException on network errors. + """ + if not url.startswith("https://api.github.com/"): + raise ValueError(f"_github_get only accepts api.github.com URLs, got: {url}") + + headers = {} + if github_token := os.getenv("GITHUB_TOKEN"): + headers["Authorization"] = f"Bearer {github_token}" + + response = requests.get(url, headers=headers, params=params, timeout=timeout) + + if response.status_code in (403, 429): + handle_github_rate_limit(response) + + response.raise_for_status() + return response.json() + + class GithubRelease(TypedDict): """ A dictionary representing a GitHub release. @@ -598,19 +622,8 @@ def get_latest_release(repo_owner: str, repo_name: str) -> GithubRelease | None: """ url = f"https://api.github.com/repos/{repo_owner}/{repo_name}/releases/latest" - headers = {} - if github_token := os.getenv("GITHUB_TOKEN"): - headers["Authorization"] = f"Bearer {github_token}" - try: - response = requests.get(url, headers=headers, timeout=5) - - if response.status_code in (403, 429): - handle_github_rate_limit(response) - - response.raise_for_status() - - data = response.json() + data = _github_get(url, timeout=5) # Forks may use non-semver tags (e.g. "release-2026-04"); the caller # only needs the raw tag string for git checkout, so let `version` @@ -672,35 +685,25 @@ def parse_pr_reference(pr_ref: str) -> tuple[str, str, int | None]: return _parse_pr_reference(pr_ref, "comfyanonymous", "ComfyUI") +def _pr_info_from_github(data: dict) -> PRInfo: + return PRInfo( + number=data["number"], + head_repo_url=data["head"]["repo"]["clone_url"], + head_branch=data["head"]["ref"], + base_repo_url=data["base"]["repo"]["clone_url"], + base_branch=data["base"]["ref"], + title=data["title"], + user=data["head"]["repo"]["owner"]["login"], + mergeable=data.get("mergeable", True), + ) + + def fetch_pr_info(repo_owner: str, repo_name: str, pr_number: int) -> PRInfo: url = f"https://api.github.com/repos/{repo_owner}/{repo_name}/pulls/{pr_number}" - headers = {} - if github_token := os.getenv("GITHUB_TOKEN"): - headers["Authorization"] = f"Bearer {github_token}" - try: - response = requests.get(url, headers=headers, timeout=10) - - if response is None: - raise Exception(f"Failed to fetch PR #{pr_number}: No response from GitHub API") - - if response.status_code in (403, 429): - handle_github_rate_limit(response) - - response.raise_for_status() - data = response.json() - - return PRInfo( - number=data["number"], - head_repo_url=data["head"]["repo"]["clone_url"], - head_branch=data["head"]["ref"], - base_repo_url=data["base"]["repo"]["clone_url"], - base_branch=data["base"]["ref"], - title=data["title"], - user=data["head"]["repo"]["owner"]["login"], - mergeable=data.get("mergeable", True), - ) + data = _github_get(url, timeout=10) + return _pr_info_from_github(data) except requests.RequestException as e: raise Exception(f"Failed to fetch PR #{pr_number}: {e}") @@ -710,27 +713,11 @@ def find_pr_by_branch(repo_owner: str, repo_name: str, username: str, branch: st url = f"https://api.github.com/repos/{repo_owner}/{repo_name}/pulls" params = {"head": f"{username}:{branch}", "state": "open"} - headers = {} - if github_token := os.getenv("GITHUB_TOKEN"): - headers["Authorization"] = f"Bearer {github_token}" - try: - response = requests.get(url, headers=headers, params=params, timeout=10) - response.raise_for_status() - data = response.json() + data = _github_get(url, params=params, timeout=10) if data: - pr_data = data[0] - return PRInfo( - number=pr_data["number"], - head_repo_url=pr_data["head"]["repo"]["clone_url"], - head_branch=pr_data["head"]["ref"], - base_repo_url=pr_data["base"]["repo"]["clone_url"], - base_branch=pr_data["base"]["ref"], - title=pr_data["title"], - user=pr_data["head"]["repo"]["owner"]["login"], - mergeable=pr_data.get("mergeable", True), - ) + return _pr_info_from_github(data[0]) return None diff --git a/tests/comfy_cli/command/github/test_pr.py b/tests/comfy_cli/command/github/test_pr.py index 3b7e93e4..86bae287 100644 --- a/tests/comfy_cli/command/github/test_pr.py +++ b/tests/comfy_cli/command/github/test_pr.py @@ -11,6 +11,7 @@ from comfy_cli.command.install import ( GitHubRateLimitError, PRInfo, + _github_get, _parse_github_owner_repo, _resolve_latest_tag_from_local, checkout_stable_comfyui, @@ -176,6 +177,26 @@ def test_find_pr_by_branch_error(self, mock_get): result = find_pr_by_branch("comfyanonymous", "ComfyUI", "testuser", "test-branch") assert result is None + @patch("requests.get") + def test_find_pr_by_branch_rate_limit(self, mock_get): + """A rate-limited 403 surfaces as GitHubRateLimitError, not a silent "no PR found\"""" + mock_response = Mock() + mock_response.status_code = 403 + mock_response.headers = {"x-ratelimit-remaining": "0", "x-ratelimit-reset": "1777415867"} + mock_get.return_value = mock_response + + with pytest.raises(GitHubRateLimitError): + find_pr_by_branch("comfyanonymous", "ComfyUI", "testuser", "test-branch") + + @patch("requests.get") + def test_github_get_rejects_non_github_urls(self, mock_get): + """_github_get attaches GITHUB_TOKEN, so a non-api.github.com URL must not be requested""" + with patch.dict("os.environ", {"GITHUB_TOKEN": "test-token"}): + with pytest.raises(ValueError): + _github_get("https://evil.example.com/x", timeout=5) + + mock_get.assert_not_called() + class TestGitOperations: """Test Git operations for PR checkout""" From 7990d0c3f0da24bbb6695c61b2be85ec64b9db4a Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 17 Jul 2026 01:56:21 -0700 Subject: [PATCH 2/2] fix: handle deleted PR forks and null mergeable in _pr_info_from_github MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both issues pre-date this PR (identical code on main) but were flagged by review on the lines this PR moved into _pr_info_from_github, and both are one-liners in that helper, so fix them here. - head.repo/base.repo come back null once a PR's source fork is deleted. Indexing them raised an opaque TypeError that escaped both callers (they only catch requests.RequestException). Guard and raise a ValueError naming the reason; both call sites already catch Exception and print it, matching this PR's thesis that real failures shouldn't be mislabeled "PR not found". Installing such PRs via the base repo's refs/pull/N/head is a follow-up. - mergeable is null (not absent) while GitHub is still computing it, which rendered a mergeable PR as '✗'. Treat null and absent alike as unknown and keep the pre-existing optimistic True default. Co-Authored-By: Claude Opus 4.8 --- comfy_cli/command/install.py | 19 +++++++++--- tests/comfy_cli/command/github/test_pr.py | 36 +++++++++++++++++++++++ 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/comfy_cli/command/install.py b/comfy_cli/command/install.py index ac5a10b0..fa2ac0a7 100755 --- a/comfy_cli/command/install.py +++ b/comfy_cli/command/install.py @@ -686,15 +686,26 @@ def parse_pr_reference(pr_ref: str) -> tuple[str, str, int | None]: def _pr_info_from_github(data: dict) -> PRInfo: + # GitHub returns head.repo/base.repo as null once the PR's source repo has been + # deleted; indexing into that would raise an opaque TypeError. + head_repo = data["head"].get("repo") + base_repo = data["base"].get("repo") + if head_repo is None or base_repo is None: + raise ValueError(f"PR #{data['number']} cannot be installed: its source repository has been deleted.") + + # Absent (list endpoint) and null (mergeability still being computed) both mean + # "unknown"; keep the pre-existing optimistic default rather than showing ✗. + mergeable = data.get("mergeable") + return PRInfo( number=data["number"], - head_repo_url=data["head"]["repo"]["clone_url"], + head_repo_url=head_repo["clone_url"], head_branch=data["head"]["ref"], - base_repo_url=data["base"]["repo"]["clone_url"], + base_repo_url=base_repo["clone_url"], base_branch=data["base"]["ref"], title=data["title"], - user=data["head"]["repo"]["owner"]["login"], - mergeable=data.get("mergeable", True), + user=head_repo["owner"]["login"], + mergeable=True if mergeable is None else mergeable, ) diff --git a/tests/comfy_cli/command/github/test_pr.py b/tests/comfy_cli/command/github/test_pr.py index 86bae287..1050be5e 100644 --- a/tests/comfy_cli/command/github/test_pr.py +++ b/tests/comfy_cli/command/github/test_pr.py @@ -188,6 +188,42 @@ def test_find_pr_by_branch_rate_limit(self, mock_get): with pytest.raises(GitHubRateLimitError): find_pr_by_branch("comfyanonymous", "ComfyUI", "testuser", "test-branch") + @patch("requests.get") + def test_fetch_pr_info_deleted_fork(self, mock_get): + """A deleted source fork (head.repo = null) reports a clear reason, not a TypeError""" + mock_response = Mock() + mock_response.status_code = 200 + mock_response.json.return_value = { + "number": 123, + "title": "Test PR", + "head": {"repo": None, "ref": "test-branch"}, + "base": {"repo": {"clone_url": "https://github.com/comfyanonymous/ComfyUI.git"}, "ref": "master"}, + "mergeable": True, + } + mock_get.return_value = mock_response + + with pytest.raises(ValueError, match="source repository has been deleted"): + fetch_pr_info("comfyanonymous", "ComfyUI", 123) + + @patch("requests.get") + def test_fetch_pr_info_mergeable_null(self, mock_get): + """A null 'mergeable' (still computing) is unknown, not un-mergeable""" + mock_response = Mock() + mock_response.status_code = 200 + mock_response.json.return_value = { + "number": 123, + "title": "Test PR", + "head": { + "repo": {"clone_url": "https://github.com/testuser/ComfyUI.git", "owner": {"login": "testuser"}}, + "ref": "test-branch", + }, + "base": {"repo": {"clone_url": "https://github.com/comfyanonymous/ComfyUI.git"}, "ref": "master"}, + "mergeable": None, + } + mock_get.return_value = mock_response + + assert fetch_pr_info("comfyanonymous", "ComfyUI", 123).mergeable is True + @patch("requests.get") def test_github_get_rejects_non_github_urls(self, mock_get): """_github_get attaches GITHUB_TOKEN, so a non-api.github.com URL must not be requested"""