Skip to content

Commit fd5716d

Browse files
committed
fix(runtimes): make detection's chosen executable the one spawn sites use
The candidate walk taught detection to skip a binary it cannot execute, but every other resolution — chat, the connection test, the memory summariser, companion install — called `resolveAgentLaunch` with no skip list and got back the first file that merely exists on PATH: the broken shim detection had just rejected. Settings advertised the agent as installed while each turn exec'd the wrapper, so the fix only ever reached the picker. Detection is the only stage that spawns anything, so it is the only one that learns which candidate works. It now publishes that winner, and resolution prefers it over a fresh first-hit walk. An explicit `*_BIN` override and a packaged built-in still outrank it, a winner whose file has since vanished is ignored, and detection clears the entry before each pass so a rescan after a repair never resolves against a stale one. This restores the invariant the file already documented — detection probes the exact path the runtime will spawn — which the walk had broken. Reported by @mrcfps in review.
1 parent 0d99a5f commit fd5716d

3 files changed

Lines changed: 88 additions & 7 deletions

File tree

apps/daemon/src/runtimes/detection.ts

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,11 @@ import { spawnEnvForAgent } from './env.js';
1111
import { probeAgentAuthStatus } from './auth.js';
1212
import { agentCapabilities } from './capabilities.js';
1313
import { installMetaForAgent } from './metadata.js';
14-
import { resolveAmrOpenCodeExecutable } from './executables.js';
14+
import {
15+
forgetDetectedExecutable,
16+
rememberDetectedExecutable,
17+
resolveAmrOpenCodeExecutable,
18+
} from './executables.js';
1519
import { resolveAmrProfile } from '../integrations/vela.js';
1620
import {
1721
buildAuthDiagnostic,
@@ -254,6 +258,9 @@ async function probe(
254258
configuredEnv: Record<string, string> = {},
255259
): Promise<DetectedAgent> {
256260
detectedRuntimeVersions.delete(def.id);
261+
// Drop the previous winner before re-probing: a rescan after the user fixes
262+
// or removes a CLI must not resolve against a stale one.
263+
forgetDetectedExecutable(def.id);
257264
// Detection must probe the exact path the runtime will spawn, not just the
258265
// PATH-visible shim. This is load-bearing for Codex under nvm/fnm/mise:
259266
// the discovered `codex` entry is often a `#!/usr/bin/env node` wrapper
@@ -337,6 +344,14 @@ async function probe(
337344
);
338345
outcome = await probeVersionAtPath(def, next.launchPath, probeEnv);
339346
}
347+
// Publish the executable this pass proved invocable, so chat, the connection
348+
// test, and every other spawn site resolve to the same binary instead of
349+
// redoing the naive first-hit-on-PATH walk. This restores the invariant
350+
// stated above — detection probes the exact path the runtime will spawn —
351+
// which the candidate walk would otherwise have broken.
352+
if (outcome.kind !== 'not-invocable') {
353+
rememberDetectedExecutable(def.id, launch.selectedPath);
354+
}
340355
if (outcome.kind === 'not-invocable') {
341356
// Report the path that was actually tried. The agent picker only renders
342357
// an unavailable agent when it carries a path (that is what makes the

apps/daemon/src/runtimes/executables.ts

Lines changed: 46 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,39 @@ export function resolveAgentExecutable(
353353
return inspectAgentExecutableResolution(def, configuredEnv).selectedPath;
354354
}
355355

356+
// The executable a completed detection pass settled on, per agent id.
357+
//
358+
// Detection is the only stage that learns which candidate can actually be
359+
// executed — it is the only one that spawns anything. Without recording that
360+
// answer, every later resolution (chat, connection test, memory summariser,
361+
// companion install) would redo the naive "first hit on PATH" walk and land
362+
// back on the very shim detection just rejected: Settings would advertise the
363+
// agent as installed while each turn exec'd a broken wrapper. Publishing the
364+
// winner here keeps detection and launch pointed at the same binary.
365+
const detectedExecutables = new Map<string, string>();
366+
367+
/** Record the executable a detection pass proved invocable. */
368+
export function rememberDetectedExecutable(agentId: string, resolvedPath: string): void {
369+
detectedExecutables.set(agentId, resolvedPath);
370+
}
371+
372+
/**
373+
* Drop an agent's remembered executable. Detection clears it before each pass
374+
* so a re-scan after the user repairs or removes a CLI never resolves against
375+
* a stale winner.
376+
*/
377+
export function forgetDetectedExecutable(agentId: string): void {
378+
detectedExecutables.delete(agentId);
379+
}
380+
381+
function rememberedExecutable(agentId: string, skip: Set<string>): string | null {
382+
const remembered = detectedExecutables.get(agentId);
383+
if (!remembered || skip.has(remembered)) return null;
384+
// A remembered winner that has since been uninstalled must not outrank a
385+
// live PATH walk.
386+
return existsSync(remembered) ? remembered : null;
387+
}
388+
356389
export function inspectAgentExecutableResolution(
357390
def: RuntimeAgentDef,
358391
configuredEnv: Record<string, string> = {},
@@ -380,12 +413,19 @@ export function inspectAgentExecutableResolution(
380413
...(Array.isArray(def.fallbackBins) ? def.fallbackBins : []),
381414
];
382415
const skip = new Set(options.skipPathCandidates ?? []);
383-
let pathResolvedPath: string | null = null;
384-
outer: for (const bin of candidates) {
385-
for (const resolved of resolveAllOnPath(bin)) {
386-
if (skip.has(resolved)) continue;
387-
pathResolvedPath = resolved;
388-
break outer;
416+
// Prefer the candidate detection proved invocable over a fresh PATH walk,
417+
// which would otherwise return the first file that merely *exists* — the
418+
// broken shim detection already walked past. An explicit `*_BIN` override
419+
// and a packaged built-in still outrank both (see the selectedPath order
420+
// below), so this only replaces the guesswork, never a deliberate choice.
421+
let pathResolvedPath: string | null = rememberedExecutable(def.id, skip);
422+
if (!pathResolvedPath) {
423+
outer: for (const bin of candidates) {
424+
for (const resolved of resolveAllOnPath(bin)) {
425+
if (skip.has(resolved)) continue;
426+
pathResolvedPath = resolved;
427+
break outer;
428+
}
389429
}
390430
}
391431
const builtInPath = packagedBuiltInExecutable(def, configuredEnv);

apps/daemon/tests/runtimes/executable-fallback.test.ts

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { tmpdir } from 'node:os';
33
import path from 'node:path';
44
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
55
import { detectAgent } from '../../src/runtimes/detection.js';
6+
import { resolveAgentLaunch } from '../../src/runtimes/launch.js';
67
import type { RuntimeAgentDef } from '../../src/runtimes/types.js';
78

89
// A minimal agent def: no compatibility probe, so these cases isolate the
@@ -90,6 +91,31 @@ describe('agent executable resolution falls back past unusable candidates', () =
9091
expect(detected.version).toBe('0.1.0-rc.6');
9192
});
9293

94+
// Detection deciding an agent is usable is worthless if the spawn sites go
95+
// back to the binary detection just rejected: Settings would advertise the
96+
// agent as installed while every chat turn execs the broken shim. Detection
97+
// and launch have to agree on which executable this agent runs.
98+
it('makes every later launch resolve to the binary detection settled on', async () => {
99+
if (process.platform === 'win32') return;
100+
const brokenDir = tempDir('broken-launch');
101+
const goodDir = tempDir('good-launch');
102+
const brokenBin = writeBrokenShim(brokenDir);
103+
const goodBin = writeWorkingShim(goodDir);
104+
105+
process.env.OD_AGENT_HOME = goodDir;
106+
process.env.PATH = [brokenDir, goodDir].join(path.delimiter);
107+
108+
const detected = await detectAgent(def);
109+
expect(detected.available).toBe(true);
110+
expect(detected.path).toBe(goodBin);
111+
112+
// What chat, the connection test, memory-llm, and companion setup all call.
113+
const launch = resolveAgentLaunch(def);
114+
expect(launch.selectedPath).toBe(goodBin);
115+
expect(launch.selectedPath).not.toBe(brokenBin);
116+
expect(launch.launchPath).toBe(goodBin);
117+
});
118+
93119
// Even when every candidate is unusable, detection must surface the path it
94120
// actually tried. The picker hides an agent that reports no path at all, so
95121
// dropping it leaves the user with an invisible agent and no way to act.

0 commit comments

Comments
 (0)