diff --git a/apps/cli/AGENTS.md b/apps/cli/AGENTS.md index 028dd16cc..18d7b4f50 100644 --- a/apps/cli/AGENTS.md +++ b/apps/cli/AGENTS.md @@ -268,7 +268,13 @@ Two things the dev build does deliberately, both load-bearing: TARGET model and the ids so validated come back as `validatedConfigIds`, which `validateTurnConfigOptionValues(..., skipIds)` must skip (the probed model's list would wrongly reject them). What cannot be checked offline is dispatched as - requested. Runtime rejections remain in debug diagnostics; Codex/Claude mismatches + requested. The rule follows the VALUES, not the caller: concrete ids that never + passed through a semantic selection — an Agent Role's stored `runConfig`, an + explicit `--config-option`, a frozen Operation replayed after recovery — go + through `resolvePerModelConfigOptionSelection` for the same exemption, because a + per-model option's ABSENCE from the snapshot is exactly what the probed model's + lack of the control looks like and can never be the reason to reject another + model's turn. Runtime rejections remain in debug diagnostics; Codex/Claude mismatches for model, reasoning effort, Fast, or Plan are not promoted to visible `agent_warning` notices, while other rejected selections still are. Compatibility exception: Claude Fable models omit the Fast mode option, so an explicit `fast=false` is skipped as diff --git a/apps/cli/src/commands/session.test.ts b/apps/cli/src/commands/session.test.ts index 5d97136ca..884e019c6 100644 --- a/apps/cli/src/commands/session.test.ts +++ b/apps/cli/src/commands/session.test.ts @@ -475,6 +475,56 @@ describe('session command helpers', () => { ).toThrow(/Allowed values/); }); + it('accepts a stored per-model config option dispatched with another model', () => { + // Codex publishes `fast-mode` only while the current model has a fast speed + // tier, so a probe under a model without one produces this snapshot. An + // Agent Role (or a frozen Operation replay) then dispatches the concrete id + // with no semantic selection to resolve. + const capability: AcpCapabilityCacheEntry = { + ...createAcpCapability(), + configOptions: [ + { + id: 'model', + name: 'Model', + category: 'model', + type: 'select', + currentValue: 'model-a', + options: [ + { value: 'model-a', name: 'Model A' }, + { value: 'model-b', name: 'Model B' }, + ], + }, + ], + models: [], + }; + const roleRunConfig = { modelId: 'model-b', configOptionValues: { 'fast-mode': true } }; + + const requested = applyAgentRunConfigSelection(roleRunConfig, capability); + + expect(requested.unverifiedSelections).toEqual(['fast-mode=true']); + expect(() => + validateTurnConfigOptionValues( + requested.config.configOptionValues, + capability, + requested.validatedConfigIds + ) + ).not.toThrow(); + + // Running the probed model keeps the snapshot authoritative: there the + // missing option really is the model's own answer. + expect(() => { + const probedModel = applyAgentRunConfigSelection( + { modelId: 'model-a', configOptionValues: { 'fast-mode': true } }, + capability + ); + validateTurnConfigOptionValues( + probedModel.config.configOptionValues, + capability, + probedModel.validatedConfigIds + ); + }).toThrow(/Unknown ACP config option/); + }); + it('drops inherited ACP config options that are no longer compatible', () => { expect( filterCompatibleTurnConfigOptionValues( diff --git a/apps/cli/src/commands/session.ts b/apps/cli/src/commands/session.ts index 198bf0d26..f773ce1da 100644 --- a/apps/cli/src/commands/session.ts +++ b/apps/cli/src/commands/session.ts @@ -51,6 +51,7 @@ import { hasAgentRunConfigSelection, resolveAgentRunConfigSelection, resolveBaseBranchPreference, + resolvePerModelConfigOptionSelection, resolveProjectGitHubRepo, type AgentRunConfigSelection, type AcpCapabilityCacheEntry, @@ -1359,23 +1360,39 @@ export function applyAgentRunConfigSelection( unverifiedSelections: readonly string[]; } { const { runConfig, ...rest } = config; - if (!hasAgentRunConfigSelection(runConfig)) { - return { config: rest, validatedConfigIds: new Set(), unverifiedSelections: [] }; - } - const resolved = resolveAgentRunConfigSelection(runConfig, capability); + const resolved = hasAgentRunConfigSelection(runConfig) + ? resolveAgentRunConfigSelection(runConfig, capability) + : {}; const configOptionValues = { ...(rest.configOptionValues ?? {}), ...(resolved.configOptionValues ?? {}), }; + const modeId = resolved.modeId ?? rest.modeId; + const modelId = resolved.modelId ?? rest.modelId; + // Concrete values that never passed through the semantic resolver — an Agent + // Role's stored run config, `--config-option`, a frozen Operation replay — + // still carry per-model options, so the same rule is applied to the merged + // table here. + const perModel = resolvePerModelConfigOptionSelection({ + configOptionValues, + modelId, + capability, + }); return { config: { ...(rest.taskToolsEnabled !== undefined ? { taskToolsEnabled: rest.taskToolsEnabled } : {}), - ...((resolved.modeId ?? rest.modeId) ? { modeId: resolved.modeId ?? rest.modeId } : {}), - ...((resolved.modelId ?? rest.modelId) ? { modelId: resolved.modelId ?? rest.modelId } : {}), + ...(modeId ? { modeId } : {}), + ...(modelId ? { modelId } : {}), ...(Object.keys(configOptionValues).length > 0 ? { configOptionValues } : {}), }, - validatedConfigIds: new Set(resolved.validatedConfigIds ?? []), - unverifiedSelections: resolved.unverifiedSelections ?? [], + validatedConfigIds: new Set([ + ...(resolved.validatedConfigIds ?? []), + ...perModel.validatedConfigIds, + ]), + unverifiedSelections: [ + ...(resolved.unverifiedSelections ?? []), + ...perModel.unverifiedSelections, + ], }; } @@ -3153,7 +3170,19 @@ export async function sendSessionChatResult( agentConfigId: session.agentConfigId, }); validateTurnModeAndModel(dispatchConfig, capability); - validateTurnConfigOptionValues(dispatchConfig.configOptionValues, capability); + // A follow-up turn carries concrete ids only, so the per-model rule is + // applied directly: `configOptions` describes the probed model, and this + // turn may be running another one. + const perModel = resolvePerModelConfigOptionSelection({ + configOptionValues: dispatchConfig.configOptionValues, + modelId: dispatchConfig.modelId, + capability, + }); + validateTurnConfigOptionValues( + dispatchConfig.configOptionValues, + capability, + new Set(perModel.validatedConfigIds) + ); } const effectiveDispatchConfig = withBuiltinDefaultTurnMode(dispatchConfig, session); diff --git a/packages/shared/src/acp-run-config.ts b/packages/shared/src/acp-run-config.ts index 33de3e8e8..deba989ea 100644 --- a/packages/shared/src/acp-run-config.ts +++ b/packages/shared/src/acp-run-config.ts @@ -335,6 +335,7 @@ export const resolveAgentRunConfigSelection = ( // Agents drop the fast toggle entirely for models that lack fast support, // and no agent publishes which models those are. unverifiedSelections.push(`fastMode=${selection.fastMode}`); + validatedConfigIds.push(option.id); } } @@ -359,3 +360,85 @@ export const resolveAgentRunConfigSelection = ( ...(unverifiedSelections.length > 0 ? { unverifiedSelections } : {}), }; }; + +/** + * Whether a bare config id names an option the agent rebuilds per MODEL. + * + * The snapshot is consulted for the option's CATEGORY, because an agent may + * publish effort under its own id, but an id the snapshot omits still matches: + * omission is exactly what a per-model option does for a model that lacks the + * control (Codex publishes `fast-mode` only while the current model has a fast + * speed tier), so it cannot be the reason to reject one. + */ +const isPerModelConfigOptionId = ( + configId: string, + capability: RunConfigCapabilitySource | undefined +): boolean => { + if (isAcpFastModeConfigId(configId)) { + return true; + } + const option = findConfigOption(capability, (candidate) => candidate.id === configId); + return isAcpThoughtLevelConfigOption(option ?? { id: configId }); +}; + +export type PerModelConfigOptionResolution = { + /** + * Ids the caller must exclude from its probed-snapshot check, because this + * module either validated them against the target model or established that + * the snapshot cannot judge them. + */ + validatedConfigIds: string[]; + /** Requested values that could not be verified offline. */ + unverifiedSelections: string[]; +}; + +/** + * The same per-model rule `resolveAgentRunConfigSelection` applies, for + * CONCRETE config option values that never passed through a semantic + * selection: an Agent Role's stored `runConfig`, an explicit `--config-option`, + * or a frozen Operation replayed after recovery. + * + * Only a turn that runs a model OTHER than the probed one is affected — when + * the turn runs the probed model the snapshot describes it exactly and stays + * authoritative. Effort is still validated strictly wherever the agent + * published its per-model breakdown; what is left is dispatched as requested + * and reported in `unverifiedSelections`, because a snapshot of a different + * model cannot prove the agent will reject it. + */ +export const resolvePerModelConfigOptionSelection = (args: { + configOptionValues: Record | undefined; + modelId: string | undefined; + capability: RunConfigCapabilitySource | undefined; +}): PerModelConfigOptionResolution => { + const entries = Object.entries(args.configOptionValues ?? {}); + const probedModelId = findCurrentModelId(args.capability); + const targetModelId = args.modelId ?? probedModelId; + if (entries.length === 0 || targetModelId === undefined || targetModelId === probedModelId) { + return { validatedConfigIds: [], unverifiedSelections: [] }; + } + + const validatedConfigIds: string[] = []; + const unverifiedSelections: string[] = []; + for (const [configId, value] of entries) { + if (!isPerModelConfigOptionId(configId, args.capability)) { + continue; + } + const targetModelEfforts = isAcpFastModeConfigId(configId) + ? undefined + : args.capability?.modelReasoningEfforts?.[targetModelId]; + if (targetModelEfforts) { + if (typeof value !== 'string' || !targetModelEfforts.includes(value)) { + throw new Error( + `Invalid reasoning effort for model ${targetModelId}: ${String(value)}. Allowed values: ${targetModelEfforts.join( + ', ' + )}.` + ); + } + validatedConfigIds.push(configId); + continue; + } + validatedConfigIds.push(configId); + unverifiedSelections.push(`${configId}=${String(value)}`); + } + return { validatedConfigIds, unverifiedSelections }; +}; diff --git a/packages/shared/tests/acp-run-config.test.ts b/packages/shared/tests/acp-run-config.test.ts index 2f219b785..2360114a1 100644 --- a/packages/shared/tests/acp-run-config.test.ts +++ b/packages/shared/tests/acp-run-config.test.ts @@ -3,6 +3,7 @@ import { describe, expect, it } from 'vitest'; import { deriveModelReasoningEffortsFromLegacyModelIds, resolveAgentRunConfigSelection, + resolvePerModelConfigOptionSelection, summarizeAgentRunConfigCapabilities, type AcpCapabilityCacheEntry, } from '../src'; @@ -146,8 +147,9 @@ describe('agent run config selection', () => { collaboration_mode: 'plan', }, // This agent published no per-model breakdown, and the selection switches - // away from the probed model, so effort/fast cannot be checked offline. - validatedConfigIds: ['reasoning_effort'], + // away from the probed model, so effort/fast cannot be checked offline: + // both are dispatched as requested and excluded from the snapshot check. + validatedConfigIds: ['reasoning_effort', 'fast-mode'], unverifiedSelections: ['reasoningEffort=high', 'fastMode=true'], }); }); @@ -353,3 +355,87 @@ describe('agent run config selection', () => { expect(resolveAgentRunConfigSelection({ planMode: true }, legacy)).toEqual({ modeId: 'plan' }); }); }); + +describe('per-model config options carried as concrete ids', () => { + /** Codex probed under a model without a fast speed tier: no `fast-mode` at all. */ + const withoutFastMode = (): AcpCapabilityCacheEntry => { + const capability = codexCapability(); + return { + ...capability, + configOptions: (capability.configOptions ?? []).filter((option) => option.id !== 'fast-mode'), + }; + }; + + it('keeps a stored fast toggle dispatchable when the turn runs another model', () => { + // An Agent Role's stored run config: concrete ids, no semantic selection. + expect( + resolvePerModelConfigOptionSelection({ + configOptionValues: { 'fast-mode': true, mode: 'agent' }, + modelId: 'gpt-5.4-mini', + capability: codexCapability(), + }) + ).toEqual({ validatedConfigIds: ['fast-mode'], unverifiedSelections: ['fast-mode=true'] }); + + // The probe never saw the option because its model had no fast tier. That + // says nothing about the model this turn selects. + expect( + resolvePerModelConfigOptionSelection({ + configOptionValues: { 'fast-mode': true }, + modelId: 'gpt-5.4-mini', + capability: withoutFastMode(), + }) + ).toEqual({ validatedConfigIds: ['fast-mode'], unverifiedSelections: ['fast-mode=true'] }); + }); + + it('leaves the snapshot authoritative for the model it was probed under', () => { + for (const modelId of ['gpt-5.6-sol', undefined]) { + expect( + resolvePerModelConfigOptionSelection({ + configOptionValues: { 'fast-mode': true }, + modelId, + capability: withoutFastMode(), + }) + ).toEqual({ validatedConfigIds: [], unverifiedSelections: [] }); + } + }); + + it('still validates a stored effort against the target model breakdown', () => { + const capability: AcpCapabilityCacheEntry = { + ...codexCapability(), + modelReasoningEfforts: { + 'gpt-5.6-sol': ['low', 'medium', 'high'], + 'gpt-5.4-mini': ['low', 'medium'], + }, + }; + + expect( + resolvePerModelConfigOptionSelection({ + configOptionValues: { reasoning_effort: 'medium' }, + modelId: 'gpt-5.4-mini', + capability, + }) + ).toEqual({ validatedConfigIds: ['reasoning_effort'], unverifiedSelections: [] }); + + expect(() => + resolvePerModelConfigOptionSelection({ + configOptionValues: { reasoning_effort: 'high' }, + modelId: 'gpt-5.4-mini', + capability, + }) + ).toThrow(/Invalid reasoning effort for model gpt-5\.4-mini.*Allowed values: low, medium/s); + }); + + it('recognizes an agent that publishes effort under its own id', () => { + // Claude: `effort` by category, `fast` as the toggle id. + expect( + resolvePerModelConfigOptionSelection({ + configOptionValues: { effort: 'high', fast: 'on', mode: 'auto' }, + modelId: 'opus', + capability: claudeCapability(), + }) + ).toEqual({ + validatedConfigIds: ['effort', 'fast'], + unverifiedSelections: ['effort=high', 'fast=on'], + }); + }); +});