Skip to content

Commit b7ac0bf

Browse files
benbrandtbenbrandt
authored
fix: publish complete configuration updates for session changes (#715)
## Summary - Extend the existing recovery path's before/after configuration comparison into one shared configuration-change helper. Publish a complete `config_option_update` when the advertised list changes, rather than duplicating publication in individual callers. - Use it for `session/set_config_option`, the legacy mode/model setters, command-driven changes, approved-plan implementation, and provider/recovery model refreshes. The requesting client's duplicate notification is written before the setter response. - Do not publish for unchanged lists, rejected changes, the existing JetBrains 2026.1 disabled-config workaround, or a session state superseded by replay. - Build setter responses from the current session state after publication. If replay replaces the session while a native settings request is pending, the old request does not advertise stale options in its notification or response. - Keep v2 field conversion in the existing conversion layer. Both ACP versions receive full lists, including dependent changes to reasoning choices and removal of fast mode after a model switch. Checked against ACP commit `854e6003`. `docs/protocol/v2/session-config-options.mdx:375-379` recommends configuration notifications whenever configuration changes, including changes requested through `session/set_config_option`, with the requesting client's duplicate before its response. Responses and notifications replace the complete list. ## Scope Based on the merged #714, not stacked on its feature branch. No review/checklist Markdown file is added. This covers configuration changes owned by the adapter; it does not add new native settings-event synchronization or change active-resume environment replacement. Remaining session/entity metadata reconstruction is separate. ## Validation - `npm run typecheck` - `npm run build` - `npm test`: 2,332 passed, 39 skipped - `npm run test:integration`: all 19 passed - `npm run codex-test -- -p "Reply with exactly OK. Do not use tools or modify any files." -o summary`: completed with `end_turn` - `git diff --check` - Opt-in E2E tests were not run. New wire tests cover notification-before-response ordering for every root config option on both protocol versions, complete dependent model changes, no-op and rejected/native-failure cases, one notification for `/plan`, legacy setters, and replay replacing the session during a pending native settings request. Existing command, plan, provider/recovery, and child configuration tests remain passing without snapshot updates.
1 parent ad993bb commit b7ac0bf

4 files changed

Lines changed: 232 additions & 47 deletions

File tree

‎src/CodexAcpServer.ts‎

Lines changed: 56 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -2735,7 +2735,11 @@ export class CodexAcpServer {
27352735
});
27362736
const sessionState = this.getSessionState(_params.sessionId);
27372737

2738-
this.applyModeChange(sessionState, _params.modeId);
2738+
await this.applySessionConfigOption(sessionState, {
2739+
sessionId: _params.sessionId,
2740+
configId: MODE_CONFIG_ID,
2741+
value: _params.modeId,
2742+
});
27392743
return {};
27402744
}
27412745

@@ -2750,7 +2754,7 @@ export class CodexAcpServer {
27502754
await this.applySessionConfigOption(sessionState, params);
27512755

27522756
return {
2753-
configOptions: this.createSessionConfigOptions(sessionState),
2757+
configOptions: this.createSessionConfigOptions(this.getSessionState(params.sessionId)),
27542758
};
27552759
}
27562760

@@ -2767,24 +2771,39 @@ export class CodexAcpServer {
27672771
}
27682772

