diff --git a/.changeset/nimble-sloths-jump.md b/.changeset/nimble-sloths-jump.md new file mode 100644 index 000000000..45a159a65 --- /dev/null +++ b/.changeset/nimble-sloths-jump.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3142 +--- +**Cross-AI reviewer lanes no longer silently drop on Windows** — `deps.spawn` used `shell: false` with a bare binary name, which fails with ENOENT on Windows .cmd shims (npm-installed CLIs). Now applies the #2667 `cmd.exe /d /s /c` shim gate. Spawn errors (ENOENT, ETIMEDOUT) are also surfaced in the reviewer err file instead of being silently dropped. (#3086) diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index 86bd33840..535323688 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -1330,7 +1330,16 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load // cannot be interrupted by --test-force-exit and hangs a whole CI chunk to its 10-minute kill. const deps = { spawn: (binary, argv, opts) => { - const r = cp.spawnSync(binary, argv, { + // #3086: on Windows, reviewer CLIs (gemini, codex, etc.) are installed + // as .cmd shims. spawnSync with a bare name + shell:false fails with + // 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). + 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 r = cp.spawnSync(spawnBinary, spawnArgv, { input: opts.input, encoding: 'utf8', timeout: opts.timeoutMs, diff --git a/src/review-lane-runner.cts b/src/review-lane-runner.cts index 6120bc7d5..b60ad97a6 100644 --- a/src/review-lane-runner.cts +++ b/src/review-lane-runner.cts @@ -664,7 +664,13 @@ function runSpawnLane(plan: SpawnPlan, deps: RunnerDeps, repoRoot: string): Lane : plan.argv; const out = deps.spawn(plan.binary, argv, { input, timeoutMs: plan.timeoutMs }); - deps.writeFile(plan.errPath, out.stderr ?? ''); + // #3086: surface spawn errors (ENOENT, ETIMEDOUT, etc.) that would otherwise + // be silently dropped — the review path read only stdout/stderr and treated + // an empty-stderr spawn failure as "the model had nothing to say". + const errContent = out.errorCode + ? `${out.stderr ?? ''}\n[spawn error: ${out.errorCode}]\n` + : (out.stderr ?? ''); + deps.writeFile(plan.errPath, errContent); // `file-arg` lanes write the review themselves and their stdout is deliberately discarded (#1698). let review = diff --git a/tests/review-lane-runner.test.cjs b/tests/review-lane-runner.test.cjs index c0df48862..210db07fd 100644 --- a/tests/review-lane-runner.test.cjs +++ b/tests/review-lane-runner.test.cjs @@ -663,3 +663,44 @@ describe('runner — orchestration', () => { } }); }); + +// #3086: spawn errors (ENOENT on Windows .cmd shims, ETIMEDOUT, etc.) must be +// surfaced in the err file so the stub reviewer output explains WHY the lane +// produced nothing, rather than silently dropping the error code. + +describe('runner — #3086: spawn errorCode surfaced in err file', () => { + test('a spawn ENOENT writes the error code to the err file', async () => { + const p = plan('gemini'); + const d = deps({ + spawn: () => ({ status: null, stdout: '', stderr: '', errorCode: 'ENOENT' }), + }); + await runLane(p, d, { repoRoot: ROOT }); + const errContent = d.files[p.errPath] || ''; + assert.ok(errContent.includes('ENOENT'), + `err file must include the spawn error code; got: ${errContent}`); + }); + + test('a spawn ETIMEDOUT writes the error code to the err file', async () => { + const p = plan('codex'); + const d = deps({ + spawn: () => ({ status: null, stdout: '', stderr: '', errorCode: 'ETIMEDOUT' }), + }); + await runLane(p, d, { repoRoot: ROOT }); + const errContent = d.files[p.errPath] || ''; + assert.ok(errContent.includes('ETIMEDOUT'), + `err file must include the spawn error code; got: ${errContent}`); + }); + + test('a successful spawn with stderr does NOT add a spawn error marker', async () => { + const p = plan('gemini'); + const d = deps({ + spawn: () => ({ status: 0, stdout: '## Review\nok', stderr: 'some warning', errorCode: undefined }), + }); + await runLane(p, d, { repoRoot: ROOT }); + const errContent = d.files[p.errPath] || ''; + assert.ok(!errContent.includes('[spawn error:'), + `err file must NOT contain a spawn error marker on success; got: ${errContent}`); + assert.ok(errContent.includes('some warning'), + 'legitimate stderr should still be written'); + }); +});