From ed0e7acecdb770a13edd89da65fea1e941896e35 Mon Sep 17 00:00:00 2001 From: Shoubhit Dash Date: Tue, 6 Oct 2026 22:49:59 +0530 Subject: [PATCH] fix(acp): reject a mismatched fork cwd and resolve overlapping provider IDs (#53534) --- packages/cli/src/acp/config-option.ts | 38 +++++++++++------------ packages/cli/src/acp/service.ts | 4 +-- packages/cli/test/acp/catalog.test.ts | 11 +++++-- packages/cli/test/acp/session.test.ts | 5 +++ services/www/src/docs/content/cli/acp.mdx | 5 ++- 5 files changed, 35 insertions(+), 28 deletions(-) diff --git a/packages/cli/src/acp/config-option.ts b/packages/cli/src/acp/config-option.ts index b930a3d5691..472c0b5ff8c 100644 --- a/packages/cli/src/acp/config-option.ts +++ b/packages/cli/src/acp/config-option.ts @@ -28,10 +28,10 @@ export function configOptions(catalog: Catalog, selection: Selection): SessionCo name: "Model", category: "model", type: "select", - currentValue: `${model.providerID}/${model.id}`, + currentValue: advertisedModel(model), options: catalog.models .toSorted((a, b) => Order.String(a.providerID, b.providerID) || a.name.localeCompare(b.name)) - .map((item) => ({ value: `${item.providerID}/${item.id}`, name: `${item.providerID}/${item.name}` })), + .map((item) => ({ value: advertisedModel(item), name: `${item.providerID}/${item.name}` })), }, ...(variants.length > 0 ? [ @@ -101,24 +101,18 @@ export const resolveChange = Effect.fnUntraced(function* ( }) export function parseModelSelection(value: string, models: ReadonlyArray): Model.Ref { - const providerID = models - .map((model) => model.providerID) - .toSorted() - .find((id) => value.startsWith(`${id}/`)) - if (!providerID) { - const separator = value.indexOf("/") - if (separator === -1) return { providerID: Provider.ID.make(value), id: Model.ID.make("") } - return { providerID: Provider.ID.make(value.slice(0, separator)), id: Model.ID.make(value.slice(separator + 1)) } - } - const id = Model.ID.make(value.slice(providerID.length + 1)) - if (findModel(models, { providerID, id })) return { providerID, id } - const separator = id.lastIndexOf("/") - const baseID = Model.ID.make(separator === -1 ? id : id.slice(0, separator)) - const variant = separator === -1 ? undefined : id.slice(separator + 1) - const model = findModel(models, { providerID, id: baseID }) - if (model && variant && model.variants.some((item) => item.id === variant)) - return { providerID, id: baseID, variant: Model.VariantID.make(variant) } - return { providerID, id } + const exact = models.find((model) => advertisedModel(model) === value) + if (exact) return { providerID: exact.providerID, id: exact.id } + const separator = value.lastIndexOf("/") + const variant = Model.VariantID.make(value.slice(separator + 1)) + const base = models.find( + (model) => + advertisedModel(model) === value.slice(0, separator) && model.variants.some((item) => item.id === variant), + ) + if (base) return { providerID: base.providerID, id: base.id, variant } + const providerEnd = value.indexOf("/") + if (providerEnd === -1) return { providerID: Provider.ID.make(value), id: Model.ID.make("") } + return { providerID: Provider.ID.make(value.slice(0, providerEnd)), id: Model.ID.make(value.slice(providerEnd + 1)) } } const requireModel = Effect.fnUntraced(function* (catalog: Catalog, value: string, current: Model.Ref) { @@ -137,6 +131,10 @@ const requireModel = Effect.fnUntraced(function* (catalog: Catalog, value: strin return { providerID: model.providerID, id: model.id, variant } satisfies Model.Ref }) +function advertisedModel(model: { readonly providerID: string; readonly id: string }) { + return `${model.providerID}/${model.id}` +} + function selectVariant(variant: string | undefined, variants: readonly string[]) { if (!variant || variant === DEFAULT_VARIANT_VALUE) return DEFAULT_VARIANT_VALUE if (variants.includes(variant)) return variant diff --git a/packages/cli/src/acp/service.ts b/packages/cli/src/acp/service.ts index 55c97f1aff3..4e10bea164c 100644 --- a/packages/cli/src/acp/service.ts +++ b/packages/cli/src/acp/service.ts @@ -216,8 +216,8 @@ export function make(input: { forkSession: Effect.fnUntraced(function* (params) { const directories = yield* ACPDirectories.parse(params.cwd, params.additionalDirectories) const mcpServers = yield* supportedMcpServers(params.mcpServers) - const sessionID = yield* ACPClient.decodeSessionID(params.sessionId) - const forked = yield* input.client.session.fork({ sessionID }).pipe(Effect.catch(ACPClient.classify)) + const session = yield* getSession(params.sessionId, params.cwd) + const forked = yield* input.client.session.fork({ sessionID: session.id }).pipe(Effect.catch(ACPClient.classify)) // Forks inherit the source's grants; replace them with this request's. yield* ACPDirectories.activate(input.client, forked, directories) const attachment = yield* input.sessions.attach(forked, forked.location.directory, mcpServers) diff --git a/packages/cli/test/acp/catalog.test.ts b/packages/cli/test/acp/catalog.test.ts index 8d1a8c2f602..f74c0c5925c 100644 --- a/packages/cli/test/acp/catalog.test.ts +++ b/packages/cli/test/acp/catalog.test.ts @@ -1,11 +1,13 @@ import { describe, expect, test } from "bun:test" import { currentValue } from "./select-options" -import { rpcError, secondModel, startSession, startWire } from "./wire-fixture" +import { rpcError, secondModel, startWire } from "./wire-fixture" describe("acp catalog and config options over the wire", () => { test("switches model, effort, and mode against the warm catalog", async () => { - await using acp = await startSession() - const sessionId = acp.sessionId + await using acp = await startWire() + acp.server.catalog.models.push({ ...secondModel, providerID: "test/sub" }) + await acp.initialize() + const sessionId = (await acp.newSession()).sessionId const set = (configId: string, value: string) => acp.request("session/set_config_option", { sessionId, configId, value }) @@ -32,6 +34,9 @@ describe("acp catalog and config options over the wire", () => { code: -32602, data: { modelId: "test/missing-model" }, }) + const overlap = "test/sub/second-model" + expect(currentValue(await set("model", overlap), "model")).toBe(overlap) + expect(currentValue(await set("model", `${overlap}/low`), "effort")).toBe("low") }) test("answers the first session after a cold location activates its plugins (#52729, #52472)", async () => { diff --git a/packages/cli/test/acp/session.test.ts b/packages/cli/test/acp/session.test.ts index ea0586413fc..38f7802d992 100644 --- a/packages/cli/test/acp/session.test.ts +++ b/packages/cli/test/acp/session.test.ts @@ -79,6 +79,11 @@ describe("acp session lifecycle over the wire", () => { expect( await rpcError(acp.request("session/load", { cwd: "/elsewhere", sessionId: "ses_loaded", mcpServers: [] })), ).toMatchObject({ code: -32602, data: { sessionId: "ses_loaded", cwd: "/elsewhere" } }) + const sessions = new Set(acp.server.sessions.keys()) + expect( + await rpcError(acp.request("session/fork", { cwd: "/elsewhere", sessionId: "ses_loaded", mcpServers: [] })), + ).toMatchObject({ code: -32602, data: { sessionId: "ses_loaded", cwd: "/elsewhere" } }) + expect(new Set(acp.server.sessions.keys())).toEqual(sessions) expect( await rpcError(acp.request("session/load", { cwd: "/workspace", sessionId: "ses_missing", mcpServers: [] })), ).toMatchObject({ code: -32602, data: { sessionId: "ses_missing" } }) diff --git a/services/www/src/docs/content/cli/acp.mdx b/services/www/src/docs/content/cli/acp.mdx index 6a444a234aa..d43cee4d1b9 100644 --- a/services/www/src/docs/content/cli/acp.mdx +++ b/services/www/src/docs/content/cli/acp.mdx @@ -122,9 +122,8 @@ including its models, agents, commands, skills, instructions, plugins, and MCP s - `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. -- A fork keeps the source session's directory. +- To load, resume, or fork a session, the client must pass that session's directory as `cwd`. OpenCode rejects a + different `cwd` with an invalid params error. - `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.