Skip to content

Commit ebc65b5

Browse files
scottzxbenbrandt
andauthored
Keep configured model id for custom providers instead of forcing the default (#199)
* fix: keep configured model id for custom providers instead of forcing the default createModelId() looked up the configured/returned model id in Codex's listModels() catalog and, when it was absent, substituted availableModels.find(m => m.isDefault). Custom providers (self-hosted or third-party endpoints) expose model ids that the catalog does not enumerate, so this silently replaced the user's configured model with the built-in default. That default id was then pinned onto every runTurn, making requests to the custom endpoint fail with "unknown model '<default>'". The Codex CLI handles the same case by keeping the configured model id and warning "Model metadata not found. Defaulting to fallback metadata." This change mirrors that behavior: keep the requested model id when it is not in the catalog, and only fall back to isDefault when no model id was provided at all. Downstream session state already degrades gracefully for unknown models (supportedReasoningEfforts ?? [], inputModalities ?? ["text","image"]). Adds unit tests covering an uncatalogued model id with and without an explicit reasoning effort. * Handle uncataloged current models in config --------- Co-authored-by: Ben Brandt <benjamin.j.brandt@gmail.com>
1 parent 373c42d commit ebc65b5

5 files changed

Lines changed: 86 additions & 14 deletions

File tree

src/CodexAcpClient.ts

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -446,15 +446,30 @@ export class CodexAcpClient {
446446
* Falls back to model defaults if parameters are missing or unsupported.
447447
*/
448448
createModelId(availableModels: Model[], modelId: string | null, reasoningEffort: ReasoningEffort | null): ModelId {
449-
const selectedModel =
450-
availableModels.find(m => m.id === modelId) ??
451-
availableModels.find(m => m.isDefault);
452-
453-
if (!selectedModel) {
449+
const selectedModel = availableModels.find(m => m.id === modelId);
450+
if (selectedModel) {
451+
return ModelId.create(selectedModel.id, reasoningEffort ?? selectedModel.defaultReasoningEffort);
452+
}
453+
454+
// The configured model is not in Codex's advertised catalog. This is
455+
// expected for custom providers (e.g. a self-hosted or third-party
456+
// model), whose model ids the catalog does not enumerate. Keep the
457+
// requested model id instead of silently substituting the built-in
458+
// default. This mirrors the Codex CLI, which keeps the configured model
459+
// and merely warns "Model metadata not found. Defaulting to fallback
460+
// metadata." Substituting the default here pins a wrong model id onto
461+
// every turn and makes requests to custom-provider endpoints fail with
462+
// "unknown model".
463+
if (modelId) {
464+
return ModelId.create(modelId, reasoningEffort ?? "medium");
465+
}
466+
467+
const defaultModel = availableModels.find(m => m.isDefault);
468+
if (!defaultModel) {
454469
throw new Error(`Model selection failed: No model found for ID "${modelId}" and no default model is defined.`);
455470
}
456471

457-
return ModelId.create(selectedModel.id, reasoningEffort ?? selectedModel.defaultReasoningEffort);
472+
return ModelId.create(defaultModel.id, reasoningEffort ?? defaultModel.defaultReasoningEffort);
458473
}
459474

460475
async subscribeToSessionEvents(

src/CodexAcpServer.ts

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -694,6 +694,10 @@ export class CodexAcpServer {
694694
private applyModelChange(sessionState: SessionState, value: string): void {
695695
const model = sessionState.availableModels.find(m => m.id === value);
696696
if (!model) {
697+
const currentModel = ModelId.fromString(sessionState.currentModelId).model;
698+
if (value === currentModel) {
699+
return;
700+
}
697701
throw RequestError.invalidParams();
698702
}
699703
const currentEffort = ModelId.fromString(sessionState.currentModelId).effort;
@@ -763,12 +767,17 @@ export class CodexAcpServer {
763767

764768
private createSessionConfigOptions(sessionState: SessionState): Array<acp.SessionConfigOption> {
765769
const currentModelId = ModelId.fromString(sessionState.currentModelId);
766-
return [
770+
const configOptions = [
767771
sessionState.agentMode.toConfigOption(),
768772
createModelConfigOption(sessionState.availableModels, currentModelId.model),
769-
createReasoningEffortConfigOption(sessionState.supportedReasoningEfforts, currentModelId.effort),
770-
createFastModeConfigOption(sessionState.fastModeEnabled),
771773
];
774+
if (sessionState.supportedReasoningEfforts.length > 0) {
775+
configOptions.push(
776+
createReasoningEffortConfigOption(sessionState.supportedReasoningEfforts, currentModelId.effort),
777+
);
778+
}
779+
configOptions.push(createFastModeConfigOption(sessionState.fastModeEnabled));
780+
return configOptions;
772781
}
773782

774783
private createSessionConfigOptionsResponse(sessionState: SessionState): {

src/ModelConfigOption.ts

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,18 +14,27 @@ export function findSupportedEffort(
1414
}
1515

1616
export function createModelConfigOption(availableModels: Array<Model>, currentBaseModelId: string): SessionConfigOption {
17+
const options: Array<{ value: string; name: string; description: string | null }> = availableModels.map(model => ({
18+
value: model.id,
19+
name: model.displayName,
20+
description: model.description,
21+
}));
22+
if (!availableModels.some(model => model.id === currentBaseModelId)) {
23+
options.unshift({
24+
value: currentBaseModelId,
25+
name: currentBaseModelId,
26+
description: null,
27+
});
28+
}
29+
1730
return {
1831
id: MODEL_CONFIG_ID,
1932
name: "Model",
2033
description: "Model Codex uses for the session",
2134
category: "model",
2235
type: "select",
2336
currentValue: currentBaseModelId,
24-
options: availableModels.map(model => ({
25-
value: model.id,
26-
name: model.displayName,
27-
description: model.description,
28-
})),
37+
options,
2938
};
3039
}
3140

src/__tests__/CodexACPAgent/CodexAcpClient.test.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1840,6 +1840,16 @@ describe('ACP server test', { timeout: 40_000 }, () => {
18401840
expect(result).toEqual(ModelId.create('5.2-codex', 'medium'));
18411841
});
18421842

1843+
it('should keep a model id that is not in the advertised catalog (custom provider)', () => {
1844+
const result = fixture.getCodexAcpClient().createModelId(mockModels, 'MiniMax-M3', 'high');
1845+
expect(result).toEqual(ModelId.create('MiniMax-M3', 'high'));
1846+
});
1847+
1848+
it('should default the effort for an uncatalogued model when reasoningEffort is null', () => {
1849+
const result = fixture.getCodexAcpClient().createModelId(mockModels, 'MiniMax-M3', null);
1850+
expect(result).toEqual(ModelId.create('MiniMax-M3', 'medium'));
1851+
});
1852+
18431853
/**
18441854
* Sets up a mock fixture with turnStart/awaitTurnCompleted spied on,
18451855
* and a given session state. Returns the fixture and turnStart spy.

src/__tests__/CodexACPAgent/session-config-options.test.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,35 @@ describe("Session config options", () => {
9090
);
9191
});
9292

93+
it("shows the current uncataloged model as its own selectable option", async () => {
94+
const {fast, slow} = buildModels();
95+
const {codexAcpAgent, response} = await createSession("custom-model[high]", [fast, slow]);
96+
97+
const ids = response.configOptions?.map(o => o.id);
98+
expect(ids).toEqual([MODE_CONFIG_ID, MODEL_CONFIG_ID, "fast-mode"]);
99+
100+
const modelOption = response.configOptions?.find(o => o.id === MODEL_CONFIG_ID);
101+
expect(modelOption).toMatchObject({
102+
category: "model",
103+
currentValue: "custom-model",
104+
type: "select",
105+
options: [
106+
{value: "custom-model", name: "custom-model", description: null},
107+
{value: "fast-model", name: "Fast model", description: "Frontier"},
108+
{value: "slow-model", name: "Slow model", description: "Strong"},
109+
],
110+
});
111+
expect(response.configOptions?.some(o => o.id === REASONING_EFFORT_CONFIG_ID)).toBe(false);
112+
113+
await codexAcpAgent.setSessionConfigOption({
114+
sessionId: "session-id",
115+
configId: MODEL_CONFIG_ID,
116+
value: "custom-model",
117+
});
118+
119+
expect(codexAcpAgent.getSessionState("session-id").currentModelId).toBe("custom-model[high]");
120+
});
121+
93122
it("keeps the legacy models list as combined model/effort entries", async () => {
94123
const {fast, slow} = buildModels();
95124
const {response} = await createSession("fast-model[medium]", [fast, slow]);

0 commit comments

Comments
 (0)