Make --disable-security-issue and --disable-overview suppress comments

Both flags were checked only after testing whether a comment of that type was
already on the pull request, so they suppressed the first post and then updated
that comment on every later run. --disable-security-issue in particular kept
refreshing an existing comment with the full alerts table.

The flags now mean the CLI does not manage that comment at all. An existing
comment is left untouched rather than rewritten, since a body claiming no alerts
would be inaccurate when reporting is merely switched off.

Moves the decision into should_write_comment() so it is covered by tests
directly; main_code() had no harness for this block.

Bumps to 2.7.0 rather than a patch, since these flags change behavior.

Author: lelia

CHANGELOG.md

@@ -39,6 +39,18 @@
   once per ignore command, and an ignore-all comment produces none, so no rows
   were removed.
 
+### Fixed: `--disable-security-issue` and `--disable-overview` now suppress the comment entirely
+
+- Both flags were checked only after testing whether a comment of that type was
+  already on the pull request, so they suppressed the first post and then
+  updated that comment on every later run. `--disable-security-issue` in
+  particular kept refreshing an existing comment with the full alerts table.
+- The flags now mean the CLI does not manage that comment at all. An existing
+  comment is left untouched rather than being rewritten, since a body claiming
+  no alerts would be inaccurate when reporting is merely switched off.
+- The decision moved into `should_write_comment()` so it is covered directly by
+  tests.
+
 ## 2.6.11
 
 ### Changed: bump pinned @coana-tech/cli to 15.10.32

socketsecurity/socketcli.py

@@ -107,6 +107,27 @@ def get_api_request_timeout(config: CliConfig) -> int:
     return config.timeout if config.timeout is not None else DEFAULT_API_TIMEOUT
 
 
+def should_write_comment(disabled: bool, has_findings: bool, update_existing: bool) -> bool:
+    """
+    Decides whether a Socket comment should be written for this run.
+
+    :param disabled: bool - The comment type is switched off by flag.
+    :param has_findings: bool - There is something to report this run.
+    :param update_existing: bool - A comment of this type is already on the PR.
+    :return: bool - True when the comment should be posted or updated.
+    """
+    if disabled:
+        # The flag means the CLI does not manage this comment at all. Checking
+        # update_existing first only suppressed the very first post, so any
+        # comment already on the pull request kept being updated.
+        return False
+    if not has_findings:
+        # Nothing to report: refresh an existing comment so a resolved alerts
+        # table is cleared, but do not open a new one.
+        return update_existing
+    return True
+
+
 def build_socket_sdk(config: CliConfig) -> socketdev:
     cli_user_agent_string = f"SocketPythonCLI/{config.version}"
     return socketdev(
@@ -780,9 +801,6 @@ def _is_unprocessed(c):
 
                 security_comment = Messages.security_comment_template(diff, config)
 
-                new_security_comment = True
-                new_overview_comment = True
-                
                 update_old_security_comment = (
                     security_comment is None or
                     security_comment == "" or
@@ -795,20 +813,22 @@ def _is_unprocessed(c):
                     (len(comments) != 0 and comments.get("overview") is not None)
                 )
 
-                if len(diff.new_alerts) == 0 or config.disable_security_issue:
-                    if not update_old_security_comment:
-                        new_security_comment = False
-                        log.debug("No new alerts or security issue comment disabled")
-                    else:
-                        log.debug("Updated security comment with no new alerts")
-                
+                new_security_comment = should_write_comment(
+                    config.disable_security_issue,
+                    len(diff.new_alerts) > 0,
+                    update_old_security_comment,
+                )
+                if not new_security_comment:
+                    log.debug("Security issue comment disabled, or no alerts and none to update")
+
                 # FIXME: diff.new_packages is never populated, neither is removed_packages
-                if (len(diff.new_packages) == 0) or config.disable_overview:
-                    if not update_old_overview_comment:
-                        new_overview_comment = False
-                        log.debug("No new/removed packages or Dependency Overview comment disabled")
-                    else:
-                        log.debug("Updated overview comment with no dependencies")
+                new_overview_comment = should_write_comment(
+                    config.disable_overview,
+                    len(diff.new_packages) > 0,
+                    update_old_overview_comment,
+                )
+                if not new_overview_comment:
+                    log.debug("Dependency Overview comment disabled, or no packages and none to update")
 
                 log.debug(f"Adding comments for {config.scm}")
                 scm.add_socket_comments(

tests/unit/test_socketcli.py

@@ -4,7 +4,7 @@
 
 from socketsecurity.core.classes import Diff, Package
 from socketsecurity import socketcli
-from socketsecurity.socketcli import build_license_artifact_payload
+from socketsecurity.socketcli import build_license_artifact_payload, should_write_comment
 
 
 # ---------------------------------------------------------------------------
@@ -228,3 +228,46 @@ class Config:
     assert payload["deepDependencies"] == []
     assert payload["copyrightsByLicense"] == {}
     assert payload["licenses"] == {}
+
+
+# ---------------------------------------------------------------------------
+# Comment write decision.
+#
+# --disable-security-issue used to be checked only after the "is there already
+# a comment" test, so it suppressed the first post on a pull request and then
+# updated that comment with the full alerts table on every later run.
+# ---------------------------------------------------------------------------
+
+
+class TestShouldWriteComment:
+    def test_disabled_never_writes_even_when_a_comment_exists(self):
+        assert should_write_comment(
+            disabled=True, has_findings=True, update_existing=True
+        ) is False
+
+    def test_disabled_never_writes_with_no_existing_comment(self):
+        assert should_write_comment(
+            disabled=True, has_findings=True, update_existing=False
+        ) is False
+
+    def test_disabled_wins_over_findings(self):
+        """The flag is not conditional on there being nothing to report."""
+        assert should_write_comment(
+            disabled=True, has_findings=False, update_existing=True
+        ) is False
+
+    def test_findings_are_written(self):
+        assert should_write_comment(
+            disabled=False, has_findings=True, update_existing=False
+        ) is True
+
+    def test_no_findings_refreshes_an_existing_comment(self):
+        """So a resolved alerts table gets cleared rather than left stale."""
+        assert should_write_comment(
+            disabled=False, has_findings=False, update_existing=True
+        ) is True
+
+    def test_no_findings_does_not_open_a_new_comment(self):
+        assert should_write_comment(
+            disabled=False, has_findings=False, update_existing=False
+        ) is False