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.
| 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.
| 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.
| 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.
| 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.
| 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.
| # 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
| # 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.
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.pyfails if one is declared above a toolchain layer again; 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 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). 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.It reuses e2e-test.yml's approach to the backend's intermittent empty results: up to three attempts while the facts file has no alerted components, then a warning instead of a failure only when the log shows the backend's "nothing to analyze" line. Validation 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, since that depends on Coana and the backend.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, andpip install --no-cache-dirthroughout.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: keepstests/and other repo files out of the context and/app.dist/stays in, becauseDockerfile.previewcopies the preview wheel from it.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 now covered by
e2e-image.ymlon this PR.Once the registry cache is in use, the old
type=ghaBuildKit entries (about 10 GB) can be deleted from the repository's Actions caches.#438 adds
sbom: truenext to the cache lines inrelease.yml, so whichever merges second will have a small conflict there.Bumps to
2.11.12. #438 also targets2.11.12; whichever merges second rides that version and only needs the changelog merged.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.