From f17b15eaa52ab97db1fead6b4f27d38cdced7403 Mon Sep 17 00:00:00 2001 From: Shoubhit Dash Date: Tue, 6 Oct 2026 17:15:51 +0530 Subject: [PATCH] fix(acp): tighten cwd, cancel, delete, and terminal auth handling (#53359) --- packages/cli/src/acp/directories.ts | 8 +++ packages/cli/src/acp/service.ts | 6 +- packages/cli/src/acp/turn.ts | 30 ++++---- packages/cli/src/commands/commands.ts | 10 ++- packages/cli/src/commands/handlers/acp.ts | 14 +++- packages/cli/test/acp/prompt.test.ts | 88 +++++++++++++++-------- packages/cli/test/acp/session.test.ts | 29 +++++++- packages/cli/test/acp/wire-fixture.ts | 3 + packages/cli/test/auth.test.ts | 7 +- services/www/src/docs/content/cli/acp.mdx | 30 ++++++-- 10 files changed, 167 insertions(+), 58 deletions(-) diff --git a/packages/cli/src/acp/directories.ts b/packages/cli/src/acp/directories.ts index e57656d3f5d..5d0816abb27 100644 --- a/packages/cli/src/acp/directories.ts +++ b/packages/cli/src/acp/directories.ts @@ -2,6 +2,7 @@ import { isAbsolute, join, resolve } from "node:path" import { isDeepStrictEqual } from "node:util" import type { OpenCodeClient } from "@opencode/client/effect" import type { Permission } from "@opencode/schema/permission" +import { AbsolutePath } from "@opencode/schema/schema" import type { Session } from "@opencode/schema/session" import { FSUtil } from "@opencode/util/fs-util" import { Effect, Option, Schema } from "effect" @@ -13,6 +14,7 @@ const decodeStored = Schema.decodeUnknownOption(Schema.Array(Schema.String)) // Permission resources treat `*` and `?` as wildcards. export const parse = Effect.fnUntraced(function* (cwd: string, directories: readonly string[] = []) { + yield* parseCwd(cwd) const invalid = directories.find((directory) => !isAbsolute(directory) || /[*?]/.test(directory)) if (invalid !== undefined) return yield* new ACPError.InvalidAdditionalDirectoryError({ directory: invalid }) const root = FSUtil.resolve(cwd) @@ -21,6 +23,12 @@ export const parse = Effect.fnUntraced(function* (cwd: string, directories: read ) }) +export const parseCwd = Effect.fnUntraced(function* (cwd: string) { + if (!isAbsolute(cwd)) + return yield* new ACPError.InvalidRequestError({ message: `cwd must be an absolute path: ${cwd}`, field: "cwd" }) + return AbsolutePath.make(cwd) +}) + export function grant(directories: readonly string[]) { if (directories.length === 0) return {} return { permissions: rules(directories), metadata: { [key]: [...directories] } } diff --git a/packages/cli/src/acp/service.ts b/packages/cli/src/acp/service.ts index a001fdb1c44..55c97f1aff3 100644 --- a/packages/cli/src/acp/service.ts +++ b/packages/cli/src/acp/service.ts @@ -109,6 +109,7 @@ export function make(input: { description: "Run `opencode auth login` in the terminal", name: "Login with opencode", id: AuthMethodID, + ...(params.clientCapabilities?.auth?.terminal ? { type: "terminal" as const, args: ["--login"] } : {}), } if (params.clientCapabilities?._meta?.["terminal-auth"] === true) { authMethod._meta = { @@ -164,7 +165,9 @@ export function make(input: { listSessions: Effect.fnUntraced(function* (params) { const page = yield* input.client.session .list({ - ...(params.cwd ? { directory: AbsolutePath.make(params.cwd) } : {}), + ...(params.cwd !== undefined && params.cwd !== null + ? { directory: yield* ACPDirectories.parseCwd(params.cwd) } + : {}), order: "desc", limit: 100, ...(params.cursor ? { cursor: Schema.decodeSync(SessionsCursor)(params.cursor) } : {}), @@ -188,6 +191,7 @@ export function make(input: { } }), deleteSession: Effect.fnUntraced(function* (params) { + yield* input.turn.cancel({ sessionId: params.sessionId }) yield* ACPClient.decodeSessionID(params.sessionId).pipe( Effect.flatMap((sessionID) => input.client.session.remove({ sessionID })), Effect.catchTag(["ACPInvalidRequestError", "SessionNotFoundError"], () => Effect.void), diff --git a/packages/cli/src/acp/turn.ts b/packages/cli/src/acp/turn.ts index 34b7164f9a4..c02aeac4ef5 100644 --- a/packages/cli/src/acp/turn.ts +++ b/packages/cli/src/acp/turn.ts @@ -17,7 +17,7 @@ import { Stream, } from "effect" import type { Capabilities } from "./capabilities" -import type { ACPCatalog } from "./catalog" +import { findModel, type ACPCatalog } from "./catalog" import { ACPChild } from "./child" import { ACPClient } from "./client" import { currentModel } from "./config-option" @@ -317,10 +317,12 @@ export const make = Effect.fnUntraced(function* (input: { exit: Exit.Exit, ) { if (Exit.isFailure(exit) && !Cause.hasInterrupts(exit.cause)) return yield* Effect.failCause(exit.cause) - const failure = ACPTranslate.failure(current) + const terminal = Exit.isSuccess(exit) ? exit.value : "interrupted" + const failure = terminal === "interrupted" ? undefined : ACPTranslate.failure(current) if (failure) return yield* failure yield* sendUsageUpdate(attached, current) - return ACPTranslate.response(current, attached.id, Exit.isSuccess(exit) ? exit.value : "interrupted") + const cancelledWhileSettling = Exit.isFailure(yield* Effect.exit(Effect.interruptible(Effect.void))) + return ACPTranslate.response(current, attached.id, cancelledWhileSettling ? "interrupted" : terminal) }) const sendUsageUpdate = Effect.fn("cli.acp.turn.usage")( @@ -328,8 +330,7 @@ export const make = Effect.fnUntraced(function* (input: { const used = state.usage ? TokenUsage.total(state.usage.last) : 0 if (!used) return const catalog = yield* input.catalog.get(attached.cwd) - const current = currentModel(catalog, yield* Ref.get(attached.selection)) - const model = catalog.models.find((item) => item.providerID === current.providerID && item.id === current.id) + const model = findModel(catalog.models, currentModel(catalog, yield* Ref.get(attached.selection))) if (!model?.limit.context) return const info = yield* input.client.session.get({ sessionID: attached.id }).pipe(Effect.catch(ACPClient.classify)) yield* input.connection.sessionUpdate({ @@ -347,8 +348,10 @@ export const make = Effect.fnUntraced(function* (input: { ), ) - // Forked uninterruptible: interruption reaches only `execute`, so the fiber still settles with a response. - const run = Effect.fn("cli.acp.turn.run")(function* (attached: Attached, prompt: ACPPrompt.Prepared) { + // Forked uninterruptible: interruption lands only in `execute` and the last check in `settle`, so a response follows. + const run = Effect.fn("cli.acp.turn.run")(function* (params: PromptRequest) { + const attached = yield* input.sessions.require(params.sessionId) + const prompt = yield* ACPPrompt.prepare(yield* input.catalog.get(attached.cwd), params.prompt) const capabilities = yield* Ref.get(input.capabilities) const state = yield* Ref.make(ACPTranslate.initial) const exit = yield* Effect.acquireUseRelease( @@ -370,21 +373,18 @@ export const make = Effect.fnUntraced(function* (input: { return { prompt: Effect.fnUntraced(function* (params, signal) { - const attached = yield* input.sessions.require(params.sessionId) - const catalog = yield* input.catalog.get(attached.cwd) - const prompt = yield* ACPPrompt.prepare(catalog, params.prompt) - // Synchronous, so concurrent prompts for one session cannot both register. + // Synchronous and before setup, so concurrent prompts cannot both register and an early cancel still applies. const turn = yield* Effect.withFiber((fiber) => { - if (FiberMap.hasUnsafe(turns, attached.id)) { + if (FiberMap.hasUnsafe(turns, params.sessionId)) { return Effect.fail( new ACPError.ServiceFailureError({ - safeMessage: `Session already has an active ACP prompt: ${attached.id}`, + safeMessage: `Session already has an active ACP prompt: ${params.sessionId}`, service: "session", }), ) } - const forked = Effect.runForkWith(fiber.context)(run(attached, prompt), { uninterruptible: true }) - FiberMap.setUnsafe(turns, attached.id, forked) + const forked = Effect.runForkWith(fiber.context)(run(params), { uninterruptible: true }) + FiberMap.setUnsafe(turns, params.sessionId, forked) return Effect.succeed(forked) }) // A `$/cancel_request` for this prompt cancels its turn like `session/cancel`, rather than failing the request. diff --git a/packages/cli/src/commands/commands.ts b/packages/cli/src/commands/commands.ts index 6adc263a692..dc197ba8a17 100644 --- a/packages/cli/src/commands/commands.ts +++ b/packages/cli/src/commands/commands.ts @@ -96,7 +96,15 @@ const Root = Spec.make(typeof OPENCODE_CLI_NAME === "string" ? OPENCODE_CLI_NAME ), }, }), - Spec.make("acp", { description: "Start an Agent Client Protocol server" }), + Spec.make("acp", { + description: "Start an Agent Client Protocol server", + params: { + login: Flag.boolean("login").pipe( + Flag.withDescription("Run auth login instead of starting the server"), + Flag.withDefault(false), + ), + }, + }), Spec.make("api", { description: "Make a request to the running server", params: { diff --git a/packages/cli/src/commands/handlers/acp.ts b/packages/cli/src/commands/handlers/acp.ts index 6e53aeb5980..6323a6651b6 100644 --- a/packages/cli/src/commands/handlers/acp.ts +++ b/packages/cli/src/commands/handlers/acp.ts @@ -2,7 +2,7 @@ import { MessageTooLargeError, ndJsonStream } from "@agentclientprotocol/sdk" import { OpenCode } from "@opencode/client/effect" import { Service } from "@opencode/client/effect/service" import { CrossSpawnSpawner } from "@opencode/util/cross-spawn-spawner" -import { Effect } from "effect" +import { Effect, Option } from "effect" import { FetchHttpClient, HttpClient, HttpClientRequest } from "effect/unstable/http" import { Writable } from "node:stream" import { ACP } from "../../acp/agent" @@ -12,7 +12,17 @@ import { Standalone } from "../../services/standalone" export default Runtime.handler( Commands.commands.acp, - Effect.fn("cli.acp")(function* () { + Effect.fn("cli.acp")(function* (input) { + if (input.login) { + const login = yield* Effect.promise(() => import("./auth/login")) + return yield* login.default({ + target: Option.none(), + method: Option.none(), + answer: [], + server: Option.none(), + standalone: false, + }) + } process.env.OPENCODE_CLIENT = "acp" const endpoint = yield* Standalone.start() const client = yield* OpenCode.make({ baseUrl: endpoint.url }).pipe( diff --git a/packages/cli/test/acp/prompt.test.ts b/packages/cli/test/acp/prompt.test.ts index 18b65d97f89..a29aa5ceb06 100644 --- a/packages/cli/test/acp/prompt.test.ts +++ b/packages/cli/test/acp/prompt.test.ts @@ -33,10 +33,25 @@ import { type WireOptions, } from "./wire-fixture" -// A "hold" prompt is admitted and starts streaming, then only finishes when interrupted. +const lost = { type: "provider.transport", message: "stream connection lost" } + +// A "hold" prompt is admitted, streams, and fails a step, then waits on its retry until interrupted. const held = { onPrompt: ({ sessionID, id, text }) => - text === "hold" ? [delivered(sessionID, id), textDelta(sessionID, "msg_held", "working")] : turn(sessionID, id), + text === "hold" + ? [ + delivered(sessionID, id), + textDelta(sessionID, "msg_held", "working"), + durableEvent("session.step.failed", { sessionID, assistantMessageID: "msg_held", error: lost }), + durableEvent("session.retry.scheduled", { + sessionID, + assistantMessageID: "msg_held", + attempt: 1, + at: 0, + error: lost, + }), + ] + : turn(sessionID, id), onInterrupt: ({ sessionID }) => [interrupted(sessionID)], } satisfies WireOptions @@ -350,7 +365,9 @@ describe("acp prompt turns over the wire", () => { expect(await rpcError(acp.prompt(acp.sessionId, "hello"))).toMatchObject(expected) }) - test("session/cancel before admission aborts the submission and returns cancelled", async () => { + test("session/cancel during prompt setup or before admission returns cancelled", async () => { + await using dir = await tmpdir() + await Bun.write(path.join(dir.path, "notes.md"), "# notes\n") const aborted = Promise.withResolvers() await using acp = await startSession({ onPrompt: ({ signal }) => @@ -366,6 +383,15 @@ describe("acp prompt turns over the wire", () => { }), }) + // The linked file's readability check holds the prompt in setup, before it submits. + const preparing = acp.prompt(acp.sessionId, [ + { type: "resource_link", uri: pathToFileURL(path.join(dir.path, "notes.md")).href, name: "notes.md" }, + ]) + await acp.notify("session/cancel", { sessionId: acp.sessionId }) + + expect(await preparing).toEqual({ stopReason: "cancelled", _meta: {} }) + expect(acp.server.submissions).toEqual([]) + const prompt = acp.prompt(acp.sessionId, "hello") await acp.until(() => acp.server.submissions.length === 1, "prompt submission") await acp.notify("session/cancel", { sessionId: acp.sessionId }) @@ -479,35 +505,39 @@ describe("acp prompt turns over the wire", () => { expect((await acp.prompt(acp.sessionId, "again")).stopReason).toBe("end_turn") }) - test("session/close settles the active turn before responding and detaches only that session", async () => { - await using acp = await startSession(held) - const other = await acp.newSession() + test.each(["session/close", "session/delete"] as const)( + "%s settles the active turn before responding and detaches only that session", + async (method) => { + await using acp = await startSession(held) + const other = await acp.newSession() - const order: string[] = [] - const prompt = acp.prompt(acp.sessionId, "hold").then((response) => { - order.push("prompt") - return response - }) - await admitted(acp, acp.sessionId) - const close = await acp.request("session/close", { sessionId: acp.sessionId }).then((response) => { - order.push("close") - return response - }) + const order: string[] = [] + const prompt = acp.prompt(acp.sessionId, "hold").then((result) => { + order.push("prompt") + return result + }) + await admitted(acp, acp.sessionId) + const response = await acp.request(method, { sessionId: acp.sessionId }).then((result) => { + order.push(method) + return result + }) - expect(close).toEqual({}) - expect(await prompt).toMatchObject({ stopReason: "cancelled" }) - expect(order).toEqual(["prompt", "close"]) - expect(acp.server.interrupts).toEqual([acp.sessionId]) - expect(await rpcError(acp.prompt(acp.sessionId, "again"))).toMatchObject({ - code: -32602, - data: { sessionId: acp.sessionId }, - }) - expect((await acp.prompt(other.sessionId, "still here")).stopReason).toBe("end_turn") + expect(response).toEqual({}) + expect(await prompt).toMatchObject({ stopReason: "cancelled" }) + expect(order).toEqual(["prompt", method]) + expect(acp.server.interrupts).toEqual([acp.sessionId]) + expect(acp.server.sessions.has(acp.sessionId)).toBe(method === "session/close") + expect(await rpcError(acp.prompt(acp.sessionId, "again"))).toMatchObject({ + code: -32602, + data: { sessionId: acp.sessionId }, + }) + expect((await acp.prompt(other.sessionId, "still here")).stopReason).toBe("end_turn") - expect((await acp.prompt(other.sessionId, "/review now")).stopReason).toBe("end_turn") - await acp.request("session/close", { sessionId: other.sessionId }) - expect(acp.server.interrupts).toEqual([acp.sessionId, other.sessionId]) - }) + expect((await acp.prompt(other.sessionId, "/review now")).stopReason).toBe("end_turn") + await acp.request("session/close", { sessionId: other.sessionId }) + expect(acp.server.interrupts).toEqual([acp.sessionId, other.sessionId]) + }, + ) }) // The server answered admission before streaming the chunk, and this request round-trips through the server after it, diff --git a/packages/cli/test/acp/session.test.ts b/packages/cli/test/acp/session.test.ts index 9f95f9bdfc8..ea0586413fc 100644 --- a/packages/cli/test/acp/session.test.ts +++ b/packages/cli/test/acp/session.test.ts @@ -10,6 +10,10 @@ describe("acp session lifecycle over the wire", () => { const plain = await acp.initialize() const terminal = await acp.initialize({ terminalAuth: true, childSessionUpdates: true }) + const standard = await acp.request("initialize", { + protocolVersion: 1, + clientCapabilities: { auth: { terminal: true } }, + }) expect(plain).toMatchObject({ protocolVersion: 1, @@ -28,6 +32,15 @@ describe("acp session lifecycle over the wire", () => { expect(terminal.authMethods?.[0]?._meta).toEqual({ "terminal-auth": { command: "opencode", args: ["auth", "login"], label: "OpenCode Login" }, }) + expect(standard.authMethods).toEqual([ + { + id: "opencode-login", + name: "Login with opencode", + description: "Run `opencode auth login` in the terminal", + type: "terminal", + args: ["--login"], + }, + ]) expect(await acp.request("authenticate", { methodId: "opencode-login" })).toEqual({}) expect(await rpcError(acp.request("authenticate", { methodId: "missing" }))).toMatchObject({ code: -32602, @@ -165,7 +178,7 @@ describe("acp session lifecycle over the wire", () => { config: { type: "remote", url: "https://example.com/mcp", headers: { Authorization: "Bearer x" }, oauth: false }, }) }) - test("rejects MCP-over-ACP and SSE servers before creating or loading a session", async () => { + test("rejects relative cwds, MCP-over-ACP, and SSE servers before creating or loading a session", async () => { await using acp = await startWire() acp.server.sessions.set("ses_saved", makeSession("ses_saved")) await acp.initialize() @@ -177,12 +190,26 @@ describe("acp session lifecycle over the wire", () => { message: "Invalid params: Only stdio and HTTP MCP servers are supported", data: { field: "mcpServers" }, } + const relative = { + code: -32602, + message: "Invalid params: cwd must be an absolute path: workspace", + data: { field: "cwd" }, + } expect(await rpcError(acp.newSession("/workspace", mcpServers))).toEqual(invalid) expect(await rpcError(acp.newSession("/workspace", sse))).toEqual(invalid) expect( await rpcError(acp.request("session/load", { cwd: "/workspace", sessionId: "ses_saved", mcpServers })), ).toEqual(invalid) + expect(await rpcError(acp.newSession("workspace"))).toEqual(relative) + expect( + await rpcError(acp.request("session/load", { cwd: "workspace", sessionId: "ses_saved", mcpServers: [] })), + ).toEqual(relative) + expect(await rpcError(acp.request("session/list", { cwd: "workspace" }))).toEqual(relative) + expect(await rpcError(acp.request("session/list", { cwd: "" }))).toMatchObject({ + code: -32602, + data: { field: "cwd" }, + }) expect(new Set(acp.server.sessions.keys())).toEqual(existing) expect(acp.server.requests.filter((request) => request.path.includes("ses_saved"))).toEqual([]) expect(acp.logs).toEqual([]) diff --git a/packages/cli/test/acp/wire-fixture.ts b/packages/cli/test/acp/wire-fixture.ts index 9224e36aada..99c6be0a61b 100644 --- a/packages/cli/test/acp/wire-fixture.ts +++ b/packages/cli/test/acp/wire-fixture.ts @@ -751,6 +751,9 @@ function startServer(options: WireOptions, changed: () => void) { }) return noContent() }), + DELETE: route((req) => + fake.sessions.delete(req.params.sessionID) ? noContent() : notFound(req.params.sessionID), + ), }, "/api/session/:sessionID/fork": { POST: route((req) => { diff --git a/packages/cli/test/auth.test.ts b/packages/cli/test/auth.test.ts index 8f33f6a9a35..a0858247111 100644 --- a/packages/cli/test/auth.test.ts +++ b/packages/cli/test/auth.test.ts @@ -289,11 +289,14 @@ describe("auth command", () => { expect(removed).toEqual(["cred_test"]) }) - test("requires a target outside an interactive terminal", async () => { - const result = await cli(["auth", "login"]) + test("requires a target outside an interactive terminal, including through acp --login", async () => { + const [result, acp] = await Promise.all([cli(["auth", "login"]), cli(["acp", "--login"])]) expect(result.exitCode).toBe(1) expect(result.stdout).toContain("Pass an integration ID or name") expect(result.stdout).not.toContain("Background service failed to start") + expect(acp.exitCode).toBe(1) + expect(acp.stdout).toContain("Pass an integration ID or name when running without an interactive terminal") + expect(acp.stdout).not.toContain("Background service failed to start") }) test("reports list connection failures without a stack trace", async () => { diff --git a/services/www/src/docs/content/cli/acp.mdx b/services/www/src/docs/content/cli/acp.mdx index e2bfe5fda7c..6a444a234aa 100644 --- a/services/www/src/docs/content/cli/acp.mdx +++ b/services/www/src/docs/content/cli/acp.mdx @@ -28,9 +28,25 @@ OpenCode uses the provider credentials it already has. Sign in from a terminal b opencode auth login ``` -OpenCode offers one auth method, `opencode-login`. The `authenticate` request accepts only that method and doesn't -collect credentials. If a prompt fails because a provider needs authentication, OpenCode returns ACP's `auth_required` -error. +OpenCode offers one auth method, `opencode-login`. By default it's an agent auth method. The `authenticate` request +accepts only that method and doesn't collect credentials. If a prompt fails because a provider needs authentication, +OpenCode returns ACP's `auth_required` error. + +If the client sets `clientCapabilities.auth.terminal` to `true`, the auth method is a terminal auth method: + +```json +{ + "id": "opencode-login", + "name": "Login with opencode", + "description": "Run `opencode auth login` in the terminal", + "type": "terminal", + "args": ["--login"] +} +``` + +The client must not send this method to `authenticate`. Instead, it adds `--login` to the command that starts OpenCode, +for example `opencode acp --login`, and runs it in a terminal. OpenCode runs the `opencode auth login` flow and exits. +Exit status 0 means the login succeeded. The client then reconnects and sends `initialize` again. If the client sets `clientCapabilities._meta["terminal-auth"]` to `true`, the auth method includes the login command: @@ -43,9 +59,6 @@ If the client sets `clientCapabilities._meta["terminal-auth"]` to `true`, the au } ``` -OpenCode doesn't support ACP's standard terminal auth yet. That is the `clientCapabilities.auth.terminal` capability -with a `type: "terminal"` auth method. - ## Troubleshooting `opencode acp` waits for ACP messages on stdin, so it looks stuck when you run it directly in a terminal. Start it from @@ -97,6 +110,7 @@ OpenCode reads these client capabilities: | Capability | Effect | | -------------------------------------- | -------------------------------------------------------------- | +| `auth.terminal` set to `true` | The auth method is a [terminal auth method](#authentication). | | `elicitation.form` | OpenCode asks [questions](#questions) as forms. | | `session.compaction` | OpenCode reports [compaction](#compaction) with ACP's updates. | | `_meta["terminal-auth"]` set to `true` | The auth method includes the [login command](#authentication). | @@ -106,6 +120,7 @@ OpenCode reads these client capabilities: The client sets the working directory when it creates a session. OpenCode uses that directory's configuration, including its models, agents, commands, skills, instructions, plugins, and MCP servers. +- `cwd` must be an absolute path. OpenCode rejects a relative `cwd` with an invalid params error. - `session/load` replays the saved messages. `session/resume` and `session/fork` don't replay them. - To load or resume a session, the client must pass the session's own directory as `cwd`. OpenCode rejects a different `cwd` with an invalid params error. @@ -113,7 +128,8 @@ including its models, agents, commands, skills, instructions, plugins, and MCP s - `session/list` returns up to 100 sessions per page. It accepts a `cwd` filter and a `cursor`. - `session/close` interrupts active work and detaches the session from the ACP process. OpenCode keeps the saved session. -- `session/delete` removes the session. Deleting a session that doesn't exist succeeds. +- `session/delete` ends an active prompt like `session/cancel`, then removes the session. Deleting a session that + doesn't exist succeeds. - A session runs one prompt at a time. OpenCode returns an error for a second `session/prompt` while the first is active.