From 66754d9b644ca9e9d9f28f13ff45bde85086719d Mon Sep 17 00:00:00 2001 From: Billy V Date: Thu, 8 Oct 2026 20:30:17 +0000 Subject: [PATCH 1/3] fix: Stop exhausting GitHub API rate limits on busy orgs Every completed workflow_job webhook made up to three GitHub API calls with the org's installation token: fetch .sentry/sentry_config.ini, fetch the run, and fetch the workflow. Skipped jobs also fetched the config, only to be dropped afterwards. Busy orgs (getsentry, cyeragit, MetaMask) ran out of their hourly installation budget, and GitHub answered the config fetch with 403 until the window reset, so their remaining jobs were dropped and reported as errors. - Cache each org's DSN for 10 minutes - Cache workflow paths, which every job of a run shares - Bail on skipped jobs before making any API call This brings a traced job down to about one API call, and skipped jobs to none. Fixes SENTRY-GITHUB-ACTIONS-APP-5W Co-Authored-By: Claude Opus 5.5 (1M context) --- README.md | 2 ++ requirements.txt | 1 + src/github_sdk.py | 18 ++++++++++++++-- src/sentry_config.py | 13 +++++++++++- src/web_app_handler.py | 3 +++ tests/conftest.py | 9 ++++++++ tests/test_github_sdk.py | 18 ++++++++++++++++ tests/test_sentry_config_file.py | 35 ++++++++++++++++++++++++++++++++ tests/test_web_app_handler.py | 19 +++++++++++++++++ 9 files changed, 115 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 7ab2720..b366f9e 100644 --- a/README.md +++ b/README.md @@ -57,6 +57,8 @@ Steps to follow: dsn = https://foo@bar.ingest.sentry.io/foobar ``` + - **Note:** The app caches this file, so changes to it can take up to 10 minutes to apply. + 4. Install [this GitHub App](https://github.com/apps/sentry-github-app-alpha) 1. **"All repositories"** - this is the easiest option; all repositories will have telemetry from GitHub Actions sent. 2. **"Only select repositories"** diff --git a/requirements.txt b/requirements.txt index 008063d..c95af15 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,3 +1,4 @@ +cachetools==5.3.2 flask==2.3.3 google-cloud-secret-manager==2.11.0 PyJWT[crypto]==2.4.0 diff --git a/src/github_sdk.py b/src/github_sdk.py index cf28d01..8826ccd 100644 --- a/src/github_sdk.py +++ b/src/github_sdk.py @@ -4,10 +4,14 @@ import hashlib import io import logging +import threading import uuid from datetime import datetime import requests +from cachetools import cached +from cachetools import TTLCache +from cachetools.keys import hashkey from sentry_sdk.envelope import Envelope from sentry_sdk.utils import format_timestamp @@ -46,10 +50,20 @@ def _fetch_github(self, url): req.raise_for_status() return req + # Every job of a run shares its workflow, whose path rarely changes, so this saves + # a GitHub API call per job + @cached( + cache=TTLCache(maxsize=4096, ttl=60 * 60), + key=lambda self, workflow_url: hashkey(workflow_url), + lock=threading.Lock(), + ) + def _fetch_workflow_path(self, workflow_url): + return self._fetch_github(workflow_url).json()["path"] + def _get_extra_metadata(self, job): # XXX: This is the slowest call runs = self._fetch_github(job["run_url"]).json() - workflow = self._fetch_github(runs["workflow_url"]).json() + workflow_path = self._fetch_workflow_path(runs["workflow_url"]) repo = runs["repository"]["full_name"] meta = { # "workflow_name": workflow["name"], @@ -68,7 +82,7 @@ def _get_extra_metadata(self, job): "run_attempt": runs["run_attempt"], # Rerunning a job "event": runs["event"], # It allows querying jobs within the same workflow (e.g. foo.yml) - "workflow": workflow["path"].rsplit("/")[-1], + "workflow": workflow_path.rsplit("/")[-1], }, } if runs.get("pull_requests"): diff --git a/src/sentry_config.py b/src/sentry_config.py index 30678da..9e0e4b9 100644 --- a/src/sentry_config.py +++ b/src/sentry_config.py @@ -3,10 +3,13 @@ import base64 import logging import os +import threading from configparser import ConfigParser -from functools import lru_cache import requests +from cachetools import cached +from cachetools import TTLCache +from cachetools.keys import hashkey LOGGING_LEVEL = os.environ.get("LOGGING_LEVEL", logging.INFO) logger = logging.getLogger(__name__) @@ -15,8 +18,16 @@ SENTRY_CONFIG_API_URL = ( "https://api.github.com/repos/{owner}/.sentry/contents/sentry_config.ini" ) +# Fetching the config on every webhook exhausts busy orgs' GitHub API rate limit +DSN_CACHE_TTL_SECONDS = 10 * 60 +@cached( + cache=TTLCache(maxsize=1024, ttl=DSN_CACHE_TTL_SECONDS), + # The token is minted per webhook, so only the org identifies the DSN + key=lambda org, token: hashkey(org), + lock=threading.Lock(), +) def fetch_dsn_for_github_org(org: str, token: str) -> str: # Using the GH app token allows fetching the file in a private repo headers = { diff --git a/src/web_app_handler.py b/src/web_app_handler.py index 5b96c07..1b20da5 100644 --- a/src/web_app_handler.py +++ b/src/web_app_handler.py @@ -29,6 +29,9 @@ def handle_event(self, data, headers): reason = "Event not supported." elif data["action"] != "completed": reason = "We cannot do anything with this workflow state." + elif data["workflow_job"]["conclusion"] == "skipped": + # These are not traced, so don't spend GitHub API calls on them + reason = "Skipped jobs are not traced." else: # For now, this simplifies testing if self.dry_run: diff --git a/tests/conftest.py b/tests/conftest.py index b1e96f0..a376726 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -4,6 +4,15 @@ import pytest +from src.github_sdk import GithubClient +from src.sentry_config import fetch_dsn_for_github_org + + +@pytest.fixture(autouse=True) +def clear_caches(): + fetch_dsn_for_github_org.cache_clear() + GithubClient._fetch_workflow_path.cache_clear() + @pytest.fixture def jobA_job(): diff --git a/tests/test_github_sdk.py b/tests/test_github_sdk.py index 7f7e401..66ee2b2 100644 --- a/tests/test_github_sdk.py +++ b/tests/test_github_sdk.py @@ -110,6 +110,24 @@ def test_trace_generation_with_failing_steps( assert trace["tags"]["event"] == "push" +@responses.activate +def test_workflow_is_fetched_once_for_jobs_of_a_run( + jobA_job, + jobA_runs, + jobA_workflow, +): + responses.get(jobA_job["run_url"], json=jobA_runs) + responses.get(jobA_runs["workflow_url"], json=jobA_workflow) + + # Each job comes from a separate webhook, thus, a separate client + for token in ("token_1", "token_2"): + trace = GithubClient(dsn=DSN, token=token)._generate_trace(jobA_job) + assert trace["tags"]["workflow"] == "acceptance.yml" + + responses.assert_call_count(jobA_job["run_url"], 2) + responses.assert_call_count(jobA_runs["workflow_url"], 1) + + @freeze_time() @responses.activate @patch("src.github_sdk.get_uuid") diff --git a/tests/test_sentry_config_file.py b/tests/test_sentry_config_file.py index db95aa4..b07f937 100644 --- a/tests/test_sentry_config_file.py +++ b/tests/test_sentry_config_file.py @@ -2,7 +2,9 @@ from unittest import TestCase +import pytest import responses +from requests import HTTPError from src.sentry_config import fetch_dsn_for_github_org from src.sentry_config import SENTRY_CONFIG_API_URL as api_url @@ -47,6 +49,39 @@ def setUp(self) -> None: def test_fetch_parse_sentry_config_file(self) -> None: assert fetch_dsn_for_github_org(org, token) == expected_dsn + @responses.activate + def test_dsn_is_cached_per_org(self) -> None: + other_org_api_url = api_url.replace("{owner}", "other_org") + responses.add( + method="GET", + url=other_org_api_url, + json=sentry_config_file_meta, + status=200, + ) + + # Every webhook comes with a new token + assert fetch_dsn_for_github_org(org, "token_1") == expected_dsn + assert fetch_dsn_for_github_org(org, "token_2") == expected_dsn + assert fetch_dsn_for_github_org("other_org", "token_3") == expected_dsn + + responses.assert_call_count(self.api_url, 1) + responses.assert_call_count(other_org_api_url, 1) + + @responses.activate + def test_failed_fetch_is_not_cached(self) -> None: + responses.replace(responses.GET, self.api_url, status=403) + with pytest.raises(HTTPError): + fetch_dsn_for_github_org(org, token) + + # e.g. the org's GitHub API rate limit has been reset + responses.replace( + responses.GET, + self.api_url, + json=sentry_config_file_meta, + status=200, + ) + assert fetch_dsn_for_github_org(org, token) == expected_dsn + def test_fetch_private_repo(self) -> None: pass diff --git a/tests/test_web_app_handler.py b/tests/test_web_app_handler.py index e5bb0ec..8515c36 100644 --- a/tests/test_web_app_handler.py +++ b/tests/test_web_app_handler.py @@ -4,6 +4,7 @@ from unittest import mock import pytest +import responses from src.web_app_handler import WebAppHandler @@ -54,6 +55,24 @@ def test_not_completed_workflow(): assert http_code == 200 +@responses.activate +def test_skipped_job_makes_no_api_calls(monkeypatch, skipped_workflow): + monkeypatch.delenv("GH_APP_ID", raising=False) + handler = WebAppHandler() + reason, http_code = handler.handle_event( + data={ + "action": "completed", + "installation": {"id": 1}, + "repository": {"owner": {"login": "getsentry"}}, + "workflow_job": skipped_workflow, + }, + headers={"X-GitHub-Event": "workflow_job"}, + ) + assert reason == "Skipped jobs are not traced." + assert http_code == 200 + assert len(responses.calls) == 0 + + @pytest.mark.skip(reason="Not so important") def test_missing_workflow_job(monkeypatch): monkeypatch.delenv("GH_APP_ID", raising=False) From f04f751a5bdae0e5922d00944e8a513e65726114 Mon Sep 17 00:00:00 2001 From: Billy V Date: Thu, 8 Oct 2026 21:06:58 +0000 Subject: [PATCH 2/3] fix: Keep cached data scoped to its installation Addresses review findings on the new caching: - Read the workflow path from the run instead of fetching and caching the workflow. A @cached wrapper holds its cache as a closure variable, so Sentry's captured frame locals sent other orgs' private repo and workflow URLs with any error. This also removes a call per job and the 422s from fetching required workflows' `workflow_url`. - Keep the DSN cache in module globals for the same reason, and key it on (installation id, org), so a DSN is only served to an installation whose token could read that org's config. - Run the skipped-job test in Github App mode, where a token would be minted, and give each org its own DSN in the cache test. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/github_sdk.py | 19 ++-------- src/sentry_config.py | 26 ++++++++----- src/web_app_handler.py | 4 +- tests/conftest.py | 20 ++-------- tests/fixtures/failedJob/workflow.json | 12 ------ tests/fixtures/jobA/runs.json | 1 + tests/fixtures/jobA/workflow.json | 12 ------ tests/test_github_sdk.py | 36 +++++------------- tests/test_sentry_config_file.py | 52 +++++++++++++++++++------- tests/test_web_app_handler.py | 21 ++++++++++- 10 files changed, 92 insertions(+), 111 deletions(-) delete mode 100644 tests/fixtures/failedJob/workflow.json delete mode 100644 tests/fixtures/jobA/workflow.json diff --git a/src/github_sdk.py b/src/github_sdk.py index 8826ccd..a72df68 100644 --- a/src/github_sdk.py +++ b/src/github_sdk.py @@ -4,14 +4,10 @@ import hashlib import io import logging -import threading import uuid from datetime import datetime import requests -from cachetools import cached -from cachetools import TTLCache -from cachetools.keys import hashkey from sentry_sdk.envelope import Envelope from sentry_sdk.utils import format_timestamp @@ -50,20 +46,9 @@ def _fetch_github(self, url): req.raise_for_status() return req - # Every job of a run shares its workflow, whose path rarely changes, so this saves - # a GitHub API call per job - @cached( - cache=TTLCache(maxsize=4096, ttl=60 * 60), - key=lambda self, workflow_url: hashkey(workflow_url), - lock=threading.Lock(), - ) - def _fetch_workflow_path(self, workflow_url): - return self._fetch_github(workflow_url).json()["path"] - def _get_extra_metadata(self, job): # XXX: This is the slowest call runs = self._fetch_github(job["run_url"]).json() - workflow_path = self._fetch_workflow_path(runs["workflow_url"]) repo = runs["repository"]["full_name"] meta = { # "workflow_name": workflow["name"], @@ -82,7 +67,9 @@ def _get_extra_metadata(self, job): "run_attempt": runs["run_attempt"], # Rerunning a job "event": runs["event"], # It allows querying jobs within the same workflow (e.g. foo.yml) - "workflow": workflow_path.rsplit("/")[-1], + # The run has the path, so there's no need to fetch its workflow, which + # fails for required workflows (`workflow_url` 422s) + "workflow": runs["path"].rsplit("/")[-1], }, } if runs.get("pull_requests"): diff --git a/src/sentry_config.py b/src/sentry_config.py index 9e0e4b9..820a2b7 100644 --- a/src/sentry_config.py +++ b/src/sentry_config.py @@ -7,9 +7,7 @@ from configparser import ConfigParser import requests -from cachetools import cached from cachetools import TTLCache -from cachetools.keys import hashkey LOGGING_LEVEL = os.environ.get("LOGGING_LEVEL", logging.INFO) logger = logging.getLogger(__name__) @@ -20,15 +18,25 @@ ) # Fetching the config on every webhook exhausts busy orgs' GitHub API rate limit DSN_CACHE_TTL_SECONDS = 10 * 60 +# Module globals rather than @cached, whose closure would put every cached DSN in the +# local variables Sentry captures with an error +_dsn_cache = TTLCache(maxsize=1024, ttl=DSN_CACHE_TTL_SECONDS) +_dsn_cache_lock = threading.Lock() -@cached( - cache=TTLCache(maxsize=1024, ttl=DSN_CACHE_TTL_SECONDS), - # The token is minted per webhook, so only the org identifies the DSN - key=lambda org, token: hashkey(org), - lock=threading.Lock(), -) -def fetch_dsn_for_github_org(org: str, token: str) -> str: +def fetch_dsn_for_github_org(org: str, token: str, installation_id: int) -> str: + # Only serve a DSN to the installation whose token could read it + key = (installation_id, org) + with _dsn_cache_lock: + dsn = _dsn_cache.get(key) + if dsn is None: + dsn = _fetch_dsn(org, token) + with _dsn_cache_lock: + _dsn_cache[key] = dsn + return dsn + + +def _fetch_dsn(org: str, token: str) -> str: # Using the GH app token allows fetching the file in a private repo headers = { "Accept": "application/vnd.github+json", diff --git a/src/web_app_handler.py b/src/web_app_handler.py index 1b20da5..d1a3d0d 100644 --- a/src/web_app_handler.py +++ b/src/web_app_handler.py @@ -46,7 +46,7 @@ def handle_event(self, data, headers): installation_id ) as token: # Once the Sentry org has a .sentry repo we can remove the DSN from the deployment - dsn = fetch_dsn_for_github_org(org, token) + dsn = fetch_dsn_for_github_org(org, token, installation_id) client = GithubClient( token=token, dsn=dsn, @@ -55,7 +55,7 @@ def handle_event(self, data, headers): client.send_trace(data["workflow_job"]) else: # Once the Sentry org has a .sentry repo we can remove the DSN from the deployment - dsn = fetch_dsn_for_github_org(org, token) + dsn = fetch_dsn_for_github_org(org, token, installation_id) client = GithubClient( token=self.config.gh.token, dsn=dsn, diff --git a/tests/conftest.py b/tests/conftest.py index a376726..a6f09bf 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -4,14 +4,12 @@ import pytest -from src.github_sdk import GithubClient -from src.sentry_config import fetch_dsn_for_github_org +from src import sentry_config @pytest.fixture(autouse=True) -def clear_caches(): - fetch_dsn_for_github_org.cache_clear() - GithubClient._fetch_workflow_path.cache_clear() +def clear_dsn_cache(): + sentry_config._dsn_cache.clear() @pytest.fixture @@ -26,12 +24,6 @@ def jobA_runs(): return json.load(f) -@pytest.fixture -def jobA_workflow(): - with open("tests/fixtures/jobA/workflow.json") as f: - return json.load(f) - - @pytest.fixture def skipped_workflow(): with open("tests/fixtures/skipped_workflow.json") as f: @@ -74,9 +66,3 @@ def failure_job(): def failure_runs(): with open("tests/fixtures/failedJob/runs.json") as f: return json.load(f) - - -@pytest.fixture -def failure_workflow(): - with open("tests/fixtures/failedJob/workflow.json") as f: - return json.load(f) diff --git a/tests/fixtures/failedJob/workflow.json b/tests/fixtures/failedJob/workflow.json deleted file mode 100644 index 259e1fd..0000000 --- a/tests/fixtures/failedJob/workflow.json +++ /dev/null @@ -1,12 +0,0 @@ -{ - "id": 48858869248, - "node_id": "CR_kwDOPkC4Ds8AAAALYDcuAA", - "name": "test", - "path": ".github/workflows/test.yml", - "state": "active", - "created_at": "2025-08-25T22:03:57Z", - "updated_at": "2025-08-25T22:03:57Z", - "url": "https://api.github.com/repos/getsentry/ken-test-repo/actions/workflows/48858869248", - "html_url": "https://github.com/getsentry/ken-test-repo/actions/workflows/test.yml", - "badge_url": "https://github.com/getsentry/ken-test-repo/workflows/test/badge.svg" -} diff --git a/tests/fixtures/jobA/runs.json b/tests/fixtures/jobA/runs.json index cc27e3e..80142ed 100644 --- a/tests/fixtures/jobA/runs.json +++ b/tests/fixtures/jobA/runs.json @@ -4,6 +4,7 @@ "node_id": "WFR_kwLOAA1TcM59c-PH", "head_branch": "ahmed/add-naming-layer", "head_sha": "fd976218a94d8a0cd203c711ea7dbe68c573ef4d", + "path": ".github/workflows/acceptance.yml", "run_number": 53142, "event": "pull_request", "status": "completed", diff --git a/tests/fixtures/jobA/workflow.json b/tests/fixtures/jobA/workflow.json deleted file mode 100644 index 3398a0f..0000000 --- a/tests/fixtures/jobA/workflow.json +++ /dev/null @@ -1,12 +0,0 @@ -{ - "id": 1174556, - "node_id": "MDg6V29ya2Zsb3cxMTc0NTU2", - "name": "acceptance", - "path": ".github/workflows/acceptance.yml", - "state": "active", - "created_at": "2020-05-01T23:00:30.000Z", - "updated_at": "2022-03-17T18:24:06.000Z", - "url": "https://api.github.com/repos/getsentry/sentry/actions/workflows/1174556", - "html_url": "https://github.com/getsentry/sentry/blob/master/.github/workflows/acceptance.yml", - "badge_url": "https://github.com/getsentry/sentry/workflows/acceptance/badge.svg" -} diff --git a/tests/test_github_sdk.py b/tests/test_github_sdk.py index 66ee2b2..0855050 100644 --- a/tests/test_github_sdk.py +++ b/tests/test_github_sdk.py @@ -66,7 +66,6 @@ def test_trace_generation( mock_get_uuid, jobA_job, jobA_runs, - jobA_workflow, jobA_trace, uuid_list, ): @@ -75,10 +74,6 @@ def test_trace_generation( "https://api.github.com/repos/getsentry/sentry/actions/runs/2104746951", json=jobA_runs, ) - responses.get( - "https://api.github.com/repos/getsentry/sentry/actions/workflows/1174556", - json=jobA_workflow, - ) client = GithubClient(dsn=DSN, token=TOKEN) trace = client._generate_trace(jobA_job) assert trace == jobA_trace @@ -91,16 +86,11 @@ def test_trace_generation( def test_trace_generation_with_failing_steps( failure_job, failure_runs, - failure_workflow, ): responses.get( failure_job["run_url"], json=failure_runs, ) - responses.get( - failure_runs["workflow_url"], - json=failure_workflow, - ) client = GithubClient(dsn=DSN, token=TOKEN) trace = client._generate_trace(failure_job) @@ -111,21 +101,18 @@ def test_trace_generation_with_failing_steps( @responses.activate -def test_workflow_is_fetched_once_for_jobs_of_a_run( - jobA_job, - jobA_runs, - jobA_workflow, -): +def test_trace_generation_for_required_workflow(jobA_job, jobA_runs): + # Fetching a required workflow's `workflow_url` fails with a 422, and `responses` + # raises on any request that isn't registered + jobA_runs[ + "workflow_url" + ] = "https://api.github.com/repos/getsentry/sentry/actions/required_workflows/1" responses.get(jobA_job["run_url"], json=jobA_runs) - responses.get(jobA_runs["workflow_url"], json=jobA_workflow) - # Each job comes from a separate webhook, thus, a separate client - for token in ("token_1", "token_2"): - trace = GithubClient(dsn=DSN, token=token)._generate_trace(jobA_job) - assert trace["tags"]["workflow"] == "acceptance.yml" + trace = GithubClient(dsn=DSN, token=TOKEN)._generate_trace(jobA_job) - responses.assert_call_count(jobA_job["run_url"], 2) - responses.assert_call_count(jobA_runs["workflow_url"], 1) + assert trace["tags"]["workflow"] == "acceptance.yml" + assert len(responses.calls) == 1 @freeze_time() @@ -135,7 +122,6 @@ def test_send_trace( mock_get_uuid, jobA_job, jobA_runs, - jobA_workflow, uuid_list, ): mock_get_uuid.side_effect = uuid_list @@ -143,10 +129,6 @@ def test_send_trace( "https://api.github.com/repos/getsentry/sentry/actions/runs/2104746951", json=jobA_runs, ) - responses.get( - "https://api.github.com/repos/getsentry/sentry/actions/workflows/1174556", - json=jobA_workflow, - ) responses.post("https://foo@random.ingest.sentry.io/api/bar/envelope/") diff --git a/tests/test_sentry_config_file.py b/tests/test_sentry_config_file.py index b07f937..7c62d70 100644 --- a/tests/test_sentry_config_file.py +++ b/tests/test_sentry_config_file.py @@ -1,5 +1,6 @@ from __future__ import annotations +import base64 from unittest import TestCase import pytest @@ -32,6 +33,15 @@ } org = "armenzg" token = "foo_token" +installation_id = 1 + + +def config_file_meta(dsn: str) -> dict: + contents = f"[sentry-github-actions-app]\ndsn = {dsn}\n" + return { + **sentry_config_file_meta, + "content": base64.b64encode(contents.encode()).decode(), + } class TestSentryConfigCase(TestCase): @@ -47,31 +57,31 @@ def setUp(self) -> None: @responses.activate def test_fetch_parse_sentry_config_file(self) -> None: - assert fetch_dsn_for_github_org(org, token) == expected_dsn + assert fetch_dsn_for_github_org(org, token, installation_id) == expected_dsn @responses.activate - def test_dsn_is_cached_per_org(self) -> None: + def test_dsn_is_cached_per_installation_and_org(self) -> None: + other_org_dsn = "https://e0bb9a6e1c4f4d3c9b3a6c2d8f1e7a5b@o1.ingest.sentry.io/2" other_org_api_url = api_url.replace("{owner}", "other_org") - responses.add( - method="GET", - url=other_org_api_url, - json=sentry_config_file_meta, - status=200, - ) + responses.get(other_org_api_url, json=config_file_meta(other_org_dsn)) # Every webhook comes with a new token - assert fetch_dsn_for_github_org(org, "token_1") == expected_dsn - assert fetch_dsn_for_github_org(org, "token_2") == expected_dsn - assert fetch_dsn_for_github_org("other_org", "token_3") == expected_dsn - + assert fetch_dsn_for_github_org(org, "token_1", 1) == expected_dsn + assert fetch_dsn_for_github_org(org, "token_2", 1) == expected_dsn + assert fetch_dsn_for_github_org("other_org", "token_3", 2) == other_org_dsn responses.assert_call_count(self.api_url, 1) responses.assert_call_count(other_org_api_url, 1) + # Another installation has to be able to read the org's config itself + responses.replace(responses.GET, self.api_url, status=404) + with pytest.raises(HTTPError): + fetch_dsn_for_github_org(org, "token_4", 2) + @responses.activate def test_failed_fetch_is_not_cached(self) -> None: responses.replace(responses.GET, self.api_url, status=403) with pytest.raises(HTTPError): - fetch_dsn_for_github_org(org, token) + fetch_dsn_for_github_org(org, token, installation_id) # e.g. the org's GitHub API rate limit has been reset responses.replace( @@ -80,7 +90,21 @@ def test_failed_fetch_is_not_cached(self) -> None: json=sentry_config_file_meta, status=200, ) - assert fetch_dsn_for_github_org(org, token) == expected_dsn + assert fetch_dsn_for_github_org(org, token, installation_id) == expected_dsn + + @responses.activate + def test_failed_fetch_does_not_expose_cached_dsns(self) -> None: + fetch_dsn_for_github_org(org, token, installation_id) + responses.get(api_url.replace("{owner}", "other_org"), status=403) + + with pytest.raises(HTTPError) as excinfo: + fetch_dsn_for_github_org("other_org", token, 2) + + # Sentry sends every frame's local variables along with an error + tb = excinfo.tb + while tb: + assert expected_dsn not in repr(tb.tb_frame.f_locals) + tb = tb.tb_next def test_fetch_private_repo(self) -> None: pass diff --git a/tests/test_web_app_handler.py b/tests/test_web_app_handler.py index 8515c36..c1034ac 100644 --- a/tests/test_web_app_handler.py +++ b/tests/test_web_app_handler.py @@ -1,10 +1,13 @@ from __future__ import annotations +import base64 import json from unittest import mock import pytest import responses +from cryptography.hazmat.primitives import serialization +from cryptography.hazmat.primitives.asymmetric import rsa from src.web_app_handler import WebAppHandler @@ -55,10 +58,24 @@ def test_not_completed_workflow(): assert http_code == 200 +@pytest.fixture +def gh_app_env(monkeypatch): + private_key = rsa.generate_private_key(public_exponent=65537, key_size=2048) + pem = private_key.private_bytes( + encoding=serialization.Encoding.PEM, + format=serialization.PrivateFormat.PKCS8, + encryption_algorithm=serialization.NoEncryption(), + ) + monkeypatch.delenv("K_SERVICE", raising=False) + monkeypatch.setenv("GH_APP_ID", "1") + monkeypatch.setenv("GH_APP_PRIVATE_KEY", base64.b64encode(pem).decode()) + + @responses.activate -def test_skipped_job_makes_no_api_calls(monkeypatch, skipped_workflow): - monkeypatch.delenv("GH_APP_ID", raising=False) +def test_skipped_job_makes_no_api_calls(gh_app_env, skipped_workflow): handler = WebAppHandler() + # Only Github App mode mints an installation token + assert handler.config.gh_app reason, http_code = handler.handle_event( data={ "action": "completed", From c449acda149c957949d6d1cae9c2bd247453aada Mon Sep 17 00:00:00 2001 From: Billy V Date: Fri, 9 Oct 2026 13:30:49 +0000 Subject: [PATCH 3/3] fix: Pass installation id from the CLI and strip refs from workflow paths Addresses review findings: - cli.py still called fetch_dsn_for_github_org without the new installation_id argument, so every CLI run raised a TypeError before ingesting the job. - A run's path can name the repo the workflow lives in and end in a ref (e.g. `org/repo/.github/workflows/foo.yml@refs/heads/main`), as the REST API documents for required workflows. Splitting on "/" then tagged the job with `main` or `foo.yml@main` instead of `foo.yml`. Strip the ref before taking the file name. Co-Authored-By: Claude Opus 5.5 (1M context) --- cli.py | 2 +- src/github_sdk.py | 5 +++-- tests/test_github_sdk.py | 13 ++++++++++++- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/cli.py b/cli.py index 3505509..63c32d0 100644 --- a/cli.py +++ b/cli.py @@ -49,7 +49,7 @@ def main() -> int: with GithubAppToken(**web_app.config.gh_app._asdict()).get_token( installation_id ) as token: - dsn = fetch_dsn_for_github_org(org, token) + dsn = fetch_dsn_for_github_org(org, token, installation_id) client = GithubClient(token=token, dsn=dsn) client.send_trace(job) diff --git a/src/github_sdk.py b/src/github_sdk.py index a72df68..4ece368 100644 --- a/src/github_sdk.py +++ b/src/github_sdk.py @@ -68,8 +68,9 @@ def _get_extra_metadata(self, job): "event": runs["event"], # It allows querying jobs within the same workflow (e.g. foo.yml) # The run has the path, so there's no need to fetch its workflow, which - # fails for required workflows (`workflow_url` 422s) - "workflow": runs["path"].rsplit("/")[-1], + # fails for required workflows (`workflow_url` 422s). Their path can end + # in a ref (e.g. `foo.yml@refs/heads/main`), which isn't part of the name + "workflow": runs["path"].split("@", 1)[0].rsplit("/")[-1], }, } if runs.get("pull_requests"): diff --git a/tests/test_github_sdk.py b/tests/test_github_sdk.py index 0855050..6df063a 100644 --- a/tests/test_github_sdk.py +++ b/tests/test_github_sdk.py @@ -100,13 +100,24 @@ def test_trace_generation_with_failing_steps( assert trace["tags"]["event"] == "push" +@pytest.mark.parametrize( + "path", + [ + ".github/workflows/acceptance.yml", + ".github/workflows/acceptance.yml@main", + "getsentry/.github/.github/workflows/acceptance.yml@refs/heads/main", + "getsentry/.github/.github/workflows/acceptance.yml@1595d4b6de6a9e9751fb270a41019ce507d4099e", + ], +) @responses.activate -def test_trace_generation_for_required_workflow(jobA_job, jobA_runs): +def test_trace_generation_for_required_workflow(jobA_job, jobA_runs, path): # Fetching a required workflow's `workflow_url` fails with a 422, and `responses` # raises on any request that isn't registered jobA_runs[ "workflow_url" ] = "https://api.github.com/repos/getsentry/sentry/actions/required_workflows/1" + # A required workflow's path can name the repo it lives in and end in a ref + jobA_runs["path"] = path responses.get(jobA_job["run_url"], json=jobA_runs) trace = GithubClient(dsn=DSN, token=TOKEN)._generate_trace(jobA_job)