diff --git a/packages/adapters/antigravity/NOTES.md b/packages/adapters/antigravity/NOTES.md index d7640e1d..1a7f2d7f 100644 --- a/packages/adapters/antigravity/NOTES.md +++ b/packages/adapters/antigravity/NOTES.md @@ -23,11 +23,17 @@ switchboard's supervised child lifecycle. ## Model catalog The probe reported human-facing names such as `Gemini 3.5 Flash (High)`. -`listModels()` runs `agy models` locally with a fixed argv and timeout, reports -slug-safe ids to Codor, and owns the reverse mapping back to display names at -spawn time. The catalog is never hard-coded here. If two names collapse to the -same slug, discovery fails rather than routing an operator selection to the -wrong model. +`listModels()` runs `agy models` as an async child with a fixed argv, a 20 s budget and +capped output, reports slug-safe ids to Codor, and owns the reverse mapping back to +display names at spawn time. The probe never occupies the event loop, and the child is +confirmed terminated on every path, including the timeout. The 20 s budget matches the +opencode probe; no slow `agy models` run was measured here, so a slower run surfaces as +the same retryable failure. The catalog is never hard-coded +here. If two names collapse to the same slug, discovery fails rather than routing an +operator selection to the wrong model; any failure is recorded per harness (`models_error`, +a controlled category, never raw CLI text) so the dialog offers a retry. The probe keeps +no CLI output on failure, so to diagnose a failing harness run `agy models` directly on +the daemon host. ## Resume boundary diff --git a/packages/adapters/antigravity/src/adapter.spec.ts b/packages/adapters/antigravity/src/adapter.spec.ts index 4fae14a5..026d28f2 100644 --- a/packages/adapters/antigravity/src/adapter.spec.ts +++ b/packages/adapters/antigravity/src/adapter.spec.ts @@ -197,6 +197,69 @@ describe('AntigravityAdapter', () => { await expect(collision.listModels()).rejects.toThrow("collide at slug 'a-b'"); }); + it('lets the event loop run while the CLI answers', async () => { + // Requirement: async bounded probe — discovery must not freeze the daemon. + const dir = mkdtempSync(join(tmpdir(), 'codor-antigravity-')); + temporary.push(dir); + const path = join(dir, 'agy'); + writeFileSync(path, '#!/usr/bin/env node\nsetTimeout(() => { process.stdout.write("Gemini 3.5 Flash (High)\\n"); }, 1200);\n'); + chmodSync(path, 0o755); + const start = Date.now(); + let firedAt = -1; + setTimeout(() => { firedAt = Date.now(); }, 20); + const catalog = await new AntigravityAdapter(path).listModels(); + expect(catalog.models).toEqual(['gemini-3.5-flash-high']); + expect(firedAt).toBeGreaterThanOrEqual(start); + expect(firedAt - start).toBeLessThan(1000); + }); + + it('fails rather than buffering unbounded CLI output', async () => { + // Requirement: bounded probe — a runaway listing must fail, not balloon memory. + const dir = mkdtempSync(join(tmpdir(), 'codor-antigravity-')); + temporary.push(dir); + const path = join(dir, 'agy'); + writeFileSync(path, "#!/usr/bin/env node\nprocess.stdout.write('x'.repeat(1_200_000));\n"); + chmodSync(path, 0o755); + await expect(new AntigravityAdapter(path).listModels()).rejects.toThrow(/exceeded/); + }); + + it('terminates the child when the budget runs out', async () => { + // Requirement: confirmed cleanup — no orphan survives a timed-out probe. + const dir = mkdtempSync(join(tmpdir(), 'codor-antigravity-')); + temporary.push(dir); + const path = join(dir, 'agy'); + const pidFile = join(dir, 'pid'); + writeFileSync(path, `#!/usr/bin/env node +require('node:fs').writeFileSync(${JSON.stringify(pidFile)}, String(process.pid)); +setInterval(() => {}, 1000); +`); + chmodSync(path, 0o755); + await expect(new AntigravityAdapter(path, 2000).listModels()).rejects.toThrow(/timed out/); + const pid = Number(readFileSync(pidFile, 'utf8')); + expect(Number.isInteger(pid)).toBe(true); + let alive = true; + try { + process.kill(pid, 0); + } catch { + alive = false; + } + expect(alive).toBe(false); + }); + + it('keeps CLI diagnostics out of probe errors', async () => { + // Requirement: harness stderr can carry secrets; it must not enter error text. + const dir = mkdtempSync(join(tmpdir(), 'codor-antigravity-')); + temporary.push(dir); + const path = join(dir, 'agy'); + writeFileSync(path, "#!/usr/bin/env node\nprocess.stderr.write('AUTH_TOKEN=hunter2\\n');\nprocess.exit(1);\n"); + chmodSync(path, 0o755); + const outcome = await new AntigravityAdapter(path).listModels().then( + () => 'resolved', + (reason: unknown) => (reason instanceof Error ? reason.message : String(reason)), + ); + expect(outcome).toBe(`Command failed: ${path} models (exit 1)`); + }); + it('classifies missing commands and nonzero exits as failed', async () => { const missing = new AntigravityAdapter(join(tmpdir(), 'codor-agy-missing')); expect((await collect(missing, missing.spawn({ cwd: process.cwd() }), 'hello')).at(-1)) diff --git a/packages/adapters/antigravity/src/adapter.ts b/packages/adapters/antigravity/src/adapter.ts index 2dc507ca..cbd3133c 100644 --- a/packages/adapters/antigravity/src/adapter.ts +++ b/packages/adapters/antigravity/src/adapter.ts @@ -48,6 +48,93 @@ function finalOutput(output: string, truncated: boolean): string { return `${utf8Prefix(output, MAX_OUTPUT_BYTES - markerBytes)}${TRUNCATED}`; } +/** Budget for the async `agy models` probe: covers the observed worst case + * without ever stalling the daemon, since the child no longer occupies the loop. */ +const MODEL_DISCOVERY_TIMEOUT_MS = 20_000; +/** Hard cap on captured `agy models` stdout, in bytes. */ +const MODEL_DISCOVERY_MAX_BYTES = 1_000_000; + +/** Confirm a discovery child is gone: SIGTERM, then SIGKILL on a finite bound. */ +async function stopDiscoveryProbe(child: ChildProcess, label: string): Promise { + if (child.exitCode !== null || child.signalCode !== null) return; + await new Promise((resolve, reject) => { + const finish = (error?: Error): void => { + clearTimeout(escalate); clearTimeout(deadline); child.off('exit', exited); + if (error) reject(error); else resolve(); + }; + const exited = (): void => finish(); + const escalate = setTimeout(() => { + try { child.kill('SIGKILL'); } catch { /* exit confirmation below remains required */ } + }, 1_000); + const deadline = setTimeout(() => finish(new Error(`${label} model probe did not exit`)), 2_000); + child.once('exit', exited); + try { + child.kill('SIGTERM'); + } catch (error) { finish(error instanceof Error ? error : new Error(`${label} model probe cleanup failed`)); } + }); +} + +/** + * Run `command models` without occupying the event loop: async spawn, capped + * output, timeout rejection, and confirmed child termination on every path. + */ +async function runBoundedModelsCommand( + command: string, + timeoutMs: number, + maxBytes: number, +): Promise { + let child: ChildProcess | undefined; + let deadline: ReturnType | undefined; + const budget = Number.isFinite(timeoutMs) && timeoutMs > 0 ? timeoutMs : MODEL_DISCOVERY_TIMEOUT_MS; + const work = async (): Promise => { + const spawned = spawn(command, ['models'], { stdio: ['ignore', 'pipe', 'ignore'] }); + child = spawned; + return await new Promise((resolve, reject) => { + let stdout = ''; + let stdoutBytes = 0; + let settled = false; + const fail = (error: Error): void => { if (!settled) { settled = true; reject(error); } }; + const done = (value: string): void => { if (!settled) { settled = true; resolve(value); } }; + spawned.stdout?.setEncoding('utf8'); + spawned.stdout?.on('data', (chunk: string) => { + stdoutBytes += Buffer.byteLength(chunk, 'utf8'); + if (stdoutBytes > maxBytes) { + fail(new Error(`${command} models output exceeded ${String(maxBytes)} bytes`)); + return; + } + stdout += chunk; + }); + spawned.once('error', (error) => { + fail(error instanceof Error ? error : new Error(String(error))); + }); + spawned.once('close', (code, signal) => { + // No stderr body here: harness diagnostics can carry secrets, and this + // text reaches both the server log and the client category. Exit evidence only. + if (code !== 0) { + fail(new Error( + `Command failed: ${command} models${signal ? ` (signal ${signal})` : ` (exit ${String(code)})`}`, + )); + return; + } + done(stdout); + }); + }); + }; + try { + return await Promise.race([ + work(), + new Promise((_resolve, reject) => { + deadline = setTimeout(() => { + reject(new Error(`${command} models timed out after ${String(budget)}ms`)); + }, budget); + }), + ]); + } finally { + clearTimeout(deadline); + if (child) await stopDiscoveryProbe(child, command); + } +} + export interface AntigravityInvocation { mode: 'plan' | 'accept-edits'; skipPermissions: boolean; @@ -116,7 +203,10 @@ export class AntigravityAdapter implements HarnessAdapter { private readonly children = new WeakMap(); private readonly displayNames = new Map(); - constructor(private readonly command = 'agy') {} + constructor( + private readonly command = 'agy', + private readonly modelDiscoveryTimeoutMs = MODEL_DISCOVERY_TIMEOUT_MS, + ) {} spawn(opts: SpawnOpts): Session { antigravityMode(opts.policy); @@ -132,17 +222,13 @@ export class AntigravityAdapter implements HarnessAdapter { } // harn:assume adapters-own-their-model-catalog ref=antigravity-model-catalog - listModels(): Promise { - const result = spawn.sync(this.command, ['models'], { - timeout: 5_000, - maxBuffer: 1_000_000, - encoding: 'utf8', - }); - if (result.error) return Promise.reject(result.error); - if (result.status !== 0) { - return Promise.reject(new Error(`Command failed: ${this.command} models`)); - } - const displays = String(result.stdout ?? '') + async listModels(): Promise { + const stdout = await runBoundedModelsCommand( + this.command, + this.modelDiscoveryTimeoutMs, + MODEL_DISCOVERY_MAX_BYTES, + ); + const displays = stdout .split(/\r?\n/) .map((line) => line.trim()) .filter(Boolean); diff --git a/packages/adapters/opencode/NOTES.md b/packages/adapters/opencode/NOTES.md index cb0b1d88..56178a96 100644 --- a/packages/adapters/opencode/NOTES.md +++ b/packages/adapters/opencode/NOTES.md @@ -133,17 +133,26 @@ It is captured once and never regenerated by the normal test suite. ## Model catalog (U3): discovered, not curated opencode is the one harness that can enumerate its own models, so `listModels()` DISCOVERS -them: it runs `opencode models` with a fixed argument vector (no shell), a 5s timeout and a -capped buffer, and reports `source: 'discovered'`. Nothing is hardcoded. +them: it runs `opencode models` as an async child with a fixed argument vector (no shell), +a 20 s budget and capped output, and reports `source: 'discovered'`. Nothing is hardcoded. +The probe never occupies the event loop, and the child is confirmed terminated on every +path, including the timeout. This matters because opencode's catalog is per-installation: run on the development host 2026-07-12 (no model spend) it printed 79 models drawn entirely from THAT machine's configured providers — `openai`, `opencode`, and `synthetic`, with zero Anthropic entries. Any hardcoded list would have been wrong for somebody. -Discovery is best-effort by design. A missing binary, a non-zero exit, a hang, or output the -daemon cannot validate all degrade silently to no list, and the dialog falls back to its -`Custom…` escape (placeholder `provider/model`, the form opencode's `--model` takes). +Discovery is best-effort by design. A missing binary, a non-zero exit, a hang past the +budget, or output the daemon cannot validate all throw, and the daemon records the failure +per harness (`models_error`, a controlled category) so the dialog offers a retry instead +of reporting no models; a prior catalog is kept when one exists. The 20 s budget covers the +observed 0.9–11.97 s spread of `opencode models`, but upstream allows 10 s per fetch attempt +with retries, so a slower run can still time out — and then surfaces as the same retryable +failure. The dialog falls back to its `Custom…` escape +(placeholder `provider/model`, the form opencode's `--model` takes) only while no catalog +was ever reported. The probe keeps no CLI output on failure, so to diagnose a failing +harness run `opencode models` directly on the daemon host. Thinking is supported and maps to `run --variant `. diff --git a/packages/adapters/opencode/src/adapter.spec.ts b/packages/adapters/opencode/src/adapter.spec.ts index fca0c076..8fa11f22 100644 --- a/packages/adapters/opencode/src/adapter.spec.ts +++ b/packages/adapters/opencode/src/adapter.spec.ts @@ -1,4 +1,4 @@ -import { chmodSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { chmodSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -178,6 +178,61 @@ describe('opencode model discovery', () => { await expect(new OpenCodeAdapter(stub('process.stderr.write("boom\\n"); process.exit(1);')).listModels()) .rejects.toThrow(); }); + + it('keeps CLI diagnostics out of probe errors', async () => { + // Requirement: harness stderr can carry secrets; it must not enter error text. + const command = stub("process.stderr.write('AUTH_TOKEN=hunter2\\n'); process.exit(1);"); + const outcome = await new OpenCodeAdapter(command).listModels().then( + () => 'resolved', + (reason: unknown) => (reason instanceof Error ? reason.message : String(reason)), + ); + expect(outcome).toBe(`Command failed: ${command} models (exit 1)`); + }); + + it('lets the event loop run while the CLI answers', async () => { + // Requirement: async bounded probe — discovery must not freeze the daemon. + const dir = mkdtempSync(join(tmpdir(), 'codor-opencode-models-')); + dirs.push(dir); + const command = join(dir, 'opencode'); + writeFileSync(command, '#!/usr/bin/env node\nsetTimeout(() => { console.log("anthropic/claude-sonnet-5\\nopenai/gpt-4o"); }, 1200);\n'); + chmodSync(command, 0o755); + const start = Date.now(); + let firedAt = -1; + setTimeout(() => { firedAt = Date.now(); }, 20); + const catalog = await new OpenCodeAdapter(command).listModels(); + expect(catalog.models).toEqual(['anthropic/claude-sonnet-5', 'openai/gpt-4o']); + expect(firedAt).toBeGreaterThanOrEqual(start); + expect(firedAt - start).toBeLessThan(1000); + }); + + it('fails rather than buffering unbounded CLI output', async () => { + // Requirement: bounded probe — a runaway listing must fail, not balloon memory. + const command = stub("process.stdout.write('x'.repeat(1_200_000));"); + await expect(new OpenCodeAdapter(command).listModels()).rejects.toThrow(/exceeded/); + }); + + it('terminates the child when the budget runs out', async () => { + // Requirement: confirmed cleanup — no orphan survives a timed-out probe. + const dir = mkdtempSync(join(tmpdir(), 'codor-opencode-models-')); + dirs.push(dir); + const command = join(dir, 'opencode'); + const pidFile = join(dir, 'pid'); + writeFileSync(command, `#!/usr/bin/env node +require('node:fs').writeFileSync(${JSON.stringify(pidFile)}, String(process.pid)); +setInterval(() => {}, 1000); +`); + chmodSync(command, 0o755); + await expect(new OpenCodeAdapter(command, 2000).listModels()).rejects.toThrow(/timed out/); + const pid = Number(readFileSync(pidFile, 'utf8')); + expect(Number.isInteger(pid)).toBe(true); + let alive = true; + try { + process.kill(pid, 0); + } catch { + alive = false; + } + expect(alive).toBe(false); + }); // harn:end windows-cli-adapters-resolve-command-shims }); diff --git a/packages/adapters/opencode/src/adapter.ts b/packages/adapters/opencode/src/adapter.ts index 63debd83..45c01604 100644 --- a/packages/adapters/opencode/src/adapter.ts +++ b/packages/adapters/opencode/src/adapter.ts @@ -19,6 +19,93 @@ import { createTurnTranslator } from './translate.js'; const DISCOVER_QUERY = 'SELECT id FROM session WHERE parent_id IS NULL ORDER BY time_updated DESC'; +/** Budget for the async `opencode models` probe: covers the observed worst case + * without ever stalling the daemon, since the child no longer occupies the loop. */ +const MODEL_DISCOVERY_TIMEOUT_MS = 20_000; +/** Hard cap on captured `opencode models` stdout, in bytes. */ +const MODEL_DISCOVERY_MAX_BYTES = 1_000_000; + +/** Confirm a discovery child is gone: SIGTERM, then SIGKILL on a finite bound. */ +async function stopDiscoveryProbe(child: ChildProcess, label: string): Promise { + if (child.exitCode !== null || child.signalCode !== null) return; + await new Promise((resolve, reject) => { + const finish = (error?: Error): void => { + clearTimeout(escalate); clearTimeout(deadline); child.off('exit', exited); + if (error) reject(error); else resolve(); + }; + const exited = (): void => finish(); + const escalate = setTimeout(() => { + try { child.kill('SIGKILL'); } catch { /* exit confirmation below remains required */ } + }, 1_000); + const deadline = setTimeout(() => finish(new Error(`${label} model probe did not exit`)), 2_000); + child.once('exit', exited); + try { + child.kill('SIGTERM'); + } catch (error) { finish(error instanceof Error ? error : new Error(`${label} model probe cleanup failed`)); } + }); +} + +/** + * Run `command models` without occupying the event loop: async spawn, capped + * output, timeout rejection, and confirmed child termination on every path. + */ +async function runBoundedModelsCommand( + command: string, + timeoutMs: number, + maxBytes: number, +): Promise { + let child: ChildProcess | undefined; + let deadline: ReturnType | undefined; + const budget = Number.isFinite(timeoutMs) && timeoutMs > 0 ? timeoutMs : MODEL_DISCOVERY_TIMEOUT_MS; + const work = async (): Promise => { + const spawned = spawn(command, ['models'], { stdio: ['ignore', 'pipe', 'ignore'] }); + child = spawned; + return await new Promise((resolve, reject) => { + let stdout = ''; + let stdoutBytes = 0; + let settled = false; + const fail = (error: Error): void => { if (!settled) { settled = true; reject(error); } }; + const done = (value: string): void => { if (!settled) { settled = true; resolve(value); } }; + spawned.stdout?.setEncoding('utf8'); + spawned.stdout?.on('data', (chunk: string) => { + stdoutBytes += Buffer.byteLength(chunk, 'utf8'); + if (stdoutBytes > maxBytes) { + fail(new Error(`${command} models output exceeded ${String(maxBytes)} bytes`)); + return; + } + stdout += chunk; + }); + spawned.once('error', (error) => { + fail(error instanceof Error ? error : new Error(String(error))); + }); + spawned.once('close', (code, signal) => { + // No stderr body here: harness diagnostics can carry secrets, and this + // text reaches both the server log and the client category. Exit evidence only. + if (code !== 0) { + fail(new Error( + `Command failed: ${command} models${signal ? ` (signal ${signal})` : ` (exit ${String(code)})`}`, + )); + return; + } + done(stdout); + }); + }); + }; + try { + return await Promise.race([ + work(), + new Promise((_resolve, reject) => { + deadline = setTimeout(() => { + reject(new Error(`${command} models timed out after ${String(budget)}ms`)); + }, budget); + }), + ]); + } finally { + clearTimeout(deadline); + if (child) await stopDiscoveryProbe(child, command); + } +} + // harn:assume harness-declares-supported-thinking-levels ref=opencode-thinking-level-declaration export const OPENCODE_THINKING_LEVELS = [ 'low', @@ -91,7 +178,10 @@ export class OpenCodeAdapter implements HarnessAdapter { private readonly children = new WeakMap(); - constructor(private readonly command = 'opencode') {} + constructor( + private readonly command = 'opencode', + private readonly modelDiscoveryTimeoutMs = MODEL_DISCOVERY_TIMEOUT_MS, + ) {} spawn(opts: SpawnOpts): Session { if (opts.policy !== undefined && !PolicySchema.safeParse(opts.policy).success) { @@ -115,19 +205,16 @@ export class OpenCodeAdapter implements HarnessAdapter { /** * opencode's models come from the operator's OWN configured providers, so no * fixed list can be right for every install — ask the CLI. Fixed argv (no - * shell), hard timeout, capped output; a failure throws and the daemon - * silently degrades this harness to the custom escape. + * shell), async probe with a 20 s budget and capped output; a failure throws + * and the daemon records it per harness for a retryable dialog. */ async listModels(): Promise { - const result = spawn.sync(this.command, ['models'], { - timeout: 5_000, - maxBuffer: 1_000_000, - encoding: 'utf8', - }); - if (result.error) throw result.error; - if (result.status !== 0) throw new Error(`Command failed: ${this.command} models`); - const listed = result.stdout; - const models = listed.split('\n').map((line) => line.trim()).filter((line) => line !== ''); + const stdout = await runBoundedModelsCommand( + this.command, + this.modelDiscoveryTimeoutMs, + MODEL_DISCOVERY_MAX_BYTES, + ); + const models = stdout.split('\n').map((line) => line.trim()).filter((line) => line !== ''); if (models.length === 0) throw new Error('opencode listed no models'); return { models, source: 'discovered' }; } diff --git a/packages/switchboard/src/daemon.spec.ts b/packages/switchboard/src/daemon.spec.ts index 4fa597fe..abb8cfbd 100644 --- a/packages/switchboard/src/daemon.spec.ts +++ b/packages/switchboard/src/daemon.spec.ts @@ -4118,6 +4118,62 @@ describe('adapter model discovery', () => { expect(adapter!.id).toBe('broken'); }); + it('records a discovery failure on the adapter entry until a retry succeeds', async () => { + // Requirement: a failed discovery must surface as retryable, not as "no models". + const listModels = vi.fn(async (): Promise => { throw new Error('harness blew up'); }); + const daemon = daemonWith([adapterWith('flaky', listModels)]); + try { + await settle(); await settle(); + expect(daemon.registeredAdapters()[0]).toMatchObject({ id: 'flaky', models_error: 'unexpected error' }); + expect(daemon.registeredAdapters()[0]?.models).toBeUndefined(); + listModels.mockResolvedValue({ models: ['a/b'], source: 'discovered' }); + daemon.refreshAdapterAvailability(); await settle(); await settle(); + expect(daemon.registeredAdapters()[0]).toMatchObject({ models: ['a/b'] }); + expect(daemon.registeredAdapters()[0]?.models_error).toBeUndefined(); + } finally { await daemon.close(); } + }); + + it('keeps a prior catalog while reporting the latest failure', async () => { + // Requirement: a later failure must not erase a working list, but must say so. + const listModels = vi.fn(async (): Promise => ({ models: ['native/model'], source: 'discovered' })); + const daemon = daemonWith([adapterWith('codex', listModels)]); + try { + await settle(); await settle(); + listModels.mockRejectedValueOnce(new Error('probe timed out')); + daemon.refreshAdapterAvailability(); await settle(); await settle(); + expect(daemon.registeredAdapters()[0]).toMatchObject({ + models: ['native/model'], + models_error: 'timed out', + }); + } finally { await daemon.close(); } + }); + + it('publishes a controlled error category instead of raw CLI diagnostics', async () => { + // Requirement: credentials and paths in CLI output never cross the API boundary. + const daemon = daemonWith([ + adapterWith('leaky', () => Promise.reject(new Error( + `Command failed: /opt/agy models\nAUTH_TOKEN=hunter2\n${'x'.repeat(8000)}`, + ))), + adapterWith('slow', () => Promise.reject(new Error('agy models timed out after 20000ms'))), + adapterWith('missing', () => Promise.reject(new Error('spawn agy ENOENT'))), + adapterWith('empty', () => Promise.reject(new Error('agy listed no models'))), + adapterWith('huge', () => Promise.reject(new Error('agy models output exceeded 1000000 bytes'))), + ]); + try { + await settle(); await settle(); + const entries = new Map(daemon.registeredAdapters().map((entry) => [entry.id, entry])); + expect(entries.get('leaky')?.models_error).toBe('unexpected error'); + expect(entries.get('slow')?.models_error).toBe('timed out'); + expect(entries.get('missing')?.models_error).toBe('harness not installed'); + expect(entries.get('empty')?.models_error).toBe('harness reported no models'); + expect(entries.get('huge')?.models_error).toBe('output limit exceeded'); + for (const entry of entries.values()) { + expect(entry.models_error ?? '').not.toContain('hunter2'); + expect(entry.models_error ?? '').not.toContain('/opt'); + } + } finally { await daemon.close(); } + }); + it('drops output it cannot validate rather than trusting harness stdout', async () => { const daemon = daemonWith([ adapterWith('noisy', () => Promise.resolve({ diff --git a/packages/switchboard/src/daemon.ts b/packages/switchboard/src/daemon.ts index 59ede080..768602bb 100644 --- a/packages/switchboard/src/daemon.ts +++ b/packages/switchboard/src/daemon.ts @@ -343,6 +343,16 @@ function countDiffLines(diff: string): { additions: number; deletions: number } */ const MODEL_ID = /^\w[\w.:-]*(?:\/[\w.:-]+)*$/; const MAX_MODELS = 200; +/** Client-facing discovery outcome: a controlled category. Adapter failure + * details never cross the API boundary. (These two probes also discard + * harness stderr at the OS boundary, so their raw output is unrecoverable.) */ +function classifyDiscoveryError(message: string): string { + if (/timed out/i.test(message)) return 'timed out'; + if (/ENOENT/i.test(message)) return 'harness not installed'; + if (/listed no models/i.test(message)) return 'harness reported no models'; + if (/output exceeded/i.test(message)) return 'output limit exceeded'; + return 'unexpected error'; +} export interface DaemonOptions { dbPath: string; @@ -532,6 +542,9 @@ export class Daemon { private acpProviderCatalog: AcpProviderMetadata[] = []; // harn:end adapter-catalog-distinguishes-installed-and-configurable private readonly modelCatalogs = new Map(); + /** Latest discovery failure per adapter, so the dialog can tell "failed, retry" + * apart from "this harness reports no models". Cleared on the next success. */ + private readonly modelDiscoveryErrors = new Map(); private pendingDiscoveries = 0; private readonly modelDiscoveryWork = new Map>(); private readonly sessions = new Map(); @@ -1364,10 +1377,11 @@ export class Daemon { // harn:assume adapters-own-their-model-catalog ref=adapter-model-discovery /** * Ask every adapter that can answer what models its harness takes. Runs in - * the background at registration or explicit refresh — never blocking a request, because a hung - * CLI must not be able to wedge /api/adapters, which gates both dialogs. - * Any failure leaves the harness without a list: the dialog then offers the - * custom escape, which is a worse UI, not a broken one. + * the background at registration or explicit refresh — never blocking a request, + * because each probe is asynchronous and a hung CLI must not be able to wedge + * /api/adapters, which gates both dialogs. A failure is recorded per harness + * (see `models_error` in `registeredAdapters`) so the dialog can offer a retry; + * a prior catalog is kept when one exists. */ private discoverModels(): void { for (const adapter of this.adapters.values()) { @@ -1382,9 +1396,12 @@ export class Daemon { if (!Array.isArray(catalog.models)) throw new Error(`${adapter.id} returned an invalid model catalog`); const models = catalog.models.filter((model) => MODEL_ID.test(model)).slice(0, MAX_MODELS); this.modelCatalogs.set(adapter.id, { ...catalog, models }); - }).catch((error: unknown) => this.onBackgroundError( - error instanceof Error ? error : new Error(`${adapter.id} model discovery failed`), - )).finally(() => { + this.modelDiscoveryErrors.delete(adapter.id); + }).catch((error: unknown) => { + const failure = error instanceof Error ? error : new Error(`${adapter.id} model discovery failed`); + this.modelDiscoveryErrors.set(adapter.id, classifyDiscoveryError(failure.message)); + this.onBackgroundError(failure); + }).finally(() => { this.pendingDiscoveries -= 1; if (this.modelDiscoveryWork.get(adapter.id) === work) this.modelDiscoveryWork.delete(adapter.id); }); @@ -1441,6 +1458,8 @@ export class Daemon { capabilities: HarnessAdapter['capabilities']; models?: string[]; models_source?: ModelCatalog['source']; + /** Latest discovery failure as a controlled category; adapter failure details never cross the API. */ + models_error?: string; }[] { // Every entry carries its runtime harness id. The generic configurable ACP transport // is the sole `advanced` custom-command tile; named providers (acp:) follow as a @@ -1449,6 +1468,7 @@ export class Daemon { ...[...this.adapters.values()] .map((adapter) => { const catalog = this.modelCatalogs.get(adapter.id); + const modelsError = this.modelDiscoveryErrors.get(adapter.id); const configurable = (adapter as RegisteredHarnessAdapter).configurable === true; return { id: adapter.id, @@ -1457,6 +1477,7 @@ export class Daemon { ...(configurable && { configurable: true, transport: 'acp' as const, advanced: true }), capabilities: adapter.capabilities, ...(catalog && { models: catalog.models, models_source: catalog.source }), + ...(modelsError !== undefined && { models_error: modelsError }), }; }) .sort((a, b) => a.id.localeCompare(b.id)), diff --git a/packages/web-next/src/app/session.spec.ts b/packages/web-next/src/app/session.spec.ts index 4b566fe0..dac3cbcf 100644 --- a/packages/web-next/src/app/session.spec.ts +++ b/packages/web-next/src/app/session.spec.ts @@ -1,7 +1,10 @@ // @vitest-environment happy-dom -import { afterEach, describe, expect, it } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { act } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import { createElement } from 'react'; -import { pageParams, roomUrl } from './session.js'; +import { pageParams, roomUrl, useAdapterCatalog } from './session.js'; // harn:assume registered-worktree-navigation-is-promotion-gated ref=worktree-url-selector describe('worktree url selector', () => { @@ -30,3 +33,79 @@ describe('worktree url selector', () => { }); }); // harn:end registered-worktree-navigation-is-promotion-gated + +declare global { + var IS_REACT_ACT_ENVIRONMENT: boolean | undefined; +} + +globalThis.IS_REACT_ACT_ENVIRONMENT = true; + +function CatalogProbe(): null { + // Stable identity like the production accessors: a fresh closure per render + // would retrigger the catalog effect on every state update. + useAdapterCatalog(stableToken); + return null; +} + +function stableToken(): string { + return 't'; +} + +// harn:assume model-catalogs-reach-a-browser-that-arrives-early ref=adapter-discovery-poll-budget +describe('adapter discovery poll budget', () => { + let container: HTMLDivElement; + let root: Root | undefined; + let discovering = true; + + const fetchMock = vi.fn(async () => + new Response(JSON.stringify({ adapters: [], discovering }), { status: 200 }), + ); + + beforeEach(() => { + discovering = true; + fetchMock.mockClear(); + vi.stubGlobal('fetch', fetchMock); + vi.useFakeTimers(); + container = document.createElement('div'); + document.body.appendChild(container); + }); + + afterEach(async () => { + if (root !== undefined) { + const unmounting = root; + root = undefined; + await act(async () => { unmounting.unmount(); }); + } + container.remove(); + vi.unstubAllGlobals(); + vi.useRealTimers(); + }); + + async function mount(): Promise { + await act(async () => { + root = createRoot(container); + root.render(createElement(CatalogProbe)); + }); + } + + it('keeps polling while discovering past the slowest probe budget', async () => { + // Requirement: the slowest probe runs 30 s, so the client must still ask at 31 s. + await mount(); + await act(async () => { await vi.advanceTimersByTimeAsync(31_000); }); + expect(fetchMock.mock.calls.length).toBeGreaterThan(62); + await act(async () => { await vi.advanceTimersByTimeAsync(60_000); }); + expect(fetchMock.mock.calls.length).toBe(71); + await act(async () => { await vi.advanceTimersByTimeAsync(30_000); }); + expect(fetchMock.mock.calls.length).toBe(71); + }); + + it('stops polling once the catalog reports discovery finished', async () => { + await mount(); + await act(async () => { await vi.advanceTimersByTimeAsync(2_000); }); + discovering = false; + await act(async () => { await vi.advanceTimersByTimeAsync(10_000); }); + const settled = fetchMock.mock.calls.length; + await act(async () => { await vi.advanceTimersByTimeAsync(30_000); }); + expect(fetchMock.mock.calls.length).toBe(settled); + }); +}); diff --git a/packages/web-next/src/app/session.ts b/packages/web-next/src/app/session.ts index ea8642ac..4d8a3251 100644 --- a/packages/web-next/src/app/session.ts +++ b/packages/web-next/src/app/session.ts @@ -114,13 +114,21 @@ export interface AdapterCatalog { } // harn:assume agent-selection-shows-detected-acp-and-advanced-custom ref=detected-acp-catalog-client +/** + * Discovery poll budget: the slowest model probe is the copilot 30 s HTTP + * timeout, so keep asking well past that before treating the catalog as final. + * Probe budgets live in the adapters; this window must outlast the slowest one. + */ +const ADAPTER_POLL_ATTEMPTS = 70; +const ADAPTER_POLL_MS = 500; + export function useAdapterCatalog(token: () => string): AdapterCatalog { const [adapters, setAdapters] = useState([]); const [refreshing, setRefreshing] = useState(false); const [refreshError, setRefreshError] = useState(); const active = useRef(true); const timer = useRef>(); - const attemptsLeft = useRef(10); + const attemptsLeft = useRef(ADAPTER_POLL_ATTEMPTS); const poll = useCallback((listing: { adapters: AdapterRegistration[]; discovering: boolean }): void => { if (!active.current) return; @@ -130,12 +138,12 @@ export function useAdapterCatalog(token: () => string): AdapterCatalog { if (timer.current !== undefined) clearTimeout(timer.current); timer.current = setTimeout(() => { void fetchAdapters({ token: token() }).then(poll).catch(() => undefined); - }, 500); + }, ADAPTER_POLL_MS); }, [token]); useEffect(() => { active.current = true; - attemptsLeft.current = 10; + attemptsLeft.current = ADAPTER_POLL_ATTEMPTS; void fetchAdapters({ token: token() }).then(poll).catch(() => undefined); return () => { active.current = false; @@ -147,7 +155,7 @@ export function useAdapterCatalog(token: () => string): AdapterCatalog { if (refreshing) return; setRefreshing(true); setRefreshError(undefined); - attemptsLeft.current = 10; + attemptsLeft.current = ADAPTER_POLL_ATTEMPTS; void refreshAdapters({ token: token() }).then( (listing) => poll(listing), (error: unknown) => { diff --git a/packages/web-next/src/room/AgentControls.spec.tsx b/packages/web-next/src/room/AgentControls.spec.tsx new file mode 100644 index 00000000..a5159628 --- /dev/null +++ b/packages/web-next/src/room/AgentControls.spec.tsx @@ -0,0 +1,52 @@ +import { renderToStaticMarkup } from 'react-dom/server'; +import { describe, expect, it } from 'vitest'; + +import { AgentControls } from './AgentControls.js'; +import type { AdapterLike, AgentConfig } from './agent-spec.js'; + +const adapter = (over: Partial = {}): AdapterLike => ({ + id: 'opencode', + capabilities: { thinking: false }, + ...over, +}); + +const config = (over: Partial = {}): AgentConfig => ({ + harness: 'opencode', model: '', thinking: '', policy: '', ...over, +}); + +function render(adapters: AdapterLike[], current: AgentConfig): string { + return renderToStaticMarkup( + undefined} + behaviourSection={2} + permissionsSection={3} + idPrefix="agent" + />, + ); +} + +// harn:assume adapters-own-their-model-catalog ref=adapter-model-discovery-dialog +describe('model discovery failure', () => { + it('offers a retry when discovery failed instead of reporting no models', () => { + // Requirement: a failed discovery reads as retryable, not as "no models". + const markup = render([adapter({ models_error: 'timed out' })], config()); + expect(markup).toContain('data-testid="agent-model-error"'); + expect(markup).toContain('Press Refresh to retry'); + expect(markup).not.toContain('did not report a model list'); + }); + + it('keeps the no-models note when discovery neither reported nor failed', () => { + const markup = render([adapter()], config()); + expect(markup).toContain('did not report a model list'); + expect(markup).not.toContain('agent-model-error'); + }); + + it('warns on a stale list after a later refresh failure', () => { + // Requirement: a later failure keeps the working list but says so. + const markup = render([adapter({ models: ['a/b'], models_error: 'timed out' })], config()); + expect(markup).toContain('data-testid="agent-model-stale"'); + expect(markup).toContain('a/b'); + }); +}); diff --git a/packages/web-next/src/room/AgentControls.tsx b/packages/web-next/src/room/AgentControls.tsx index 91eb5b4d..4f4e348f 100644 --- a/packages/web-next/src/room/AgentControls.tsx +++ b/packages/web-next/src/room/AgentControls.tsx @@ -297,7 +297,13 @@ export function AgentControls(props: { aria-label="Model" data-testid={`${id}-model-input`} /> -

