fix(cli): prefer puppeteer cache + numeric version sort (staff review)

Two correctness fixes from PR #821 self-review:

1. Cache priority order. Previous order was hyperframes-managed cache →
   puppeteer cache. HF cache is pinned to CHROME_VERSION (131-era) which
   lags 17+ releases behind upstream; if a user separately installed a
   newer chrome-headless-shell via @puppeteer/browsers install, the CLI
   would silently hand engine the older HF-cache binary while engine's
   own resolveHeadlessShellPath would have picked the newer one. Flip
   the priority so puppeteer cache wins, matching engine semantics.

2. Numeric (not lexicographic) version sort. `readdirSync.sort().reverse()`
   over names like `linux-148.0.7778.97` and `linux-99.0.6533.123` would
   return `linux-99...` first because character '9' outranks '1'. Parse
   each name into integer segments and compare them numerically.

Tests: add both-caches-populated and linux-148-beats-linux-99 cases.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: vanceingalls

packages/cli/src/browser/manager.test.ts

@@ -102,7 +102,10 @@ describe("findBrowser — cache resolution", () => {
     vi.doUnmock("@puppeteer/browsers");
   });
 
-  it("resolves to the hyperframes-managed cache when present", async () => {
+  it("resolves to the hyperframes-managed cache when puppeteer cache is empty", async () => {
+    // Only HF cache populated. Puppeteer cache is the higher-priority path
+    // (see "prefers puppeteer cache" test below), so this exercises the
+    // last-resort fallback.
     installFsMocks({ existing: new Set([HF_CACHE, HF_BINARY]) });
     installPuppeteerBrowsersMock({
       installedInHfCache: [{ browser: "chrome-headless-shell", executablePath: HF_BINARY }],
@@ -129,8 +132,35 @@ describe("findBrowser — cache resolution", () => {
     expect(result).toEqual({ executablePath: PUPPETEER_BINARY, source: "cache" });
   });
 
+  it("prefers the puppeteer cache over the hyperframes cache when BOTH are populated", async () => {
+    // The HF cache is pinned to `CHROME_VERSION` (131-era) which lags upstream
+    // by many releases. The engine's `resolveHeadlessShellPath` scans the
+    // puppeteer cache and selects newest-version-first; if the CLI handed
+    // engine the older HF-cache binary while a newer puppeteer-cache binary
+    // exists, the two would silently disagree on which binary to use.
+    // This test pins the priority: puppeteer cache wins when both are populated.
+    installFsMocks({
+      existing: new Set([HF_CACHE, HF_BINARY, PUPPETEER_CACHE, PUPPETEER_BINARY]),
+      dirs: { [PUPPETEER_CACHE]: ["linux-148.0.7778.97"] },
+    });
+    installPuppeteerBrowsersMock({
+      installedInHfCache: [{ browser: "chrome-headless-shell", executablePath: HF_BINARY }],
+    });
+
+    const { findBrowser } = await import("./manager.js");
+    const result = await findBrowser();
+
+    expect(result?.executablePath).toBe(PUPPETEER_BINARY);
+    expect(result?.source).toBe("cache");
+  });
+
   it("picks the newest version when multiple chrome-headless-shell builds are cached", async () => {
-    const olderBinary = `${PUPPETEER_CACHE}/linux-131.0.6778.85/chrome-headless-shell-linux64/chrome-headless-shell`;
+    const olderBinary = join(
+      PUPPETEER_CACHE,
+      "linux-131.0.6778.85",
+      "chrome-headless-shell-linux64",
+      "chrome-headless-shell",
+    );
     installFsMocks({
       existing: new Set([PUPPETEER_CACHE, PUPPETEER_BINARY, olderBinary]),
       dirs: { [PUPPETEER_CACHE]: ["linux-131.0.6778.85", "linux-148.0.7778.97"] },
@@ -143,6 +173,32 @@ describe("findBrowser — cache resolution", () => {
     expect(result?.executablePath).toBe(PUPPETEER_BINARY);
   });
 
+  it("uses numeric (not lexicographic) version ordering — linux-148 beats linux-99", async () => {
+    // Regression guard for the lexicographic-sort bug: `"linux-99..."` sorts
+    // after `"linux-148..."` character-by-character (because `'9' > '1'`),
+    // which would have caused the CLI to hand engine an ancient 99-era binary
+    // when a fresh 148 was sitting right next to it. Numeric semver-style
+    // ordering is the only correct semantic.
+    const linux99Binary = join(
+      PUPPETEER_CACHE,
+      "linux-99.0.6533.123",
+      "chrome-headless-shell-linux64",
+      "chrome-headless-shell",
+    );
+    installFsMocks({
+      existing: new Set([PUPPETEER_CACHE, PUPPETEER_BINARY, linux99Binary]),
+      // Intentionally list the entries in an order that would expose the bug
+      // under naive `.sort().reverse()` (which puts `linux-99...` first).
+      dirs: { [PUPPETEER_CACHE]: ["linux-99.0.6533.123", "linux-148.0.7778.97"] },
+    });
+    installPuppeteerBrowsersMock();
+
+    const { findBrowser } = await import("./manager.js");
+    const result = await findBrowser();
+
+    expect(result?.executablePath).toBe(PUPPETEER_BINARY);
+  });
+
   it("falls back to system Chrome and warns on Linux when no cache has headless-shell", async () => {
     installFsMocks({ existing: new Set([SYSTEM_CHROME]) });
     installPuppeteerBrowsersMock();

packages/cli/src/browser/manager.ts

@@ -73,7 +73,26 @@ function findFromEnv(): BrowserResult | undefined {
 }
 
 async function findFromCache(): Promise<BrowserResult | undefined> {
-  // 1) Hyperframes-managed cache (populated by `clearBrowser` + `install` below).
+  // 1) Puppeteer's managed cache — where `npx @puppeteer/browsers install
+  // chrome-headless-shell` lands, and where `puppeteer install` from a project
+  // depending on full `puppeteer` (not `puppeteer-core`) lands. The engine's
+  // `resolveHeadlessShellPath` reads from here and selects newest-version-
+  // first; the CLI must match that semantic or it will silently hand the
+  // engine an older binary than the engine itself would pick.
+  //
+  // We intentionally check puppeteer BEFORE the hyperframes-managed cache:
+  // the HF cache is pinned to `CHROME_VERSION` (above) which lags behind
+  // upstream Chrome by many releases. If a user installed chrome-headless-shell
+  // separately (via `@puppeteer/browsers install`) we want to use that
+  // newer binary, not the pinned-stale fallback.
+  const fromPuppeteer = findFromPuppeteerCache();
+  if (fromPuppeteer) {
+    return fromPuppeteer;
+  }
+
+  // 2) Hyperframes-managed cache (populated by `ensureBrowser` below as a
+  // download-of-last-resort). This is the fallback path: only reached when
+  // no puppeteer-cache binary exists.
   if (existsSync(CACHE_DIR)) {
     const installed = await getInstalledBrowsers({ cacheDir: CACHE_DIR });
     const match = installed.find((b) => b.browser === Browser.CHROMEHEADLESSSHELL);
@@ -82,27 +101,61 @@ async function findFromCache(): Promise<BrowserResult | undefined> {
     }
   }
 
-  // 2) Puppeteer's managed cache — where `npx @puppeteer/browsers install
-  // chrome-headless-shell` lands, and where `puppeteer install` from a project
-  // that depends on full `puppeteer` (not `puppeteer-core`) lands. The engine
-  // already reads from here (`resolveHeadlessShellPath`); without this branch
-  // the CLI would skip past a perfectly good chrome-headless-shell and fall
-  // through to `findFromSystem()`, picking regular Chrome which has dropped
-  // `HeadlessExperimental.enable` and disables the perf-optimized capture
-  // path.
-  const fromPuppeteer = findFromPuppeteerCache();
-  if (fromPuppeteer) {
-    return fromPuppeteer;
+  return undefined;
+}
+
+/**
+ * Parse a puppeteer-cache version directory name (`linux-148.0.7778.97`,
+ * `mac_arm-131.0.6778.85`, etc.) into a numeric tuple for ordering.
+ *
+ * Lexicographic sort on these strings is buggy because `"99"` > `"148"` (the
+ * `9` outranks the `1` character-wise), so a 99-era binary would beat a
+ * 148-era binary in `.sort().reverse()`. We split on `-` to drop the platform
+ * prefix, then on `.` to get integer segments. Returns `undefined` for names
+ * that don't have at least one parseable numeric segment so they sort last.
+ */
+function parseVersionSegments(versionDir: string): number[] | undefined {
+  const dashIdx = versionDir.indexOf("-");
+  const versionPart = dashIdx >= 0 ? versionDir.slice(dashIdx + 1) : versionDir;
+  const segments = versionPart.split(".");
+  const parsed: number[] = [];
+  for (const seg of segments) {
+    const n = parseInt(seg, 10);
+    if (!Number.isFinite(n)) {
+      // Stop at the first non-numeric segment but keep what we've collected.
+      break;
+    }
+    parsed.push(n);
   }
+  return parsed.length > 0 ? parsed : undefined;
+}
 
-  return undefined;
+/** Numeric semver-style descending comparator for puppeteer cache dirs. */
+function compareVersionDirsDescending(a: string, b: string): number {
+  const pa = parseVersionSegments(a);
+  const pb = parseVersionSegments(b);
+  // Unparseable names sort after parseable ones (so we still try them, just last).
+  if (!pa && !pb) return 0;
+  if (!pa) return 1;
+  if (!pb) return -1;
+  const len = Math.max(pa.length, pb.length);
+  for (let i = 0; i < len; i += 1) {
+    const av = pa[i] ?? 0;
+    const bv = pb[i] ?? 0;
+    if (av !== bv) return bv - av; // descending (newest first)
+  }
+  return 0;
 }
 
 function findFromPuppeteerCache(): BrowserResult | undefined {
   if (!existsSync(PUPPETEER_CACHE_DIR)) return undefined;
   let versions: string[];
   try {
-    versions = readdirSync(PUPPETEER_CACHE_DIR).sort().reverse(); // newest first
+    // Numeric semver-style sort, newest first. Lexicographic `.sort().reverse()`
+    // (the previous implementation, still in engine `resolveHeadlessShellPath`)
+    // mis-orders `linux-99...` ahead of `linux-148...` because character `'9'`
+    // outranks `'1'`. See `parseVersionSegments` above.
+    versions = [...readdirSync(PUPPETEER_CACHE_DIR)].sort(compareVersionDirsDescending);
   } catch {
     return undefined;
   }
@@ -159,7 +212,7 @@ function warnSystemFallbackOnce(executablePath: string): void {
   if (isHeadlessShellBinary(executablePath)) return;
   _warnedSystemFallback = true;
   console.warn(
-    `[hyperframes] Using system Chrome at ${executablePath}; HeadlessExperimental.beginFrame is unavailable in regular Chrome builds, so the perf-optimized capture path falls back to screenshot mode. Install chrome-headless-shell for the optimized path:\n  npx @puppeteer/browsers install chrome-headless-shell`,
+    `[hyperframes] Using system Chrome at ${executablePath}; HeadlessExperimental.beginFrame is unavailable in regular Chrome builds, so the perf-optimized capture path falls back to screenshot mode. Install chrome-headless-shell for the optimized path:\n  npx @puppeteer/browsers install chrome-headless-shell\n(Or set HYPERFRAMES_BROWSER_PATH to point at an existing chrome-headless-shell binary.)`,
   );
 }