RomanNum3ral
/
gstack
mirror of https://github.com/garrytan/gstack.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix(make-pdf): Bun.which-based binary resolution for browse + pdftotext on Windows 

Extends v1.24.0.0's Bun.which + GSTACK_*_BIN override pattern (introduced in
browse/src/claude-bin.ts via #1252) to the two other binary resolvers in the
codebase: make-pdf/src/browseClient.ts:resolveBrowseBin and
make-pdf/src/pdftotext.ts:resolvePdftotext.

Same Windows quirks (fs.accessSync(X_OK) degrades to existence-check; `which`
isn't available outside Git Bash; bun --compile --outfile X emits X.exe), same
Bun.which-based fix shape, same env override convention.

Changes:
  - GSTACK_BROWSE_BIN / GSTACK_PDFTOTEXT_BIN as the v1.24-aligned overrides;
    BROWSE_BIN / PDFTOTEXT_BIN remain as back-compat aliases.
  - Bun.which() replaces execFileSync('which', ...) for PATH lookup. Handles
    Windows PATHEXT natively; no more `where`-vs-`which` branch.
  - findExecutable(base) helper exported from each module, probes .exe/.cmd/.bat
    after the bare-path miss on win32. Linux/macOS behavior is bit-identical
    (isExecutable short-circuits before the win32 branch ever runs).
  - macCandidates renamed posixCandidates (always was — /opt/homebrew, /usr/local,
    /usr/bin). No Windows candidates added; Poppler installs scatter across
    Scoop/Chocolatey/portable zips and guessing causes false positives.
  - Error messages get a Windows install hint (scoop install poppler / oschwartz10612)
    and `setx` example for GSTACK_*_BIN.
  - Pre-existing test 'honors BROWSE_BIN when it points at a real executable'
    was hardcoded /bin/sh — made cross-platform via a REAL_EXE constant
    (cmd.exe on win32, /bin/sh on POSIX). Was a Windows-CI blocker on its own.

Coordination: PR #1094 (@BkashJEE) covered browseClient.ts independently with a
narrower scope; this PR's pdftotext + cross-platform tests + GSTACK_*_BIN naming
are additive. Either order of merge works.

Test plan:
  - bun test make-pdf/test/browseClient.test.ts make-pdf/test/pdftotext.test.ts
    on win32 — 29 pass, 0 fail (12 new assertions: findExecutable POSIX/win32/null,
    resolveBrowseBin GSTACK_BROWSE_BIN + BROWSE_BIN + precedence + quote-strip,
    same shape for resolvePdftotext + Windows install hint in error message).
  - POSIX branch unchanged — fs.accessSync(X_OK) on Linux/macOS short-circuits
    before any win32 logic runs, matching the v1.24 claude-bin.ts pattern.
This commit is contained in:
Samuel Carson 2026-05-03 15:50:03 -05:00
parent bf65487162
commit b0c138c545
4 changed files with 329 additions and 88 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

99
make-pdf/src/browseClient.ts
View File

@ -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";
@ -55,15 +63,51 @@ export interface JsOptions {
}
/**
* Locate the browse binary. Throws a BrowseClientError with a
* canonical setup message if not found.
* 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.
*/
export function resolveBrowseBin(): string {
const envOverride = process.env.BROWSE_BIN;
if (envOverride && isExecutable(envOverride)) return envOverride;
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;
}
// 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
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,
@ -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:",
" 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"),
);
}

82
make-pdf/src/pdftotext.ts
View File

@ -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.
@ -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 {
const envOverride = process.env.PDFTOTEXT_BIN;
if (envOverride && isExecutable(envOverride)) {
return describeBinary(envOverride);
}
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);
// Try PATH
try {
const which = execFileSync("which", ["pdftotext"], { encoding: "utf8" }).trim();
if (which && isExecutable(which)) return describeBinary(which);
} catch {
// fall through
}
// 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);
}
@ -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"));
}

133
make-pdf/test/browseClient.test.ts
View File

@ -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.
const originalEnv = process.env.BROWSE_BIN;
process.env.BROWSE_BIN = "/nonexistent/browse-does-not-exist";
// We can't easily mock the sibling and global paths without touching
// 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;
// Point overrides at non-existent paths and clear PATH so Bun.which finds
// nothing. Sibling/global probes go through findExecutable on real paths,
// but the test asserts on the error shape rather than depending on whether
// a real browse install exists on the box.
let thrown: unknown = 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.message).toContain("./setup");
expect(thrown.message).toContain("BROWSE_BIN");
}
// Restore env
if (originalEnv === undefined) {
delete process.env.BROWSE_BIN;
} else {
process.env.BROWSE_BIN = originalEnv;
expect((thrown as BrowseClientError).message).toContain("browse binary not found");
expect((thrown as BrowseClientError).message).toContain("./setup");
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");
}
// 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", () => {
const originalEnv = process.env.BROWSE_BIN;
// `/bin/sh` exists on every POSIX system and is executable.
process.env.BROWSE_BIN = "/bin/sh";
test("honors GSTACK_BROWSE_BIN when it points at a real executable", () => {
const resolved = withEnv({ GSTACK_BROWSE_BIN: REAL_EXE }, () => resolveBrowseBin());
expect(resolved).toBe(REAL_EXE);
});
try {
const resolved = resolveBrowseBin();
expect(resolved).toBe("/bin/sh");
} finally {
if (originalEnv === undefined) {
delete process.env.BROWSE_BIN;
} else {
process.env.BROWSE_BIN = originalEnv;
}
}
test("honors BROWSE_BIN as a back-compat alias", () => {
const resolved = withEnv(
{ GSTACK_BROWSE_BIN: undefined, BROWSE_BIN: REAL_EXE },
() => resolveBrowseBin(),
);
expect(resolved).toBe(REAL_EXE);
});
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);
});
});

103
make-pdf/test/pdftotext.test.ts
View File

@ -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");
}
});
});