Reduce scan startup time in large repositories - #301
Open
lelia wants to merge 15 commits into
Open
Conversation
find_files() started a separate recursive rglob traversal for every expanded manifest pattern, so a scan re-walked each root once per pattern and only filtered excluded directories after descending into them. Replace that with one os.walk() per scan root: - Expand and case-fold all active patterns once, then match in memory. - Prune excluded directories, including .git, before descending. - Reject non-manifests on the basename alone (one set lookup plus one compiled glob alternation) before building a relative path or running a path match. - Cache supported manifest patterns per Core instance, but only when the API lookup succeeds, so a transient failure does not pin the run to the smaller local fallback pattern set. - Emit INFO durations for organization setup, pattern retrieval and discovery, with files/directories visited, directories pruned and manifests found. Matching behaviour is unchanged apart from intentionally excluding .git metadata. Adds parity tests against the previous rglob implementation for every built-in ecosystem and pattern, covering case-insensitivity, brace expansion, nested patterns, dot-directories, exclusions, inclusions, symlinks, excluded ecosystems, multiple roots, sorting and deduplication, plus an opt-in benchmark that asserts old/new result equality on a synthetic large-monorepo fixture. Ref: CE-379
Git.__init__() ran `git fetch --all` on every invocation, pulling every remote branch and tag before changed-file detection even began. Resolve commit and branch metadata locally instead, and for pull-request comparisons prefer refs already present in the checkout, fetching a single base or head ref only when it is missing. Also recognises Buildkite's native BUILDKITE_COMMIT, BUILDKITE_BRANCH, BUILDKITE_PULL_REQUEST and BUILDKITE_PULL_REQUEST_BASE_BRANCH so Buildkite jobs can calculate a complete base-to-head changed-file range without mapping their environment onto GitHub Actions variable names. Buildkite is checked before GitHub because some pipelines deliberately export GitHub-compatible variables. Adds INFO durations for Git initialisation, changed-file detection and each fetch, including the ref requested and why. Existing GitHub Actions, GitLab CI, Bitbucket Pipelines and local behaviour is preserved; tests cover local-ref preference, absence of an unconditional fetch, the targeted-fetch fallback, all four CI providers, and non-PR and detached-HEAD execution. Ref: CE-379
`--scm github` read its configuration solely from GITHUB_* variables, so Buildkite users had to shim every one of them to get PR comments. Fall back to Buildkite's own variables when the GITHUB_* equivalents are absent: PR number, commit, branch, checkout path, commit message, build creator, and owner/repository parsed from BUILDKITE_REPO (preferring the pipeline repository over a contributor's fork). Explicit GITHUB_* and PR_NUMBER values still take priority, and GitHub Enterprise remains configurable via GITHUB_API_URL. A running Buildkite PR build maps to the supported `synchronize` comment path, and a non-PR build maps to `push`, so event routing is unchanged. Default-branch detection requires an actual branch name rather than treating two unset variables as a match, which would otherwise mark any build as the default branch and overwrite the repository baseline. Ref: CE-379
The --sub-path routing pre-check walked every selected path to decide whether any manifests existed, then discarded the result so scan creation walked the same paths again. Retain and reuse it. Apply --excluded-ecosystems before the pre-check rather than after, so every find_files() call in a run sees the same ecosystem filter. Add an INFO duration for CLI run registration, and replace the "No Manifest files changed" line with wording that describes the decision being made: no supported manifest was detected in the changed-file set, so a full report is created. Scan-routing semantics are unchanged. Ref: CE-379
Bumped via .hooks/sync_version.py so __init__.py, pyproject.toml and uv.lock stay in sync, and moved the changelog entry under a 2.6.5 heading. Ref: CE-379
lelia
added
publish-preview
|
🚀 CLI preview published: pip install --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple socketsecurity==2.6.5.dev3165619303101TestPyPI's package index can take several minutes to expose a newly uploaded version. |
The publish-docker job downloads the built wheel to ./dist, but the build step omitted `context`, so docker/build-push-action used its default Git context. Buildx then cloned the repository as the build context, where ./dist does not exist, and `COPY dist/socketsecurity-*.whl` failed with "lstat /dist: no such file or directory". Set `context: .` so the build uses the workspace the artifact was downloaded into. This also makes the job's existing trust boundary hold as documented: the context is now the default-branch checkout rather than the pull-request ref, so Dockerfile.preview is read from trusted code and the pull request still enters the image only through the built wheel. Pre-existing; the TestPyPI half of the workflow is unaffected.
lelia
added
publish-docker-preview
|
🐳 Docker preview published: This mutable tag is only created when a Docker preview is explicitly requested. |
The preview image was amd64-only while the release and stable images are built for linux/amd64,linux/arm64, so a preview tag could not stand in for socketdev/cli:latest on arm64 hosts without emulation. Match the release arch matrix and enable QEMU so the arm64 layer can be built on an amd64 runner. Previews are opt-in via label, so the extra build time is an acceptable tradeoff for making the tag a drop-in replacement.
lelia
added
publish-docker-preview
…utable A finished comparison could sit unobserved for up to 30s between polls, which is dead time on every PR job. Lower the ceiling to 10s: a multi-minute comparison costs roughly 2x the polls while cutting worst-case dead time to 10s. Diff scans now log their ID, poll count, and the wait before the final poll at INFO. Previously the ID was debug-only, so a slow comparison in a customer CI log could not be tied back to a server-side diff scan, and there was no way to tell backend comparison time apart from time the result spent ready-but-unpolled. Also document the diff-scans token scopes. A token missing them still completes the scan, silently falling back to the streaming comparison, which differs in both transport and payload (cached diff-scan responses always embed per-package license details; the streaming path requests a lean payload). Ref: CE-379
PR/MR runs logged the head and new scan IDs but no link to the result, so a CI log gave no way to reach the report. Log the diff report URL where it is computed, so every diff flow gets it rather than only the full-scan-only branches. Also add a regression test asserting manifest discovery's peak allocation stays bounded by the widest single directory and the result set rather than by repository size. Measured against the per-pattern rglob approach this replaced, on a tree of 59,300 files including one 50,000-entry directory: 3.25 MB peak vs 10.72 MB. os.walk keeps a list of names per directory where rglob materialised DirEntry objects and a Path per candidate, so the single-pass walk allocates strictly less. Ref: CE-379
Probed the live API against an existing diff scan to confirm what the polling path can and cannot ask for: - omit_license_details is ignored when cached=true, as the existing comment said. License fields remain in the response. - omit_unchanged IS honored and removes unchanged artifacts entirely, measured at ~1.1 KB per artifact (225,542 B -> 78,003 B when dropping 135 of 192 artifacts). Record why the CLI still does not send omit_unchanged: unchanged artifacts feed diff.unchanged_alerts, which create_security_comment_gitlab and the FOSSA compat issue list read unconditionally, not only under --strict-blocking. Omitting them would silently shrink those outputs, so this needs proper gating in its own change rather than a param tweak here. Ref: CE-379
Cached diff-scan responses embed every unchanged artifact at roughly 1 KB each. On a large dependency tree that is nearly the whole response — measured at ~11 MB for a tree with ~10k unchanged packages — downloaded, deserialised into Package objects and then discarded on every pull request. omit_unchanged is honored by the API (unlike omit_license_details, which cached responses ignore), so request it whenever no enabled output reads that half of the comparison. Verified against the live API through the SDK: 192 artifacts -> 57. Every consumer is behind an opt-in flag, so the gate is centralised in Core._requires_unchanged_artifacts with the reasoning recorded there: - --strict-blocking blocks on pre-existing issues via diff.unchanged_alerts - --enable-gitlab-security includes them in the dependency scanning report - --generate-license enumerates diff.packages, which must list every dependency - --legal-format fossa reports all currently-present issues Diff.to_dict serialises them too but has no callers. When cli_config is absent the caller is unknown, so the full payload is kept. Tests parametrise over every flag in that list so a new reader of diff.unchanged_alerts or diff.packages cannot be added without also updating the gate. The completion log reports omit_unchanged so it is visible whether the optimisation engaged on a given run. Ref: CE-379
lelia
added
publish-docker-preview
…rtup-time # Conflicts: # CHANGELOG.md
lelia
added
publish-docker-preview
lelia
marked this pull request as ready for review
August 18, 2026 18:24
Contributor
Author
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Author
|
bugbot run |
There was a problem hiding this comment.
✅ 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 a7c6ba3. Configure here.
Oskar Haarklou Veileborg (BarrensZeppelin)
approved these changes
Aug 19, 2026
Oskar Haarklou Veileborg (BarrensZeppelin)
left a comment
Oskar Haarklou Veileborg (BarrensZeppelin)
left a comment
Member
There was a problem hiding this comment.
Perf improvements LGTM. ✅
I don't have context on Buildkite/GHA.
Comment on lines
+106
to
+112
| # | ||
| # The max interval is also the upper bound on how long a finished comparison sits | ||
| # unnoticed between polls, which is dead time added to every PR job. Callers commonly | ||
| # run this inside a CI step with a per-step time budget of a few minutes, so the cap is | ||
| # kept small: a multi-minute comparison costs roughly 2x the polls of a 30s cap while | ||
| # cutting the worst-case dead time from 30s to 10s. Diff scans log their poll count and | ||
| # last interval on completion so this tradeoff can be re-evaluated against real timings. |
There was a problem hiding this comment.
Suggested change
| # | |
| # The max interval is also the upper bound on how long a finished comparison sits | |
| # unnoticed between polls, which is dead time added to every PR job. Callers commonly | |
| # run this inside a CI step with a per-step time budget of a few minutes, so the cap is | |
| # kept small: a multi-minute comparison costs roughly 2x the polls of a 30s cap while | |
| # cutting the worst-case dead time from 30s to 10s. Diff scans log their poll count and | |
| # last interval on completion so this tradeoff can be re-evaluated against real timings. |
git blame will tell you why 10s was chosen. 🤷
| candidate_basenames = set(literal_basenames) | ||
| candidate_basename_globs = set(basename_globs) | ||
| for pattern in path_globs: | ||
| final_segment = pattern.rstrip("/").rsplit("/", 1)[-1] or "*" |
There was a problem hiding this comment.
The only way final_segment becomes "*" is if the pattern is empty or has a suffix of "//". Is that intended?
There was a problem hiding this comment.
IMO this can be trimmed a bit. Claude likes to include a lot of details from the problem investigation, which is maybe not necessary for a user-facing changelog.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reduce Python CLI scan setup time in large repositories by replacing repeated filesystem traversal and broad Git fetching with single-pass, local-first implementations. Add native Buildkite pull-request context and enough diff-scan telemetry to distinguish client work from backend comparison time.
In an affected Buildkite pipeline, the preview reduced the two local bottlenecks from roughly 8 minutes combined to about 3 seconds:
The preview found the same 765 manifests after visiting 175,045 files. The remaining end-to-end time was dominated by backend scan comparison and is being investigated separately.
Changes
os.walk()per scan root, pruning excluded directories and.gitbefore descent. Cache successfully loaded patterns and reuse discovery results for--sub-pathscans.git fetch --all. Prefer refs already in the checkout and fetch only a missing base or head ref when a comparison requires it.Behavior Notes
.gitis intentionally excluded from manifest discovery.*.egg-infoare applied as intended.Validation
omit_unchangedremoves unused artifacts while preserving them for every output mode that consumes them.476 passed, 2 skipped; Ruff clean on all changed Python files; source and wheel builds verified.Ref: CE-379
Note
Medium Risk
Touches core manifest discovery, Git/PR detection, and diff comparison payload gating across CI providers; behavior is heavily tested but regressions could affect scan routing or PR comment context on edge-case checkouts.
Overview
Cuts local scan startup on large repos by walking each scan root once for manifests (pruning
.gitand excluded dirs before descent) instead of per-patternrglob, caching API manifest patterns, and reusing--sub-pathdiscovery results for scan creation. Git init no longer runsfetch --all; PR changed-file detection uses checkout refs and fetches only missing base/head history when needed.Buildkite is recognized natively for commit, branch, PR range, and GitHub SCM comment context (without GitHub Actions env shims). INFO timings cover org setup, Git, changed files, pattern load, manifest discovery, and diff polling; diff reports log URL and scan ID.
PR comparisons request
omit_unchangedon cached diff-scans when no flag needs unchanged packages (--strict-blocking, GitLab security,--generate-license, FOSSA legal format), shrinking large cached responses. Poll backoff caps at 10s (was 30s). Docs cover required diff-scan API token scopes and troubleshooting.CI preview images build from the workspace wheel context and publish linux/amd64 and linux/arm64 (QEMU enabled).
Reviewed by Cursor Bugbot for commit a7c6ba3. Configure here.