Skip to content
This repository was archived by the owner on Aug 6, 2026. It is now read-only.

Commit df5d67c

Browse files
authored
fix(models): Persist selected model into new sessions (#2690)
1 parent 2d05add commit df5d67c

12 files changed

Lines changed: 308 additions & 12 deletions

File tree

‎packages/agent/src/adapters/acp-connection.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,7 @@ function createCodexConnection(config: AcpConnectionConfig): AcpConnection {
206206
processCallbacks: config.processCallbacks,
207207
posthogApiConfig: resolveEnricherApiConfig(config),
208208
onStructuredOutput: config.onStructuredOutput,
209+
logger: config.logger?.child("CodexAcpAgent"),
209210
});
210211
return agent;
211212
}, agentStream);

‎packages/agent/src/adapters/base-acp-agent.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,17 @@ export abstract class BaseAcpAgent implements Agent {
158158

159159
if (!options.some((opt) => opt.value === currentModelId)) {
160160
if (!isAnthropicModelId(currentModelId)) {
161+
// A non-Anthropic model id reached the Claude adapter, which means the
162+
// adapter and model desynced upstream (e.g. a Codex model paired with
163+
// the Claude adapter). Log it instead of silently masquerading as a
164+
// deliberate Opus session.
165+
this.logger.warn(
166+
"Non-Anthropic model requested on Claude adapter; falling back to default model",
167+
{
168+
requestedModel: currentModelId,
169+
fallbackModel: DEFAULT_GATEWAY_MODEL,
170+
},
171+
);
161172
currentModelId = DEFAULT_GATEWAY_MODEL;
162173
}
163174
}

‎packages/agent/src/adapters/claude/claude-agent.resume-model.test.ts‎

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import * as os from "node:os";
33
import * as path from "node:path";
44
import type { AgentSideConnection } from "@agentclientprotocol/sdk";
55
import { afterAll, beforeEach, describe, expect, it, vi } from "vitest";
6+
import { DEFAULT_GATEWAY_MODEL } from "../../gateway-models";
67

78
type SdkQueryHandle = {
89
interrupt: ReturnType<typeof vi.fn>;
@@ -153,18 +154,43 @@ describe("ClaudeAcpAgent session model on resume", () => {
153154
},
154155
);
155156

156-
it("does not call setModel for a new session on the default model", async () => {
157+
// New sessions pass the model to the SDK at spawn, never via setModel. The
158+
// Codex-model row guards the desync that surfaced as "picked gpt-5.5, session
159+
// ran Opus": a non-Anthropic id on the Claude adapter must fall back to the
160+
// default AND warn rather than silently masquerade as a deliberate Opus run.
161+
it.each([
162+
{
163+
name: "uses the gateway default and never calls setModel without a requested model",
164+
model: undefined,
165+
expectsWarn: false,
166+
},
167+
{
168+
name: "warns and falls back to the default when a Codex model reaches the Claude adapter",
169+
model: "gpt-5.5",
170+
expectsWarn: true,
171+
},
172+
])("newSession $name", async ({ model, expectsWarn }) => {
157173
const agent = makeAgent();
174+
const warnSpy = vi.spyOn(agent.logger, "warn");
158175

159-
await agent.newSession({
176+
const response = await agent.newSession({
160177
cwd,
161178
mcpServers: [],
162-
_meta: { taskRunId: "run-3" },
179+
_meta: { taskRunId: "run-new", ...(model ? { model } : {}) },
163180
});
164181

165-
// New sessions already pass options.model to the SDK at spawn.
166-
expect(createdQueries).toHaveLength(1);
167182
expect(createdQueries[0].setModel).not.toHaveBeenCalled();
183+
expect(getModelConfigOption(response)?.currentValue).toBe(
184+
DEFAULT_GATEWAY_MODEL,
185+
);
186+
if (expectsWarn) {
187+
expect(warnSpy).toHaveBeenCalledWith(
188+
expect.stringContaining("Non-Anthropic model"),
189+
expect.objectContaining({ requestedModel: model }),
190+
);
191+
} else {
192+
expect(warnSpy).not.toHaveBeenCalled();
193+
}
168194
});
169195

170196
// The timeout *message* (RequestError "... timed out after ...") is covered

‎packages/agent/src/adapters/codex/codex-agent.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,13 @@ export interface CodexAcpAgentOptions {
128128
processCallbacks?: ProcessSpawnedCallback;
129129
posthogApiConfig?: PostHogAPIConfig;
130130
onStructuredOutput?: (output: Record<string, unknown>) => Promise<void>;
131+
/**
132+
* Logger wired to the host log sink. Without it the codex-acp subprocess
133+
* stderr, spawn failures and exit codes are written to a throwaway logger and
134+
* never reach the exported logs, so a crash surfaces only as a generic
135+
* "ACP connection closed" with no cause.
136+
*/
137+
logger?: Logger;
131138
}
132139

133140
type CodexSession = BaseSession & {
@@ -320,7 +327,8 @@ export class CodexAcpAgent extends BaseAcpAgent {
320327

321328
constructor(client: AgentSideConnection, options: CodexAcpAgentOptions) {
322329
super(client);
323-
this.logger = new Logger({ debug: true, prefix: "[CodexAcpAgent]" });
330+
this.logger =
331+
options.logger ?? new Logger({ debug: true, prefix: "[CodexAcpAgent]" });
324332

325333
// Load user codex settings before spawning so spawnCodexProcess can
326334
// filter out any [mcp_servers.*] entries from ~/.codex/config.toml.
Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
import { describe, expect, it, vi } from "vitest";
2+
3+
const readFileSync = vi.hoisted(() => vi.fn());
4+
vi.mock("node:fs", () => ({ readFileSync }));
5+
6+
const { CodexSettingsManager } = await import("./settings");
7+
8+
function serverNamesFor(toml: string): string[] {
9+
readFileSync.mockReturnValue(toml);
10+
return new CodexSettingsManager("/repo").getSettings().mcpServerNames.sort();
11+
}
12+
13+
describe("CodexSettingsManager MCP server names", () => {
14+
// Regression: a `[mcp_servers.<name>.env]` table was treated as its own
15+
// server, so the spawn emitted `mcp_servers.<name>.env.enabled=false`, which
16+
// sets a boolean on codex's string-typed env map. codex-acp then rejected the
17+
// whole config, crashed the session, and the host silently ran Claude/Opus.
18+
it.each([
19+
{
20+
name: "collapses a nested sub-table to its parent server and dedupes",
21+
toml: [
22+
"[mcp_servers.node_repl]",
23+
'command = "node"',
24+
"[mcp_servers.node_repl.env]",
25+
'FOO = "bar"',
26+
"[mcp_servers.other]",
27+
'command = "x"',
28+
],
29+
expected: ["node_repl", "other"],
30+
},
31+
{
32+
name: "collapses a deeply nested sub-table to its parent server",
33+
toml: ["[mcp_servers.foo.bar.baz]", 'k = "v"'],
34+
expected: ["foo"],
35+
},
36+
{
37+
name: "keeps the inner name for a double-quoted dotted server key",
38+
toml: ['[mcp_servers."my.server"]', 'command = "x"'],
39+
expected: ["my.server"],
40+
},
41+
{
42+
name: "keeps the inner name for a single-quoted dotted server key",
43+
toml: ["[mcp_servers.'my.server']", 'command = "x"'],
44+
expected: ["my.server"],
45+
},
46+
{
47+
name: "returns no servers when none are declared",
48+
toml: ['model = "gpt-5.5"'],
49+
expected: [],
50+
},
51+
])("$name", ({ toml, expected }) => {
52+
expect(serverNamesFor(toml.join("\n"))).toEqual(expected);
53+
});
54+
});

‎packages/agent/src/adapters/codex/settings.ts‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,24 @@ export class CodexSettingsManager {
7171
}
7272
}
7373

74+
/**
75+
* Extracts the server name from a `mcp_servers.<name>...` section path, taking
76+
* only the first key segment. A nested table like `mcp_servers.foo.env`
77+
* describes the `env` field of server `foo`, not a separate server, so it must
78+
* collapse to `foo`. Treating it as its own server emits
79+
* `mcp_servers.foo.env.enabled=false`, which sets a boolean on the string-typed
80+
* env map and makes codex-acp reject the whole config (it then crashes and the
81+
* host silently falls back to Claude). Quoted segments (`"a.b"`) keep their dots.
82+
*/
83+
function firstMcpServerName(sectionPath: string): string | null {
84+
const trimmed = sectionPath.trim();
85+
if (!trimmed) return null;
86+
const quoted = trimmed.match(/^(["'])(.*?)\1/);
87+
if (quoted) return quoted[2] ?? null;
88+
const dotIndex = trimmed.indexOf(".");
89+
return dotIndex === -1 ? trimmed : trimmed.slice(0, dotIndex);
90+
}
91+
7492
/**
7593
* Minimal TOML parser for codex config.toml.
7694
* Handles flat key=value pairs and [projects."path"] sections.
@@ -90,7 +108,10 @@ function parseCodexToml(content: string, cwd: string): CodexSettings {
90108
if (sectionMatch) {
91109
currentSection = sectionMatch[1] ?? "";
92110
if (currentSection.startsWith("mcp_servers.")) {
93-
mcpServerNames.add(currentSection.slice("mcp_servers.".length));
111+
const serverName = firstMcpServerName(
112+
currentSection.slice("mcp_servers.".length),
113+
);
114+
if (serverName) mcpServerNames.add(serverName);
94115
}
95116
continue;
96117
}
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
import { describe, expect, it, vi } from "vitest";
2+
import { Logger } from "../../utils/logger";
3+
4+
const spawnMock = vi.hoisted(() => vi.fn());
5+
vi.mock("node:child_process", () => ({ spawn: spawnMock }));
6+
7+
const { spawnCodexProcess } = await import("./spawn");
8+
9+
function makeFakeChild() {
10+
return {
11+
stdin: { destroy: vi.fn() },
12+
stdout: { on: vi.fn(), destroy: vi.fn() },
13+
stderr: { on: vi.fn(), destroy: vi.fn() },
14+
on: vi.fn(),
15+
kill: vi.fn(),
16+
pid: 1234,
17+
};
18+
}
19+
20+
describe("spawnCodexProcess MCP disable args", () => {
21+
it("disables bare-key servers but skips names codex's -c parser cannot express", () => {
22+
spawnMock.mockReturnValue(makeFakeChild());
23+
24+
spawnCodexProcess({
25+
logger: new Logger({ debug: false }),
26+
settings: {
27+
mcpServerNames: ["simple", "with-dash", "my.server", "weird name"],
28+
},
29+
});
30+
31+
const args: string[] = spawnMock.mock.calls[0][1];
32+
expect(args).toContain("mcp_servers.simple.enabled=false");
33+
expect(args).toContain("mcp_servers.with-dash.enabled=false");
34+
// A dotted or otherwise non-bare name would emit an override codex rejects,
35+
// which crashes the whole session, so it is skipped (the server stays
36+
// enabled, which is harmless).
37+
expect(args.some((arg) => arg.includes("my.server"))).toBe(false);
38+
expect(args.some((arg) => arg.includes("weird name"))).toBe(false);
39+
});
40+
});

‎packages/agent/src/adapters/codex/spawn.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,14 @@ function buildConfigArgs(options: CodexProcessOptions): string[] {
3636
// Disable the user's local MCPs one-by-one so Codex only uses the MCPs we
3737
// provide via ACP. We can't use `-c mcp_servers={}` because that makes Codex
3838
// ignore MCPs entirely, including the ones we inject later.
39+
//
40+
// Only bare-key names are emitted: codex's `-c` parser rejects quoted key
41+
// segments, so a name with a dot or other special character cannot be
42+
// expressed as `mcp_servers.<name>.enabled=false` without producing an
43+
// override that fails to load and crashes the whole codex session. Skipping
44+
// such a name leaves that server enabled (harmless) instead of killing codex.
3945
for (const name of options.settings?.mcpServerNames ?? []) {
46+
if (!/^[A-Za-z0-9_-]+$/.test(name)) continue;
4047
args.push("-c", `mcp_servers.${name}.enabled=false`);
4148
}
4249

‎packages/ui/src/features/task-detail/components/TaskInput.tsx‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -808,7 +808,9 @@ export function TaskInput({
808808
isLoading={isCreatingTask}
809809
autoFocus
810810
clearOnSubmit={false}
811-
submitDisabledExternal={!canSubmit || isCreatingTask || !isOnline}
811+
submitDisabledExternal={
812+
!canSubmit || isCreatingTask || !isOnline || isPreviewLoading
813+
}
812814
tourTarget="task-input"
813815
repoPath={selectedDirectory}
814816
modeOption={modeOption}

‎packages/ui/src/features/task-detail/hooks/usePreviewConfig.ts‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,15 +50,27 @@ export function usePreviewConfig(
5050
const [configOptions, setConfigOptions] = useState<SessionConfigOption[]>([]);
5151
const [isLoading, setIsLoading] = useState(true);
5252
const abortRef = useRef<AbortController | null>(null);
53+
const hasHydrated = useSettingsStore((state) => state._hasHydrated);
5354

5455
useEffect(() => {
5556
if (!apiHost) return;
5657

58+
// Wait for the settings store to finish its async hydration before
59+
// resolving the model. Otherwise lastUsedModel and lastUsedAdapter read as
60+
// their pre-hydration defaults, the restore below is skipped, and the
61+
// selector silently falls back to the server default (Opus for Claude).
62+
// isLoading initializes to true, so the picker stays loading until hydration
63+
// lands and the fetch below resolves.
64+
if (!hasHydrated) return;
65+
5766
abortRef.current?.abort();
5867
const abort = new AbortController();
5968
abortRef.current = abort;
6069

6170
setIsLoading(true);
71+
// Drop the previous adapter's options so a stale model id can never be sent
72+
// as the current selection while the new adapter's config is loading.
73+
setConfigOptions([]);
6274

6375
hostClient.agent.getPreviewConfigOptions
6476
.query({ apiHost, adapter }, { signal: abort.signal })
@@ -122,7 +134,7 @@ export function usePreviewConfig(
122134
return () => {
123135
abort.abort();
124136
};
125-
}, [adapter, apiHost, hostClient]);
137+
}, [adapter, apiHost, hostClient, hasHydrated]);
126138

127139
const setConfigOption = useCallback(
128140
(configId: string, value: string) => {

0 commit comments

Comments
 (0)