fix(contributor-check): retry transient GitHub API failures within a bound

.github/actions/contributor-check/tests/test_api_retry.py

@@ -0,0 +1,153 @@
+"""Regression tests for the bounded retry in vendor/agt/contributor_check.py `_api`.
+
+Covers agentrust-io/.github#27 item 1. The rule under test: retry transient
+failures within a bound, and report everything else as itself. A 404 is an
+answer. A rate limit that outlasts the bound is still a rate limit.
+"""
+from __future__ import annotations
+
+import importlib.util
+import io
+import sys
+from pathlib import Path
+from urllib.error import HTTPError, URLError
+
+import pytest
+
+VENDOR = Path(__file__).resolve().parents[1] / "vendor" / "agt"
+
+
+def _load():
+    spec = importlib.util.spec_from_file_location(
+        "agt_contributor_check", VENDOR / "contributor_check.py"
+    )
+    mod = importlib.util.module_from_spec(spec)
+    sys.modules[spec.name] = mod
+    spec.loader.exec_module(mod)
+    return mod
+
+
+@pytest.fixture()
+def cc(monkeypatch):
+    mod = _load()
+    monkeypatch.setattr(mod, "_get_token", lambda: "t")
+    monkeypatch.setattr(mod.time, "sleep", lambda _s: None)
+    return mod
+
+
+class _Resp:
+    def __init__(self, payload: bytes):
+        self._payload = payload
+
+    def read(self):
+        return self._payload
+
+    def __enter__(self):
+        return self
+
+    def __exit__(self, *a):
+        return False
+
+
+def _http(code: int, headers: dict | None = None) -> HTTPError:
+    return HTTPError("https://api.github.com/x", code, "err", headers or {}, None)
+
+
+def _seq(monkeypatch, cc, outcomes):
+    """urlopen that yields each outcome in turn; raises if an exception."""
+    calls = {"n": 0}
+
+    def fake(_req, timeout=None):
+        i = calls["n"]
+        calls["n"] += 1
+        out = outcomes[min(i, len(outcomes) - 1)]
+        if isinstance(out, Exception):
+            raise out
+        return _Resp(out)
+
+    monkeypatch.setattr(cc, "urlopen", fake)
+    return calls
+
+
+def test_5xx_retries_then_succeeds(cc, monkeypatch):
+    calls = _seq(monkeypatch, cc, [_http(502), _http(502), b'{"ok":1}'])
+    assert cc._api("/x") == {"ok": 1}
+    assert calls["n"] == 3
+
+
+def test_urlerror_retries_then_succeeds(cc, monkeypatch):
+    calls = _seq(monkeypatch, cc, [URLError("timed out"), b'{"ok":1}'])
+    assert cc._api("/x") == {"ok": 1}
+    assert calls["n"] == 2
+
+
+def test_404_returns_none_without_retrying(cc, monkeypatch):
+    calls = _seq(monkeypatch, cc, [_http(404)])
+    assert cc._api("/x") is None
+    assert calls["n"] == 1, "a 404 is an answer and must not be retried"
+
+
+def test_rate_limit_exhausted_reports_as_a_rate_limit(cc, monkeypatch, capsys):
+    calls = _seq(monkeypatch, cc, [_http(403, {"Retry-After": "7"})])
+    with pytest.raises(HTTPError):
+        cc._api("/x")
+    assert calls["n"] == cc._RETRY_MAX_ATTEMPTS
+    last = capsys.readouterr().err.strip().splitlines()[-1]
+    # Must be the final cause line, not one of the "waiting Ns..." lines that
+    # also contain "Rate limited". contributor_check_action.py reports the last
+    # stderr line, so that line is what a maintainer sees.
+    assert "still limited after" in last, (
+        f"last stderr line must name the exhausted rate limit, got {last!r}"
+    )
+
+
+def test_server_error_exhausted_reports_as_a_server_error(cc, monkeypatch, capsys):
+    calls = _seq(monkeypatch, cc, [_http(503)])
+    with pytest.raises(HTTPError):
+        cc._api("/x")
+    assert calls["n"] == cc._RETRY_MAX_ATTEMPTS
+    last = capsys.readouterr().err.strip().splitlines()[-1]
+    assert "503" in last and "attempts" in last
+
+
+def test_network_error_exhausted_reports_as_a_network_error(cc, monkeypatch, capsys):
+    calls = _seq(monkeypatch, cc, [URLError("dns failure")])
+    with pytest.raises(URLError):
+        cc._api("/x")
+    assert calls["n"] == cc._RETRY_MAX_ATTEMPTS
+    last = capsys.readouterr().err.strip().splitlines()[-1]
+    # Not just "Network error": the per-attempt "retrying in Xs..." lines say
+    # that too, so a bare substring check passes even with the final line gone.
+    assert "on all" in last and str(cc._RETRY_MAX_ATTEMPTS) in last, (
+        f"last stderr line must name the exhausted network error, got {last!r}"
+    )
+
+
+def test_other_http_status_raises_on_first_attempt(cc, monkeypatch):
+    calls = _seq(monkeypatch, cc, [_http(422)])
+    with pytest.raises(HTTPError):
+        cc._api("/x")
+    assert calls["n"] == 1, "a 422 is the API answering and must not be retried"
+
+
+def test_backoff_is_bounded_and_jittered(cc):
+    for attempt in range(cc._RETRY_MAX_ATTEMPTS):
+        for _ in range(50):
+            s = cc._retry_sleep_seconds(attempt)
+            assert 0 <= s <= cc._RETRY_MAX_SLEEP_SECONDS
+
+
+def test_backoff_cap_binds_above_the_curve(cc):
+    # Across the attempts the loop actually uses, the raw curve stays under the
+    # cap, so those attempts cannot show whether the cap is applied at all.
+    # Exercise it at an attempt where it has to bite.
+    attempt = 30
+    assert cc._RETRY_BASE_SECONDS * (2 ** attempt) > cc._RETRY_MAX_SLEEP_SECONDS
+    for _ in range(200):
+        assert cc._retry_sleep_seconds(attempt) <= cc._RETRY_MAX_SLEEP_SECONDS
+
+
+def test_backoff_is_not_a_constant(cc):
+    # Full jitter, so a fleet of runners hitting the same limit does not retry
+    # in lockstep. A fixed attempt must not return one value.
+    assert len({cc._retry_sleep_seconds(2) for _ in range(50)}) > 1

