Skip to content

fix(make-pdf): Bun.which-based binary resolution for browse + pdftotext on Windows (v1.24 extension) #1307

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
scarson wants to merge 1 commit into from
+327 −86
Closed

fix(make-pdf): Bun.which-based binary resolution for browse + pdftotext on Windows (v1.24 extension) #1307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 72 additions & 25 deletions make-pdf/src/browseClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
* 1. $BROWSE_BIN env override
* 2. sibling dir: dirname(argv[0])/../browse/dist/browse
* 3. ~/.claude/skills/gstack/browse/dist/browse
* 4. PATH lookup: `browse`
* 5. error with setup hint
* Binary resolution order (Codex round 2 #4, v1.24-aligned):
* 1. $GSTACK_BROWSE_BIN env override (preferred, matches v1.24 GSTACK_*_BIN pattern)
* 2. $BROWSE_BIN env override (back-compat alias)
* 3. sibling dir: dirname(argv[0])/../browse/dist/browse[.exe]
* 4. ~/.claude/skills/gstack/browse/dist/browse[.exe]
* 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";
Expand Down Expand Up @@ -54,38 +62,74 @@ export interface JsOptions {
expression: string; // JS expression to evaluate
}

/**
* Resolve an absolute or PATH-resolvable command via Bun.which-style semantics,
* 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.
*/
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;
}

/**
* 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.
* canonical setup message if not found. See header for resolution order.
*/
export function resolveBrowseBin(): string {
const envOverride = process.env.BROWSE_BIN;
if (envOverride && isExecutable(envOverride)) return envOverride;
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;

// Sibling: look relative to this process's binary
// (for when make-pdf and browse live next to each other in dist/)
// 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"),
path.resolve(selfDir, "../../browse/dist/browse"),
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
try {
const which = execFileSync("which", ["browse"], { encoding: "utf8" }).trim();
if (which && isExecutable(which)) return which;
} catch {
// `which` exited non-zero; fall through to error
}
// 5: PATH lookup via Bun.which — handles Windows PATHEXT natively (no `which`
// dependency on cmd.exe / PowerShell, no `where`-vs-`which` branch).
const PATH = env.PATH ?? env.Path ?? '';
const onPath = Bun.which('browse', { PATH });
if (onPath) return onPath;

throw new BrowseClientError(
/* exitCode */ 127,
Expand All @@ -95,16 +139,19 @@ 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`",
"",
"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",
"Or set GSTACK_BROWSE_BIN explicitly:",
process.platform === "win32"
? ' setx GSTACK_BROWSE_BIN "C:\\path\\to\\browse.exe"'
: " export GSTACK_BROWSE_BIN=/path/to/browse",
].join("\n"),
);
}
Expand Down
82 changes: 56 additions & 26 deletions make-pdf/src/pdftotext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
* 1. $PDFTOTEXT_BIN env override
* 2. `which pdftotext` on PATH
* 3. standard Homebrew paths on macOS
* 4. throws a friendly "install poppler" error
* Resolution order for the pdftotext binary (v1.24-aligned):
* 1. $GSTACK_PDFTOTEXT_BIN env override (preferred, matches v1.24 GSTACK_*_BIN pattern)
* 2. $PDFTOTEXT_BIN env override (back-compat alias)
* 3. PATH lookup via Bun.which('pdftotext') — handles Windows PATHEXT natively
* 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.
Expand All @@ -42,29 +45,52 @@ export interface PdftotextInfo {
}

/**
* Locate pdftotext. Throws PdftotextUnavailableError if none is found.
* 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 resolvePdftotext(): PdftotextInfo {
const envOverride = process.env.PDFTOTEXT_BIN;
if (envOverride && isExecutable(envOverride)) {
return describeBinary(envOverride);
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;
}

// Try PATH
try {
const which = execFileSync("which", ["pdftotext"], { encoding: "utf8" }).trim();
if (which && isExecutable(which)) return describeBinary(which);
} catch {
// fall through
}
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(env: NodeJS.ProcessEnv = process.env): PdftotextInfo {
// 1 + 2: env overrides (GSTACK_PDFTOTEXT_BIN preferred, PDFTOTEXT_BIN back-compat).
const overrideRaw = env.GSTACK_PDFTOTEXT_BIN ?? env.PDFTOTEXT_BIN;
const override = resolveOverride(overrideRaw, env);
if (override) return describeBinary(override);

// 3: PATH lookup via Bun.which — handles Windows PATHEXT natively.
const PATH = env.PATH ?? env.Path ?? '';
const onPath = Bun.which('pdftotext', { PATH });
if (onPath) return describeBinary(onPath);

// Common macOS Homebrew locations
const macCandidates = [
"/opt/homebrew/bin/pdftotext", // Apple Silicon
// 4: POSIX-only standard locations. No Windows candidates — Poppler installs
// scatter across Scoop/Chocolatey/portable zips and guessing causes false
// 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);
}

Expand All @@ -75,12 +101,16 @@ export function resolvePdftotext(): PdftotextInfo {
"(Runtime rendering does NOT need it. This only affects tests.)",
"",
"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",
"Or set GSTACK_PDFTOTEXT_BIN to an explicit path:",
process.platform === "win32"
? ' setx GSTACK_PDFTOTEXT_BIN "C:\\path\\to\\pdftotext.exe"'
: " export GSTACK_PDFTOTEXT_BIN=/path/to/pdftotext",
].join("\n"));
}

Expand Down
Loading