feat: add pr_comment_enabled and pr_comment_collapse_all PR comment controls

[John-David Dalton (jdalton)]

LLM Description written by CLAUDE_CODE:claude-opus-5

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.

Nothing changes for anyone until a workflow opts in. Both switches default to today's behavior, so existing users see no difference when this merges. The comment disappears only for a workflow that sets pr_comment_enabled: 'false', and findings still reach the Socket dashboard either way. Like every change to this action, it reaches users at the next release tag — and the requesting team also needs to re-mirror onto that tag, because their current pin predates these inputs.

Design & behavior

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.

Diagnosis

Getting a flag into the config had the same string-vs-boolean bug, pointing the wrong way

coerce_bool() handles a flag once it is in the config. Getting it there went
through the environment loader, which read every notifier boolean as
env_value.lower() == 'true' — so anything that was not the literal word true
came out False, including an empty string.

Action inputs are always strings, and action.yml passes
INPUT_PR_COMMENT_ENABLED: ${{ inputs.pr_comment_enabled }} on every run. A
workflow that forwards a variable which turns out to be unset hands the
container '', which read as "off" — so the comment vanished from a workflow
that never asked to suppress it. pr_labels_enabled had it too, and '1',
'yes' and 'on' did not turn a flag on even though coerce_bool accepts all
three one layer up.

Input value	Before	After
unset	on	on
'true' / 'TRUE'	on	on
'false'	off	off
'0' / 'no' / 'off'	on	off
'1' / 'yes' / 'on'	off	on
'' (forwarded unset variable)	off	on
' '	off	on
'maybe'	off	on

coerce_bool now lives in the config layer and both paths use it, so a value
that says nothing falls back to the flag's declared default rather than to off,
and every spelling behaves the same from an action input, a JSON config and a
Socket dashboard config. pr_comment_collapse_all is unaffected in the blank
case because its default is already off; only the flags that default on were
being flipped.

11 tests in TestActionInputCoercion drive the real loader over every spelling
above. Reverting the loader to the bare == 'true' comparison fails 7 of them.

Code, testing & review

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, and how a boolean option reads a string
README.md	Added both switches to the PR comment feature list
CHANGELOG.md	Entries under [Unreleased]

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

Command	Result
uv run --no-sync pytest -q tests/	exit 0, 252 passed (was 216 on main; 36 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, 4 in tests/test_pr_formatters.py::TestFeatureFlagCoercion, and 11 in tests/test_pr_formatters.py::TestActionInputCoercion driving the real environment loader.

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

Medium Risk
Changes default interpretation of env/dashboard boolean strings for existing PR comment and label flags, which could flip behavior for misconfigured workflows; comment suppression is scoped to the notifier after scan/upload.

Overview
Adds pr_comment_enabled (default on) so scans can run without posting or updating the PR comment, while dashboard upload, job failure on high/critical, and other notifiers stay the same. pr_labels_enabled remains separate so labels can still be reconciled when comments are off.

Adds pr_comment_collapse_all so OpenGrep and Socket Tier 1 formatters can keep every <details> section collapsed, including critical findings, overriding the existing non-critical collapse behavior.

Introduces coerce_bool() in the config layer and uses it for notifier env loading (notifications.yaml booleans), PR feature flags in get_feature_flags(), and GithubPRNotifier (pr_comment_enabled, pr_labels_enabled). Blank or unrecognized strings fall back to documented defaults instead of being treated as off; string forms like 'false', '0', 'yes', and 'on' parse consistently across Action inputs, env, JSON, and dashboard config.

Wires new inputs through action.yml, notifications.yaml, docs, changelog, preview script, and expanded unit tests.

^{Reviewed by Cursor Bugbot for commit afba61a. Configure here.}

[John-David Dalton (jdalton)]

bugbot run

[cursor]

✅ 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.}

[John-David Dalton (jdalton)]

[agent] One more string-vs-boolean hole, in the layer underneath the one this PR already fixed, and it pointed the wrong way for exactly the switch this PR adds.

coerce_bool() handles a flag once it is in the config. Getting it into the config went through the environment loader, which read every notifier boolean as env_value.lower() == 'true'. Anything that is not the literal word true came out False, including an empty string.

Action inputs are always strings, and action.yml passes INPUT_PR_COMMENT_ENABLED: ${{ inputs.pr_comment_enabled }} on every run. A workflow that forwards a variable which turns out to be unset — pr_comment_enabled: ${{ vars.PR_COMMENTS }}, or a reusable workflow passing an input through — hands the container an empty string. So the comment vanished from a workflow that never asked to suppress it, and nothing said why. Same loader, so pr_labels_enabled had it too. '1', 'yes' and 'on' did not turn a flag on either, even though coerce_bool accepts all three one layer up.

I probed the real loader before and after, which is the difference in one table:

Input value	Before	After
unset	on	on
'true' / 'TRUE'	on	on
'false'	off	off
'0' / 'no' / 'off'	on	off
'1' / 'yes' / 'on'	off	on
'' (forwarded unset variable)	off	on
' '	off	on
'maybe'	off	on

Fixed in afba61a. coerce_bool moved to the config layer so the environment loader, a --config JSON file and a Socket dashboard config all read a value the same way, and a value that says nothing falls back to the flag's declared default rather than to off. github_pr_helpers imports it from there, so there is one truth table rather than two.

That fallback is why pr_comment_collapse_all is unaffected by the change in the blank case: its default is false, so blank stays off. Only the flags that default on were being flipped.

11 new tests in TestActionInputCoercion drive the real loader over every spelling in that table plus the blank forms. Reverting the loader to the bare == 'true' comparison fails 7 of them. Suite: 252 passed.

The other three things you asked me to confirm all hold:

Suppressing the comment does not suppress the dashboard. main() uploads at step 3 and runs notifiers last, the switch lives inside GithubPRNotifier.notify(), and no notifier touches the upload path — grep for submit_socket_facts across core/notification/ finds only the comment explaining that. test_notify_leaves_facts_untouched_when_pr_comment_disabled pins it.

pr_comment_collapse_all does reach critical sections, in both formatters that compute expansion, and the Trivy formatter needed no change because it never expanded anything.

Defaults are right in action.yml, notifications.yaml and the guide: comment on, collapse-all off, so an existing workflow sees no change. I added both switches to the README feature list and documented the boolean truth table in the PR comment guide, including that a blank value means "use the default".

[John-David Dalton (jdalton)]

bugbot run

[cursor]

✅ 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 afba61a. Configure here.}

[lelia]

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.

[lelia]

Could we wire notification options through create_config_from_args() before exposing this as a CLI flag? add_dynamic_cli_args() registers options from notifications.yaml, but create_config_from_args() only copies values defined in connectors.yaml. I verified that parsing --pr-comment-collapse-all produces True while the effective pr_comment_collapse_all config remains False. The new --pr-comment option also cannot disable a default-true flag. Please apply notifier CLI arguments, expose a negative/default-true form such as --no-pr-comment (or BooleanOptionalAction), and add an end-to-end parser/config test.

[lelia]

Can we either implement this behavior for the flat-table formatters or narrow the contract? Only OpenGrep and Tier 1 consume pr_comment_collapse_all. TruffleHog and Trivy Dockerfile findings remain expanded tables, and even SAST leaves one visible collapsed row per file, so the guide's claim that the comment becomes one line is not accurate. Please add formatter coverage for the remaining outputs, or describe this specifically as collapsing the currently collapsible SAST/Tier 1 sections and adjust the one-line wording.