Skip to content

fix(bitgo): ship npm-shrinkwrap.json for npm consumers#9213

Open
mihir-bitgo wants to merge 2 commits into
masterfrom
mihirseth/wcn-604-bitgo-shrinkwrap
Open

fix(bitgo): ship npm-shrinkwrap.json for npm consumers#9213
mihir-bitgo wants to merge 2 commits into
masterfrom
mihirseth/wcn-604-bitgo-shrinkwrap

Conversation

@mihir-bitgo

Copy link
Copy Markdown

Summary

  • overrides (fix(root): add npm overrides mirroring yarn resolutions #9212) only pins versions when bitgo is the root of an npm install; consumers installing bitgo as a dependency still resolve unpinned transitive versions
  • Add a prepack script on modules/bitgo (scripts/generate-bitgo-shrinkwrap.ts) that generates npm-shrinkwrap.json in an isolated temp copy (outside the workspace, since npm shrinkwrap isn't workspace-aware and modules/bitgo/.npmrc disables package-lock) and ships it in the published tarball
  • Using prepack instead of a separate release-pipeline step means it fires after lerna bumps the version on disk but before packing, so the shrinkwrap's top-level version always matches what's actually published
  • Add a post-publish workflow step that fails the release if the registry's hasShrinkwrap metadata isn't true — this was never confirmed on a real npm publish per the ticket (Verdaccio didn't set it)

Test plan

  • Both files (.github/workflows/npmjs-release.yml, modules/bitgo/package.json) parse/lint cleanly
  • Canary publish required before merge: publish bitgo@0.0.0-canary.N from a temp copy including a generated shrinkwrap and confirm npm view bitgo@<canary> dist.hasShrinkwrap is true
  • Fresh-install the canary as a dependency in a throwaway project and confirm pinned versions land / npm audit is clean
  • Dry-run the release workflow to confirm the new verification step doesn't false-fail

Depends on #9212 (root overrides) merging first so the generated shrinkwrap reflects the pinned tree.

TICKET: WCN-604

🤖 Generated with Claude Code

@mihir-bitgo mihir-bitgo requested review from a team as code owners July 8, 2026 19:18
@linear-code

linear-code Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

WCN-604

@mihir-bitgo mihir-bitgo force-pushed the mihirseth/wcn-604-bitgo-shrinkwrap branch from 4d56e22 to 790cf76 Compare July 8, 2026 20:34

Copy link
Copy Markdown
Contributor

Review

Overview

Adds a prepack script to modules/bitgo that generates npm-shrinkwrap.json in an isolated temp copy (stripping devDependencies/scripts, injecting root overrides, resolving against the real registry), ships it via the files array, and adds a post-publish workflow step that fails the release if npm's dist.hasShrinkwrap isn't true. The mechanism is sound — a dependency's npm-shrinkwrap.json is honored by npm to pin that dependency's own subtree, so this is the right lever for the goal.

🔴 Critical — sibling version resolution at pack time

modules/bitgo depends on 88 @bitgo/* siblings, and the shrinkwrap is generated by running npm install --package-lock-only against the public npm registry. prepack fires inside yarn lerna publish, which bumps versions and packs. At the moment bitgo is packed, the newly-bumped sibling versions are almost certainly not yet published/propagated on registry.npmjs.org. Two failure modes, both bad:

  1. npm install can't resolve @bitgo/sdk-core@<new-version> → pack fails → every release blocked.
  2. It resolves the previous published versions → the shipped shrinkwrap pins stale sibling versions, arguably worse than no shrinkwrap.

The canary plan won't surface this — a standalone bitgo@0.0.0-canary.N publishes against an already-published sibling tree, hiding the exact ordering hazard of a coordinated monorepo release. Recommend validating against a real/dry-run lerna release, and consider resolving siblings from the local workspace tarballs rather than the registry.

🟠 Significant

  • prepack now requires network + a full npm install on every pack, including local/dev/offline packing (slow 88+ dep resolution; hard failure offline). Consider gating on a CI/release env var so local packs aren't broken.
  • hasShrinkwrap verification may permanently false-fail. The PR body admits dist.hasShrinkwrap was never confirmed on a real npm publish; if npm doesn't populate it, this step fails every release. Querying the registry immediately post-publish is also subject to metadata propagation lag — add retry/backoff (the workflow already uses that pattern elsewhere). Confirm the field is real before merge.
  • Undeclared dependency on root-hoisted tsx/execa. Both are root devDependencies, not declared in modules/bitgo. Fine within the hoisted workspace publish, but the script silently relies on hoisting.

🟡 Minor / style

  • const { devDependencies, scripts, ...isolatedPackageJson } — confirm eslint no-unused-vars has ignoreRestSiblings: true, else this lint-fails.
  • Script otherwise matches existing scripts/*.ts conventions (execa default import, main().catch(...), process.exit(1)). 👍
  • if (!rootPackageJson.overrides) fail-fast is good; correctly depends on fix(root): add npm overrides mirroring yarn resolutions #9212.
  • Consider logging which versions got pinned for release auditability, given the stale-pin risk.

Test coverage

No automated test (reasonable for release tooling), but given the sibling-timing risk, the unchecked canary/dry-run boxes should be treated as merge blockers.

Bottom line

Right mechanism, but the pack-time sibling resolution against the public registry is a real correctness/reliability hazard for a coordinated monorepo release, and the hasShrinkwrap gate could block releases if that metadata field isn't actually populated. Validate both on a real (dry-run) release before merge — the standalone canary alone is insufficient.


Generated by Claude Code

@OttoAllmendinger OttoAllmendinger left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment

npm `overrides` only pins versions when bitgo is the root of an
`npm install`; consumers installing bitgo as a dependency still resolve
unpinned transitive versions. Ship npm-shrinkwrap.json inside the
published package so those installs are pinned too.

Wired as bitgo's `prepack` script (not a separate release-pipeline step)
so it always runs after lerna bumps the version on disk but before the
tarball is packed, keeping the shrinkwrap's top-level version in sync
with whatever actually gets published. Generation happens in an isolated
temp copy since `npm shrinkwrap` isn't workspace-aware and
modules/bitgo/.npmrc disables package-lock.

`@bitgo/*` siblings are excluded from resolution: `lerna publish` bumps
and packs every package in the same operation, so a sibling's new
version isn't guaranteed to be live on the registry yet when bitgo is
packed — resolving it here would fail the release or pin a stale
version. Consumers resolve `@bitgo/*` siblings normally at install time
instead; the shrinkwrap still pins whatever those siblings pull in
transitively.

Generation only runs when BITGO_GENERATE_SHRINKWRAP=true (set by the
release workflow), so a plain local `npm pack` doesn't force a network
install of the full dependency tree.

TICKET: WCN-604
npm only honors a shipped npm-shrinkwrap.json for consumers when the
registry's own metadata reports hasShrinkwrap: true — this was never
confirmed against a real npm publish (only Verdaccio, which didn't set
it). Add a post-publish check so a silent registry-side omission fails
the release instead of shipping a bitgo package whose shrinkwrap
consumers silently don't get.

Set BITGO_GENERATE_SHRINKWRAP=true on the publish steps so bitgo's
prepack script actually runs during release. Retry the registry check
with the job's existing `retry` binary since dist metadata can lag
briefly right after publish.

TICKET: WCN-604
@mihir-bitgo mihir-bitgo force-pushed the mihirseth/wcn-604-bitgo-shrinkwrap branch from 790cf76 to 23ec271 Compare July 9, 2026 15:50
@mihir-bitgo

Copy link
Copy Markdown
Author

Thanks for the review — the critical finding is correct and I hadn't accounted for the sibling-publish race. Fixed:

  • Critical (sibling resolution race): @bitgo/* deps are now excluded from the temp package.json before resolution, so npm install never touches the public registry for a just-bumped-but-not-yet-published sibling. Consumers resolve @bitgo/* siblings normally at install time (by then they're published); everything transitively pulled in beneath a sibling still gets pinned via overrides. The shrinkwrap's own dependencies field is patched back to the real sibling list after generation so what's shipped still matches package.json.
  • prepack forcing network installs on local/offline packs: generation is now gated behind BITGO_GENERATE_SHRINKWRAP=true, set only on the release workflow's publish steps.
  • hasShrinkwrap propagation lag: wrapped the registry check in the job's existing retry binary (--up-to 5x --every 5s) instead of a single curl.
  • Lint: dropped the unused-var destructure (no ignoreRestSiblings in this repo's eslint config) in favor of explicit delete.
  • Undeclared tsx/execa — left as-is, intentionally relying on root hoisting like other scripts/*.ts in this repo call out.

Agreed the canary-only validation was insufficient for the sibling-timing hazard specifically — that risk is now structurally eliminated rather than something to catch in testing. Canary/dry-run validation of the hasShrinkwrap metadata assumption is still outstanding before merge.

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @mihir-bitgo's task —— View job


Code Review Complete

  • Read and analyze the changed files
  • Review the generate-bitgo-shrinkwrap.ts script implementation
  • Assess workflow changes and package.json modifications
  • Evaluate if Otto's concerns about sibling resolution race have been addressed
  • Provide comprehensive code review feedback

Review Summary

This PR effectively addresses the critical sibling resolution race condition identified by Otto while implementing a sound mechanism for shipping npm-shrinkwrap.json to npm consumers.

✅ Key Strengths

Sibling Resolution Race Fix

The implementation correctly addresses Otto's critical concern by:

  • Excluding @bitgo/* siblings from temp package.json before resolution (scripts/generate-bitgo-shrinkwrap.ts:59-61)
  • Restoring real dependencies in the final shrinkwrap (scripts/generate-bitgo-shrinkwrap.ts:78-81)
  • This prevents resolution failures or stale version pinning during coordinated monorepo releases

Environment Gating

  • BITGO_GENERATE_SHRINKWRAP=true guard (scripts/generate-bitgo-shrinkwrap.ts:34-37) prevents local/offline pack operations from requiring network access
  • Only runs during CI release workflow (npmjs-release.yml:296)

Registry Verification

  • Retry-wrapped hasShrinkwrap check (npmjs-release.yml:344-351) handles metadata propagation lag
  • Fails release if npm doesn't set dist.hasShrinkwrap: true, ensuring consumers actually get the shrinkwrap

Clean Implementation

  • Proper temp directory cleanup (scripts/generate-bitgo-shrinkwrap.ts:86)
  • Explicit error handling with process.exit(1) (scripts/generate-bitgo-shrinkwrap.ts:90-93)
  • Dependency injection of root overrides (scripts/generate-bitgo-shrinkwrap.ts:62)

🟡 Minor Observations

Code Style

  • Direct property deletion (scripts/generate-bitgo-shrinkwrap.ts:57-58) avoids unused variable lint issues - good choice given this repo's eslint config
  • Consistent logging with existing scripts/*.ts patterns

Dependencies

  • Implicit dependency on root-hoisted tsx/execa - acceptable per repo conventions, though worth noting for future maintainers

Package.json Changes

  • files array inclusion (modules/bitgo/package.json:186) correctly ships the generated shrinkwrap
  • prepack script timing is optimal - runs after version bump but before tarball creation

🔍 Edge Cases Handled

  1. Missing overrides block - Fails fast with clear error (scripts/generate-bitgo-shrinkwrap.ts:42-44)
  2. Failed shrinkwrap generation - Validates file existence (scripts/generate-bitgo-shrinkwrap.ts:70-72)
  3. Registry propagation lag - Retries metadata check up to 5x (npmjs-release.yml:344)
  4. Package structure variations - Handles both flat and nested dependency structures (scripts/generate-bitgo-shrinkwrap.ts:79-81)

Recommendation

✅ Approve - The implementation correctly solves the stated problem while addressing all critical concerns from the initial review. The sibling resolution race condition is structurally eliminated, network access is properly gated, and the registry verification includes appropriate retry logic.

The canary validation mentioned in the PR description remains the only outstanding item, but the code changes themselves are sound and ready for merge pending that validation.


Job run: https://github.com/BitGo/BitGoJS/actions/runs/...
Branch: mihirseth/wcn-604-bitgo-shrinkwrap

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