coder(providers): v2.3 provider-lifecycle phase 3 — generic ACP dispatch
ACP dispatch now spawns from the resolved registry's launch spec instead of a hardcoded per-name switch. acp-spawn.ts gains resolveLaunchSpec(resolved, installPath): launchCommand (config override / custom-ACP command) wins, else the kept resolveAcpSpawnArgs switch is the built-in fallback. acp-dispatch.ts spawns spec.binary/spec.args with env { ...process.env, ...spec.env }; dispatcher.ts loads the resolved def by task.agent and passes it through. Config-defined custom ACP providers dispatch with no new switch case. Built-in dispatch (opencode/goose/qwen) is byte-identical to pre-v2.3 — proven by a regression test (opencode->['acp'], goose->['acp'], qwen->['--acp'], binary=installPath ?? id, empty env -> plain process.env). Deliberate deviation from design's !installPath->null: the installPath ?? id fallback is preserved. setSessionMode/permission/streaming and the dispatcher poll/NOTIFY/running-guard untouched. 7 new acp-spawn.test.ts cases. No routes/UI (Phase 4+).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
73
apps/coder/src/services/__tests__/acp-spawn.test.ts
Normal file
73
apps/coder/src/services/__tests__/acp-spawn.test.ts
Normal file
@@ -0,0 +1,73 @@
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import { resolveLaunchSpec, resolveAcpSpawnArgs } from '../acp-spawn.js';
|
||||
import { buildResolvedRegistry } from '../provider-config-registry.js';
|
||||
import type { CoderProvidersFile } from '../provider-config.js';
|
||||
import { PROVIDERS } from '../provider-registry.js';
|
||||
|
||||
/** Resolved def for a provider id under the given config (default: no override). */
|
||||
function builtin(name: string, providers: CoderProvidersFile['providers'] = {}) {
|
||||
const def = buildResolvedRegistry(PROVIDERS, { providers }).get(name);
|
||||
if (!def) throw new Error(`no resolved def for ${name}`);
|
||||
return def;
|
||||
}
|
||||
|
||||
describe('resolveLaunchSpec', () => {
|
||||
// --- byte-identical built-in regression (the HARD CONSTRAINT) ---------------
|
||||
// These argv values are the pre-v2.3 resolveAcpSpawnArgs switch outputs and
|
||||
// MUST NOT change. spawn() is `spawn(spec.binary, spec.args, ...)`, so argv
|
||||
// parity here is dispatch parity.
|
||||
it('opencode (no override) → byte-identical argv ["acp"], binary = installPath', () => {
|
||||
const spec = resolveLaunchSpec(builtin('opencode'), '/usr/bin/opencode');
|
||||
expect(spec).not.toBeNull();
|
||||
expect(spec!.args).toEqual(['acp']); // pre-v2.3 value
|
||||
expect(spec!.binary).toBe('/usr/bin/opencode');
|
||||
expect(spec!.env).toBeUndefined();
|
||||
// cross-check against the switch source-of-truth
|
||||
expect(spec!.args).toEqual(resolveAcpSpawnArgs('opencode'));
|
||||
});
|
||||
|
||||
it('goose → ["acp"], qwen → ["--acp"] (byte-identical)', () => {
|
||||
expect(resolveLaunchSpec(builtin('goose'), '/usr/bin/goose')!.args).toEqual(['acp']);
|
||||
expect(resolveLaunchSpec(builtin('qwen'), '/usr/bin/qwen')!.args).toEqual(['--acp']);
|
||||
});
|
||||
|
||||
it('built-in with null installPath falls back to the bare id (pre-v2.3 `installPath ?? agent`)', () => {
|
||||
const spec = resolveLaunchSpec(builtin('opencode'), null);
|
||||
expect(spec!.binary).toBe('opencode');
|
||||
expect(spec!.args).toEqual(['acp']);
|
||||
});
|
||||
|
||||
it('non-ACP / unknown provider → null (claude has no ACP argv)', () => {
|
||||
expect(resolveLaunchSpec(builtin('claude'), '/usr/bin/claude')).toBeNull();
|
||||
expect(resolveLaunchSpec(builtin('boocode'), null)).toBeNull();
|
||||
});
|
||||
|
||||
// --- config-driven launch (the new capability) ------------------------------
|
||||
it('custom ACP entry → configured command + env reach the spec', () => {
|
||||
const def = builtin('amp-acp', {
|
||||
'amp-acp': { extends: 'acp', label: 'Amp', command: ['amp-acp', '--acp'], env: { AMP_KEY: 'x' } },
|
||||
});
|
||||
const spec = resolveLaunchSpec(def, '/usr/local/bin/amp-acp');
|
||||
expect(spec).not.toBeNull();
|
||||
expect(spec!.binary).toBe('amp-acp'); // command[0], not the resolved install path
|
||||
expect(spec!.args).toEqual(['--acp']); // command.slice(1)
|
||||
expect(spec!.env).toEqual({ AMP_KEY: 'x' });
|
||||
});
|
||||
|
||||
it('built-in WITH a config command override uses the override, not the switch default', () => {
|
||||
const def = builtin('opencode', { opencode: { command: ['opencode', 'acp', '--verbose'], env: { DEBUG: '1' } } });
|
||||
const spec = resolveLaunchSpec(def, '/usr/bin/opencode');
|
||||
expect(spec!.binary).toBe('opencode');
|
||||
expect(spec!.args).toEqual(['acp', '--verbose']);
|
||||
expect(spec!.env).toEqual({ DEBUG: '1' });
|
||||
});
|
||||
});
|
||||
|
||||
describe('acp-dispatch spawn wiring (documented pass-through)', () => {
|
||||
// dispatchViaAcp spawns `spawn(spec.binary, spec.args, { env: { ...process.env, ...spec.env } })`.
|
||||
// The env merge layers config env over process.env; for a built-in with no
|
||||
// config env, spec.env is undefined → { ...process.env } (byte-identical).
|
||||
it('built-in with no config env yields an undefined spec.env (→ plain process.env at spawn)', () => {
|
||||
expect(resolveLaunchSpec(builtin('opencode'), '/usr/bin/opencode')!.env).toBeUndefined();
|
||||
});
|
||||
});
|
||||
@@ -26,7 +26,8 @@ import type { Broker } from '@boocode/server/broker';
|
||||
import type { WsFrame } from '@boocode/server/ws-frames';
|
||||
import { spawn } from 'node:child_process';
|
||||
import { findThoughtLevelConfigId } from './acp-derive.js';
|
||||
import { resolveAcpSpawnArgs } from './acp-spawn.js';
|
||||
import { resolveLaunchSpec } from './acp-spawn.js';
|
||||
import { getResolvedRegistry, type ResolvedProviderDef } from './provider-config-registry.js';
|
||||
import { createAcpNdJsonStream } from './acp-stream.js';
|
||||
import { waitForPermissionResponse, waitForElicitationResponse, cancelPendingPermission } from './permission-waiter.js';
|
||||
import { mergeTaskCommands, getTaskCommands } from './agent-commands-cache.js';
|
||||
@@ -59,6 +60,9 @@ export interface AcpDispatchOpts {
|
||||
messageId?: string;
|
||||
broker?: Broker;
|
||||
installPath?: string;
|
||||
/** v2.3 phase 3: resolved registry def for launch-spec resolution. The
|
||||
* dispatcher loads this by task.agent; falls back to a registry lookup here. */
|
||||
resolved?: ResolvedProviderDef;
|
||||
signal?: AbortSignal;
|
||||
log: FastifyBaseLogger;
|
||||
}
|
||||
@@ -282,8 +286,12 @@ export async function dispatchViaAcp(opts: AcpDispatchOpts): Promise<AcpDispatch
|
||||
broker,
|
||||
} = opts;
|
||||
|
||||
const args = resolveAcpSpawnArgs(agent);
|
||||
if (!args) {
|
||||
// v2.3 phase 3: launch from the resolved registry def (config override /
|
||||
// custom-ACP command) with the built-in switch as the fallback. The dispatcher
|
||||
// passes `resolved`; fall back to a registry lookup if it didn't.
|
||||
const resolved = opts.resolved ?? getResolvedRegistry().get(agent);
|
||||
const spec = resolved ? resolveLaunchSpec(resolved, installPath ?? null) : null;
|
||||
if (!spec) {
|
||||
return {
|
||||
exitCode: 1,
|
||||
output: `Agent '${agent}' does not support ACP.`,
|
||||
@@ -293,12 +301,11 @@ export async function dispatchViaAcp(opts: AcpDispatchOpts): Promise<AcpDispatch
|
||||
};
|
||||
}
|
||||
|
||||
const binary = installPath ?? agent;
|
||||
log.info({ agent, binary, worktreePath, modeId, model: opts.model }, 'acp-dispatch: spawning');
|
||||
const child = spawn(binary, args, {
|
||||
log.info({ agent, binary: spec.binary, worktreePath, modeId, model: opts.model }, 'acp-dispatch: spawning');
|
||||
const child = spawn(spec.binary, spec.args, {
|
||||
cwd: worktreePath,
|
||||
stdio: ['pipe', 'pipe', 'pipe'],
|
||||
env: { ...process.env },
|
||||
env: { ...process.env, ...spec.env },
|
||||
});
|
||||
|
||||
const streamCtx = new AcpStreamContext(
|
||||
|
||||
@@ -1,5 +1,9 @@
|
||||
import type { ResolvedProviderDef } from './provider-config-registry.js';
|
||||
|
||||
/**
|
||||
* Resolve ACP spawn argv per provider (host-probe verified 2026-05-25).
|
||||
* Resolve ACP spawn argv per built-in provider (host-probe verified 2026-05-25).
|
||||
* Source of truth for built-in default argv — resolveLaunchSpec wraps these; it
|
||||
* does NOT replace them.
|
||||
*/
|
||||
export function resolveAcpSpawnArgs(agent: string): string[] | null {
|
||||
switch (agent) {
|
||||
@@ -13,6 +17,34 @@ export function resolveAcpSpawnArgs(agent: string): string[] | null {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* v2.3 phase 3: resolve the launch spec for an ACP dispatch (design.md §5.1).
|
||||
* Consults the resolved registry's launchCommand (config override or custom-ACP
|
||||
* entry) first; otherwise falls back to the built-in default argv above.
|
||||
*
|
||||
* Byte-identical to pre-v2.3 for built-ins with no override: binary is
|
||||
* `installPath ?? id` and args come from resolveAcpSpawnArgs — exactly the
|
||||
* `binary = installPath ?? agent` + `resolveAcpSpawnArgs(agent)` the dispatcher
|
||||
* used before. (Deliberate deviation from design §5.1's `!installPath → null`:
|
||||
* the old path spawned the bare agent name when install_path was missing, so we
|
||||
* preserve the `?? id` fallback rather than fail.)
|
||||
*/
|
||||
export function resolveLaunchSpec(
|
||||
resolved: ResolvedProviderDef,
|
||||
installPath: string | null,
|
||||
): { binary: string; args: string[]; env?: Record<string, string> } | null {
|
||||
if (resolved.launchCommand) {
|
||||
return {
|
||||
binary: resolved.launchCommand[0],
|
||||
args: resolved.launchCommand.slice(1),
|
||||
env: resolved.env,
|
||||
};
|
||||
}
|
||||
const args = resolveAcpSpawnArgs(resolved.id);
|
||||
if (!args) return null;
|
||||
return { binary: installPath ?? resolved.id, args, env: resolved.env };
|
||||
}
|
||||
|
||||
export function resolveAcpProbeBinaries(agent: string): string[] {
|
||||
return [agent];
|
||||
}
|
||||
|
||||
@@ -5,6 +5,7 @@ import type { WsFrame } from '@boocode/server/ws-frames';
|
||||
import type { Config } from '../config.js';
|
||||
import { createWorktree, diffWorktree, cleanupWorktree } from './worktrees.js';
|
||||
import { dispatchViaAcp } from './acp-dispatch.js';
|
||||
import { getResolvedRegistry } from './provider-config-registry.js';
|
||||
import { dispatchViaPty } from './pty-dispatch.js';
|
||||
import { clearTaskCommands, setTaskCommands } from './agent-commands-cache.js';
|
||||
import { getManifestCommands } from './provider-commands.js';
|
||||
@@ -340,6 +341,7 @@ export function createDispatcher(deps: Deps): { start(): void; stop(): Promise<v
|
||||
if (supportsAcp) {
|
||||
const result = await dispatchViaAcp({
|
||||
agent,
|
||||
resolved: getResolvedRegistry().get(agent),
|
||||
task: task.input,
|
||||
worktreePath,
|
||||
installPath: installPath ?? undefined,
|
||||
|
||||
Reference in New Issue
Block a user