.github/actions/contributor-check/vendor/agt/README.md

@@ -16,16 +16,19 @@ level. See agentrust-io/.github#27.
 
 | File | Upstream path | SHA-256 |
 |---|---|---|
-| `contributor_check.py` | `scripts/contributor_check.py` | `f1c1179c921f59f836e85355b2efdf8641f65b763516f05853f4d216e70ae164` |
+| `contributor_check.py` | `scripts/contributor_check.py` | modified, see below |
 | `contributor_check_allowlist.json` | `scripts/contributor_check_allowlist.json` | `d8a5ddbb0695c21b3eae88e6bc85edcda6cbd9721cad3ca9d10fbeeed4fc0636` |
 | `credential_audit.py` | `scripts/credential_audit.py` | `9650675038e7720a6d5cdd8be058fa38b2e004cc1e6f7bc81c48ed100e4680e1` |
 | `cluster_detect.py` | `scripts/cluster_detect.py` | `da36c10074aa638bf61c5873fbe6cff975533563553689eba5e90329e3d8d7cf` |
 | `contributor_check_action.py` | `scripts/contributor_check_action.py` | modified, see below |
 
-The first four are byte-identical to upstream at that commit. The checksums
-above are the upstream bytes, so `sha256sum` here is a one-line verification.
+The first three unmodified files are byte-identical to upstream at that commit.
+The checksums above are the upstream bytes, so `sha256sum` here is a one-line
+verification for those three.
 
