feat: add pr_comment_enabled and pr_comment_collapse_all PR comment controls - #97
Open
John-David Dalton (jdalton) wants to merge 1 commit into
Open
feat: add pr_comment_enabled and pr_comment_collapse_all PR comment controls#97John-David Dalton (jdalton) wants to merge 1 commit into
John-David Dalton (jdalton) wants to merge 1 commit into
Conversation
Contributor
Author
|
bugbot run |
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
Author
|
bugbot run |
lelia
requested changes
Aug 5, 2026
lelia
left a comment
Contributor
There was a problem hiding this comment.
The separation between comment suppression, labels, and the scan/upload path looks good. I found two user-facing gaps that should be resolved before merge: the newly exposed CLI switches are not applied to the effective config, and the documented collapse-all contract is broader than the formatter behavior. Details are inline; the action/env/dashboard behavior and test coverage otherwise look solid.
John-David Dalton (jdalton)
force-pushed
the
feat/pr-comment-suppression
branch
from
August 7, 2026 19:44
4122796 to
947b8ca
Compare
John-David Dalton (jdalton)
force-pushed
the
feat/pr-comment-suppression
branch
from
August 7, 2026 20:04
947b8ca to
a3e2663
Compare
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.
Teams can now turn the Socket Basics pull request comment off, or collapse every findings section, without giving up scanning. Findings still reach the Socket dashboard either way. This unblocks teams who want to evaluate finding quality in the dashboard first, without a comment landing on every developer's pull request.
Before this change the comment could not be turned off at all. There was a setting to auto-collapse non-critical findings, but critical findings always stayed expanded, so a single critical finding forced the whole comment open on every push.
What you get
pr_comment_enabledtruefalseand no comment is posted or updated.pr_comment_collapse_allfalsetrueand the collapsible findings sections start collapsed.Nothing changes until a workflow opts in. Both switches default to today's behavior, so existing users see no difference when this merges.
Why turning off the comment cannot turn off the dashboard - the upload happens before any notifier runs
main()insocket_basics/socket_basics.pyruns in a fixed order: scanners execute, results are written to.socket.facts.json, the facts are uploaded to the Socket dashboard, and only then do the notifiers run.Because the comment is a notifier, it is the last step. Suppressing it cannot reach backward and suppress the upload that already happened. That ordering is what makes
pr_comment_enabled: falsesafe to recommend: you lose the comment, never the data.What collapse-all actually collapses - the SAST and Tier 1 sections, not every formatter
pr_comment_collapse_allis read by the OpenGrep and Tier 1 formatters only. The other outputs are unaffected, for two different reasons:An earlier draft of this description claimed the comment becomes a single line. That was wrong in three separate ways, and the wording in
action.yml, the docs, and the README now describes the real behavior.TestCollapseAllScopeasserts byte-identical output with the flag on and off for the formatters above, so the narrowed contract cannot drift back without a test going red.One fix that came out of review - CLI flags were parsed but never reached the config
add_dynamic_cli_args()registers CLI options fromnotifications.yaml, butcreate_config_from_args()only copied values defined inconnectors.yaml. Parsing--pr-comment-collapse-allproducedTruewhile the effective config stayedFalse.Both YAML files describe their parameters in the same shape, so one shared
_apply_param_cli_overrides()now serves both, which also replaced three inlined copies of the same YAML read.The subtle part is that every bool option now parses to
Nonewhen absent rather thanFalse. Otherwise "the user said nothing" and "the user said false" are the same value, and copying CLI values would clobber environment, JSON, and dashboard config.Flags that default to true also gained negative forms through
argparse.BooleanOptionalAction, so--no-pr-commentand friends exist. This was applied to every default-true bool parameter rather than justpr_comment_enabled, since they all shared the same defect. No connector flag defaults to true, so connector behavior is unchanged.Testing
264 tests pass. Twelve are new: six covering the parser-to-config path and the negative flag forms, and six pinning which formatters collapse-all leaves alone.
Like every change to this action, it reaches users at the next release tag.