Skip to content

fix(cli): preflight browser archive extraction#2469

Merged
miguel-heygen merged 1 commit into
mainfrom
fix/doctor-archive-extractor
Jul 15, 2026
Merged

fix(cli): preflight browser archive extraction#2469
miguel-heygen merged 1 commit into
mainfrom
fix/doctor-archive-extractor

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

What

hyperframes doctor now checks for the archive extractor required before downloading managed Chrome on macOS and Linux.

Why

Fresh Linux hosts without unzip previously passed setup preflight, then failed later during render with no 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 uses execFileSync with a fixed command and no shell interpolation.

Test plan

  • Unit tests cover missing/present unzip and the Windows path (20 doctor tests passing)
  • CLI TypeScript check, lint, formatting, Fallow, and build pass
  • Built doctor --json reports Archive extractor: unzip on this Linux host
  • Documentation updated (not applicable)

Compound Engineering
Codex

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. execFileSync with a fixed "which" executable + arg-array. No env-derived path, no shell metachars. ✓
  • Sensible timeout. 5s on a which probe is generous; a hung which is 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. ✓
  • CheckResult shape matches sibling checks. { ok, detail?, hint? } — same as checkDisk and friends. Downstream buildDoctorReport treats it uniformly. ✓
  • Registration position in the checks array (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 which being installed. which is nearly universal on macOS + Linux but not guaranteed on minimal-image containers (some Alpine builds strip it in favor of the shell builtin command -v / type, which aren't executables and can't be probed via execFileSync). On such a host, execFileSync throws → commandExists returns false → doctor reports unzip missing even when it's installed, and the user follows the hint pointlessly. Low-risk (which is present on the vast majority of dev / CI hosts) but worth being aware of. If it ever surfaces as an issue, fs.accessSync(...) scanning process.env.PATH splits would be a which-free alternative.

  • 🟢 The Windows message ("Built into Windows") is a Puppeteer implementation claim. True today — @puppeteer/browsers uses PowerShell's Expand-Archive / native tar on Windows and shells out to unzip on Unix — but if Puppeteer ever switches its Windows extractor to something that DOES need unzip.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 doctor and runs setup directly 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.

Review by Rames D Jusso

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 / checkDockerRunning already report their own missing state via try/catch — covered.
  • checkFFmpeg / checkFFprobe in preflight.ts similarly report missing binaries with an install hint — covered.
  • Chrome shared libraries are covered by chromeSharedLibOutcome (linuxDeps.ts) — covered.
  • manager.ts:whichBinary still uses raw which / 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

@miguel-heygen miguel-heygen merged commit d2aa4f8 into main Jul 15, 2026
43 checks passed
@miguel-heygen miguel-heygen deleted the fix/doctor-archive-extractor branch July 15, 2026 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants