Skip to content

Commit 14eb495

Browse files
fix(install): treat throttled GitHub probe as indeterminate, not inaccessible (#2037)
* chore: release v0.24.0 Bump pyproject.toml and uv.lock to 0.24.0 and move the [Unreleased] CHANGELOG block to [0.24.0] - 2026-07-05 (one so-what entry per merged PR). MINOR bump: new `apm config list` alias (#1991) and Antigravity `trigger: glob` frontmatter support (#1984); no BREAKING changes. Lint mirror green locally (ruff check + format, pylint R0801, auth-signals). Post-merge: tag v0.24.0 to trigger the release workflow. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix(install): treat throttled GitHub probe as indeterminate, not inaccessible The pre-flight accessibility probe treated a rate-limited 403/429 (primary 60/hr or secondary concurrency limit) as 'package not accessible' and aborted the install -- a false negative, since a throttled response is no evidence the repo is missing. On single-IP runners that concentrate many public-package installs (the consolidated macOS release job), this made the release/nightly integration suite flake red for days. Detect a genuine rate-limit response (403 + X-RateLimit-Remaining: 0, or 403/429 + Retry-After) and let the install proceed so the download step becomes the source of truth. Genuine 404s and permission 403s still fail closed. Adds unit coverage for the helpers and the allow-through / fail-closed end-to-end paths. Lint mirror green. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * chore: spec-conformance waiver for rate-limit probe hardening apm-spec-waiver: robustness bugfix -- throttled GitHub probe no longer false-negatives a public package; existing install existence-validation contract is unchanged (no new normative behaviour) --------- Co-authored-by: danielmeppiel <danielmeppiel@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent a8a2393 commit 14eb495

3 files changed

Lines changed: 192 additions & 0 deletions

File tree

CHANGELOG.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
5151

5252
### Fixed
5353

54+
- `apm install` no longer aborts with a false "package not accessible" error
55+
when GitHub's API rate limit (primary 60/hr or secondary concurrency) throttles
56+
the pre-flight probe; a throttled response is now treated as indeterminate and
57+
the download step confirms the package. Genuine 404s and permission 403s still
58+
fail closed. (#2032)
5459
- Generated `CLAUDE.md` now uses a single H1 title with nested H2/H3 sections,
5560
fixing duplicate top-level headings in Claude Code. (by @WilliamK112, #2016)
5661
- `apm install` now deploys hook script directories as self-contained bundles

src/apm_cli/install/validation.py

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,54 @@ def _is_tls_failure(exc: BaseException) -> bool:
5858
return False
5959

6060

61+
# Marker prefix used on RuntimeError messages raised when the GitHub REST
62+
# probe is throttled (primary 60/hr or secondary concurrency rate limit).
63+
# A throttled response is NOT evidence the repo is missing, so the marker
64+
# lets the caller proceed to the download step (the real source of truth)
65+
# instead of surfacing a false "package not accessible" error.
66+
_RATE_LIMIT_PREFIX = "GitHub API rate limit"
67+
68+
69+
def _is_rate_limit_failure(exc: BaseException) -> bool:
70+
"""Return True if exc (or any cause in its chain) is a rate-limit throttle."""
71+
cur: BaseException | None = exc
72+
seen = 0
73+
while cur is not None and seen < 8:
74+
if _RATE_LIMIT_PREFIX in str(cur):
75+
return True
76+
cur = cur.__cause__ or cur.__context__
77+
seen += 1
78+
return False
79+
80+
81+
def _raise_if_rate_limited(resp, host_display: str) -> None:
82+
"""Raise a marked RuntimeError when *resp* is a GitHub rate-limit throttle.
83+
84+
GitHub signals primary exhaustion with HTTP 403 + ``X-RateLimit-Remaining: 0``
85+
and secondary (concurrency) limits with 403/429 + a ``Retry-After`` header.
86+
Either way the repo's existence is unknown, so the marker lets the caller
87+
proceed rather than report a false negative. Plain 403s (SSO / permission)
88+
carry neither signal and fall through to the normal not-accessible path.
89+
"""
90+
if resp.status_code not in (403, 429):
91+
return
92+
remaining = resp.headers.get("X-RateLimit-Remaining")
93+
retry_after = resp.headers.get("Retry-After")
94+
if resp.status_code == 429 or remaining == "0" or retry_after:
95+
raise RuntimeError(f"{_RATE_LIMIT_PREFIX} hit for {host_display} ({resp.status_code})")
96+
97+
98+
def _log_rate_limit_allow(host_display: str, verbose_log, logger) -> None:
99+
"""Note that a throttled probe is allowed through to the download step."""
100+
if logger:
101+
logger.info(
102+
f"GitHub API rate limit hit while checking {host_display}; skipping the "
103+
"pre-flight accessibility probe and letting the download step confirm the package"
104+
)
105+
if verbose_log:
106+
verbose_log(f"rate limit reached for {host_display}; proceeding to download")
107+
108+
61109
def _log_tls_failure(host_display: str, exc: BaseException, verbose_log, logger) -> None:
62110
"""Surface a TLS verification failure with an actionable CA-trust hint.
63111
@@ -593,6 +641,7 @@ def _check_repo(token, git_env) -> bool:
593641
if resp.status_code == 404 and token:
594642
# 404 with token could mean no access -- raise to trigger fallback
595643
raise RuntimeError(f"API returned {resp.status_code}")
644+
_raise_if_rate_limited(resp, host_info.display_name)
596645
raise RuntimeError(f"API returned {resp.status_code}: {resp.reason}")
597646

598647
try:
@@ -612,6 +661,9 @@ def _check_repo(token, git_env) -> bool:
612661
if _is_tls_failure(exc):
613662
_log_tls_failure(host_info.display_name, exc, verbose_log, logger)
614663
return False
664+
if _is_rate_limit_failure(exc):
665+
_log_rate_limit_allow(host_info.display_name, verbose_log, logger)
666+
return True
615667
if verbose_log:
616668
try:
617669
ctx = auth_resolver.build_error_context(
@@ -687,6 +739,7 @@ def _check_repo_fallback(token, git_env) -> bool:
687739
return True
688740
if verbose_log:
689741
verbose_log(f"API fallback -> {resp.status_code} {resp.reason}")
742+
_raise_if_rate_limited(resp, host_info.display_name)
690743
raise RuntimeError(f"API returned {resp.status_code}")
691744

692745
try:
@@ -703,6 +756,9 @@ def _check_repo_fallback(token, git_env) -> bool:
703756
# See note above: logged once here, skip auth context render.
704757
_log_tls_failure(host, exc, verbose_log, logger)
705758
return False
759+
if _is_rate_limit_failure(exc):
760+
_log_rate_limit_allow(host, verbose_log, logger)
761+
return True
706762
if verbose_log:
707763
try:
708764
ctx = auth_resolver.build_error_context(
Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
"""Tests for the GitHub API rate-limit classification path in install.validation.
2+
3+
Bug: on a single-IP runner that concentrates many public-package installs
4+
(e.g. the consolidated macOS release job), the accessibility probe hits
5+
GitHub's primary (60/hr) or secondary (concurrency) rate limit and gets a
6+
403/429. The old code treated that as "package not accessible" and aborted
7+
the install -- a false negative, since a throttled response is no evidence
8+
the repo is missing. After the fix a throttled probe is allowed through so
9+
the download step becomes the source of truth, while genuine 404s and plain
10+
permission 403s still fail closed.
11+
"""
12+
13+
from __future__ import annotations
14+
15+
from unittest.mock import MagicMock, patch
16+
17+
import pytest
18+
19+
from apm_cli.install import validation
20+
21+
22+
class TestRateLimitHelpers:
23+
def test_is_rate_limit_failure_detects_marker(self):
24+
exc = RuntimeError("GitHub API rate limit hit for github.com (403)")
25+
assert validation._is_rate_limit_failure(exc) is True
26+
27+
def test_is_rate_limit_failure_detects_via_cause_chain(self):
28+
original = RuntimeError("GitHub API rate limit hit for github.com (429)")
29+
wrapped = RuntimeError("authenticated retry also failed")
30+
wrapped.__cause__ = original
31+
assert validation._is_rate_limit_failure(wrapped) is True
32+
33+
def test_is_rate_limit_failure_false_for_generic_errors(self):
34+
assert validation._is_rate_limit_failure(RuntimeError("API returned 404")) is False
35+
assert validation._is_rate_limit_failure(ValueError("nope")) is False
36+
37+
def test_is_rate_limit_failure_bounded_chain_walk(self):
38+
exc = RuntimeError("oops")
39+
exc.__cause__ = exc
40+
assert validation._is_rate_limit_failure(exc) is False
41+
42+
def test_raise_if_rate_limited_primary_exhaustion(self):
43+
resp = MagicMock(status_code=403, headers={"X-RateLimit-Remaining": "0"})
44+
with pytest.raises(RuntimeError, match=r"GitHub API rate limit"):
45+
validation._raise_if_rate_limited(resp, "github.com")
46+
47+
def test_raise_if_rate_limited_secondary_retry_after(self):
48+
resp = MagicMock(status_code=403, headers={"Retry-After": "60"})
49+
with pytest.raises(RuntimeError, match=r"GitHub API rate limit"):
50+
validation._raise_if_rate_limited(resp, "github.com")
51+
52+
def test_raise_if_rate_limited_http_429(self):
53+
resp = MagicMock(status_code=429, headers={})
54+
with pytest.raises(RuntimeError, match=r"GitHub API rate limit"):
55+
validation._raise_if_rate_limited(resp, "github.com")
56+
57+
def test_raise_if_rate_limited_ignores_plain_403(self):
58+
# SSO / permission 403 carries no rate-limit signal -> no raise.
59+
resp = MagicMock(status_code=403, headers={"X-RateLimit-Remaining": "4900"})
60+
validation._raise_if_rate_limited(resp, "github.com")
61+
62+
def test_raise_if_rate_limited_ignores_404(self):
63+
resp = MagicMock(status_code=404, headers={"Retry-After": "60"})
64+
validation._raise_if_rate_limited(resp, "github.com")
65+
66+
67+
class TestValidateRateLimitClassification:
68+
"""End-to-end: a throttled probe returns True (allow-through)."""
69+
70+
def _setup_resolver(self, token=None):
71+
resolver = MagicMock()
72+
host_info = MagicMock()
73+
host_info.api_base = "https://api.github.com"
74+
host_info.display_name = "github.com"
75+
host_info.kind = "github"
76+
host_info.has_public_repos = True
77+
resolver.classify_host.return_value = host_info
78+
ctx = MagicMock(source="env", token_type="pat", token=token)
79+
resolver.resolve.return_value = ctx
80+
resolver.resolve_for_dep.return_value = ctx
81+
return resolver
82+
83+
def test_throttled_probe_allows_through(self):
84+
"""Both anon and token attempts hit the limit -> proceed to download."""
85+
resolver = self._setup_resolver(token="ghp_fake")
86+
87+
def _throttled_fallback(host, op, **kwargs):
88+
try:
89+
return op(None, {})
90+
except Exception:
91+
# Authenticated retry also throttled; exception propagates.
92+
return op("ghp_fake", {})
93+
94+
resolver.try_with_fallback.side_effect = _throttled_fallback
95+
96+
logger = MagicMock()
97+
rate_limited = MagicMock(
98+
ok=False,
99+
status_code=403,
100+
reason="Forbidden",
101+
headers={"X-RateLimit-Remaining": "0"},
102+
)
103+
104+
with patch("apm_cli.install.validation.requests.get", return_value=rate_limited):
105+
result = validation._validate_package_exists(
106+
"octocat/hello-world",
107+
verbose=False,
108+
auth_resolver=resolver,
109+
logger=logger,
110+
)
111+
112+
assert result is True
113+
114+
def test_plain_403_still_returns_false(self):
115+
"""Permission 403 (no rate-limit signal) must still fail closed."""
116+
resolver = self._setup_resolver()
117+
resolver.try_with_fallback.side_effect = lambda host, op, **kw: op(None, {})
118+
119+
forbidden = MagicMock(
120+
ok=False,
121+
status_code=403,
122+
reason="Forbidden",
123+
headers={"X-RateLimit-Remaining": "4900"},
124+
)
125+
126+
with patch("apm_cli.install.validation.requests.get", return_value=forbidden):
127+
result = validation._validate_package_exists(
128+
"octocat/private-repo", verbose=False, auth_resolver=resolver
129+
)
130+
131+
assert result is False

0 commit comments

Comments
 (0)