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
Open
feat: add pr_comment_enabled and pr_comment_collapse_all PR comment controls#97John-David Dalton (jdalton) wants to merge 2 commits into
John-David Dalton (jdalton) wants to merge 2 commits into
Conversation
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
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.
Contributor
Author
|
bugbot run |
There was a problem hiding this comment.
✅ 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
pr_comment_enabledtruefalseand no comment is posted or updated at all.pr_comment_collapse_allfalsetrueand 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.jsonis 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.pymain()runs in a fixed order:scanner.run_all_scans()— scanners executescanner.save_results(...)— writes.socket.facts.jsonscanner.submit_socket_facts(...)— uploads to the Socket dashboardscanner.notification_manager.notify_all(results)— notifiers, including the GitHub PR commentThe 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_criticalmeans "collapse the sections that are not critical". Both formatters that build collapsible sections computed expansion the same way:has_criticalis an unconditional OR, so no value ofpr_comment_collapse_non_criticalcan 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_allis a new flag that wins over both: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_enabledalready exists and already defaults totrue. Folding label suppression intopr_comment_enabledwould mean a team that wants labels but no comment cannot have that. So when comments are off,notify()still reconciles labels ifpr_labels_enabledis true, and there is a test for each of the four on/off combinations.String booleans.
get_feature_flags()read every flag with a bareconfig.get(...). Flags arriving from the environment loader are real Python booleans, but a Socket dashboard config can deliver them as strings — andbool("false")isTrue, so a flag disabled in the dashboard read as enabled. All the PR comment flags now go through a newcoerce_bool()helper ingithub_pr_helpers.py, which acceptstrue/1/yes/onandfalse/0/no/offin 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
action.ymlINPUT_*env mappingssocket_basics/notifications.yamlgithub_prnotifier parameters, matching the shape of the five existingpr_comment_*onessocket_basics/core/notification/github_pr_notifier.pynotify()when comments are disabled; labels still reconciledsocket_basics/core/notification/github_pr_helpers.pycoerce_bool();collapse_alladded to the feature-flag dictsocket_basics/core/connector/opengrep/github_pr.pycollapse_allsocket_basics/core/connector/socket_tier1/github_pr.pycollapse_allscripts/preview_pr_comments.pycollapse_allso previews and formatter tests can exercise itdocs/github-pr-comment-guide.md,docs/github-action.mdCHANGELOG.md[Unreleased]Testing
Ran — exit codes read directly from the harness, not through a pipe.
uv run --no-sync pytest -q tests/main; 17 new)python scripts/sync_release_version.py --checkaction.ymlandnotifications.yamlNew tests: 8 in
tests/test_github_pr_notifier.pycovering the suppression switch, 4 intests/test_pr_formatters.py::TestCollapseAll, and 4 intests/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.
comment_enabledhard-coded toTruetest_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_configtest_notify_still_applies_labels_when_pr_comment_disabledcollapse_allTestCollapseAll::test_opengrep_collapses_critical_section_when_enabledcollapse_allTestCollapseAll::test_tier1_collapses_critical_section_when_enabledcoerce_booldegraded to a plainbool()casttest_notify_honors_string_false_from_dashboard_config,TestFeatureFlagCoercion::test_string_false_does_not_enable_collapse_all,TestFeatureFlagCoercion::test_string_false_disables_linksAfter 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_enabledwas still a rawconfig.get(...)truthiness read whilepr_comment_enabledwent throughcoerce_bool(). It was right, and it mattered for exactly the case this PR is about: a team that sets bothpr_comment_enabled: 'false'andpr_labels_enabled: 'false'in a Socket dashboard config would get no comment but would still get labels, becausebool("false")isTrue.labels_enablednow goes throughcoerce_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 failstest_notify_honors_string_false_for_labelsandtest_notify_string_false_labels_are_skipped_on_the_normal_path, exit 1. Suite after the fix: exit 0, 233 passed.Did not run
_post_comment,_update_comment, and_reconcile_pr_labelsand assert on what would have been sent, so no network call is made.action.ymlstill points at the released2.2.1image; the image reference is bumped at release time, not here.pyproject.tomlandsocket_basics/version.pyare 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(defaulttrue) 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(defaultfalse) collapses every findings section, including critical, and overridespr_comment_collapse_non_critical.GithubPRNotifierreturns early when comments are disabled (no new posts, no all-clear rewrites) but still reconciles severity labels whenpr_labels_enabledis on. OpenGrep and Socket Tier 1 PR formatters applycollapse_allwhen 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 newcoerce_bool()ingithub_pr_helpers, instead of treatingbool("false")as enabled. Wired throughaction.yml,notifications.yaml, docs, changelog, and tests.Reviewed by Cursor Bugbot for commit 8322efa. Configure here.