fix(ci): keep branch pipelines out of pull request handling

Two ways an SCM branch build could still be treated like a pull request:

Buildkite always sets BUILDKITE_PULL_REQUEST, to the string "false" on a branch
build, so the documented --pr-number "$BUILDKITE_PULL_REQUEST" form delivers a
truthy non-numeric value. resolve_pull_request_context read it as no PR but only
wrote the normalized number back when one was found, so GithubConfig still saw
"false", check_event_type returned "diff" for a push, and comment lookups went to
issues/false/comments. Canonicalize config.pr_number before any adapter reads it.

A branch run creating a full scan then blocked on diff.new_alerts, which a full
scan cannot fill meaningfully: empty with no alert-bearing output format enabled,
and every alert in the scan rather than the newly introduced ones with one. The
exit code therefore depended on which output format was requested. Treat these
runs the way a run with no supported manifest files is already treated and skip
blocking, leaving pull request pipelines to enforce policy.

Move the scan-type decision into create_scm_scan, which returns the diff and
whether it came from a comparison, so the branch is exercised by tests rather
than only its predicate. Document both the scan-type table and the blocking
consequence in the CI/CD guide.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: lelia

docs/ci-cd.md

@@ -455,12 +455,36 @@ pipelines:
           - socketcli --config .socketcli.toml --target-path .
 ```
 
+## Scan type by pipeline
+
+With `--scm github` or `--scm gitlab`, the detected event decides the scan type:
+
+| Event | Scan | Blocks the build |
+|:------|:-----|:-----------------|
+| Pull request / merge request | Diff scan against the repository's baseline | Yes, on newly introduced alerts |
+| Any other pipeline, including default-branch pushes | Full scan | No |
+
+A full scan has no baseline, so it cannot tell a newly introduced alert from one
+that was already there. Rather than block on a number that would mean something
+different depending on which output format was enabled, those runs behave as if
+`--disable-blocking` was supplied and report through the Dashboard instead. This
+matches how the CLI already treats a run with no supported manifest files.
+
+The event type is authoritative once `--scm` is set: `--enable-diff` and
+`--ignore-commit-files` do not turn a branch pipeline into a comparison. To diff
+a branch build, drop `--scm` and use `--enable-diff` with `--integration`, which
+runs the comparison without the PR comment adapter.
+
+`--generate-license` and `--legal-format fossa` work on both paths; a full scan
+fetches the package list for them.
+
 ## Pull request and Dashboard association
 
 The CLI sends the resolved pull request number with each full scan and attaches
 the pull request URL to diff scans so the Socket Dashboard can associate the
 report with its originating change. If `--pr-number` is supplied, it wins;
-passing `--pr-number 0` explicitly disables automatic association.
+passing `--pr-number 0` explicitly disables automatic association. Any value that
+is not a positive integer, including Buildkite's `false`, means no pull request.
 
 Without an explicit value, the CLI recognizes:
 

socketsecurity/core/pull_request.py

@@ -12,7 +12,14 @@ class PullRequestContext:
     url: Optional[str] = None
 
 
-def _positive_int(value) -> int:
+def parse_pull_request_number(value) -> int:
+    """Coerce a configured or CI-supplied pull request number to a positive int.
+
+    Anything that is not a positive integer means "no pull request", including the
+    literal ``false`` that Buildkite puts in ``BUILDKITE_PULL_REQUEST`` on non-PR
+    builds. Callers that hand the value on to a comment adapter should store this
+    result rather than the raw string, which is truthy.
+    """
     try:
         parsed = int(value)
     except (TypeError, ValueError):
@@ -31,11 +38,11 @@ def _repository_url(value: Optional[str]) -> Optional[str]:
 
 
 def _github_number(env: Mapping[str, str]) -> int:
-    number = _positive_int(env.get("PR_NUMBER"))
+    number = parse_pull_request_number(env.get("PR_NUMBER"))
     if number:
         return number
     match = re.match(r"^refs/pull/(\d+)/", env.get("GITHUB_REF", ""))
-    return _positive_int(match.group(1)) if match else 0
+    return parse_pull_request_number(match.group(1)) if match else 0
 
 
 def _github_url(number: int, repo: Optional[str], env: Mapping[str, str]) -> Optional[str]:
@@ -89,17 +96,17 @@ def resolve_pull_request_context(
     """
     environment = env or {}
     provider = str(integration_type or "api").lower()
-    number = _positive_int(configured_number)
+    number = parse_pull_request_number(configured_number)
 
     if not configured_explicit and not number:
         if provider == "github":
             number = _github_number(environment)
         elif provider == "gitlab":
-            number = _positive_int(environment.get("CI_MERGE_REQUEST_IID"))
+            number = parse_pull_request_number(environment.get("CI_MERGE_REQUEST_IID"))
         elif provider == "azure":
             number = (
-                _positive_int(environment.get("SYSTEM_PULLREQUEST_PULLREQUESTNUMBER")) or
-                _positive_int(environment.get("SYSTEM_PULLREQUEST_PULLREQUESTID"))
+                parse_pull_request_number(environment.get("SYSTEM_PULLREQUEST_PULLREQUESTNUMBER")) or
+                parse_pull_request_number(environment.get("SYSTEM_PULLREQUEST_PULLREQUESTID"))
             )
 
     if not number:

socketsecurity/socketcli.py

@@ -4,6 +4,7 @@
 import sys
 import traceback
 from datetime import datetime, timezone
+from typing import List, Optional, Tuple
 from uuid import uuid4
 
 from dotenv import load_dotenv
@@ -18,7 +19,10 @@
 from socketsecurity.core.git_interface import Git
 from socketsecurity.core.logging import initialize_logging, set_debug_mode
 from socketsecurity.core.messages import Messages
-from socketsecurity.core.pull_request import resolve_pull_request_context
+from socketsecurity.core.pull_request import (
+    parse_pull_request_number,
+    resolve_pull_request_context,
+)
 from socketsecurity.core.scm_comments import Comments
 from socketsecurity.core.socket_config import SocketConfig, module_folder_dirs
 from socketsecurity.core.streaming import StreamingLogs
@@ -139,8 +143,47 @@ def _select_pull_request_provider(integration_type: str, scm_type: str) -> str:
     return scm_type if scm_type in ("github", "gitlab") else integration_type
 
 
-def _should_create_scm_diff(event_type: str) -> bool:
-    return event_type == "diff"
+def create_scm_scan(
+    core: Core,
+    config: CliConfig,
+    scm_event_type: Optional[str],
+    *,
+    scan_paths: List[str],
+    params: FullScanParams,
+    no_change: bool,
+    base_paths: Optional[List[str]],
+    explicit_files: Optional[List[str]],
+    external_href: Optional[str],
+) -> Tuple[Diff, bool]:
+    """Create the scan for an SCM-integrated run.
+
+    Only a pull request or merge request event has a baseline to compare against,
+    so every other pipeline -- default-branch pushes included -- gets a full scan.
+    The detected event type is authoritative: API-only diff flags cannot turn an
+    ordinary branch pipeline into a comparison.
+
+    Returns the diff and whether it came from a comparison. Callers need the second
+    value because a full scan carries no "new alerts" category to comment on or to
+    block a build with.
+    """
+    scan_kwargs = {
+        "no_change": no_change,
+        "save_files_list_path": config.save_submitted_files_list,
+        "save_manifest_tar_path": config.save_manifest_tar,
+        "base_paths": base_paths,
+        "explicit_files": explicit_files,
+    }
+    if scm_event_type == "diff":
+        log.info("Starting comment logic for PR/MR event")
+        diff = core.create_new_diff(
+            scan_paths, params, external_href=external_href, **scan_kwargs
+        )
+        return diff, True
+
+    log.info("Starting non-PR/MR flow")
+    # No before/after pair here, so there is nothing for external_href to hang off.
+    diff = core.create_full_scan_with_report_url(scan_paths, params, **scan_kwargs)
+    return diff, False
 
 
 def build_socket_sdk(config: CliConfig) -> socketdev:
@@ -507,6 +550,13 @@ def main_code():
 
             log.info("Continuing with normal scan flow...")
 
+        # Canonicalize before any adapter reads it. Buildkite always sets
+        # BUILDKITE_PULL_REQUEST -- to the string "false" on non-PR builds -- so the
+        # documented --pr-number "$BUILDKITE_PULL_REQUEST" form delivers a truthy
+        # non-numeric value that GithubConfig would otherwise treat as a real PR,
+        # making a branch build look like a pull request event.
+        config.pr_number = str(parse_pull_request_number(config.pr_number))
+
         scm = None
         if config.scm == "github":
             from socketsecurity.core.scm.github import Github, GithubConfig
@@ -689,6 +739,10 @@ def _is_unprocessed(c):
             return True
 
         scm_event_type = scm.check_event_type() if scm is not None else None
+        # Every branch below except the SCM full-scan one produces a comparison, or
+        # is already covered by force_api_mode. See the blocking guard after the
+        # scan for why this is tracked.
+        comparison_ran = True
         if scm_event_type == "comment":
             # FIXME: This entire flow should be a separate command called "filter_ignored_alerts_in_comments"
             # It's not related to scanning or diff generation - it just:
@@ -745,18 +799,18 @@ def _is_unprocessed(c):
 
         elif scm is not None and not force_api_mode:
             log.info("Push initiated flow")
-            if _should_create_scm_diff(scm_event_type):
-                log.info("Starting comment logic for PR/MR event")
-                diff = core.create_new_diff(
-                    scan_paths,
-                    params,
-                    no_change=should_skip_scan,
-                    save_files_list_path=config.save_submitted_files_list,
-                    save_manifest_tar_path=config.save_manifest_tar,
-                    base_paths=base_paths,
-                    explicit_files=scan_explicit_files,
-                    external_href=pr_context.url,
-                )
+            diff, comparison_ran = create_scm_scan(
+                core,
+                config,
+                scm_event_type,
+                scan_paths=scan_paths,
+                params=params,
+                no_change=should_skip_scan,
+                base_paths=base_paths,
+                explicit_files=scan_explicit_files,
+                external_href=pr_context.url,
+            )
+            if comparison_ran:
                 comments = scm.get_comments_for_pr()
 
                 # FIXME: this overwrites diff.new_alerts, which was previously populated by Core.create_issue_alerts
@@ -881,17 +935,6 @@ def _is_unprocessed(c):
                     new_security_comment,
                     new_overview_comment
                 )
-            else:
-                log.info("Starting non-PR/MR flow")
-                diff = core.create_full_scan_with_report_url(
-                    scan_paths,
-                    params,
-                    no_change=should_skip_scan,
-                    save_files_list_path=config.save_submitted_files_list,
-                    save_manifest_tar_path=config.save_manifest_tar,
-                    base_paths=base_paths,
-                    explicit_files=scan_explicit_files,
-                )
 
             output_handler.handle_output(diff)
 
@@ -991,13 +1034,20 @@ def _is_unprocessed(c):
             )
             _write_attribution_file(config, all_packages)
 
-        # If we forced API mode due to no supported files, behave as if --disable-blocking was set
-        if force_api_mode:
+        # A run that created a full scan instead of a comparison has no baseline, so
+        # diff.new_alerts is not a meaningful thing to block on: with no alert-bearing
+        # output format enabled it is empty, and with --enable-json/--sarif/
+        # --enable-gitlab-security it holds every alert in the scan rather than the
+        # newly introduced ones. Blocking on it would make the exit code depend on
+        # which output format happened to be requested, so behave as if
+        # --disable-blocking was set. force_api_mode arrives here for the same reason
+        # (no supported manifest files, so nothing to compare).
+        if force_api_mode or not comparison_ran:
             if config.strict_blocking:
                 log.warning("--strict-blocking is only supported in diff mode. "
-                           "API mode (no diff) cannot evaluate existing violations.")
+                           "A full scan (no diff) cannot evaluate existing violations.")
             if not config.disable_blocking:
-                log.debug("Temporarily enabling disable_blocking due to no supported manifest files")
+                log.debug("Temporarily enabling disable_blocking: this run created a full scan, not a comparison")
                 config.disable_blocking = True
 
         # Post commit status to GitLab if enabled

tests/unit/test_pull_request_context.py

@@ -1,4 +1,25 @@
-from socketsecurity.core.pull_request import resolve_pull_request_context
+import pytest
+
+from socketsecurity.core.pull_request import (
+    parse_pull_request_number,
+    resolve_pull_request_context,
+)
+
+
+@pytest.mark.parametrize(
+    "value",
+    # "false" is what Buildkite puts in BUILDKITE_PULL_REQUEST on a branch build.
+    # main_code stores this result rather than the raw value, because the string
+    # is truthy and GithubConfig would read it as a real pull request number.
+    ["false", "0", "", None, "-1", "not-a-number"],
+)
+def test_non_pull_request_values_canonicalize_to_zero(value):
+    assert parse_pull_request_number(value) == 0
+
+
+def test_pull_request_numbers_survive_canonicalization():
+    assert parse_pull_request_number("42") == 42
+    assert parse_pull_request_number(42) == 42
 
 
 def test_explicit_pr_number_wins_over_detected_context():

tests/unit/test_socketcli.py

@@ -3,6 +3,7 @@
 import pytest
 
 from socketsecurity import socketcli
+from socketsecurity.config import CliConfig
 from socketsecurity.core.classes import Diff, Package
 from socketsecurity.socketcli import (
     build_license_artifact_payload,
@@ -74,12 +75,90 @@ def test_pr_context_provider_uses_integration_without_comment_adapter():
     assert socketcli._select_pull_request_provider("azure", "api") == "azure"
 
 
-def test_scm_merge_request_event_creates_diff():
-    assert socketcli._should_create_scm_diff("diff") is True
+# ---------------------------------------------------------------------------
+# SCM scan selection.
+#
+# Only a pull request or merge request event has a baseline, so every other
+# pipeline gets a full scan. These drive create_scm_scan against a recording
+# stub rather than asserting on the branch predicate, so swapping the call back
+# to create_new_diff fails them.
+# ---------------------------------------------------------------------------
 
 
-def test_scm_branch_event_always_uses_full_scan():
-    assert socketcli._should_create_scm_diff("main") is False
+class _RecordingCore:
+    def __init__(self):
+        self.calls = []
+
+    def create_new_diff(self, *args, **kwargs):
+        self.calls.append(("create_new_diff", args, kwargs))
+        return Diff(id="diff-scan")
+
+    def create_full_scan_with_report_url(self, *args, **kwargs):
+        self.calls.append(("create_full_scan_with_report_url", args, kwargs))
+        return Diff(id="full-scan")
+
+
+def _run_scm_scan(scm_event_type, **overrides):
+    core = _RecordingCore()
+    config = CliConfig.from_args(["--api-token", "test"])
+    diff, comparison_ran = socketcli.create_scm_scan(
+        core,
+        config,
+        scm_event_type,
+        **{
+            "scan_paths": ["."],
+            "params": object(),
+            "no_change": False,
+            "base_paths": None,
+            "explicit_files": None,
+            "external_href": "https://github.com/acme/widgets/pull/42",
+            **overrides,
+        },
+    )
+    return core, diff, comparison_ran
+
+
+def test_pull_request_event_creates_a_comparison_with_the_pr_link():
+    core, diff, comparison_ran = _run_scm_scan("diff")
+
+    method, _, kwargs = core.calls[0]
+    assert method == "create_new_diff"
+    assert kwargs["external_href"] == "https://github.com/acme/widgets/pull/42"
+    assert comparison_ran is True
+    assert diff.id == "diff-scan"
+
+
+@pytest.mark.parametrize("scm_event_type", ["main", None])
+def test_branch_event_creates_a_full_scan(scm_event_type):
+    core, diff, comparison_ran = _run_scm_scan(scm_event_type)
+
+    method, _, kwargs = core.calls[0]
+    assert method == "create_full_scan_with_report_url"
+    # A full scan has no before/after pair to associate the link with.
+    assert "external_href" not in kwargs
+    assert comparison_ran is False
+    assert diff.id == "full-scan"
+
+
+def test_api_only_diff_flags_do_not_force_a_comparison_on_a_branch_build():
+    """The detected event type is authoritative once an SCM adapter is active."""
+    core = _RecordingCore()
+    config = CliConfig.from_args(["--api-token", "test", "--enable-diff"])
+
+    _, comparison_ran = socketcli.create_scm_scan(
+        core,
+        config,
+        "main",
+        scan_paths=["."],
+        params=object(),
+        no_change=False,
+        base_paths=None,
+        explicit_files=None,
+        external_href=None,
+    )
+
+    assert core.calls[0][0] == "create_full_scan_with_report_url"
+    assert comparison_ran is False
 
 
 # ---------------------------------------------------------------------------