fix(deps): dedupe nested msgpackr instance under rocksdb-js - #2442
Merged
Conversation
package-lock.json carried a stale node_modules/@harperfast/rocksdb-js/node_modules/msgpackr entry (added by 2e65550, an unrelated REST pagination PR) even though root and rocksdb-js both pin msgpackr to the same exact 2.0.6. build-tools/check-shrinkwrap-pins.mjs correctly rejects any nested copy under rocksdb-js regardless of version, since msgpackr carries per-instance structure state and two instances sharing one database is a decode-corruption risk, not a cosmetic duplicate. Removing the redundant lock entry lets npm hoist rocksdb-js's msgpackr dependency to the root instance, matching what the exact-version pins already declare. Verified clean via `npm ci`: no nested node_modules/@harperfast/rocksdb-js/node_modules/msgpackr directory, and require.resolve('msgpackr') from rocksdb-js resolves to the same file as the root instance. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
kriszyp
marked this pull request as ready for review
September 1, 2026 21:24
github-actions
Bot
requested review from
Ethan-Arrowood,
cb1kenobi and
heskew
September 1, 2026 21:25
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
mainis red on Docker Image Smoke Test (bothlinux/amd64andlinux/arm64) becausebuild-tools/check-shrinkwrap-pins.mjsrejects the install: rocksdb-js loads a nestedmsgpackr@2.0.6instead of sharing the root one.package-lock.jsoncarried anode_modules/@harperfast/rocksdb-js/node_modules/msgpackrentry even though rootmsgpackrand rocksdb-js's own
msgpackrdependency are pinned to the identical exact2.0.6(
package.json:227,package-lock.json:2504pre-fix) — a pure hoisting artifact, not a version conflict. It was added incidentally by
2e65550 (#2147), an unrelated REST-pagination PR that happened to bump
@harperfast/rocksdb-jsto 2.8.0; that commit's lockfile diff only ever adds the nestedentry, nothing else. The nested copy is exactly what
verifyRocksDbDependencyAlignmentinbuild-tools/check-shrinkwrap-pins.mjsexists to catch: msgpackr carries per-instance structure state, so two module instances
sharing one database is a decode-corruption risk class, not a cosmetic duplicate — the check
is correct and is unchanged by this PR.
Fix is a single 9-line lockfile deletion —
the exact inverse of the culprit commit's diff — removing the nested entry so npm hoists
rocksdb-js's
msgpackrdependency to the single root instance, matching what the exact pinsalready declare.
Verification
rm -rf node_modules && npm cifrom the fixed lockfile: nonode_modules/@harperfast/rocksdb-js/node_modules/msgpackrdirectory, andnpm cidoes notreintroduce it (proves the fix is self-consistent under npm's own resolver, not just a hand
edit of the lock text).
require.resolve('msgpackr')from the root and viarequire.resolve('msgpackr', { paths: [require.resolve('@harperfast/rocksdb-js')] })return the identical file path.dockerbinary available in thisenvironment):
npm shrinkwrap→build-tools/prune-shrinkwrap-dev.mjs→build-tools/prune-shrinkwrap-react-native.mjs→ extracted into a scratch dir →npm install --omit=dev --ignore-scripts --no-audit --no-fund(mirrors the Dockerfile'sglobal-install step) → ran
build-tools/check-shrinkwrap-pins.mjsdirectly against it —this is the exact check that is red in CI and it now exits 0:
shrinkwrap pin matches (exact manifest spec): @harperfast/rocksdb-js@2.8.0 matches the packed pin.npm run build(tsc) — clean.npx mocha unitTests/build-tools/checkShrinkwrapPins.test.mjs— 15/15 passing.package-lock.json | 9 deletions(-)againstmain— no other filestouched.
For the human reviewer
Independent pre-push review (codex + Harper domain adjudication; Gemini/Cursor legs did not
produce coverage this round — see receipt) came back LGTM with one minor, non-blocking
durability observation, deliberately left out of this PR as out of the stated task scope
(explicitly asked to keep the diff to
package-lock.jsononly):msgpackrand rocksdb-js's ownmsgpackrspechappen to be pinned to the identical exact version.
check-shrinkwrap-pins.mjsalreadyre-verifies both that equality and the no-nested-copy invariant on every push/PR that
touches
package.jsonorpackage-lock.json— which is exactly the mechanism that caughtthis regression — so there is live enforcement today. A stronger, more mechanical guarantee
(e.g. a scoped
overridesentry pinning rocksdb-js'smsgpackrto$msgpackr) would removethe dependency on the two pins being kept in sync by convention, at the cost of an
overridesfield inpackage.json. Leaving that decision to a maintainer rather thanexpanding this PR.
node_modules/lmdb/node_modules/msgpackr@1.12.1remains nested (lmdb's^1.11.2rangecan't accept
2.0.6, and Harper'sRecordEncoder/StructonEncoder— the actualrecord-encoding hot path, both LMDB and RocksDB — is built from the root msgpackr
regardless of engine, so this pre-existing duplicate is unrelated to the invariant the check
protects). No action taken; flagging so it isn't mistaken for a miss.
Framing-Verdict: not applicable — mechanical lockfile-only fix restoring an existing,
already-declared invariant (no new API surface, no cross-layer boundary, no new shared
behavior); harper-engineering-guidelines step 6 planning gate does not trigger.
Review-Coverage: authored=claude; ran=codex; adjudicated=domain; blocked=gemini(auth),cursor-composer(failed); declined=cursor-grok; rounds=1 @ 7ff3f79
Human-Review-Need: 3 (decisions: enforce-via-overrides-vs-lockfile-only, leave-lmdb-nested-copy, no-verification-added) @ 7ff3f79