feat: add pr_comment_enabled and pr_comment_collapse_all PR comment controls - #97
Conversation
|
bugbot run |
This comment was marked as outdated.
This comment was marked as outdated.
|
bugbot run |
lelia
left a comment
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.
4122796 to
947b8ca
Compare
947b8ca to
a3e2663
Compare
|
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 ab96ff8. Configure here.
lelia
left a comment
There was a problem hiding this comment.
thanks for submitting this John-David Dalton (@jdalton) ! i pushed a small docs fix to update to the proper Socket Basics version ref FYI.
|
Thank you lelia ! |
* docs(changelog): draft 3.1.0 entry for the bundled release Covers #97, #98, #104, #105 and #106. Internal-only changes (#104, #106) are collapsed into a short section; customer-facing changes keep the detail needed to plan an upgrade, including the behavioral change where an unresolvable changed_files scope now fails instead of scanning. * chore(deps): refresh Socket-owned tool pins Socket npm CLI 1.1.154 -> 1.1.165 across all three images, matching the current npm release. Socket Python CLI 2.6.3 -> 2.7.0 in the heavy and app-tests images. 2.7.0 is NOT published to PyPI yet (latest is 2.6.11), so this is scaffolding: the heavy and app-tests image builds and core-tool-watch both fail until it lands. Keep this commit separate so it can be dropped or held if the CLI release slips. The socketdev Python SDK is already current at 3.5.0, so no change. * chore(release): 3.1.0 Version metadata, uv.lock, CHANGELOG date stamp, and 74 current-release documentation references, via scripts/prep_release.py --version 3.1.0. * fix(app-tests): refresh socketsecurity index metadata on install This install pins an exact version, so a stale cached uv index response makes a freshly published release look like it does not exist. Use --refresh-package for just this package rather than --no-cache, which would discard the cache mount's benefit for bandit and built wheels. Dockerfile.heavy already passes --no-cache-dir on its pip equivalent. * test: assert the socketsecurity pin, not the RUN's formatting The assertion matched an exact literal, so it broke when the install gained a --refresh-package flag and a line continuation even though the version pin it guards was unchanged. Collapse continuations and match the ARG-pinned spec after 'uv tool install' instead. Verified the guard still fails for an unpinned spec and for a hardcoded version that bypasses the ARG.
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.
Note
Medium Risk
Changes PR comment posting and shared config/CLI merging for all notifier bool flags; defaults preserve current behavior, but mis-parsed flags or suppression could surprise teams expecting comments or label behavior.
Overview
Adds
pr_comment_enabled(default on) so workflows can run scans and Socket dashboard upload without posting or updating the PR findings comment; job failure on high/critical and other notifiers still run, andpr_labels_enabledstays separate.Adds
pr_comment_collapse_allso OpenGrep SAST and Socket Tier 1<details>sections start collapsed even when critical; it overridespr_comment_collapse_non_critical. TruffleHog and Dockerfile Trivy output (flat tables) is unchanged.Introduces shared
coerce_boolfor PR comment flags and env loading so string"false"/"true", dashboard strings, and blank or unsetINPUT_*values resolve consistently (empty → documented default, not silent off).Config/CLI: notifier parameters from
notifications.yamlare applied via the same_apply_param_cli_overridespath as connectors (fixes parsed flags like--pr-comment-collapse-allnever reaching effective config); default-true bools get--no-*forms; absent CLI options leave env/JSON/dashboard values in place.action.yml,notifications.yaml, README, and PR comment docs updated; preview script and tests cover suppression, collapse scope, coercion, and CLI wiring.Reviewed by Cursor Bugbot for commit ab96ff8. Configure here.