diff --git a/src/daemon/agents/kimi.ts b/src/daemon/agents/kimi.ts index 052b1cd..852b87a 100644 --- a/src/daemon/agents/kimi.ts +++ b/src/daemon/agents/kimi.ts @@ -28,7 +28,7 @@ import { quoteValue, quotePath, validateValue } from './quote.js'; import { spawnHeadless } from '../headless.js'; import { parseOpencode, parseOpencodeExit, parseKimi } from './parsers/index.js'; import { atomicWriteJsonSync } from '../../lib/atomic-write.js'; -import { detectAllClis } from '../../lib/cli-detect.js'; +import { detectAllClis, resolveCliBinaryPath } from '../../lib/cli-detect.js'; import { wrapWithPty } from './opencode.js'; import { assertSandboxSupported, sandboxFailClosed } from './sandbox-guard.js'; @@ -312,7 +312,10 @@ export const kimiShim: AgentShim = { // failure mode the opencode PTY fix was written to prevent. Confirmed // launch-eve by both deepseek and gemini reviewing the shims; the bug // was a copy-paste oversight when the PTY fix landed in opencode.ts. - const { command, args } = wrapWithPty('opencode', opencodeArgs); + // #104: resolve opencode to its detected absolute path before PTY-wrapping + // — the wrapper embeds the binary in argv, so the spawn core only sees + // `script` and can't resolve it. (Same rationale as opencodeShim.) + const { command, args } = wrapWithPty(resolveCliBinaryPath('opencode'), opencodeArgs); const run = spawnHeadless({ command, args, diff --git a/src/daemon/agents/opencode.ts b/src/daemon/agents/opencode.ts index 1ac843f..bac5eca 100644 --- a/src/daemon/agents/opencode.ts +++ b/src/daemon/agents/opencode.ts @@ -14,6 +14,7 @@ import type { } from './types.js'; import { quotePath, validateValue } from './quote.js'; import { spawnHeadless } from '../headless.js'; +import { resolveCliBinaryPath } from '../../lib/cli-detect.js'; import { parseOpencode, parseOpencodeExit } from './parsers/index.js'; import { assertSandboxSupported, sandboxFailClosed } from './sandbox-guard.js'; import * as fs from 'fs'; @@ -197,7 +198,10 @@ export const opencodeShim: AgentShim = { // forwards the child exit code so cli_failed detection still works. // Header/footer are absent under -q, so parseOpencode (which already // tryJson's every line and discards non-JSON) needs no change. - const { command, args } = wrapWithPty('opencode', opencodeArgs); + // #104: resolve opencode to its detected absolute path HERE. The PTY + // wrapper embeds the binary inside argv (Linux: `script -qfec '…'`), + // so the spawn core only ever sees `script` and can't resolve it for us. + const { command, args } = wrapWithPty(resolveCliBinaryPath('opencode'), opencodeArgs); const run = spawnHeadless({ command, diff --git a/src/daemon/headless.ts b/src/daemon/headless.ts index a8db8fb..dd86dbc 100644 --- a/src/daemon/headless.ts +++ b/src/daemon/headless.ts @@ -28,38 +28,51 @@ import * as os from 'os'; import * as path from 'path'; import type { AgentEvent } from './agents/types.js'; import { cliPaths } from '../lib/cli-paths.js'; +import { resolveCliBinaryPath } from '../lib/cli-detect.js'; // Cache binary→path resolutions. `where` shells out — don't repeat per spawn. const binaryPathCache = new Map(); /** - * Resolve a binary name to a full path with extension. Critical on Windows - * where Node's spawn won't resolve `.cmd` shims (npm globals like - * claude.cmd, codex.cmd, gemini.cmd) without shell:true (DEP0190 in - * Node 22+). On Unix returns the name unchanged — spawn handles PATH - * resolution natively for ELF/script files with shebangs. + * Resolve a binary name to a full path with extension. Two stages: + * + * 1. #104 — prefer the absolute path detection already resolved AND verified + * for this CLI, so the daemon spawns exactly the binary detection chose. + * This closes two gaps between detection and execution: ENOENT (a CLI + * found only via the fallback-dir scan whose dir isn't on the spawn PATH) + * and shadowing (two same-named binaries, e.g. ~/.kimi/bin/kimi vs + * ~/.kimi-code/bin/kimi, where bare-name spawn could pick the other one). + * When detection has no path, `resolveCliBinaryPath` returns the bare name + * unchanged and the existing resolution below applies — no behaviour + * change for the unresolved case. + * 2. Windows `where` fallback — critical where Node's spawn won't resolve + * `.cmd` shims (npm globals like claude.cmd) without shell:true (DEP0190 + * in Node 22+). Skipped once stage 1 yields an absolute path. On Unix a + * bare name is passed through — spawn handles PATH resolution natively + * for ELF/script files with shebangs. */ function resolveBinaryPath(command: string): string { - if (process.platform !== 'win32') return command; + const detected = resolveCliBinaryPath(command); + if (process.platform !== 'win32') return detected; // Use the Windows-specific isAbsolute (`path.win32`) so absolute // detection works the same on a real Windows host AND on a Linux CI // run where the test stubs `process.platform = 'win32'` but the // top-level `path` module is still POSIX. - if (path.win32.isAbsolute(command)) return command; - const cached = binaryPathCache.get(command); + if (path.win32.isAbsolute(detected)) return detected; + const cached = binaryPathCache.get(detected); if (cached) return cached; - const r = spawnSync('where', [command], { encoding: 'utf-8', timeout: 3000 }); + const r = spawnSync('where', [detected], { encoding: 'utf-8', timeout: 3000 }); if (r.status !== 0 || !r.stdout) { - binaryPathCache.set(command, command); - return command; // fallback — daemon will surface ENOENT cleanly. + binaryPathCache.set(detected, detected); + return detected; // fallback — daemon will surface ENOENT cleanly. } // npm globals on Windows ship two siblings: `claude` (Bash shim, not // executable by Node spawn) and `claude.cmd` (Windows shim). `where` // returns both; we must pick the .cmd/.bat/.exe variant for Node. const lines = r.stdout.split(/\r?\n/).map((s) => s.trim()).filter(Boolean); const preferred = lines.find((l) => /\.(cmd|bat|exe)$/i.test(l)); - const resolved = preferred ?? lines[0] ?? command; - binaryPathCache.set(command, resolved); + const resolved = preferred ?? lines[0] ?? detected; + binaryPathCache.set(detected, resolved); return resolved; } diff --git a/src/lib/cli-detect.ts b/src/lib/cli-detect.ts index bd838f1..312105a 100644 --- a/src/lib/cli-detect.ts +++ b/src/lib/cli-detect.ts @@ -447,6 +447,54 @@ export function clearDetectionCache(): void { detectCache = null; } +/** + * Reverse of BINARY_NAME: bare binary name → DetectableCli id. BINARY_NAME's + * values are unique, so the inversion is lossless. Lets a spawn site that only + * knows the bare command (e.g. `kimi`) recover the detection id without + * threading it through every shim. + */ +const CLI_ID_BY_BINARY: Record = Object.fromEntries( + (Object.entries(BINARY_NAME) as [DetectableCli, string][]).map( + ([id, bin]) => [bin, id], + ), +) as Record; + +/** + * Resolve a bare CLI binary name to the absolute path detection already + * verified for it, so the daemon spawns the SAME binary it detected (#104). + * + * Detection resolves a concrete path (PATH lookup → fallback-dir scan → manual + * override); spawning by bare name re-resolves against the daemon's PATH and + * can disagree two ways: + * - ENOENT — a CLI found only via the fallback-dir scan (its install dir is + * not on the spawn PATH) can't be launched by bare name at all. + * - shadowing — when two same-named binaries coexist (e.g. ~/.kimi/bin/kimi + * and ~/.kimi-code/bin/kimi), bare-name spawn may run a different build + * than detection resolved, so the transport decision (#101) and the binary + * that actually runs end up describing different binaries. + * + * Returns `command` UNCHANGED when: + * - it already carries a path separator (the caller passed a concrete path — + * e.g. a manual override, or the `script` PTY wrapper's resolved target); + * - it isn't one of our known CLI binaries (e.g. `script`, `sh`); + * - detection currently can't find that CLI — falling through to the + * caller's existing bare-name resolution (Unix PATH / Windows `where`), + * preserving pre-#104 behaviour. + * + * Reads the cached detection result (30s TTL) — the steady-state cost is a map + * lookup; a cold cache pays one detect sweep, the same one the kimi transport + * probe already triggers. + */ +export function resolveCliBinaryPath(command: string): string { + // Only ever rewrite a bare name. Anything with a separator is a concrete + // path the caller has already committed to. + if (command.includes('/') || command.includes('\\')) return command; + const id = CLI_ID_BY_BINARY[command]; + if (!id) return command; + const hit = detectAllClis().find((c) => c.id === id && c.found && c.path); + return hit?.path ?? command; +} + /** * Validate a user-supplied path for a given CLI. Used by the * "Set path manually" fallback when auto-detect misses. diff --git a/tests/cli-detect-resolve.test.ts b/tests/cli-detect-resolve.test.ts new file mode 100644 index 0000000..ed4a420 --- /dev/null +++ b/tests/cli-detect-resolve.test.ts @@ -0,0 +1,121 @@ +/** + * resolveCliBinaryPath — spawn the binary detection verified (issue #104). + * + * Detection (`detectAllClis`) resolves a concrete absolute path: PATH lookup + * (`which`/`where`) → fallback-dir scan → manual override. Headless spawns, + * however, used the BARE name and re-resolved against the daemon's PATH. The + * two could disagree: + * - ENOENT: a CLI found only via the fallback-dir scan (its dir not on the + * spawn PATH) can't be spawned by bare name. + * - shadowing: two same-named binaries (e.g. ~/.kimi/bin/kimi and + * ~/.kimi-code/bin/kimi) — bare-name spawn may run a different build than + * detection resolved. + * + * resolveCliBinaryPath closes that gap by mapping a bare CLI name back to the + * absolute path detection already chose. These tests drive REAL detection + * (staged executables on a prepended PATH) so they fail if the reverse map or + * the found-predicate regresses. + */ +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { randomUUID } from 'node:crypto'; + +import { + resolveCliBinaryPath, + detectAllClis, + clearDetectionCache, +} from '../src/lib/cli-detect.js'; + +// Mirrors BINARY_NAME in cli-detect.ts. Duplicated intentionally: a rename of +// a detection binary is a deliberate change that should make this test fail. +const BINARY_NAME: Record = { + 'claude-code': 'claude', + 'codex-cli': 'codex', + 'gemini-cli': 'gemini', + 'opencode-cli': 'opencode', + 'kimi-cli': 'kimi', + 'grok-cli': 'grok', + 'antigravity-cli': 'agy', +}; + +let tmpRoot: string; +let savedPath: string | undefined; + +beforeEach(() => { + tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'chorus-resolve-')); + savedPath = process.env.PATH; + clearDetectionCache(); +}); + +afterEach(() => { + if (savedPath === undefined) delete process.env.PATH; + else process.env.PATH = savedPath; + clearDetectionCache(); + fs.rmSync(tmpRoot, { recursive: true, force: true }); +}); + +/** + * Stage an executable shell stub named `name` in a fresh dir, prepend that dir + * to PATH so `which`/`where` resolves it FIRST (ahead of any host install), + * and return its absolute path. The stub echoes `versionOutput` so detection's + * ` --version` signature gate passes. + */ +function stageOnPath(name: string, versionOutput: string): string { + const dir = path.join(tmpRoot, randomUUID()); + fs.mkdirSync(dir, { recursive: true }); + const full = path.join(dir, name); + fs.writeFileSync(full, `#!/bin/sh\necho "${versionOutput}"\n`, { mode: 0o755 }); + process.env.PATH = `${dir}${path.delimiter}${process.env.PATH ?? ''}`; + return full; +} + +describe('resolveCliBinaryPath (issue #104)', () => { + it('resolves a bare known CLI name to the absolute path detection found', () => { + const staged = stageOnPath('kimi', 'kimi, version 1.46.0'); + detectAllClis(true); // prime the cache against the staged PATH + expect(resolveCliBinaryPath('kimi')).toBe(staged); + }); + + it('leaves an already-absolute path unchanged (caller already decided)', () => { + // No detection lookup — a path-bearing command is spawned verbatim. + expect(resolveCliBinaryPath('/opt/custom/kimi')).toBe('/opt/custom/kimi'); + }); + + it('leaves a relative path (with separator) unchanged', () => { + expect(resolveCliBinaryPath('./bin/kimi')).toBe('./bin/kimi'); + }); + + it('leaves an unknown binary name unchanged (e.g. the `script` PTY wrapper)', () => { + // `script` is not one of our CLIs, so it must never be rewritten — the + // opencode PTY path spawns it with opencode embedded in argv. + const staged = stageOnPath('kimi', 'kimi, version 1.46.0'); + detectAllClis(true); + expect(staged).toContain('kimi'); // sanity: detection primed + expect(resolveCliBinaryPath('script')).toBe('script'); + expect(resolveCliBinaryPath('sh')).toBe('sh'); + }); + + it('returns the bare name for a known CLI that detection cannot find', () => { + // Pick whichever supported CLI the host genuinely lacks so the assertion + // is deterministic regardless of what's installed. (At least grok or + // antigravity is absent on any normal dev/CI box.) Skip in the unlikely + // event every CLI is installed. + process.env.PATH = path.join(tmpRoot, 'empty'); + fs.mkdirSync(process.env.PATH, { recursive: true }); + const missing = detectAllClis(true).find((d) => !d.found); + if (!missing) return; + const bare = BINARY_NAME[missing.id]; + expect(resolveCliBinaryPath(bare)).toBe(bare); + }); + + it('mirrors detection for every CLI: detected path when found, bare name otherwise', () => { + const results = detectAllClis(true); + for (const d of results) { + const bare = BINARY_NAME[d.id]; + const expected = d.found && d.path ? d.path : bare; + expect(resolveCliBinaryPath(bare)).toBe(expected); + } + }); +}); diff --git a/tests/headless-spawn-windows.test.ts b/tests/headless-spawn-windows.test.ts index 1290aaf..f4750c3 100644 --- a/tests/headless-spawn-windows.test.ts +++ b/tests/headless-spawn-windows.test.ts @@ -54,6 +54,23 @@ function stubCliPaths(): void { })); } +/** + * #104: resolveBinaryPath now first asks cli-detect for the absolute path it + * resolved (so the daemon spawns exactly the detected binary). Stub it so + * these tests stay deterministic and don't load the heavy real cli-detect + * module under a flipped process.platform. + * + * Default `resolve` is identity — modelling "detection has no path for this + * CLI" so the EXISTING bare-name / Windows-`where` resolution below is what's + * under test. Pass a custom `resolve` to model "detection resolved an absolute + * path" (the #104 happy path). + */ +function stubCliDetect(resolve: (command: string) => string = (c) => c): void { + vi.doMock('../src/lib/cli-detect', () => ({ + resolveCliBinaryPath: resolve, + })); +} + function setPlatform(p: NodeJS.Platform): void { Object.defineProperty(process, 'platform', { value: p, @@ -134,6 +151,7 @@ describe('resolveBinaryPath (via spawnHeadless) — Unix branch', () => { it('passes the command through unchanged on Linux and never calls `where`', async () => { setPlatform('linux'); stubCliPaths(); + stubCliDetect(); const spawnSyncSpy = vi.fn(); const { child, triggerExit } = makeFakeChild(); @@ -171,6 +189,7 @@ describe('resolveBinaryPath (via spawnHeadless) — Windows branch', () => { it('prefers the .cmd shim when `where` returns both bash sibling and .cmd', async () => { setPlatform('win32'); stubCliPaths(); + stubCliDetect(); const spawnSyncSpy = vi.fn().mockReturnValue({ status: 0, @@ -217,6 +236,7 @@ describe('resolveBinaryPath (via spawnHeadless) — Windows branch', () => { it('caches the resolution: a second spawn for the same command does not re-invoke `where`', async () => { setPlatform('win32'); stubCliPaths(); + stubCliDetect(); const spawnSyncSpy = vi.fn().mockReturnValue({ status: 0, @@ -268,6 +288,7 @@ describe('resolveBinaryPath (via spawnHeadless) — Windows branch', () => { it('falls back to the original command name when `where` exits non-zero', async () => { setPlatform('win32'); stubCliPaths(); + stubCliDetect(); const spawnSyncSpy = vi.fn().mockReturnValue({ status: 1, @@ -306,6 +327,7 @@ describe('resolveBinaryPath (via spawnHeadless) — Windows branch', () => { it('falls back to the original command when `where` reports success but stdout is empty', async () => { setPlatform('win32'); stubCliPaths(); + stubCliDetect(); const spawnSyncSpy = vi.fn().mockReturnValue({ status: 0, @@ -344,6 +366,7 @@ describe('resolveBinaryPath (via spawnHeadless) — Windows branch', () => { it('preserves an absolute command path without invoking `where`', async () => { setPlatform('win32'); stubCliPaths(); + stubCliDetect(); const spawnSyncSpy = vi.fn(); const { child, triggerExit } = makeFakeChild(); @@ -382,6 +405,7 @@ describe('spawnHeadless shell:true gating', () => { it('does NOT set shell:true on darwin', async () => { setPlatform('darwin'); stubCliPaths(); + stubCliDetect(); const spawnSyncSpy = vi.fn(); const { child, triggerExit } = makeFakeChild(); @@ -412,3 +436,105 @@ describe('spawnHeadless shell:true gating', () => { expect(spawnSyncSpy).not.toHaveBeenCalled(); }); }); + +describe('resolveBinaryPath — #104: spawn the detection-resolved absolute path', () => { + it('spawns the absolute path detection resolved on Linux (not the bare name)', async () => { + setPlatform('linux'); + stubCliPaths(); + // Detection resolved `kimi` to a fallback-dir install not on PATH — the + // ENOENT case bare-name spawn could never have launched. + const resolved = '/home/u/.kimi-code/bin/kimi'; + stubCliDetect((c) => (c === 'kimi' ? resolved : c)); + + const spawnSyncSpy = vi.fn(); + const { child, triggerExit } = makeFakeChild(); + const spawnSpy = vi.fn((_cmd: string, _args: string[], _opts?: object) => child); + + vi.doMock('node:child_process', () => ({ spawn: spawnSpy, spawnSync: spawnSyncSpy })); + vi.doMock('child_process', () => ({ spawn: spawnSpy, spawnSync: spawnSyncSpy })); + + const { spawnHeadless } = await import('../src/daemon/headless'); + const run = spawnHeadless({ + command: 'kimi', + args: ['--print'], + cwd: process.cwd(), + parseLine: () => [], + cli: 'kimi', + }); + triggerExit(0); + await run.done; + + expect(spawnSpy.mock.calls[0]?.[0]).toBe(resolved); // not 'kimi' + expect(spawnSyncSpy).not.toHaveBeenCalled(); // no `where` on Unix + const opts = spawnSpy.mock.calls[0]?.[2] as { shell?: boolean }; + expect(opts.shell).toBe(false); + }); + + it('spawns the detection-resolved absolute path on Windows and skips `where`', async () => { + setPlatform('win32'); + stubCliPaths(); + // An absolute path short-circuits the `where` lookup entirely — detection + // already did the resolution, so no second, possibly-divergent resolve. + const resolved = 'C:\\Users\\u\\.kimi-code\\bin\\kimi.cmd'; + stubCliDetect((c) => (c === 'kimi' ? resolved : c)); + + const spawnSyncSpy = vi.fn(); + const { child, triggerExit } = makeFakeChild(); + const spawnSpy = vi.fn((_cmd: string, _args: string[], _opts?: object) => child); + + vi.doMock('node:child_process', () => ({ spawn: spawnSpy, spawnSync: spawnSyncSpy })); + vi.doMock('child_process', () => ({ spawn: spawnSpy, spawnSync: spawnSyncSpy })); + + const { spawnHeadless } = await import('../src/daemon/headless'); + const run = spawnHeadless({ + command: 'kimi', + args: [], + cwd: process.cwd(), + parseLine: () => [], + cli: 'kimi', + }); + triggerExit(0); + await run.done; + + expect(spawnSyncSpy).not.toHaveBeenCalled(); // detection won; no `where` + expect(spawnSpy.mock.calls[0]?.[0]).toBe(resolved); + const opts = spawnSpy.mock.calls[0]?.[2] as { shell?: boolean }; + expect(opts.shell).toBe(true); // still shell:true on Windows + }); + + it('falls back to bare-name `where` resolution when detection has no path (Windows)', async () => { + setPlatform('win32'); + stubCliPaths(); + stubCliDetect(); // identity → detection found nothing for this CLI + + const spawnSyncSpy = vi.fn().mockReturnValue({ + status: 0, + stdout: 'C:\\path\\grok.cmd\r\n', + stderr: '', + pid: 0, + output: [], + signal: null, + }); + const { child, triggerExit } = makeFakeChild(); + const spawnSpy = vi.fn((_cmd: string, _args: string[], _opts?: object) => child); + + vi.doMock('node:child_process', () => ({ spawn: spawnSpy, spawnSync: spawnSyncSpy })); + vi.doMock('child_process', () => ({ spawn: spawnSpy, spawnSync: spawnSyncSpy })); + + const { spawnHeadless } = await import('../src/daemon/headless'); + const run = spawnHeadless({ + command: 'grok', + args: [], + cwd: process.cwd(), + parseLine: () => [], + cli: 'grok', + }); + triggerExit(0); + await run.done; + + // Detection had nothing → the existing `where grok` path still runs. + expect(spawnSyncSpy).toHaveBeenCalledTimes(1); + expect(spawnSyncSpy.mock.calls[0]?.[1]).toEqual(['grok']); + expect((spawnSpy.mock.calls[0]?.[0] as string).toLowerCase()).toMatch(/grok\.cmd$/); + }); +});