Fix pull request comment rendering and disable flags (#322)

* Fix orphaned tags and empty tables in PR comments

A whitespace-only line closes a CommonMark HTML block. Optional sections that
rendered as empty left one behind inside the alerts table, so the indented
closing tags after it were rendered as a literal code block reading
`</blockquote></details>` instead of markup.

- Drop blank lines from generated comment markup and keep indentation below the
  four spaces that start a code block.
- Collapse alert descriptions, suggestions and license findings onto a single
  line so multi-line API text cannot break the table either.
- Replace the comment body with a short confirmation when no alerts are left to
  report, instead of keeping the caution banner above a table with no rows. The
  comment marker is preserved so the same comment is updated later.
- Apply ignore-all to the pre-2.0.55 Markdown table format. The check was made
  once per ignore command and an ignore-all comment produces none, so no rows
  were removed.

Bumps to 2.6.8.

* 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

@@ -1,6 +1,6 @@
 # Changelog
 
-## 2.6.12
+## 2.7.0
 
 ### Fixed: unreadable reachability facts no longer report a blocking package
 
@@ -18,6 +18,39 @@
   temporary directory previously used the same path and could remove each other's
   placeholder mid-upload.
 
+### Fixed: pull request comments no longer show orphaned tags or an empty table
+
+- Optional sections that rendered as empty, such as the ignore instructions
+  suppressed by `--disable-ignore`, left a whitespace-only line in the alerts
+  table. That line closed the surrounding HTML block, and the indented
+  `</blockquote></details>` tags after it were rendered as a literal code block.
+  Generated comment markup now omits blank lines and stays under the indentation
+  that starts a code block.
+- Alert descriptions, suggestions and license findings are collapsed onto a
+  single line so multi-line API text cannot break the table markup either.
+- When a pull request has no alerts left to report, the security comment is
+  replaced with a short confirmation instead of keeping the "Caution" banner
+  above a table with no rows. This happens both when a later commit resolves
+  every alert and when every alert is ignored by comment. The comment marker is
+  preserved, so a commit that reintroduces an alert updates the same comment
+  rather than posting a second one.
+- `@SocketSecurity ignore-all` now applies to comments written by CLI versions
+  before 2.0.55, which use the older Markdown alerts table. The check was made
+  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
@@ -49,7 +82,6 @@
 - Bumped the pinned reachability engine (`@coana-tech/cli`) from `15.10.23` to
   `15.10.25`. See the [Coana changelogs](https://docs.coana.tech/changelogs) for
   engine changes.
-
 ## 2.6.7
 
 ### Changed: bump pinned @coana-tech/cli to 15.10.23

pyproject.toml

@@ -6,7 +6,7 @@ build-backend = "hatchling.build"
 
 [project]
 name = "socketsecurity"
-version = "2.6.12"
+version = "2.7.0"
 requires-python = ">= 3.11"
 license = {"file" = "LICENSE"}
 dependencies = [

socketsecurity/__init__.py

@@ -1,3 +1,3 @@
 __author__ = 'socket.dev'
-__version__ = '2.6.12'
+__version__ = '2.7.0'
 USER_AGENT = f'SocketPythonCLI/{__version__}'

socketsecurity/core/messages.py

@@ -806,6 +806,90 @@ def create_security_comment_gitlab(diff: Diff) -> dict:
 
         return gitlab_report
 
+    # A blank line terminates a CommonMark HTML block. When that happens inside
+    # the alerts table the closing tags that follow are no longer treated as
+    # markup, and because they are indented four or more spaces they render as a
+    # literal code block containing `</blockquote></details>` instead.
+    MAX_HTML_INDENT = 3
+
+    @staticmethod
+    def inline_html_text(value) -> str:
+        """
+        Collapses API supplied text onto a single line.
+
+        Alert descriptions and suggestions are interpolated into the comment HTML,
+        so an embedded newline would otherwise be able to close the surrounding
+        HTML block early.
+
+        :param value: The value to flatten. ``None`` becomes an empty string.
+        :return: str - The value with all whitespace runs collapsed to a single space.
+        """
+        if value is None:
+            return ""
+        return " ".join(str(value).split())
+
+    @staticmethod
+    def normalize_comment_html(comment: str) -> str:
+        """
+        Makes generated comment markup safe for the CommonMark renderers used by
+        GitHub and GitLab.
+
+        Drops whitespace-only lines (an optional section that rendered as empty
+        leaves one behind) and caps indentation below the four spaces that would
+        start an indented code block. Intentional separators - lines that are
+        genuinely empty - are preserved so markdown blocks still break apart.
+
+        :param comment: str - The generated comment body.
+        :return: str - The comment body with unrenderable whitespace removed.
+        """
+        lines = []
+        for line in comment.split("\n"):
+            if line and not line.strip():
+                continue
+            stripped = line.lstrip()
+            indent = min(len(line) - len(stripped), Messages.MAX_HTML_INDENT)
+            lines.append(" " * indent + stripped)
+        return "\n".join(lines)
+
+    @staticmethod
+    def security_comment_no_alerts_template(view_report_url: str = "") -> str:
+        """
+        Generates the body used when there is nothing left to report.
+
+        Alerts raised on an early commit are frequently resolved later in the same
+        pull request. Rewriting the comment to this body keeps the Socket comment
+        in place - so a later commit that reintroduces an alert updates it rather
+        than posting a second comment - without leaving the "Caution" banner above
+        an empty alerts table.
+
+        :param view_report_url: str - Optional link to the full Socket report.
+        :return: str - The formatted Markdown/HTML string.
+        """
+        lines = [
+            "<!-- socket-security-comment-actions -->",
+            "",
+            "> **✅ Socket Security**  ",
+            "> No dependency alerts to report. Any alerts previously reported on this "
+            "pull request have been resolved or ignored.",
+        ]
+        if view_report_url:
+            lines += ["", f"[View full report]({view_report_url})"]
+        return "\n".join(lines) + "\n"
+
+    @staticmethod
+    def get_view_report_url(diff: Diff) -> str:
+        """
+        Resolves the report link for a diff, preferring the PR/MR diff view.
+
+        :param diff: Diff - Diff report to pull the URL from.
+        :return: str - The report URL, or an empty string when neither is set.
+        """
+        if getattr(diff, "diff_url", None):
+            return diff.diff_url
+        if getattr(diff, "report_url", None):
+            return diff.report_url
+        return ""
+
     @staticmethod
     def security_comment_template(diff: Diff, config=None) -> str:
         """
@@ -819,7 +903,7 @@ def security_comment_template(diff: Diff, config=None) -> str:
         # Group license policy violations by PURL (ecosystem/package@version)
         license_groups = {}
         security_alerts = []
-        
+
         for alert in diff.new_alerts:
             if alert.type == "licenseSpdxDisj":
                 purl_key = f"{alert.pkg_type}/{alert.pkg_name}@{alert.pkg_version}"
@@ -829,6 +913,13 @@ def security_comment_template(diff: Diff, config=None) -> str:
             else:
                 security_alerts.append(alert)
 
+        view_report_url = Messages.get_view_report_url(diff)
+
+        # Without this the caution banner would sit above a table with no rows,
+        # which is how a comment looks once every alert it raised is resolved.
+        if not security_alerts and not license_groups:
+            return Messages.security_comment_no_alerts_template(view_report_url)
+
         # Start of the comment
         comment = """<!-- socket-security-comment-actions -->
 
@@ -875,15 +966,15 @@ def security_comment_template(diff: Diff, config=None) -> str:
   </td>
   <td>
     <details {details_open}>
-      <summary>{alert.pkg_name}@{alert.pkg_version} - {alert.title}</summary>
-      <p><strong>Note:</strong> {alert.description}</p>
+      <summary>{alert.pkg_name}@{alert.pkg_version} - {Messages.inline_html_text(alert.title)}</summary>
+      <p><strong>Note:</strong> {Messages.inline_html_text(alert.description)}</p>
       <p><strong>Source:</strong> <a href="{manifest_url}">Manifest File</a></p>
       <p>ℹ️ Read more on:
       <a href="{alert.purl}">This package</a> |
       <a href="{alert.url}">This alert</a> |
       <a href="https://socket.dev/alerts/malware">What is known malware?</a></p>
       <blockquote>
-        <p><em>Suggestion:</em> {alert.suggestion}</p>
+        <p><em>Suggestion:</em> {Messages.inline_html_text(alert.suggestion)}</p>
         {ignore_html}
       </blockquote>
     </details>
@@ -917,7 +1008,7 @@ def security_comment_template(diff: Diff, config=None) -> str:
       <ul>
 """
             for finding in license_findings:
-                comment += f"        <li>{finding}</li>\n"
+                comment += f"        <li>{Messages.inline_html_text(finding)}</li>\n"
 
 
             # Generate proper manifest URL for license violations
@@ -944,13 +1035,6 @@ def security_comment_template(diff: Diff, config=None) -> str:
     """
 
         # Close table
-        # Use diff_url for PRs, report_url for non-PR scans
-        view_report_url = ""
-        if hasattr(diff, 'diff_url') and diff.diff_url:
-            view_report_url = diff.diff_url
-        elif hasattr(diff, 'report_url') and diff.report_url:
-            view_report_url = diff.report_url
-            
         comment += f"""
   </tbody>
 </table>
@@ -959,7 +1043,7 @@ def security_comment_template(diff: Diff, config=None) -> str:
 [View full report]({view_report_url}?action=error%2Cwarn)
     """
 
-        return comment
+        return Messages.normalize_comment_html(comment)
 
     @staticmethod
     def get_severity_icon(severity: str) -> str:

socketsecurity/core/scm_comments.py

@@ -1,12 +1,16 @@
 import json
+import re
 
 from requests import Response
 
 from socketsecurity.core import log
 from socketsecurity.core.classes import Comment, Issue
+from socketsecurity.core.messages import Messages
 
 
 class Comments:
+    VIEW_REPORT_PATTERN = re.compile(r"\[View full report\]\(([^)\s]+)\)")
+
     @staticmethod
     def process_response(response: Response) -> dict:
         output = {}
@@ -84,6 +88,20 @@ def is_heading_line(line) -> bool:
             is_heading_line = False
         return is_heading_line
 
+    @staticmethod
+    def extract_report_url(body: str) -> str:
+        """
+        Pulls the Socket report link out of an existing comment body so it can be
+        carried over when the comment is rewritten.
+
+        :param body: str - The existing comment body.
+        :return: str - The report URL without its query string, or "" if absent.
+        """
+        match = Comments.VIEW_REPORT_PATTERN.search(body)
+        if not match:
+            return ""
+        return match.group(1).split("?", 1)[0]
+
     @staticmethod
     def process_security_comment(comment: Comment, comments) -> str:
         ignore_all, ignore_commands = Comments.get_ignore_options(comments)
@@ -102,6 +120,7 @@ def process_original_security_comment(
     ) -> str:
         start = False
         lines = []
+        kept_alert = False
         for line in comment.body_list:
             line = line.strip()
             if "start-socket-alerts-table" in line:
@@ -114,17 +133,25 @@ def process_original_security_comment(
                 ecosystem = ecosystem.lstrip("[")
                 pkg_name, pkg_version = details.split("@")
                 pkg_name = f"{ecosystem}/{pkg_name}"
-                ignore = False
-                for name, version in ignore_commands:
-                    if ignore_all or Comments.is_ignore(pkg_name, pkg_version, name, version):
-                        ignore = True
+                # ignore_all has to be checked outside the loop: an ignore-all
+                # comment produces no ignore_commands, so a loop-internal check
+                # never runs and every row was kept.
+                ignore = ignore_all or any(
+                    Comments.is_ignore(pkg_name, pkg_version, name, version)
+                    for name, version in ignore_commands
+                )
                 if not ignore:
+                    kept_alert = True
                     lines.append(line)
             elif "end-socket-alerts-table" in line:
                 start = False
                 lines.append(line)
             else:
                 lines.append(line)
+        if not kept_alert:
+            return Messages.security_comment_no_alerts_template(
+                Comments.extract_report_url("\n".join(comment.body_list))
+            )
         return "\n".join(lines)
 
     @staticmethod
@@ -145,17 +172,21 @@ def process_updated_security_comment(
         """
         lines = []
         ignore_section = False
+        kept_alert = False  # Whether any alert row survived the ignore commands
         pkg_name = pkg_version = ""  # Track current package and version
 
         # Loop through the comment lines
         for line in comment.body_list:
-            line = line.strip()
+            # Match on the stripped line but keep the original, so the markup is
+            # rewritten with the same indentation it was generated with.
+            line = line.rstrip("\r")
+            stripped = line.strip()
 
             # Detect the start of an alert section
-            if line.startswith("<!-- start-socket-alert-"):
+            if stripped.startswith("<!-- start-socket-alert-"):
                 # Extract package name and version from the comment
                 try:
-                    start_marker = line[len("<!-- start-socket-alert-"):-4]  # Strip the comment markers
+                    start_marker = stripped[len("<!-- start-socket-alert-"):-4]  # Strip the comment markers
                     pkg_name, pkg_version = start_marker.split("@")  # Extract pkg_name and pkg_version
                 except ValueError:
                     pkg_name, pkg_version = "", ""
@@ -168,10 +199,11 @@ def process_updated_security_comment(
 
                 # If not ignored, include this start marker
                 if not ignore_section:
+                    kept_alert = True
                     lines.append(line)
 
             # Detect the end of an alert section
-            elif line.startswith("<!-- end-socket-alert-"):
+            elif stripped.startswith("<!-- end-socket-alert-"):
                 # Only include if we are not ignoring this section
                 if not ignore_section:
                     lines.append(line)
@@ -181,7 +213,14 @@ def process_updated_security_comment(
             elif not ignore_section:
                 lines.append(line)
 
-        return "\n".join(lines)
+        # Every row was ignored, so drop the table rather than leaving the caution
+        # banner sitting above an empty one.
+        if not kept_alert:
+            return Messages.security_comment_no_alerts_template(
+                Comments.extract_report_url("\n".join(comment.body_list))
+            )
+
+        return Messages.normalize_comment_html("\n".join(lines))
 
     @staticmethod
     def extract_alert_details_from_row(row: str, ignore_all: bool, ignore_commands: list[tuple[str, str]]) -> tuple: