fix(make-pdf): reject a directory when resolving the browse binary

`isExecutable()` tested `access(X_OK)` alone. That is TRUE for directories — they
carry the execute/traverse bit on POSIX and pass the check on Windows too — so
binary discovery accepted a directory as the browse binary.

It bites on a stock global install. `~/.claude/skills/browse` is the skill's own
docs folder and contains nothing but SKILL.md, but it sits on one of the probed
paths, passes X_OK, and wins. From then on every browse invocation runs a
directory as a program and fails with an EMPTY error string, which make-pdf
surfaces as:

    [1/5] Checking browse binary... OK (C:\Users\...\.claude\skills\browse)
    [2/5] Launching Chromium... FAIL
    Chromium failed to launch: browse newtab exited 1:

Note the first line reports the wrong path as OK, so the output actively points
away from the cause. Setting GSTACK_BROWSE_BIN worked around it, which made it
look like a discovery-order problem rather than a type-check problem.

Fix is one `statSync(p).isFile()` before the access check.

Tests: 2 added, both failing before this change and passing after. The first
asserts the precondition explicitly — that the directory really does pass
`access(X_OK)` — so the test documents WHY the bare check was wrong rather than
just pinning the new behaviour. The second reproduces the exact shape: a
directory named `browse` containing a SKILL.md.
This commit is contained in:
Jake Wilk 2026-08-12 16:37:24 -04:00
parent d078622b73
commit 699c0f48db
2 changed files with 40 additions and 0 deletions

View File

@ -158,6 +158,11 @@ export function resolveBrowseBin(env: NodeJS.ProcessEnv = process.env): string {
function isExecutable(p: string): boolean {
try {
// Must be a regular FILE. access(X_OK) alone is true for directories — they carry the
// execute/traverse bit on POSIX and pass the Windows check too — so discovery happily
// "found" ~/.claude/skills/browse, which is the skill's docs folder containing nothing
// but SKILL.md, and returned a directory as the browse binary.
if (!fs.statSync(p).isFile()) return false;
fs.accessSync(p, fs.constants.X_OK);
return true;
} catch {

View File

@ -59,6 +59,41 @@ describe("findExecutable", () => {
const found = findExecutable("/nonexistent/path/to/nothing");
expect(found).toBeNull();
});
// access(X_OK) is TRUE for directories — they carry the execute/traverse bit — so a
// bare X_OK test returned ~/.claude/skills/browse, the skill's docs folder, as "the
// browse binary". Every browse call then failed with an empty error, which surfaced
// as make-pdf reporting "Chromium failed to launch".
test("rejects a DIRECTORY even though it passes access(X_OK)", () => {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), "mkpdf-dir-"));
try {
// Prove the precondition: the directory really does pass the old test.
let passesXok = true;
try {
fs.accessSync(dir, fs.constants.X_OK);
} catch {
passesXok = false;
}
expect(passesXok).toBe(true);
expect(findExecutable(dir)).toBeNull();
} finally {
fs.rmSync(dir, { recursive: true, force: true });
}
});
test("rejects a directory that shadows a real binary name", () => {
// The exact shape of the bug: a directory named like the thing being looked for.
const base = fs.mkdtempSync(path.join(os.tmpdir(), "mkpdf-shadow-"));
const shadow = path.join(base, "browse");
fs.mkdirSync(shadow);
fs.writeFileSync(path.join(shadow, "SKILL.md"), "# not a binary\n");
try {
expect(findExecutable(shadow)).toBeNull();
} finally {
fs.rmSync(base, { recursive: true, force: true });
}
});
});
describe("resolveBrowseBin", () => {