From bddb8584e012792e944587dd3002c1e0dc0acecb Mon Sep 17 00:00:00 2001 From: jdalton Date: Mon, 3 Aug 2026 14:27:35 -0400 Subject: [PATCH 1/3] feat: add pr_comment_enabled and pr_comment_collapse_all The Socket Basics PR comment could not be turned off, and pr_comment_collapse_non_critical deliberately leaves critical findings expanded, so a single critical finding always forced the comment open. Teams evaluating finding quality in the Socket dashboard had no way to keep the scan running without the comment appearing on every PR. Add two independent switches: pr_comment_enabled (default true) - post/update the PR comment pr_comment_collapse_all (default false) - collapse every section Notifiers run last, after the scan and after the Socket dashboard upload, so suppressing the comment cannot suppress either. Severity labels stay under the separate pr_labels_enabled switch. Feature flags are now read through coerce_bool so a dashboard config that supplies them as strings is honored; bool("false") is True, which previously read a disabled flag as enabled. Refs: SURF-1451 --- CHANGELOG.md | 18 +++ action.yml | 18 +++ docs/github-action.md | 2 + docs/github-pr-comment-guide.md | 71 ++++++++++- scripts/preview_pr_comments.py | 2 + .../core/connector/opengrep/github_pr.py | 7 +- .../core/connector/socket_tier1/github_pr.py | 6 +- .../core/notification/github_pr_helpers.py | 40 +++++- .../core/notification/github_pr_notifier.py | 22 +++- socket_basics/notifications.yaml | 12 ++ tests/test_github_pr_notifier.py | 115 ++++++++++++++++++ tests/test_pr_formatters.py | 69 +++++++++++ 12 files changed, 371 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fde72b..a238d56 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,24 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Added + +- Added `pr_comment_enabled` (default `true`). Set it to `false` to run the scan + without posting or updating the pull request comment. Scanning, the Socket + dashboard upload, `.socket.facts.json`, the high/critical job failure, and the + other notifiers are all unaffected; severity labels stay under the separate + `pr_labels_enabled` switch. +- Added `pr_comment_collapse_all` (default `false`), which collapses every + findings section including critical ones. Previously + `pr_comment_collapse_non_critical` always left critical findings expanded, so + a single critical finding forced the whole comment open. + +### Fixed + +- PR comment feature flags now accept string values (`'false'`, `'0'`, `'no'`, + `'off'`) as well as booleans. A Socket dashboard config that supplied a flag + as a string previously read as enabled, because `bool("false")` is `True`. + ## [2.2.1] - 2026-07-30 ### Fixed diff --git a/action.yml b/action.yml index dcb6dc7..3a65ae4 100644 --- a/action.yml +++ b/action.yml @@ -92,9 +92,11 @@ runs: INPUT_WEBHOOK_URL: ${{ inputs.webhook_url }} SOCKET_ADDITIONAL_PARAMS: ${{ inputs.socket_additional_params }} SOCKET_TIER_1_ENABLED: ${{ inputs.socket_tier_1_enabled }} + INPUT_PR_COMMENT_ENABLED: ${{ inputs.pr_comment_enabled }} INPUT_PR_COMMENT_LINKS_ENABLED: ${{ inputs.pr_comment_links_enabled }} INPUT_PR_COMMENT_COLLAPSE_ENABLED: ${{ inputs.pr_comment_collapse_enabled }} INPUT_PR_COMMENT_COLLAPSE_NON_CRITICAL: ${{ inputs.pr_comment_collapse_non_critical }} + INPUT_PR_COMMENT_COLLAPSE_ALL: ${{ inputs.pr_comment_collapse_all }} INPUT_PR_COMMENT_CODE_FENCING_ENABLED: ${{ inputs.pr_comment_code_fencing_enabled }} INPUT_PR_COMMENT_SHOW_RULE_NAMES: ${{ inputs.pr_comment_show_rule_names }} INPUT_PR_LABELS_ENABLED: ${{ inputs.pr_labels_enabled }} @@ -443,6 +445,15 @@ inputs: description: "Generic webhook URL for WebhookNotifier" required: false default: "" + pr_comment_enabled: + description: >- + Post the findings comment on the pull request. Set to 'false' to run the + scan silently: findings are still uploaded to the Socket dashboard and the + action still fails the job on high/critical findings, but no comment is + posted or updated. Severity labels are controlled separately by + pr_labels_enabled. + required: false + default: "true" pr_comment_links_enabled: description: "Enable clickable file/line links in PR comments" required: false @@ -455,6 +466,13 @@ inputs: description: "Auto-collapse non-critical findings (critical stays expanded)" required: false default: "true" + pr_comment_collapse_all: + description: >- + Collapse every findings section, including critical ones. Use this when + you want the comment to stay small no matter what it finds. Overrides + pr_comment_collapse_non_critical. + required: false + default: "false" pr_comment_code_fencing_enabled: description: "Enable language-aware code fencing for trace output" required: false diff --git a/docs/github-action.md b/docs/github-action.md index b9ba113..b2863f8 100644 --- a/docs/github-action.md +++ b/docs/github-action.md @@ -326,6 +326,8 @@ jobs: Socket Basics automatically posts enhanced PR comments with **smart defaults that work out of the box** ā€” clickable file links, collapsible sections, syntax highlighting, CVE links, CVSS scores, and auto-labels are all enabled by default. +To run the scan without commenting on the PR at all, set `pr_comment_enabled: 'false'`. The scan still runs, findings are still uploaded to the Socket dashboard, and the job still fails on high/critical findings ā€” only the comment is suppressed. If you want a quieter comment rather than no comment, `pr_comment_collapse_all: 'true'` collapses every section including critical ones. + šŸ“– **[PR Comment Guide ā†’](github-pr-comment-guide.md)** ā€” Complete customization options, configuration examples, and reference table ## Enterprise Features diff --git a/docs/github-pr-comment-guide.md b/docs/github-pr-comment-guide.md index 640e28f..1485268 100644 --- a/docs/github-pr-comment-guide.md +++ b/docs/github-pr-comment-guide.md @@ -90,8 +90,18 @@ pr_comment_collapse_enabled: 'false' # Keep collapsible but expand everything pr_comment_collapse_enabled: 'true' pr_comment_collapse_non_critical: 'false' + +# Keep collapsible and collapse everything, critical included +pr_comment_collapse_enabled: 'true' +pr_comment_collapse_all: 'true' ``` +> [!NOTE] +> `pr_comment_collapse_non_critical` deliberately leaves critical findings +> expanded, so a single critical finding always opens the comment. Set +> `pr_comment_collapse_all: 'true'` when you want the comment to stay small no +> matter what it finds. It overrides `pr_comment_collapse_non_critical`. + --- ### 3. Syntax Highlighting (`pr_comment_code_fencing_enabled`) @@ -291,7 +301,47 @@ The logo is a 32px PNG rendered at 24x24 for retina-crisp display, with a transp --- -### 9. All-Clear Comment Updates +### 9. Turning the Comment Off (`pr_comment_enabled`) + +**Default:** `true` + +Set `pr_comment_enabled: 'false'` to run the scan without saying anything on the +PR. This is for teams who want to review finding quality in the Socket dashboard +first, without every PR growing a comment that developers have to scroll past. + +```yaml +- uses: SocketDev/socket-basics@v2 + with: + socket_security_api_key: ${{ secrets.SOCKET_SECURITY_API_KEY }} + github_token: ${{ secrets.GITHUB_TOKEN }} + pr_comment_enabled: 'false' +``` + +**What still happens when the comment is off:** + +| Behavior | Still happens? | +|----------|----------------| +| Scanners run (SAST, secrets, containers) | āœ… Yes | +| Findings uploaded to the Socket dashboard | āœ… Yes | +| `.socket.facts.json` written | āœ… Yes | +| Job fails on high/critical findings | āœ… Yes | +| Other notifiers (Slack, Jira, webhook, ...) | āœ… Yes | +| Severity labels added to the PR | āœ… Yes, unless `pr_labels_enabled: 'false'` | +| Comment posted or updated | āŒ No | + +Notifiers are the very last thing the run does ā€” the scan finishes and the +findings are uploaded to Socket before any comment would be posted ā€” so turning +the comment off cannot turn the dashboard off. Labels are a separate switch +(`pr_labels_enabled`) so you can keep or drop them independently. + +> [!TIP] +> If you want to keep the comment but make it quieter, use +> `pr_comment_collapse_all: 'true'` instead. That collapses every section, +> including critical ones, so the comment is one line until someone opens it. + +--- + +### 10. All-Clear Comment Updates When a later Socket Basics run no longer has active findings for a previously-reported scanner section, the existing PR comment section is updated in place instead of being left stale or deleted. @@ -313,9 +363,11 @@ When a later Socket Basics run no longer has active findings for a previously-re | Option | Default | Type | Description | |--------|---------|------|-------------| +| `pr_comment_enabled` | `true` | boolean | Post/update the findings comment on the PR | | `pr_comment_links_enabled` | `true` | boolean | Enable clickable file/line links | | `pr_comment_collapse_enabled` | `true` | boolean | Enable collapsible sections | -| `pr_comment_collapse_non_critical` | `true` | boolean | Auto-collapse non-critical findings | +| `pr_comment_collapse_non_critical` | `true` | boolean | Auto-collapse non-critical findings (critical stays expanded) | +| `pr_comment_collapse_all` | `false` | boolean | Collapse every section, critical included | | `pr_comment_code_fencing_enabled` | `true` | boolean | Enable syntax highlighting | | `pr_comment_show_rule_names` | `true` | boolean | Show explicit rule names | | `pr_labels_enabled` | `true` | boolean | Add severity-based labels to PRs | @@ -395,6 +447,21 @@ pr_label_high: 'security' pr_label_medium: 'security' ``` +### Evaluation / Trial (Dashboard Only) + +Review findings in the Socket dashboard without putting anything on the PR: +```yaml +pr_comment_enabled: 'false' +pr_labels_enabled: 'false' +``` + +### Quiet Comment (Everything Collapsed) + +Keep a single collapsed comment even when there are critical findings: +```yaml +pr_comment_collapse_all: 'true' +``` + --- ## šŸš€ Migration Guide diff --git a/scripts/preview_pr_comments.py b/scripts/preview_pr_comments.py index a7e6d55..edb761a 100644 --- a/scripts/preview_pr_comments.py +++ b/scripts/preview_pr_comments.py @@ -36,6 +36,7 @@ def make_mock_config( repo="SocketDev/example-app", commit="a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2", full_scan_url="https://socket.dev/dashboard/scan/12345", + collapse_all=False, ): """Build a mock config object matching the real pipeline shape.""" return MockConfig( @@ -44,6 +45,7 @@ def make_mock_config( pr_comment_links_enabled=True, pr_comment_collapse_enabled=True, pr_comment_collapse_non_critical=True, + pr_comment_collapse_all=collapse_all, pr_comment_code_fencing_enabled=True, pr_comment_show_rule_names=True, full_scan_html_url=full_scan_url, diff --git a/socket_basics/core/connector/opengrep/github_pr.py b/socket_basics/core/connector/opengrep/github_pr.py index 36171ad..9d869ae 100644 --- a/socket_basics/core/connector/opengrep/github_pr.py +++ b/socket_basics/core/connector/opengrep/github_pr.py @@ -34,6 +34,7 @@ def format_notifications(groups: Dict[str, List[Dict[str, Any]]], config=None) - enable_links = flags['enable_links'] enable_collapse = flags['enable_collapse'] collapse_non_critical = flags['collapse_non_critical'] + collapse_all = flags['collapse_all'] enable_code_fencing = flags['enable_code_fencing'] show_rule_names = flags['show_rule_names'] repository = flags['repository'] @@ -202,8 +203,10 @@ def format_notifications(groups: Dict[str, List[Dict[str, Any]]], config=None) - if enable_collapse: # Determine if this should be auto-expanded has_critical = file_severities['critical'] > 0 - # Auto-expand if: no collapse requested OR has critical findings - auto_expand = (not collapse_non_critical) or has_critical + # Auto-expand if: no collapse requested OR has critical findings. + # collapse_all wins over both so the comment can stay small + # even when a critical finding is present. + auto_expand = ((not collapse_non_critical) or has_critical) and not collapse_all collapsible = helpers.create_collapsible_section( display_path, # Don't use backticks in summary - they don't render in GitHub diff --git a/socket_basics/core/connector/socket_tier1/github_pr.py b/socket_basics/core/connector/socket_tier1/github_pr.py index 0e33741..9fd48aa 100644 --- a/socket_basics/core/connector/socket_tier1/github_pr.py +++ b/socket_basics/core/connector/socket_tier1/github_pr.py @@ -34,6 +34,7 @@ def format_notifications(components_list: List[Dict[str, Any]], config=None) -> enable_links = flags['enable_links'] enable_collapse = flags['enable_collapse'] collapse_non_critical = flags['collapse_non_critical'] + collapse_all = flags['collapse_all'] enable_code_fencing = flags['enable_code_fencing'] show_rule_names = flags['show_rule_names'] repository = flags['repository'] @@ -154,7 +155,10 @@ def format_notifications(components_list: List[Dict[str, Any]], config=None) -> severity_summary = " | ".join(severity_parts) if severity_parts else "No issues" - open_attr = ' open' if (not collapse_non_critical or has_critical) else '' + # collapse_all wins over both so the comment can stay small even + # when a critical finding is present. + auto_expand = (not collapse_non_critical or has_critical) and not collapse_all + open_attr = ' open' if auto_expand else '' content_lines.append(f"") content_lines.append(f"{purl} ({severity_summary})") content_lines.append("") diff --git a/socket_basics/core/notification/github_pr_helpers.py b/socket_basics/core/notification/github_pr_helpers.py index d678fc6..54f93f0 100644 --- a/socket_basics/core/notification/github_pr_helpers.py +++ b/socket_basics/core/notification/github_pr_helpers.py @@ -85,6 +85,34 @@ def wrap_pr_comment_section( # Configuration Helper # ============================================================================ +def coerce_bool(value: Any, default: bool) -> bool: + """Coerce a config value to a bool, tolerating the string forms. + + Flags reach us as real booleans from the environment loader, but a Socket + dashboard config can supply them as strings. ``bool("false")`` is ``True``, + so a plain cast would silently turn a disabled flag back on. + + Args: + value: Raw config value (bool, string, None, ...) + default: Value to use when nothing usable was provided + + Returns: + The resolved boolean + """ + if value is None: + return default + if isinstance(value, bool): + return value + if isinstance(value, str): + normalized = value.strip().lower() + if normalized in ('true', '1', 'yes', 'on'): + return True + if normalized in ('false', '0', 'no', 'off'): + return False + return default + return bool(value) + + def get_feature_flags(config) -> Dict[str, Any]: """Extract PR comment feature flags from config object. @@ -99,6 +127,7 @@ def get_feature_flags(config) -> Dict[str, Any]: 'enable_links': True, 'enable_collapse': True, 'collapse_non_critical': True, + 'collapse_all': False, 'enable_code_fencing': True, 'show_rule_names': True, 'repository': '', @@ -107,11 +136,12 @@ def get_feature_flags(config) -> Dict[str, Any]: } return { - 'enable_links': config.get('pr_comment_links_enabled', True), - 'enable_collapse': config.get('pr_comment_collapse_enabled', True), - 'collapse_non_critical': config.get('pr_comment_collapse_non_critical', True), - 'enable_code_fencing': config.get('pr_comment_code_fencing_enabled', True), - 'show_rule_names': config.get('pr_comment_show_rule_names', True), + 'enable_links': coerce_bool(config.get('pr_comment_links_enabled'), True), + 'enable_collapse': coerce_bool(config.get('pr_comment_collapse_enabled'), True), + 'collapse_non_critical': coerce_bool(config.get('pr_comment_collapse_non_critical'), True), + 'collapse_all': coerce_bool(config.get('pr_comment_collapse_all'), False), + 'enable_code_fencing': coerce_bool(config.get('pr_comment_code_fencing_enabled'), True), + 'show_rule_names': coerce_bool(config.get('pr_comment_show_rule_names'), True), 'repository': config.repo if hasattr(config, 'repo') else '', 'commit_hash': config.commit_hash if hasattr(config, 'commit_hash') else '', 'full_scan_url': config.get('full_scan_html_url') if config else None diff --git a/socket_basics/core/notification/github_pr_notifier.py b/socket_basics/core/notification/github_pr_notifier.py index a87e1ea..4636d91 100644 --- a/socket_basics/core/notification/github_pr_notifier.py +++ b/socket_basics/core/notification/github_pr_notifier.py @@ -3,6 +3,7 @@ from urllib.parse import quote from socket_basics.core.notification.base import BaseNotifier +from socket_basics.core.notification.github_pr_helpers import coerce_bool from socket_basics.core.config import get_github_token, get_github_repository, get_github_pr_number logger = logging.getLogger(__name__) @@ -37,7 +38,8 @@ def __init__(self, params: Dict[str, Any] | None = None): def notify(self, facts: Dict[str, Any]) -> None: notifications = facts.get('notifications', []) or [] labels_enabled = self.config.get('pr_labels_enabled', True) - + comment_enabled = coerce_bool(self.config.get('pr_comment_enabled'), True) + if not isinstance(notifications, list): logger.error('GithubPRNotifier: only supports new format - list of dicts with title/content') return @@ -57,6 +59,24 @@ def notify(self, facts: Dict[str, Any]) -> None: notification_section_types = self._extract_section_types_from_notifications(valid_notifications) facts_section_types = self._infer_section_types_from_facts(facts) + # Comment suppression: the scan has already run and the findings have + # already been uploaded to the Socket dashboard by the time notifiers + # execute, so this only silences the PR comment. Severity labels stay + # under pr_labels_enabled so the two can be turned off independently. + if not comment_enabled: + logger.info( + 'GithubPRNotifier: PR comments disabled (pr_comment_enabled=false); ' + '%d finding section(s) were scanned and uploaded to the Socket dashboard ' + 'but no comment will be posted or updated', + len(valid_notifications), + ) + if labels_enabled: + pr_number = self._get_pr_number() + if pr_number: + labels = self._determine_pr_labels(valid_notifications) + self._reconcile_pr_labels(pr_number, labels) + return + if not valid_notifications: pr_number = self._get_pr_number() if pr_number: diff --git a/socket_basics/notifications.yaml b/socket_basics/notifications.yaml index 02c2e3d..4782108 100644 --- a/socket_basics/notifications.yaml +++ b/socket_basics/notifications.yaml @@ -90,6 +90,12 @@ notifiers: option: --github-api-url env_variable: GITHUB_API_URL type: str + - name: pr_comment_enabled + option: --pr-comment + env_variable: INPUT_PR_COMMENT_ENABLED + type: bool + default: true + description: "Post/update the findings comment on the PR (scanning and dashboard upload are unaffected)" - name: pr_comment_links_enabled option: --pr-comment-links env_variable: INPUT_PR_COMMENT_LINKS_ENABLED @@ -108,6 +114,12 @@ notifiers: type: bool default: true description: "Auto-collapse non-critical findings (critical stays expanded)" + - name: pr_comment_collapse_all + option: --pr-comment-collapse-all + env_variable: INPUT_PR_COMMENT_COLLAPSE_ALL + type: bool + default: false + description: "Collapse every findings section, including critical ones" - name: pr_comment_code_fencing_enabled option: --pr-comment-code-fencing env_variable: INPUT_PR_COMMENT_CODE_FENCING_ENABLED diff --git a/tests/test_github_pr_notifier.py b/tests/test_github_pr_notifier.py index 196e150..98cdb92 100644 --- a/tests/test_github_pr_notifier.py +++ b/tests/test_github_pr_notifier.py @@ -347,3 +347,118 @@ def test_notify_zero_alert_enabled_sast_config_rewrites_matching_section(monkeyp assert len(updated_bodies) == 1 assert 'Socket Basics found no active findings in the latest run.' in updated_bodies[0] assert '' in updated_bodies[0] + + +# --------------------------------------------------------------------------- +# pr_comment_enabled: suppress the PR comment without suppressing the scan +# --------------------------------------------------------------------------- + +def _suppression_notifier(monkeypatch, **params): + """A notifier whose every GitHub write is recorded instead of performed.""" + notifier = GithubPRNotifier({'repository': 'SocketDev/socket-basics', **params}) + calls = {'posted': [], 'updated': [], 'labels': []} + + monkeypatch.setattr(notifier, '_get_pr_number', lambda: 123) + monkeypatch.setattr(notifier, '_get_pr_comments', lambda pr_number: []) + monkeypatch.setattr( + notifier, '_post_comment', lambda pr_number, body: calls['posted'].append(body) or True + ) + monkeypatch.setattr( + notifier, + '_update_comment', + lambda pr_number, comment_id, body: calls['updated'].append(body) or True, + ) + monkeypatch.setattr( + notifier, + '_reconcile_pr_labels', + lambda pr_number, labels: calls['labels'].append(labels) or True, + ) + return notifier, calls + + +_FINDING = { + 'title': 'Socket SAST JavaScript', + 'content': '\nšŸ”“ Critical: 1\n', +} + + +def test_notify_posts_comment_by_default(monkeypatch): + notifier, calls = _suppression_notifier(monkeypatch) + + notifier.notify({'notifications': [_FINDING], 'components': []}) + + assert len(calls['posted']) == 1 + + +def test_notify_posts_no_comment_when_pr_comment_disabled(monkeypatch): + notifier, calls = _suppression_notifier(monkeypatch, pr_comment_enabled=False) + + notifier.notify({'notifications': [_FINDING], 'components': []}) + + assert calls['posted'] == [] + assert calls['updated'] == [] + + +def test_notify_posts_no_all_clear_comment_when_pr_comment_disabled(monkeypatch): + """An empty run must not rewrite an existing comment into "no findings".""" + existing = """ +## Socket SAST JavaScript + +### Summary +šŸŸ  High: 1 +""" + + notifier, calls = _suppression_notifier(monkeypatch, pr_comment_enabled=False) + notifier.app_config = {'javascript_sast_enabled': True} + monkeypatch.setattr(notifier, '_get_pr_comments', lambda pr_number: [{'id': 99, 'body': existing}]) + + notifier.notify({'notifications': [], 'components': []}) + + assert calls['posted'] == [] + assert calls['updated'] == [] + + +def test_notify_still_applies_labels_when_pr_comment_disabled(monkeypatch): + """Comments and labels are independent switches.""" + notifier, calls = _suppression_notifier( + monkeypatch, pr_comment_enabled=False, pr_labels_enabled=True + ) + + notifier.notify({'notifications': [_FINDING], 'components': []}) + + assert calls['labels'] == [['security: critical']] + + +def test_notify_skips_labels_when_both_switches_are_off(monkeypatch): + notifier, calls = _suppression_notifier( + monkeypatch, pr_comment_enabled=False, pr_labels_enabled=False + ) + + notifier.notify({'notifications': [_FINDING], 'components': []}) + + assert calls['posted'] == [] + assert calls['labels'] == [] + + +def test_notify_honors_string_false_from_dashboard_config(monkeypatch): + """A Socket dashboard config supplies flags as strings, not booleans.""" + notifier, calls = _suppression_notifier(monkeypatch, pr_comment_enabled='false') + + notifier.notify({'notifications': [_FINDING], 'components': []}) + + assert calls['posted'] == [] + + +def test_notify_leaves_facts_untouched_when_pr_comment_disabled(monkeypatch): + """Suppression is comment-only: the uploaded facts payload is not altered.""" + notifier, _calls = _suppression_notifier(monkeypatch, pr_comment_enabled=False) + facts = { + 'notifications': [_FINDING], + 'components': [{'id': 'src/index.js', 'alerts': [{'severity': 'critical'}]}], + 'full_scan_html_url': 'https://socket.dev/dashboard/scan/12345', + } + + notifier.notify(facts) + + assert facts['components'] == [{'id': 'src/index.js', 'alerts': [{'severity': 'critical'}]}] + assert facts['full_scan_html_url'] == 'https://socket.dev/dashboard/scan/12345' diff --git a/tests/test_pr_formatters.py b/tests/test_pr_formatters.py index 564ad0a..cf0e276 100644 --- a/tests/test_pr_formatters.py +++ b/tests/test_pr_formatters.py @@ -356,3 +356,72 @@ def test_tier1_empty_list(self, config): results = format_notifications([], config=config) assert len(results) == 1 assert "No reachability issues found" in results[0]["content"] + + +# --------------------------------------------------------------------------- +# pr_comment_collapse_all: keep every section collapsed, critical included +# --------------------------------------------------------------------------- + +@pytest.fixture +def config_collapse_all(): + return make_mock_config(collapse_all=True) + + +class TestCollapseAll: + """`pr_comment_collapse_all` is the only way to collapse critical findings. + + `pr_comment_collapse_non_critical` deliberately leaves critical sections + expanded, so before this flag existed a critical finding always forced the + comment open. + """ + + def test_opengrep_critical_section_is_expanded_by_default(self, config): + from socket_basics.core.connector.opengrep.github_pr import format_notifications + results = format_notifications(OPENGREP_FIXTURES, config=config) + js_content = next(r["content"] for r in results if "JavaScript" in r["title"]) + assert "
" in js_content + + def test_opengrep_collapses_critical_section_when_enabled(self, config_collapse_all): + from socket_basics.core.connector.opengrep.github_pr import format_notifications + results = format_notifications(OPENGREP_FIXTURES, config=config_collapse_all) + js_content = next(r["content"] for r in results if "JavaScript" in r["title"]) + assert "
" not in js_content + # Sections are still present, just closed. + assert "
" in js_content + + def test_tier1_critical_section_is_expanded_by_default(self, config): + from socket_basics.core.connector.socket_tier1.github_pr import format_notifications + content = format_notifications(TIER1_FIXTURES, config=config)[0]["content"] + assert "
" in content + + def test_tier1_collapses_critical_section_when_enabled(self, config_collapse_all): + from socket_basics.core.connector.socket_tier1.github_pr import format_notifications + content = format_notifications(TIER1_FIXTURES, config=config_collapse_all)[0]["content"] + assert "
" not in content + assert "
" in content + + +class TestFeatureFlagCoercion: + """Dashboard-sourced config can deliver flags as strings, not booleans.""" + + def test_string_true_enables_collapse_all(self): + from socket_basics.core.notification.github_pr_helpers import get_feature_flags + flags = get_feature_flags(make_mock_config(collapse_all="true")) + assert flags["collapse_all"] is True + + def test_string_false_does_not_enable_collapse_all(self): + from socket_basics.core.notification.github_pr_helpers import get_feature_flags + flags = get_feature_flags(make_mock_config(collapse_all="false")) + assert flags["collapse_all"] is False + + def test_string_false_disables_links(self): + from socket_basics.core.notification.github_pr_helpers import get_feature_flags + config = make_mock_config() + config["pr_comment_links_enabled"] = "false" + assert get_feature_flags(config)["enable_links"] is False + + def test_missing_config_keeps_documented_defaults(self): + from socket_basics.core.notification.github_pr_helpers import get_feature_flags + flags = get_feature_flags(None) + assert flags["collapse_all"] is False + assert flags["collapse_non_critical"] is True From 8322efa1e8acd28f8994295ce676daa668496f09 Mon Sep 17 00:00:00 2001 From: jdalton Date: Mon, 3 Aug 2026 15:11:03 -0400 Subject: [PATCH 2/3] fix: coerce pr_labels_enabled the same way as the comment flags Bugbot caught that pr_labels_enabled was still a raw truthiness read, so a Socket dashboard config supplying 'false' as a string kept applying severity labels -- including on the new comments-disabled path, where a user who turned both off would still get labels. --- CHANGELOG.md | 7 +++--- .../core/notification/github_pr_notifier.py | 2 +- tests/test_github_pr_notifier.py | 22 +++++++++++++++++++ 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a238d56..811c3aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,9 +22,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed -- PR comment feature flags now accept string values (`'false'`, `'0'`, `'no'`, - `'off'`) as well as booleans. A Socket dashboard config that supplied a flag - as a string previously read as enabled, because `bool("false")` is `True`. +- PR comment feature flags, including `pr_labels_enabled`, now accept string + values (`'false'`, `'0'`, `'no'`, `'off'`) as well as booleans. A Socket + dashboard config that supplied a flag as a string previously read as enabled, + because `bool("false")` is `True`. ## [2.2.1] - 2026-07-30 diff --git a/socket_basics/core/notification/github_pr_notifier.py b/socket_basics/core/notification/github_pr_notifier.py index 4636d91..a7bcca4 100644 --- a/socket_basics/core/notification/github_pr_notifier.py +++ b/socket_basics/core/notification/github_pr_notifier.py @@ -37,7 +37,7 @@ def __init__(self, params: Dict[str, Any] | None = None): def notify(self, facts: Dict[str, Any]) -> None: notifications = facts.get('notifications', []) or [] - labels_enabled = self.config.get('pr_labels_enabled', True) + labels_enabled = coerce_bool(self.config.get('pr_labels_enabled'), True) comment_enabled = coerce_bool(self.config.get('pr_comment_enabled'), True) if not isinstance(notifications, list): diff --git a/tests/test_github_pr_notifier.py b/tests/test_github_pr_notifier.py index 98cdb92..6752716 100644 --- a/tests/test_github_pr_notifier.py +++ b/tests/test_github_pr_notifier.py @@ -449,6 +449,28 @@ def test_notify_honors_string_false_from_dashboard_config(monkeypatch): assert calls['posted'] == [] +def test_notify_honors_string_false_for_labels(monkeypatch): + """pr_labels_enabled needs the same string handling as pr_comment_enabled.""" + notifier, calls = _suppression_notifier( + monkeypatch, pr_comment_enabled='false', pr_labels_enabled='false' + ) + + notifier.notify({'notifications': [_FINDING], 'components': []}) + + assert calls['posted'] == [] + assert calls['labels'] == [] + + +def test_notify_string_false_labels_are_skipped_on_the_normal_path(monkeypatch): + """Not just the suppression branch -- the ordinary posting path too.""" + notifier, calls = _suppression_notifier(monkeypatch, pr_labels_enabled='false') + + notifier.notify({'notifications': [_FINDING], 'components': []}) + + assert len(calls['posted']) == 1 + assert calls['labels'] == [] + + def test_notify_leaves_facts_untouched_when_pr_comment_disabled(monkeypatch): """Suppression is comment-only: the uploaded facts payload is not altered.""" notifier, _calls = _suppression_notifier(monkeypatch, pr_comment_enabled=False) From afba61a7584df90d1abc5f795bac8ed8f860f609 Mon Sep 17 00:00:00 2001 From: jdalton Date: Tue, 4 Aug 2026 18:34:00 -0400 Subject: [PATCH 3/3] fix(config): read a blank PR comment flag as unset, not as off The environment loader compared the raw input against the single string 'true', so a flag arrived off unless it was spelled exactly that way. An action input forwarded from an unset workflow variable arrives as an empty string, which meant pr_comment_enabled read as false and the comment disappeared from a workflow that never asked for that. '1', 'yes' and 'on' did not turn it on either, even though the dashboard path already accepted them. coerce_bool now lives in the config layer and both paths use it, so a value that says nothing falls back to the flag's documented default and every spelling behaves the same from an action input, a JSON config and a Socket dashboard config. Documents the truth table in the PR comment guide, and adds the two new switches to the README feature list. --- CHANGELOG.md | 6 ++ README.md | 3 +- docs/github-pr-comment-guide.md | 12 ++++ socket_basics/core/config.py | 34 ++++++++++- .../core/notification/github_pr_helpers.py | 32 ++-------- tests/test_pr_formatters.py | 60 +++++++++++++++++++ 6 files changed, 117 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 811c3aa..2146702 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). values (`'false'`, `'0'`, `'no'`, `'off'`) as well as booleans. A Socket dashboard config that supplied a flag as a string previously read as enabled, because `bool("false")` is `True`. +- A blank PR comment flag now falls back to its documented default instead of + reading as off. The environment loader compared the raw value against the + single string `'true'`, so an action input forwarded from an unset workflow + variable arrived as `''` and disabled the flag, and `'1'`, `'yes'` and `'on'` + did not enable it. All the spellings the flags document now work from the + environment, a JSON config and a Socket dashboard config alike. ## [2.2.1] - 2026-07-30 diff --git a/README.md b/README.md index d13d9e1..b93bae7 100644 --- a/README.md +++ b/README.md @@ -107,11 +107,12 @@ Socket Basics can also run locally or in other CI/CD environments: Socket Basics delivers **beautifully formatted, actionable PR comments** with smart defaults ā€” all enabled by default, zero configuration needed. - šŸ”— **Clickable File Links** ā€” Jump directly to the vulnerable code in GitHub -- šŸ“‹ **Collapsible Sections** ā€” Critical findings auto-expand, others collapse +- šŸ“‹ **Collapsible Sections** ā€” Critical findings auto-expand, others collapse; `pr_comment_collapse_all: 'true'` collapses critical ones too - šŸŽØ **Syntax Highlighting** ā€” Language-aware code blocks - šŸ·ļø **Auto-Labels** ā€” PRs tagged with severity-based labels (e.g., `security: critical`) - šŸ”“ **CVE Links & CVSS Scores** ā€” One-click access to NVD with risk context - šŸš€ **Full Scan Link** ā€” Report link prominently displayed at the top +- šŸ”‡ **Fully Suppressible** ā€” `pr_comment_enabled: 'false'` stops the comment entirely while the scan still runs, still uploads to the Socket dashboard, and still fails the job on high/critical findings Every feature is customizable via GitHub Actions inputs, CLI flags, or environment variables. diff --git a/docs/github-pr-comment-guide.md b/docs/github-pr-comment-guide.md index 1485268..d119780 100644 --- a/docs/github-pr-comment-guide.md +++ b/docs/github-pr-comment-guide.md @@ -376,6 +376,18 @@ When a later Socket Basics run no longer has active findings for a previously-re | `pr_label_medium` | `"security: medium"` | string | Label name for medium findings | | `pr_label_low` | `"security: low"` | string | Label name for low findings | +### How boolean options are read + +Every boolean above accepts `true`, `1`, `yes` and `on` for on, and `false`, +`0`, `no` and `off` for off, in any capitalization, whether it arrives as a +GitHub Action input, an environment variable, a `--config` JSON file, or a +Socket dashboard config. + +A value that says nothing ā€” blank, whitespace, or a word that is neither ā€” falls +back to the default in the table. This matters for `pr_comment_enabled`: passing +it a workflow variable that turns out to be unset gives the action an empty +string, and that leaves the comment on rather than silently switching it off. + ### Configuration Methods **1. GitHub Actions (Recommended)** diff --git a/socket_basics/core/config.py b/socket_basics/core/config.py index ac623b4..b3ec0df 100644 --- a/socket_basics/core/config.py +++ b/socket_basics/core/config.py @@ -14,6 +14,38 @@ logger = logging.getLogger(__name__) +TRUE_STRINGS = ('true', '1', 'yes', 'on') +FALSE_STRINGS = ('false', '0', 'no', 'off') + + +def coerce_bool(value: Any, default: bool) -> bool: + """Coerce a config value to a bool, tolerating the string forms. + + Every layer that carries a flag delivers it differently: the environment + loader sees strings because GitHub Action inputs are always strings, a + Socket dashboard config can send either, and a JSON config sends real + booleans. ``bool("false")`` is ``True``, so a plain cast turns a disabled + flag back on. + + A value that says nothing -- ``None``, an empty string, or a word that is + neither true nor false -- resolves to ``default`` rather than to ``False``. + An action input forwarded from an unset workflow variable arrives as an + empty string, and reading that as "off" silently disables a flag that + nobody asked to disable. + """ + if value is None: + return default + if isinstance(value, bool): + return value + if isinstance(value, str): + normalized = value.strip().lower() + if normalized in TRUE_STRINGS: + return True + if normalized in FALSE_STRINGS: + return False + return default + return bool(value) + def _normalize_path_parts(path_value: str | None) -> List[str] | None: """Normalize a path-like string into comparable POSIX-style path segments.""" @@ -812,7 +844,7 @@ def load_config_from_env() -> Dict[str, Any]: env_value = os.getenv(env_var) if env_value is not None: if p_type == 'bool': - config[p_name] = env_value.lower() == 'true' + config[p_name] = coerce_bool(env_value, bool(default_value)) elif p_type == 'int': try: config[p_name] = int(env_value) diff --git a/socket_basics/core/notification/github_pr_helpers.py b/socket_basics/core/notification/github_pr_helpers.py index 54f93f0..311e70c 100644 --- a/socket_basics/core/notification/github_pr_helpers.py +++ b/socket_basics/core/notification/github_pr_helpers.py @@ -14,6 +14,10 @@ from typing import Dict, Any, Optional, List, Tuple from pathlib import Path +# coerce_bool lives in the config layer so the environment loader, a Socket +# dashboard config and these flags all read a string the same way. +from socket_basics.core.config import coerce_bool + # ============================================================================ # Severity Constants (shared across all scanners) @@ -85,34 +89,6 @@ def wrap_pr_comment_section( # Configuration Helper # ============================================================================ -def coerce_bool(value: Any, default: bool) -> bool: - """Coerce a config value to a bool, tolerating the string forms. - - Flags reach us as real booleans from the environment loader, but a Socket - dashboard config can supply them as strings. ``bool("false")`` is ``True``, - so a plain cast would silently turn a disabled flag back on. - - Args: - value: Raw config value (bool, string, None, ...) - default: Value to use when nothing usable was provided - - Returns: - The resolved boolean - """ - if value is None: - return default - if isinstance(value, bool): - return value - if isinstance(value, str): - normalized = value.strip().lower() - if normalized in ('true', '1', 'yes', 'on'): - return True - if normalized in ('false', '0', 'no', 'off'): - return False - return default - return bool(value) - - def get_feature_flags(config) -> Dict[str, Any]: """Extract PR comment feature flags from config object. diff --git a/tests/test_pr_formatters.py b/tests/test_pr_formatters.py index cf0e276..31c12ba 100644 --- a/tests/test_pr_formatters.py +++ b/tests/test_pr_formatters.py @@ -425,3 +425,63 @@ def test_missing_config_keeps_documented_defaults(self): flags = get_feature_flags(None) assert flags["collapse_all"] is False assert flags["collapse_non_critical"] is True + + +class TestActionInputCoercion: + """GitHub Action inputs are always strings, and blank means "unset". + + An input forwarded from an unset workflow variable arrives as an empty + string. Reading that as "off" would silently suppress the PR comment for a + workflow that never asked to suppress it, so a value that says nothing has + to fall back to the documented default. + """ + + def _load(self, monkeypatch, **env): + from socket_basics.core.config import load_config_from_env + for name in ( + "INPUT_PR_COMMENT_ENABLED", + "INPUT_PR_COMMENT_COLLAPSE_ALL", + "INPUT_PR_LABELS_ENABLED", + ): + monkeypatch.delenv(name, raising=False) + for name, value in env.items(): + monkeypatch.setenv(name, value) + return load_config_from_env() + + def test_comment_stays_enabled_when_the_input_is_unset(self, monkeypatch): + assert self._load(monkeypatch).get("pr_comment_enabled") is True + + def test_comment_stays_enabled_for_a_blank_input(self, monkeypatch): + config = self._load(monkeypatch, INPUT_PR_COMMENT_ENABLED="") + assert config.get("pr_comment_enabled") is True + + def test_comment_stays_enabled_for_a_whitespace_input(self, monkeypatch): + config = self._load(monkeypatch, INPUT_PR_COMMENT_ENABLED=" ") + assert config.get("pr_comment_enabled") is True + + def test_comment_stays_enabled_for_an_unrecognized_input(self, monkeypatch): + config = self._load(monkeypatch, INPUT_PR_COMMENT_ENABLED="maybe") + assert config.get("pr_comment_enabled") is True + + @pytest.mark.parametrize("value", ["false", "False", "FALSE", "0", "no", "off"]) + def test_comment_is_disabled_by_every_false_spelling(self, monkeypatch, value): + config = self._load(monkeypatch, INPUT_PR_COMMENT_ENABLED=value) + assert config.get("pr_comment_enabled") is False + + @pytest.mark.parametrize("value", ["true", "True", "TRUE", "1", "yes", "on"]) + def test_comment_is_enabled_by_every_true_spelling(self, monkeypatch, value): + config = self._load(monkeypatch, INPUT_PR_COMMENT_ENABLED=value) + assert config.get("pr_comment_enabled") is True + + def test_collapse_all_stays_off_for_a_blank_input(self, monkeypatch): + """A blank input falls back to the default, which here is off.""" + config = self._load(monkeypatch, INPUT_PR_COMMENT_COLLAPSE_ALL="") + assert config.get("pr_comment_collapse_all") is False + + def test_collapse_all_is_enabled_by_a_string_true(self, monkeypatch): + config = self._load(monkeypatch, INPUT_PR_COMMENT_COLLAPSE_ALL="true") + assert config.get("pr_comment_collapse_all") is True + + def test_labels_stay_enabled_for_a_blank_input(self, monkeypatch): + config = self._load(monkeypatch, INPUT_PR_LABELS_ENABLED="") + assert config.get("pr_labels_enabled") is True