Skip to content

Detect manifest changes across the whole pull request range, and fix full-scan reporting - #371

Merged
lelia merged 5 commits into
mainfrom
leliahui/ce-471-customer-gong-is-impacted-by-an-issue-with-pr-scan-diffing
Sep 24, 2026
Merged

lelia merged 5 commits into
mainfrom
leliahui/ce-471-customer-gong-is-impacted-by-an-issue-with-pr-scan-diffing

Conversation

@lelia

@lelia lelia commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Four fixes to how a pull request scan decides whether to compare, and what it reports when it does not.

Changed-file detection covers the whole range. Detection read a full comparison range only inside a recognized GitHub, GitLab, Bitbucket, or Buildkite pull request. Every other run fell through to git show HEAD and saw the tip commit alone, which made dependency gating depend on commit ordering: a pull request whose manifest changed in an earlier commit, followed by a source-only commit, looked like a source-only change. The comparison was abandoned for a full scan, blocking was suppressed, and the run exited 0. --base-commit-sha now also sets the range detection reads, reusing the same range detection the recognized providers already use.

A full scan no longer reports its findings as new. A run with no baseline suppresses blocking, because nothing can be attributed to the change. With an alert-bearing output format enabled the scan still carries every finding in the repository, and the console summary labeled those NEW and their link Diff Url — so it reported blocking issues while exiting 0, which reads as gating that silently failed rather than gating that correctly did not apply.

Removed packages no longer carry a doubled namespace. update_package_values already prefixes a namespaced purl, so prefixing it again while collecting removed artifacts produced com.example/widget@1.0.0com.example/com.example/widget@1.0.0 on every removed or replaced row of the dependency overview comment.

--ignore-commit-files is described by what it does. The CLI reference called it a way to force a full scan in four places, and --help said only "Ignore commit files". The flag forces a comparison. "Full scan" carries two meanings here — the set of files scanned, where the documentation was right, and the scan mode, where it stated the opposite — so anyone looking for a way to run a comparison when the changed-file check would skip one would rule out the only flag that does it.

Why these belong together

The first two are the same code path from both ends: when the no-supported-manifest fallback fires, and what it reports once it has. The documentation fix answers the question that path raises for anyone hitting it. The purl fix is a two-line deletion that would otherwise wait on its own review cycle.

Decisions worth confirming

  • An explicit base commit wins over CI environment inference. Passing --base-commit-sha states the range outright, so it is honored ahead of anything inferred from the environment. A test pins the precedence with the two sources deliberately disagreeing.
  • An unresolvable base commit warns rather than failing. The run continues down the existing ladder, but silently narrowing the comparison to one commit is the defect this fixes, so it says so. A base commit absent from a shallow checkout is fetched first; git fetch origin <sha> needs uploadpack.allowReachableSHA1InWant on the server, and the warning points at deepening the clone.
  • The alert list itself is unchanged. Only the console labels move. SARIF and JSON output read new_alerts, and renaming their fields would break consumers.

Testing

  • pytest -q tests/unit tests/core — 726 passed, 0 failed. A bare pytest honors testpaths = tests/unit and skips tests/core; both directories are run explicitly here, as CI does.
  • ruff check socketsecurity tests — clean.
  • uv lock --check and git diff --check — clean.
  • 12 new tests. Detection coverage builds real repositories with git init: a manifest changed mid-range under a source-only commit, precedence against CI variables, and an unresolvable base. The purl test was confirmed to fail without its fix, emitting the doubled string above.

Notes for review

  • uv.lock carries only the version line. Regenerating it locally rewrote nine dependency markers with unrelated emscripten conditions, which appears to be a uv version difference rather than anything this branch needs.
  • Each concern is its own commit, so any one can be reverted alone.

Fixes: CE-471, CE-472, CE-480


Note

Medium Risk
Changes core git changed-file logic and when diff vs full-scan paths run, which can alter CI gating behavior; output labeling changes are console-only but affect how operators interpret exit codes.

Overview
Release 2.10.0 tightens when PR-style comparisons run and how full-scan output reads in CI.

