diff --git a/make-pdf/src/browseClient.ts b/make-pdf/src/browseClient.ts index 9284590731..fe1dda97b0 100644 --- a/make-pdf/src/browseClient.ts +++ b/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,22 +74,35 @@ 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", @@ -94,21 +110,52 @@ export function resolveBrowseBin(): string { "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); diff --git a/make-pdf/src/pdftotext.ts b/make-pdf/src/pdftotext.ts index 33e79fc64c..8198b217e1 100644 --- a/make-pdf/src/pdftotext.ts +++ b/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); diff --git a/make-pdf/test/browseClient.test.ts b/make-pdf/test/browseClient.test.ts index b3459713a3..f6a4ac2f10 100644 --- a/make-pdf/test/browseClient.test.ts +++ b/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", () => { diff --git a/make-pdf/test/pdftotext.test.ts b/make-pdf/test/pdftotext.test.ts index cfeebd14fb..aec0c60252 100644 --- a/make-pdf/test/pdftotext.test.ts +++ b/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 */ } + } + }); +});