fix: detect shrinkwrap siblings by workspace, not @bitgo/ prefix - #9422
Merged
Conversation
Contributor
davidkaplanbitgo
approved these changes
Aug 5, 2026
zahin-mohammad
approved these changes
Aug 5, 2026
zahin-mohammad
approved these changes
Aug 5, 2026
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.
Ticket: WCI-1818
Summary
Phase 1 of WCN-1818: fixes two latent bugs in
scripts/generate-bitgo-shrinkwrap.ts(sibling detection breaks on alpha/beta, the resolved-entry safety check was weaker than it should be), adds a bounded retry for ordinary registry propagation lag, and adds a new standalone script (set-umbrella-publishable.ts) that phase 2 will use to implement the actual two-phase publish split. This PR does not change any workflow behavior and does not fix the underlying race condition on its own — see "What this PR does not fix" below.Background
PR #9404 (WCI-1200) fixed
npm install bitgoshipping with zero@bitgo/*siblings by makinggenerate-bitgo-shrinkwrap.tsactually resolve siblings into the shrinkwrap instead of stripping them. That PR assumedlerna publishpublishes packages in dependency-topological order, so siblings would already be live on the registry by the timebitgois packed.That assumption is false. Lerna runs every package's lifecycle hooks (including
bitgo'sprepack) before uploading any of them. The next prod release after #9404 merged failed exactly this way:Zero packages published in that run. WCN-1818 designed the real fix: split publishing into two passes — siblings first (with
bitgotemporarily held out via aprivateflag),bitgosecond, once siblings are actually live. This PR is the first of two phases implementing that design.What changed
1.
scripts/generate-bitgo-shrinkwrap.tsObject.keys(bitgoPackageJson.dependencies).filter(name => name.startsWith('@bitgo/')). Now reads everymodules/*/package.jsonname from disk (getWorkspacePackageNames()) and intersects that with bitgo's dependencies. On the real@bitgo/*scope this produces the same 89 siblings as before (no behavior change). On alpha/beta, whereprepare-release.tsrescopes everything to@bitgo-beta/*, the old check matched zero siblings — meaning the WCI-1200 safety check was silently verifying nothing on those channels. The new check is scope-agnostic because it reads the actual current names instead of assuming a fixed prefix.packages["node_modules/<name>"]key existed. Now requiresversion,resolved, andintegrityto each be present as strings. A key that exists but is missing install-relevant fields now correctly fails the check instead of passing it.npmInstallWithRetry): wrapsnpm install --package-lock-only --ignore-scriptswith up to 5 attempts / 5s backoff, but only when the failure looks likeETARGET/E404(i.e., "not found yet," not "will never exist"). This exists for phase 2: once siblings publish in their own pass immediately before bitgo's pass, there's a real possibility bitgo's resolution runs before the registry has fully propagated a just-published sibling. Any other error type is rethrown immediately with zero retries — confirmed in local testing.2.
scripts/set-umbrella-publishable.ts(new)Toggles
privateonmodules/bitgo/package.json:This is the mechanism phase 2 will use to hold
bitgoout of lerna's pack step during the siblings-only pass (lerna'sfilterPrivatePkgUpdatesfilters private packages before packing, confirmed against lerna 9.0.0's source innode_modules/lerna/dist/commands/publish/index.js). Deliberately a scripted temporary toggle rather than lerna's built-in--include-private(which would requirebitgoto be permanentlyprivate: truein the committed manifest) — three separate pipeline checks (pre-publish existence check, recovery verification, beta verification/auto-retry) enumerate non-private packages, and a permanently-privatebitgowould silently stop being covered by all three. The script itself holds no state and does not decide when to run — the workflow (phase 2) is responsible for calling the restore (true) underalways()so a failed pass 1 can't leave the manifest stuck flipped.What this fixes
set-umbrella-publishable.ts) phase 2 needs, and hardens the resolution step against a known-expected side effect of phase 2's design (propagation lag) ahead of time.What this PR does not fix
The original race condition —
bitgo'sprepacktrying to resolve siblings before they're published, because both currently happen inside one combinedlerna publish— is unchanged by this PR. Neither.github/workflows/publish.ymlnor.github/workflows/npmjs-release.ymlare touched here. If a prod release ran today with only this PR merged, it would still fail the same way as before (this is expected — see "Upcoming: Phase 2" below).How I tested this locally
All of the following were actually run, not just reasoned about. Working tree was clean before and after (temporary edits made during testing were reverted/diffed to confirm exact restoration).
1. Type-check and lint the two files:
Result: 0 errors on both. Only pre-existing
no-sync/no-consolestyle warnings, same kind/count pattern as the rest ofscripts/.2.
set-umbrella-publishable.ts— both directions, plus invalid input:3. Workspace-based sibling detection against the real repo (standalone script comparing old vs. new detection logic against the actual
modules/directory):Result: both old and new logic found the same 89 siblings on the real
@bitgo/*scope — confirms no regression.4. Simulated beta-rescoped state — reproducing the actual bug (small script simulating
prepare-release.ts's rescoping of a few sample names/deps to@bitgo-beta/*in memory, no files touched):Result: old prefix-based logic found 0/3 siblings on the simulated beta scope (the bug). New workspace-membership logic found 3/3 correctly.
5. Retry/backoff logic, mocked runner, three scenarios:
ETARGET-like text, then succeeds -> recovers on attempt 3.ETARGET-like text every time -> exhausts all 5 attempts, fails cleanly (no infinite loop).6. Real end-to-end run against the live npm registry (safe: resolution/read-only against the registry, only local temp-dir +
modules/bitgo/npm-shrinkwrap.jsonwrites, no publish):.nvmrc-required 24.13.0, surfaced asEBADENGINEfrom a transitive dependency. Notably, this triggered zero retry attempts — real-world confirmation that the retry logic correctly classifies non-transient errors and doesn't waste time on them.nvm install 24.13.0 && nvm use 24.13.0and re-running: exit code 0. Realnpm-shrinkwrap.jsongenerated. Spot-checked several sibling entries directly in the output — all had realversion/resolved/integrity:node_modules/@bitgo/*entries (89 direct siblings + their own transitive@bitgo/*deps). Deleted the generated file afterward — it's a build artifact, not meant to be committed here.7. Fault injection — proving the strengthened guard actually catches breakage:
cp modules/bitgo/package.json /tmp/bitgo-package.json.bak # edited package.json: set the @bitgo/sdk-core dependency to the nonexistent "999.999.999" BITGO_GENERATE_SHRINKWRAP=true npx tsx scripts/generate-bitgo-shrinkwrap.tsResult: exit code 1. Because
ETARGETlooks like it could be propagation lag, retried 4 times (~20s) before correctly giving up and reporting the real error:Confirms the retry logic has a bounded cost even in the worst case (a version that will never resolve) and never masks a genuine failure — it just adds a small, fixed delay before reporting accurately.
How to verify this after merging
Since this PR doesn't change when/how publishing happens, there's no new end-to-end release behavior to verify yet. What's verifiable post-merge:
publish.ymldispatch should still show:ETARGET, the log should show a few "retrying in 5000ms" lines before the final failure (proves the retry wrapper is wired in and behaving as tested)._hasShrinkwrap: trueand a clean-directorynpm install bitgoworking. That's out of scope for this PR.Upcoming: Phase 2
Not included in this PR. Tracked in WCN-1818, planned as a follow-up PR:
.github/workflows/publish.yml(alpha/beta): split the singleLerna Publishstep into:set-umbrella-publishable false->lerna publish from-package(siblings only, bitgo skipped)set-umbrella-publishable true(if: always(), so a failed pass 1 can't leave bitgo stuck private)lerna publish from-packagewithBITGO_GENERATE_SHRINKWRAP=true(bitgo only, siblings now live).github/workflows/npmjs-release.yml(prod): same split, applied to both the normal publish path and the recovery-mode path, on top of the existing (unchanged)lerna versionstep.BITGO_GENERATE_SHRINKWRAPwould need to be set on that path for the test, unlike today's beta).bitgohas_hasShrinkwrap: truewith fully-resolved entries for every sibling.npm install @bitgo-beta/bitgo@alphain a clean, empty directory — confirm siblings install andrequire()succeeds.rel/latestalready has an orphaned 52.4.2 version bump/tags from the original failed release that a normal run would skip past.scripts/verify-release.ts's recovery-retry path doesn't setBITGO_GENERATE_SHRINKWRAP, so a beta package republished via that specific path would ship without a shrinkwrap. Not exploitable today (beta never ships one anyway, and prod doesn't use that script), but worth its own follow-up ticket rather than leaving it undocumented indefinitely.rel/latestandmasterhave diverged (one unpublished version-bump commit onrel/latest, three unrelated commits onmasterit doesn't have) and need reconciling before any real release attempt — tracked as a separate, pre-release-time task, not blocking this PR or phase 2's code review.Ticket: WCN-1818