Changed-file detection now honors the full --base-commit-sha..HEAD range (explicit base wins over CI inference, uses .. instead of merge-base ...). Outside recognized PR/MR CI, runs no longer only inspect the tip commit—so a manifest change in an earlier commit still triggers a comparison. Unresolvable base SHAs emit a warning instead of silently narrowing to one commit.

Full-scan console summaries stop labeling repository-wide findings as NEW, use Report Url, and explain that counts do not affect exit code when there is no baseline.

Dependency diff comments no longer double-prefix namespaces on removed/replaced package purls.

Docs and --help for --ignore-commit-files now describe forcing a comparison over all manifests, not a “full scan” in the scan-mode sense.

Reviewed by Cursor Bugbot for commit 7fddc19. Configure here.

@lelia
lelia requested a review from a team as a code owner September 23, 2026 21:50

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread CHANGELOG.md Outdated
@lelia lelia changed the title Compare the whole base..HEAD range, and report a full scan as a full scan Detect manifest changes across the whole pull request range, and fix full-scan reporting Sep 23, 2026
@lelia
lelia force-pushed the leliahui/ce-471-customer-gong-is-impacted-by-an-issue-with-pr-scan-diffing branch from d350bd1 to d4bb253 Compare September 23, 2026 23:24
@lelia
lelia deployed to socket-firewall September 23, 2026 23:25 — with GitHub Actions Active
@lelia
lelia deployed to socket-firewall September 23, 2026 23:30 — with GitHub Actions Active
@lelia

lelia commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Changed-file detection reads a full comparison range only when it recognizes
the CI environment: a GitHub pull request, a GitLab merge request, a Bitbucket
pull request, or a Buildkite pull request. Every other run falls through to
`git show HEAD`, which sees the tip commit alone.

That makes dependency gating depend on commit ordering. A pull request whose
manifest changed in an earlier commit, followed by a source-only commit, looks
like a source-only change: the supported-manifest check fails, the comparison
is abandoned for a full scan, and blocking is suppressed, so the run reports no
new issues and exits 0.

A caller that supplies a base commit has stated the range outright, so honor it
ahead of any inference from the environment, reusing the same range detection
the recognized providers already use. An unresolvable base commit warns rather
than degrading quietly, because the fallback silently narrows the comparison to
one commit.
A run with no baseline creates a full scan and suppresses blocking, because
there is nothing to compare against and so nothing can be attributed to the
change. When an alert-bearing output format is enabled the scan still carries
every finding in the repository, and the console summary labeled those `NEW`
and their link `Diff Url`.

Both are wrong for a full scan, and the first contradicts the exit code: the
summary reported blocking issues while the run exited 0, which reads as gating
that silently failed rather than gating that correctly did not apply.

Label the counts and the link by what the run produced, and say why the counts
do not gate. The alert list itself is left alone, since SARIF and JSON output
read it and renaming their fields would break consumers.
update_package_values already prefixes a namespaced package's purl with its
namespace, so prefixing it again while collecting removed artifacts produced
`com.example/widget@1.0.0com.example/com.example/widget@1.0.0`.

The purl reaches the dependency overview comment verbatim, so every removed or
replaced row for a namespaced package rendered with an unreadable name. The
loop collecting added artifacts calls the same function and never did this.
The CLI reference described `--ignore-commit-files` as forcing a full scan in
four places, and `--help` said only "Ignore commit files". The flag forces a
comparison. The confusion is that "full scan" carries two meanings here: the
set of files scanned, where the documentation was right, and the scan mode,
where it stated the opposite of the behavior.

Anyone looking for a way to run a comparison when the changed-file check would
skip one would rule out the only flag that does it.

Also documents the range that changed-file detection reads, and that supplying
a base commit widens it.
@lelia
lelia force-pushed the leliahui/ce-471-customer-gong-is-impacted-by-an-issue-with-pr-scan-diffing branch from 7fddc19 to 5839d10 Compare September 24, 2026 02:11
@lelia
lelia deployed to socket-firewall September 24, 2026 02:11 — with GitHub Actions Active
@lelia
lelia merged commit 184dc0d into main Sep 24, 2026
32 checks passed

This branch was successfully deployed

1 active deployment
socket-firewall — 5839d100 Deployed Sep 24, 2026 by lelia via python-sfw-smoke-enterprise #427
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants