fix(engine): reuse HyperFrames browser cache

[miguel-heygen]

Summary

• honor HYPERFRAMES_BROWSER_PATH in the engine browser resolver
• reuse the HyperFrames-managed headless-shell cache before falling back to Puppeteer
• sort cached browser versions numerically so Chrome 152 is not outranked by Chrome 99
• cover explicit-path and managed-cache resolution with regression tests

Verification

• bun --filter @hyperframes/engine test -- src/services/browserManager.test.ts (29 passed)
• bun --filter @hyperframes/engine typecheck
• pre-commit lint, format, fallow, tracked-artifact, and typecheck gates
• full engine suite: 989 passed; two unrelated VFR tests require a newer FFmpeg than the host provides (fps_mode is unsupported by its FFmpeg 4.2.2)

[vanceingalls]

Reviewed the diff at c002a6a end-to-end. Small, well-scoped fix; CI green across all required lanes (Build, Lint, Format, Typecheck, Tests, CLI smoke required, regression shards, perf, windows-latest render).

Strengths

• browserManager.ts:106-115 — numeric segment comparison correctly fixes the linux-99 > linux-152 lex-sort inversion; the regression test at browserManager.test.ts:47-84 seeds both versions in the cache and pins 152.0.7928.2 as the chosen binary, which would fail under the old sort().reverse() shape.
• browserManager.ts:117-136 — extracting findCachedHeadlessShell cleanly enables the fallback chain HYPERFRAMES_BROWSER_PATH → hyperframes cache → puppeteer cache without duplicating the candidate list.
• browserManager.ts:164-174 — HYPERFRAMES_BROWSER_PATH mirrors the PRODUCER_HEADLESS_SHELL_PATH throw-on-missing shape and adds an actionable hint (Run \hyperframes browser ensure``).

Notes (nit)

• browserManager.test.ts covers happy paths for both HYPERFRAMES_BROWSER_PATH and the managed cache, but not the missing-binary throw for HYPERFRAMES_BROWSER_PATH (there's an equivalent test for PRODUCER_HEADLESS_SHELL_PATH). Symmetric coverage would be a follow-up nit — not blocking, the throw shape is straightforward.
• compareBrowserVersionsDescending returns 0 when parse() yields [] on both sides (e.g. malformed dir names); those cluster stably, which is fine, but worth being aware of if future cache layouts change the dir-name shape.

Verdict: APPROVE
Reasoning: Correct fix for the sort inversion + a clean cache-priority chain, backed by a targeted regression test. All required checks pass at head; no blockers, no important findings.

— Via

[james-russo-rames-d-jusso]

Reviewed at c002a6abc4. Traced findCachedHeadlessShell + compareBrowserVersionsDescending against the CLI's browser-cache install path (packages/cli/src/browser/manager.ts).

Verdict framing

Framing correction on Magi's briefing: this PR does NOT reuse a Chrome user-data-dir (no session state, cookies, IndexedDB, or GPU shader cache is shared). It reuses the installed chrome-headless-shell binary cache at ~/.cache/hyperframes/chrome/ (a path the CLI has been writing to since #2060; the engine only knew about ~/.cache/puppeteer/). None of the SingletonLock / profile-state-leak concerns apply — this is read-only path resolution for the already-installed shell binary. Concurrency-safe (existsSync + readdirSync only), install-time races already guarded by INSTALL_LOCK_DIR in packages/cli/src/browser/manager.ts:43, consistent with the #2415 / #2555 shared-cache pattern.

Fix is narrow, correct, and improves version-sort. Three edge concerns worth surfacing.

Blockers

(none)

Concerns

See inline for anchored detail.

• 🟡 C1 (low) Version comparator at browserManager.ts:109-128 is silently permissive on non-standard prefixes. Puppeteer's real dir names (linux-152.0.7928.2, mac_arm-152.0.7928.2, win64-152.0.7928.2) are all safe today, but a hypothetical mac-arm64-152.0.7928.2 (double-hyphen) would tokenize as ["arm64-152", ...], parseInt → NaN, break immediately, segments [], and every such dir would sort as equal — order becomes readdir order, and an older build could win. Fallback: strip any leading non-numeric run, not just up to the first -.
• 🟡 C2 (low-medium) Engine picks newest-in-cache; CLI pins CHROME_VERSION. packages/cli/src/browser/manager.ts:288-290 requires buildId === CHROME_VERSION (currently "152.0.7928.2", line 23). findCachedHeadlessShell at browserManager.ts:130-150 picks the newest thing on disk. If someone drives the engine directly (tests, standalone use) without HYPERFRAMES_BROWSER_PATH set and only an older HF-cached binary remains, the engine launches the un-pinned older build — the exact scenario the #2060 pinning was supposed to prevent. Not a runtime bug in the CLI-driven path (CLI always passes HYPERFRAMES_BROWSER_PATH, see manager.ts:254), but a latent surprise for direct-engine consumers.
• 🟡 C3 (low) Second test at browserManager.test.ts:264-290 spawns bun --eval for what looks like path-resolution testing. Requires bun on PATH during test + a .ts loader (fine in this repo). Reason for the subprocess isn't obvious from the diff — os.homedir() re-reads HOME on every call, so an in-process process.env.HOME = ... would work in normal Node. If the motivation is Windows USERPROFILE caching semantics, a comment would help; otherwise this is a heavy way to unit-test path resolution.

Nits

• N1 writeFileSync(binary, "") at browserManager.test.ts:262-266 produces zero-byte non-executable files. The test only exercises path resolution, not launch — worth a comment noting the scope.
• N2 mkdirSync(join(binary, ".."), { recursive: true }) — dirname(binary) is clearer.
• N3 No negative test for the "HF cache empty, Puppeteer cache populated" fallback (browserManager.ts:186-190). Trivial to add.
• N4 Docstring update at lines 40-43 says "CLI browser override, HyperFrames' managed cache, and Puppeteer's cache" but skips PRODUCER_HEADLESS_SHELL_PATH which sits between them in the actual precedence chain. Minor stale copy.

What I didn't verify

• Actual on-disk directory naming under @puppeteer/browsers for macOS in the exact pinned version — relied on convention.
• Behavior on Windows (USERPROFILE) beyond what the test asserts — didn't run it.
• Whether any CI job runs the engine test file without bun available.
• Cache-size / GC policy — unchanged by this PR; not audited.

Merge gate

All required checks green. Nothing merge-blocking. LGTM from my side, COMMENTED.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

🟡 C1: Version comparator is silently permissive on non-standard prefixes.

compareBrowserVersionsDescending slices from the first -. Puppeteer's real dir names — linux-152.0.7928.2, mac_arm-152.0.7928.2 (underscore in platform token), win64-152.0.7928.2 — are all safe today.

Hostile case: a hypothetical mac-arm64-152.0.7928.2 (double-hyphen) would tokenize as ["arm64-152", ...], parseInt("arm64-152") → NaN, break immediately, segments [], and every such dir sorts as equal. Order collapses to readdir order — a stale older build in that dir could win over a fresher one.

Future-proof fix: split the prefix by stripping any leading non-numeric run, not just up to the first -:

const versionPart = dirName.replace(/^[^0-9]*/, '');
const segments = versionPart.split('.').map(n => parseInt(n, 10)).filter(n => !Number.isNaN(n));

That handles single-hyphen, double-hyphen, underscore, and dot separators uniformly.

Low severity — Puppeteer's naming convention is stable — but the current predicate is fragile on any future rename. — Review by Rames D Jusso

[james-russo-rames-d-jusso]

🟡 C2: Engine picks newest-in-cache; CLI pins CHROME_VERSION.

packages/cli/src/browser/manager.ts:288-290 requires buildId === CHROME_VERSION (currently "152.0.7928.2" at line 23). findCachedHeadlessShell here picks the newest thing on disk.

Divergence path: after a version bump, if only an older HF-cached binary remains (e.g. install failed halfway, user manually cleaned only the newer entry), and HYPERFRAMES_BROWSER_PATH isn't set (someone driving the engine directly — tests, standalone consumer, integration harnesses), the engine happily launches the un-pinned older build. That's the exact scenario the #2060 pinning was supposed to prevent.

Not a runtime bug on the CLI-driven happy path: CLI always sets HYPERFRAMES_BROWSER_PATH (manager.ts:254), so the engine's discovery is bypassed. But a latent surprise for direct-engine consumers.

Mitigation option: import CHROME_VERSION from CLI (or move it to a shared package) and have findCachedHeadlessShell filter to matching buildIds before sorting. Then the engine's pick is version-aligned with the CLI's install policy.

Low-med severity — depends on who drives the engine directly. — Review by Rames D Jusso

[james-russo-rames-d-jusso]

🟡 C3: bun --eval subprocess for what looks like path-resolution testing.

Spawning bun requires bun on PATH during test + the .ts loader path setup. Reason for the subprocess isn't obvious from the diff.

os.homedir() re-reads HOME on every call — so an in-process process.env.HOME = tempdir; ...; delete process.env.HOME would work in normal Node without spawning anything.

If the motivation is Windows-specific — USERPROFILE caching in the Node process where a change to process.env.USERPROFILE isn't picked up by later os.homedir() calls — a one-line comment would help future maintainers understand the constraint. Otherwise this is a heavy hammer for a unit test.

Also: does the CI matrix guarantee bun on all runners (Ubuntu / macOS / Windows)? A quiet bun: command not found skip is worse than an in-process test. — Review by Rames D Jusso