-## The one local modification
+## The two local modifications
+
+### `contributor_check_action.py`
 
 `contributor_check_action.py` differs from upstream in `_run_check` and in the
 block of `main()` that calls it. `_run_check` now reads the subprocess return
@@ -36,6 +39,31 @@ contributor-risk level `UNKNOWN`.
 `UNKNOWN` still means what its upstream comment says it means: a check that ran
 and could not determine an answer. That case is unchanged and still labels.
 
+### `contributor_check.py`
+
+`_api` now carries a bounded retry. Upstream loops three times but only a 403
+retries; a 5xx raises on the first attempt and `URLError` is not imported at
+all, so the `timeout=15` socket timeout propagates straight through the loop.
+That is the failure behind agentrust-io/.github#27. Since the
+`contributor_check_action.py` change above, it no longer mislabels a
+contributor; it fails the action instead. A single transient blip still ends a
+check that a retry would have passed, which is what this change addresses.
+
+The retry is limited to transient failures and reports anything else as itself:
+
+| Condition | Behaviour |
+|---|---|
+| 404 | Returns `None`. An answer, never retried. |
+| 403 rate limit | Honours `Retry-After`, clamped to 5-60s, within the attempt bound. |
+| 5xx | Full-jitter exponential backoff within the attempt bound. |
+| `URLError` | Full-jitter exponential backoff within the attempt bound. |
+| Any other HTTP status | Raised on the first attempt. |
+
+When a retryable condition outlasts the bound, the final line written to stderr
+names the cause before the exception propagates. `contributor_check_action.py`
+reports the last stderr line, so a rate limit surfaces as a rate limit and a
+server error as a server error rather than as an unattributed execution fault.
+
 ## Why more than two files
 
 `contributor_check_action.py` resolves the scripts it runs as siblings of
@@ -51,8 +79,12 @@ than a silent `UNKNOWN`. All five move together or none of them do.
 
 ## Updating
 
