Merge PR #1307: Bun.which-based binary resolution for browse + pdftotext on Windows

make-pdf/src/browseClient.ts

@@ -7,12 +7,20 @@
  *     (Windows argv cap is 8191 chars; 200KB HTML dies without this).
  *   - One place that maps non-zero exit codes to typed errors.
  *
- * Binary resolution order (Codex round 2 #4):
+ * Binary resolution order (Codex round 2 #4, v1.24-aligned):
- *   1. $BROWSE_BIN env override
+ *   1. $GSTACK_BROWSE_BIN env override (preferred, matches v1.24 GSTACK_*_BIN pattern)
- *   2. sibling dir: dirname(argv[0])/../browse/dist/browse
+ *   2. $BROWSE_BIN env override (back-compat alias)
- *   3. ~/.claude/skills/gstack/browse/dist/browse
+ *   3. sibling dir: dirname(argv[0])/../browse/dist/browse[.exe]
- *   4. PATH lookup: `browse`
+ *   4. ~/.claude/skills/gstack/browse/dist/browse[.exe]
- *   5. error with setup hint
+ *   5. PATH lookup via Bun.which('browse') — handles Windows PATHEXT natively
+ *   6. error with setup hint
+ *
+ * Windows quirks:
+ *   - bun build --compile --outfile X emits X.exe on win32, so candidate paths
+ *     need a .exe probe pass (fs.accessSync(X_OK) degrades to existence-checking
+ *     on Windows per Node docs, so the bare path silently misses the .exe file).
+ *   - `which` only exists in Git Bash; Bun.which() handles cmd.exe / PowerShell
+ *     natively via PATHEXT semantics.
  */
 
 import { execFileSync } from "node:child_process";
@@ -55,15 +63,51 @@ export interface JsOptions {
 }
 
 /**
- * Locate the browse binary. Throws a BrowseClientError with a
+ * Resolve an absolute or PATH-resolvable command via Bun.which-style semantics,
- * canonical setup message if not found.
+ * with a Windows .exe/.cmd/.bat extension probe for absolute paths. Mirrors
+ * the v1.24 claude-bin.ts override-resolution shape.
+ *
+ * Returns null if nothing resolves; callers degrade with a typed error rather
+ * than throwing here.
  */
-export function resolveBrowseBin(): string {
+function resolveOverride(value: string | undefined, env: NodeJS.ProcessEnv): string | null {
-  const envOverride = process.env.BROWSE_BIN;
+  if (!value?.trim()) return null;
-  if (envOverride && isExecutable(envOverride)) return envOverride;
+  const trimmed = value.trim().replace(/^"(.*)"$/, '$1');
+  if (path.isAbsolute(trimmed)) return findExecutable(trimmed);
+  const PATH = env.PATH ?? env.Path ?? '';
+  return Bun.which(trimmed, { PATH }) ?? null;
+}
 
-  // Sibling: look relative to this process's binary
+/**
-  // (for when make-pdf and browse live next to each other in dist/)
+ * Probe a base path for executability, honoring Windows extension suffixes.
+ *
+ * On POSIX, isExecutable(base) is the only check that matters. On Windows,
+ * fs.accessSync(p, X_OK) degrades to an existence check — so a bare-path probe
+ * misses bun-compiled binaries (which land at base.exe). After the bare probe
+ * fails on win32, try .exe / .cmd / .bat. Linux/macOS behavior is unchanged.
+ */
+export function findExecutable(base: string): string | null {
+  if (isExecutable(base)) return base;
+  if (process.platform === "win32") {
+    for (const ext of [".exe", ".cmd", ".bat"]) {
+      const withExt = base + ext;
+      if (isExecutable(withExt)) return withExt;
+    }
+  }
+  return null;
+}
+
+/**
+ * Locate the browse binary. Throws a BrowseClientError with a
+ * canonical setup message if not found. See header for resolution order.
+ */
+export function resolveBrowseBin(env: NodeJS.ProcessEnv = process.env): string {
+  // 1 + 2: env overrides (GSTACK_BROWSE_BIN preferred, BROWSE_BIN back-compat).
+  const overrideRaw = env.GSTACK_BROWSE_BIN ?? env.BROWSE_BIN;
+  const override = resolveOverride(overrideRaw, env);
+  if (override) return override;
+
+  // 3: sibling — make-pdf and browse co-located in dist/.
   const selfDir = path.dirname(process.argv[0]);
   const siblingCandidates = [
     path.resolve(selfDir, "../browse/dist/browse"),
@@ -71,21 +115,21 @@ export function resolveBrowseBin(): string {
     path.resolve(selfDir, "../browse"),
   ];
   for (const candidate of siblingCandidates) {
-    if (isExecutable(candidate)) return candidate;
+    const found = findExecutable(candidate);
+    if (found) return found;
   }
 
-  // Global install
+  // 4: global install.
   const home = os.homedir();
   const globalPath = path.join(home, ".claude/skills/gstack/browse/dist/browse");
-  if (isExecutable(globalPath)) return globalPath;
+  const globalFound = findExecutable(globalPath);
+  if (globalFound) return globalFound;
 
-  // PATH lookup
+  // 5: PATH lookup via Bun.which — handles Windows PATHEXT natively (no `which`
-  try {
+  // dependency on cmd.exe / PowerShell, no `where`-vs-`which` branch).
-    const which = execFileSync("which", ["browse"], { encoding: "utf8" }).trim();
+  const PATH = env.PATH ?? env.Path ?? '';
-    if (which && isExecutable(which)) return which;
+  const onPath = Bun.which('browse', { PATH });
-  } catch {
+  if (onPath) return onPath;
-    // `which` exited non-zero; fall through to error
-  }
 
   throw new BrowseClientError(
     /* exitCode */ 127,
@@ -95,7 +139,8 @@ export function resolveBrowseBin(): string {
       "",
       "make-pdf needs browse (the gstack Chromium daemon) to render PDFs.",
       "Tried:",
-      `  - $BROWSE_BIN (${envOverride || "unset"})`,
+      `  - $GSTACK_BROWSE_BIN (${env.GSTACK_BROWSE_BIN || "unset"})`,
+      `  - $BROWSE_BIN (${env.BROWSE_BIN || "unset"})`,
       `  - sibling: ${siblingCandidates.join(", ")}`,
       `  - global: ${globalPath}`,
       "  - PATH: `browse`",
@@ -103,8 +148,10 @@ export function resolveBrowseBin(): string {
       "To fix: run gstack setup from the gstack repo:",
       "  cd ~/.claude/skills/gstack && ./setup",
       "",
-      "Or set BROWSE_BIN explicitly:",
+      "Or set GSTACK_BROWSE_BIN explicitly:",
-      "  export BROWSE_BIN=/path/to/browse",
+      process.platform === "win32"
+        ? '  setx GSTACK_BROWSE_BIN "C:\\path\\to\\browse.exe"'
+        : "  export GSTACK_BROWSE_BIN=/path/to/browse",
     ].join("\n"),
   );
 }

make-pdf/src/pdftotext.ts

@@ -13,11 +13,14 @@
  *        between paragraphs, and homoglyph substitution. We add a word-token
  *        diff and a paragraph-boundary assertion on top.
  *
- * Resolution order for the pdftotext binary:
+ * Resolution order for the pdftotext binary (v1.24-aligned):
- *   1. $PDFTOTEXT_BIN env override
+ *   1. $GSTACK_PDFTOTEXT_BIN env override (preferred, matches v1.24 GSTACK_*_BIN pattern)
- *   2. `which pdftotext` on PATH
+ *   2. $PDFTOTEXT_BIN env override (back-compat alias)
- *   3. standard Homebrew paths on macOS
+ *   3. PATH lookup via Bun.which('pdftotext') — handles Windows PATHEXT natively
- *   4. throws a friendly "install poppler" error
+ *   4. standard POSIX paths (Homebrew + distro) — no Windows candidates because
+ *      Poppler scatters across Scoop / Chocolatey / oschwartz10612-poppler-windows
+ *      and guessing causes false positives. Set GSTACK_PDFTOTEXT_BIN explicitly.
+ *   5. throws a friendly "install poppler" error
  *
  * The wrapper is *optional at runtime*: production renders don't need it.
  * Only the CI gate and unit tests invoke pdftotext.
@@ -41,30 +44,53 @@ export interface PdftotextInfo {
   flavor: "poppler" | "xpdf" | "unknown";
 }
 
+/**
+ * Probe a base path for executability, honoring Windows extension suffixes.
+ * Matches browseClient.ts:findExecutable — duplicated rather than shared
+ * because the two modules already duplicate isExecutable for compile-isolation.
+ */
+export function findExecutable(base: string): string | null {
+  if (isExecutable(base)) return base;
+  if (process.platform === "win32") {
+    for (const ext of [".exe", ".cmd", ".bat"]) {
+      const withExt = base + ext;
+      if (isExecutable(withExt)) return withExt;
+    }
+  }
+  return null;
+}
+
+function resolveOverride(value: string | undefined, env: NodeJS.ProcessEnv): string | null {
+  if (!value?.trim()) return null;
+  const trimmed = value.trim().replace(/^"(.*)"$/, '$1');
+  if (path.isAbsolute(trimmed)) return findExecutable(trimmed);
+  const PATH = env.PATH ?? env.Path ?? '';
+  return Bun.which(trimmed, { PATH }) ?? null;
+}
+
 /**
  * Locate pdftotext. Throws PdftotextUnavailableError if none is found.
  */
-export function resolvePdftotext(): PdftotextInfo {
+export function resolvePdftotext(env: NodeJS.ProcessEnv = process.env): PdftotextInfo {
-  const envOverride = process.env.PDFTOTEXT_BIN;
+  // 1 + 2: env overrides (GSTACK_PDFTOTEXT_BIN preferred, PDFTOTEXT_BIN back-compat).
-  if (envOverride && isExecutable(envOverride)) {
+  const overrideRaw = env.GSTACK_PDFTOTEXT_BIN ?? env.PDFTOTEXT_BIN;
-    return describeBinary(envOverride);
+  const override = resolveOverride(overrideRaw, env);
-  }
+  if (override) return describeBinary(override);
 
-  // Try PATH
+  // 3: PATH lookup via Bun.which — handles Windows PATHEXT natively.
-  try {
+  const PATH = env.PATH ?? env.Path ?? '';
-    const which = execFileSync("which", ["pdftotext"], { encoding: "utf8" }).trim();
+  const onPath = Bun.which('pdftotext', { PATH });
-    if (which && isExecutable(which)) return describeBinary(which);
+  if (onPath) return describeBinary(onPath);
-  } catch {
-    // fall through
-  }
 
-  // Common macOS Homebrew locations
+  // 4: POSIX-only standard locations. No Windows candidates — Poppler installs
-  const macCandidates = [
+  // scatter across Scoop/Chocolatey/portable zips and guessing causes false
-    "/opt/homebrew/bin/pdftotext",     // Apple Silicon
+  // positives. Windows users set GSTACK_PDFTOTEXT_BIN explicitly.
+  const posixCandidates = [
+    "/opt/homebrew/bin/pdftotext",     // Apple Silicon Homebrew
     "/usr/local/bin/pdftotext",        // Intel Mac or Linuxbrew
     "/usr/bin/pdftotext",              // distro package
   ];
-  for (const candidate of macCandidates) {
+  for (const candidate of posixCandidates) {
     if (isExecutable(candidate)) return describeBinary(candidate);
   }
 
@@ -75,12 +101,16 @@ export function resolvePdftotext(): PdftotextInfo {
     "(Runtime rendering does NOT need it. This only affects tests.)",
     "",
     "To install:",
-    "  macOS:  brew install poppler",
+    "  macOS:    brew install poppler",
-    "  Ubuntu: sudo apt-get install poppler-utils",
+    "  Ubuntu:   sudo apt-get install poppler-utils",
-    "  Fedora: sudo dnf install poppler-utils",
+    "  Fedora:   sudo dnf install poppler-utils",
+    "  Windows:  scoop install poppler  (or download from",
+    "            https://github.com/oschwartz10612/poppler-windows)",
     "",
-    "Or set PDFTOTEXT_BIN to an explicit path:",
+    "Or set GSTACK_PDFTOTEXT_BIN to an explicit path:",
-    "  export PDFTOTEXT_BIN=/path/to/pdftotext",
+    process.platform === "win32"
+      ? '  setx GSTACK_PDFTOTEXT_BIN "C:\\path\\to\\pdftotext.exe"'
+      : "  export GSTACK_PDFTOTEXT_BIN=/path/to/pdftotext",
   ].join("\n"));
 }
 

make-pdf/test/browseClient.test.ts

@@ -2,60 +2,123 @@
  * browseClient unit tests — binary resolution and error mapping.
  *
  * These are pure unit tests; they do NOT require a running browse daemon.
+ * Cross-platform: assertions that pin POSIX behavior early-return on win32
+ * and vice versa, so both lanes only exercise their own branch.
  */
 
 import { describe, expect, test } from "bun:test";
+import * as fs from "node:fs";
+import * as os from "node:os";
+import * as path from "node:path";
 
 import { BrowseClientError } from "../src/types";
-import { resolveBrowseBin } from "../src/browseClient";
+import { resolveBrowseBin, findExecutable } from "../src/browseClient";
+
+// A real, always-present executable for the test platform — `cmd.exe` on
+// Windows (System32 is on every install) and `/bin/sh` on POSIX. Lets the
+// "honors override when it points at a real executable" test work in both
+// lanes without writing a temp script.
+const REAL_EXE: string =
+  process.platform === "win32"
+    ? path.join(process.env.SystemRoot ?? "C:\\Windows", "System32", "cmd.exe")
+    : "/bin/sh";
+
+function withEnv<T>(overrides: Record<string, string | undefined>, fn: () => T): T {
+  const saved: Record<string, string | undefined> = {};
+  for (const k of Object.keys(overrides)) saved[k] = process.env[k];
+  for (const [k, v] of Object.entries(overrides)) {
+    if (v === undefined) delete process.env[k];
+    else process.env[k] = v;
+  }
+  try {
+    return fn();
+  } finally {
+    for (const [k, v] of Object.entries(saved)) {
+      if (v === undefined) delete process.env[k];
+      else process.env[k] = v;
+    }
+  }
+}
+
+describe("findExecutable", () => {
+  test("returns the bare path on POSIX when it's executable", () => {
+    if (process.platform === "win32") return;
+    const found = findExecutable("/bin/sh");
+    expect(found).toBe("/bin/sh");
+  });
+
+  test("on win32, probes .exe / .cmd / .bat after the bare-path miss", () => {
+    if (process.platform !== "win32") return;
+    // cmd.exe lives at System32\cmd.exe — probe with the bare base.
+    const base = path.join(process.env.SystemRoot ?? "C:\\Windows", "System32", "cmd");
+    const found = findExecutable(base);
+    expect(found).toBe(base + ".exe");
+  });
+
+  test("returns null when no extension matches", () => {
+    const found = findExecutable("/nonexistent/path/to/nothing");
+    expect(found).toBeNull();
+  });
+});
 
 describe("resolveBrowseBin", () => {
   test("throws BrowseClientError with setup hint when nothing is found", () => {
-    // Point every candidate path to a non-existent location.
+    // Point overrides at non-existent paths and clear PATH so Bun.which finds
-    const originalEnv = process.env.BROWSE_BIN;
+    // nothing. Sibling/global probes go through findExecutable on real paths,
-    process.env.BROWSE_BIN = "/nonexistent/browse-does-not-exist";
+    // but the test asserts on the error shape rather than depending on whether
-
+    // a real browse install exists on the box.
-    // We can't easily mock the sibling and global paths without touching
+    let thrown: unknown = null;
-    // the filesystem, so in a typical dev environment this will usually
-    // find the real browse. That's fine — on CI it will throw, and the
-    // error message shape is what we're actually asserting.
-    let thrown: any = null;
     try {
-      resolveBrowseBin();
+      withEnv(
+        {
+          GSTACK_BROWSE_BIN: "/nonexistent/gstack-browse-bin",
+          BROWSE_BIN: "/nonexistent/browse-bin",
+          PATH: "",
+          Path: "",
+        },
+        () => resolveBrowseBin(),
+      );
     } catch (err) {
       thrown = err;
     }
 
     if (thrown) {
       expect(thrown).toBeInstanceOf(BrowseClientError);
-      expect(thrown.message).toContain("browse binary not found");
+      expect((thrown as BrowseClientError).message).toContain("browse binary not found");
-      expect(thrown.message).toContain("./setup");
+      expect((thrown as BrowseClientError).message).toContain("./setup");
-      expect(thrown.message).toContain("BROWSE_BIN");
+      expect((thrown as BrowseClientError).message).toContain("GSTACK_BROWSE_BIN");
-    }
+      // Back-compat alias still surfaces in the diagnostic.
-
+      expect((thrown as BrowseClientError).message).toContain("BROWSE_BIN");
-    // Restore env
-    if (originalEnv === undefined) {
-      delete process.env.BROWSE_BIN;
-    } else {
-      process.env.BROWSE_BIN = originalEnv;
     }
+    // If the test box has a real browse install on disk, sibling/global may
+    // resolve and the helper won't throw — that's fine; the assertion is
+    // gated on whether it threw at all.
   });
 
-  test("honors BROWSE_BIN when it points at a real executable", () => {
+  test("honors GSTACK_BROWSE_BIN when it points at a real executable", () => {
-    const originalEnv = process.env.BROWSE_BIN;
+    const resolved = withEnv({ GSTACK_BROWSE_BIN: REAL_EXE }, () => resolveBrowseBin());
-    // `/bin/sh` exists on every POSIX system and is executable.
+    expect(resolved).toBe(REAL_EXE);
-    process.env.BROWSE_BIN = "/bin/sh";
+  });
 
-    try {
+  test("honors BROWSE_BIN as a back-compat alias", () => {
-      const resolved = resolveBrowseBin();
+    const resolved = withEnv(
-      expect(resolved).toBe("/bin/sh");
+      { GSTACK_BROWSE_BIN: undefined, BROWSE_BIN: REAL_EXE },
-    } finally {
+      () => resolveBrowseBin(),
-      if (originalEnv === undefined) {
+    );
-        delete process.env.BROWSE_BIN;
+    expect(resolved).toBe(REAL_EXE);
-      } else {
+  });
-        process.env.BROWSE_BIN = originalEnv;
+
-      }
+  test("GSTACK_BROWSE_BIN takes precedence over BROWSE_BIN", () => {
-    }
+    const resolved = withEnv(
+      { GSTACK_BROWSE_BIN: REAL_EXE, BROWSE_BIN: "/nonexistent/legacy" },
+      () => resolveBrowseBin(),
+    );
+    expect(resolved).toBe(REAL_EXE);
+  });
+
+  test("strips wrapping double quotes from override values", () => {
+    const resolved = withEnv({ GSTACK_BROWSE_BIN: `"${REAL_EXE}"` }, () => resolveBrowseBin());
+    expect(resolved).toBe(REAL_EXE);
   });
 });
 

make-pdf/test/pdftotext.test.ts

@@ -8,7 +8,8 @@
 
 import { describe, expect, test } from "bun:test";
 
-import { normalize, copyPasteGate } from "../src/pdftotext";
+import * as path from "node:path";
+import { normalize, copyPasteGate, findExecutable, resolvePdftotext, PdftotextUnavailableError } from "../src/pdftotext";
 
 describe("normalize", () => {
   test("strips trailing spaces", () => {
@@ -104,3 +105,103 @@ describe("copyPasteGate — assertion logic", () => {
     expect(Math.abs(expectedBreaks - tooManyBreaksNormalized)).toBeLessThanOrEqual(4);
   });
 });
+
+// ─── Binary resolution (v1.24-aligned) ──────────────────────────
+
+const REAL_EXE: string =
+  process.platform === "win32"
+    ? path.join(process.env.SystemRoot ?? "C:\\Windows", "System32", "cmd.exe")
+    : "/bin/sh";
+
+function withEnv<T>(overrides: Record<string, string | undefined>, fn: () => T): T {
+  const saved: Record<string, string | undefined> = {};
+  for (const k of Object.keys(overrides)) saved[k] = process.env[k];
+  for (const [k, v] of Object.entries(overrides)) {
+    if (v === undefined) delete process.env[k];
+    else process.env[k] = v;
+  }
+  try {
+    return fn();
+  } finally {
+    for (const [k, v] of Object.entries(saved)) {
+      if (v === undefined) delete process.env[k];
+      else process.env[k] = v;
+    }
+  }
+}
+
+describe("findExecutable (pdftotext.ts)", () => {
+  test("returns the bare path on POSIX when it's executable", () => {
+    if (process.platform === "win32") return;
+    expect(findExecutable("/bin/sh")).toBe("/bin/sh");
+  });
+
+  test("on win32, probes .exe / .cmd / .bat after the bare-path miss", () => {
+    if (process.platform !== "win32") return;
+    const base = path.join(process.env.SystemRoot ?? "C:\\Windows", "System32", "cmd");
+    expect(findExecutable(base)).toBe(base + ".exe");
+  });
+
+  test("returns null when no extension matches", () => {
+    expect(findExecutable("/nonexistent/path/to/nothing")).toBeNull();
+  });
+});
+
+describe("resolvePdftotext (override resolution, v1.24-aligned)", () => {
+  test("honors GSTACK_PDFTOTEXT_BIN when it points at a real executable", () => {
+    // We can't fake a real pdftotext, but we can fake "any executable" to
+    // exercise the override-resolution path. describeBinary will mark flavor
+    // as "unknown" since cmd.exe / /bin/sh don't respond to -v like pdftotext;
+    // the test asserts on the bin-path resolution, not the version probe.
+    const info = withEnv({ GSTACK_PDFTOTEXT_BIN: REAL_EXE }, () => resolvePdftotext());
+    expect(info.bin).toBe(REAL_EXE);
+  });
+
+  test("honors PDFTOTEXT_BIN as a back-compat alias", () => {
+    const info = withEnv(
+      { GSTACK_PDFTOTEXT_BIN: undefined, PDFTOTEXT_BIN: REAL_EXE },
+      () => resolvePdftotext(),
+    );
+    expect(info.bin).toBe(REAL_EXE);
+  });
+
+  test("GSTACK_PDFTOTEXT_BIN takes precedence over PDFTOTEXT_BIN", () => {
+    const info = withEnv(
+      { GSTACK_PDFTOTEXT_BIN: REAL_EXE, PDFTOTEXT_BIN: "/nonexistent/legacy" },
+      () => resolvePdftotext(),
+    );
+    expect(info.bin).toBe(REAL_EXE);
+  });
+
+  test("strips wrapping double quotes from override values", () => {
+    const info = withEnv({ GSTACK_PDFTOTEXT_BIN: `"${REAL_EXE}"` }, () => resolvePdftotext());
+    expect(info.bin).toBe(REAL_EXE);
+  });
+
+  test("error message includes Windows install hint and GSTACK_PDFTOTEXT_BIN", () => {
+    let thrown: unknown = null;
+    try {
+      withEnv(
+        {
+          GSTACK_PDFTOTEXT_BIN: "/nonexistent/gstack-pdftotext",
+          PDFTOTEXT_BIN: "/nonexistent/pdftotext",
+          PATH: "",
+          Path: "",
+        },
+        () => resolvePdftotext(),
+      );
+    } catch (err) {
+      thrown = err;
+    }
+    // If the test box has a real pdftotext on disk, resolution succeeds
+    // (POSIX candidates) — that's fine; the assertion is gated on whether
+    // it threw. On Windows-CI without poppler, it throws.
+    if (thrown) {
+      expect(thrown).toBeInstanceOf(PdftotextUnavailableError);
+      expect((thrown as Error).message).toContain("pdftotext not found");
+      expect((thrown as Error).message).toContain("GSTACK_PDFTOTEXT_BIN");
+      expect((thrown as Error).message).toContain("Windows");
+      expect((thrown as Error).message).toContain("scoop install poppler");
+    }
+  });
+});