27692773
private async applySessionConfigOption(sessionState: SessionState, params: acp.SetSessionConfigOptionRequest): Promise<void> {
2770-
switch (params.configId) {
2771-
case FAST_MODE_CONFIG_ID:
2772-
this.applyFastModeChange(sessionState, params);
2773-
break;
2774-
case MODE_CONFIG_ID:
2775-
this.applyModeChange(sessionState, this.stringConfigValue(params));
2776-
break;
2777-
case COLLABORATION_MODE_CONFIG_ID:
2778-
await this.applyCollaborationModeChange(sessionState, this.stringConfigValue(params));
2779-
break;
2780-
case MODEL_CONFIG_ID:
2781-
this.applyModelChange(sessionState, this.stringConfigValue(params));
2782-
break;
2783-
case REASONING_EFFORT_CONFIG_ID:
2784-
this.applyReasoningEffortChange(sessionState, this.stringConfigValue(params));
2785-
break;
2786-
default:
2787-
throw RequestError.invalidParams();
2774+
await this.updateSessionConfig(sessionState, async () => {
2775+
switch (params.configId) {
2776+
case FAST_MODE_CONFIG_ID:
2777+
this.applyFastModeChange(sessionState, params);
2778+
break;
2779+
case MODE_CONFIG_ID:
2780+
this.applyModeChange(sessionState, this.stringConfigValue(params));
2781+
break;
2782+
case COLLABORATION_MODE_CONFIG_ID:
2783+
await this.applyCollaborationModeChange(sessionState, this.stringConfigValue(params));
2784+
break;
2785+
case MODEL_CONFIG_ID:
2786+
this.applyModelChange(sessionState, this.stringConfigValue(params));
2787+
break;
2788+
case REASONING_EFFORT_CONFIG_ID:
2789+
this.applyReasoningEffortChange(sessionState, this.stringConfigValue(params));
2790+
break;
2791+
default:
2792+
throw RequestError.invalidParams();
2793+
}
2794+
});
2795+
}
2796+
2797+
private async updateSessionConfig(sessionState: SessionState, change: () => void | Promise<void>): Promise<void> {
2798+
const previous = JSON.stringify(this.createSessionConfigOptions(sessionState));
2799+
await change();
2800+
const configOptions = this.createSessionConfigOptions(sessionState);
2801+
if (this.isSessionConfigEnabled() && this.sessions.get(sessionState.sessionId) === sessionState
2802+
&& JSON.stringify(configOptions) !== previous) {
2803+
await new ACPSessionConnection(this.connection, sessionState.sessionId).update({
2804+
sessionUpdate: "config_option_update",
2805+
configOptions,
2806+
});
27882807
}
27892808
}
27902809

@@ -2858,20 +2877,15 @@ export class CodexAcpServer {
28582877
* The session keeps its model when it can, the way a resumed thread keeps its own; see `createModelId`.
28592878
*/
28602879
private async applyResumedModels(sessionState: SessionState, client: CodexAcpClient, resumed: SessionMetadata): Promise<void> {
2861-
const configOptions = JSON.stringify(this.createSessionConfigOptions(sessionState));
2862-
const {model, effort} = ModelId.fromString(sessionState.currentModelId);
2863-
sessionState.availableModels = resumed.models;
2864-
sessionState.currentModelId = client.createModelId(resumed.models, model, effort as ReasoningEffort).toString();
2865-
const currentModel = this.findCurrentModel(resumed.models, sessionState.currentModelId);
2866-
sessionState.supportedReasoningEfforts = currentModel?.supportedReasoningEfforts ?? [];
2867-
sessionState.supportedInputModalities = currentModel?.inputModalities ?? ["text", "image"];
2868-
sessionState.currentModelSupportsFast = modelSupportsFast(currentModel);
2869-
if (JSON.stringify(this.createSessionConfigOptions(sessionState)) !== configOptions) {
2870-
await new ACPSessionConnection(this.connection, sessionState.sessionId).update({
2871-
sessionUpdate: "config_option_update",
2872-
configOptions: this.createSessionConfigOptions(sessionState),
2873-
});
2874-
}
2880+
await this.updateSessionConfig(sessionState, () => {
2881+
const {model, effort} = ModelId.fromString(sessionState.currentModelId);
2882+
sessionState.availableModels = resumed.models;
2883+
sessionState.currentModelId = client.createModelId(resumed.models, model, effort as ReasoningEffort).toString();
2884+
const currentModel = this.findCurrentModel(resumed.models, sessionState.currentModelId);
2885+
sessionState.supportedReasoningEfforts = currentModel?.supportedReasoningEfforts ?? [];
2886+
sessionState.supportedInputModalities = currentModel?.inputModalities ?? ["text", "image"];
2887+
sessionState.currentModelSupportsFast = modelSupportsFast(currentModel);
2888+
});
28752889
}
28762890

28772891
private applyModelAndEffort(sessionState: SessionState, model: Model, effort: ReasoningEffort): void {
@@ -2908,8 +2922,10 @@ export class CodexAcpServer {
29082922
reasoningEffort = model.defaultReasoningEffort;
29092923
}
29102924

2911-
sessionState.availableModels = models;
2912-
this.applyModelAndEffort(sessionState, model, reasoningEffort);
2925+
await this.updateSessionConfig(sessionState, () => {
2926+
sessionState.availableModels = models;
2927+
this.applyModelAndEffort(sessionState, model, reasoningEffort);
2928+
});
29132929

29142930
return {};
29152931
}
@@ -5028,11 +5044,6 @@ export class CodexAcpServer {
50285044
configId,
50295045
value,
50305046
});
5031-
const session = new ACPSessionConnection(this.connection, sessionState.sessionId);
5032-
await session.update({
5033-
sessionUpdate: "config_option_update",
5034-
configOptions: this.createSessionConfigOptions(sessionState),
5035-
});
50365047
},
50375048
});
50385049
void commandPromise.catch((err) => {
@@ -5221,11 +5232,10 @@ export class CodexAcpServer {
52215232
return cancelledPromptResponse();
52225233
}
52235234
if (approved && !this.promptShouldStop(params.sessionId, activePrompt)) {
5224-
await this.applyCollaborationModeChange(sessionState, DEFAULT_COLLABORATION_MODE);
5225-
const session = new ACPSessionConnection(this.connection, sessionState.sessionId);
5226-
await session.update({
5227-
sessionUpdate: "config_option_update",
5228-
configOptions: this.createSessionConfigOptions(sessionState),
5235+
await this.applySessionConfigOption(sessionState, {
5236+
sessionId: sessionState.sessionId,
5237+
configId: COLLABORATION_MODE_CONFIG_ID,
5238+
value: DEFAULT_COLLABORATION_MODE,
52295239
});
52305240

52315241
const implementationRequest: acp.PromptRequest = {

‎src/__tests__/CodexACPAgent/config-notifications.test.ts‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
import {describe, expect, it} from "vitest";
2+
import type {AnyMessage} from "@agentclientprotocol/sdk/experimental/v2/unstable";
3+
import {LEGACY_SET_SESSION_MODEL_METHOD} from "../../AcpExtensions";
4+
import {createTestModel} from "../acp-test-utils";
5+
import {codexResponse, connectSession, sessionId, type PromptSession} from "./v2-prompt-harness";
6+
7+
async function connect(version: 1 | 2) {
8+
const fast = createTestModel({id: "fast-model", additionalSpeedTiers: ["fast"],
9+
supportedReasoningEfforts: [
10+
{reasoningEffort: "medium", description: "Balanced"},
11+
{reasoningEffort: "high", description: "Thorough"},
12+
]});
13+
const slow = createTestModel({id: "slow-model", isDefault: false, defaultReasoningEffort: "low",
14+
supportedReasoningEfforts: [{reasoningEffort: "low", description: "Fast"}]});
15+
return connectSession(version, {
16+
codexResponses: {
17+
"thread/start": async () => ({...codexResponse("thread/start") as object, model: fast.id}),
18+
"thread/resume": async () => ({...codexResponse("thread/resume") as object, model: fast.id}),
19+
"model/list": async () => ({data: [fast, slow], nextCursor: null}),
20+
},
21+
});
22+
}
23+
24+
function configUpdates(frames: AnyMessage[]) {
25+
return frames.flatMap((frame, index) => {
26+
if (!("method" in frame) || frame.method !== "session/update") return [];
27+
const params = frame.params as {update: {sessionUpdate: string; configOptions: unknown[]}};
28+
return params.update.sessionUpdate === "config_option_update"
29+
? [{index, configOptions: params.update.configOptions}] : [];
30+
});
31+
}
32+
33+
function assertBeforeResponse(h: PromptSession, start: number, expected?: unknown[]) {
34+
const frames = h.wireMessages.slice(start);
35+
const updates = configUpdates(frames);
36+
expect(updates).toHaveLength(1);
37+
const response = frames.findIndex(frame => "result" in frame);
38+
expect(updates[0]!.index).toBeLessThan(response);
39+
if (expected) expect(updates[0]!.configOptions).toEqual(expected);
40+
}
41+
42+
describe.each([1, 2] as const)("ACP v%s config notifications", version => {
43+
it.each([
44+
{configId: "mode", value: "read-only"},
45+
{configId: "collaboration_mode", value: "plan"},
46+
{configId: "model", value: "slow-model"},
47+
{configId: "reasoning_effort", value: "high"},
48+
{configId: "fast-mode", value: "on"},
49+
])("publishes complete $configId state before its response, but not for a no-op", async ({configId, value}) => {
50+
const h = await connect(version);
51+
try {
52+
const params = {sessionId, configId,
53+
...(version === 2 ? {type: configId === "fast-mode" ? "boolean" : "id"} : {}),
54+
value: version === 2 && configId === "fast-mode" ? true : value};
55+
const start = h.wireMessages.length;
56+
const response = await h.request("session/set_config_option", params);
57+
assertBeforeResponse(h, start, response.configOptions);
58+
if (configId === "model") {
59+
const options = response.configOptions.map((option: {
60+
id?: string; configId?: string; type: string; currentValue: unknown; options: Array<{value: string}>;
61+
}) => ({id: option.id ?? option.configId, currentValue: option.currentValue,
62+
values: option.options.map(value => value.value)}));
63+
await expect(JSON.stringify(options, null, 2) + "\n")
64+
.toMatchFileSnapshot("data/config-notification-dependent-model.json");
65+
}
66+
const repeated = h.wireMessages.length;
67+
expect((await h.request("session/set_config_option", params)).configOptions).toEqual(response.configOptions);
68+
expect(configUpdates(h.wireMessages.slice(repeated))).toEqual([]);
69+
} finally { h.connection.close(); }
70+
});
71+
72+
it.each([
73+
{configId: "missing", value: "anything", nativeFailure: false, code: -32602},
74+
{configId: "mode", value: "missing", nativeFailure: false, code: -32602},
75+
{configId: "collaboration_mode", value: "plan", nativeFailure: true, code: -32603},
76+
])("does not publish a rejected $configId change (native failure: $nativeFailure)", async ({configId, value, nativeFailure, code}) => {
77+
const h = await connect(version);
78+
try {
79+
if (nativeFailure) h.setCodexResponse("thread/settings/update", async () => { throw new Error("Settings failed"); });
80+
const start = h.wireMessages.length;
81+
await expect(h.request("session/set_config_option", {sessionId, configId, value,
82+
...(version === 2 ? {type: "id"} : {}),
83+
})).rejects.toMatchObject({code});
84+
expect(configUpdates(h.wireMessages.slice(start))).toEqual([]);
85+
expect(h.agent.getSessionState(sessionId).collaborationMode).toBe("default");
86+
} finally { h.connection.close(); }
87+
});
88+
89+
it("publishes one complete update for /plan", async () => {
90+
const h = await connect(version);
91+
try {
92+
const start = h.wireMessages.length;
93+
await h.sendPrompt([{type: "text", text: "/plan"}]);
94+
await h.promptRunFinished();
95+
const updates = configUpdates(h.wireMessages.slice(start));
96+
expect(updates).toHaveLength(1);
97+
expect(updates[0]!.configOptions).toHaveLength(5);
98+
} finally { h.connection.close(); }
99+
});
100+
101+
it("does not publish stale options when replay replaces the session during a native settings request", async () => {
102+
const h = await connect(version);
103+
let entered!: () => void;
104+
let release!: () => void;
105+
const writing = new Promise<void>(resolve => { entered = resolve; });
106+
const held = new Promise<void>(resolve => { release = resolve; });
107+
try {
108+
h.setCodexResponse("thread/settings/update", async () => { entered(); await held; return {}; });
109+
const setting = h.request("session/set_config_option", {
110+
sessionId, configId: "collaboration_mode", value: "plan", ...(version === 2 ? {type: "id"} : {}),
111+
});
112+
await writing;
113+
const resumed = await h.request(version === 1 ? "session/load" : "session/resume", {
114+
sessionId, cwd: "/workspace", mcpServers: [],
115+
...(version === 2 ? {replayFrom: {type: "start"}} : {}),
116+
});
117+
const start = h.wireMessages.length;
118+
release();
119+
expect((await setting).configOptions).toEqual(resumed.configOptions);
120+
expect(configUpdates(h.wireMessages.slice(start))).toEqual([]);
121+
} finally { release(); h.connection.close(); }
122+
});
123+
});
124+
125+
describe("Legacy configuration entry points", () => {
126+
it.each([
127+
{method: "session/set_mode", params: {modeId: "read-only"}},
128+
{method: LEGACY_SET_SESSION_MODEL_METHOD, params: {modelId: "slow-model[low]"}},
129+
])("publishes current config before the $method response", async ({method, params}) => {
130+
const h = await connect(1);
131+
try {
132+
const start = h.wireMessages.length;
133+
await h.request(method, {sessionId, ...params});
134+
assertBeforeResponse(h, start);
135+
} finally { h.connection.close(); }
136+
});
137+
});

‎src/__tests__/CodexACPAgent/data/config-notification-dependent-model.json‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
[
2+
{
3+
"id": "mode",
4+
"currentValue": "agent",
5+
"values": [
6+
"read-only",
7+
"workspace-write",
8+
"agent",
9+
"agent-full-access"
10+
]
11+
},
12+
{
13+
"id": "collaboration_mode",
14+
"currentValue": "default",
15+
"values": [
16+
"default",
17+
"plan"
18+
]
19+
},
20+
{
21+
"id": "model",
22+
"currentValue": "slow-model",
23+
"values": [
24+
"fast-model",
25+
"slow-model"
26+
]
27+
},
28+
{
29+
"id": "reasoning_effort",
30+
"currentValue": "low",
31+
"values": [
32+
"low"
33+
]
34+
}
35+
]

‎src/__tests__/CodexACPAgent/fast-mode-config.test.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ describe("Fast mode session config", () => {
107107
});
108108

109109
it("omits Fast mode config options for JetBrains 2026.1 IntelliJ clients", async () => {
110-
const {response} = await createSession(null, {
110+
const {response, fixture, codexAcpAgent} = await createSession(null, {
111111
name: "JetBrains.WebStorm",
112112
version: "2026.1.1",
113113
title: "WebStorm 2026.1.1",
@@ -117,6 +117,9 @@ describe("Fast mode session config", () => {
117117
});
118118

119119
expect(response.configOptions).toBeUndefined();
120+
await codexAcpAgent.setSessionConfigOption({sessionId: response.sessionId, configId: FAST_MODE_CONFIG_ID, value: FAST_MODE_ON});
121+
expect(fixture.getAcpConnectionEvents([]).some(event =>
122+
event.method === "sessionUpdate" && event.args[0]?.update?.sessionUpdate === "config_option_update")).toBe(false);
120123
});
121124

122125
it("omits Fast mode config options for JetBrains 2026.1 clients by name", async () => {

0 commit comments

Comments
 (0)