fix(release): bind client platform versions at pack time - #795
Conversation
The Node client manifests pinned their platform packages to the release being prepared. Those versions do not exist on npm while the release PR is open, so npm collapses them to placeholder lock entries, and npm tolerates that only until the release publishes. From that moment `npm ci` fails with EUSAGE on protected main, and every cycle repaired it by hand. Two gates enforced the same property in different places. The planner enforced it on the checked-in manifest and lockfile, which is what forced the contradiction; client_registry.py enforces it on the packed tarball, which is the guarantee a consumer actually depends on. Keep the second and invert the first: the tree binds nothing, the release binds the exact set into the manifest it packs, and validate-dist reads the tarball back to prove the binding landed. The binding sits after `npm ci`, which rejects a manifest its lockfile does not match, and before `npm pack`, which freezes the published manifest. A skipped binding fails validate-dist before publish, not after. The tree keeps naming its platforms in npm/<platform>/package.json, and the planner still checks each one, so the supported platform list stays visible and enforced where it is authored. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7f5a21f8d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Three of the four binding tests asserted substring ordering inside workflow `run:` blocks, so a reformat broke them without any behaviour changing. Drop them; the binder's own tests cover what it writes, and the call sites are shell wiring that first executes for real at the next rehearsal. The fourth was not a structure test. It reads the three checked-in manifests and lockfiles and asserts they bind no platform versions, which is the regression this change exists to prevent and the only guard that catches a re-add outside a prepare run. Move it beside the binder it belongs to. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
|
Both Codex findings reviewed. Neither changes the diff. P1 "Add the required DCO sign-off" — false positive. Both commits carry the trailer, and CI's P1 "Keep the change within one owning area" — real rule (AGENTS.md:321), correctly cited, but the remedy does not exist here. The two halves are mutually blocking, so either ordering leaves
The Splitting would trade an atomic, gated change for two commits that each break the default branch. Keeping it as one PR. |
Closes the structural half of #794. That PR repaired the v0.23.0 lockfiles by
hand; this removes the reason they needed repairing.
The contradiction
The three Node client manifests bound their platform packages to the exact
release version:
While the release PR is open those versions are unpublished, so npm cannot
resolve them and collapses the lock entries to
{"optional": true}. npmtolerates a placeholder only for as long as the version does not exist. The
moment the release publishes,
npm cion protectedmainfails withEUSAGEfor every contributor and every CI job that installs a client, until the next
prep rewrites the lockfiles. There is no version string that satisfies both
ends: correct-for-publish is unresolvable-while-open.
Why dropping it from the tree is safe
Two gates were enforcing the same property in different places:
registry-releasevalidate_client_package_versionspackage.json+ lockfileclient_registry.pyvalidate_npm_packagesrelease-candidate.yml,release.ymlThe second is the consumer-facing guarantee, and it already runs on the publish
path. The first only forced the contradiction. So the tree binds nothing, and
the release binds the exact set into the manifest it packs.
The field was dead weight in the tree anyway: the generated loader is
local-first, trying
require('./evidence-client.darwin-arm64.node')beforerequire('@registrystack/evidence-client-darwin-arm64'), and CI builds that.nodenatively.Where the binding happens
New
client_registry.py bind-optional-deps, called at three sites:release-candidate.ymljobassemble— the root tarball that actually gets publishedrelease-candidate.ymljobclients— the smoke stagerelease-rehearsal.ymljobnode-clients— the smoke stageOrdering is load-bearing and asserted by a structure test: after
npm ci,which rejects a manifest its lockfile does not match, and before
npm pack,which freezes the published manifest.
The planner's gate is inverted rather than deleted: a prepared tree that binds
platform versions is now rejected. Its per-platform loop over
npm/<platform>/package.jsonis unchanged, so the supported platform list staysvisible and enforced where it is authored.
Failure mode
Loud and pre-publish. A skipped or wrong binding fails
validate_npm_packageswith "root npm package does not bind the exact platform versions" while
validating the packed tarball, before anything reaches npm.
Security review notes
Release provenance is security-sensitive under
AGENTS.md, so, explicitly:root manifest carries the same
optionalDependenciesset as before; the testsassert the bound set is identical to what
validate_npm_packagesrequires.a second gate was already enforcing it on the publish path.
nameorversiondoes not match what itwas asked to bind, so it cannot silently bind the wrong package or version.
does pure local JSON I/O.
identical (asserted).
One cosmetic artifact worth naming: the binder writes with
json.dumps(indent=2),so the packed manifest's
publishConfigexpands from the tree's hand-compactedsingle line to three lines. Deterministic and repeatable, affects only the packed
copy, never the tree.
Scope
Cross-area (
crates/+release/+.github/workflows/) by necessity: the gateand the manifests it governs cannot move separately without leaving
mainin astate one of them rejects. Both directions are proven, not asserted:
origin/main'sregistry-release, all withmust bind the exact platform package versions.test_prepare_rejects_checked_in_platform_version_binding.The workflow half is the same atom: it is the call site that restores the
binding at pack time, without which
validate-distrejects the candidate.Tests
TDD, tests written first. 9 new tests:
test_client_registry.py—BindOptionalDependenciesTest, 8 tests: binds everyplatform at the exact version; binds exactly what the packed root package is
validated against; repeats byte-for-byte; keeps every other manifest field;
refuses another version; refuses another client; refuses a missing manifest;
CLI entry point.
test_client_registry.py—CheckedInClientManifestTest, 1 test: none of thethree real manifests or lockfiles binds platform versions. Mutation-tested by
re-adding the field, which fails it.
test_registry_release_plans.py— 1 test: prepare rejects a tree that bindsplatform versions, on both
package.jsonand the lockfile.An earlier revision also asserted the three call sites by matching substrings
inside workflow
run:blocks. Those were dropped: a reformat broke them with nobehaviour change, and the binder's own tests already cover what it writes. The
call sites are shell wiring, first executed for real at the next rehearsal.
Coverage the PR does not claim:
bind-optional-depsnever runs in PR CI,because
release-candidate.ymlandrelease-rehearsal.ymlare dispatch-only.What PR CI does prove is the regression that was actually breaking — the
Release Linux Node clientsjob runsnpm ciagainst the de-bound lockfiles onboth release runners, then builds and smoke-loads each native client.
No gates-inventory entry was added; that inventory matches run-step substrings,
which is the same brittleness the dropped tests had.
Verification
test_client_registrytest_registry_release_planstest_release_workflow_structuretest_registry_releasetest_release_candidate,test_verify_public_release,test_release_rehearsal,test_release_workflow_guard,test_check_gates_inventory,test_build_linux_node_clienttest_check_release_storage,test_cleanup_release_candidates,test_release_repeatability_workflow,test_zig_glibc_compiler,test_check_release_image_oci_labels,test_compare_release_image_layouts,test_check_release_source_modelcheck-gates-inventory.pyregistry-release validate-currentcheck-release-source-model.sh(monorepo mode)npm ciin all three client dirsnpm ci+build:debug+test+check:typesoptionalDependencieson buildDocs
release/OPERATIONS.mdgains a paragraph after the prepare-PR step explainingwhy the manifests bind nothing and where the binding happens instead, so the
next operator does not re-add the field.