fix(docker): install harper from the packed tarball's own shrinkwrap - #2042
Open
kriszyp wants to merge 10 commits into
Open
fix(docker): install harper from the packed tarball's own shrinkwrap#2042kriszyp wants to merge 10 commits into
kriszyp wants to merge 10 commits into
Conversation
npm install --global harper-*.tgz has no registry packument, so npm never learns the tarball has a bundled npm-shrinkwrap.json and re-resolves every dependency fresh against package.json's ranges instead of the pinned tree. Registry installs are unaffected; only the image build was. Extract the tarball into a normal project directory and run npm install --omit=dev there instead, so npm reads npm-shrinkwrap.json off disk like it would for any checked-out project. Since that's a local (non-global) install, replicate npm's own global-install layout by hand: extract into lib/node_modules/harper and symlink dist/bin/harper.js into the global bin dir. Extend docker-smoke.yml with a `which harper` check and a second boot pass under HARPER_RUNTIME=bun, since the existing smoke test only covered the default runtime and this change touches both paths. Refs #1960
…nore glob Address pre-push review findings on the shrinkwrap-honoring change: - Fail the build loudly if npm-shrinkwrap.json is missing from the packed tarball, instead of silently falling back to a fresh resolve. - Add a CI step that compares an installed dependency's version against the shrinkwrap's pin, so a regression back to npm install --global (or a build that drops the shrinkwrap from the tarball) fails CI instead of staying green. - Give both boot-check curl loops a --max-time so a hung response can't eat the whole job timeout instead of failing fast. - Run the HARPER_RUNTIME=bun boot leg even if the default-runtime leg failed, so a bun-only regression isn't masked by an unrelated flake. - Fix .dockerignore's /harperfast-harper-*.tgz pattern, which never matched this package's actual harper-*.tgz tarball name. - Update DESIGN.md's packaging note, which still described the pre-fix behavior.
…en copy npm rewrites npm-shrinkwrap.json in place to match whatever it actually installs (same as package-lock.json), so the smoke test's comparison against the live file in the image was comparing npm's output to itself -- verified locally that it stays green even when npm silently re-resolves a version upward. Freeze the packed pins to npm-shrinkwrap.packed.json before npm install runs, and compare against that instead. Widen the check to more than one dependency via a small script (build-tools/check-shrinkwrap-pins.mjs) rather than a single package that happens to sit at the floor of its own range. Also: trigger docker-smoke.yml on package.json and build-tools/** changes (both feed the packed shrinkwrap, and neither was in the path filter), and raise the job timeout to fit two boot-check loops that can each take several minutes in the worst case.
…docs
- Scope the bun boot leg's if: to require the build actually
succeeded, not just success()||failure() on the whole job so far
-- a failed build previously still let the bun leg run and fail
on an unrelated "image doesn't exist locally" error.
- Report actual elapsed time (${SECONDS}) in the boot-check
messages instead of a hardcoded "60s"/loop-iteration count that
understated the real worst-case wait by up to 6x.
- Tried a whole-tree shrinkwrap comparison (walk every packed
dependency instead of two named canaries) to address the
decays-over-time concern on canary checks; reverted it after
testing against a real build -- it's permanently red today
because the open react-native-fs gap drags shared transitive
deps (e.g. @babel/*) to newer versions than the shrinkwrap pins
for them. Left a comment explaining why, to revisit once that
gap closes.
- Document two things a reviewer surfaced: the react-native-fs
subtree that npm re-adds has no pinned version and no lockfile
integrity hash (dependencies.md), and pinning the image means an
in-range dependency fix now needs a lock bump and re-release,
not just a rebuild (DESIGN.md).
…-edge drift Round-4 review found two majors: `npm install --omit=dev` still resolves devDependency edges to compute the ideal tree even though it won't install them, which can (a) silently lift a *production* package above its shrinkwrap pin when a devDependency also wants it at a looser range (verified with harper's own tree: `ws` and `mqtt-packet` are both pinned production deps `mqtt`'s devDependency range also covers), and (b) forces a network fetch of every dev package's manifest during the image build, including one pinned to a GitHub tarball URL that never ends up installed. Strip devDependencies from the Dockerfile's own extracted copy of package.json before installing -- not the published tarball, which registry consumers never see, so this doesn't touch what they receive. Verified both effects are gone: no phantom devDependency directories, no GitHub fetch, and the remaining whole-tree mismatch count dropped, isolating what's left to the already-documented react-native gap.
…docs Address round-5 review findings: - check-shrinkwrap-pins.mjs now verifies its own canaries still discriminate (npm view against the registry) before trusting a pass -- a canary whose pin catches up to registry-latest would otherwise let a reverted, unpinned Dockerfile pass silently. - Correct the header comment's characterization of the whole-tree check's remaining mismatches: @endo/static-module-record is a real production dependency (used by the SES sandbox, security/jsLoader.ts) that shares @babel/parser/traverse/types with the react-native chain, so that drift is live risk in a security-relevant parser, not harmless dead-code collateral -- another concrete reason the react-native gap needs closing, not just a reason the check is noisy. - Add package-lock.json to docker-smoke.yml's path filters -- build-tools/build.sh derives the packed shrinkwrap from it, so it's the actual source of the image's pinned tree and the file a lock-bump PR touches most. - Qualify DESIGN.md's "frozen to the shrinkwrap" claim: true for the pinned tree, not for the still-unpinned react-native residual documented two bullets up.
…ute latest npm view <dep> version returns the registry's bare "latest" dist-tag, which can be a newer major a dependency's declared range excludes -- a broken/reverted install could never resolve to it either, so comparing against it could report a canary as "still discriminates" when it's actually gone vacuous within the range that matters. Compare against the max version satisfying the dependency's declared range instead (npm view <dep>@<range> version --json, last entry -- that flag returns every satisfying version in ascending order when more than one matches, not just the max). Verified both directions against a real build: passes when the pins lag their range max (today's state), and correctly fails when both canaries are rigged to their range-max-satisfying version.
…ing array order npm view <dep>@<range> version --json returns every matching version, but the order isn't a documented contract -- it can reflect publish/insertion order rather than semver order (e.g. a backported patch published after a newer minor), so taking the last entry could silently pick the wrong "range max" reference. Compare all returned versions with a small numeric major.minor.patch comparator instead. Verified against a real build in both directions again after the change.
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the Dockerfile to extract the local tarball and run npm install directly in the project directory, ensuring that npm-shrinkwrap.json is honored for version pinning. It also strips devDependencies before installation to prevent unexpected dependency hoisting. Additionally, a new verification script check-shrinkwrap-pins.mjs is introduced to assert that the installed dependencies match the packed shrinkwrap pins. The reviewer suggested strictly validating that the packed shrinkwrap uses lockfileVersion: 3 in the verification script to prevent silent failures if the lockfile format changes.
Contributor
|
Reviewed; no blockers found. |
Gemini's PR review flagged that the script assumes lockfileVersion 3's "packages" map layout without checking it -- a future lockfile format change would fail confusingly (or silently check nothing) instead of failing clearly. Fail loudly with an explicit message if it's not 3.
kriszyp
marked this pull request as ready for review
August 1, 2026 02:53
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.
Offered for @heskew since #1960 is assigned to them — Kris asked for this to be dispatched anyway. Please take/edit/close as you see fit.
What
The Docker image installs Harper from a locally-built tarball with
npm install --ignore-scripts --global harper-*.tgz. npm only honors a package's bundlednpm-shrinkwrap.jsonwhen the registry packument says the package has one (_hasShrinkwrap) — metadata set at publish time. A local tarball has no packument, so npm never learns the shrinkwrap exists and resolves the whole dependency tree fresh againstpackage.json's ranges instead of the pinned tree. Registry installs (npm install harper) are unaffected; only the image build was.This extracts the tarball into a normal npm project directory and runs
npm install --omit=dev --ignore-scriptsthere instead, so npm readsnpm-shrinkwrap.jsonstraight off disk the same way it would for any checked-out project. Since that's a local (non-global) install, it doesn't create the usual global bin symlink, so the change replicates npm's own global-install layout by hand: extract into$NPM_CONFIG_PREFIX/lib/node_modules/harper, then symlinkdist/bin/harper.jsinto$NPM_CONFIG_PREFIX/bin/harper.docker-entrypoint.sh'swhich harper/bun "$(which harper)"paths only depend onPATH, so this is transparent to it. Also addednpm cache clean --forceafter install — neither Dockerfile variant cleaned the npm download cache before, and left uncleaned it made the fixed image bigger than the current one (see Verification).Also strips
devDependenciesfrom the Dockerfile's own extracted copy ofpackage.jsonbefore installing — not the published tarball, registry consumers never see this. Pre-push review (round 4) found thatnpm install --omit=devstill resolves dev edges to compute the ideal tree even though it won't install them, which turned out to be a real, not hypothetical, correctness gap: a production package that a devDependency also happens to want (verified with harper's own tree —mqttis a devDependency whosews/mqtt-packetranges also cover the pinned production copies of those same packages) could silently resolve above its shrinkwrap pin the next time either range moves — the exact class of bug this whole PR exists to close, just reachable through a side door. It also forced an unnecessary network fetch of every dev package's manifest during the image build, including one (uWebSockets.js) pinned to a raw GitHub tarball URL that never ends up installed. Verified both are fixed (see Verification).Also extends
.github/workflows/docker-smoke.yml, which is the CI guard for exactly this class of change, with:which harpercheck,HARPER_RUNTIME=bun(runs even if the default-runtime leg fails, so one doesn't mask the other),build-tools/check-shrinkwrap-pins.mjscompares two installed dependency versions against the shrinkwrap's pins inside the built image. The comparison is againstnpm-shrinkwrap.packed.json, a copy the Dockerfile freezes beforenpm installruns — not the livenpm-shrinkwrap.json, because npm rewrites that file in place to match whatever it actually installs, which would make a post-install comparison vacuous (confirmed this by testing it — round 1 of pre-push review shipped exactly that bug, round 2 caught and fixed it; see Verification below).curl --max-timeon both boot-check loops and atest -f npm-shrinkwrap.jsonfail-fast guard, so a hang or a packaging regression fails loud instead of silent.The existing smoke test only exercised the default runtime and never actually checked that a version was pinned.
Also fixes
.dockerignore's/harperfast-harper-*.tgzpattern (stale from a scoped-name era; this package isharper, so it never matched anything) and updatesDESIGN.md's packaging note, which still described the pre-fix behavior.Why now
rocksdb-js 2.6.0/2.6.1 (published 2026-07-31) redefined the
compressionstore option in a way that throws on the value Harper passes, breaking every table open. harpermainpins^2.5.0, which is satisfied by 2.6.1. The lockfile/shrinkwrap pin the working 2.5.0, so CI and registry installs are protected — but the image's fresh-resolve-at-build-time install would have shipped 2.6.1: a green build producing a completely broken container. See #2036 / #2037 for that thread. This PR doesn't touch that fix directly — it closes the reproducibility gap that turned a caret-range regression into a shipped image regression. I reproduced this concretely (see Verification): the current Dockerfile resolves rocksdb-js to 2.6.1 today; this PR's Dockerfile resolves it to the pinned 2.5.0.Open questions from #1960
npm install --global <tarball>re-resolves fresh (reproducing the bug), and thatnpm install --global <local-directory>is actually a link install — it symlinks the package in without installing any of its dependencies at all, which is worse. Extracting and running a plainnpm installinside the extracted directory was the only one of the three that honored the shrinkwrap for version pinning.npm civiability (strippingdevDependenciesfrom the packedpackage.json). Not doing this for the react-native exclusion — see "Risks & open questions" below and Docker image still ships the react-native-fs subtree unpinned (residual from #1960/#2042) #2043, the follow-up issue. (This PR does stripdevDependenciesfrom the Docker image's own ephemeral extracted copy, for an unrelated and unconditionally-safe reason — see "What" above.)$NPM_CONFIG_PREFIX/bin, matching npm's own global-install convention — see "What" above.Verification
Built both the current
mainDockerfile ("before") and this PR's Dockerfile ("after") locally (no registry push) and compared:main)@harperfast/rocksdb-jsresolved^2.5.0, breaks table opens)fastifyresolvedwhich harperHARPER_RUNTIME=bun)dockerisn't available as a CLI in the sandbox this ran in, but the daemon socket was reachable; I downloaded staticdocker/buildxbinaries (no root) to drive real builds against it rather than asserting the result.Independent review: codex + gemini + grok (experimental) + harper-domain, run 7 times as I fixed what each round found (round 5 was a full re-review after a major behavioral fix; the rest were delta rounds against the growing diff). Two rounds caught defects in my own review-driven fixes before they shipped: round 2 caught that my round-1 CI fix compared npm's post-install output to itself (vacuous); round 6 caught that my round-5 self-verifying canary compared against the wrong reference value (absolute latest instead of range-max). Both fixed and reverified against a real build before the next round.
Risks & open questions
npm-shrinkwrap.jsonof thealasql → react-native-fs → react-nativesubtree (~200 packages,react-native/metro/hermes/etc.), but the packedpackage.jsonis untouched —alasql's own manifest inside the tarball still declaresreact-native-fsas anoptionalDependency. Plainnpm install(unlike a registry install of harper as someone else's dependency, where the shrinkwrap is fully authoritative andpackage.jsonisn't consulted) reconciles the local project's lockfile against its ownpackage.json, so it silently re-adds the pruned subtree to satisfy that edge. I confirmed this three ways:npm ci(after also strippingdevDependenciesto pass its root sync check) refuses outright:Missing: react-native-fs@2.20.0 from lock fileplus ~30 more entries — i.e. ci is strict enough to not re-add it, but strict enough that it won't proceed either.npm install --omit=optional(tree-wide) does drop the react-native subtree, but it also drops@harperfast/rocksdb-js's per-platform native-binaryoptionalDependencies— confirmed thelinux-x64-glibcbinding package is simply absent — which would silently break every table open. Rejected.npm ciagainst a package.json wherealasql's own packed manifest also has the optional edge removed. That's a materially bigger change to the published tarball (patching a third-party dependency's manifest inside our own package) than thedevDependencies-strip Docker image dependency tree is resolved fresh at build time, not from the published shrinkwrap #1960 anticipated, so I raised it with Kris instead of deciding unilaterally — the call was to ship this PR now and track the fuller fix separately (Docker image still ships the react-native-fs subtree unpinned (residual from #1960/#2042) #2043), rather than fold a published-artifact change into an already-large PR.isReactNativeguard unreachable under Node either way, but it's unverified code in a shipped image.@endo/static-module-record— a real production dependency used by the SES sandbox (security/jsLoader.ts) — needs@babel/parser/@babel/traverse/@babel/types, the same packages the react-native chain re-adds unpinned. When npm reconciles the two, the shared copy can drift above the shrinkwrap's pin. Confirmed the mechanism, not confirmed it's live today (npm would normally keep the locked entry since ranges are satisfied) — but it means the residual gap isn't just image bloat, it's version drift risk in a parser the sandbox runs on application code. Written up in full in Docker image still ships the react-native-fs subtree unpinned (residual from #1960/#2042) #2043.npm install harperfrom the registry is untouched. Docker image still ships the react-native-fs subtree unpinned (residual from #1960/#2042) #2043's fuller fix would.build-tools/check-shrinkwrap-pins.mjschecks two named dependencies, not the whole tree — deliberately, after testing the alternative, and it now verifies its own canaries still work. A reviewer correctly pointed out a 2-canary check can decay: it only proves anything while those two pins lag the registry. I tried the more thorough fix (compare every dependency in the packed shrinkwrap) and reverted it after testing against a real build: it's permanently red today, because the still-open react-native gap drags shared production packages to newer versions than the shrinkwrap pins for them — and this isn't harmless:@endo/static-module-record(a real production dependency used by the SES sandbox,security/jsLoader.ts) needs@babel/parser/@babel/traverse/@babel/types, the same packages the react-native chain re-adds unpinned, so this is a second, independent, security-relevant reason to close that gap, not just noise in a check. The check now also verifies its own discrimination: it callsnpm view <dep>@<range> version(the max version satisfying the dependency's declared range — not the barelatestdist-tag, which could be a newer major the range excludes and a broken install could never reach either) and fails loudly if both canaries have caught up to what their range allows, instead of silently becoming a no-op. Verified both directions against a real build, including rigging both canaries to their range-max to confirm the guard fires.publish-docker.yaml(the workflow that builds and pushes the image consumers actually pull) doesn't run any of these checks. All the guards added here —which harper, the shrinkwrap-pin comparison, both boot legs — live only indocker-smoke.yml, which never runs on a release tag. A reviewer flagged this; I'm not folding a change to the release pipeline into an already-large PR without it getting its own attention, but it means a packaging regression thatdocker-smoke.yml's path filters miss (e.g. abuild-tools/build.shchange not covered by this PR's widened filters) would only be caught by thetest -f npm-shrinkwrap.jsonfail-closed guard at release-build time, after the npm package has already published. Worth a follow-up — reusingdocker-smoke.ymlas aworkflow_calljob in the publish pipeline, or runningcheck-shrinkwrap-pins.mjsagainst the built image before the push step, would close it.DESIGN.md): before this PR, rebuilding the image picked up any newer in-range dependency automatically, which is how the rocksdb-js 2.4.0→2.5.0 fix could ship as "just rebuild." After this PR, that path needs a lock bump + re-release; a rebuild alone reproduces the same pinned tree, bug included.Test coverage
docker-smoke.yml's existing "container boots" step now also runs underHARPER_RUNTIME=bun(scoped to only run if the image actually built), and there are new steps assertingwhich harperresolves and that two representative dependencies match their shrinkwrap pins — all previously unguarded.check-shrinkwrap-pins.mjsdoesn't have its own unit test — it's a 50-line CI-only script exercised end-to-end by the workflow itself.🤖 Generated with Claude Code