Repository navigation
Conversation
…slim them CLI_VERSION was declared at the top of the Dockerfile. Every RUN after an ARG is cache-keyed on its value, and CLI_VERSION changes on every release, so each release rebuilt every toolchain layer even when nothing in them changed. Between 2.11.10 and 2.11.11, 1,028 MB of the 1,049 MB compressed amd64 image was new layers, so every upgrade was close to a full pull. The release ARGs are now declared just before the CLI install, and a unit test keeps them there. The image also drops payload nothing runs: - rustup's minimal profile replaces the default one, which added offline HTML docs, rustfmt and clippy. Reachability runs only cargo. - npm and pip caches are no longer left in the image. - strace is no longer installed. - A .dockerignore keeps tests and other repo files out of the build context. dist/ stays in, since Dockerfile.preview copies the preview wheel from it.
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 29a60c9. Configure here.
…e the image in CI The release build cached layers with type=gha. Actions caches are scoped to the ref that wrote them, and each release builds on its own tag, so no release could read another's cache: every release rebuilt every layer under a new digest, and each wrote about 2.6 GB of cache nobody read. A registry cache at socketdev/cli:buildcache is readable from any tag, and reusing its layers keeps their digests stable across releases. e2e-image.yml builds the image from the commit, reusing that cache read-only, and runs --reach inside it against the npm fixture and a new Rust fixture. It follows e2e-test.yml's retry policy for the backend's intermittent empty results. Validation asserts what the image controls: the run completes and Coana reaches a reachable or unreachable verdict for at least one vulnerability, which needs the ecosystem's toolchain to work. It does not assert which verdict, since that depends on Coana and the backend.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Oskar Haarklou Veileborg (BarrensZeppelin)
left a comment
Oskar Haarklou Veileborg (BarrensZeppelin)
left a comment
There was a problem hiding this comment.
[agent] Simplification pass (quality only, not a bug hunt). Five suggestions and one comment-wording note, all inline. Looked fine: the Dockerfile parser in the test, the validator's own logic, the minimal Rust profile with cargo --version, npm cache clean, dropping apk update, adding Dockerfile to the unit-test push paths, and the .dockerignore entries (none are dead).
| matrix: | ||
| fixture: [simple-npm, simple-cargo] |
There was a problem hiding this comment.
[agent] Each matrix entry builds the ~3.4 GB image separately and pulls the cached layers from Docker Hub on its own, so every run builds the image twice. That build is by far the most expensive step here. One job could build once and loop over both fixtures, recording each fixture's result so one failure doesn't hide the other. The trade-off is losing a separate check name per fixture.
There was a problem hiding this comment.
Done in ede86cf. One job now builds the image once and loops over both fixtures. Each fixture's result is recorded and the job fails at the end with the list of failures, so one fixture can't hide another. Fixtures are grouped in the log. We accepted the trade-off of a single check name, e2e-image-reachability.
| max_attempts=3 | ||
| attempt=1 | ||
| while :; do | ||
| in_image "$IMAGE" --target-path "$FIXTURE" --reach --disable-blocking --enable-debug 2>&1 | tee /tmp/e2e-output.log | ||
| if in_image --entrypoint python "$IMAGE" tests/e2e/validate-image-reachability.py "$FIXTURE" --probe; then | ||
| break | ||
| fi | ||
| if [ "$attempt" -ge "$max_attempts" ]; then | ||
| echo "::warning title=e2e-image incomplete results::no alerted components after ${max_attempts} attempts; letting validation classify the result" | ||
| echo "diagnostics=true" >> "$GITHUB_OUTPUT" | ||
| break | ||
| fi | ||
| echo "::warning title=e2e-image transient retry::attempt ${attempt} produced no alerted components (suspected backend transient); retrying" | ||
| attempt=$((attempt+1)) | ||
| sleep 30 | ||
| done |
There was a problem hiding this comment.
[agent] This retry loop is a copy of the one in e2e-test.yml, and the probe and the inconclusive check in validate-image-reachability.py repeat reach-facts-probe.sh and validate-reachability.sh (see the comments there). That's two copies of the policy for the backend's intermittent empty results, which will drift. A shared script for the loop, used by both workflows, would keep them in step. This touches the e2e-test files, so it may suit a follow-up better.
There was a problem hiding this comment.
Done in ede86cf, in this PR rather than a follow-up. tests/e2e/run-with-retries.sh is now the only copy of the retry loop. e2e-test.yml calls it in place of its inline loop, and e2e-image.yml calls it per fixture. Both green on this run, including the required e2e-reachability.
| if args.probe: | ||
| return 0 if alert_states(args.fixture) else 1 |
There was a problem hiding this comment.
[agent] --probe repeats tests/e2e/reach-facts-probe.sh. This script could be the single implementation, with reach-facts-probe.sh reduced to uv run python tests/e2e/<script>.py <fixture> --probe. The script would then want a name that isn't specific to the image.
There was a problem hiding this comment.
Done in ede86cf. tests/e2e/reach_facts.py (renamed, so the name isn't image-specific) is the single implementation of the probe. reach-facts-probe.sh is gone: e2e-test.yml's retry-probe and validate-reachability.sh call uv run python tests/e2e/reach_facts.py <fixture> --probe directly, and the image job runs the same script with the image's Python.
| FACTS_FILE = ".socket.facts.json" | ||
| ANALYZED = ("reachable", "unreachable") | ||
| # Coana's log line when the backend returned no vulnerable components to analyze. | ||
| NOTHING_TO_ANALYZE = "Found 0 projects across 0 ecosystems to analyze" |
There was a problem hiding this comment.
[agent] validate-reachability.sh recognizes the same backend case by three log lines, including the "orphaned component" one, while this check uses only the first. Sharing one definition of that signature between the two validators would stop them from disagreeing about what counts as inconclusive.
There was a problem hiding this comment.
Done in ede86cf. reach_facts.py holds the one definition of the inconclusive backend result, as the same three log lines validate-reachability.sh checked: manifests uploaded, 0 projects to analyze, orphaned components filtered. The counts are generalized so it fits any fixture. validate-reachability.sh now calls --inconclusive instead of its own greps, and the image validation uses the same function.
| RUN uv export --directory /tmp/socket-cli-lock --frozen --no-dev --no-emit-project \ | ||
| --format requirements-txt -o /tmp/socket-cli-lock/requirements.txt && \ | ||
| pip install --require-hashes --no-deps \ | ||
| pip install --no-cache-dir --require-hashes --no-deps \ |
There was a problem hiding this comment.
[agent] --no-cache-dir is now on every pip install in this file (here and at lines 27, 169, 184 and 192). A single ARG PIP_NO_CACHE_DIR=1 just after FROM python:3-alpine would cover them all, including any future calls. Its value never changes, so it wouldn't cause the cache misses this PR fixes, and as an ARG rather than an ENV it applies only at build time.
There was a problem hiding this comment.
Done in ede86cf. ARG PIP_NO_CACHE_DIR=1 sits just after FROM python:3-alpine, and all the --no-cache-dir flags are removed, including the pre-existing one on the pip upgrade. It's on the unit test's list of ARGs allowed above the toolchain layers, since its value never changes.
| RELEASE_ARGS = { | ||
| "CLI_VERSION", | ||
| "SDK_VERSION", | ||
| "PIP_INDEX_URL", | ||
| "PIP_EXTRA_INDEX_URL", | ||
| "USE_LOCAL_INSTALL", | ||
| } |
There was a problem hiding this comment.
[agent] The test only knows these five ARGs, so a new ARG whose value changes per release, declared near the top of the Dockerfile, would still pass, even though that's the regression this test guards against. Turning the check around would cover it: list the ARGs allowed above the toolchain layers (GO_VERSION, JAVA_VERSION, DOTNET_VERSION, TARGETARCH) and fail on any other.
There was a problem hiding this comment.
Done in ede86cf. The test now lists the ARGs allowed above the toolchain layers (GO_VERSION, JAVA_VERSION, DOTNET_VERSION, TARGETARCH, PIP_NO_CACHE_DIR) and fails on any other. I checked it by adding an ARG BUILD_DATE near the top: the test failed and named it.
| # The image needs only the package source, its build metadata and the entrypoint | ||
| # script. Keep dist/: Dockerfile.preview copies the preview wheel from it. |
There was a problem hiding this comment.
[agent] Optional: the header says the image needs only the package source, its build metadata and the entrypoint script, but the file lists exclusions, which must grow whenever a new top-level directory appears. An allow-list would say what the comment says:
*
!socketsecurity
!pyproject.toml
!uv.lock
!README.md
!LICENSE
!scripts/docker-entrypoint.sh
!dist
There was a problem hiding this comment.
Done in ede86cf. Switched to your allow-list, plus **/__pycache__ and **/*.pyc exclusions. dockerfile-smoke (which builds with USE_LOCAL_INSTALL=true, so it exercises COPY . /app and the editable install) and e2e-image both pass with it.
| # Declared here rather than at the top: every RUN after an ARG is cache-keyed on | ||
| # its value, and CLI_VERSION changes on every release. Declared earlier, it | ||
| # would rebuild every toolchain layer above, so each release would be a full | ||
| # re-pull for anyone upgrading. |
There was a problem hiding this comment.
[agent] Minor: this comment and the cache comment in release.yml ("A registry cache rather than type=gha") both compare the code with the alternative it replaced. Our comment conventions keep code comments to the lasting reason. Here that's the warning itself, which is worth keeping: declared earlier, these ARGs would rebuild every toolchain layer on each release. It just doesn't need the "rather than at the top" framing.
There was a problem hiding this comment.
Done in ede86cf. The Dockerfile comment now states only the warning: keep these below the toolchain layers, because declared earlier they would rebuild every toolchain layer on each release. The release.yml comment says why the cache lives in the registry (Actions caches are scoped to the ref, and each release builds on its own tag) without comparing it to the old setting.
The reachability engine sets a minimum Node version, and Dependabot doesn't know Node's release schedule, so it would propose odd-numbered majors, which never get LTS. The floating node tag already picks up minor and patch releases.
… the Dockerfile guards - tests/e2e/run-with-retries.sh is the one retry loop for the backend's intermittent empty results, used by e2e-test.yml and e2e-image.yml. - tests/e2e/reach_facts.py is the one probe and the one definition of the inconclusive backend result, replacing reach-facts-probe.sh and the image-only validator. validate-reachability.sh calls it. - e2e-image.yml builds the image once and runs every fixture against it, recording each fixture's result so one failure doesn't hide another. - The Dockerfile test lists the ARGs allowed above the toolchain layers and fails on any other, so a new per-release ARG is caught too. - PIP_NO_CACHE_DIR as a build ARG replaces the per-call --no-cache-dir flags. - .dockerignore lists what the build context includes instead of what it excludes.
…ayers # Conflicts: # CHANGELOG.md # pyproject.toml # socketsecurity/__init__.py # uv.lock
Every release currently rebuilds every layer of the Docker image, so upgrading
socketdev/clidownloads nearly the whole image. This PR fixes that and removes about 1.1 GB of payload nothing runs.Root cause of the per-release re-pull
ARG CLI_VERSION(with the other release ARGs) was declared at the top of the Dockerfile. BuildKit cache-keys everyRUNafter anARGon that ARG's value, andCLI_VERSIONchanges on every release, so every toolchain layer missed the cache on every release build even when nothing in it changed. From the published amd64 manifests:The base image digest was the same across 2.11.10 → 2.11.11, so the churn didn't come from upstream. The release ARGs are now declared just before the CLI install.
tests/unit/test_dockerfile.pylists the ARGs allowed above the toolchain layers (GO_VERSION,JAVA_VERSION,DOTNET_VERSION,TARGETARCH,PIP_NO_CACHE_DIR) and fails on any other, so a new per-release ARG is caught as well. It fails againstmain's Dockerfile.Dockerfileis added to the Unit Testspushpaths, since the suite now reads it.Release build cache
Moving the ARGs only helps if a release can read the previous release's cache, and it can't today.
release.ymlcaches withtype=gha. Actions caches are scoped to the ref that wrote them, and each release builds on its own tag: the repository's BuildKit cache entries all sit underrefs/tags/v2.11.x, a full set per release, none ever read by the next one. That is also why the Actions cache is pinned at its 10 GB limit. The release build now uses a registry cache atsocketdev/cli:buildcache(mode=max), which any tag can read.I checked the mechanism with a local registry and a multi-platform push. With the registry cache, two builds with different
CLI_VERSIONvalues produced identical layer digests for aRUNstep that writes a timestamp, so the step was reused rather than re-run. An inline cache (cache-to: type=inlinewithcache-frompointing at the published image) did not hit after a multi-platform push, which is why this uses a dedicated cache tag. The cache tag is pushed to the same Docker Hub repository and isn't meant to be pulled.Reachability inside the image (
e2e-image.yml)e2e-test.ymlruns reachability on the runner, so nothing checked that the image's toolchains actually work for Coana. The new workflow builds the image once from the commit (reading the release cache, never writing it) and runs--reachinside it against the existing npm fixture and a new Rust fixture (smallvec 1.6.0, which has a known advisory). Each fixture's result is recorded, so one failure doesn't hide the other. It runs when the Dockerfile, the reachability launcher, or the e2e fixtures change, on push tomain, and on demand. It isn't a required check.Both e2e workflows now share one policy for the backend's intermittent empty results:
tests/e2e/run-with-retries.shis the retry loop: up to three attempts while a probe fails, with retries surfaced as warnings.e2e-test.ymlnow calls it instead of its inline loop.tests/e2e/reach_facts.pyis the probe, and the one definition of the inconclusive backend result (a manifest uploaded, components orphaned, nothing to analyze). It replacesreach-facts-probe.sh, andvalidate-reachability.shcalls it instead of its owngreps.The image check asserts only what the image controls: the run completes, and Coana reaches a
reachableorunreachableverdict for at least one vulnerability, which needs the ecosystem's toolchain to work. It doesn't assert which verdict.Dependabot and Node majors
.github/dependabot.ymlnow ignores major updates to thenodeimage. Dependabot doesn't know Node's release schedule, so it proposes odd-numbered majors, which never become LTS, and it can't check the reachability engine's minimum Node version. The floatingnode:24-alpine3.24tag already picks up Node 24 minor and patch releases on every build. A Node major becomes a deliberate change, checked withe2e-image.yml.Payload removed
minimalprofile (rustc,cargo,rust-std) instead ofdefault. Drops about 900 MB of offline HTML docs, plusrustfmtandclippy. Reachability only invokescargo.npm cache clean --forceafter the global install, andARG PIP_NO_CACHE_DIR=1so nopip installleaves a cache. As an ARG it applies only at build time, and its value never changes, so it doesn't affect layer caching.apk updateis gone;apk add --no-cachedoesn't need it.straceis no longer installed (it was referenced nowhere). The changelog notesapk add stracefor anyone who relied on it..dockerignore: lists what the build context includes (package source, build metadata, entrypoint, anddist/forDockerfile.preview's wheel) instead of what it excludes, so new top-level directories stay out by default.Verification
CI's
dockerfile-smokejob builds this Dockerfile natively on amd64 and arm64 and runs its tool and TLS checks. The measurements below are from locallinux/arm64builds.socketdev/cli:2.11.11vs 3,446 MB for this branch (uncompressed, arm64)./root/.rustupwent from 1,693 to 753 MB,/root/.npmfrom 20 to 1 MB,/appfrom 11 to 2 MB.CLI_VERSION=2.11.10, then rebuilt withCLI_VERSION=2.11.11. Steps 2–17, everything through the locked dependency install, wereCACHED. Only the CLI install and the small layers after it rebuilt. A real release also changespyproject.toml/uv.lock, which adds the locked-dependency layer (about 21 MB). So an upgrade is roughly 25 MB uncompressed instead of about 1 GB compressed.socketcli,cargo/rustc,go,java,dotnet,node,ruby/bundle,pyenv,uvandsocketall run.cargo generate-lockfile,cargo metadataandcargo buildwork on a crate with a crates.io dependency.Rust reachability in the image is covered by
e2e-image.ymlon this PR. A throwaway run withnode:26-alpine3.24(#444, closed) also passed both fixtures, for when Node 26 is adopted.Once the registry cache is in use, the old
type=ghaBuildKit entries (about 10 GB) can be deleted from the repository's Actions caches.Bumps to
2.12.0, a minor version because the image drops binaries (strace,rustfmt,clippy) and changes how it is built and published. #438 also targets2.12.0; whichever merges second rides that version, and needs a small merge in the changelog and inrelease.yml's build step.Fixes CE-430
🤖 Generated with Claude Code
Note
Medium Risk
Changes the default Docker toolchain (minimal Rust, no strace) and layer layout that all CLI container users depend on; Rust reachability in the image was not fully re-verified in this PR.
Overview
Release 2.11.12 reshapes how the published
socketdev/cliimage is built so version bumps reuse cached toolchain layers instead of forcing a near-full re-download, and trims roughly a gigabyte of unused payload.Layer caching:
CLI_VERSIONand the other release-specific build args move to just before the CLI install step. BuildKit invalidates every subsequentRUNwhen those args change; having them at the top previously busted cache for Go, Java, Rust, pyenv, locked deps, etc. on every release. A new unit test intest_dockerfile.pyasserts release ARGs stay below toolchain installs, andDockerfileis included in the CI paths filter on push.Smaller image: Rust is installed with rustup’s minimal profile (no offline docs,
rustfmt, orclippy);straceis dropped from the base packages; npm and pip caches are cleared or use--no-cache-dir; redundantapk updateis removed. A new.dockerignorekeeps tests, docs, and similar out of the build context while still allowing preview wheels fromdist/.Reviewed by Cursor Bugbot for commit 29a60c9. Configure here.