From 1ec5579a4c1f3e6786a88028554bc2c7831ef961 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Mon, 14 Sep 2026 03:34:21 -0400 Subject: [PATCH] fix(notifications): read github_token param in GitHub PR notifier The GitHub PR notifier resolved its credential from a `token` key, but the notifier parameter is declared as `github_token` in notifications.yaml, which is the key the notification manager resolves dashboard configuration and the GITHUB_TOKEN environment variable into. A token supplied through dashboard configuration was enough to enable the notifier but never reached the GitHub API call, so the run logged `no GitHub token available` and posted no comment. Runs configuring the token through the environment were unaffected, because the notifier fell back to reading GITHUB_TOKEN directly. `token` is still accepted so callers constructing the notifier directly keep working. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 12 +++ .../core/notification/github_pr_notifier.py | 6 +- tests/test_github_pr_notifier_params.py | 94 +++++++++++++++++++ 3 files changed, 111 insertions(+), 1 deletion(-) create mode 100644 tests/test_github_pr_notifier_params.py diff --git a/CHANGELOG.md b/CHANGELOG.md index cb71927..f7fe19f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,18 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Fixed + +- **GitHub PR notifier now reads the `github_token` notifier parameter.** The + parameter is declared as `github_token` in `notifications.yaml`, which is the + key the notification manager resolves dashboard configuration and the + `GITHUB_TOKEN` environment variable into, but the notifier looked up `token`. + A token supplied through dashboard configuration enabled the notifier without + reaching the GitHub API call, so the run logged `no GitHub token available` + and posted nothing. Environment-variable configuration was unaffected, since + the notifier fell back to reading `GITHUB_TOKEN` directly. `token` is still + accepted for callers that construct the notifier themselves. + ## [3.2.0] - 2026-09-10 Scanner accuracy release. TruffleHog secret verification and the Java SAST rule diff --git a/socket_basics/core/notification/github_pr_notifier.py b/socket_basics/core/notification/github_pr_notifier.py index a7bcca4..ea23b34 100644 --- a/socket_basics/core/notification/github_pr_notifier.py +++ b/socket_basics/core/notification/github_pr_notifier.py @@ -22,8 +22,12 @@ class GithubPRNotifier(BaseNotifier): def __init__(self, params: Dict[str, Any] | None = None): super().__init__(params or {}) - # GitHub token from params, env variable, or app config + # GitHub token from params, env variable, or app config. The notifier + # parameter is named github_token in notifications.yaml, which is the + # key the manager resolves dashboard config and env vars into; token is + # kept for callers that construct the notifier directly. self.token = ( + self.config.get('github_token') or self.config.get('token') or get_github_token() ) diff --git a/tests/test_github_pr_notifier_params.py b/tests/test_github_pr_notifier_params.py new file mode 100644 index 0000000..f1cdf42 --- /dev/null +++ b/tests/test_github_pr_notifier_params.py @@ -0,0 +1,94 @@ +from socket_basics.core.notification.github_pr_notifier import GithubPRNotifier +from socket_basics.core.notification.manager import NotificationManager + + +def _base_cfg(): + return { + "notifiers": { + "github_pr": { + "module_path": "socket_basics.core.notification.github_pr_notifier", + "class": "GithubPRNotifier", + "parameters": [ + {"name": "github_token", "env_variable": "GITHUB_TOKEN", "type": "str"}, + ], + } + } + } + + +def _clear_token_env(monkeypatch): + monkeypatch.delenv("GITHUB_TOKEN", raising=False) + monkeypatch.delenv("INPUT_GITHUB_TOKEN", raising=False) + + +def test_github_pr_notifier_reads_token_from_params(monkeypatch): + """github_token param from dashboard config should populate self.token""" + _clear_token_env(monkeypatch) + n = GithubPRNotifier({"github_token": "ghp_from_params"}) + assert n.token == "ghp_from_params" + + +def test_github_pr_notifier_token_is_none_without_config(monkeypatch): + """Without any config or env var, token should be falsy""" + _clear_token_env(monkeypatch) + n = GithubPRNotifier({}) + assert not n.token + + +def test_github_pr_notifier_falls_back_to_env_var(monkeypatch): + """GITHUB_TOKEN env var should work as fallback when params empty""" + _clear_token_env(monkeypatch) + monkeypatch.setenv("GITHUB_TOKEN", "ghp_from_env") + n = GithubPRNotifier({}) + assert n.token == "ghp_from_env" + + +def test_github_pr_notifier_params_take_precedence_over_env(monkeypatch): + """Dashboard config (params) should take precedence over env var""" + monkeypatch.setenv("GITHUB_TOKEN", "ghp_from_env") + n = GithubPRNotifier({"github_token": "ghp_from_dashboard"}) + assert n.token == "ghp_from_dashboard" + + +def test_github_pr_notifier_accepts_legacy_token_key(monkeypatch): + """A directly constructed notifier may still pass token""" + _clear_token_env(monkeypatch) + n = GithubPRNotifier({"token": "ghp_legacy"}) + assert n.token == "ghp_legacy" + + +def test_github_pr_enabled_via_app_config(monkeypatch): + """Notifier should load and receive the token when github_token is in app_config""" + _clear_token_env(monkeypatch) + + cfg = _base_cfg() + nm = NotificationManager(cfg, app_config={"github_token": "ghp_from_dashboard"}) + nm.load_from_config() + + notifier = next(n for n in nm.notifiers if getattr(n, "name", "") == "github_pr") + assert notifier.token == "ghp_from_dashboard" + + +def test_github_pr_enabled_via_env_var(monkeypatch): + """Notifier should load and receive the token when GITHUB_TOKEN is set""" + _clear_token_env(monkeypatch) + monkeypatch.setenv("GITHUB_TOKEN", "ghp_from_env") + + cfg = _base_cfg() + nm = NotificationManager(cfg, app_config={}) + nm.load_from_config() + + notifier = next(n for n in nm.notifiers if getattr(n, "name", "") == "github_pr") + assert notifier.token == "ghp_from_env" + + +def test_github_pr_app_config_precedence_over_env(monkeypatch): + """app_config github_token should take precedence over env var in manager flow""" + monkeypatch.setenv("GITHUB_TOKEN", "ghp_from_env") + + cfg = _base_cfg() + nm = NotificationManager(cfg, app_config={"github_token": "ghp_from_dashboard"}) + nm.load_from_config() + + notifier = next(n for n in nm.notifiers if getattr(n, "name", "") == "github_pr") + assert notifier.token == "ghp_from_dashboard"