From f78a4b3b809cfa77735ec080bc0b4f572d8b5585 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 14 Aug 2026 02:34:29 -0400 Subject: [PATCH] fix(#3275): resolve reviewer lane binaries via shared pathext scan (#3445) * fix(#3275): resolve reviewer lane binaries via shared pathext scan * fix(#3275): pin changeset fragment to pr 3445 * fix(#3275): stage p4 resolver fixture in pathext casing --------- Co-authored-by: sim --- .changeset/quick-finches-snooze.md | 5 + gsd-core/bin/gsd-tools.cjs | 97 +++++-- ...iew-lane-windows-spawn-resolution.test.cjs | 268 ++++++++++++++++++ 3 files changed, 349 insertions(+), 21 deletions(-) create mode 100644 .changeset/quick-finches-snooze.md create mode 100644 tests/review-lane-windows-spawn-resolution.test.cjs diff --git a/.changeset/quick-finches-snooze.md b/.changeset/quick-finches-snooze.md new file mode 100644 index 000000000..f573a0ccd --- /dev/null +++ b/.changeset/quick-finches-snooze.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3445 +--- +Cross-AI reviewer lanes now run on Windows: the declared bare CLI name is resolved through one shared PATH+PATHEXT lookup before spawning, so npm-installed .cmd/.bat shims start via cmd.exe mediation instead of failing with spawn ENOENT (#3275). diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index af6f17376..7e734a879 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -1386,10 +1386,19 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load // ENOENT (CreateProcess cannot start .cmd). Apply the same #2667 shim // gate used in runWithTimeout: detect .cmd/.bat and mediate through // cmd.exe /d /s /c with an explicit argv array (no shell:true). + // + // #3275: descriptors declare BARE names, so the gate above never saw an + // extension — resolve through the shared PATH+PATHEXT resolver FIRST + // (the same one `hasBinary` uses, so probe and spawn can never disagree + // about what the lane's binary is). POSIX keeps the bare name: Node's own + // PATH search already worked there, and the #3275 acceptance contract + // holds macOS/Linux behavior unchanged. A name that resolves to nothing + // falls back to the declared name so the ENOENT still surfaces (#3086). const isWin = process.platform === 'win32'; - const winShim = isWin && /\.(cmd|bat)$/i.test(path.basename(binary)); - const spawnBinary = winShim ? (process.env.ComSpec || 'cmd.exe') : binary; - const spawnArgv = winShim ? ['/d', '/s', '/c', binary, ...argv] : argv; + const target = isWin ? (resolveSpawnBinary(binary) || binary) : binary; + const winShim = isWin && /\.(cmd|bat)$/i.test(path.basename(target)); + const spawnBinary = winShim ? (process.env.ComSpec || 'cmd.exe') : target; + const spawnArgv = winShim ? ['/d', '/s', '/c', target, ...argv] : argv; const r = cp.spawnSync(spawnBinary, spawnArgv, { input: opts.input, encoding: 'utf8', @@ -1429,24 +1438,13 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load // all (a probe that costs a process is a probe you avoid running, which is how the original // Kimi probe ended up unbounded), and `shell: true` with an args array is deprecated in // Node 26 (DEP0190) because the arguments are concatenated rather than escaped. - hasBinary: (name) => { - if (!name || name.includes('/') || name.includes('\\')) { - try { return fsx.statSync(name).isFile(); } catch { return false; } - } - const exts = process.platform === 'win32' - ? (process.env.PATHEXT || '.EXE;.CMD;.BAT;.COM').split(';').filter(Boolean) - : ['']; - for (const dir of (process.env.PATH || '').split(path.delimiter).filter(Boolean)) { - for (const ext of exts) { - const candidate = path.join(dir, name + ext); - try { - const st = fsx.statSync(candidate); - if (st.isFile()) return true; - } catch { /* next candidate */ } - } - } - return false; - }, + // + // #3275: the scan lives in `resolveSpawnBinary` now, SHARED with `deps.spawn` + // above. Two private copies of "what is this declared binary?" is how the + // defect hid: the probe resolved WITH PATHEXT while spawn resolved WITHOUT, + // so a lane reported available for a spawn that could never start. One + // resolver, both seams — if one changes, the other changes with it. + hasBinary: (name) => resolveSpawnBinary(name) !== null, configGet, homeDir: os.homedir(), warn: (m) => process.stderr.write(`${m}\n`), @@ -3558,6 +3556,60 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load } +/** + * #3275: resolve a DECLARED command name to the file a spawn can actually start. + * + * Lane descriptors (src/review-lane-descriptor.cts) declare BARE, platform-unaware + * binary names ('codex', 'gemini', 'kimi', 'agy'), and `review-lane invoke`'s + * `deps.spawn` + `deps.hasBinary` both need the on-disk form of that name. Before + * this helper existed they disagreed: `hasBinary` scanned PATH WITH PATHEXT (so + * probes reported lanes AVAILABLE on Windows) while `spawn` received the bare name + * — `CreateProcess` performs no PATHEXT resolution, so every spawn-transport lane + * ENOENT'd there, and the #3086 `.cmd`/`.bat` cmd.exe mediation gate never fired + * because the declared name never carried an extension. One shared resolver is the + * only shape that cannot drift back apart. + * + * win32: tries PATHEXT entries ONLY — never the bare name. npm global installs + * drop an EXTENSIONLESS POSIX sh shim (`...\npm\codex`) next to `codex.CMD`; a + * bare-name-first scan resolves to it, the mediation gate sees no `.cmd`, and the + * ENOENT returns unchanged (field-reported on Windows 11 — see the #3275 issue + * comment pinning exactly this pitfall). + * + * POSIX: answers EXISTENCE only (the old `hasBinary` contract, preserved + * byte-for-byte) by scanning PATH for the bare name. `deps.spawn` does NOT consult + * this on POSIX — the bare name goes to spawnSync unchanged and Node's own PATH + * search does the work, so macOS/Linux behavior is untouched (#3275 acceptance). + * + * Path-like names (any '/' or '\') bypass the PATH scan: the name is already an + * address, so it passes through when the file exists and is a file. + */ +function resolveSpawnBinary(name, platform = process.platform, env = process.env) { + if (!name) return null; + if (name.includes('/') || name.includes('\\')) { + try { return fs.statSync(name).isFile() ? name : null; } catch { return null; } + } + const segments = String(env.PATH || '').split(path.delimiter).filter(Boolean); + if (platform !== 'win32') { + for (const dir of segments) { + const candidate = path.join(dir, name); + try { + if (fs.statSync(candidate).isFile()) return candidate; + } catch { /* next candidate */ } + } + return null; + } + const exts = String(env.PATHEXT || '.EXE;.CMD;.BAT;.COM').split(';').filter(Boolean); + for (const dir of segments) { + for (const ext of exts) { + const candidate = path.join(dir, name + ext); + try { + if (fs.statSync(candidate).isFile()) return candidate; + } catch { /* next candidate */ } + } + } + return null; +} + const HOST_COMMAND_ROUTERS = { // Each entry wraps its `route*Command` router so it receives the module-scope // lib the old `case` arm passed, plus the per-dispatch context @@ -4294,5 +4346,8 @@ module.exports = { TOP_LEVEL_USAGE, skipsRootResolution, resolveMainWorktreeCwd, + // #3275: exported for tests — the shared PATH+PATHEXT resolver behind + // review-lane invoke's `deps.spawn` / `deps.hasBinary` seams. + resolveSpawnBinary, }; diff --git a/tests/review-lane-windows-spawn-resolution.test.cjs b/tests/review-lane-windows-spawn-resolution.test.cjs new file mode 100644 index 000000000..2b93d3a85 --- /dev/null +++ b/tests/review-lane-windows-spawn-resolution.test.cjs @@ -0,0 +1,268 @@ +'use strict'; + +/** + * Regression test for #3275 — reviewer lanes fail with spawn ENOENT on Windows. + * + * Root cause: `routeReviewLane`'s `deps.spawn` (gsd-core/bin/gsd-tools.cjs) mediate + * Windows `.cmd`/`.bat` shims through cmd.exe only when the binary name ALREADY + * carries the extension, but lane descriptors declare BARE names ('codex', 'kimi', + * 'agy') and nothing resolved them to the on-disk extensioned form — while + * `deps.hasBinary` scanned PATH WITH PATHEXT, so probes reported the lane + * available for a spawn that could never start. + * + * The fix gives both seams ONE shared resolver, `resolveSpawnBinary`, exported + * from gsd-tools.cjs. This file exercises: + * - the resolver itself, portably, against staged fake-bin dirs (P1–P8); + * - the real CLI end-to-end on Windows: staged `codex.CMD` (primary invocation + * path, file-arg) and `kimi.CMD` (capability-probe path) invoked through + * `gsd-tools review-lane invoke` with the staged dir first on PATH (W1–W2). + * + * The E2E cases stage an extensionless POSIX sh shim NEXT to each `.CMD` on + * purpose: an `['', ...PATHEXT]` resolver resolves to it and re-opens the exact + * ENOENT (field-reported on Windows 11), so its presence pins the ordering. + * + * Matrix: .gsd/bug/fix-3275-reviewer-lanes-enoent-windows/50-test-matrix.md + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const { runGsdTools, createTempDir, cleanup, TOOLS_PATH } = require('./helpers.cjs'); +const { resolveSpawnBinary } = require('../gsd-core/bin/gsd-tools.cjs'); + +const WIN32 = { PATH: '', PATHEXT: '.COM;.EXE;.BAT;.CMD' }; +const POSIX = { PATH: '' }; + +/** Stage a fake bin dir: entries are written verbatim (name → content). */ +function stageBin(dir, entries) { + fs.mkdirSync(dir, { recursive: true }); + for (const [name, content] of Object.entries(entries)) { + fs.writeFileSync(path.join(dir, name), content); + } +} + +describe('resolveSpawnBinary (#3275)', () => { + test('is exported as a function', () => { + assert.equal(typeof resolveSpawnBinary, 'function'); + }); + + test('P1: win32 resolves the .CMD shim, never the extensionless POSIX sh shim beside it', () => { + const dir = createTempDir('gsd-3275-p1-'); + try { + stageBin(dir, { + codex: '#!/bin/sh\nexec node "$0".js "$@"\n', + 'codex.CMD': '@echo off\r\nexit /b 0\r\n', + }); + const resolved = resolveSpawnBinary('codex', 'win32', { ...WIN32, PATH: dir }); + assert.equal(resolved, path.join(dir, 'codex.CMD')); + // The resolution must carry the extension the cmd.exe mediation gate keys on. + assert.match(path.basename(resolved || ''), /\.(cmd|bat)$/i); + } finally { + cleanup(dir); + } + }); + + test('P2: win32 returns null when only the extensionless sh shim exists', () => { + const dir = createTempDir('gsd-3275-p2-'); + try { + stageBin(dir, { codex: '#!/bin/sh\nexit 0\n' }); + assert.equal(resolveSpawnBinary('codex', 'win32', { ...WIN32, PATH: dir }), null); + } finally { + cleanup(dir); + } + }); + + test('P3: win32 honors PATHEXT ORDER, not just membership', () => { + const dir = createTempDir('gsd-3275-p3-'); + try { + stageBin(dir, { 'gemini.CMD': '@echo off\r\n', 'gemini.EXE': 'exe' }); + const resolved = resolveSpawnBinary('gemini', 'win32', { PATH: dir, PATHEXT: '.CMD;.EXE' }); + assert.equal(resolved, path.join(dir, 'gemini.CMD')); + const flipped = resolveSpawnBinary('gemini', 'win32', { PATH: dir, PATHEXT: '.EXE;.CMD' }); + assert.equal(flipped, path.join(dir, 'gemini.EXE')); + } finally { + cleanup(dir); + } + }); + + test('P4: win32 scans PATH dirs in order across multiple entries', () => { + const early = createTempDir('gsd-3275-p4a-'); + const late = createTempDir('gsd-3275-p4b-'); + try { + // Staged in PATHEXT casing: the resolver probes `name + ext` verbatim, and a + // case-SENSITIVE CI filesystem (Linux ext4) must find it exactly as a + // case-insensitive Windows one would. + stageBin(late, { 'tool.EXE': 'exe' }); + const resolved = resolveSpawnBinary('tool', 'win32', { + PATH: [early, late].join(path.delimiter), + PATHEXT: '.EXE;.CMD', + }); + assert.equal(resolved, path.join(late, 'tool.EXE')); + } finally { + cleanup(early); + cleanup(late); + } + }); + + test('P5: win32 returns null when the name exists nowhere on PATH', () => { + const dir = createTempDir('gsd-3275-p5-'); + try { + stageBin(dir, {}); + assert.equal( + resolveSpawnBinary('definitely-not-installed-3275', 'win32', { ...WIN32, PATH: dir }), + null, + ); + } finally { + cleanup(dir); + } + }); + + test('P6: a path-like name bypasses the PATH scan — existing file passes through, missing is null', () => { + const dir = createTempDir('gsd-3275-p6-'); + try { + const file = path.join(dir, 'tool.CMD'); + fs.writeFileSync(file, '@echo off\r\n'); + assert.equal(resolveSpawnBinary(file, 'win32', WIN32), file); + assert.equal(resolveSpawnBinary(path.join(dir, 'absent.CMD'), 'win32', WIN32), null); + } finally { + cleanup(dir); + } + }); + + test('P7: POSIX existence semantics — bare name found in a PATH dir, null when absent', () => { + const dir = createTempDir('gsd-3275-p7-'); + try { + stageBin(dir, { codex: '#!/bin/sh\nexit 0\n' }); + assert.equal(resolveSpawnBinary('codex', 'darwin', { ...POSIX, PATH: dir }), path.join(dir, 'codex')); + assert.equal(resolveSpawnBinary('codex', 'linux', POSIX), null); + } finally { + cleanup(dir); + } + }); + + test('P8: an empty name resolves to null on every platform', () => { + assert.equal(resolveSpawnBinary('', 'win32', WIN32), null); + assert.equal(resolveSpawnBinary('', 'darwin', POSIX), null); + }); +}); + +// ──────────────────────────────────────────────────────────────────────── +// Windows-gated end-to-end: the REAL CLI seam (no injected deps), staged +// shims first on PATH, mirroring how `npm install -g` lays down CLIs. +// Pre-fix, W1 stubs with `[spawn error: ENOENT]` and W2 fails its +// capability probe — the two symptoms the issue reports. +// ──────────────────────────────────────────────────────────────────────── +describe('review-lane invoke on Windows (#3275)', () => { + const isWin = process.platform === 'win32'; + + /** PATH env key as spelled on this process (Windows env keys are case-folded). */ + function withStagedPath(binDir) { + const pathKey = + Object.keys(process.env).find((k) => k.toLowerCase() === 'path') || 'PATH'; + return { + [pathKey]: [binDir, process.env[pathKey]].filter(Boolean).join(path.delimiter), + }; + } + + /** Run the real `gsd-tools review-lane invoke` for one slug and parse its JSON result. */ + function invokeLane({ slug, binDir, runDir, repoRoot }) { + const r = runGsdTools( + [ + 'review-lane', 'invoke', + '--selected', slug, + '--slug', slug, + '--run-dir', runDir, + '--repo-root', repoRoot, + '--explicit', + ], + repoRoot, + withStagedPath(binDir), + ); + assert.equal(r.exitCode, 0, `invoke exited ${r.exitCode}: ${r.error}`); + return { result: JSON.parse(r.output), stdout: r.output }; + } + + test('W1: codex lane runs to completion through a staged codex.CMD shim (primary invocation path)', { skip: !isWin }, () => { + const binDir = createTempDir('gsd-3275-w1-bin-'); + const runDir = createTempDir('gsd-3275-w1-run-'); + const repoRoot = createTempDir('gsd-3275-w1-repo-'); + try { + // The pitfall, live: the extensionless sh shim sits beside codex.CMD exactly + // as npm lays it down. A bare-name-first resolver picks it and re-ENOENTs. + stageBin(binDir, { + codex: '#!/bin/sh\nexit 1\n', + 'codex.CMD': [ + '@echo off', + 'setlocal', + 'set "OUT="', + ':parse', + 'if "%~1"=="" goto write', + 'if /I "%~1"=="-o" set "OUT=%~2"', + 'shift', + 'goto parse', + ':write', + '> "%OUT%" echo fake codex review from the #3275 Windows shim', + 'exit /b 0', + ].join('\r\n'), + }); + const { result } = invokeLane({ slug: 'codex', binDir, runDir, repoRoot }); + + assert.equal(result.ok, true, `lane failed: ${result.reason} ${result.detail}`); + assert.equal(result.stubbed, false, `review was stubbed: ${JSON.stringify(result)}`); + + const review = fs.readFileSync(path.join(runDir, 'gsd-review-codex.md'), 'utf8'); + assert.ok( + review.includes('#3275 Windows shim'), + `review file should carry the shim's output, got: ${review}`, + ); + const errSide = fs.readFileSync(path.join(runDir, 'gsd-review-codex.err'), 'utf8'); + assert.ok(!errSide.includes('[spawn error: ENOENT]'), `sidecar carries ENOENT: ${errSide}`); + } finally { + cleanup(binDir); + cleanup(runDir); + cleanup(repoRoot); + } + }); + + test('W2: kimi-code lane passes its capability probe and runs through a staged kimi.CMD shim', { skip: !isWin }, () => { + const binDir = createTempDir('gsd-3275-w2-bin-'); + const runDir = createTempDir('gsd-3275-w2-run-'); + const repoRoot = createTempDir('gsd-3275-w2-repo-'); + try { + stageBin(binDir, { + kimi: '#!/bin/sh\nexit 1\n', + 'kimi.CMD': [ + '@echo off', + 'if /I "%~1"=="--help" (', + ' echo Usage: kimi [-m MODEL] [-p PROMPT] --output-format FORMAT', + ' exit /b 0', + ')', + 'echo fake kimi-code review from the #3275 Windows shim', + 'exit /b 0', + ].join('\r\n'), + }); + const { result } = invokeLane({ slug: 'kimi-code', binDir, runDir, repoRoot }); + + assert.equal(result.ok, true, `lane failed: ${result.reason} ${result.detail}`); + assert.equal(result.stubbed, false, `review was stubbed: ${JSON.stringify(result)}`); + + const review = fs.readFileSync(path.join(runDir, 'gsd-review-kimi-code.md'), 'utf8'); + assert.ok( + review.includes('#3275 Windows shim'), + `review file should carry the shim's output, got: ${review}`, + ); + } finally { + cleanup(binDir); + cleanup(runDir); + cleanup(repoRoot); + } + }); + + test('E2E wiring is aimed at the real CLI seam', () => { + // Guards the test itself: if TOOLS_PATH or the review-lane verb ever moves, + // W1/W2 would silently skip their assertions behind the win32 gate. + assert.ok(fs.existsSync(TOOLS_PATH), `gsd-tools.cjs not found at ${TOOLS_PATH}`); + }); +});