Make package and Docker previews opt-in - #287

✅ 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 73504a6. Configure here.}

ai@cursor: review of the workflow restructuring, the Dockerfile change, and the preview overlay. The split into a credential-free build job that produces an artifact and separate publish jobs that consume it is the right shape, and it's a clear improvement over holding id-token: write through a ten-minute TestPyPI polling loop. Most of what follows is about the manual-dispatch path and the Docker preview.

Manual dispatch bypasses the fork check

The fork check sits inside the label branch of the ||, so workflow_dispatch short-circuits it:

if: >-
  github.event_name == 'workflow_dispatch' ||
  ((github.event.label.name == 'publish-preview' ||
  github.event.label.name == 'publish-docker-preview') &&
  github.event.pull_request.head.repo.full_name == github.repository)

Once github.event_name == 'workflow_dispatch' is true, head.repo.full_name is never consulted, and the checkout resolves to format('refs/pull/{0}/head', inputs.pr_number) for any PR number including fork PRs. The build job then runs code from that ref: .hooks/sync_version.py, the local composite action ./.github/actions/setup-hatch, and the hatchling build hooks. build itself is well isolated with contents: read and no secrets, but its artifact is published to TestPyPI under the org's trusted publisher.

publish-docker is more direct than that. It checks out the PR ref, runs docker/login-action with DOCKERHUB_TOKEN, and then builds a Dockerfile.preview taken from that same ref and pushes to socketdev/cli. A dispatched fork PR gets to author a Dockerfile that runs with Docker Hub credentials already on the runner.

I assume this is intentional and the maintainer running the dispatch is vouching for the code. Two things would make that safe to rely on. First, say so explicitly in CONTRIBUTING — the current text puts "Label-triggered publication is skipped for pull requests from forks" directly above the dispatch paragraph, which reads as though the protection carries over to dispatch. Second, put a GitHub environment: on publish-package and publish-docker so the gate is enforced by GitHub with a required reviewer rather than by an if: expression, and scope the TestPyPI trusted publisher to that environment. As things stand, any workflow in the repo with id-token: write can mint a TestPyPI token; release.yml has the same property, so this is pre-existing rather than a regression, but this PR is the natural place to close it.

inputs.pr_number is unvalidated

It's type: string and reaches echo "pr_number=$INPUT_PR_NUMBER" >> "$GITHUB_OUTPUT", so a multi-line value can set arbitrary additional job outputs. It also becomes the Docker tag socketdev/cli:pr-<value> and Number(process.env.PR_NUMBER) in github-script, where a non-numeric value produces NaN and an opaque API error rather than a clear failure. Dispatch is maintainer-only so the severity is low, but a numeric guard in the "Record preview context" step is one line.

The format('refs/pull/{0}/head', inputs.pr_number) usage is fine — that's an action input rather than a shell.

Dockerfile.preview

--no-deps on the wheel install. If a PR adds or bumps a runtime dependency, the preview image silently won't have it. socketcli --help catches top-level import failures, so many cases fail at build time, but a lazily imported new dependency ships a broken preview. "I added a dependency" is close to the most likely reason someone asks for a preview image in the first place, so this is worth either fixing or documenting in CONTRIBUTING.

ARG PIP_INDEX_URL=https://test.pypi.org/simple/. BuildKit exposes build args to RUN as environment variables and pip reads PIP_INDEX_URL from the environment, so TestPyPI is the implicit index for that entire layer. Harmless today because the wheel install is --no-deps from a local path, but if --no-deps ever comes off, dependency resolution silently moves to TestPyPI. Defaulting the ARG to real PyPI and passing the TestPyPI URL only on the SDK_PREVIEW_VERSION install removes the trap.

FROM socketdev/cli:latest without pull: true. docker/build-push-action doesn't pull by default, so a warm builder can resolve a stale base and nobody would know. Either set pull: true or include the resolved base digest in the PR comment so the tester knows what they're on top of.

The GHA cache isn't buying anything. COPY dist/socketsecurity-*.whl changes on every run, so no layer after it can be reused, and cache-to: mode=max then exports the whole base image's layers on each build. For a two-instruction overlay I'd drop cache-from and cache-to entirely.

sync_version.py

--preview-id validates with str.isdigit(), which returns True for characters like ² and ٣. Those produce an invalid PEP 440 version that fails later in twine check or at upload with a much worse error than the guard was meant to give. CI only ever passes $((run_id * 100 + attempt)) so it can't happen today, but if the check is worth having it may as well be correct — int() in a try, or isascii() and isdigit().

Separately, the file now has four flags parsed three different ways: membership tests for --dev and --skip-lock, and sys.argv.index with manual IndexError handling for --preview-id. argparse would be shorter and would give the type and arity checks for free. Not blocking.

The Dockerfile fix should probably be its own PR

The retry-loop change is a real bug fix and it's correct. The old for ... done && if [ -n "$SDK_VERSION" ] chain always exited 0, because a shell for loop's status is that of its last command — sleep 30 — so ten consecutive failed installs produced a "successful" image with no CLI in it. Skipping the final sleep is a nice touch too.

That said, it's the only change in this PR that affects a released artifact, and it's the change most likely to need an independent revert. Splitting it out would make both halves easier to reason about.

The dependency-review.yml change follows from it: with the fix in place, the smoke build's empty CLI_VERSION would hard-fail, so USE_LOCAL_INSTALL=true is necessary, and it's a better test for a dependency bump anyway. Worth noting the side effect, though — no CI job builds the Dockerfile's published-package install path anymore, so a regression in that branch first surfaces at release. Also, the local-install branch runs pip install --upgrade socketdev unpinned, which can install an SDK outside this project's own socketdev>=3.3.0,<4.0.0 constraint. Pre-existing, but it's now on the smoke path.

Package Check as a required check

The description proposes making this the required branch-protection check. Note that Unit Tests is path-filtered on socketsecurity/**, tests/**, pyproject.toml, and uv.lock, so it didn't run on this PR at all — this branch has had no Python test coverage in CI. If Package Check becomes the gate, it's a required check that runs no tests, on exactly the PRs the path filter excludes. Either run the unit tests inside Package Check (as the SDK's version of this workflow does) or make both required.

While on the subject: CONTRIBUTING now tells contributors to run make lint before opening a PR, and no workflow runs ruff. If Package Check is becoming the gate, lint is the obvious thing to add to it.

Smaller notes

pr-preview.yml now inlines setup-buildx-action and login-action instead of using ./.github/actions/setup-docker, which still exists and is used by release.yml and docker-stable.yml. The reason is clear — the composite pulls in QEMU and previews are amd64-only — but the pinned SHAs now live in two places and will drift. A qemu: false input on the composite would keep them in one place.

pypa/gh-action-pypi-publish is pinned to v1.14.1 here and v1.14.2 in SocketDev/socket-sdk-python#100. Worth aligning while the two are being changed together.

Checked and fine

The needs: build plus explicit if: combination on both publish jobs is correct. Neither condition uses always(), failure(), or cancelled(), so the implicit needs-success gate still applies and a failed build skips both. Worth a comment in the file, since it's a well-known footgun and the next person to add always() for a notification step would quietly open the gate.

Dropping | cut -d+ -f1 from the version read is safe: inject_version writes an exact version into both socketsecurity/__init__.py and the static version = in pyproject.toml, so no local segment can appear and get rejected at upload. Removing the rm -rf dist/ step is likewise fine on a fresh runner, and with no .dockerignore in the repo the downloaded dist/ artifact reaches the build context despite being gitignored. Both labels exist in the repo.