From 5c26cb263b46dcc81a34b36c93029cc23964398a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=8E=E5=86=A0=E8=BE=B0?= Date: Sat, 23 May 2026 20:06:00 +0800 Subject: [PATCH 1/4] fix(daemon): version-aware Gemini binary resolution (#978) macOS GUI launches inherit a stripped PATH /usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin. The previous resolveOnPath was first-match, so a stale /usr/local/bin/gemini=0.1.12 left over from an ancient global npm install would shadow the modern Homebrew/nvm gemini and the daemon spawn landed on yargs Unknown arguments: output-format, outputFormat. The user had to know to set GEMINI_BIN by hand. Opt-in per agent via RuntimeAgentDef.minVersion. Currently set only on Gemini at 0.30.0 (the first stable --output-format release cited in #978). Other agents skip the version walk and keep first-match resolution unchanged. When minVersion is set, chooseExecutableByMinVersion: - Honors GEMINI_BIN verbatim without probing and without writing the cache (clearing the env reliably falls back to auto-pick). - Enumerates every match on PATH + toolchain dirs. - Runs --version on each in parallel with a 1.5 s timeout. - Picks the first whose semver meets the floor; falls through to first-found when nothing qualifies so the existing detection error path still fires (regression-safe). Caches the version-checked pick on a module-scoped Map so inspectAgentExecutableResolution returns the same binary at chat-spawn time. compareSemver is strict and anchored: unparseable input returns null instead of silently passing the gate (the failure mode the original #1007 reviewer round-1 P1 fix closed). Re-implements the maintainer-approved design from PR #1007 against the current apps/daemon/src/runtimes/* layout (PR #1007 targeted the pre-refactor agents.ts and is dirty/conflicting; original author has not pushed in 12 days). --- apps/daemon/src/runtimes/defs/gemini.ts | 10 + apps/daemon/src/runtimes/detection.ts | 10 + apps/daemon/src/runtimes/executables.ts | 209 ++++++++- apps/daemon/src/runtimes/types.ts | 1 + .../runtimes/version-aware-resolver.test.ts | 415 ++++++++++++++++++ 5 files changed, 636 insertions(+), 9 deletions(-) create mode 100644 apps/daemon/tests/runtimes/version-aware-resolver.test.ts diff --git a/apps/daemon/src/runtimes/defs/gemini.ts b/apps/daemon/src/runtimes/defs/gemini.ts index e265f8d93..253bec81d 100644 --- a/apps/daemon/src/runtimes/defs/gemini.ts +++ b/apps/daemon/src/runtimes/defs/gemini.ts @@ -35,4 +35,14 @@ export const geminiAgentDef = { promptViaStdin: true, streamFormat: 'json-event-stream', eventParser: 'gemini', + // The `--output-format stream-json` flag did not exist in early + // Gemini CLI builds; an old `npm i -g @google/gemini-cli` left in + // `/usr/local/bin` shadows the modern Homebrew/nvm install on + // macOS GUI launches and the spawn fails with yargs + // `Unknown arguments: output-format, outputFormat` (#978). Pin a + // version floor so the resolver picks the first candidate on + // PATH+toolchain that actually supports the flag we send. + // 0.30.0 is the cited baseline from #978 where the flag shipped + // in stable form. + minVersion: '0.30.0', } satisfies RuntimeAgentDef; diff --git a/apps/daemon/src/runtimes/detection.ts b/apps/daemon/src/runtimes/detection.ts index 7a1779586..55a44ff59 100644 --- a/apps/daemon/src/runtimes/detection.ts +++ b/apps/daemon/src/runtimes/detection.ts @@ -1,3 +1,4 @@ +import { chooseExecutableByMinVersion } from './executables.js'; import { execAgentFile } from './invocation.js'; import { AGENT_DEFS } from './registry.js'; import { DEFAULT_MODEL_OPTION, rememberLiveModels } from './models.js'; @@ -131,6 +132,15 @@ async function probe( def: RuntimeAgentDef, configuredEnv: Record = {}, ): Promise { + // For agents that declare a minimum CLI version, run the + // version-aware resolver first so the version-checked binary is + // cached before `resolveAgentLaunch` reads it (#978). Without this + // pre-step, `inspectAgentExecutableResolution` would still return + // first-found and we would probe a stale binary even though a + // modern one sits later on the PATH/toolchain. + if (def.minVersion) { + await chooseExecutableByMinVersion(def, configuredEnv); + } // Detection must probe the exact path the runtime will spawn, not just the // PATH-visible shim. This is load-bearing for Codex under nvm/fnm/mise: // the discovered `codex` entry is often a `#!/usr/bin/env node` wrapper diff --git a/apps/daemon/src/runtimes/executables.ts b/apps/daemon/src/runtimes/executables.ts index ce0135a64..299f40f6f 100644 --- a/apps/daemon/src/runtimes/executables.ts +++ b/apps/daemon/src/runtimes/executables.ts @@ -5,6 +5,7 @@ import { homedir } from 'node:os'; import { fileURLToPath } from 'node:url'; import { wellKnownUserToolchainBins } from '@open-design/platform'; import { resolveSandboxRuntimeConfigFromEnv } from '../sandbox-mode.js'; +import { execAgentFile } from './invocation.js'; import { expandHomePath } from './paths.js'; import type { RuntimeAgentDef } from './types.js'; @@ -105,6 +106,31 @@ export function resolveOnPath(bin: string): string | null { return null; } +// Same search shape as `resolveOnPath`, but returns *every* directory +// the bin resolves in (PATH order, then toolchain dirs). Used by the +// version-aware chooser to evaluate every candidate rather than stop +// at the first match (#978). +export function enumerateOnPath(bin: string): string[] { + const exts = + process.platform === 'win32' + ? (process.env.PATHEXT || '.EXE;.CMD;.BAT').split(';') + : ['']; + const dirs = resolvePathDirs(); + const found: string[] = []; + const seen = new Set(); + for (const dir of dirs) { + for (const ext of exts) { + const full = path.join(dir, bin + ext); + if (!full || seen.has(full)) continue; + if (existsSync(full)) { + seen.add(full); + found.push(full); + } + } + } + return found; +} + function looksExecutableOnWindows(filePath: string): boolean { const ext = path.extname(filePath).trim().toUpperCase(); if (!ext) return false; @@ -253,16 +279,29 @@ export function inspectAgentExecutableResolution( }; } const configuredOverridePath = configuredExecutableOverride(def, configuredEnv); - const candidates = [ - def.bin, - ...(Array.isArray(def.fallbackBins) ? def.fallbackBins : []), - ]; + // Version-aware cache lookup runs only when there is no explicit + // override and the def opts in via `minVersion`. The cache is + // populated by `chooseExecutableByMinVersion` during detection so + // chat-time spawn sees the same binary detection picked instead of + // falling back to first-match (#978). let pathResolvedPath: string | null = null; - for (const bin of candidates) { - const resolved = resolveOnPath(bin); - if (resolved) { - pathResolvedPath = resolved; - break; + if (!configuredOverridePath && def.minVersion) { + const cached = versionAwareCache.get(def.id); + if (cached && existsSync(cached)) { + pathResolvedPath = cached; + } + } + if (!pathResolvedPath) { + const candidates = [ + def.bin, + ...(Array.isArray(def.fallbackBins) ? def.fallbackBins : []), + ]; + for (const bin of candidates) { + const resolved = resolveOnPath(bin); + if (resolved) { + pathResolvedPath = resolved; + break; + } } } const builtInPath = packagedBuiltInExecutable(def, configuredEnv); @@ -272,3 +311,155 @@ export function inspectAgentExecutableResolution( selectedPath: configuredOverridePath || builtInPath || pathResolvedPath, }; } + +// ---- Version-aware resolution (#978) ------------------------------------- +// +// Gemini-style "stale binary shadows the modern one" problem: macOS GUI +// launches inherit `/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin`, and an +// ancient `/usr/local/bin/gemini=0.1.12` left by an old `npm i -g +// @google/gemini-cli` shadows the modern Homebrew/nvm install. The old +// binary lacks `--output-format`, so the daemon spawn lands on yargs +// `Unknown arguments`. The fix runs `--version` against every candidate +// and picks the first that meets the floor pinned on the def. + +const VERSION_PROBE_TIMEOUT_MS = 1_500; + +// agent.id → resolved path that passed the version gate. Populated by +// `chooseExecutableByMinVersion`; consulted by +// `inspectAgentExecutableResolution` so the sync chat-spawn path sees +// the same pick detection landed on. Only writes for the auto-pick path +// — an explicit `_BIN` override is intentionally NOT cached so +// clearing the env reliably falls back to auto-pick (#1007 round-2 P2). +const versionAwareCache = new Map(); + +export function clearVersionAwareResolutionCache(agentId?: string): void { + if (agentId === undefined) { + versionAwareCache.clear(); + } else { + versionAwareCache.delete(agentId); + } +} + +// Strict, anchored semver parse. Accepts a leading `v` and tolerates +// trailing pre-release (`-rc.1`) / build metadata (`+build.5`) but +// only major.minor.patch participates in comparison. Returns `null` +// for unparseable input so the chooser explicitly rejects the +// candidate instead of letting it pass the gate (#1007 round-1 P1). +const SEMVER_RE = /^v?(\d+)\.(\d+)\.(\d+)(?:[-+][0-9A-Za-z.-]+)?$/; + +export function compareSemver(a: string, b: string): number | null { + const left = parseSemver(a); + const right = parseSemver(b); + if (!left || !right) return null; + const [la, lb, lc] = left; + const [ra, rb, rc] = right; + if (la !== ra) return la - ra; + if (lb !== rb) return lb - rb; + if (lc !== rc) return lc - rc; + return 0; +} + +function parseSemver(value: string): [number, number, number] | null { + if (typeof value !== 'string') return null; + const m = SEMVER_RE.exec(value.trim()); + if (!m) return null; + return [Number(m[1]), Number(m[2]), Number(m[3])]; +} + +export interface ChooseExecutableByMinVersionOptions { + // Test seam: inject a fake `runVersion(path)` so unit tests do not + // need real binaries that print versions. Production passes the + // default that spawns ` ` with a timeout. + runVersion?: (resolvedPath: string) => Promise; +} + +export async function chooseExecutableByMinVersion( + def: RuntimeAgentDef, + configuredEnv: Record = {}, + options: ChooseExecutableByMinVersionOptions = {}, +): Promise { + if (!def?.bin || !def.minVersion) return resolveAgentExecutable(def, configuredEnv); + + // Explicit user override always wins; do not probe and do not pollute + // the cache (a later run with the override cleared must rediscover + // a fresh auto-pick). + const override = configuredExecutableOverride(def, configuredEnv); + if (override) return override; + + // Enumerate every match for def.bin and (if declared) any fallback + // bins, in the same order resolveOnPath would walk. + const candidates: string[] = []; + const seen = new Set(); + const bins = [def.bin, ...(Array.isArray(def.fallbackBins) ? def.fallbackBins : [])]; + for (const bin of bins) { + for (const hit of enumerateOnPath(bin)) { + if (seen.has(hit)) continue; + seen.add(hit); + candidates.push(hit); + } + } + if (candidates.length === 0) { + versionAwareCache.delete(def.id); + return null; + } + + const runVersion = options.runVersion ?? ((p) => probeVersionWithTimeout(p, def)); + const probes = await Promise.all( + candidates.map(async (p) => { + try { + const out = await runVersion(p); + return { path: p, version: typeof out === 'string' ? out.trim().split('\n')[0] ?? '' : '' }; + } catch { + return { path: p, version: '' }; + } + }), + ); + + for (const probe of probes) { + const cmp = compareSemver(probe.version, def.minVersion); + if (cmp !== null && cmp >= 0) { + versionAwareCache.set(def.id, probe.path); + return probe.path; + } + } + + // Regression-safe fallback: keep the previous behavior (first-found) + // so the existing "agent exited with code 1" surface still fires + // when nothing meets the floor, instead of the agent silently + // disappearing from the picker. Drop any stale cache entry so a + // later install of a modern binary is not occluded. + versionAwareCache.delete(def.id); + return candidates[0] ?? null; +} + +async function probeVersionWithTimeout( + resolvedPath: string, + def: RuntimeAgentDef, +): Promise { + // Mirror the core of `applyAgentLaunchEnv`: prepend the daemon's + // own Node binary directory and the candidate's directory to PATH so + // Node-wrapper CLIs (`#!/usr/bin/env node`, npm `.cmd` shims on + // Windows) can resolve `node` even when the daemon was GUI-launched + // with a stripped PATH. We do NOT import `applyAgentLaunchEnv` + // directly: launch.ts depends on executables.ts and the import + // cycle would break. + const env: NodeJS.ProcessEnv = { ...process.env, ...(def.env || {}) }; + const pathKey = Object.keys(env).find((k) => k.toLowerCase() === 'path') ?? 'PATH'; + const existing = typeof env[pathKey] === 'string' ? (env[pathKey] as string) : ''; + const prepend = [path.dirname(process.execPath), path.dirname(resolvedPath)].filter(Boolean); + const merged = [...prepend, ...existing.split(delimiter).filter(Boolean)]; + const seen = new Set(); + const deduped: string[] = []; + for (const dir of merged) { + if (!seen.has(dir)) { + seen.add(dir); + deduped.push(dir); + } + } + env[pathKey] = deduped.join(delimiter); + const { stdout } = await execAgentFile(resolvedPath, def.versionArgs, { + env, + timeout: VERSION_PROBE_TIMEOUT_MS, + }); + return String(stdout); +} diff --git a/apps/daemon/src/runtimes/types.ts b/apps/daemon/src/runtimes/types.ts index 48eb927bb..5ca9cd364 100644 --- a/apps/daemon/src/runtimes/types.ts +++ b/apps/daemon/src/runtimes/types.ts @@ -162,6 +162,7 @@ export type RuntimeAgentDef = { // present in the daemon's `process.env`; Settings-UI per-agent env // values only reach the spawned child and are NOT consulted here. defaultModelEnvVar?: string; + minVersion?: string; }; export type DetectedAgent = Omit< diff --git a/apps/daemon/tests/runtimes/version-aware-resolver.test.ts b/apps/daemon/tests/runtimes/version-aware-resolver.test.ts new file mode 100644 index 000000000..69952ecd8 --- /dev/null +++ b/apps/daemon/tests/runtimes/version-aware-resolver.test.ts @@ -0,0 +1,415 @@ +// Version-aware executable resolution for Gemini (#978). +// +// macOS GUI launchers strip PATH to `/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin`, +// then `userToolchainDirs()` appends Homebrew / nvm / npm-global / mise paths +// at the end. The previous `resolveOnPath()` was first-match, so a stale +// `/usr/local/bin/gemini=0.1.12` left over from an old global npm install +// would shadow the modern `/opt/homebrew/bin/gemini=0.40.1+` and the daemon +// spawn landed on yargs `Unknown arguments: output-format, outputFormat` +// (PR #1007 design, reviewer-approved). +// +// The fix is opt-in per agent via `RuntimeAgentDef.minVersion`. When set, +// the resolver enumerates every candidate path, probes `--version` on each +// in parallel, and returns the first whose version meets `minVersion`. Other +// agents (claude / codex / opencode / ...) skip the version walk and keep +// first-match resolution unchanged. + +import { afterEach, describe, expect, it } from 'vitest'; +import { + chmodSync, + mkdirSync, + mkdtempSync, + rmSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { delimiter } from 'node:path'; +import { + chooseExecutableByMinVersion, + clearVersionAwareResolutionCache, + compareSemver, + enumerateOnPath, + inspectAgentExecutableResolution, + resolveAgentExecutable, +} from '../../src/runtimes/executables.js'; +import { gemini, minimalAgentDef } from './helpers/test-helpers.js'; + +const fsTest = process.platform === 'win32' ? it.skip : it; + +afterEach(() => { + clearVersionAwareResolutionCache(); +}); + +describe('compareSemver (#978 reviewer round 1: unparseable input must not pass through)', () => { + it('returns 0 for equal versions', () => { + expect(compareSemver('1.2.3', '1.2.3')).toBe(0); + }); + + it('returns positive when a > b on major / minor / patch', () => { + expect(compareSemver('2.0.0', '1.99.99')).toBeGreaterThan(0); + expect(compareSemver('1.3.0', '1.2.99')).toBeGreaterThan(0); + expect(compareSemver('1.2.4', '1.2.3')).toBeGreaterThan(0); + }); + + it('returns negative when a < b', () => { + expect(compareSemver('0.1.12', '0.30.0')).toBeLessThan(0); + expect(compareSemver('0.29.9', '0.30.0')).toBeLessThan(0); + }); + + it('tolerates a leading "v" and trailing pre-release / metadata', () => { + // Real-world Gemini CLI prints like "0.40.1", but other CLIs print + // "v1.2.3", "1.2.3-rc.1", or "1.2.3+build.5". Major.minor.patch is + // the only thing the version gate cares about. + expect(compareSemver('v0.40.1', '0.30.0')).toBeGreaterThan(0); + expect(compareSemver('0.40.1-rc.1', '0.30.0')).toBeGreaterThan(0); + expect(compareSemver('0.40.1+build.5', '0.30.0')).toBeGreaterThan(0); + }); + + it('returns null for unparseable input — prose like CLI help text must not be silently accepted as ≥ minVersion', () => { + // The previous draft of compareSemver let unparseable input return 0, + // which meant a stale binary that printed `Usage: gemini ...` on + // --version would silently pass the version gate. The reviewer + // P1 fix forces unparseable inputs to null so the chooser explicitly + // rejects them. + expect(compareSemver('not-a-version', '0.30.0')).toBeNull(); + expect(compareSemver('Usage: gemini [options]', '0.30.0')).toBeNull(); + expect(compareSemver('', '0.30.0')).toBeNull(); + expect(compareSemver('0.30.0', 'minVersion-broken')).toBeNull(); + }); + + it('is anchored — refuses prose-wrapped versions to prevent a partial regex match', () => { + // "Help: see version 1.2.3 for details" must not parse as 1.2.3. + expect(compareSemver('Help: see version 1.2.3 for details', '0.30.0')).toBeNull(); + }); +}); + +describe('enumerateOnPath (#978: list every match across PATH + toolchain, not just the first)', () => { + fsTest('returns one entry per PATH directory that contains the binary, in PATH order', () => { + const a = mkdtempSync(join(tmpdir(), 'od-enum-a-')); + const b = mkdtempSync(join(tmpdir(), 'od-enum-b-')); + try { + writeFileSync(join(a, 'gemini'), '#!/bin/sh\necho 0.1.12\n'); + writeFileSync(join(b, 'gemini'), '#!/bin/sh\necho 0.40.1\n'); + chmodSync(join(a, 'gemini'), 0o755); + chmodSync(join(b, 'gemini'), 0o755); + process.env.OD_AGENT_HOME = a; // suppresses toolchain leakage + process.env.PATH = `${a}${delimiter}${b}`; + + const found = enumerateOnPath('gemini'); + expect(found).toEqual([join(a, 'gemini'), join(b, 'gemini')]); + } finally { + rmSync(a, { recursive: true, force: true }); + rmSync(b, { recursive: true, force: true }); + } + }); + + fsTest('returns an empty array when the binary is not on PATH', () => { + const dir = mkdtempSync(join(tmpdir(), 'od-enum-empty-')); + try { + process.env.OD_AGENT_HOME = dir; + process.env.PATH = dir; + expect(enumerateOnPath('gemini')).toEqual([]); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + fsTest('deduplicates identical paths that appear twice in PATH', () => { + const dir = mkdtempSync(join(tmpdir(), 'od-enum-dedup-')); + try { + writeFileSync(join(dir, 'gemini'), '#!/bin/sh\necho 0.40.1\n'); + chmodSync(join(dir, 'gemini'), 0o755); + process.env.OD_AGENT_HOME = dir; + process.env.PATH = `${dir}${delimiter}${dir}`; + expect(enumerateOnPath('gemini')).toEqual([join(dir, 'gemini')]); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); + +describe('chooseExecutableByMinVersion (#978: skip stale binaries that fail the floor)', () => { + // The chooser is pure on top of an injectable `runVersion` — we pass a + // fake probe so the tests do not depend on a real `gemini` install. + + fsTest('picks the second-on-PATH when the first is too old (the headline #978 scenario)', async () => { + const usrLocal = mkdtempSync(join(tmpdir(), 'od-mv-usrlocal-')); + const homebrew = mkdtempSync(join(tmpdir(), 'od-mv-homebrew-')); + try { + writeFileSync(join(usrLocal, 'gemini'), ''); + writeFileSync(join(homebrew, 'gemini'), ''); + chmodSync(join(usrLocal, 'gemini'), 0o755); + chmodSync(join(homebrew, 'gemini'), 0o755); + process.env.OD_AGENT_HOME = usrLocal; + process.env.PATH = `${usrLocal}${delimiter}${homebrew}`; + + const def = minimalAgentDef({ id: 'gemini', bin: 'gemini', minVersion: '0.30.0' }); + const versions = new Map([ + [join(usrLocal, 'gemini'), '0.1.12'], + [join(homebrew, 'gemini'), '0.40.1'], + ]); + + const chosen = await chooseExecutableByMinVersion(def, {}, { + runVersion: async (p) => versions.get(p) ?? '', + }); + expect(chosen).toBe(join(homebrew, 'gemini')); + } finally { + rmSync(usrLocal, { recursive: true, force: true }); + rmSync(homebrew, { recursive: true, force: true }); + } + }); + + fsTest('returns the first candidate when no candidate meets minVersion (regression-safe fallback)', async () => { + // When every installed gemini is too old, fall through to the + // first-found path. The existing "agent exited with code 1" error + // surfaces — same as before — instead of the chooser silently + // hiding the agent. + const a = mkdtempSync(join(tmpdir(), 'od-mv-allold-a-')); + const b = mkdtempSync(join(tmpdir(), 'od-mv-allold-b-')); + try { + writeFileSync(join(a, 'gemini'), ''); + writeFileSync(join(b, 'gemini'), ''); + chmodSync(join(a, 'gemini'), 0o755); + chmodSync(join(b, 'gemini'), 0o755); + process.env.OD_AGENT_HOME = a; + process.env.PATH = `${a}${delimiter}${b}`; + + const def = minimalAgentDef({ id: 'gemini', bin: 'gemini', minVersion: '0.30.0' }); + const chosen = await chooseExecutableByMinVersion(def, {}, { + runVersion: async () => '0.1.12', + }); + expect(chosen).toBe(join(a, 'gemini')); + } finally { + rmSync(a, { recursive: true, force: true }); + rmSync(b, { recursive: true, force: true }); + } + }); + + fsTest('returns null when the binary is on no PATH directory at all', async () => { + const dir = mkdtempSync(join(tmpdir(), 'od-mv-none-')); + try { + process.env.OD_AGENT_HOME = dir; + process.env.PATH = dir; + const def = minimalAgentDef({ id: 'gemini', bin: 'gemini', minVersion: '0.30.0' }); + const chosen = await chooseExecutableByMinVersion(def, {}, { + runVersion: async () => '0.40.1', + }); + expect(chosen).toBeNull(); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + fsTest('rejects unparseable version output and continues probing the next candidate', async () => { + // A stale binary that prints help text to `--version` (typical of + // pre-1.0 CLIs) used to silently pass when compareSemver returned 0 + // for unparseable input. Now it does not parse, the chooser treats + // the candidate as failed, and proceeds to the next one. + const broken = mkdtempSync(join(tmpdir(), 'od-mv-broken-')); + const ok = mkdtempSync(join(tmpdir(), 'od-mv-ok-')); + try { + writeFileSync(join(broken, 'gemini'), ''); + writeFileSync(join(ok, 'gemini'), ''); + chmodSync(join(broken, 'gemini'), 0o755); + chmodSync(join(ok, 'gemini'), 0o755); + process.env.OD_AGENT_HOME = broken; + process.env.PATH = `${broken}${delimiter}${ok}`; + + const def = minimalAgentDef({ id: 'gemini', bin: 'gemini', minVersion: '0.30.0' }); + const versions = new Map([ + [join(broken, 'gemini'), 'Usage: gemini [options]'], + [join(ok, 'gemini'), '0.40.1'], + ]); + const chosen = await chooseExecutableByMinVersion(def, {}, { + runVersion: async (p) => versions.get(p) ?? '', + }); + expect(chosen).toBe(join(ok, 'gemini')); + } finally { + rmSync(broken, { recursive: true, force: true }); + rmSync(ok, { recursive: true, force: true }); + } + }); + + fsTest('honors the configured `_BIN` override without running any version probes', async () => { + // Explicit env override means the user has opted out of auto-pick. + // The chooser must return the override verbatim and must not run + // --version on anything (probing the override would be both + // pointless and a noisy startup-time side effect). + const override = mkdtempSync(join(tmpdir(), 'od-mv-override-')); + const onPath = mkdtempSync(join(tmpdir(), 'od-mv-onpath-')); + try { + writeFileSync(join(override, 'gemini'), ''); + writeFileSync(join(onPath, 'gemini'), ''); + chmodSync(join(override, 'gemini'), 0o755); + chmodSync(join(onPath, 'gemini'), 0o755); + process.env.OD_AGENT_HOME = onPath; + process.env.PATH = onPath; + + const def = minimalAgentDef({ id: 'gemini', bin: 'gemini', minVersion: '0.30.0' }); + let probeCalls = 0; + const chosen = await chooseExecutableByMinVersion( + def, + { GEMINI_BIN: join(override, 'gemini') }, + { + runVersion: async () => { + probeCalls++; + return '0.40.1'; + }, + }, + ); + expect(chosen).toBe(join(override, 'gemini')); + expect(probeCalls).toBe(0); + } finally { + rmSync(override, { recursive: true, force: true }); + rmSync(onPath, { recursive: true, force: true }); + } + }); + + fsTest('falls through to first-found when a version probe throws (e.g. binary times out or segfaults)', async () => { + // We do not want a single misbehaving binary to break detection + // for every Gemini install on the box. A throw should be treated + // the same as "could not parse" — try the next candidate; if + // nothing meets the floor, regression-safe fallback fires. + const flaky = mkdtempSync(join(tmpdir(), 'od-mv-flaky-')); + const ok = mkdtempSync(join(tmpdir(), 'od-mv-flaky-ok-')); + try { + writeFileSync(join(flaky, 'gemini'), ''); + writeFileSync(join(ok, 'gemini'), ''); + chmodSync(join(flaky, 'gemini'), 0o755); + chmodSync(join(ok, 'gemini'), 0o755); + process.env.OD_AGENT_HOME = flaky; + process.env.PATH = `${flaky}${delimiter}${ok}`; + + const def = minimalAgentDef({ id: 'gemini', bin: 'gemini', minVersion: '0.30.0' }); + const chosen = await chooseExecutableByMinVersion(def, {}, { + runVersion: async (p) => { + if (p === join(flaky, 'gemini')) throw new Error('boom'); + return '0.40.1'; + }, + }); + expect(chosen).toBe(join(ok, 'gemini')); + } finally { + rmSync(flaky, { recursive: true, force: true }); + rmSync(ok, { recursive: true, force: true }); + } + }); +}); + +describe('inspectAgentExecutableResolution + minVersion cache wiring (#978)', () => { + // The async chooser populates a module-scoped cache; the sync resolver + // used at chat-spawn time consults that cache so detection-time and + // spawn-time always land on the same binary. + + fsTest('after a successful async chooser populates the cache, the sync resolver returns the version-checked pick', async () => { + const usrLocal = mkdtempSync(join(tmpdir(), 'od-cache-usrlocal-')); + const homebrew = mkdtempSync(join(tmpdir(), 'od-cache-homebrew-')); + try { + writeFileSync(join(usrLocal, 'gemini'), ''); + writeFileSync(join(homebrew, 'gemini'), ''); + chmodSync(join(usrLocal, 'gemini'), 0o755); + chmodSync(join(homebrew, 'gemini'), 0o755); + process.env.OD_AGENT_HOME = usrLocal; + process.env.PATH = `${usrLocal}${delimiter}${homebrew}`; + + const def = minimalAgentDef({ id: 'gemini', bin: 'gemini', minVersion: '0.30.0' }); + const versions = new Map([ + [join(usrLocal, 'gemini'), '0.1.12'], + [join(homebrew, 'gemini'), '0.40.1'], + ]); + const chosen = await chooseExecutableByMinVersion(def, {}, { + runVersion: async (p) => versions.get(p) ?? '', + }); + expect(chosen).toBe(join(homebrew, 'gemini')); + + // Without the cache, this sync call would still return usrLocal + // (first-found). With it, it returns the version-checked pick. + expect(resolveAgentExecutable(def)).toBe(join(homebrew, 'gemini')); + expect(inspectAgentExecutableResolution(def).selectedPath).toBe(join(homebrew, 'gemini')); + } finally { + rmSync(usrLocal, { recursive: true, force: true }); + rmSync(homebrew, { recursive: true, force: true }); + } + }); + + fsTest('configured override beats the cache so the user escape hatch always wins', async () => { + const cached = mkdtempSync(join(tmpdir(), 'od-cache-cached-')); + const override = mkdtempSync(join(tmpdir(), 'od-cache-override-')); + try { + writeFileSync(join(cached, 'gemini'), ''); + writeFileSync(join(override, 'gemini'), ''); + chmodSync(join(cached, 'gemini'), 0o755); + chmodSync(join(override, 'gemini'), 0o755); + process.env.OD_AGENT_HOME = cached; + process.env.PATH = cached; + + const def = minimalAgentDef({ id: 'gemini', bin: 'gemini', minVersion: '0.30.0' }); + await chooseExecutableByMinVersion(def, {}, { + runVersion: async () => '0.40.1', + }); + // Cache is populated for the no-override case. + expect(resolveAgentExecutable(def)).toBe(join(cached, 'gemini')); + + // Now the user pins GEMINI_BIN. The configured override must win + // regardless of cache contents. + expect(resolveAgentExecutable(def, { GEMINI_BIN: join(override, 'gemini') })) + .toBe(join(override, 'gemini')); + } finally { + rmSync(cached, { recursive: true, force: true }); + rmSync(override, { recursive: true, force: true }); + } + }); + + fsTest('clearing GEMINI_BIN after a previous override-with-cache run still falls back to the auto-pick (override does not pollute cache, #1007 round-2 P2)', async () => { + // Reviewer P2 fix on PR #1007: if `chooseExecutableByMinVersion` + // were to write the override path into the cache, clearing the env + // would leave the daemon pinned to a stale binary even though the + // user has opted back into auto-pick. The override path must skip + // the cache write entirely. + const usrLocal = mkdtempSync(join(tmpdir(), 'od-cache-clear-usr-')); + const homebrew = mkdtempSync(join(tmpdir(), 'od-cache-clear-brew-')); + const override = mkdtempSync(join(tmpdir(), 'od-cache-clear-override-')); + try { + writeFileSync(join(usrLocal, 'gemini'), ''); + writeFileSync(join(homebrew, 'gemini'), ''); + writeFileSync(join(override, 'gemini'), ''); + chmodSync(join(usrLocal, 'gemini'), 0o755); + chmodSync(join(homebrew, 'gemini'), 0o755); + chmodSync(join(override, 'gemini'), 0o755); + process.env.OD_AGENT_HOME = usrLocal; + process.env.PATH = `${usrLocal}${delimiter}${homebrew}`; + + const def = minimalAgentDef({ id: 'gemini', bin: 'gemini', minVersion: '0.30.0' }); + const versions = new Map([ + [join(usrLocal, 'gemini'), '0.1.12'], + [join(homebrew, 'gemini'), '0.40.1'], + ]); + + // First run with the override active — must not pollute the cache. + await chooseExecutableByMinVersion( + def, + { GEMINI_BIN: join(override, 'gemini') }, + { runVersion: async (p) => versions.get(p) ?? '' }, + ); + + // Next call with no override: cache lookup must MISS for the + // override path and instead trigger a fresh auto-pick. + const after = await chooseExecutableByMinVersion(def, {}, { + runVersion: async (p) => versions.get(p) ?? '', + }); + expect(after).toBe(join(homebrew, 'gemini')); + } finally { + rmSync(usrLocal, { recursive: true, force: true }); + rmSync(homebrew, { recursive: true, force: true }); + rmSync(override, { recursive: true, force: true }); + } + }); +}); + +describe('Gemini def carries the minVersion floor (#978)', () => { + it('ships with minVersion set to the first stable --output-format release', () => { + // Hard-pin the value so a future tweak that drops or changes the + // floor is intentional, not a refactor accident. + expect((gemini as { minVersion?: string }).minVersion).toBe('0.30.0'); + }); +}); From d6e41af6172de48b8e85208bff4eddfd88359c58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=8E=E5=86=A0=E8=BE=B0?= Date: Sat, 23 May 2026 23:31:05 +0800 Subject: [PATCH 2/4] fix(daemon): make every spawn path warm the version-aware cache, not just /api/agents (#978 review follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first round only warmed the chooser cache inside detection's probe(), so the three actual spawn paths — apps/daemon/src/server.ts:10455 (chat run), apps/daemon/src/connectionTest.ts:1135 (connection test), and apps/daemon/src/memory-llm.ts:827 (auto-memory extract) — still called the sync resolveAgentLaunch directly. inspectAgentExecutableResolution then read an empty cache and fell back to the first-PATH match. On a fresh daemon where /api/agents has not been hit yet (the typical 'open the app, start chatting' shape), the spawn could still land on the stale /usr/local/bin/gemini even though a modern Homebrew install sits later on PATH. Introduce resolveAgentLaunchWithMinVersion(def, configuredEnv): when def.minVersion is set, await chooseExecutableByMinVersion first; either way, return the same shape resolveAgentLaunch produces. Every spawn path switches to the async wrapper so the cache is guaranteed populated before the launch decision happens. detection.ts intentionally keeps the explicit two-step shape (await chooser, then sync resolveAgentLaunch) so the existing detection-resilience mock in tests/runtimes/detection-resilience.test.ts, which mocks module-level resolveAgentLaunch to simulate a per-agent throw, keeps working. The second connectionTest.ts call site (testAgentConnection at 1562) keeps the sync resolveAgentLaunch — testAgentConnectionInternal above it already warmed the cache via the async wrapper, so this synchronous read sees the same version-checked pick that the spawn just used (the diagnostics summary must match what actually ran). --- apps/daemon/src/agents.ts | 2 +- apps/daemon/src/connectionTest.ts | 14 ++- apps/daemon/src/memory-llm.ts | 9 +- apps/daemon/src/runtimes/detection.ts | 17 ++-- apps/daemon/src/runtimes/launch.ts | 22 ++++- apps/daemon/src/server.ts | 8 +- .../runtimes/version-aware-resolver.test.ts | 90 +++++++++++++++++++ 7 files changed, 151 insertions(+), 11 deletions(-) diff --git a/apps/daemon/src/agents.ts b/apps/daemon/src/agents.ts index 2195fd961..66e2b33ad 100644 --- a/apps/daemon/src/agents.ts +++ b/apps/daemon/src/agents.ts @@ -10,7 +10,7 @@ export { inspectAgentExecutableResolution, resolveAgentExecutable, } from './runtimes/executables.js'; -export { applyAgentLaunchEnv, resolveAgentLaunch } from './runtimes/launch.js'; +export { applyAgentLaunchEnv, resolveAgentLaunch, resolveAgentLaunchWithMinVersion } from './runtimes/launch.js'; export { resolveAgentBin } from './runtimes/resolution.js'; export { spawnEnvForAgent } from './runtimes/env.js'; export { buildLiveArtifactsMcpServersForAgent } from './runtimes/mcp.js'; diff --git a/apps/daemon/src/connectionTest.ts b/apps/daemon/src/connectionTest.ts index 95514ddd2..f19da6d8d 100644 --- a/apps/daemon/src/connectionTest.ts +++ b/apps/daemon/src/connectionTest.ts @@ -27,6 +27,7 @@ import { applyAgentLaunchEnv, getAgentDef, resolveAgentLaunch, + resolveAgentLaunchWithMinVersion, spawnEnvForAgent, } from './agents.js'; import { @@ -1664,7 +1665,13 @@ async function testAgentConnectionInternal( validateAgentCliEnv(input.agentCliEnv), input.agentId, ); - const executableResolution = resolveAgentLaunch(def, configuredAgentEnv); + // Async variant so any def with `minVersion` (currently Gemini, + // #978) warms the version-aware resolver cache before the + // connection test spawn picks a binary — otherwise the test could + // succeed against the version-checked pick while the actual chat + // run still launches the stale first-PATH match (or vice versa), + // making the test result misleading. + const executableResolution = await resolveAgentLaunchWithMinVersion(def, configuredAgentEnv); const resolvedBin = executableResolution.selectedPath; if (!resolvedBin || !executableResolution.launchPath) { return { @@ -2189,6 +2196,11 @@ export async function testAgentConnection( const configuredCodexBin = validatedPrefs?.codex?.CODEX_BIN?.trim() || ''; const configuredAgentEnv = agentCliEnvForAgent(validatedPrefs, input.agentId); const def = getAgentDef(input.agentId); + // testAgentConnectionInternal above already warmed the chooser + // cache for this agent via resolveAgentLaunchWithMinVersion, so + // this synchronous resolveAgentLaunch reads the same + // version-checked pick that the spawn just used (the diagnostics + // summary must match what actually ran). const executableResolution = def ? resolveAgentLaunch(def, configuredAgentEnv) : { diff --git a/apps/daemon/src/memory-llm.ts b/apps/daemon/src/memory-llm.ts index e0b570c61..8e9413200 100644 --- a/apps/daemon/src/memory-llm.ts +++ b/apps/daemon/src/memory-llm.ts @@ -66,6 +66,7 @@ import { applyAgentLaunchEnv, getAgentDef, resolveAgentLaunch, + resolveAgentLaunchWithMinVersion, spawnEnvForAgent, } from './agents.js'; import { agentCliEnvForAgent, readAppConfig } from './app-config.js'; @@ -811,7 +812,13 @@ async function callLocalCli(provider, system, user, options) { configuredAgentEnv = {}; } - const launch = resolveAgentLaunch(def, configuredAgentEnv); + // Async variant so any def with `minVersion` (currently Gemini, + // #978) warms the version-aware resolver cache before the + // memory-llm spawn picks a binary. Without this an auto-memory + // extract on a fresh daemon (no /api/agents call yet) could pick + // up a stale `/usr/local/bin/gemini` even when a modern Homebrew + // install sits later on PATH. + const launch = await resolveAgentLaunchWithMinVersion(def, configuredAgentEnv); if (!launch?.launchPath) { throw new Error(`${def.name} CLI is not installed or not on PATH`); } diff --git a/apps/daemon/src/runtimes/detection.ts b/apps/daemon/src/runtimes/detection.ts index 55a44ff59..f5fb08edb 100644 --- a/apps/daemon/src/runtimes/detection.ts +++ b/apps/daemon/src/runtimes/detection.ts @@ -132,12 +132,17 @@ async function probe( def: RuntimeAgentDef, configuredEnv: Record = {}, ): Promise { - // For agents that declare a minimum CLI version, run the - // version-aware resolver first so the version-checked binary is - // cached before `resolveAgentLaunch` reads it (#978). Without this - // pre-step, `inspectAgentExecutableResolution` would still return - // first-found and we would probe a stale binary even though a - // modern one sits later on the PATH/toolchain. + // For agents that declare a minimum CLI version, warm the + // version-aware resolver cache first so the version-checked binary + // is cached before `resolveAgentLaunch` reads it (#978). Without + // this pre-step, `inspectAgentExecutableResolution` would still + // return first-found and we would probe a stale binary even though + // a modern one sits later on the PATH/toolchain. The two-step + // shape (await chooser, then sync resolveAgentLaunch) — instead of + // the `resolveAgentLaunchWithMinVersion` helper that the spawn + // paths use — keeps the existing `detection-resilience` mock + // intact, which exercises module-level `resolveAgentLaunch` to + // simulate a single-agent throw. if (def.minVersion) { await chooseExecutableByMinVersion(def, configuredEnv); } diff --git a/apps/daemon/src/runtimes/launch.ts b/apps/daemon/src/runtimes/launch.ts index 0fbcf10c5..a23906fd1 100644 --- a/apps/daemon/src/runtimes/launch.ts +++ b/apps/daemon/src/runtimes/launch.ts @@ -1,6 +1,6 @@ import { accessSync, constants, readdirSync, readFileSync, realpathSync, statSync } from 'node:fs'; import path, { delimiter } from 'node:path'; -import { inspectAgentExecutableResolution } from './executables.js'; +import { chooseExecutableByMinVersion, inspectAgentExecutableResolution } from './executables.js'; import type { RuntimeAgentDef } from './types.js'; export type AgentLaunchKind = 'selected' | 'codex-native'; @@ -12,6 +12,26 @@ export type AgentLaunchResolution = ReturnType = {}, +): Promise { + if (def.minVersion) { + await chooseExecutableByMinVersion(def, configuredEnv); + } + return resolveAgentLaunch(def, configuredEnv); +} + export function resolveAgentLaunch( def: RuntimeAgentDef, configuredEnv: Record = {}, diff --git a/apps/daemon/src/server.ts b/apps/daemon/src/server.ts index 7a195e137..f2fe8063c 100644 --- a/apps/daemon/src/server.ts +++ b/apps/daemon/src/server.ts @@ -41,6 +41,7 @@ import { isKnownModel, applyAgentLaunchEnv, resolveAgentLaunch, + resolveAgentLaunchWithMinVersion, sanitizeCustomModel, spawnEnvForAgent, } from './agents.js'; @@ -11286,7 +11287,12 @@ export async function startServer({ configuredAgentEnv = {}; } - const agentLaunch = resolveAgentLaunch(def, configuredAgentEnv); + // Use the async variant so any def with `minVersion` set + // (currently Gemini, #978) warms the version-aware resolver cache + // before this spawn picks a binary. Without this the chat-run + // launch could land on the stale first-PATH match even when + // detection had not run yet. + const agentLaunch = await resolveAgentLaunchWithMinVersion(def, configuredAgentEnv); const resolvedBin = agentLaunch.selectedPath; // Hoisted above the AMR catalog preflight: the empty-catalog branch diff --git a/apps/daemon/tests/runtimes/version-aware-resolver.test.ts b/apps/daemon/tests/runtimes/version-aware-resolver.test.ts index 69952ecd8..46fbe1058 100644 --- a/apps/daemon/tests/runtimes/version-aware-resolver.test.ts +++ b/apps/daemon/tests/runtimes/version-aware-resolver.test.ts @@ -33,6 +33,7 @@ import { inspectAgentExecutableResolution, resolveAgentExecutable, } from '../../src/runtimes/executables.js'; +import { resolveAgentLaunchWithMinVersion } from '../../src/runtimes/launch.js'; import { gemini, minimalAgentDef } from './helpers/test-helpers.js'; const fsTest = process.platform === 'win32' ? it.skip : it; @@ -406,6 +407,95 @@ describe('inspectAgentExecutableResolution + minVersion cache wiring (#978)', () }); }); +describe('resolveAgentLaunchWithMinVersion (#978 round-2 review on PR #2797: every spawn path must warm the cache, not just /api/agents)', () => { + // The fix in PR #2797 round 1 only warmed the cache inside + // `probe()` (the detection path that feeds /api/agents). Actual + // spawn paths in server.ts, connectionTest.ts, and memory-llm.ts + // called the sync `resolveAgentLaunch` directly, so a chat-run / + // connection-test / memory-extract on a fresh daemon (no + // /api/agents call yet) still landed on the stale first-PATH + // match. The async wrapper guarantees the cache is populated + // before the launch decision happens, no matter which path + // triggered the spawn. + + fsTest('on a cold cache, returns the version-checked binary instead of the stale first-found one', async () => { + // Use real fixture binaries so the helper's default `runVersion` + // actually spawns them. Two shell scripts print fixed semvers; + // we trust the daemon's own Node binary is on PATH to resolve + // `#!/bin/sh` (which is always the case on macOS/Linux). + const usrLocal = mkdtempSync(join(tmpdir(), 'od-launch-usr-')); + const homebrew = mkdtempSync(join(tmpdir(), 'od-launch-brew-')); + try { + writeFileSync(join(usrLocal, 'gemini'), '#!/bin/sh\necho 0.1.12\n'); + writeFileSync(join(homebrew, 'gemini'), '#!/bin/sh\necho 0.40.1\n'); + chmodSync(join(usrLocal, 'gemini'), 0o755); + chmodSync(join(homebrew, 'gemini'), 0o755); + process.env.OD_AGENT_HOME = usrLocal; + process.env.PATH = `${usrLocal}${delimiter}${homebrew}`; + + const def = minimalAgentDef({ id: 'gemini', bin: 'gemini', minVersion: '0.30.0' }); + const launch = await resolveAgentLaunchWithMinVersion(def); + + expect(launch.selectedPath).toBe(join(homebrew, 'gemini')); + } finally { + rmSync(usrLocal, { recursive: true, force: true }); + rmSync(homebrew, { recursive: true, force: true }); + } + }); + + fsTest('skips the chooser when def.minVersion is undefined (no behavior change for other agents)', async () => { + // Pin the no-op shape: agents without `minVersion` (claude, codex, + // opencode, ...) must keep first-match resolution. The wrapper + // never calls the chooser for them, so even a stale binary on PATH + // wins (mirroring the pre-#978 status quo for those agents). + const a = mkdtempSync(join(tmpdir(), 'od-launch-noop-a-')); + const b = mkdtempSync(join(tmpdir(), 'od-launch-noop-b-')); + try { + writeFileSync(join(a, 'opencode'), ''); + writeFileSync(join(b, 'opencode'), ''); + chmodSync(join(a, 'opencode'), 0o755); + chmodSync(join(b, 'opencode'), 0o755); + process.env.OD_AGENT_HOME = a; + process.env.PATH = `${a}${delimiter}${b}`; + + // No minVersion on this def. + const def = minimalAgentDef({ id: 'opencode-test', bin: 'opencode' }); + const launch = await resolveAgentLaunchWithMinVersion(def); + + expect(launch.selectedPath).toBe(join(a, 'opencode')); + } finally { + rmSync(a, { recursive: true, force: true }); + rmSync(b, { recursive: true, force: true }); + } + }); + + fsTest('honors a configured `_BIN` override without probing', async () => { + // Reuses the override-skips-probes invariant the chooser already + // owns; assert it at the wrapper level so spawn callers can rely + // on it without knowing the internal layering. + const override = mkdtempSync(join(tmpdir(), 'od-launch-override-')); + const onPath = mkdtempSync(join(tmpdir(), 'od-launch-onpath-')); + try { + writeFileSync(join(override, 'gemini'), ''); + writeFileSync(join(onPath, 'gemini'), '#!/bin/sh\necho 0.40.1\n'); + chmodSync(join(override, 'gemini'), 0o755); + chmodSync(join(onPath, 'gemini'), 0o755); + process.env.OD_AGENT_HOME = onPath; + process.env.PATH = onPath; + + const def = minimalAgentDef({ id: 'gemini', bin: 'gemini', minVersion: '0.30.0' }); + const launch = await resolveAgentLaunchWithMinVersion(def, { + GEMINI_BIN: join(override, 'gemini'), + }); + + expect(launch.selectedPath).toBe(join(override, 'gemini')); + } finally { + rmSync(override, { recursive: true, force: true }); + rmSync(onPath, { recursive: true, force: true }); + } + }); +}); + describe('Gemini def carries the minVersion floor (#978)', () => { it('ships with minVersion set to the first stable --output-format release', () => { // Hard-pin the value so a future tweak that drops or changes the From c274e916898abbd69673b96e267fe9818336dfb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=8E=E5=86=A0=E8=BE=B0?= Date: Fri, 29 May 2026 20:41:49 +0800 Subject: [PATCH 3/4] fix(daemon): cache the version-aware resolver pick so every Gemini launch reuses it (#2797 review follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit chooseExecutableByMinVersion walked every PATH candidate and ran --version on each in parallel on every call. resolveAgentLaunchWithMinVersion awaits the chooser before each Gemini spawn, so a stale .cmd shim or hung wrapper hanging to the probe timeout (~1.5 s) added that delay to every chat-run, connection-test, and memory-extract launch — not just the first cold lookup. Consult the version-aware cache before re-probing. The cached value is gated by existsSync so a relocated or removed binary still triggers a fresh probe. clearVersionAwareResolutionCache() still wipes the entry for explicit resets. Regression: a new test counts probe invocations across two consecutive chooser calls and asserts the second call does zero probes; a sibling test removes the cached path between calls and asserts the chooser re-probes onto the new location. Surfaced by mrcfps's inline review on launch.ts:30 of PR #2797. --- apps/daemon/src/runtimes/executables.ts | 15 ++++ .../runtimes/version-aware-resolver.test.ts | 78 +++++++++++++++++++ 2 files changed, 93 insertions(+) diff --git a/apps/daemon/src/runtimes/executables.ts b/apps/daemon/src/runtimes/executables.ts index 299f40f6f..f69d15085 100644 --- a/apps/daemon/src/runtimes/executables.ts +++ b/apps/daemon/src/runtimes/executables.ts @@ -386,6 +386,21 @@ export async function chooseExecutableByMinVersion( const override = configuredExecutableOverride(def, configuredEnv); if (override) return override; + // Cached fast path: reuse the resolution from a prior call without + // re-probing every PATH candidate. `probeVersionWithTimeout` has a + // ~1.5 s timeout per candidate and the enumeration below waits on + // `Promise.all`, so a stale `.cmd` shim or a hung wrapper would add + // that delay to every chat-run / connection-test / memory-extract + // launch instead of only the first cold lookup (#2797 mrcfps review + // on `launch.ts:30`). The cache is cleared on negative resolution + // (line ~390 below) and via `clearVersionAwareResolutionCache()`, so + // a missing or relocated cached pick still gets re-probed at the + // next launch. + const cached = versionAwareCache.get(def.id); + if (cached && existsSync(cached)) { + return cached; + } + // Enumerate every match for def.bin and (if declared) any fallback // bins, in the same order resolveOnPath would walk. const candidates: string[] = []; diff --git a/apps/daemon/tests/runtimes/version-aware-resolver.test.ts b/apps/daemon/tests/runtimes/version-aware-resolver.test.ts index 46fbe1058..15609f9fe 100644 --- a/apps/daemon/tests/runtimes/version-aware-resolver.test.ts +++ b/apps/daemon/tests/runtimes/version-aware-resolver.test.ts @@ -295,6 +295,84 @@ describe('chooseExecutableByMinVersion (#978: skip stale binaries that fail the rmSync(ok, { recursive: true, force: true }); } }); + + fsTest('second call reuses the cached pick without re-probing every PATH candidate (mrcfps review on PR #2797)', async () => { + // Background: chooseExecutableByMinVersion enumerates every PATH + // candidate and runs `--version` on each in parallel. Without a + // cached fast path, a stale `.cmd` shim that hangs to the probe + // timeout (1.5 s) would add that delay to every chat-run / + // connection-test / memory-extract launch instead of only the + // first cold lookup. The fix consults the version-aware cache + // before re-probing, and the regression below proves a second + // call uses zero `runVersion` invocations. + const flaky = mkdtempSync(join(tmpdir(), 'od-cache-flaky-')); + const ok = mkdtempSync(join(tmpdir(), 'od-cache-ok-')); + try { + writeFileSync(join(flaky, 'gemini'), ''); + writeFileSync(join(ok, 'gemini'), ''); + chmodSync(join(flaky, 'gemini'), 0o755); + chmodSync(join(ok, 'gemini'), 0o755); + process.env.OD_AGENT_HOME = flaky; + process.env.PATH = `${flaky}${delimiter}${ok}`; + + const def = minimalAgentDef({ id: 'gemini', bin: 'gemini', minVersion: '0.30.0' }); + let probes = 0; + const runVersion = async (p: string) => { + probes += 1; + if (p === join(flaky, 'gemini')) return '0.1.12'; + return '0.40.1'; + }; + + // First call: cold cache — probes every candidate (>= 2 here). + const firstChosen = await chooseExecutableByMinVersion(def, {}, { runVersion }); + expect(firstChosen).toBe(join(ok, 'gemini')); + const probesAfterCold = probes; + expect(probesAfterCold).toBeGreaterThanOrEqual(2); + + // Second call: warm cache — must not invoke runVersion at all. + const secondChosen = await chooseExecutableByMinVersion(def, {}, { runVersion }); + expect(secondChosen).toBe(join(ok, 'gemini')); + expect(probes).toBe(probesAfterCold); + } finally { + rmSync(flaky, { recursive: true, force: true }); + rmSync(ok, { recursive: true, force: true }); + } + }); + + fsTest('cached pick that no longer exists on disk is re-probed instead of returned stale', async () => { + // existsSync gate on the cached value: if the user removed or + // moved the binary between launches, the cache must NOT serve a + // dangling path. Re-probe and re-cache instead. + const oldDir = mkdtempSync(join(tmpdir(), 'od-cache-old-')); + const newDir = mkdtempSync(join(tmpdir(), 'od-cache-new-')); + try { + writeFileSync(join(oldDir, 'gemini'), ''); + chmodSync(join(oldDir, 'gemini'), 0o755); + process.env.OD_AGENT_HOME = oldDir; + process.env.PATH = oldDir; + + const def = minimalAgentDef({ id: 'gemini', bin: 'gemini', minVersion: '0.30.0' }); + const firstChosen = await chooseExecutableByMinVersion(def, {}, { + runVersion: async () => '0.40.1', + }); + expect(firstChosen).toBe(join(oldDir, 'gemini')); + + // User moves the binary: cached path no longer exists. + rmSync(join(oldDir, 'gemini')); + writeFileSync(join(newDir, 'gemini'), ''); + chmodSync(join(newDir, 'gemini'), 0o755); + process.env.OD_AGENT_HOME = newDir; + process.env.PATH = newDir; + + const secondChosen = await chooseExecutableByMinVersion(def, {}, { + runVersion: async () => '0.40.1', + }); + expect(secondChosen).toBe(join(newDir, 'gemini')); + } finally { + rmSync(oldDir, { recursive: true, force: true }); + rmSync(newDir, { recursive: true, force: true }); + } + }); }); describe('inspectAgentExecutableResolution + minVersion cache wiring (#978)', () => { From b6f0c562b34878dd310d18dabd5ec1732f332302 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=8E=E5=86=A0=E8=BE=B0?= Date: Sun, 31 May 2026 12:55:47 +0800 Subject: [PATCH 4/4] fix(daemon): invalidate Gemini resolver cache on binary replacement --- apps/daemon/src/runtimes/executables.ts | 91 ++++++++++++++++--- .../runtimes/version-aware-resolver.test.ts | 42 +++++++++ 2 files changed, 122 insertions(+), 11 deletions(-) diff --git a/apps/daemon/src/runtimes/executables.ts b/apps/daemon/src/runtimes/executables.ts index f69d15085..fdc4da109 100644 --- a/apps/daemon/src/runtimes/executables.ts +++ b/apps/daemon/src/runtimes/executables.ts @@ -1,4 +1,4 @@ -import { accessSync, constants, existsSync, statSync } from 'node:fs'; +import { accessSync, constants, existsSync, realpathSync, statSync } from 'node:fs'; import { delimiter } from 'node:path'; import path from 'node:path'; import { homedir } from 'node:os'; @@ -286,10 +286,7 @@ export function inspectAgentExecutableResolution( // falling back to first-match (#978). let pathResolvedPath: string | null = null; if (!configuredOverridePath && def.minVersion) { - const cached = versionAwareCache.get(def.id); - if (cached && existsSync(cached)) { - pathResolvedPath = cached; - } + pathResolvedPath = cachedVersionAwarePath(def.id); } if (!pathResolvedPath) { const candidates = [ @@ -324,13 +321,28 @@ export function inspectAgentExecutableResolution( const VERSION_PROBE_TIMEOUT_MS = 1_500; -// agent.id → resolved path that passed the version gate. Populated by -// `chooseExecutableByMinVersion`; consulted by +interface VersionAwareExecutableIdentity { + realpath: string; + dev: number; + ino: number; + mode: number; + size: number; + mtimeMs: number; + ctimeMs: number; +} + +interface VersionAwareCacheEntry { + path: string; + identity: VersionAwareExecutableIdentity; +} + +// agent.id → resolved path + file identity that passed the version gate. +// Populated by `chooseExecutableByMinVersion`; consulted by // `inspectAgentExecutableResolution` so the sync chat-spawn path sees // the same pick detection landed on. Only writes for the auto-pick path // — an explicit `_BIN` override is intentionally NOT cached so // clearing the env reliably falls back to auto-pick (#1007 round-2 P2). -const versionAwareCache = new Map(); +const versionAwareCache = new Map(); export function clearVersionAwareResolutionCache(agentId?: string): void { if (agentId === undefined) { @@ -340,6 +352,63 @@ export function clearVersionAwareResolutionCache(agentId?: string): void { } } +function readVersionAwareExecutableIdentity(filePath: string): VersionAwareExecutableIdentity | null { + try { + const realpath = realpathSync(filePath); + const stat = statSync(realpath); + if (!stat.isFile()) return null; + return { + realpath, + dev: stat.dev, + ino: stat.ino, + mode: stat.mode, + size: stat.size, + mtimeMs: stat.mtimeMs, + ctimeMs: stat.ctimeMs, + }; + } catch { + return null; + } +} + +function versionAwareExecutableIdentityEquals( + a: VersionAwareExecutableIdentity, + b: VersionAwareExecutableIdentity, +): boolean { + return ( + a.realpath === b.realpath && + a.dev === b.dev && + a.ino === b.ino && + a.mode === b.mode && + a.size === b.size && + a.mtimeMs === b.mtimeMs && + a.ctimeMs === b.ctimeMs + ); +} + +function cachedVersionAwarePath(agentId: string): string | null { + const cached = versionAwareCache.get(agentId); + if (!cached) return null; + const currentIdentity = readVersionAwareExecutableIdentity(cached.path); + if ( + !currentIdentity || + !versionAwareExecutableIdentityEquals(cached.identity, currentIdentity) + ) { + versionAwareCache.delete(agentId); + return null; + } + return cached.path; +} + +function rememberVersionAwarePath(agentId: string, selectedPath: string): void { + const identity = readVersionAwareExecutableIdentity(selectedPath); + if (!identity) { + versionAwareCache.delete(agentId); + return; + } + versionAwareCache.set(agentId, { path: selectedPath, identity }); +} + // Strict, anchored semver parse. Accepts a leading `v` and tolerates // trailing pre-release (`-rc.1`) / build metadata (`+build.5`) but // only major.minor.patch participates in comparison. Returns `null` @@ -396,8 +465,8 @@ export async function chooseExecutableByMinVersion( // (line ~390 below) and via `clearVersionAwareResolutionCache()`, so // a missing or relocated cached pick still gets re-probed at the // next launch. - const cached = versionAwareCache.get(def.id); - if (cached && existsSync(cached)) { + const cached = cachedVersionAwarePath(def.id); + if (cached) { return cached; } @@ -433,7 +502,7 @@ export async function chooseExecutableByMinVersion( for (const probe of probes) { const cmp = compareSemver(probe.version, def.minVersion); if (cmp !== null && cmp >= 0) { - versionAwareCache.set(def.id, probe.path); + rememberVersionAwarePath(def.id, probe.path); return probe.path; } } diff --git a/apps/daemon/tests/runtimes/version-aware-resolver.test.ts b/apps/daemon/tests/runtimes/version-aware-resolver.test.ts index 15609f9fe..64e3270a9 100644 --- a/apps/daemon/tests/runtimes/version-aware-resolver.test.ts +++ b/apps/daemon/tests/runtimes/version-aware-resolver.test.ts @@ -19,6 +19,7 @@ import { chmodSync, mkdirSync, mkdtempSync, + readFileSync, rmSync, writeFileSync, } from 'node:fs'; @@ -373,6 +374,47 @@ describe('chooseExecutableByMinVersion (#978: skip stale binaries that fail the rmSync(newDir, { recursive: true, force: true }); } }); + + fsTest('cached pick replaced in place with an older binary is re-probed instead of returned stale', async () => { + const cachedDir = mkdtempSync(join(tmpdir(), 'od-cache-replace-cached-')); + const fallbackDir = mkdtempSync(join(tmpdir(), 'od-cache-replace-fallback-')); + try { + const cachedGemini = join(cachedDir, 'gemini'); + const fallbackGemini = join(fallbackDir, 'gemini'); + writeFileSync(cachedGemini, '0.40.1\n'); + writeFileSync(fallbackGemini, '0.41.0\n'); + chmodSync(cachedGemini, 0o755); + chmodSync(fallbackGemini, 0o755); + process.env.OD_AGENT_HOME = cachedDir; + process.env.PATH = `${cachedDir}${delimiter}${fallbackDir}`; + + const def = minimalAgentDef({ id: 'gemini', bin: 'gemini', minVersion: '0.30.0' }); + let probes = 0; + const runVersion = async (p: string) => { + probes += 1; + return readFileSync(p, 'utf8'); + }; + + const firstChosen = await chooseExecutableByMinVersion(def, {}, { runVersion }); + expect(firstChosen).toBe(cachedGemini); + const probesAfterCold = probes; + expect(probesAfterCold).toBeGreaterThanOrEqual(2); + + // Simulate a package-manager rewrite at the same visible path: + // the path still exists, but it now points at an older build that + // should not keep bypassing the min-version gate until daemon restart. + rmSync(cachedGemini); + writeFileSync(cachedGemini, '0.1.12 downgraded in-place\n'); + chmodSync(cachedGemini, 0o755); + + const secondChosen = await chooseExecutableByMinVersion(def, {}, { runVersion }); + expect(secondChosen).toBe(fallbackGemini); + expect(probes).toBeGreaterThan(probesAfterCold); + } finally { + rmSync(cachedDir, { recursive: true, force: true }); + rmSync(fallbackDir, { recursive: true, force: true }); + } + }); }); describe('inspectAgentExecutableResolution + minVersion cache wiring (#978)', () => {