fix(server): a legacy hook's replaced speed and an apply request's options convert before they are compared or applied

A hook that swaps the canonical speed it was sent for fast_mode is diffed in canonical form, so
its replacement is not read as a deletion. An apply request carrying featureValues and options
sends one options patch with options winning, so a superseded legacy value is never applied.
This commit is contained in:
Mohamed Boudra
2026-11-01 19:41:34 +01:00
parent 2fddf0b98c
commit f783c46301
9 changed files with 87 additions and 25 deletions
@@ -2323,9 +2323,18 @@ export class AgentManager {
* `settings.features` sends composer values as `featureValues`; this is where they become an
* options patch for the agent's provider.
*/
async setAgentFeatureValues(agentId: string, values: Record<string, unknown>): Promise<void> {
async setAgentFeatureValues(input: {
agentId: string;
values: Record<string, unknown>;
/** Options sent with the values; they win over a value of the same name. */
options?: Record<string, unknown>;
}): Promise<void> {
const { agentId, values, options } = input;
const provider = this.requireAgent(agentId).provider;
await this.setAgentOptions(agentId, convertLegacyOptions({ provider, options: values }));
await this.setAgentOptions(agentId, {
...convertLegacyOptions({ provider, options: values }),
...options,
});
}
/** The config as the opened session left it: its own difference from the options it was given. */
@@ -4718,8 +4718,9 @@ describe("update_agent MCP tool", () => {
expect(spies.agentManager.setAgentMode).toHaveBeenCalledWith("agent-1", "full-access");
expect(spies.agentManager.setAgentModel).toHaveBeenCalledWith("agent-1", "gpt-5.4");
expect(spies.agentManager.setAgentThinkingOption).toHaveBeenCalledWith("agent-1", "high");
expect(spies.agentManager.setAgentFeatureValues).toHaveBeenCalledWith("agent-1", {
fast_mode: true,
expect(spies.agentManager.setAgentFeatureValues).toHaveBeenCalledWith({
agentId: "agent-1",
values: { fast_mode: true },
});
expect(spies.agentManager.updateAgentMetadata).toHaveBeenCalledWith("agent-1", {
title: "Updated agent",
@@ -49,4 +49,17 @@ describe("legacy Codex Fast at the other entry points", () => {
service_tier: "fast",
});
});
test("a hook that replaces the speed it was sent with fast_mode selects Fast", () => {
const sent = {
provider: "codex",
providerOptions: { service_tier: "default" },
featureValues: { service_tier: "default" },
};
const returned = { ...sent, featureValues: { fast_mode: true } };
expect(foldHookFeatureValues({ returned, sent }).providerOptions).toEqual({
service_tier: "fast",
});
});
});
@@ -111,7 +111,9 @@ export function foldHookFeatureValues<
// No copy was sent, so everything the hook put in `featureValues` is new.
return foldFeatureValues({ config: returned, provider: returned.provider });
}
const after = featureValues ?? {};
// The copy the hook was sent is canonical; what it returns may name the old key, so both
// sides are compared in the same form and a replaced speed is not read as a deletion.
const after = convertLegacyOptions({ provider: returned.provider, options: featureValues ?? {} });
const patch: ProviderOptionsPatch = {};
for (const key of new Set([...Object.keys(before), ...Object.keys(after)])) {
if (isDeepStrictEqual(after[key], before[key])) continue;
@@ -124,7 +126,7 @@ export function foldHookFeatureValues<
provider: returned.provider,
options: rest.providerOptions ?? undefined,
}),
patch: convertLegacyOptions({ provider: returned.provider, options: patch }),
patch,
}),
};
}
@@ -797,7 +797,7 @@ describe("Codex app-server provider controls", () => {
const agentId = await create();
expect(shownSpeed(agentId).selected).toBe("default");
await manager.setAgentFeatureValues(agentId, { fast_mode: true });
await manager.setAgentFeatureValues({ agentId, values: { fast_mode: true } });
expect(shownSpeed(agentId)).toMatchObject({ own: { service_tier: "priority" }, ...FAST });
await runTurn(agentId, "priority");
@@ -818,7 +818,24 @@ describe("Codex app-server provider controls", () => {
const { manager, create, shownSpeed, runTurn } = createLegacyFastHarness();
const agentId = await create({ service_tier: "priority" });
await manager.setAgentFeatureValues(agentId, { service_tier: "default", fast_mode: true });
await manager.setAgentFeatureValues({
agentId,
values: { service_tier: "default", fast_mode: true },
});
expect(shownSpeed(agentId)).toMatchObject({ own: { service_tier: "default" } });
await runTurn(agentId, "default");
});
test("options sent with the older values replace them before anything is applied", async () => {
const { manager, create, shownSpeed, runTurn } = createLegacyFastHarness();
const agentId = await create({ service_tier: "priority" });
await manager.setAgentFeatureValues({
agentId,
values: { service_tier: "retired-tier" },
options: { service_tier: "default" },
});
expect(shownSpeed(agentId)).toMatchObject({ own: { service_tier: "default" } });
await runTurn(agentId, "default");
@@ -828,7 +845,7 @@ describe("Codex app-server provider controls", () => {
const { manager, create, shownSpeed, runTurn } = createLegacyFastHarness();
const agentId = await create({ service_tier: "priority" });
await manager.setAgentFeatureValues(agentId, { fast_mode: false });
await manager.setAgentFeatureValues({ agentId, values: { fast_mode: false } });
expect(shownSpeed(agentId)).toMatchObject({ own: { service_tier: "default" } });
await runTurn(agentId, "default");
@@ -2292,7 +2292,7 @@ function createToolCatalog(
await agentManager.setAgentThinkingOption(agentId, settings.thinkingOptionId);
}
if (settings?.features) {
await agentManager.setAgentFeatureValues(agentId, settings.features);
await agentManager.setAgentFeatureValues({ agentId, values: settings.features });
}
await updateAgentCommand({ agentManager }, { agentId, name, labels });
+1 -1
View File
@@ -1067,7 +1067,7 @@ export class Session {
(await setAgentModeCommand({ agentManager }, { agentId, modeId })).notice,
setModel: (agentId, modelId) => agentManager.setAgentModel(agentId, modelId),
setOptions: (agentId, patch) => agentManager.setAgentOptions(agentId, patch),
setFeatureValues: (agentId, values) => agentManager.setAgentFeatureValues(agentId, values),
setFeatureValues: (input) => agentManager.setAgentFeatureValues(input),
setThinking: (agentId, thinkingOptionId) =>
agentManager.setAgentThinkingOption(agentId, thinkingOptionId),
},
@@ -13,7 +13,11 @@ class FakeAgentConfigOperations implements AgentConfigOperations {
readonly modeCalls: Array<{ agentId: string; modeId: string }> = [];
readonly modelCalls: Array<{ agentId: string; modelId: string | null }> = [];
readonly optionsCalls: Array<{ agentId: string; patch: Record<string, unknown> }> = [];
readonly featureValueCalls: Array<{ agentId: string; values: Record<string, unknown> }> = [];
readonly featureValueCalls: Array<{
agentId: string;
values: Record<string, unknown>;
options?: Record<string, unknown>;
}> = [];
readonly thinkingCalls: Array<{ agentId: string; thinkingOptionId: string | null }> = [];
/** Cross-operation ordering, which the per-operation arrays above cannot show. */
readonly callLog: string[] = [];
@@ -46,8 +50,12 @@ class FakeAgentConfigOperations implements AgentConfigOperations {
if (this.failWith) throw this.failWith;
}
async setFeatureValues(agentId: string, values: Record<string, unknown>): Promise<void> {
this.featureValueCalls.push({ agentId, values });
async setFeatureValues(input: {
agentId: string;
values: Record<string, unknown>;
options?: Record<string, unknown>;
}): Promise<void> {
this.featureValueCalls.push(input);
this.callLog.push("featureValues");
if (this.failWith) throw this.failWith;
}
@@ -366,7 +374,7 @@ describe("AgentConfigSession", () => {
]);
});
test("apply: legacy featureValues are applied first and options after, so options win", async () => {
test("apply: legacy featureValues and options go in one call, options winning", async () => {
const { subsystem, operations } = makeSubsystem();
await subsystem.handleAgentConfigApplyRequest({
@@ -379,11 +387,14 @@ describe("AgentConfigSession", () => {
requestId: "req-1",
});
expect(operations.callLog).toEqual(["featureValues", "options"]);
expect(operations.callLog).toEqual(["featureValues"]);
expect(operations.featureValueCalls).toEqual([
{ agentId: "agent-1", values: { fast_mode: true, service_tier: "fast" } },
{
agentId: "agent-1",
values: { fast_mode: true, service_tier: "fast" },
options: { fast_mode: false },
},
]);
expect(operations.optionsCalls).toEqual([{ agentId: "agent-1", patch: { fast_mode: false } }]);
});
test("apply: leaves omitted fields alone", async () => {
@@ -31,7 +31,11 @@ export interface AgentConfigOperations {
setModel(agentId: string, modelId: string | null): Promise<void>;
setOptions(agentId: string, patch: ProviderOptionsPatch): Promise<void>;
/** COMPAT(featureValues): added in v0.15.0, remove after 2027-05-01. Older clients' values. */
setFeatureValues(agentId: string, values: Record<string, unknown>): Promise<void>;
setFeatureValues(input: {
agentId: string;
values: Record<string, unknown>;
options?: ProviderOptionsPatch;
}): Promise<void>;
setThinking(
agentId: string,
thinkingOptionId: string | null,
@@ -117,7 +121,7 @@ export class AgentConfigSession {
failureText: "Failed to set agent feature",
// COMPAT(featureValues): added in v0.15.0, remove after 2027-05-01. A feature is a one-key options patch.
run: async () => {
await this.operations.setFeatureValues(agentId, { [featureId]: value });
await this.operations.setFeatureValues({ agentId, values: { [featureId]: value } });
return undefined;
},
emitResponse: (payload) => this.host.emit({ type: "set_agent_feature_response", payload }),
@@ -194,11 +198,16 @@ export class AgentConfigSession {
notice ??= thinkingNotice;
}
// COMPAT(featureValues): added in v0.15.0, remove after 2027-05-01. `featureValues` is an options patch.
if (config.featureValues && Object.keys(config.featureValues).length > 0) {
await this.operations.setFeatureValues(agentId, config.featureValues);
}
if (config.options && Object.keys(config.options).length > 0) {
await this.operations.setOptions(agentId, config.options);
const hasValues = config.featureValues && Object.keys(config.featureValues).length > 0;
const hasOptions = config.options && Object.keys(config.options).length > 0;
if (hasValues) {
await this.operations.setFeatureValues({
agentId,
values: config.featureValues ?? {},
options: config.options,
});
} else if (hasOptions) {
await this.operations.setOptions(agentId, config.options ?? {});
}
return notice;