-Re-copy from the same upstream paths at a newer commit, update the commit SHA
-and the checksums above, and re-apply the `_run_check` modification. Upstream
-has a known divergence between `scripts/` and the packaged
-`agent_compliance/cli/` copies (AGT#3571), so check which copy carries the
-retry behaviour before advancing.
+These files are no longer tracked against upstream. Per the decision recorded
+on agentrust-io/.github#27, AGT is not being invested in and this copy is owned
+here. The commit above is provenance, not a sync point, and no resync is
+planned.
+
+Change these files directly, as you would any other file in this repository.
+If a change lands in one of the three that remain byte-identical, move it out of
+the checksum table above and out of the `vendor-integrity` job, and describe the
+change here, the way the two modified files are described.

.github/actions/contributor-check/vendor/agt/contributor_check.py

@@ -21,14 +21,15 @@
 import json
 import math
 import os
+import random
 import subprocess
 import sys
 import time
 from dataclasses import dataclass, field
 from datetime import datetime, timezone
 from pathlib import Path
 from typing import Any
-from urllib.error import HTTPError
+from urllib.error import HTTPError, URLError
 from urllib.parse import quote
 from urllib.request import Request, urlopen
 
@@ -67,6 +68,22 @@ def _get_token() -> str:
     return token
 
 
+# Bounded retry, transient failures only. A 404 is an answer and is never
+# retried. A rate limit and a server error are retried within the bound and,
+# if they outlast it, are reported as themselves: the final stderr line names
+# the cause, which is what contributor_check_action.py surfaces when a check
+# cannot be executed. See agentrust-io/.github#27.
+_RETRY_MAX_ATTEMPTS = 3
+_RETRY_BASE_SECONDS = 1.0
+_RETRY_MAX_SLEEP_SECONDS = 60.0
+
+
+def _retry_sleep_seconds(attempt: int) -> float:
+    """Full-jitter exponential backoff: random in [0, base * 2**attempt)."""
+    upper = min(_RETRY_BASE_SECONDS * (2 ** attempt), _RETRY_MAX_SLEEP_SECONDS)
+    return random.uniform(0, upper)
+
+
 def _api(path: str, params: dict[str, str] | None = None) -> Any:
     """Call the GitHub REST API and return parsed JSON."""
     url = f"https://api.github.com{path}"
@@ -79,19 +96,65 @@ def _api(path: str, params: dict[str, str] | None = None) -> Any:
     req.add_header("Accept", "application/vnd.github+json")
     req.add_header("X-GitHub-Api-Version", "2022-11-28")
 
-    for attempt in range(3):
+    last = _RETRY_MAX_ATTEMPTS - 1
+    for attempt in range(_RETRY_MAX_ATTEMPTS):
         try:
             with urlopen(req, timeout=15) as resp:
                 return json.loads(resp.read())
         except HTTPError as exc:
-            if exc.code == 403 and attempt < 2:
-                wait = int(exc.headers.get("Retry-After", "10"))
-                wait = min(max(wait, 5), 60)
-                print(f"  Rate limited, waiting {wait}s...", file=sys.stderr)
-                import time; time.sleep(wait)
-                continue
+            # 404 is an answer, not a failure. Never retried.
             if exc.code == 404:
                 return None
+            # GitHub's documented rate-limit envelope: 403 with Retry-After.
+            # Honour it, clamped to a sane band.
+            if exc.code == 403:
+                if attempt < last:
+                    wait = int(exc.headers.get("Retry-After", "10"))
+                    wait = min(max(wait, 5), 60)
+                    print(f"  Rate limited, waiting {wait}s...", file=sys.stderr)
+                    time.sleep(wait)
+                    continue
+                print(
+                    f"  Rate limited by the GitHub API and still limited after "
+                    f"{_RETRY_MAX_ATTEMPTS} attempts",
+                    file=sys.stderr,
+                )
+                raise
+            # 5xx is transient on GitHub's side.
+            if 500 <= exc.code < 600:
+                if attempt < last:
+                    wait = _retry_sleep_seconds(attempt)
+                    print(
+                        f"  Server error {exc.code}, retrying in {wait:.1f}s...",
+                        file=sys.stderr,
+                    )
+                    time.sleep(wait)
+                    continue
+                print(
+                    f"  GitHub API returned {exc.code} on all "
+                    f"{_RETRY_MAX_ATTEMPTS} attempts",
+                    file=sys.stderr,
+                )
+                raise
+            # Anything else is the API answering. Report it as itself.
+            raise
+        except URLError as exc:
+            # Network-layer failures: DNS, TCP reset, TLS abort, socket
+            # timeout. The timeout=15 above raises here, which is the case
+            # that produced the UNKNOWN labels in #27.
+            if attempt < last:
+                wait = _retry_sleep_seconds(attempt)
+                print(
+                    f"  Network error ({exc.reason}), retrying in {wait:.1f}s...",
+                    file=sys.stderr,
+                )
+                time.sleep(wait)
+                continue
+            print(
+                f"  Network error ({exc.reason}) on all "
+                f"{_RETRY_MAX_ATTEMPTS} attempts",
+                file=sys.stderr,
+            )
             raise
 
 

.github/workflows/contributor-check-tests.yml

@@ -35,11 +35,10 @@ jobs:
       - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
         with:
           persist-credentials: false
-      - name: Vendored files match the checksums recorded in vendor/agt/README.md
+      - name: Unmodified vendored files match the checksums in vendor/agt/README.md
         run: |
           cd .github/actions/contributor-check/vendor/agt
           sha256sum -c --strict - <<'SUMS'
-          f1c1179c921f59f836e85355b2efdf8641f65b763516f05853f4d216e70ae164  contributor_check.py
           9650675038e7720a6d5cdd8be058fa38b2e004cc1e6f7bc81c48ed100e4680e1  credential_audit.py
           da36c10074aa638bf61c5873fbe6cff975533563553689eba5e90329e3d8d7cf  cluster_detect.py
           d8a5ddbb0695c21b3eae88e6bc85edcda6cbd9721cad3ca9d10fbeeed4fc0636  contributor_check_allowlist.json