fix(make-pdf): resolve browse binary on Windows without BROWSE_BIN#1094
fix(make-pdf): resolve browse binary on Windows without BROWSE_BIN#1094BkashJEE wants to merge 1 commit intogarrytan:mainfrom
Conversation
…_BIN The binary resolution chain in `make-pdf/src/browseClient.ts` hardcoded the Unix binary name `browse` and the Unix lookup tool `which`. On Windows the compiled binary is `browse.exe`, so every fallback (sibling paths, global install at `~/.claude/skills/gstack/browse/dist/browse`, PATH lookup) missed, and users hit a cryptic "browse binary not found" error unless they knew to set `BROWSE_BIN` manually. Changes: - Branch binary name on `process.platform === "win32"` → append `.exe`. - Use `where` on Windows, `which` on Unix; parse first line of `where` output (can be multi-line across PATHEXT hits). - `isExecutable()` checks `F_OK` on Windows because NTFS has no execute bit and `X_OK` is unreliable (Node delegates to `AccessCheck` which can false-negative on .exe files depending on ACLs). - Error message now shows the actual binary name being searched and suggests `setx BROWSE_BIN ...` on Windows instead of `export`. Verified on Windows 11 + Git Bash: - Before patch: `pdf.exe generate ...` fails with "browse binary not found" unless `BROWSE_BIN` is set explicitly via `setx`. - After patch: succeeds with empty `BROWSE_BIN`, finds `~/.claude/skills/gstack/browse/dist/browse.exe` via the global path branch. Rendered a 51KB smoke-test PDF end-to-end. No behavior change on macOS/Linux (suffix is empty string, lookup tool stays `which`, permission check stays `X_OK`).
AI Code Trust — ship readinessScore: 100 · Verdict: SAFE · Model: deterministic-v1 No blocking issues detected by automated checks. |
Adopt the platform-aware error-message UX from @BkashJEE's independent concurrent fix (garrytan#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: <process.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 garrytan#1094. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Hey @BkashJEE — just noticed this after I'd already prepped a fix for the same bug at #1118. You're the first-mover on the diagnosis by a day, and the Cross-linking for the reviewer's convenience. Our two PRs overlap on the core diagnosis (Node's Mine goes a bit wider on scope: applies the same fix to Nice catch on the diagnosis. |
|
For context: opened #1122 today ("Boiling the Windows CI Lake") proposing a Phase-1 Windows smoke CI that would have caught this class of bug at PR time. This PR is cited in the RFC's coverage table as one of the five recent examples that motivated it — not asking for anything on this PR, it can merge on its own merits. Cross-linking because the "Why this shipped unnoticed" paragraph above reads differently with a systemic answer in flight. Also a companion issue at #1123 for higher-level discussion if the PR's review thread gets crowded. |
Summary
make-pdf/src/browseClient.tsresolveBrowseBin()hardcoded the Unix binary namebrowseand the Unixwhichlookup tool. On Windows the compiled binary isbrowse.exe, so every fallback (sibling paths, global install at~/.claude/skills/gstack/browse/dist/browse, PATH lookup) missed — users hitbrowse binary not foundunless they setBROWSE_BINmanually.This PR makes every fallback Windows-aware while keeping Unix behavior unchanged.
Changes
process.platform === "win32"→ append.exe.whereon Windows,whichon Unix; parse first line ofwhereoutput (can span multiple PATHEXT hits).isExecutable()checksF_OKon Windows — NTFS has no execute bit and Node'sX_OKdelegates toAccessCheck, which false-negatives on.exefiles depending on ACLs.setx BROWSE_BIN ...on Windows instead ofexport.Verification
Environment: Windows 11 Pro + Git Bash + gstack v1.4.0.0 (main).
Before patch: with
BROWSE_BINunset,pdf.exe generate smoke.md out.pdffails:Root cause:
fs.accessSync("~/.claude/skills/gstack/browse/dist/browse", X_OK)throws ENOENT because the actual file isbrowse.exe.After patch: same command succeeds. Binary resolved via the global-install branch. Generated a 51KB single-page PDF end-to-end. Full PDF pipeline validated (cover + TOC + paged.js render + pdftotext QA) in a separate run against
~/.claude/CLAUDE.md(10 pages, 208KB).No platform-conditional test added — the change is a pure cross-platform branching patch with the same semantics on macOS/Linux (empty suffix,
which,X_OK).Test plan
pdf.exe generatewithBROWSE_BINunset succeedsbrowse.exeandsetxwhen binary truly missingwhich,X_OKpreserved — request reviewer run existing test suite)Context
Discovered while dogfooding
/make-pdfon Windows the day after the v1.4.0.0 release. Without this fix, the skill is effectively blocked on Windows until users know to runsetx BROWSE_BIN "C:\path\to\browse.exe"themselves — not documented inSKILL.md.