This harness did not report a model list.

+ {adapter?.models_error !== undefined ? ( +

+ Model discovery failed ({adapter.models_error}). Press Refresh to retry. +

+ ) : ( +

This harness did not report a model list.

+ )} ) : ( <> @@ -348,6 +354,11 @@ export function AgentControls(props: { aria-label="Custom model" data-testid={`${id}-model-custom`} /> + {adapter?.models_error !== undefined && ( +

+ Last refresh failed; showing the previous list. Press Refresh to retry. +

+ )} )} diff --git a/packages/web-next/src/room/agent-spec.ts b/packages/web-next/src/room/agent-spec.ts index dd6c2d0c..3219b3a5 100644 --- a/packages/web-next/src/room/agent-spec.ts +++ b/packages/web-next/src/room/agent-spec.ts @@ -50,6 +50,8 @@ export interface AdapterLike { approvals?: string; }; models?: string[]; + /** Latest discovery failure as a controlled category: the list is missing or stale, press Refresh to retry. */ + models_error?: string; } /** Handle length cap enforced by the protocol's member schema. */ diff --git a/packages/web-next/src/runtime/api.ts b/packages/web-next/src/runtime/api.ts index 9ff1fd58..357e9354 100644 --- a/packages/web-next/src/runtime/api.ts +++ b/packages/web-next/src/runtime/api.ts @@ -65,6 +65,8 @@ export interface AdapterRegistration { /** Models the harness itself reported. The web never hardcodes a model id. */ models?: string[]; models_source?: 'discovered' | 'curated'; + /** Latest discovery failure as a controlled category; adapter failure details never cross the API. */ + models_error?: string; } // harn:end agent-selection-shows-detected-acp-and-advanced-custom