Conversation
…ing from api.github.com GHSA-pfwx-36v6-832x.test.ts and the git dependency lifecycle script tests installed real repositories from api.github.com, so any GitHub outage failed them on every PR. Both now build the tarball GitHub would return with a new githubTarball() harness helper and serve it from a local Bun.serve that the install is pointed at through GITHUB_API_URL. The integrity tests also now assert the exact sha512 of the served bytes, that a rejected tarball is not installed and leaves the lockfile alone, and count tarball requests; each case runs against both the buffered and the streaming extractor, which verify the integrity independently.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughChangesThe install tests now use deterministic local GitHub-style tarballs. A shared GitHub tarball test infrastructure
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: fixed; waiting on CI for b8e1140. Reproduced by running the previous version of each test with outbound network removed (only loopback available), which fails the same assertions as build 98240 ( Review: the unread stdout pipe is fixed in 7eaa29f. Self-review pointed out that Build 98340 (previous revision): 177 jobs passed, the only red job was a |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Updated 3:52 PM PT - Aug 15th, 2026
✅ @robobun, your commit b8e1140fe0ee068e1fa5664763059a6e3b1bbb4f passed in 🧪 To try this PR locally: bunx bun-pr 39115That installs a local version of the PR into your bun-39115 --bun |
There was a problem hiding this comment.
LGTM — the earlier stdout-pipe nit is addressed in 7eaa29f (stdout: "ignore"), and the rest is a straightforward hermeticization following the GITHUB_API_URL + local Bun.serve pattern already used elsewhere in test/cli/install/.
What was reviewed:
githubTarball()ustar layout — field offsets, checksum (blank-then-sum), 512-byte padding, and the two-block EOF marker match the spec; the root directory entry comes first as libarchive/bun require.- GHSA test coverage vs. the original — every prior assertion is preserved and tightened (exact lockfile entry match, request count,
node_modulespresence/absence), and both extractors are now exercised viaBUN_INSTALL_STREAMING_MIN_SIZE. - Lifecycle test — both spawns use
testEnv(which now carriesGITHUB_API_URL), and the six<script>.txtfilenames the fixture writes line up with the unchangedexists(...)assertions.
Extended reasoning...
Overview
Test-only PR that removes the api.github.com network dependency from two install test files: GHSA-pfwx-36v6-832x.test.ts (rewritten to serve a locally-built GitHub-shaped tarball via Bun.serve + GITHUB_API_URL) and one test in bun-install-lifecycle-scripts.test.ts (same mechanism, fixture reproduces the six-script dylan-conway/lifecycle-install-test repo). A new githubTarball(rootDir, files) helper in test/harness.ts hand-builds the ustar-gzip archive because Bun.Archive cannot emit the leading directory entry that bun reads the resolved commit from. No production code is touched.
Security risks
None introduced. The GHSA file tests security-relevant behavior (tarball integrity verification), so I compared the new assertions against the old ones case-by-case: none are weakened, and several are strengthened — the lockfile entry is now compared exactly against the sha512 of the served bytes (was a regex toContain), the reject case additionally asserts the package is not written to node_modules and the bad hash stays in the lockfile, and the cache-hit case now proves no second download via the request array. The describe.each over buffered vs. streaming extractors doubles coverage relative to the original, which only ever hit the buffered path.
Level of scrutiny
Medium. This is test infrastructure following an established repo pattern — GITHUB_API_URL with a local server is already used in isolated-install.test.ts, bun-patch.test.ts, symlink-path-traversal.test.ts, and others (grepped 8 hits). REVIEW.md explicitly calls for hermetic tests ("Never contact external network hosts"). The one novel piece is the hand-rolled ustar builder, which I checked field-by-field against the format: name@0, mode@100, uid/gid@108/116, size@124 (11-digit octal + NUL), mtime@136, checksum@148 (space-filled during summation, then 6-octal + NUL + space), typeflag@156, magic "ustar\0"@257, version "00"@263, per-entry padding to 512 with the (512 - n%512) % 512 guard for exact multiples, and a 1024-byte end marker. The 100-byte name-length guard is present.
Other factors
- I left one finding on the previous revision (
stdout: "pipe"without a reader in theinstall()helper); commit 7eaa29f changed it tostdout: "ignore", which resolves it. - Verified the second
spawnin the lifecycle test also usesenv: testEnv(line 2066), so both installs go through the local server. - The lifecycle fixture's six script names (
preinstall/install/postinstall/preprepare/prepare/postprepare) match the unchangedexists(join(..., "<script>.txt"))assertions at lines 2041–2046 and 2078–2083. - The fake owner/repo names mean a test that ever bypassed
GITHUB_API_URLwould 404 against real GitHub rather than silently pass, per the PR description. - PR description reports
bun bd testverification with network removed: 10/10 GHSA and 2/2 lifecycle passing.
… isolated-install.test.ts A key ending in "/" makes Bun.Archive write the root directory entry ahead of the files, which is the only thing the GitHub layout needs, so the hand-written ustar writer (and its claim that Bun.Archive could not do this) goes away. isolated-install.test.ts carried the same writer inline for its github dependency test; it now uses the helper too.
There was a problem hiding this comment.
The earlier stdout-pipe finding is fixed in 7eaa29f; this pass found nothing new. Because this is a full rewrite of the GHSA-pfwx-36v6-832x regression test (the guard for the tarball-integrity advisory) and b8e1140 swaps the tarball builder for Bun.Archive, a human look would still be worthwhile.
Checked: Bun.Archive writes every entry as FILETYPE_REGULAR (src/runtime/api/Archive.rs:369), so the ${rootDir}/ key is a 0-byte regular file, not a directory — but on_first_directory_name reads only the pathname and the depth-stripped empty path is skipped at src/libarchive/lib.rs:1581, so the fixture works for the resolved-commit read.
Checked: the new assertions preserve or tighten every check the old file made (exact [pkg, {}, resolved, sha512] vs. regex; node_modules absence and unchanged lockfile on rejection; request count on cache hit).
Checked: the lifecycle fixture keeps the six-script contract, the Blocked 6 postinstalls line, and the #1234567 short-commit output; github.stop() is awaited in afterAll.
Extended reasoning...
Overview
Test-only PR touching four files: a full rewrite of test/cli/install/GHSA-pfwx-36v6-832x.test.ts (5 tests → 10, now local-server-backed and covering both extractors), two cases in bun-install-lifecycle-scripts.test.ts moved from dylan-conway/lifecycle-install-test to a locally-served fixture, a new githubTarball() helper in test/harness.ts, and isolated-install.test.ts migrated from its inline ustar writer to that helper. No production code changes.
Security risks
None from the diff itself — it removes outbound network calls and touches only test code. The relevant risk is coverage: the GHSA file is the regression test for the tarball-integrity advisory, so a maintainer should confirm the rewritten cases still cover what the advisory requires. From my reading they are strictly stronger (exact sha512 comparison against the served bytes, both buffered and streaming extractors asserted via the --verbose Extract / Streamed markers, explicit node_modules absence and unchanged locked hash on rejection), but that call belongs to someone who owns the install code.
Level of scrutiny
Medium. Mechanically it follows the existing GITHUB_API_URL local-server pattern from isolated-install.test.ts / bun-patch.test.ts, uses tempDir, test.concurrent, await using for the server, and per-test BUN_INSTALL_CACHE_DIR. The parts that need a second pair of eyes are the non-obvious assumptions: the 600 KiB incompressible filler sized to LIBUS_RECV_BUFFER_LENGTH so streaming commits deterministically, and the b8e1140 switch to Bun.Archive — which writes the root entry as FILETYPE_REGULAR (not a directory) but still satisfies the extractor because on_first_directory_name only reads the pathname and the empty stripped path is then skipped. That works, but it contradicts the PR description's earlier claim that Bun.Archive couldn't be used, so it's worth a maintainer confirming they're comfortable with the fixture shape.
Other factors
My previous comment (unread stdout pipe) was addressed in 7eaa29f. CI build 98340 was green on the pre-b8e1140f state; #98705 for the final commit was still building at the time of the last timeline update. The lifecycle-test change is small and keeps every existing assertion. isolated-install.test.ts loses ~30 lines of hand-rolled tar code in favor of the shared helper, which is a net simplification.
…pen PRs use BranchPackage gets the optional name and files fields and makeSharedRepo the repoName parameter that #38816 and #38681 add (both are in #39403). Commit names its ref in full and takes an explicit parent, so a commit can land on a tag or start a branch from another one, which is what the cases in #35566 need. The tarball builder is split so that the GitHub shaped part has the signature of the githubTarball helper in #39115.
…assertions (#39737) ### Problem - This file took 23s on the windows 11 aarch64 lane (build 101560). Process creation is the cost: 139 fixture processes, a `checkout`, `add`, `commit` and `push` per branch, a repo per test. - 8 assertions were a bare exit code. No test read bun's output or bun.lock. ### Fix - `makeSharedRepo` writes all branches with one `git fast-import`. The 16-branch repo is built once in `beforeAll` for the three tests that only read it. The two that move a branch build their own. `Bun.Archive` builds the tarballs. 13 fixture processes remain. - The helpers take the shapes the open PRs for this file use: `name?`, `files?` and `repoName` (#38816 and #38681, in #39403), and a `Commit` with an explicit ref and parent (the tag cases of #35566). Checks in Notes. - Every install now checks stdout (what each package resolved to), stderr, the installed markers and the bun.lock rows. The tarball tests also check that each tarball is downloaded once. The scenarios are unchanged. - Verified: `bun bd test test/cli/install/bun-install-git-deps.test.ts`, 15 of 15 Linux runs. Windows 11 aarch64: 22s to 12s debug, 20s to 11s release (Notes). ### Background - git's dumb HTTP protocol serves a bare repo as static files. `update-server-info` writes `info/refs`, the branch tips the tests read as expected commits. - `git fast-import` writes a stream of commits, file contents inline, to any refs in one process. `from <ref>^0` parents a commit on the ref as the repo has it. - For a `github:` dependency bun reads the resolved committish off the tarball's first entry, the `<owner>-<repo>-<committish>` directory. `Bun.Archive` writes a key ending in `/` as that entry. <details><summary>Notes</summary> Open PRs that touch this file. This PR can land before or after them. What each order costs: - #39403 (the install fold, which carries #38816 and #38681) has its own version of this file: the old helpers plus `name?`, `files?` and `repoName`, five tests, a `tar` based `packTarball` and a `writeProject` identical to the one here. Merging it onto this PR conflicts in the helper region (take this PR's) and duplicates `writeProject` (delete one). Checked: this PR's helpers with the fold's five tests appended run all 12 tests. This PR's 7 pass. The fold's 5 build their fixtures (the `files` of the `file:` test, the unnamed package, the `odd@repo.git` name) and fail only on the assertions its source changes make pass, for example the lockfile name `odd@repo.git@...` instead of `unnamed-package@...`. - #35566 adds `commitOn`, `pushRef` and `installedFromBranch`, which drive a work tree that no longer exists, plus 8 tests. On this PR they become `moveBranch` or `commitTo` calls and `git tag` / `git branch` in the bare repo. Checked with a scratch test: `git tag v1 main`, a commit that only `refs/tags/v2` reaches, `refs/heads/release/2.0` started from `main`, and `v1` moved to a new commit, in 3 `fast-import` runs. `main` kept its commit, and `bun install` of `#main`, `#v2`, `#release/2.0` and `#v1` installed `main`, `main-v2`, `release-2.0` and `v1-moved`. - #39115 adds `githubTarball(rootDir, files)` to the harness, built the same way. `tarballOf` here has that signature, so whichever PR lands second deletes the local copy. Timings. The old and the new file were run alternately on the same machine. - Windows 11 aarch64, 16 vCPUs, debug build (`bun bd`): old 22.2s, 21.0s, 21.8s. New 11.9s, 11.9s, 11.6s. Same machine, release canary (`USE_SYSTEM_BUN=1`, what the CI lanes run): old 19.8s, 19.9s, 20.1s, 20.4s. New 10.4s, 10.6s, 11.2s, 11.5s. All runs passed. The 16-branch test alone went from 20.4s to 10.1s and is now the whole wall time of the file. A cold install of it makes `bun install` spawn about 60 `git` processes (one bare clone, then `clone --no-checkout` and `checkout` per package, and one `git log` per dependency edge), and the test does two of them on purpose. - Linux x64 debug+ASAN, `bun bd test`: old 4.8s, 5.0s, 5.0s. New 4.0s, 4.0s, 4.1s (the final revision: 4.1s to 4.4s on a busier machine). A test file that only imports the harness takes 2.3s on this build, so the work of the file went from about 2.7s to about 1.7s. CPU time (user+sys, children included): 8.9s to 7.8s. - Processes, counted with `git` and `tar` shims on PATH. Old: 299. The fixtures spawned 139 of them (10 `init`, 25 `checkout`, 23 `add`, 25 `commit`, 25 `push`, 7 `update-server-info`, 24 `tar`) and `bun install` 160 (54 `clone`, 106 `git -C`). New: 173 in each of 5 runs. The fixtures spawn 13 (3 `init`, 5 `fast-import`, 5 `update-server-info`) and `bun install` the same 160. Each `git push` also forked `receive-pack` and `pack-objects`, which the shims do not see. The Linux numbers therefore understate the gain, and the Linux timings understate it more, because process creation is cheap there. Assertion changes, per test. - 16 branches, directly and transitively (2 attempts): stderr inline snapshot (`[17]` tasks: one clone, 16 checkouts), stdout with the commit each of the 16 branches resolved to, the 16 installed markers as one object, the 16 bun.lock rows including pkg-a's dependencies, exit code. Before: two `not.toContain` on stderr, the markers, exit code. - Tarball URLs and `github:` (2 attempts each): the same, with `[32]` and `[16]` tasks (download and extract per package), bun.lock rows with the integrity of the served bytes (and, for GitHub, the resolved tag `scope-pkg-x-0000000`), stdout with `#0000000` for GitHub, and each tarball downloaded exactly once per attempt although 11 (GitHub: 7) of them are depended on twice. - Lockfile on a cold cache: both installs check stdout, stderr (the frozen install prints nothing), the markers and the bun.lock rows, which the frozen install must leave unchanged. Before: the first install checked the markers and the exit code. - Hoisted and isolated moved branch: the warm install checks stdout, stderr, markers and bun.lock. After `moveBranch` the test checks that `pkg-m` points at a new commit and `pkg-n` does not. The cold install checks stdout (the locked commits, not the new tip), empty stderr, markers and unchanged bun.lock. Before: `not.toContain("error:")`, markers, exit code. - `git+file://`: stderr snapshot (`[2]`), stdout with the commit, marker, bun.lock row, exit code. Before: `not.toContain`, marker, exit code. Fixture details. - Below 100 objects `fast-import` writes loose objects (`fastimport.unpackLimit`), the same layout the old `push` produced. A pack would work too, because `update-server-info` lists it in `objects/info/packs` for dumb HTTP clients. - The commits carry a fixed committer date, so the SHAs of a repo depend only on its contents. The tests still read them from `info/refs` instead of hard-coding them. - The old tarballs were `tar -czf` of a directory, so they also started with the directory entry. The bun.lock integrity is the sha512 of the tarball bytes, which the test computes from the bytes it serves. - `bun install` prints the `+` lines in name order (a package's dependencies are sorted when its package.json is parsed, `src/install/lockfile/Package.rs`). `expectInstalled` sorts its expectations the same way. - `test/expected-durations.json` is not touched. CI regenerates it. - Commits: f33a5e5 the rewrite, c0dd54e the sort (review), 635fe1f the helper shapes above (self-review). </details>
|
Closing in favor of #42800. It makes the same changes to these four files: the added and removed lines are identical. It also converts the GitHub tarball cases in |
1 participant
Problem
test/cli/install/GHSA-pfwx-36v6-832x.test.tsfailed 5/5 on build 98240 witherror: GET https://api.github.com/repos/jonschlinkert/is-number/tarball/98e8ff1 - 504; the same build failed bothgit dependencies also run preprepare, prepare, and postprepare scriptscases inbun-install-lifecycle-scripts.test.tswith the same 504 fordylan-conway/lifecycle-install-test.src/install/extract_tarball.rs:47-79andsrc/install/TarballStream.rs:1111-1182, and the prepare scripts of a GitHub dependency) needs a tarball with GitHub's layout, not GitHub itself.Fix
test/harness.ts:githubTarball(rootDir, files)builds the tarball GitHub returns for/repos/<owner>/<repo>/tarball/<ref>withBun.Archive: a<rootDir>/entry first, then the files under it. bun takes the resolved commit from the archive's first entry (src/libarchive/lib.rs:1488-1508), which is why the root directory needs its own entry; the trailing-slash key is what produces it.isolated-install.test.tscarried an inline tar writer for exactly this layout and now uses the helper (net -29 lines there).Bun.serveand pass its origin asGITHUB_API_URL, whichalloc_github_url(src/install/PackageManager/runTasks.rs:1700) uses in place ofhttps://api.github.com. This is the mechanismisolated-install.test.ts,bun-patch.test.tsandsymlink-path-traversal.test.tsalready use; the dependency specs are fake names, so a test that stopped going through the local server would fail instead of silently reaching GitHub.node_modulesand must not replace the locked hash, and the tarball request count proves the cache-hit case downloads nothing.BUN_INSTALL_STREAMING_MIN_SIZE=1that makes the streaming commit deterministic, and without it the default threshold keeps the buffered path. The--verboselines[gh-dep] Extract/[gh-dep] Streamedassert which extractor ran.package.jsonwhose six scripts runbun <script>.js, each writing<script>.txt, so theBlocked 6 postinstallsline, the#1234567short-commit output and the six file checks are unchanged apart from the names.bun bd testand with outbound network removed (HTTP_PROXY/HTTPS_PROXYunset in this container, which leaves only loopback): old GHSA file 0/5 pass, new file 10/10; old lifecycle cases 0/2, new cases 2/2;isolated-install.test.ts66/66; the rest of the lifecycle file is unchanged (the three tests that fail here also fail on main in this container, they neednode/bunonPATH).Background
owner/repo#refdependencies are downloaded as a tarball from the GitHub API rather than cloned. GitHub's tarballs unpack into a single<owner>-<repo>-<short sha>directory; bun records that directory name in the lockfile as the resolved commit (["dep@github:owner/repo#ref", {}, "owner-repo-sha", "sha512-..."]) and, since fix(install): store and verify SHA-512 integrity hash for GitHub tarball dependencies #27019, the sha512 of the tarball bytes after it, so a later install that has to refetch the tarball is checked against it.bun installhas two tarball extractors: bodies belowBUN_INSTALL_STREAMING_MIN_SIZEare buffered and extracted in one go (extract_tarball.rs); larger bodies are fed into libarchive as chunks arrive (TarballStream.rs). The decision is made on the first body chunk (src/install/NetworkTask.rs:223-240) and needs a 2xx status, a body at least the threshold size, and more data still to come, which is why the fixture has to be larger than one socket read.Still reaching GitHub after this PR
should handle GitHub URL ...cases inbun-install.test.ts, thegit dependenciescase inbun-install-registry.test.ts,bun-add.test.ts(test(install): serve GitHub tarball fixtures locally in bun-add.test.ts #35149 is open for it with its own builder indummy.registry.ts) andbunx.test.ts(test(bunx): install fixture packages from a local registry instead of real ones #38467, likewise). Those PRs can import this export instead once it lands; the remaining files are left for follow-ups so this one stays a fix for the two files that went red.git clone(git+https://github.com/..., SCP-style URLs,bun create), which needs a local repository rather than a tarball; test(install): deflake SCP-style git URL test in bun-add.test.ts #35035 covers thebun-add.test.tsinstance.no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.