fix(make-pdf): Windows .exe resolution for browse + pdftotext

make-pdf/src/browseClient.ts

@@ -60,7 +60,10 @@ export interface JsOptions {
  */
 export function resolveBrowseBin(): string {
   const envOverride = process.env.BROWSE_BIN;
-  if (envOverride && isExecutable(envOverride)) return envOverride;
+  if (envOverride) {
+    const resolved = findExecutable(envOverride);
+    if (resolved) return resolved;
+  }
 
   // Sibling: look relative to this process's binary
   // (for when make-pdf and browse live next to each other in dist/)
@@ -71,44 +74,88 @@ export function resolveBrowseBin(): string {
     path.resolve(selfDir, "../browse"),
   ];
   for (const candidate of siblingCandidates) {
-    if (isExecutable(candidate)) return candidate;
+    const resolved = findExecutable(candidate);
+    if (resolved) return resolved;
   }
 
   // Global install
   const home = os.homedir();
   const globalPath = path.join(home, ".claude/skills/gstack/browse/dist/browse");
-  if (isExecutable(globalPath)) return globalPath;
+  const globalResolved = findExecutable(globalPath);
+  if (globalResolved) return globalResolved;
 
-  // PATH lookup
+  // PATH lookup — use `where` on Windows (native, always in System32),
+  // `which` on POSIX. Git Bash provides `which` too, but Windows users
+  // running from cmd.exe or PowerShell don't have it.
+  const pathLookupCmd = process.platform === "win32" ? "where" : "which";
   try {
-    const which = execFileSync("which", ["browse"], { encoding: "utf8" }).trim();
-    if (which && isExecutable(which)) return which;
+    const out = execFileSync(pathLookupCmd, ["browse"], { encoding: "utf8" }).trim();
+    // `where` can return multiple matches; take the first.
+    const firstHit = out.split(/\r?\n/)[0]?.trim();
+    if (firstHit) {
+      const resolved = findExecutable(firstHit);
+      if (resolved) return resolved;
+    }
   } catch {
-    // `which` exited non-zero; fall through to error
+    // non-zero exit; fall through to error
   }
 
+  const setCmd = process.platform === "win32"
+    ? '  setx BROWSE_BIN "C:\\path\\to\\browse.exe"'
+    : "  export BROWSE_BIN=/path/to/browse";
   throw new BrowseClientError(
     /* exitCode */ 127,
     "resolve",
     [
       "browse binary not found.",
       "",
       "make-pdf needs browse (the gstack Chromium daemon) to render PDFs.",
+      `Platform: ${process.platform}`,
       "Tried:",
       `  - $BROWSE_BIN (${envOverride || "unset"})`,
       `  - sibling: ${siblingCandidates.join(", ")}`,
       `  - global: ${globalPath}`,
-      "  - PATH: `browse`",
+      `  - PATH: \`browse\` (via ${pathLookupCmd})`,
       "",
       "To fix: run gstack setup from the gstack repo:",
       "  cd ~/.claude/skills/gstack && ./setup",
       "",
       "Or set BROWSE_BIN explicitly:",
-      "  export BROWSE_BIN=/path/to/browse",
+      setCmd,
     ].join("\n"),
   );
 }
 
+/**
+ * Resolve a base path to an executable, probing platform-specific extensions.
+ *
+ * On win32, probes `{base}.exe`, `{base}.cmd`, `{base}.bat` in addition to
+ * `{base}` so callers can pass POSIX-style candidate paths and still hit
+ * Windows artifacts (bun --compile emits `.exe`, batch wrappers are common).
+ *
+ * Returns the resolved path or null if nothing is executable.
+ *
+ * Why this exists: on Windows, `fs.constants.X_OK` is degraded to an
+ * existence check (the Node docs are explicit:
+ * https://nodejs.org/api/fs.html#fsaccesspath-mode-callback —
+ * "On Windows, the file system accessibility checks can behave differently.
+ * For example, changing visibility using chmod does not update read and
+ * write permissions... Only the presence of the read-only attribute is
+ * reflected."). So `isExecutable("/path/to/browse")` returns false when
+ * the actual file on disk is `/path/to/browse.exe` — the extension probe
+ * is the only thing that closes the gap.
+ */
+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 isExecutable(p: string): boolean {
   try {
     fs.accessSync(p, fs.constants.X_OK);

make-pdf/src/pdftotext.ts

@@ -15,10 +15,14 @@
  *
  * Resolution order for the pdftotext binary:
  *   1. $PDFTOTEXT_BIN env override
- *   2. `which pdftotext` on PATH
- *   3. standard Homebrew paths on macOS
+ *   2. PATH lookup (`which` on POSIX, `where` on Windows)
+ *   3. standard Homebrew / distro paths on macOS/Linux
  *   4. throws a friendly "install poppler" error
  *
+ * On Windows, every probe also tries `.exe` / `.cmd` / `.bat` suffixes,
+ * so users can set `PDFTOTEXT_BIN=C:\tools\poppler\bin\pdftotext` and have
+ * it resolve to `pdftotext.exe` without the extension in the env var.
+ *
  * The wrapper is *optional at runtime*: production renders don't need it.
  * Only the CI gate and unit tests invoke pdftotext.
  */
@@ -46,44 +50,79 @@ export interface PdftotextInfo {
  */
 export function resolvePdftotext(): PdftotextInfo {
   const envOverride = process.env.PDFTOTEXT_BIN;
-  if (envOverride && isExecutable(envOverride)) {
-    return describeBinary(envOverride);
+  if (envOverride) {
+    const resolved = findExecutable(envOverride);
+    if (resolved) return describeBinary(resolved);
   }
 
-  // Try PATH
+  // PATH lookup — `where` on Windows (native, always in System32),
+  // `which` on POSIX. Git Bash provides `which` too, but cmd.exe and
+  // PowerShell don't.
+  const pathLookupCmd = process.platform === "win32" ? "where" : "which";
   try {
-    const which = execFileSync("which", ["pdftotext"], { encoding: "utf8" }).trim();
-    if (which && isExecutable(which)) return describeBinary(which);
+    const out = execFileSync(pathLookupCmd, ["pdftotext"], { encoding: "utf8" }).trim();
+    const firstHit = out.split(/\r?\n/)[0]?.trim();
+    if (firstHit) {
+      const resolved = findExecutable(firstHit);
+      if (resolved) return describeBinary(resolved);
+    }
   } catch {
     // fall through
   }
 
-  // Common macOS Homebrew locations
-  const macCandidates = [
-    "/opt/homebrew/bin/pdftotext",     // Apple Silicon
-    "/usr/local/bin/pdftotext",        // Intel Mac or Linuxbrew
+  // Common POSIX install locations (Homebrew, distro packages).
+  // Windows users rely on PATH + env override; Poppler distributions
+  // on Windows land in too many places to guess (Scoop, Chocolatey,
+  // oschwartz10612/poppler-windows standalone, portable zips).
+  const posixCandidates = [
+    "/opt/homebrew/bin/pdftotext",     // Apple Silicon Homebrew
+    "/usr/local/bin/pdftotext",        // Intel Mac / Linuxbrew
     "/usr/bin/pdftotext",              // distro package
   ];
-  for (const candidate of macCandidates) {
-    if (isExecutable(candidate)) return describeBinary(candidate);
+  for (const candidate of posixCandidates) {
+    const resolved = findExecutable(candidate);
+    if (resolved) return describeBinary(resolved);
   }
 
+  const setCmd = process.platform === "win32"
+    ? '  setx PDFTOTEXT_BIN "C:\\path\\to\\pdftotext.exe"'
+    : "  export PDFTOTEXT_BIN=/path/to/pdftotext";
   throw new PdftotextUnavailableError([
     "pdftotext not found.",
     "",
     "make-pdf needs pdftotext to run the copy-paste CI gate.",
     "(Runtime rendering does NOT need it. This only affects tests.)",
+    `Platform: ${process.platform}`,
     "",
     "To install:",
-    "  macOS:  brew install poppler",
-    "  Ubuntu: sudo apt-get install poppler-utils",
-    "  Fedora: sudo dnf install poppler-utils",
+    "  macOS:    brew install poppler",
+    "  Ubuntu:   sudo apt-get 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:",
-    "  export PDFTOTEXT_BIN=/path/to/pdftotext",
+    setCmd,
   ].join("\n"));
 }
 
+/**
+ * Resolve a base path to an executable, probing platform-specific extensions.
+ * See browseClient.findExecutable for the full rationale — this is a local
+ * duplicate to keep module independence (matches the existing `isExecutable`
+ * duplication pattern in this file and browseClient.ts).
+ */
+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 isExecutable(p: string): boolean {
   try {
     fs.accessSync(p, fs.constants.X_OK);

make-pdf/test/browseClient.test.ts

@@ -5,9 +5,12 @@
  */
 
 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";
 
 describe("resolveBrowseBin", () => {
   test("throws BrowseClientError with setup hint when nothing is found", () => {
@@ -43,12 +46,16 @@ describe("resolveBrowseBin", () => {
 
   test("honors BROWSE_BIN when it points at a real executable", () => {
     const originalEnv = process.env.BROWSE_BIN;
-    // `/bin/sh` exists on every POSIX system and is executable.
-    process.env.BROWSE_BIN = "/bin/sh";
+    // Pick a path that exists and is executable on the current platform.
+    // `/bin/sh` is universal on POSIX; `cmd.exe` ships with every Windows.
+    const realExe = process.platform === "win32"
+      ? "C:\\Windows\\System32\\cmd.exe"
+      : "/bin/sh";
+    process.env.BROWSE_BIN = realExe;
 
     try {
       const resolved = resolveBrowseBin();
-      expect(resolved).toBe("/bin/sh");
+      expect(resolved).toBe(realExe);
     } finally {
       if (originalEnv === undefined) {
         delete process.env.BROWSE_BIN;
@@ -57,6 +64,70 @@ describe("resolveBrowseBin", () => {
       }
     }
   });
+
+  test("on win32, honors BROWSE_BIN pointing at a base path that needs .exe", () => {
+    if (process.platform !== "win32") return;
+    const originalEnv = process.env.BROWSE_BIN;
+    const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "make-pdf-resolve-"));
+    const exePath = path.join(tmpDir, "browse.exe");
+    try {
+      fs.writeFileSync(exePath, "");
+      // Point BROWSE_BIN at the base path WITHOUT .exe — mirrors the real
+      // failure Sam hit with BROWSE_BIN=/c/.../dist/browse when the on-disk
+      // artifact was browse.exe (https://github.com/garrytan/gstack/pull/???).
+      process.env.BROWSE_BIN = path.join(tmpDir, "browse");
+      const resolved = resolveBrowseBin();
+      expect(resolved).toBe(exePath);
+    } finally {
+      if (originalEnv === undefined) delete process.env.BROWSE_BIN;
+      else process.env.BROWSE_BIN = originalEnv;
+      try { fs.unlinkSync(exePath); } catch { /* best-effort */ }
+      try { fs.rmdirSync(tmpDir); } catch { /* best-effort */ }
+    }
+  });
+});
+
+describe("findExecutable", () => {
+  test("returns the path as-is when it's directly executable", () => {
+    const probe = process.platform === "win32"
+      ? "C:\\Windows\\System32\\cmd.exe"
+      : "/bin/sh";
+    expect(findExecutable(probe)).toBe(probe);
+  });
+
+  test("returns null when the path does not exist in any known form", () => {
+    expect(findExecutable("/nonexistent/definitely-not-here")).toBeNull();
+  });
+
+  test("on win32, probes .exe when the bare path is missing", () => {
+    if (process.platform !== "win32") return;
+    const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "make-pdf-findexe-"));
+    const exePath = path.join(tmpDir, "fake-browse.exe");
+    try {
+      fs.writeFileSync(exePath, "");
+      // Base path WITHOUT .exe — exactly how the hardcoded sibling and
+      // global candidates inside resolveBrowseBin probe for the binary.
+      const resolved = findExecutable(path.join(tmpDir, "fake-browse"));
+      expect(resolved).toBe(exePath);
+    } finally {
+      try { fs.unlinkSync(exePath); } catch { /* best-effort */ }
+      try { fs.rmdirSync(tmpDir); } catch { /* best-effort */ }
+    }
+  });
+
+  test("on win32, probes .cmd and .bat as well as .exe", () => {
+    if (process.platform !== "win32") return;
+    const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "make-pdf-findexe-"));
+    const cmdPath = path.join(tmpDir, "wrapper.cmd");
+    try {
+      fs.writeFileSync(cmdPath, "@echo off\r\n");
+      const resolved = findExecutable(path.join(tmpDir, "wrapper"));
+      expect(resolved).toBe(cmdPath);
+    } finally {
+      try { fs.unlinkSync(cmdPath); } catch { /* best-effort */ }
+      try { fs.rmdirSync(tmpDir); } catch { /* best-effort */ }
+    }
+  });
 });
 
 describe("BrowseClientError", () => {

make-pdf/test/pdftotext.test.ts

@@ -7,8 +7,11 @@
  */
 
 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 { normalize, copyPasteGate } from "../src/pdftotext";
+import { normalize, copyPasteGate, resolvePdftotext } from "../src/pdftotext";
 
 describe("normalize", () => {
   test("strips trailing spaces", () => {
@@ -104,3 +107,26 @@ describe("copyPasteGate — assertion logic", () => {
     expect(Math.abs(expectedBreaks - tooManyBreaksNormalized)).toBeLessThanOrEqual(4);
   });
 });
+
+describe("resolvePdftotext", () => {
+  test("on win32, honors PDFTOTEXT_BIN pointing at a base path that needs .exe", () => {
+    if (process.platform !== "win32") return;
+    const originalEnv = process.env.PDFTOTEXT_BIN;
+    const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "pdftotext-resolve-"));
+    const exePath = path.join(tmpDir, "pdftotext.exe");
+    try {
+      // Empty .exe is fine — describeBinary() swallows the non-zero exit
+      // from `bin -v` and returns version: "unknown". We only assert on
+      // the resolved path.
+      fs.writeFileSync(exePath, "");
+      process.env.PDFTOTEXT_BIN = path.join(tmpDir, "pdftotext");
+      const info = resolvePdftotext();
+      expect(info.bin).toBe(exePath);
+    } finally {
+      if (originalEnv === undefined) delete process.env.PDFTOTEXT_BIN;
+      else process.env.PDFTOTEXT_BIN = originalEnv;
+      try { fs.unlinkSync(exePath); } catch { /* best-effort */ }
+      try { fs.rmdirSync(tmpDir); } catch { /* best-effort */ }
+    }
+  });
+});