Skip to content

feat: add pr_comment_enabled and pr_comment_collapse_all PR comment controls - #97

Open
John-David Dalton (jdalton) wants to merge 2 commits into
mainfrom
feat/pr-comment-suppression
Open

feat: add pr_comment_enabled and pr_comment_collapse_all PR comment controls#97
John-David Dalton (jdalton) wants to merge 2 commits into
mainfrom
feat/pr-comment-suppression

Conversation

@jdalton

@jdalton John-David Dalton (jdalton) commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Right now the Socket Basics PR comment cannot be turned off. There is a setting to auto-collapse non-critical findings, but critical findings always stay expanded, so a single critical finding forces the whole comment open on every push. A team that wants to evaluate finding quality in the Socket dashboard first has no way to keep the scan running without the comment landing on every developer's PR.

This PR adds two independent switches so a team can pick how quiet they want the PR to be:

Input Default What it does
pr_comment_enabled true Set to false and no comment is posted or updated at all.
pr_comment_collapse_all false Set to true and every findings section is collapsed, including critical ones.

The scan and the dashboard upload are unaffected either way. With pr_comment_enabled: 'false' the scanners still run, the findings are still uploaded to the Socket dashboard, .socket.facts.json is still written, the job still fails on high/critical findings, and Slack/Jira/webhook notifiers still fire. Only the comment is suppressed.

Reference: SURF-1451.

Suppressing the comment cannot suppress the dashboard, because notifiers run last

socket_basics/socket_basics.py main() runs in a fixed order:

  1. scanner.run_all_scans() — scanners execute
  2. scanner.save_results(...) — writes .socket.facts.json
  3. scanner.submit_socket_facts(...) — uploads to the Socket dashboard
  4. exit code computed from high/critical findings
  5. scanner.notification_manager.notify_all(results) — notifiers, including the GitHub PR comment

The new switch lives in step 5, inside GithubPRNotifier.notify(). Everything the customer cares about keeping has already happened by then. There is no code path where the comment flag reaches the scanner or the uploader.

Why "collapse all" is a separate setting rather than a change to the existing one

pr_comment_collapse_non_critical means "collapse the sections that are not critical". Both formatters that build collapsible sections computed expansion the same way:

# opengrep
auto_expand = (not collapse_non_critical) or has_critical
# socket_tier1
open_attr = ' open' if (not collapse_non_critical or has_critical) else ''

has_critical is an unconditional OR, so no value of pr_comment_collapse_non_critical can collapse a critical section. That is the documented intent of the setting, and some teams rely on it, so changing its meaning would be a silent behavior change for everyone else. pr_comment_collapse_all is a new flag that wins over both:

auto_expand = ((not collapse_non_critical) or has_critical) and not collapse_all

The Trivy formatter already emitted a plain <details> with no expansion logic, so it needed no change.

Labels stayed on their own switch, and a string-vs-boolean bug got fixed on the way past

Labels. pr_labels_enabled already exists and already defaults to true. Folding label suppression into pr_comment_enabled would mean a team that wants labels but no comment cannot have that. So when comments are off, notify() still reconciles labels if pr_labels_enabled is true, and there is a test for each of the four on/off combinations.

String booleans. get_feature_flags() read every flag with a bare config.get(...). Flags arriving from the environment loader are real Python booleans, but a Socket dashboard config can deliver them as strings — and bool("false") is True, so a flag disabled in the dashboard read as enabled. All the PR comment flags now go through a new coerce_bool() helper in github_pr_helpers.py, which accepts true/1/yes/on and false/0/no/off in either case and passes real booleans through untouched. This is a strict improvement for the existing flags, not just the new ones.

Files changed and where each switch is read
File Change
action.yml Two new inputs plus their INPUT_* env mappings
socket_basics/notifications.yaml Two new github_pr notifier parameters, matching the shape of the five existing pr_comment_* ones
socket_basics/core/notification/github_pr_notifier.py Early return in notify() when comments are disabled; labels still reconciled
socket_basics/core/notification/github_pr_helpers.py New coerce_bool(); collapse_all added to the feature-flag dict
socket_basics/core/connector/opengrep/github_pr.py Honors collapse_all
socket_basics/core/connector/socket_tier1/github_pr.py Honors collapse_all
scripts/preview_pr_comments.py Mock config gained collapse_all so previews and formatter tests can exercise it
docs/github-pr-comment-guide.md, docs/github-action.md Documented both switches, including a table of what still happens when the comment is off
CHANGELOG.md Entries under [Unreleased]

