diff --git a/packages/cli/src/acp/elicitation.ts b/packages/cli/src/acp/elicitation.ts index 7fad756ae96..30c628ef5e3 100644 --- a/packages/cli/src/acp/elicitation.ts +++ b/packages/cli/src/acp/elicitation.ts @@ -16,8 +16,9 @@ type InputField = Exclude type SelectField = Form.StringField | Form.MultiselectField // Form mode must not collect secrets, so only forms from flows known not to ask for credentials are elicited. -const ElicitedKind = Schema.Struct({ kind: Schema.Literals(["question", "websearch.provider"]) }) -const Credential = /password|passphrase|secret|token|api[_-]?key|credential|private[_-]?key/i +const QuestionKind = "question" +const ElicitedKind = Schema.Struct({ kind: Schema.Literals([QuestionKind, "websearch.provider"]) }) +const Credential = /password|passphrase|secret|token|api[_ -]?key|credential|private[_ -]?key/i const ToolSource = Schema.Struct({ tool: Schema.Struct({ id: Schema.String }) }) type Input = { @@ -57,9 +58,12 @@ export const reply = Effect.fn("cli.acp.elicitation.reply")(function* (input: In ) }) +export const UnshownQuestionMessage = + "The question couldn't be shown to the user in this client. Continue without an answer: make reasonable assumptions and state them, or ask the user in your reply if you can't proceed." + /** Cancels a form, interrupting its session when the server can't cancel it. */ -export function cancel(client: OpenCodeClient, form: AskedForm) { - return settle(() => client.session.form.cancel({ sessionID: form.sessionID, formID: form.id })).pipe( +function cancel(client: OpenCodeClient, form: AskedForm, message?: string) { + return settle(() => client.session.form.cancel({ sessionID: form.sessionID, formID: form.id, message })).pipe( Effect.catch(() => Effect.tryPromise(() => client.session.interrupt({ sessionID: form.sessionID })).pipe(Effect.ignore), ), @@ -68,15 +72,16 @@ export function cancel(client: OpenCodeClient, form: AskedForm) { /** * The form-mode schema for a form, or undefined when the form is cancelled instead: the client lacks form - * elicitation, the form is not from an allowed flow or has a field that looks like a credential, or ACP can't - * represent it faithfully. Unrepresentable forms have `external` fields, `when` conditions, a hidden required field - * without a default, a default outside a field's options, or a free-text answer alongside options that must also - * satisfy `required` or item bounds across both inputs. Hidden fields are not asked and answer with their default. + * elicitation, the form is not from an allowed flow or has a field whose key, title, description, or option label + * looks like a credential, or ACP can't represent it faithfully. Unrepresentable forms have `external` fields, `when` + * conditions, a hidden required field without a default, a default outside a field's options, or a free-text answer + * alongside options that must also satisfy `required` or item bounds across both inputs. Hidden fields are not asked + * and answer with their default. */ export function requestedSchema(form: AskedForm, capabilities: ACPService.Capabilities): ElicitationSchema | undefined { if (!capabilities.formElicitation) return undefined if (Option.isNone(Schema.decodeUnknownOption(ElicitedKind)(form.metadata))) return undefined - if (form.fields.some((field) => Credential.test(field.key) || Credential.test(field.title ?? ""))) return undefined + if (form.fields.some(credentialLike)) return undefined const fields = form.fields.filter((field): field is InputField => field.type !== "external") if (fields.length !== form.fields.length || fields.some((field) => field.when?.length)) return undefined if (fields.some((field) => field.hidden && field.required && field.default === undefined)) return undefined @@ -90,6 +95,14 @@ export function requestedSchema(form: AskedForm, capabilities: ACPService.Capabi } } +/** + * Cancels a form that `requestedSchema` won't show. A question is cancelled with a message the question tool returns + * to the model, so the turn continues instead of ending as interrupted; other forms are cancelled silently. + */ +export function cancelUnshown(client: OpenCodeClient, form: AskedForm) { + return cancel(client, form, form.metadata?.kind === QuestionKind ? UnshownQuestionMessage : undefined) +} + /** The answer for an accepted response, or undefined when the user declined, cancelled, or sent invalid content. */ export function answer(form: AskedForm, response: CreateElicitationResponse): Form.Answer | undefined { if (response.action !== "accept") return undefined @@ -137,6 +150,14 @@ function settle(evaluate: () => Promise) { ) } +function credentialLike(field: Form.Field) { + const labels = + field.type === "string" || field.type === "multiselect" ? (field.options ?? []).map((option) => option.label) : [] + return [field.key, field.title, field.description, ...labels].some( + (text) => text !== undefined && Credential.test(text), + ) +} + function representable(field: InputField, keys: ReadonlySet) { if (field.type !== "string" && field.type !== "multiselect") return true if (!hasOptions(field)) return true diff --git a/packages/cli/src/acp/turn.ts b/packages/cli/src/acp/turn.ts index 6aa00a0e06b..8821d4897a8 100644 --- a/packages/cli/src/acp/turn.ts +++ b/packages/cli/src/acp/turn.ts @@ -161,7 +161,7 @@ export const make = Effect.fnUntraced(function* (input: { return Effect.gen(function* () { const capabilities = yield* Ref.get(input.capabilities) const requestedSchema = ACPElicitation.requestedSchema(output.form, capabilities) - if (!requestedSchema) return yield* ACPElicitation.cancel(input.client, output.form) + if (!requestedSchema) return yield* ACPElicitation.cancelUnshown(input.client, output.form) const settled = yield* Deferred.make() subscription.forms.set(output.form.id, settled) yield* Queue.offer( diff --git a/packages/cli/test/acp/additional-directories.subprocess.test.ts b/packages/cli/test/acp/additional-directories.subprocess.test.ts index 779c8c5d690..2636018f4d8 100644 --- a/packages/cli/test/acp/additional-directories.subprocess.test.ts +++ b/packages/cli/test/acp/additional-directories.subprocess.test.ts @@ -2,30 +2,16 @@ import type { NewSessionResponse, PromptResponse } from "@agentclientprotocol/sd import { describe, expect, test } from "bun:test" import fs from "node:fs/promises" import path from "node:path" -import { createAcpFixture, expectOk, initialize } from "./subprocess" +import { createAcpFixture, expectOk, initialize, toolCallStream } from "./subprocess" // The first completion reads the file outside cwd; the follow-up completion ends the turn. function readingModel(file: () => string) { return (request: unknown) => { if (JSON.stringify(request).includes('"role":"tool"')) return "done" - return new Response(toolCall(file()), { headers: { "content-type": "text/event-stream" } }) + return toolCallStream("call_read", "read", { path: file() }) } } -function toolCall(file: string) { - const call = { index: 0, id: "call_read", type: "function", function: { name: "read", arguments: "" } } - const chunks = [ - { choices: [{ delta: { role: "assistant", tool_calls: [call] }, finish_reason: null }], usage: null }, - { - choices: [{ delta: { tool_calls: [{ index: 0, function: { arguments: JSON.stringify({ path: file }) } }] } }], - usage: null, - }, - { choices: [{ delta: {}, finish_reason: "tool_calls" }], usage: null }, - { choices: [], usage: { prompt_tokens: 10, completion_tokens: 1, total_tokens: 11 } }, - ] - return `${chunks.map((chunk) => `data: ${JSON.stringify(chunk)}\n\n`).join("")}data: [DONE]\n\n` -} - describe("acp additional directories subprocess", () => { test("tools read files in an additional directory without an external directory ask", async () => { const target = { file: "" } diff --git a/packages/cli/test/acp/elicitation.test.ts b/packages/cli/test/acp/elicitation.test.ts index 35c5dbc4dbb..5afb1a69b45 100644 --- a/packages/cli/test/acp/elicitation.test.ts +++ b/packages/cli/test/acp/elicitation.test.ts @@ -191,6 +191,8 @@ describe("acp elicitation mapping", () => { [{ key: "api_key", type: "string" }], [{ key: "q0", title: "GitHub token", type: "string" }], [{ key: "q0", title: "Password", type: "string", hidden: true, default: "" }], + [{ key: "q0", title: "Setup", description: "Paste your API key", type: "string" }], + [{ key: "q0", title: "Setup", type: "string", options: [{ value: "a", label: "Use my access token" }] }], ] expect(credentials.map((fields) => ACPElicitation.requestedSchema(form(fields), capable))).toEqual( credentials.map(() => undefined), @@ -332,6 +334,7 @@ describe("acp elicitation over the wire", () => { expect((await acp.prompt(acp.sessionId, "hello")).stopReason).toBe("end_turn") expect(acp.elicitations).toHaveLength(ids.length) expect(acp.server.cancelledForms.map((item) => item.formID)).toEqual(ids) + expect(acp.server.cancelledForms.map((item) => item.message)).toEqual(ids.map(() => undefined)) expect(acp.server.repliedForms).toEqual([]) expect(acp.server.interrupts).toEqual([]) }) @@ -359,6 +362,62 @@ describe("acp elicitation over the wire", () => { expect(acp.server.cancelledForms).toEqual([{ sessionID: acp.sessionId, formID: "frm_plugin" }]) }) + test("cancels a question that looks like it asks for a credential with a message for the model", async () => { + await using acp = await startSession({ + capabilities: { elicitation: true }, + onPrompt: ({ sessionID, id }) => [ + delivered(sessionID, id), + ephemeralEvent("form.created", { + form: { + id: "frm_token", + sessionID, + title: "Questions", + metadata: { kind: "question", tool: { messageID: "msg_tools", id: "call_question" } }, + fields: [ + { + key: "q0", + title: "Setup", + description: "Paste your API key", + type: "string", + options: [{ value: "Later", label: "Later" }], + custom: true, + }, + ], + }, + }), + ], + onFormCancel: ({ sessionID }) => [succeeded(sessionID)], + }) + + expect((await acp.prompt(acp.sessionId, "hello")).stopReason).toBe("end_turn") + expect(acp.elicitations).toEqual([]) + expect(acp.server.cancelledForms).toEqual([ + { sessionID: acp.sessionId, formID: "frm_token", message: ACPElicitation.UnshownQuestionMessage }, + ]) + }) + + test("cancels other forms it cannot show without a message", async () => { + await using acp = await startSession({ + onPrompt: ({ sessionID, id }) => [ + delivered(sessionID, id), + ephemeralEvent("form.created", { + form: { + id: "frm_websearch", + sessionID, + title: "Web search provider", + metadata: { kind: "websearch.provider" }, + fields: [{ key: "provider", type: "string", options: [{ value: "exa", label: "Exa" }] }], + }, + }), + ], + onFormCancel: ({ sessionID }) => [succeeded(sessionID)], + }) + + expect((await acp.prompt(acp.sessionId, "hello")).stopReason).toBe("end_turn") + expect(acp.elicitations).toEqual([]) + expect(acp.server.cancelledForms).toEqual([{ sessionID: acp.sessionId, formID: "frm_websearch" }]) + }) + test("cancelling the turn cancels its pending elicitation and the form, and never sends queued ones", async () => { await using acp = await startSession({ capabilities: { elicitation: true }, diff --git a/packages/cli/test/acp/question.subprocess.test.ts b/packages/cli/test/acp/question.subprocess.test.ts new file mode 100644 index 00000000000..2c37cfd3e0c --- /dev/null +++ b/packages/cli/test/acp/question.subprocess.test.ts @@ -0,0 +1,52 @@ +import type { PromptResponse } from "@agentclientprotocol/sdk" +import { describe, expect, test } from "bun:test" +import { Option, Schema } from "effect" +import { ACPElicitation } from "../../src/acp/elicitation" +import { createAcpFixture, expectOk, initialize, newSession, toolCallStream } from "./subprocess" + +const ChatRequest = Schema.Struct({ + messages: Schema.Array( + Schema.Struct({ role: Schema.String, tool_call_id: Schema.optional(Schema.String), content: Schema.Unknown }), + ), +}) + +// The first completion asks a question; the follow-up completion ends the turn. +function askingModel(request: unknown) { + if (JSON.stringify(request).includes('"role":"tool"')) return "done" + return toolCallStream("call_question", "question", { + questions: [ + { + header: "Runtime", + question: "Which runtime?", + options: [ + { label: "Bun", description: "Fast" }, + { label: "Node", description: "Stable" }, + ], + }, + ], + }) +} + +describe("acp question subprocess", () => { + test("a question the client cannot show returns to the model and the turn continues", async () => { + await using fixture = await createAcpFixture({ respond: askingModel }) + const acp = fixture.spawn() + await initialize(acp) + const session = await newSession(acp, fixture.home) + + const result = expectOk( + await acp.request("session/prompt", { + sessionId: session.sessionId, + prompt: [{ type: "text", text: "ask me something" }], + }), + ) + + expect(result.stopReason).toBe("end_turn") + const results = fixture.llm.requests + .flatMap((request) => Option.toArray(Schema.decodeUnknownOption(ChatRequest)(request))) + .flatMap((request) => request.messages) + .filter((message) => message.role === "tool" && message.tool_call_id === "call_question") + expect(results).toHaveLength(1) + expect(String(results[0]?.content)).toContain(ACPElicitation.UnshownQuestionMessage) + }, 60_000) +}) diff --git a/packages/cli/test/acp/subprocess.ts b/packages/cli/test/acp/subprocess.ts index 2968bd6117c..b8346d75e00 100644 --- a/packages/cli/test/acp/subprocess.ts +++ b/packages/cli/test/acp/subprocess.ts @@ -161,6 +161,30 @@ export function expectOk(response: JsonRpcResponse) { return response.result } +/** A streamed completion that calls one tool. */ +export function toolCallStream(id: string, name: string, input: unknown) { + const chunks = [ + { + choices: [ + { + delta: { role: "assistant", tool_calls: [{ index: 0, id, type: "function", function: { name, arguments: "" } }] }, + finish_reason: null, + }, + ], + usage: null, + }, + { + choices: [{ delta: { tool_calls: [{ index: 0, function: { arguments: JSON.stringify(input) } }] } }], + usage: null, + }, + { choices: [{ delta: {}, finish_reason: "tool_calls" }], usage: null }, + { choices: [], usage: { prompt_tokens: 10, completion_tokens: 1, total_tokens: 11 } }, + ] + return new Response(`${chunks.map((chunk) => `data: ${JSON.stringify(chunk)}\n\n`).join("")}data: [DONE]\n\n`, { + headers: { "content-type": "text/event-stream" }, + }) +} + function verifierConfig(llmUrl: string, skills?: string) { const model = { capabilities: { tools: true, input: ["text", "image"], output: ["text"] }, diff --git a/packages/cli/test/acp/turn.test.ts b/packages/cli/test/acp/turn.test.ts index 9b2caa21d12..3870e365f4d 100644 --- a/packages/cli/test/acp/turn.test.ts +++ b/packages/cli/test/acp/turn.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from "bun:test" import type { SessionNotification } from "@agentclientprotocol/sdk" import { resolve } from "node:path" +import { ACPElicitation } from "../../src/acp/elicitation" import { assistantMessage, childCreated, @@ -363,7 +364,9 @@ describe("acp turn events over the wire", () => { }) expect((await acp.prompt(acp.sessionId, "hello")).stopReason).toBe("end_turn") - expect(acp.server.cancelledForms).toEqual([{ sessionID: acp.sessionId, formID: "frm_question" }]) + expect(acp.server.cancelledForms).toEqual([ + { sessionID: acp.sessionId, formID: "frm_question", message: ACPElicitation.UnshownQuestionMessage }, + ]) expect(acp.elicitations).toEqual([]) }) diff --git a/packages/cli/test/acp/wire-fixture.ts b/packages/cli/test/acp/wire-fixture.ts index 41650c4e9e8..54fdcc4b3c1 100644 --- a/packages/cli/test/acp/wire-fixture.ts +++ b/packages/cli/test/acp/wire-fixture.ts @@ -569,7 +569,7 @@ function startServer(options: WireOptions, changed: () => void) { const selections: Selection[] = [] const interrupts: string[] = [] const replies: Array<{ readonly sessionID: string; readonly requestID: string; readonly decision: string }> = [] - const cancelledForms: Array<{ readonly sessionID: string; readonly formID: string }> = [] + const cancelledForms: Array<{ readonly sessionID: string; readonly formID: string; readonly message?: string }> = [] const repliedForms: FormReply[] = [] const mcp: Array<{ readonly name: string; readonly directory?: string; readonly config: unknown }> = [] const fake = { @@ -831,8 +831,8 @@ function startServer(options: WireOptions, changed: () => void) { }), }, "/api/session/:sessionID/form/:formID": { - DELETE: route(async (req) => { - const form = { sessionID: req.params.sessionID, formID: req.params.formID } + DELETE: route(async (req, query) => { + const form = { sessionID: req.params.sessionID, formID: req.params.formID, message: query.message } fake.cancelledForms.push(form) await emit(options.onFormCancel?.(form)) return noContent()