diff --git a/apps/workers/coder-acp-copilot/src/acp-client.test.ts b/apps/workers/coder-acp-copilot/src/acp-client.test.ts index f4a126ca..71538144 100644 --- a/apps/workers/coder-acp-copilot/src/acp-client.test.ts +++ b/apps/workers/coder-acp-copilot/src/acp-client.test.ts @@ -2,7 +2,20 @@ // Licensed under the MIT License. import { describe, it, expect, vi } from "vitest"; -import { runACPSession, selectModel, selectReasoningEffort, selectPermissionMode, formatModeError, formatToolArgs, formatToolContent, AUTOPILOT_MODE_ID } from "./acp-client.js"; +import { + runACPSession, + selectModel, + resolveRequestedModel, + getCurrentModelId, + hasModelSelectionCapability, + selectFirstAvailableNonDefaultModel, + selectReasoningEffort, + selectPermissionMode, + formatModeError, + formatToolArgs, + formatToolContent, + AUTOPILOT_MODE_ID, +} from "./acp-client.js"; import type * as acp from "@agentclientprotocol/sdk"; import os from "node:os"; @@ -288,6 +301,238 @@ describe("selectModel", () => { }); }); +describe("selectFirstAvailableNonDefaultModel", () => { + function makeSession( + overrides?: Partial + ): acp.NewSessionResponse { + return { + sessionId: "session-1", + ...overrides, + } as acp.NewSessionResponse; + } + + it("selects the first advertised model different from the current model", () => { + const session = makeSession({ + models: { + currentModelId: "default-model", + availableModels: [ + { modelId: "default-model", name: "Default Model" }, + { modelId: "alternate-model", name: "Alternate Model" }, + { modelId: "another-model", name: "Another Model" }, + ], + }, + }); + + expect(selectFirstAvailableNonDefaultModel(session)).toBe( + "alternate-model" + ); + }); + + it("returns undefined when the session does not advertise models", () => { + expect(selectFirstAvailableNonDefaultModel(makeSession())).toBeUndefined(); + }); + + it("returns undefined when only the current model is available", () => { + const session = makeSession({ + models: { + currentModelId: "default-model", + availableModels: [ + { modelId: "default-model", name: "Default Model" }, + ], + }, + }); + + expect(selectFirstAvailableNonDefaultModel(session)).toBeUndefined(); + }); + + it("selects a non-default model from a model config option", () => { + const session = makeSession({ + configOptions: [ + { + id: "model-picker", + category: "model", + name: "Model", + currentValue: "default-model", + options: [ + { value: "default-model", name: "Default Model" }, + { value: "alternate-model", name: "Alternate Model" }, + ], + type: "select", + }, + ], + }); + + expect(selectFirstAvailableNonDefaultModel(session)).toBe( + "alternate-model" + ); + }); + + it("selects a non-default model from a grouped model config option", () => { + const session = makeSession({ + configOptions: [ + { + id: "model-picker", + category: "model", + name: "Model", + currentValue: "default-model", + options: [ + { + group: "recommended", + name: "Recommended", + options: [ + { value: "default-model", name: "Default Model" }, + { value: "alternate-model", name: "Alternate Model" }, + ], + }, + ], + type: "select", + }, + ], + }); + + expect(selectFirstAvailableNonDefaultModel(session)).toBe( + "alternate-model" + ); + }); + + it("returns undefined when a model config option only lists the current model", () => { + const session = makeSession({ + configOptions: [ + { + id: "model-picker", + category: "model", + name: "Model", + currentValue: "default-model", + options: [ + { value: "default-model", name: "Default Model" }, + ], + type: "select", + }, + ], + }); + + expect(selectFirstAvailableNonDefaultModel(session)).toBeUndefined(); + }); + + it("prefers the models field when both representations are advertised", () => { + const session = makeSession({ + models: { + currentModelId: "models-default", + availableModels: [ + { modelId: "models-default", name: "Models Default" }, + { modelId: "models-alternate", name: "Models Alternate" }, + ], + }, + configOptions: [ + { + id: "model-picker", + category: "model", + name: "Model", + currentValue: "config-default", + options: [ + { value: "config-alternate", name: "Config Alternate" }, + ], + type: "select", + }, + ], + }); + + expect(selectFirstAvailableNonDefaultModel(session)).toBe( + "models-alternate" + ); + }); +}); + +describe("resolveRequestedModel", () => { + const session = { + sessionId: "session-1", + } as acp.NewSessionResponse; + + it("preserves a fixed model id", () => { + expect(resolveRequestedModel("fixed-model", session)).toBe("fixed-model"); + }); + + it("resolves a selector with the new session response", () => { + const selector = vi.fn().mockReturnValue("selected-model"); + + expect(resolveRequestedModel(selector, session)).toBe("selected-model"); + expect(selector).toHaveBeenCalledWith(session); + }); +}); + +describe("model selection session metadata", () => { + function makeSession( + overrides?: Partial + ): acp.NewSessionResponse { + return { + sessionId: "session-1", + ...overrides, + } as acp.NewSessionResponse; + } + + it("returns the current model advertised by the session", () => { + const session = makeSession({ + models: { + currentModelId: "default-model", + availableModels: [ + { modelId: "default-model", name: "Default Model" }, + ], + }, + }); + + expect(getCurrentModelId(session)).toBe("default-model"); + }); + + it("returns the current model from a model config option", () => { + const session = makeSession({ + configOptions: [ + { + id: "model-picker", + category: "model", + name: "Model", + currentValue: "default-model", + options: [], + type: "select", + }, + ], + }); + + expect(getCurrentModelId(session)).toBe("default-model"); + }); + + it("detects models-field capability", () => { + const session = makeSession({ + models: { + currentModelId: "default-model", + availableModels: [], + }, + }); + + expect(hasModelSelectionCapability(session)).toBe(true); + }); + + it("detects model config-option capability", () => { + const session = makeSession({ + configOptions: [ + { + id: "model-picker", + category: "model", + name: "Model", + currentValue: "default-model", + options: [], + type: "select", + }, + ], + }); + + expect(hasModelSelectionCapability(session)).toBe(true); + }); + + it("returns false when no model selection mechanism is advertised", () => { + expect(hasModelSelectionCapability(makeSession())).toBe(false); + }); +}); + describe("selectReasoningEffort", () => { function makeConnection(overrides?: Partial): acp.ClientSideConnection { return { diff --git a/apps/workers/coder-acp-copilot/src/acp-client.ts b/apps/workers/coder-acp-copilot/src/acp-client.ts index 4116977b..7d3c6072 100644 --- a/apps/workers/coder-acp-copilot/src/acp-client.ts +++ b/apps/workers/coder-acp-copilot/src/acp-client.ts @@ -127,8 +127,11 @@ export interface ACPClientOptions { mcpServers?: McpServerConfig[]; /** Session timeout in milliseconds (default: 30 min). Set to 0 to disable. */ sessionTimeoutMs?: number; - /** Model to select after the session is created (e.g. "gpt-5.4"). */ - model?: string; + /** + * Model to select after the session is created, or a selector resolved from + * the new session response for session-scoped policies and integration probes. + */ + model?: string | ACPModelSelector; /** Reasoning effort level to apply via ACP config option (e.g. "low", "medium", "high"). */ reasoningEffort?: string; /** Use shell to spawn the process (required on Windows for .cmd shim resolution). */ @@ -138,10 +141,89 @@ export interface ACPClientOptions { export interface ACPSessionResult { response: string; stopReason: string; - /** Model that was successfully activated via ACP set_model, or undefined if model selection was not requested or did not succeed. */ + /** Model active when the ACP session was created, when advertised by the agent. */ + initialModel?: string; + /** Model successfully activated through ACP, or undefined if selection was not requested or did not succeed. */ confirmedModel?: string; } +/** + * Select a model after ACP session creation, when session-scoped model + * capabilities are available. + */ +export type ACPModelSelector = ( + sessionResult: acp.NewSessionResponse +) => string | undefined; + +export function resolveRequestedModel( + model: string | ACPModelSelector | undefined, + sessionResult: acp.NewSessionResponse +): string | undefined { + return typeof model === "function" ? model(sessionResult) : model; +} + +export function getCurrentModelId( + sessionResult: acp.NewSessionResponse +): string | undefined { + return ( + sessionResult.models?.currentModelId ?? + getModelConfigOption(sessionResult)?.currentValue + ); +} + +function getModelConfigOption( + sessionResult: acp.NewSessionResponse +): acp.SessionConfigOption | undefined { + return sessionResult.configOptions?.find( + (option: acp.SessionConfigOption) => option.category === "model" + ); +} + +function getConfigOptionValues( + configOption: acp.SessionConfigOption +): string[] { + return configOption.options.flatMap((option) => + "value" in option + ? [option.value] + : option.options.map((groupedOption) => groupedOption.value) + ); +} + +export function hasModelSelectionCapability( + sessionResult: acp.NewSessionResponse +): boolean { + return ( + sessionResult.models != null || + getModelConfigOption(sessionResult) !== undefined + ); +} + +/** + * Choose the first advertised model that differs from the session default. + * + * Intended for integration coverage that must exercise model switching without + * depending on a server-controlled model id. + */ +export function selectFirstAvailableNonDefaultModel( + sessionResult: acp.NewSessionResponse +): string | undefined { + const models = sessionResult.models; + if (models) { + return models.availableModels.find( + (candidate) => candidate.modelId !== models.currentModelId + )?.modelId; + } + + const modelConfigOption = getModelConfigOption(sessionResult); + if (!modelConfigOption) { + return undefined; + } + + return getConfigOptionValues(modelConfigOption).find( + (value) => value !== modelConfigOption.currentValue + ); +} + /** * ACP Client implementation that handles permission requests and session updates */ @@ -293,28 +375,26 @@ export async function selectModel( } // Path 2: stable session/set_config_option with category "model" - if (sessionResult.configOptions) { - const modelConfigOption = sessionResult.configOptions.find( - (o) => o.category === "model" - ); - if (modelConfigOption) { - try { - await connection.setSessionConfigOption({ - sessionId: sessionResult.sessionId, - configId: modelConfigOption.id, - value: model, - }); - onLog(`Model set to "${model}" via session/set_config_option (configId: ${modelConfigOption.id})`); - return model; - } catch (err) { - onLog(`Warning: session/set_config_option failed for model "${model}": ${err instanceof Error ? err.message : String(err)}`); - return undefined; - } + const modelConfigOption = getModelConfigOption(sessionResult); + if (modelConfigOption) { + try { + await connection.setSessionConfigOption({ + sessionId: sessionResult.sessionId, + configId: modelConfigOption.id, + value: model, + }); + onLog(`Model set to "${model}" via session/set_config_option (configId: ${modelConfigOption.id})`); + return model; + } catch (err) { + onLog(`Warning: session/set_config_option failed for model "${model}": ${err instanceof Error ? err.message : String(err)}`); + return undefined; } } // Neither mechanism available — warn and continue - onLog(`Warning: agent does not advertise model selection capability (no models field or model config option); model "${model}" may not be honoured`); + if (!hasModelSelectionCapability(sessionResult)) { + onLog(`Warning: agent does not advertise model selection capability (no models field or model config option); model "${model}" may not be honoured`); + } return undefined; } @@ -571,10 +651,28 @@ export async function runACPSession( onLog(`Session config options: ${sessionResult.configOptions.map((o: acp.SessionConfigOption) => `${o.id}${o.category ? ` (${o.category})` : ""}`).join(", ")}`); } - // Select model if requested + // Select model if requested. A selector is resolved only after newSession + // because the current and available models are session-scoped. + const initialModel = getCurrentModelId(sessionResult); let confirmedModel: string | undefined; - if (model) { - confirmedModel = await selectModel(connection, sessionResult, model, onLog); + const requestedModel = resolveRequestedModel(model, sessionResult); + if (requestedModel) { + confirmedModel = await selectModel( + connection, + sessionResult, + requestedModel, + onLog + ); + } else if (typeof model === "function") { + if (hasModelSelectionCapability(sessionResult)) { + onLog( + "Warning: model selector did not choose a model from the advertised model options" + ); + } else { + onLog( + "Warning: agent does not advertise model selection capability (no models field or model config option)" + ); + } } // Set reasoning effort if requested @@ -606,6 +704,7 @@ export async function runACPSession( return { response: clientHandler.getResponse(), stopReason: promptResult.stopReason, + initialModel, confirmedModel, }; }; diff --git a/apps/workers/coder-acp-copilot/src/copilot-cli.integration.test.ts b/apps/workers/coder-acp-copilot/src/copilot-cli.integration.test.ts index b44fb1ea..0ebdb882 100644 --- a/apps/workers/coder-acp-copilot/src/copilot-cli.integration.test.ts +++ b/apps/workers/coder-acp-copilot/src/copilot-cli.integration.test.ts @@ -63,6 +63,7 @@ describe("coder-acp-copilot integration", async () => { const docker = new Docker(); let workerResult: { result: TestResult; exitCode: number } | undefined; + let modelSelectionResult: { result: TestResult; exitCode: number } | undefined; beforeAll(async () => { if (!dockerAvailable) return; @@ -89,12 +90,29 @@ describe("coder-acp-copilot integration", async () => { env.push( `GITHUB_TOKEN=${GITHUB_TOKEN}`, "TEST_PROMPT=Generate a Hello World REST API in Python using Flask.", - "TEST_MODEL=claude-opus-4.6", ); } workerResult = await runTestWorker(docker, { image: IMAGE_TAG, workingDir: WORKING_DIR, env }); log(`exit=${workerResult.exitCode} lastStep=${workerResult.result.lastStep}`); - }, 600_000); // 10 min for Docker build + + // Keep the real coding prompt on the session default. Exercise model + // switching in a separate short session so server-side model ordering + // cannot make the coding assertion flaky. + if (hasCredentials) { + modelSelectionResult = await runTestWorker(docker, { + image: IMAGE_TAG, + workingDir: WORKING_DIR, + env: [ + `GITHUB_TOKEN=${GITHUB_TOKEN}`, + "TEST_PROMPT=Reply with OK.", + "TEST_SELECT_NON_DEFAULT_MODEL=true", + ], + }); + log( + `model selection exit=${modelSelectionResult.exitCode} lastStep=${modelSelectionResult.result.lastStep}` + ); + } + }, 900_000); // 15 min for Docker build plus two authenticated sessions afterAll(async () => { // Image kept for faster re-runs. Use `docker system prune` to clean up. @@ -145,42 +163,49 @@ describe("coder-acp-copilot integration", async () => { ); // ----------------------------------------------------------------------- - // Model selection test: ACP set_model actually changes the active model + // Model selection test: ACP changes to an advertised non-default model // ----------------------------------------------------------------------- it.skipIf(!canRun)( - "honours the requested model via ACP set_model", + "selects an advertised non-default model via ACP", { timeout: 300_000 }, async () => { - const { result } = workerResult!; + const { result } = modelSelectionResult!; const first = result.prompts[0]; + expect( + first.error, + `Model selection probe failed: ${first?.error}` + ).toBeUndefined(); + expect(first.success).toBe(true); log(`confirmedModel=${first.confirmedModel}`); - // KNOWN LIMITATION: model selection depends on the server, not on us. The - // ACP newSession response must advertise a `models` field or a `configOptions` - // entry with category "model", and the subsequent set call must succeed. Any - // of those can change independently of the CLI version, so when the model is - // not confirmed we verify the graceful fallback rather than assert a model. if (first.confirmedModel === undefined) { - // selectModel() gives up on three distinct paths, and each one logs a - // different warning. Asserting only the capability message made the other - // two report "expected a capability warning" — which describes the test's - // assumption rather than what actually happened, and sends the reader - // looking for a capability problem that is not there. - const fallbackWarnings = [ - "does not advertise model selection capability", - "session/set_model failed", - "session/set_config_option failed", - ]; - const matched = fallbackWarnings.find((w) => result.logs?.some((l) => l.includes(w))); + const selectionUnavailable = result.logs?.some( + (line) => + line.includes("does not advertise model selection capability") || + line.includes( + "model selector did not choose a model from the advertised model options" + ) + ); + const selectionFailed = result.logs?.some( + (line) => + line.includes("session/set_model failed") || + line.includes("session/set_config_option failed for model") + ); + + expect( + selectionFailed, + "Dynamic model selection reached an ACP selection method but failed", + ).toBe(false); expect( - matched, - `selectModel() returned undefined without any known fallback warning. Logs:\n${(result.logs ?? []).join("\n")}`, - ).toBeDefined(); - log(`SKIPPED (model not confirmed: ${matched})`); + selectionUnavailable, + "Expected an explicit warning when no alternate advertised model can be selected", + ).toBe(true); + log("SKIPPED (server did not advertise an alternate selectable model)"); } else { - expect(first.confirmedModel).toBe("claude-opus-4.6"); + expect(first.initialModel, "Expected the session's initial model").toBeTruthy(); + expect(first.confirmedModel).not.toBe(first.initialModel); } }, ); diff --git a/apps/workers/coder-acp-copilot/src/test-worker.ts b/apps/workers/coder-acp-copilot/src/test-worker.ts index 2b6b5d28..bfcb9eb5 100644 --- a/apps/workers/coder-acp-copilot/src/test-worker.ts +++ b/apps/workers/coder-acp-copilot/src/test-worker.ts @@ -14,12 +14,28 @@ * Env vars: * GITHUB_TOKEN — GitHub PAT for Copilot auth (required to run prompts) * TEST_PROMPT / TEST_PROMPT_2 / TEST_MODEL — see test-utils/harness + * TEST_SELECT_NON_DEFAULT_MODEL — select an advertised non-default model + * when TEST_MODEL is not set */ import { runTestHarness } from "test-utils/harness"; -import { runACPSession } from "./acp-client.js"; +import { + runACPSession, + selectFirstAvailableNonDefaultModel, +} from "./acp-client.js"; + +const selectNonDefaultModel = + process.env.TEST_SELECT_NON_DEFAULT_MODEL === "true"; await runTestHarness({ - runSession: runACPSession, + runSession: (prompt, options) => + runACPSession(prompt, { + ...options, + model: + options.model ?? + (selectNonDefaultModel + ? selectFirstAvailableNonDefaultModel + : undefined), + }), command: "copilot", args: ["--acp", "--yolo"], getCredentialEnv: () => { diff --git a/packages/test-utils/src/harness.ts b/packages/test-utils/src/harness.ts index a1ce4e95..2a1b0ee2 100644 --- a/packages/test-utils/src/harness.ts +++ b/packages/test-utils/src/harness.ts @@ -30,6 +30,7 @@ export interface HarnessSessionOptions { export interface HarnessSessionResult { response: string; stopReason: string; + initialModel?: string; confirmedModel?: string; } @@ -149,6 +150,7 @@ export async function runTestHarness(config: TestHarnessConfig): Promise promptResult.success = true; promptResult.response = acpResult.response; promptResult.stopReason = acpResult.stopReason; + promptResult.initialModel = acpResult.initialModel; promptResult.confirmedModel = acpResult.confirmedModel; } catch (error) { const msg = error instanceof Error ? error.message : String(error); diff --git a/packages/test-utils/src/types.ts b/packages/test-utils/src/types.ts index 30317c9d..aea78738 100644 --- a/packages/test-utils/src/types.ts +++ b/packages/test-utils/src/types.ts @@ -11,7 +11,9 @@ export interface PromptResult { response?: string; stopReason?: string; error?: string; - /** Model confirmed active by ACP set_model, or undefined if not requested/unavailable. */ + /** Model active when the ACP session was created, when advertised by the agent. */ + initialModel?: string; + /** Model confirmed active through ACP, or undefined if not requested/unavailable. */ confirmedModel?: string; }