Testing

Ran — exit codes read directly from the harness, not through a pipe.

Command Result
uv run --no-sync pytest -q tests/ exit 0, 233 passed (was 216 on main; 17 new)
python scripts/sync_release_version.py --check exit 0, version metadata in sync at 2.2.1
YAML parse of action.yml and notifications.yaml OK

New tests: 8 in tests/test_github_pr_notifier.py covering the suppression switch, 4 in tests/test_pr_formatters.py::TestCollapseAll, and 4 in tests/test_pr_formatters.py::TestFeatureFlagCoercion.

Mutation checks: every new behavior was broken on purpose and a named test went red

Each mutation was applied, the suite was run, the mutation was reverted, and the suite was re-run green.

Mutation Named tests that failed Exit
comment_enabled hard-coded to True test_notify_posts_no_comment_when_pr_comment_disabled, test_notify_posts_no_all_clear_comment_when_pr_comment_disabled, test_notify_skips_labels_when_both_switches_are_off, test_notify_honors_string_false_from_dashboard_config 1
Suppression branch also drops labels test_notify_still_applies_labels_when_pr_comment_disabled 1
OpenGrep formatter ignores collapse_all TestCollapseAll::test_opengrep_collapses_critical_section_when_enabled 1
Tier 1 formatter ignores collapse_all TestCollapseAll::test_tier1_collapses_critical_section_when_enabled 1
coerce_bool degraded to a plain bool() cast test_notify_honors_string_false_from_dashboard_config, TestFeatureFlagCoercion::test_string_false_does_not_enable_collapse_all, TestFeatureFlagCoercion::test_string_false_disables_links 1

After restoring all five: exit 0, 231 passed.

Review feedback addressed: pr_labels_enabled had the same string-boolean bug

Bugbot pointed out that pr_labels_enabled was still a raw config.get(...) truthiness read while pr_comment_enabled went through coerce_bool(). It was right, and it mattered for exactly the case this PR is about: a team that sets both pr_comment_enabled: 'false' and pr_labels_enabled: 'false' in a Socket dashboard config would get no comment but would still get labels, because bool("false") is True.

labels_enabled now goes through coerce_bool() too. Two tests cover it — one on the comments-disabled path and one on the ordinary posting path. Mutation check: reverting to the raw read fails test_notify_honors_string_false_for_labels and test_notify_string_false_labels_are_skipped_on_the_normal_path, exit 1. Suite after the fix: exit 0, 233 passed.

Did not run

  • No live GitHub PR was commented on. The notifier tests monkeypatch _post_comment, _update_comment, and _reconcile_pr_labels and assert on what would have been sent, so no network call is made.
  • The Docker image was not rebuilt. Both switches are plain config reads with no new dependency, and action.yml still points at the released 2.2.1 image; the image reference is bumped at release time, not here.
  • No version bump, no tag. pyproject.toml and socket_basics/version.py are untouched.

Note

Low Risk
Changes are limited to PR comment formatting and notifier gating; scan and upload paths are unchanged. The string-to-boolean fix may flip behavior for dashboard configs that sent flags as strings, which is intentional but worth noting for Enterprise dashboard users.

Overview
Adds two GitHub Action inputs for quieter PR feedback: pr_comment_enabled (default true) skips posting or updating findings comments while scans, Socket dashboard upload, .socket.facts.json, high/critical job failure, and other notifiers keep running; pr_comment_collapse_all (default false) collapses every findings section, including critical, and overrides pr_comment_collapse_non_critical.

GithubPRNotifier returns early when comments are disabled (no new posts, no all-clear rewrites) but still reconciles severity labels when pr_labels_enabled is on. OpenGrep and Socket Tier 1 PR formatters apply collapse_all when deciding whether <details> sections stay open.

Fixes PR comment feature flags (including pr_labels_enabled) so dashboard string values like 'false' are parsed correctly via new coerce_bool() in github_pr_helpers, instead of treating bool("false") as enabled. Wired through action.yml, notifications.yaml, docs, changelog, and tests.

Reviewed by Cursor Bugbot for commit 8322efa. Configure here.

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
@jdalton
John-David Dalton (jdalton) requested a review from a team as a code owner August 3, 2026 18:29
Comment thread socket_basics/core/notification/github_pr_notifier.py
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.
@jdalton

Copy link
Copy Markdown
Contributor Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 8322efa. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant