Skip to content

fix(release): resolve native platform entries and registry metadata in shrinkwrap - #1564

Merged
lavaman131 merged 1 commit into
mainfrom
fix/native-shrinkwrap-metadata
Jun 30, 2026
Merged

fix(release): resolve native platform entries and registry metadata in shrinkwrap#1564
lavaman131 merged 1 commit into
mainfrom
fix/native-shrinkwrap-metadata

Conversation

@flora131

@flora131 flora131 commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

Hardens the @bastani/atomic npm shrinkwrap generator so that release publishes correctly resolve the already-published @bastani/atomic-natives registry metadata and all six optional native platform packages, while keeping the versionless main shrinkwrap output deterministic with 0.0.0 placeholder entries.

Changes

  • Registry metadata resolutiongenerate-coding-agent-shrinkwrap.mjs now fetches real npm registry metadata (tarball URL, integrity hash, optionalDependencies) for non-placeholder versions of @bastani/atomic-natives, respecting npm_config_registry / NPM_CONFIG_REGISTRY env overrides. Placeholder 0.0.0 versions skip the fetch and keep deterministic output for main.
  • Optional native platform packages — All six @bastani/atomic-natives-* platform packages (darwin-arm64, darwin-x64, linux-arm64-gnu, linux-x64-gnu, win32-arm64-msvc, win32-x64-msvc) are now generated and preserved in the shrinkwrap, including optional: true, os, cpu, and Linux libc: ["glibc"] constraints derived from the package's napi.targets.
  • Async shrinkwrap generationgenerateShrinkwrap() is now async; the script entry point uses import.meta.url detection and exports generateShrinkwrap for unit testing.
  • Shrinkwrap updatedpackages/coding-agent/npm-shrinkwrap.json now includes optionalDependencies on the @bastani/atomic-natives entry and all six platform package entries.
  • Tests — Added test/unit/shrinkwrap-atomic-natives.test.ts to assert that the checked-in shrinkwrap contains all required optional native platform package entries with correct version pins, os/cpu/libc constraints, and resolved tarball URLs.
  • Changelog — Added entry under ## [Unreleased] > ### Fixed in packages/coding-agent/CHANGELOG.md.

Validation

  • bun run check:shrinkwrap
  • NPM_CONFIG_REGISTRY=https://example.invalid/ bun run check:shrinkwrap
  • npm_config_registry=https://example.invalid/ bun run check:shrinkwrap
  • bun test test/unit/shrinkwrap-atomic-natives.test.ts
  • bun run typecheck
  • bun run check:file-length
  • git diff --check origin/main
  • Commit and push hooks passed, including bun run test:unit
  • Temporary 0.9.4-alpha.1 published-version simulation verified registry-resolved native platform entries include integrity, optional flag, os/cpu, and Linux libc constraints

Notes

This intentionally does not include the temporary recovery-only publish.yml checkout pinning used to complete 0.9.4-alpha.1.

Resolve published @bastani/atomic-natives metadata when generating release shrinkwraps, while keeping versionless main output deterministic.

Preserve generated native optional package constraints, including optional flags and Linux glibc libc guards, and cover the checked-in shrinkwrap shape with a focused regression test.

Assistant-model: GPT-5.5
@claude claude Bot changed the title fix(release): resolve native shrinkwrap metadata fix(release): resolve native platform entries and registry metadata in shrinkwrap Jun 30, 2026
@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown

Code Review — fix(release): resolve native shrinkwrap metadata

Reviewed the generator changes, the regenerated shrinkwrap, the new test, and the changelog entry. The core design is sound: the placeholder (0.0.0) path stays fully offline/deterministic while real release versions resolve published registry metadata, and assertRegistryMetadata is a nice guardrail. The napi-target → platform mapping is well centralized. A few things worth addressing before merge.

1. Test coverage only exercises the placeholder path (medium)

test/unit/shrinkwrap-atomic-natives.test.ts reads the checked-in shrinkwrap and asserts on the 0.0.0 placeholder entries. That's a useful snapshot, but the genuinely new and complex logic in this PR — fetchRegistryPackageVersion, copyRegistryPackageEntry, the metadata→entry mapping, and the libc override — has no automated coverage; it was only validated manually via the 0.9.4-alpha.1 simulation. Since this code only runs at release time, a regression here surfaces during a publish rather than in CI.

Consider making buildInternalPackageEntries testable with an injectable fetch (or factoring the metadata→entry transform into a pure function) so the registry branch can be unit-tested against a fixture packument — including the integrity/os/cpu/libc assertions. You already export { generateShrinkwrap }, but nothing imports it yet; wiring a real test to it would close the loop.

2. No timeout/retry on the registry fetch (low–medium)

fetchRegistryPackageVersion issues a bare fetch with no timeout or retry. At release time a transient registry hiccup (or slow DNS) will hard-fail generation and abort the publish. Given this is on the release critical path, a small bounded retry + AbortSignal.timeout(...) would make it more robust. Note this is nicely consistent with the versionless main, where the 0.0.0 short-circuit means hooks/CI never touch the network — good call.

3. Optional-dep versions are assumed to be exact (low / latent)

In the loop, optionalVersion = mainEntry.optionalDependencies[packageName] is passed straight into fetchRegistryPackageVersion, which does an exact packument.versions[version] lookup. This works today because napi pins the platform packages to exact versions, but if a range (^0.9.4, *) ever lands in the published optionalDependencies, the lookup returns undefined and throws the misleading "metadata is not available" error. A short comment documenting the exact-pin assumption (or a clearer error when the value isn't an exact semver) would save a future debugging session.

4. Local descriptor overrides registry-published os/cpu/libc (low)

On the registry path, optionalEntry starts from copyRegistryPackageEntry(optionalMetadata) but then unconditionally overwrites os/cpu/libc from the local napi descriptor. If a published platform package's constraints ever diverged from the local mapping, the shrinkwrap would silently disagree with what's actually on the registry. This is probably intentional (local mapping as source of truth), but a one-line comment stating that intent would prevent it from looking like a bug.

Minor

  • registryMetadataUrl uses encodeURIComponent, which encodes @%40 and /%2F. npmjs handles this and you verified against the real registry, so fine — just noting some stricter mirrors prefer the @scope%2fname form (leaving @ literal).
  • package.json still invokes these scripts via node (not bun, per CLAUDE.md). Not introduced by this PR, but the new fetch + import.meta.url/pathToFileURL guard now require Node ≥18 — worth keeping in mind.
  • Changelog entry is appropriately detailed and correctly placed under [Unreleased] → Fixed. 👍

Nothing here is blocking correctness-wise given the manual alpha.1 validation, but #1 (test coverage of the registry branch) is the one I'd most like to see before this becomes load-bearing for every future release.

@lavaman131
lavaman131 merged commit 90fd8f3 into main Jun 30, 2026
10 checks passed
@lavaman131
lavaman131 deleted the fix/native-shrinkwrap-metadata branch June 30, 2026 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants