From 705996b01c6860a83d78ecf7e76159eaf5fd9eec Mon Sep 17 00:00:00 2001 From: Samuel Carson Date: Tue, 21 Apr 2026 01:21:32 -0500 Subject: [PATCH 1/3] fix(make-pdf): resolve bun-compiled .exe binaries on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resolveBrowseBin()` probed bare paths (`browse`, `$selfDir/../browse/dist/browse`, `~/.claude/skills/gstack/browse/dist/browse`) with `fs.accessSync(path, X_OK)`. On Windows, bun's `--compile --outfile browse/dist/browse` lands the artifact at `browse/dist/browse.exe`, and `fs.constants.X_OK` is degraded to an existence check (Node docs: "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 every probe returned false even though the binary was present, and the tool errored with `browse binary not found` unless the user set `BROWSE_BIN=/c/.../dist/browse.exe` explicitly. Fix: a new exported helper `findExecutable(base)` returns the resolved path, probing `.exe`/`.cmd`/`.bat` suffixes on win32. Every call site in `resolveBrowseBin` now routes through it. The POSIX path is semantically unchanged — `isExecutable(base)` runs first and returns the base as-is when it exists, so Linux/macOS callers see no behavior change. Also: the final PATH fallback ran `which browse`, which exists under Git Bash but not in cmd.exe or PowerShell. On win32 we now shell out to `where` (always in `System32`, so it resolves without Git Bash in PATH). POSIX still uses `which`. Regression tests cover the three moving parts: the `findExecutable` helper directly (baseline, missing path, `.exe` probe, `.cmd`/`.bat` probe), and the `resolveBrowseBin` env-override path on win32 with a base path that needs `.exe`. The pre-existing "honors BROWSE_BIN" test hardcoded `/bin/sh` and always failed on win32 — made cross-platform with `cmd.exe` as the Windows probe target, so Windows CI (currently commented out in .github/workflows/make-pdf-gate.yml) won't trip on this when enabled. Verified end-to-end on win32: with BROWSE_BIN unset, `bun run make-pdf/src/cli.ts generate in.md out.pdf` now resolves `browse.exe` via the global path probe and produces a valid 31KB PDF. Co-Authored-By: Claude Opus 4.7 (1M context) --- make-pdf/src/browseClient.ts | 59 +++++++++++++++++++--- make-pdf/test/browseClient.test.ts | 79 ++++++++++++++++++++++++++++-- 2 files changed, 126 insertions(+), 12 deletions(-) diff --git a/make-pdf/src/browseClient.ts b/make-pdf/src/browseClient.ts index 9284590731..11df9434a3 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,20 +74,30 @@ 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 } throw new BrowseClientError( @@ -98,7 +111,7 @@ export function resolveBrowseBin(): string { ` - $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", @@ -109,6 +122,36 @@ export function resolveBrowseBin(): string { ); } +/** + * 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/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", () => { From f04bbd1c48abc245cea1d7177ddb4704d199bcbe Mon Sep 17 00:00:00 2001 From: Samuel Carson Date: Tue, 21 Apr 2026 01:21:53 -0500 Subject: [PATCH 2/3] fix(make-pdf): apply .exe extension probe to pdftotext resolver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same root cause as the previous commit, smaller blast radius. The pdftotext wrapper only runs in the copy-paste CI gate and unit tests (never in production renders), so Windows users don't hit it at runtime — but anyone running `bun test make-pdf/` on Windows with poppler installed via Scoop or Chocolatey and `PDFTOTEXT_BIN=C:\tools\poppler\bin\pdftotext` set would have fallen through to `PdftotextUnavailableError` because the on-disk artifact is `pdftotext.exe`. Changes mirror `browseClient.ts`: 1. File-local `findExecutable(base)` helper that probes `.exe`/`.cmd`/`.bat` on win32. Duplicated rather than shared with browseClient to keep the existing module-independence convention (both files already duplicate `isExecutable`). 2. `which pdftotext` PATH lookup becomes `where pdftotext` on win32, so users running tests from cmd.exe or PowerShell (no Git Bash in PATH) still get PATH resolution. 3. `macCandidates` renamed `posixCandidates` — it was always a POSIX-only set (`/opt/homebrew/bin`, `/usr/local/bin`, `/usr/bin`). No Windows candidates added: Poppler on Windows scatters across Scoop, Chocolatey, oschwartz10612/poppler-windows releases, and portable zips — guessing causes false positives. Users with poppler outside PATH set the env var, which now works. 4. Error message picks up a Windows install hint: `scoop install poppler` + the oschwartz10612/poppler-windows link. Regression test: env-override probe with a base path that needs `.exe`, guarded to win32 with early-return on POSIX. Empty `.exe` is sufficient because `describeBinary()` catches the non-zero exit from `bin -v` and records `version: "unknown"` — the test asserts only on the resolved path. Co-Authored-By: Claude Opus 4.7 (1M context) --- make-pdf/src/pdftotext.ts | 67 +++++++++++++++++++++++++-------- make-pdf/test/pdftotext.test.ts | 28 +++++++++++++- 2 files changed, 78 insertions(+), 17 deletions(-) diff --git a/make-pdf/src/pdftotext.ts b/make-pdf/src/pdftotext.ts index 33e79fc64c..10eadf2f36 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,26 +50,38 @@ 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); } throw new PdftotextUnavailableError([ @@ -75,15 +91,34 @@ 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", ].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/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 */ } + } + }); +}); From 7b13d190ea43b8f5ce5ecc84c566397a84acf041 Mon Sep 17 00:00:00 2001 From: Samuel Carson Date: Tue, 21 Apr 2026 01:58:08 -0500 Subject: [PATCH 3/3] feat(make-pdf): Windows-aware `setx` hint in resolver error messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adopt the platform-aware error-message UX from @BkashJEE's independent concurrent fix (#1094). On Windows, `export FOO=...` is the wrong syntax — users want `setx FOO "..."` for a persistent environment variable in cmd.exe/PowerShell, or `$env:FOO = "..."` for session-local PowerShell. `setx` is the closest one-liner equivalent and matches what Bikash proposed. Also adds `Platform: ` to both error messages — tiny but useful for bug reports, especially given both fix sites (browse and pdftotext) have subtle platform-specific behavior. Credit: @BkashJEE for the `setx` hint shape in #1094. Co-Authored-By: Claude Opus 4.7 (1M context) --- make-pdf/src/browseClient.ts | 6 +++++- make-pdf/src/pdftotext.ts | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/make-pdf/src/browseClient.ts b/make-pdf/src/browseClient.ts index 11df9434a3..fe1dda97b0 100644 --- a/make-pdf/src/browseClient.ts +++ b/make-pdf/src/browseClient.ts @@ -100,6 +100,9 @@ export function resolveBrowseBin(): string { // 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", @@ -107,6 +110,7 @@ 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(", ")}`, @@ -117,7 +121,7 @@ export function resolveBrowseBin(): string { " cd ~/.claude/skills/gstack && ./setup", "", "Or set BROWSE_BIN explicitly:", - " export BROWSE_BIN=/path/to/browse", + setCmd, ].join("\n"), ); } diff --git a/make-pdf/src/pdftotext.ts b/make-pdf/src/pdftotext.ts index 10eadf2f36..8198b217e1 100644 --- a/make-pdf/src/pdftotext.ts +++ b/make-pdf/src/pdftotext.ts @@ -84,11 +84,15 @@ export function resolvePdftotext(): PdftotextInfo { 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", @@ -98,7 +102,7 @@ export function resolvePdftotext(): PdftotextInfo { " https://github.com/oschwartz10612/poppler-windows)", "", "Or set PDFTOTEXT_BIN to an explicit path:", - " export PDFTOTEXT_BIN=/path/to/pdftotext", + setCmd, ].join("\n")); }