fix(cli): preflight browser archive extraction#2469
Conversation
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 7f908f364d3970761d3992829e909af881cd9b33.
What this does
Adds an "Archive extractor" preflight check to hyperframes doctor so a fresh non-Windows host is warned that unzip is missing before Puppeteer's managed-Chrome download fails mid-extraction. New checkArchiveExtractor(hostPlatform, exists) at packages/cli/src/commands/doctor.ts:143-160 returns { ok: true, detail: "Built into Windows" } for win32, { ok: true, detail: "unzip" } when commandExists("unzip") returns truthy on macOS / Linux, and { ok: false, detail: "Not found", hint: "Install unzip so HyperFrames can extract its managed Chrome download." } otherwise. Wired into the checks array at :272.
commandExists at :136-142 uses execFileSync("which", [command], { stdio: "ignore", timeout: 5000 }) — fixed executable + arg-array, no shell interpolation, 5-second cap on the probe.
Three unit tests at doctor.test.ts:91-117 pin the three platform × availability combinations via the injected exists fn.
Verification
- No shell injection surface.
execFileSyncwith a fixed"which"executable + arg-array. No env-derived path, no shell metachars. ✓ - Sensible timeout. 5s on a
whichprobe is generous; a hungwhichis pathological but bounded. ✓ - Dependency injection is clean.
checkArchiveExtractor(hostPlatform = platform(), exists = commandExists)— both defaults match production behavior; both overridable in tests. Sibling checks (checkDisk,checkCPU) don't have this shape, so this one is slightly more test-friendly than the neighbours. Note only. ✓ CheckResultshape matches sibling checks.{ ok, detail?, hint? }— same ascheckDiskand friends. DownstreambuildDoctorReporttreats it uniformly. ✓- Registration position in the
checksarray (after Disk, before /dev/shm branch) doesn't affect any prior check's outcome. ✓ - CI progressing. All
Detect changes+ File size + Semantic PR title + player-perf + WIP green; remaining pending will settle. Windows-latest not gated on Linux/macOS behavior for this specific fix. ✓
Concerns
-
🟢 The check depends on
whichbeing installed.whichis nearly universal on macOS + Linux but not guaranteed on minimal-image containers (some Alpine builds strip it in favor of the shell builtincommand -v/type, which aren't executables and can't be probed viaexecFileSync). On such a host,execFileSyncthrows →commandExistsreturns false → doctor reportsunzipmissing even when it's installed, and the user follows the hint pointlessly. Low-risk (whichis present on the vast majority of dev / CI hosts) but worth being aware of. If it ever surfaces as an issue,fs.accessSync(...)scanningprocess.env.PATHsplits would be awhich-free alternative. -
🟢 The Windows message ("Built into Windows") is a Puppeteer implementation claim. True today —
@puppeteer/browsersuses PowerShell'sExpand-Archive/ native tar on Windows and shells out tounzipon Unix — but if Puppeteer ever switches its Windows extractor to something that DOES needunzip.exe, the doctor's Windows path silently reports "OK" when it isn't. Non-blocking (would only surface as follow-up if the upstream backend changes) but the phrasing "Built into Windows" is more forgiving than "Puppeteer uses native extraction on Windows" — the latter would age better against upstream changes. -
🟢 Doctor is advisory, not blocking. If a user skips
doctorand runssetupdirectly on a bare Linux host, they still hit the mid-extraction failure this PR is meant to prevent — the PR fixes the diagnostic, not the failure mode itself. Gating the actual setup call on the same check would close the loop; probably a follow-up. Note-only.
Verdict framing
Small, well-scoped, well-tested addition. Safe injection surface, matches sibling-check shape, adds meaningful triage value. LGTM from my side.
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: RIGHT. Small, platform-aware preflight that adds a copy-paste hint before Puppeteer's unzip shell-out fails mid-extraction on a fresh Linux host. Injection-safe, test-friendly, correctly bypasses Windows.
Reviewed at 7f908f364d3970761d3992829e909af881cd9b33. Read alongside Rames — my lens is extrapolation + test hygiene + hint quality, not the injection / shape / DI axis he already cleared.
Cross-check on Rames
Concur on all three of his green notes (which on stripped Alpine, "Built into Windows" as a Puppeteer-implementation claim, doctor-advisory-vs-blocking). Nothing to add on those.
Additional lens
🟢 Hint could leverage the existing distro-aware install-command machinery. packages/cli/src/browser/linuxDeps.ts already exports detectLinuxDistro() + INSTALL_PREFIX (sudo apt-get install -y, sudo dnf install -y, sudo pacman -S --needed, sudo apk add). chromeDepsInstallCommand(distro.family) builds a copy-paste line for the shared-lib case. The new unzip hint at doctor.ts:158 is a generic "Install unzip so HyperFrames can extract its managed Chrome download." — it doesn't stamp the distro-specific apt-get install -y unzip / apk add unzip / etc. that the neighbouring Chrome-deps hint does. Non-blocking; the current text is still actionable. But a apk add unzip line saves the user one Google when they hit this on Alpine, and the plumbing already exists one directory over. Note only.
🟢 Context: PR #2400 (bundle zip extraction fallback — direct yauzl dep so no system unzip needed) is CLOSED. So the strategic bet is "preflight + hint" over "bundle a JS unzipper and silently fall back." That's a defensible call — closer to Puppeteer's own guidance, keeps the CLI dep tree light, and doctor's diagnostic surface is where users go when things fail. Just flagging the design choice explicitly for future travelers who wonder why HF doesn't just auto-fall-back to a JS unzipper.
🟢 Test hygiene — parametrization + import consistency. The three it blocks at doctor.test.ts:92-116 walk the platform × availability matrix by hand: one for linux + missing, one for darwin + present, one for win32. They'd read cleaner as a single it.each([...]) table — same coverage, easier to spot a hole (e.g. linux + present is currently only asserted transitively via the darwin case; win32 + present is untested). Also the first test does const doctor = await import(...); expect(doctor).toHaveProperty("checkArchiveExtractor") as a defensive existence assertion, while the other two destructure — pick one style. Minor; non-blocking.
🟢 Extrapolation — sibling shell-outs. Audited doctor.ts for other silent-binary-required paths:
checkDocker/checkDockerRunningalready report their own missing state via try/catch — covered.checkFFmpeg/checkFFprobeinpreflight.tssimilarly report missing binaries with an install hint — covered.- Chrome shared libraries are covered by
chromeSharedLibOutcome(linuxDeps.ts) — covered. manager.ts:whichBinarystill uses rawwhich/where, but that's for locating optional system Chrome as a preferred path; a miss there just falls through to download-managed. Not a silent-failure surface.
I don't see another sibling that silently hard-fails on a missing external binary the way unzip did before this PR. So the class is closed — this is the only preflight gap of this shape.
Envelope
Clean. Commit 7f908f36, single commit, no Co-Authored-By: Claude / no 🤖 Generated with [Claude Code]. Body carries Compound Engineering + GPT-5 badges only. Miguel-authored, no cascade-stamp concerns.
CI
headRefOid=7f908f36, mergeStateStatus=BLOCKED (approvals pending). Green: Detect changes ×6, Format, File size, Semantic PR title, player-perf, regression (early lanes), CLI: npx shim (ubuntu), WIP, CodeQL (early). In progress at review-time: Build, Lint, Typecheck, Fallow, Test, Producer unit/integration, SDK unit/contract/smoke, Studio smoke, CLI: npx shim (macos + windows), CLI smoke (required), CodeQL analyze × 2, preflight lanes. Nothing failed; the fix is contained to doctor.ts + its test file, so the pending long-runners aren't at risk from this diff.
Verdict
LGTM. Approving. The three green follow-ups (distro-aware hint, parametrized test table, and doctor-as-blocking-gate — Rames' third point) are worth a lightweight follow-up ticket but shouldn't hold this.
— Via
What
hyperframes doctornow checks for the archive extractor required before downloading managed Chrome on macOS and Linux.Why
Fresh Linux hosts without
unzippreviously passed setup preflight, then failed later during render withno zip archiver is available. The new check surfaces that missing dependency before the browser download begins and gives an actionable install hint. Windows remains green because Puppeteer uses built-in tar/PowerShell extraction there.Source feedback: https://heygen.slack.com/archives/C0BGC335AQY/p1784094494115199
How
Added a platform-aware archive-extractor check to the existing doctor report. Non-Windows hosts require
unzip; Windows reports its built-in extraction path. Dependency detection usesexecFileSyncwith a fixed command and no shell interpolation.Test plan
unzipand the Windows path (20 doctor tests passing)doctor --jsonreportsArchive extractor: unzipon this Linux host