diff --git a/.github/actions/pr-review/scripts/fetch-pr-context.py b/.github/actions/pr-review/scripts/fetch-pr-context.py index 78a3aa3..24c7aa5 100644 --- a/.github/actions/pr-review/scripts/fetch-pr-context.py +++ b/.github/actions/pr-review/scripts/fetch-pr-context.py @@ -14,16 +14,19 @@ import re import subprocess import sys +import time from typing import Optional REVIEW_STATE_PATTERN = re.compile( r"", re.DOTALL ) +HTTP_STATUS_PATTERN = re.compile(r"HTTP\s+(\d{3})") # Bot logins that post review comments via GitHub Actions. BOT_LOGINS = {"github-actions[bot]", "github-actions"} DEFAULT_REVIEW_SUMMARY_HEADING = "### Connector PR Review:" LEGACY_REVIEW_SUMMARY_HEADING = "### PR Review:" +DEFAULT_API_ATTEMPTS = 3 def review_comment_heading(comment: dict, summary_heading: str) -> Optional[str]: @@ -47,18 +50,75 @@ def is_legacy_review_comment(comment: dict, summary_heading: str) -> bool: return review_comment_heading(comment, summary_heading) == LEGACY_REVIEW_SUMMARY_HEADING +def command_error_summary(error: subprocess.CalledProcessError) -> str: + detail = (error.stderr or error.stdout or "").strip() + if not detail: + detail = f"exit status {error.returncode}" + return detail.splitlines()[-1] + + +def error_http_status(error: subprocess.CalledProcessError) -> Optional[int]: + match = HTTP_STATUS_PATTERN.search(error.stderr or error.stdout or "") + if not match: + return None + return int(match.group(1)) + + +def retry_limit_for_error( + error: subprocess.CalledProcessError, + attempts: int, +) -> int: + status = error_http_status(error) + if status == 404: + return min(attempts, 2) + if status in (408, 429) or (status is not None and status >= 500): + return attempts + if status is None: + return attempts + return 1 + + +def gh_api(args: list[str], *, attempts: int = DEFAULT_API_ATTEMPTS) -> subprocess.CompletedProcess: + """Run gh api with a small retry window for transient GitHub API failures.""" + last_error = None + for attempt in range(1, attempts + 1): + try: + return subprocess.run( + ["gh", "api", *args], + capture_output=True, + text=True, + check=True, + ) + except subprocess.CalledProcessError as e: + last_error = e + retry_limit = retry_limit_for_error(e, attempts) + e.retry_limit = retry_limit + if attempt >= retry_limit: + break + print( + "::warning::GitHub API request failed; " + f"retrying ({attempt}/{retry_limit}): gh api {' '.join(args)}: " + f"{command_error_summary(e)}", + file=sys.stderr, + ) + time.sleep(attempt) + raise last_error + + def gh_api_paginate(endpoint: str) -> list[dict]: """Fetch all pages from a gh api endpoint.""" - result = subprocess.run( - ["gh", "api", endpoint, "--paginate"], - capture_output=True, - text=True, - check=True, + result = gh_api( + [endpoint, "--paginate"], ) + return parse_paginated_json(result.stdout) + + +def parse_paginated_json(output: str) -> list[dict]: + """Parse gh api --paginate output.""" # --paginate concatenates JSON arrays; each page is a JSON array # Parse by finding all top-level arrays entries = [] - for line in result.stdout.strip().splitlines(): + for line in output.strip().splitlines(): line = line.strip() if not line: continue @@ -73,7 +133,7 @@ def gh_api_paginate(endpoint: str) -> list[dict]: # If the whole output is a single JSON array, handle that too if not entries: try: - entries = json.loads(result.stdout) + entries = json.loads(output) except json.JSONDecodeError: pass return entries @@ -83,12 +143,7 @@ def fetch_compare_diff(head_repo: str, base_sha: str, head_sha: str) -> Optional """Fetch a compare diff from the PR head repo without checking out PR code.""" endpoint = f"repos/{head_repo}/compare/{base_sha}...{head_sha}" try: - metadata = subprocess.run( - ["gh", "api", endpoint], - capture_output=True, - text=True, - check=True, - ) + metadata = gh_api([endpoint]) compare = json.loads(metadata.stdout) status = compare.get("status", "") if status != "ahead": @@ -97,12 +152,7 @@ def fetch_compare_diff(head_repo: str, base_sha: str, head_sha: str) -> Optional file=sys.stderr, ) return None - result = subprocess.run( - ["gh", "api", "-H", "Accept: application/vnd.github.diff", endpoint], - capture_output=True, - text=True, - check=True, - ) + result = gh_api(["-H", "Accept: application/vnd.github.diff", endpoint]) except subprocess.CalledProcessError as e: print( f"Could not fetch incremental diff from {head_repo}: {e.stderr}", @@ -134,7 +184,18 @@ def main(): endpoint = f"repos/{repo}/issues/{pr_number}/comments" print(f"Fetching comments from {endpoint}...") - raw_comments = gh_api_paginate(endpoint) + try: + raw_comments = gh_api_paginate(endpoint) + except subprocess.CalledProcessError as e: + retry_limit = getattr(e, "retry_limit", DEFAULT_API_ATTEMPTS) + print( + "::error::Could not fetch prior PR comments after " + f"{retry_limit} attempt(s). Endpoint: {endpoint}. " + f"Repository: {repo}. PR: {pr_number}. Last error: " + f"{command_error_summary(e)}", + file=sys.stderr, + ) + raise print(f"Found {len(raw_comments)} comments") # Extract comment summaries @@ -183,12 +244,7 @@ def main(): summary_comment_id = legacy_summary_comment_id pr_endpoint = f"repos/{repo}/pulls/{pr_number}" - pr_result = subprocess.run( - ["gh", "api", pr_endpoint], - capture_output=True, - text=True, - check=True, - ) + pr_result = gh_api([pr_endpoint]) pr = json.loads(pr_result.stdout) current_sha = pr["head"]["sha"] current_base_sha = pr["base"]["sha"] diff --git a/.github/actions/pr-review/scripts/resolve-outdated-threads.py b/.github/actions/pr-review/scripts/resolve-outdated-threads.py index a5a2d28..bca5021 100644 --- a/.github/actions/pr-review/scripts/resolve-outdated-threads.py +++ b/.github/actions/pr-review/scripts/resolve-outdated-threads.py @@ -11,10 +11,15 @@ import json import os +import re import subprocess import sys +import time +from typing import Optional REVIEW_PREFIXES = ("🔴 Security:", "🟠 Bug:", "🟡 Suggestion:") +DEFAULT_API_ATTEMPTS = 3 +HTTP_STATUS_PATTERN = re.compile(r"HTTP\s+(\d{3})") LIST_THREADS_QUERY = """ query($owner: String!, $repo: String!, $number: Int!, $after: String) { @@ -50,14 +55,56 @@ """ +def command_error_summary(error: subprocess.CalledProcessError) -> str: + detail = (error.stderr or error.stdout or "").strip() + if not detail: + detail = f"exit status {error.returncode}" + return detail.splitlines()[-1] + + +def error_http_status(error: subprocess.CalledProcessError) -> Optional[int]: + match = HTTP_STATUS_PATTERN.search(error.stderr or error.stdout or "") + if not match: + return None + return int(match.group(1)) + + +def retry_limit_for_error(error: subprocess.CalledProcessError) -> int: + status = error_http_status(error) + if status == 404: + return min(DEFAULT_API_ATTEMPTS, 2) + if status in (408, 429) or (status is not None and status >= 500): + return DEFAULT_API_ATTEMPTS + if status is None: + return DEFAULT_API_ATTEMPTS + return 1 + + def gh_graphql(query: str, **variables: str) -> dict: """Call gh api graphql and return parsed JSON.""" cmd = ["gh", "api", "graphql", "-f", f"query={query}"] for key, value in variables.items(): flag = "-F" if isinstance(value, int) else "-f" cmd.extend([flag, f"{key}={value}"]) - result = subprocess.run(cmd, capture_output=True, text=True, check=True) - return json.loads(result.stdout) + last_error = None + for attempt in range(1, DEFAULT_API_ATTEMPTS + 1): + try: + result = subprocess.run(cmd, capture_output=True, text=True, check=True) + return json.loads(result.stdout) + except subprocess.CalledProcessError as e: + last_error = e + retry_limit = retry_limit_for_error(e) + e.retry_limit = retry_limit + if attempt >= retry_limit: + break + print( + "::warning::GitHub GraphQL request failed; " + f"retrying ({attempt}/{retry_limit}): " + f"{command_error_summary(e)}", + file=sys.stderr, + ) + time.sleep(attempt) + raise last_error def get_all_threads(owner: str, repo: str, number: int) -> list[dict]: @@ -101,6 +148,14 @@ def resolve_thread(thread_id: str) -> bool: return False +def write_summary(summary: dict) -> None: + output_path = os.path.join(".github", "resolved-threads.json") + os.makedirs(os.path.dirname(output_path), exist_ok=True) + with open(output_path, "w") as f: + json.dump(summary, f, indent=2) + print(f"Summary written to {output_path}") + + def main(): repo = os.environ.get("GITHUB_REPOSITORY", "") pr_number = os.environ.get("PR_NUMBER", "") @@ -112,7 +167,17 @@ def main(): number = int(pr_number) print(f"Fetching review threads for {owner}/{repo_name}#{number}...") - threads = get_all_threads(owner, repo_name, number) + try: + threads = get_all_threads(owner, repo_name, number) + except subprocess.CalledProcessError as e: + retry_limit = getattr(e, "retry_limit", DEFAULT_API_ATTEMPTS) + print( + "::error::Could not fetch review threads after " + f"{retry_limit} attempt(s). Repository: {repo}. " + f"PR: {pr_number}. Last error: {command_error_summary(e)}", + file=sys.stderr, + ) + raise print(f"Found {len(threads)} total review threads") to_resolve = [t for t in threads if should_resolve(t)] @@ -137,13 +202,9 @@ def main(): "resolved": resolved, } - output_path = os.path.join(".github", "resolved-threads.json") - os.makedirs(os.path.dirname(output_path), exist_ok=True) - with open(output_path, "w") as f: - json.dump(summary, f, indent=2) + write_summary(summary) print(f"\nDone: resolved {len(resolved)}/{len(to_resolve)} threads") - print(f"Summary written to {output_path}") if __name__ == "__main__": diff --git a/tools/connector-docs/check-docs-change.mjs b/tools/connector-docs/check-docs-change.mjs index ea44c7c..1944264 100644 --- a/tools/connector-docs/check-docs-change.mjs +++ b/tools/connector-docs/check-docs-change.mjs @@ -6,6 +6,8 @@ import { pathToFileURL } from "node:url"; const CONNECTOR_DOCS_PATH = "docs/connector.mdx"; const PER_PAGE = 100; const PAGE_LIMIT = 30; +const DEFAULT_MAX_ATTEMPTS = 3; +const DEFAULT_RETRY_DELAY_MS = 1000; function normalizeApiUrl(apiUrl) { return apiUrl.replace(/\/+$/, ""); @@ -27,11 +29,75 @@ function writeOutput(name, value) { } } +function sleep(ms) { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +function retryLimitForStatus(status, maxAttempts) { + if (status === 404) { + return Math.min(maxAttempts, 2); + } + if (status === 408 || status === 429 || status >= 500) { + return maxAttempts; + } + return 1; +} + +function retryDelay(response, fallbackDelayMs, attempt) { + const retryAfter = response.headers?.get?.("retry-after"); + if (!retryAfter) { + return fallbackDelayMs * attempt; + } + + const seconds = Number.parseInt(retryAfter, 10); + if (Number.isFinite(seconds)) { + return seconds * 1000; + } + + const retryAt = Date.parse(retryAfter); + if (Number.isFinite(retryAt)) { + return Math.max(0, retryAt - Date.now()); + } + + return fallbackDelayMs * attempt; +} + +function requestId(response) { + return response.headers?.get?.("x-github-request-id") || "unavailable"; +} + +async function fetchWithRetry({ + fetchFn, + url, + headers, + maxAttempts, + retryDelayMs, + sleepFn, +}) { + for (let attempt = 1; attempt <= maxAttempts; attempt += 1) { + const response = await fetchFn(url, { headers }); + const retryLimit = retryLimitForStatus(response.status, maxAttempts); + if (response.ok || attempt >= retryLimit) { + return { attempts: attempt, response }; + } + + process.stderr.write( + `GitHub PR files request returned ${response.status} ${response.statusText}; ` + + `retrying (${attempt}/${retryLimit}). ` + + `Request ID: ${requestId(response)}.\n`, + ); + await sleepFn(retryDelay(response, retryDelayMs, attempt)); + } +} + export async function checkConnectorDocsChange({ apiUrl = "https://api.github.com", fetchFn = fetch, + maxAttempts = DEFAULT_MAX_ATTEMPTS, prNumber, repository, + retryDelayMs = DEFAULT_RETRY_DELAY_MS, + sleepFn = sleep, token, } = {}) { if (!prNumber) { @@ -52,10 +118,20 @@ export async function checkConnectorDocsChange({ headers.Authorization = `Bearer ${token}`; } - const response = await fetchFn(url, { headers }); + const { attempts, response } = await fetchWithRetry({ + fetchFn, + url, + headers, + maxAttempts, + retryDelayMs, + sleepFn, + }); if (!response.ok) { throw new Error( - `GitHub PR files request failed: ${response.status} ${response.statusText}`, + `GitHub PR files request failed after ${attempts} attempt(s): ` + + `${response.status} ${response.statusText}. ` + + `Endpoint: ${url}. Repository: ${repository}. PR: ${prNumber}. ` + + `Request ID: ${requestId(response)}.`, ); } diff --git a/tools/connector-docs/check-docs-change.test.mjs b/tools/connector-docs/check-docs-change.test.mjs index 1e9ced9..bddeed5 100644 --- a/tools/connector-docs/check-docs-change.test.mjs +++ b/tools/connector-docs/check-docs-change.test.mjs @@ -79,7 +79,67 @@ describe("checkConnectorDocsChange", () => { assert.equal(fetchFn.calls.length, 30); }); - it("fails closed on GitHub API errors", async () => { + it("retries transient GitHub API errors", async () => { + const calls = []; + const fetchFn = async () => { + calls.push(null); + if (calls.length === 1) { + return { + ok: false, + status: 404, + statusText: "Not Found", + }; + } + return { + ok: true, + status: 200, + statusText: "OK", + async json() { + return [{ filename: "README.md" }]; + }, + }; + }; + const result = await checkConnectorDocsChange({ + fetchFn, + maxAttempts: 3, + prNumber: "12", + repository: "example/repo", + retryDelayMs: 0, + sleepFn: async () => {}, + }); + assert.deepEqual(result, { + validate: "false", + reason: "docs_path_unchanged", + }); + assert.equal(calls.length, 2); + }); + + it("does not retry authorization-style GitHub API errors", async () => { + const calls = []; + const fetchFn = async () => { + calls.push(null); + return { + ok: false, + status: 403, + statusText: "Forbidden", + }; + }; + await assert.rejects( + () => + checkConnectorDocsChange({ + fetchFn, + maxAttempts: 3, + prNumber: "12", + repository: "example/repo", + retryDelayMs: 0, + sleepFn: async () => {}, + }), + /GitHub PR files request failed after 1 attempt\(s\): 403 Forbidden/, + ); + assert.equal(calls.length, 1); + }); + + it("reports GitHub API errors with endpoint context", async () => { const fetchFn = async () => ({ ok: false, status: 502, @@ -89,10 +149,13 @@ describe("checkConnectorDocsChange", () => { () => checkConnectorDocsChange({ fetchFn, + maxAttempts: 2, prNumber: "12", repository: "example/repo", + retryDelayMs: 0, + sleepFn: async () => {}, }), - /GitHub PR files request failed: 502 Bad Gateway/, + /GitHub PR files request failed after 2 attempt\(s\): 502 Bad Gateway. Endpoint: https:\/\/api.github.com\/repos\/example\/repo\/pulls\/12\/files\?per_page=100&page=1/, ); }); });