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 1 commit 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 1 commit into
mainfrom
feat/pr-comment-suppression

Conversation

@jdalton

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

Copy link
Copy Markdown
Contributor

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

Input Default What it does
pr_comment_enabled true Set to false and no comment is posted or updated.
pr_comment_collapse_all false Set to true and 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() in socket_basics/socket_basics.py runs 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: false safe 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_all is read by the OpenGrep and Tier 1 formatters only. The other outputs are unaffected, for two different reasons:

Output Effect Why
OpenGrep (SAST) Collapses Reads the flag. One summary row per file stays visible.
Socket Tier 1 Collapses Reads the flag.
TruffleHog No change Renders a flat table with nothing to collapse.
Trivy Dockerfile No change Renders a flat table with nothing to collapse.
Trivy image and CVE No change Already always collapsed, and never reads the flag.

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. TestCollapseAllScope asserts 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 from notifications.yaml, but create_config_from_args() only copied values defined in connectors.yaml. Parsing --pr-comment-collapse-all produced True while the effective config stayed False.

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 None when absent rather than False. 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-comment and friends exist. This was applied to every default-true bool parameter rather than just pr_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.

@jdalton
John-David Dalton (jdalton) requested a review from a team as a code owner August 3, 2026 18:29
cursor[bot]

This comment was marked as resolved.

@jdalton

Copy link
Copy Markdown
Contributor Author

bugbot run

cursor[bot]

This comment was marked as resolved.

@jdalton

This comment was marked as outdated.

@jdalton

Copy link
Copy Markdown
Contributor Author

bugbot run

cursor[bot]

This comment was marked as resolved.

@lelia lelia self-assigned this Aug 5, 2026

@lelia lelia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread socket_basics/notifications.yaml
Comment thread action.yml Outdated
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.

2 participants