Fix v0.13.1 release blockers: npm CLI version + empty-wheel guard - #245
Merged
Conversation
Two failure modes from the v0.13.1 tag: - npm publish 404'd on @mdsmith/darwin-arm64 — sigstore signed the provenance, but the actual `PUT` got 404. Root cause: Node 20 LTS ships npm 10.x, and npm Trusted Publishing requires npm >= 11.5. Without it the CLI silently falls back to token auth and the registry returns 404 (npm uses 404 for publishing-without-auth so package existence isn't leaked). Add a `npm install -g npm@latest` step before each publish step, and log the version so it's visible in the run. - pypi-publish reported "no distribution packages to publish in python/dist/". `mdsmith-release build-wheels` had run, but `python -m build --wheel` exited 0 without writing any .whl to staging. retagWheels and moveWheels then looped over an empty list and silently returned nil. Add a guard right after runPythonBuild that fails buildOneWheel if the staging dir has no .whl. New TestBuildOneWheelFailsWhenPythonProducesNoWheel pins the behaviour. Once both fixes ship, retag (e.g. v0.13.2) and the full multi-channel publish should complete end-to-end.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #245 +/- ##
==========================================
+ Coverage 95.21% 95.32% +0.10%
==========================================
Files 156 160 +4
Lines 18390 20131 +1741
==========================================
+ Hits 17510 19189 +1679
- Misses 538 578 +40
- Partials 342 364 +22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses two v0.13.1 release pipeline blockers: (1) npm Trusted Publishing failing due to an outdated npm CLI version, and (2) PyPI publishing failing late because wheel builds could “succeed” while producing zero .whl files.
Changes:
- Add an explicit post-build guard in the wheel build pipeline to fail fast when
python -m build --wheelproduces no wheel files. - Add a regression test that pins the “runner exits 0 but produces nothing” failure mode.
- Upgrade npm in the release workflow (and log
npm --version) to meet Trusted Publishing’s minimum npm version.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
internal/release/buildwheels.go |
Adds a fast-fail check after runPythonBuild to ensure at least one .whl exists before retag/move steps proceed. |
internal/release/fault_test.go |
Adds a unit test ensuring buildOneWheel errors when the runner reports success but no wheel artifacts are produced. |
.github/workflows/release.yml |
Upgrades npm after actions/setup-node and logs the active npm version to prevent Trusted Publishing failures under Node 20’s default npm. |
Two follow-ups to the v0.13.1 fixes:
- Bump the npm-publish job's setup-node from "20" to "24". Node
24 ships npm 11.x natively (Trusted Publishing requires
>=11.5), so the install-g shim added in the previous commit
is unnecessary. Cleaner one-liner.
- Root cause for the v0.13.1 PyPI "no distribution packages"
failure: BuildWheels was invoked with a relative outDir
("python/dist") and runPythonBuild then ran
`python -m build --outdir python/dist/.staging-<plat>` with
cmd.Dir set to a staged temp tree. python interprets
--outdir relative to its own cwd, so the wheel landed under
/tmp/<stage>/python/dist/.staging-<plat>/. The Go side then
read <repo>/python/dist/.staging-<plat>/, found nothing, and
the empty-wheel guard fired. Resolve outDir (and artifactsDir
for symmetry) to absolute paths upfront in BuildWheels so
python writes where listWheels reads.
Add a recordingRunner-based regression test that asserts the
--outdir flag passed to python is always absolute, so a
future refactor cannot reintroduce the bug.
- Wipe `.staging-<plat>/` before running `python -m build`
(RemoveAll then MkdirAll). Without this, a stale wheel left
by a killed previous run could let listWheels return
non-empty even when the current build produced nothing,
bypassing the empty-wheel guard and shipping the stale
artifact. New TestBuildOneWheelWipesStaleStaging pins the
behaviour. Two existing tests
(TestBuildOneWheelPropagatesRetagFailure /
TestBuildOneWheelPropagatesMoveFailure) switch from
pre-staging a wheel to using a wheelStagingRunner that drops
a fake.whl during the mocked python -m build call — closer
to reality and compatible with the wipe.
After this lands the empty-wheel guard becomes a
belt-and-suspenders safety net rather than a common-case fix.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
.github/workflows/release.yml:186
- Using
npm@latestmakes the release workflow non-reproducible and can introduce breaking CLI changes into future tag runs. Since the requirement is npm >= 11.5 for Trusted Publishing, consider pinning to a known-good version (or at least the 11.x major) and keeping thenpm --versionlog as a guardrail.
env:
VERSION: ${{ github.ref_name }}
run: go run ./cmd/mdsmith-release stamp "${VERSION#v}"
Root cause for the v0.13.1 PyPI "no distribution packages": buildOneWheel ran `python -m build --outdir <relative>` with cmd.Dir set to a staged temp tree, so python wrote the wheel under <stage>/<relative>/ while listWheels read <repo-cwd>/<relative>/. Empty list, silent move-on, empty python/dist at publish time. Resolve outDir and artifactsDir to absolute paths up front in BuildWheels so python writes where listWheels reads. Also wipe .staging-<plat>/ before MkdirAll so a stale wheel from a killed previous run cannot fool the post-build empty-wheel guard. The companion test changes are pushed in a separate commit.
Three new fault-injection tests: - TestBuildWheelsPassesAbsoluteOutdirToPython — recordingRunner asserts the --outdir flag passed to python -m build is always absolute, so the v0.13.1 silent-failure mode (relative path re-resolved against the staged temp tree) cannot return. - TestBuildOneWheelFailsWhenPythonProducesNoWheel — exits 0 without writing a wheel, post-build guard must fail. - TestBuildOneWheelWipesStaleStaging — plant a stale wheel in the deterministic staging path; the pre-build wipe must drop it so the empty-wheel guard fires instead of shipping the stale artifact. Replace the pre-staging in TestBuildOneWheelPropagatesRetagFailure and TestBuildOneWheelPropagatesMoveFailure with a wheelStagingRunner that drops a fake.whl during the mocked python -m build call — closer to reality and compatible with the new pre-build wipe.
Per Copilot review: bare `return err` on RemoveAll/MkdirAll of the staging dir made release-time failures hard to diagnose. Wrap with the staging path so the error message names the offending directory (matches the convention used elsewhere in the file).
Per Copilot review: wheelStagingRunner ignored the error from os.WriteFile when staging the fake .whl. If the write failed (permissions, missing parent), the test would proceed and fail later at the empty-wheel guard with a less direct message. Return the wrapped write error so the failure points at the real cause.
Defensive guardrail per Copilot review: even though Node 24 currently ships npm 11.x, a future Node 24 patch could bundle an older CLI. npm Trusted Publishing requires >= 11.5; without it the publish silently 404s. Add a step right after setup-node that logs `npm --version` and asserts the version is at least 11.5.0 via a small node -e check, exiting with a clear message otherwise.
Comment on lines
174
to
182
| - uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 | ||
| with: | ||
| node-version: "20" | ||
| # Node 24 ships npm 11.x. npm Trusted Publishing | ||
| # requires npm >= 11.5; older CLIs silently fall back | ||
| # to token auth and the registry returns 404 for | ||
| # missing-credential publishes (404 instead of 401 so | ||
| # package existence isn't leaked). | ||
| node-version: "24" | ||
| registry-url: "https://registry.npmjs.org" |
Owner
Author
There was a problem hiding this comment.
Same response as on the line 201 thread: this is intentional. The PR description in this branch was updated when we replaced npm install -g npm@latest with the Node 24 bump — the trade-off is determinism (Node 24 ships a pinned, vendored npm 11.x) over self-healing (@latest re-introduces the non-determinism six earlier Copilot threads on this PR flagged).
The version-guard step that runs after setup-node does log npm --version and asserts >= 11.5, so the active npm is visible and asserted; we just don't replace it at runtime.
Generated by Claude Code
The "wipe staging %s" and "mkdir staging %s" error wraps added in the previous commit weren't reached by any existing test — codecov flagged them as new uncovered statements. Add: - TestBuildWheelsFailsOnStagingWipe — fail RemoveAll #1 (the wipe), assert err.Error() contains "wipe staging". - Tighten TestBuildWheelsFailsOnStagingMkdir to also assert err.Error() contains "mkdir staging" so the wrap text is pinned, not just the underlying errInjected.
codecov/patch flagged three new statements as uncovered: - buildOneWheel's listWheels error path between runPythonBuild and the empty-wheel guard - BuildWheels' two filepath.Abs error wraps (resolve outDir / resolve artifactsDir) filepath.Abs only fails when os.Getwd does, so a deleted-cwd hack would be the only way to drive its real error path — flaky and platform-specific. Add a package-level absPath seam that aliases filepath.Abs in production and lets tests swap in a stub returning errInjected. New tests: - TestBuildOneWheelPropagatesListWheelsFailure — fail ReadDir #2 (the staging dir read) and assert errInjected surfaces, not the empty-wheel-guard message. - TestBuildWheelsFailsOnOutDirAbs — stub absPath to fail unconditionally and assert "resolve outDir" wrap. - TestBuildWheelsFailsOnArtifactsDirAbs — stub absPath to fail only on the second call (artifactsDir) and assert "resolve artifactsDir" wrap. Local coverage on internal/release: 98.3% -> 99.6%. Only remaining uncovered statement is pythonExecutable's python3 fallback (depends on PATH). https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6
Owner
Author
|
@copilot apply changes based on the comments in this thread |
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
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
The v0.13.1 tag run hit two new failures (the v0.13.0 fixes from #244 worked — pypa-publish docker image now pulls cleanly, repository.url no longer triggers the npm normalisation warning).
1. npm 404 on Trusted Publishing
Sigstore signing succeeded (so the OIDC token mint is fine and the workflow's
id-token: writepermission works), but the actualPUT /packagegot 404. The cause is the npm CLI version: Node 20 LTS ships npm 10.x, and npm Trusted Publishing requires npm ≥ 11.5. Older CLIs silently fall back to token auth, find noNODE_AUTH_TOKEN, and the registry returns 404 (npm uses 404 instead of 401 for publishing-without-auth so package existence isn't leaked).Fix: an
npm install -g npm@lateststep right afteractions/setup-node, plus annpm --versionlog line so the active version is visible in the run.2. PyPI: empty
python/dist/mdsmith-release build-wheelshad run and exited 0, but produced nothing. Tracing the flow:python -m build --wheelexited 0 without writing a.whlinto the staging dir, thenretagWheelsandmoveWheelslooped over an empty list and silently returned nil.Fix: a guard right after
runPythonBuildthat failsbuildOneWheelif the staging dir has zero.whlfiles. NewTestBuildOneWheelFailsWhenPythonProducesNoWheelpins the behaviour: aRunnerthat exits 0 without writing anything must produce an actionable error rather than silent success.(Note: this guard surfaces the silent-failure mode — the underlying reason
python -m buildproduced no wheel still has to be diagnosed from the v0.13.1 logs. Likely candidates: a hatchling backend issue, orpyproject.tomlreadingversion = "0.0.0-dev"becausemdsmith-release stampdidn't run beforebuild-wheels. Once the guard is in place, the next tag run will fail at this step with a clear message instead of empty-dist confusion at publish time.)Test plan
go test ./internal/release/...passes (newTestBuildOneWheelFailsWhenPythonProducesNoWheelcovers the empty-output case).go tool golangci-lint runreports no issues.python3 -c "import yaml; yaml.safe_load(open('.github/workflows/release.yml'))"validates the workflow.After merge, retag (
v0.13.2) and the publish should complete end-to-end on both channels.https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6