diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_profile.go b/apps/backend/internal/agent/runtime/lifecycle/manager_profile.go index 2b285f3d17..3fd7b801d2 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_profile.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_profile.go @@ -162,6 +162,17 @@ func (m *Manager) resolveProfileSessionConfig(ctx context.Context, profileID str // invokes the callback when there's no taskDescription/attachments to send, which is a *boot* // signal (the agent has never run a turn). When there IS a prompt, the callback is unused and // MarkReady fires later from handleCompleteEvent — that path is the true turn-end. +// +// Note: the only session-level override applied on resume is `mode` (see +// effectiveSessionMode / issue #1183). We deliberately do NOT replay the +// model or dynamic config options from AgentProfileSnapshot here. Agents +// that support session/load preserve those values themselves; agents that +// don't can use the profile defaults. Replaying snapshot state on every +// resume issues redundant SetModel / SetConfigOption RPCs that cycle the +// session through STARTING / RUNNING and flicker the task into the sidebar's +// Running bucket (see session-resume-keeps-review-state.spec.ts). +// SSR-side page-reload persistence still works because the model selector +// reads `agent_profile_snapshot.model` directly. func (m *Manager) initializeACPSession(ctx context.Context, execution *AgentExecution, agentConfig agents.Agent, taskDescription string, attachments []MessageAttachment, mcpServers []agentctltypes.McpServer) error { profileModel, profileMode, profileConfigOptions := m.resolveProfileSessionConfig(ctx, execution.AgentProfileID) mode := m.effectiveSessionMode(ctx, execution, profileMode) diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go index 7754eaeb4a..c1fec48615 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go @@ -41,16 +41,6 @@ const ( // configOptionIDModel is the well-known ConfigOption ID/Category value used // by ACP agents to surface the active model as a selectable option. configOptionIDModel = "model" - - // Codex surfaces reasoning effort as a separate config option while its - // availableModels list uses model/effort IDs such as gpt-5.5/medium. - configOptionIDReasoningEffort = "reasoning_effort" - configOptionCategoryThoughtLevel = "thought_level" - - reasoningEffortLow = "low" - reasoningEffortMedium = "medium" - reasoningEffortHigh = "high" - reasoningEffortXHigh = "xhigh" ) // wakeupPromptTimeout bounds how long a synthetic wakeup prompt can run. diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_session.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_session.go index 3687f20e6e..d17ec876a7 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_session.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_session.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "strings" "github.com/coder/acp-go-sdk" "github.com/kandev/kandev/internal/agentctl/server/adapter/transport/shared" @@ -430,14 +429,14 @@ func (a *Adapter) emitSessionModels(sessionID string, models *sessionModelState, configOptions := sessionConfigOptions(meta, acpConfigOptions) // Fallback: if the SDK didn't parse currentModelId (some agents omit it), - // try to resolve it from a model-shaped configOption. We deliberately do - // NOT fall back to AvailableModels[0]: agents like auggie return an - // alphabetically-sorted list whose first entry is a pseudo-agent ("Build - // Analyzer"), which clobbered the profile model in the UI. When neither - // CurrentModelId nor a configOption surface a value, emit empty and let - // the frontend fall through to its profile/snapshot resolution. + // take the model-shaped configOption's CurrentValue verbatim. We + // deliberately do NOT fall back to AvailableModels[0]: agents like auggie + // return an alphabetically-sorted list whose first entry is a pseudo-agent + // ("Build Analyzer"), which clobbered the profile model in the UI. When + // neither CurrentModelId nor a configOption surface a value, emit empty + // and let the frontend fall through to its profile/snapshot resolution. if currentModelID == "" { - currentModelID = resolveCurrentModelFromConfig(configOptions, models.AvailableModels) + currentModelID = currentModelFromConfig(configOptions) } // Cache config options so emitSetModelEvent can include them in the @@ -481,21 +480,18 @@ func (a *Adapter) emitSetModelEvent(sessionID, modelID string, cachedModels []mo // deep copy to avoid aliasing the caller's backing array. outConfig = make([]streams.ConfigOption, len(cachedConfig)) copy(outConfig, cachedConfig) - baseModelID, reasoningEffort, splitReasoningModel := splitReasoningModelID(modelID, outConfig) for i := range outConfig { if outConfig[i].ID == configOptionIDModel || outConfig[i].Category == configOptionIDModel { - if splitReasoningModel { - outConfig[i].CurrentValue = baseModelID - } else { - outConfig[i].CurrentValue = modelID - } - } - if splitReasoningModel && - (outConfig[i].ID == configOptionIDReasoningEffort || - outConfig[i].Category == configOptionCategoryThoughtLevel) { - outConfig[i].CurrentValue = reasoningEffort + outConfig[i].CurrentValue = modelID } } + // Refresh the cached config options so a subsequent SetConfigOption + // (e.g. user toggles reasoning effort after switching model) doesn't + // reuse the stale model CurrentValue from session/new and clobber the + // just-applied model in the convergence event. + a.mu.Lock() + a.availableConfigOptions = outConfig + a.mu.Unlock() } a.logger.Info("emitting session_models convergence event after SetModel", @@ -511,83 +507,16 @@ func (a *Adapter) emitSetModelEvent(sessionID, modelID string, cachedModels []mo }) } -// resolveCurrentModelFromConfig extracts current model ID from configOptions. -func resolveCurrentModelFromConfig(options []streams.ConfigOption, available []modelInfo) string { - modelID := "" - reasoningEffort := "" +// currentModelFromConfig returns the CurrentValue of the model-shaped +// configOption (matched by well-known ID or Category="model"), or empty +// when none is present. +func currentModelFromConfig(options []streams.ConfigOption) string { for _, opt := range options { if opt.ID == configOptionIDModel || opt.Category == configOptionIDModel { - modelID = opt.CurrentValue - } - if opt.ID == configOptionIDReasoningEffort || opt.Category == configOptionCategoryThoughtLevel { - reasoningEffort = opt.CurrentValue + return opt.CurrentValue } } - if modelID == "" { - return "" - } - if reasoningEffort != "" { - combined := modelID + "/" + reasoningEffort - if modelIDExists(combined, available) { - return combined - } - } - if modelIDExists(modelID, available) { - return modelID - } - // Keep the agent-reported config value as a best-effort fallback for - // providers that expose the current model only through configOptions. - return modelID -} - -func splitReasoningModelID(modelID string, options []streams.ConfigOption) (string, string, bool) { - allowedReasoningEfforts := map[string]bool{} - hasReasoningOption := false - for _, opt := range options { - if opt.ID == configOptionIDReasoningEffort || opt.Category == configOptionCategoryThoughtLevel { - hasReasoningOption = true - for _, optionValue := range opt.Options { - if optionValue.Value != "" { - allowedReasoningEfforts[optionValue.Value] = true - } - } - } - } - if hasReasoningOption && len(allowedReasoningEfforts) == 0 { - for _, effort := range []string{ - reasoningEffortLow, - reasoningEffortMedium, - reasoningEffortHigh, - reasoningEffortXHigh, - } { - allowedReasoningEfforts[effort] = true - } - } - if len(allowedReasoningEfforts) == 0 { - return "", "", false - } - slashIndex := strings.LastIndex(modelID, "/") - if slashIndex < 1 || slashIndex == len(modelID)-1 { - return "", "", false - } - baseModelID := modelID[:slashIndex] - reasoningEffort := modelID[slashIndex+1:] - if !allowedReasoningEfforts[reasoningEffort] { - return "", "", false - } - return baseModelID, reasoningEffort, true -} - -func modelIDExists(modelID string, available []modelInfo) bool { - if len(available) == 0 { - return false - } - for _, model := range available { - if model.ModelId == modelID { - return true - } - } - return false + return "" } // SetMode changes the agent's session mode via ACP session/set_mode. @@ -734,10 +663,20 @@ func (a *Adapter) maybeEmitAuthRequired(err error) bool { // SetConfigOption sets a session configuration option via ACP session/set_config_option. // configID is the option's ID; value is the option-value ID to apply. +// +// On success a session_models convergence event is emitted with the updated +// option's CurrentValue. The orchestrator persists the change to +// AgentProfileSnapshot so model + secondary options (reasoning effort, +// thought level, …) survive page refresh and backend restart. Agents that +// proactively send a ConfigOptionUpdate notification will produce a second, +// equivalent event; downstream persistence is idempotent so duplicates are +// harmless. func (a *Adapter) SetConfigOption(ctx context.Context, configID, value string) error { a.mu.RLock() conn := a.acpConn sessionID := a.sessionID + cachedModels := a.availableModels + cachedConfig := a.availableConfigOptions a.mu.RUnlock() if conn == nil { @@ -757,9 +696,89 @@ func (a *Adapter) SetConfigOption(ctx context.Context, configID, value string) e if err != nil { return fmt.Errorf("set session config option failed: %w", err) } + if isModelConfigID(configID, cachedConfig) { + a.emitSetModelEvent(sessionID, value, cachedModels, cachedConfig) + } else { + a.emitSetConfigOptionEvent(sessionID, configID, value, cachedModels, cachedConfig) + } return nil } +// emitSetConfigOptionEvent emits a session_models convergence event after a +// non-model SetConfigOption RPC succeeds. The frontend uses this to keep the +// option dropdowns in sync with the agent without waiting for an agent-driven +// ConfigOptionUpdate. +func (a *Adapter) emitSetConfigOptionEvent(sessionID, configID, value string, cachedModels []modelInfo, cachedConfig []streams.ConfigOption) { + if len(cachedConfig) == 0 { + // In normal operation session/new populates availableConfigOptions + // before the frontend can fire a SetConfigOption. Hitting this branch + // means we accepted the RPC against a cache that was never seeded — + // skip the convergence event entirely (an empty one would briefly + // blank the UI selectors) and rely on the agent's own + // ConfigOptionUpdate notification to correct the snapshot. + a.logger.Warn("SetConfigOption succeeded but local config cache is empty; skipping convergence event", + zap.String("session_id", sessionID), + zap.String("config_id", configID), + ) + return + } + outConfig := make([]streams.ConfigOption, len(cachedConfig)) + copy(outConfig, cachedConfig) + found := false + for i := range outConfig { + if outConfig[i].ID == configID { + outConfig[i].CurrentValue = value + found = true + break + } + } + if !found { + // The agent accepted a configID that wasn't in its own + // availableConfigOptions list — most likely a stale frontend cache + // or an agent-side drift between session/new and ConfigOptionUpdate. + // The event carries the unmutated CurrentValues; the agent's own + // ConfigOptionUpdate notification will correct the snapshot shortly. + a.logger.Warn("SetConfigOption: configID not in local cache; convergence event carries stale options", + zap.String("session_id", sessionID), + zap.String("config_id", configID), + ) + } + // Refresh the cached config options so consecutive SetConfigOption + // calls (or a follow-up SetModel) read the latest CurrentValues + // instead of the stale session/new snapshot. + a.mu.Lock() + a.availableConfigOptions = outConfig + a.mu.Unlock() + + a.logger.Info("emitting session_models convergence event after SetConfigOption", + zap.String("session_id", sessionID), + zap.String("config_id", configID), + zap.String("value", value), + ) + a.sendUpdate(AgentEvent{ + Type: streams.EventTypeSessionModels, + SessionID: sessionID, + CurrentModelID: currentModelFromConfig(outConfig), + SessionModels: convertSessionModels(cachedModels), + ConfigOptions: outConfig, + }) +} + +// isModelConfigID reports whether configID identifies the model-shaped +// SessionConfigOption — either the well-known "model" ID, or a custom ID that +// the agent tagged with Category="model" in its session config options. +func isModelConfigID(configID string, cachedConfig []streams.ConfigOption) bool { + if configID == configOptionIDModel { + return true + } + for _, opt := range cachedConfig { + if opt.ID == configID && opt.Category == configOptionIDModel { + return true + } + } + return false +} + // Authenticate triggers ACP session/authenticate for a given auth method. func (a *Adapter) Authenticate(ctx context.Context, methodID string) error { a.mu.RLock() diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_updates.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_updates.go index 456f918dd2..7c162f51ab 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_updates.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_updates.go @@ -230,11 +230,10 @@ func (a *Adapter) convertNotification(n acp.SessionNotification) *AgentEvent { cachedModels := a.availableModels a.availableConfigOptions = configOptions a.mu.Unlock() - currentModelID := resolveCurrentModelFromConfig(configOptions, cachedModels) return &AgentEvent{ Type: streams.EventTypeSessionModels, SessionID: sessionID, - CurrentModelID: currentModelID, + CurrentModelID: currentModelFromConfig(configOptions), SessionModels: convertSessionModels(cachedModels), ConfigOptions: configOptions, } diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/session_models_test.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/session_models_test.go index 2b20dac73f..4def26a229 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/session_models_test.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/session_models_test.go @@ -74,91 +74,6 @@ func TestEmitSessionModels_EmptyCurrentIDFromConfigOption(t *testing.T) { } } -// TestEmitSessionModels_EmptyCurrentIDComposesReasoningEffort pins Codex's -// split config-option shape: configOptions reports model="gpt-5.5" and -// reasoning_effort="medium", while availableModels carries the actual -// selectable ID "gpt-5.5/medium". -func TestEmitSessionModels_EmptyCurrentIDComposesReasoningEffort(t *testing.T) { - a := newTestAdapter() - models := &sessionModelState{ - CurrentModelId: "", - AvailableModels: []modelInfo{ - {ModelId: "gpt-5.5/low", Name: "GPT-5.5 (low)"}, - {ModelId: "gpt-5.5/medium", Name: "GPT-5.5 (medium)"}, - }, - } - meta := map[string]any{ - "configOptions": []any{ - map[string]any{ - "type": "select", - "id": "model", - "name": "Model", - "category": "model", - "currentValue": "gpt-5.5", - }, - map[string]any{ - "type": "select", - "id": "reasoning_effort", - "name": "Reasoning Effort", - "category": "thought_level", - "currentValue": "medium", - }, - }, - } - - a.emitSessionModels("sess-1", models, meta, nil) - - ev := findSessionModelsEvent(t, drainEvents(a)) - if ev.CurrentModelID != "gpt-5.5/medium" { - t.Errorf("CurrentModelID = %q, want %q", ev.CurrentModelID, "gpt-5.5/medium") - } -} - -func TestEmitSessionModels_EmptyCurrentIDComposesReasoningEffortFromTypedOptions(t *testing.T) { - a := newTestAdapter() - modelCategory := acp.SessionConfigOptionCategoryModel - thoughtCategory := acp.SessionConfigOptionCategoryThoughtLevel - modelOptions := acp.SessionConfigSelectOptionsUngrouped{ - {Name: "GPT-5.5", Value: "gpt-5.5"}, - } - effortOptions := acp.SessionConfigSelectOptionsUngrouped{ - {Name: "Low", Value: reasoningEffortLow}, - {Name: "Medium", Value: reasoningEffortMedium}, - } - models := &sessionModelState{ - CurrentModelId: "", - AvailableModels: []modelInfo{ - {ModelId: "gpt-5.5/low", Name: "GPT-5.5 (low)"}, - {ModelId: "gpt-5.5/medium", Name: "GPT-5.5 (medium)"}, - }, - } - configOptions := []acp.SessionConfigOption{ - {Select: &acp.SessionConfigOptionSelect{ - Type: "select", - Id: "model", - Name: "Model", - Category: &modelCategory, - CurrentValue: "gpt-5.5", - Options: acp.SessionConfigSelectOptions{Ungrouped: &modelOptions}, - }}, - {Select: &acp.SessionConfigOptionSelect{ - Type: "select", - Id: "reasoning_effort", - Name: "Reasoning Effort", - Category: &thoughtCategory, - CurrentValue: reasoningEffortMedium, - Options: acp.SessionConfigSelectOptions{Ungrouped: &effortOptions}, - }}, - } - - a.emitSessionModels("sess-1", models, nil, configOptions) - - ev := findSessionModelsEvent(t, drainEvents(a)) - if ev.CurrentModelID != "gpt-5.5/medium" { - t.Errorf("CurrentModelID = %q, want %q", ev.CurrentModelID, "gpt-5.5/medium") - } -} - func TestInitialSessionModelState_UsesConfigOptionsWithoutModels(t *testing.T) { modelCategory := acp.SessionConfigOptionCategoryModel modelOptions := acp.SessionConfigSelectOptionsUngrouped{ @@ -398,35 +313,50 @@ func TestInitialSessionModelState_TypedConfigOptionsBeatLegacyModels(t *testing. } } -func TestResolveCurrentModelFromConfig_ComposesReasoningEffort(t *testing.T) { - options := []streams.ConfigOption{ - {Type: "select", ID: "model", Category: "model", CurrentValue: "gpt-5.5"}, - {Type: "select", ID: "reasoning_effort", Category: "thought_level", CurrentValue: reasoningEffortMedium}, +// TestCurrentModelFromConfig_ReadsModelShapedOption pins the simple verbatim +// fallback used by emitSessionModels / emitSetConfigOptionEvent / +// ConfigOptionUpdate: the model-shaped option (well-known ID or category) +// surfaces its CurrentValue unchanged, regardless of any other options. +func TestCurrentModelFromConfig_ReadsModelShapedOption(t *testing.T) { + cases := []struct { + name string + options []streams.ConfigOption + want string + }{ + { + name: "well-known id", + options: []streams.ConfigOption{ + {ID: "model", CurrentValue: "gpt-5.5"}, + }, + want: "gpt-5.5", + }, + { + name: "category-tagged custom id", + options: []streams.ConfigOption{ + {ID: "primary_model", Category: "model", CurrentValue: "claude-opus-4-7"}, + }, + want: "claude-opus-4-7", + }, + { + name: "non-model options ignored", + options: []streams.ConfigOption{ + {ID: "reasoning_effort", CurrentValue: "high"}, + {ID: "mode", CurrentValue: "default"}, + }, + want: "", + }, + { + name: "empty options", + options: nil, + want: "", + }, } - available := []modelInfo{ - {ModelId: "gpt-5.5/low", Name: "GPT-5.5 (low)"}, - {ModelId: "gpt-5.5/medium", Name: "GPT-5.5 (medium)"}, - } - - got := resolveCurrentModelFromConfig(options, available) - if got != "gpt-5.5/medium" { - t.Errorf("resolveCurrentModelFromConfig() = %q, want %q", got, "gpt-5.5/medium") - } -} - -func TestResolveCurrentModelFromConfig_PrefersReasoningModelWhenBaseAlsoAvailable(t *testing.T) { - options := []streams.ConfigOption{ - {Type: "select", ID: "model", Category: "model", CurrentValue: "gpt-5.5"}, - {Type: "select", ID: "reasoning_effort", Category: "thought_level", CurrentValue: reasoningEffortMedium}, - } - available := []modelInfo{ - {ModelId: "gpt-5.5", Name: "GPT-5.5"}, - {ModelId: "gpt-5.5/medium", Name: "GPT-5.5 (medium)"}, - } - - got := resolveCurrentModelFromConfig(options, available) - if got != "gpt-5.5/medium" { - t.Errorf("resolveCurrentModelFromConfig() = %q, want %q", got, "gpt-5.5/medium") + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := currentModelFromConfig(tc.options); got != tc.want { + t.Errorf("currentModelFromConfig() = %q, want %q", got, tc.want) + } + }) } } @@ -506,150 +436,6 @@ func TestEmitSetModelEvent_EmitsSessionModelsWithCachedState(t *testing.T) { } } -func TestEmitSetModelEvent_RewritesSplitReasoningOptions(t *testing.T) { - a := newTestAdapter() - - reasoningOptions := []streams.ConfigOptionValue{ - {Name: "Medium", Value: reasoningEffortMedium}, - {Name: "High", Value: reasoningEffortHigh}, - } - cachedModels := []modelInfo{ - {ModelId: "gpt-5.5/medium", Name: "GPT-5.5 (medium)"}, - {ModelId: "gpt-5.5/high", Name: "GPT-5.5 (high)"}, - } - cachedConfig := []streams.ConfigOption{ - {Type: "select", ID: "model", Category: "model", Name: "Model", CurrentValue: "gpt-5.5"}, - { - Type: "select", - ID: "reasoning_effort", - Category: "thought_level", - Name: "Reasoning Effort", - CurrentValue: reasoningEffortMedium, - Options: reasoningOptions, - }, - } - - a.emitSetModelEvent("sess-1", "gpt-5.5/high", cachedModels, cachedConfig) - - ev := findSessionModelsEvent(t, drainEvents(a)) - if ev.CurrentModelID != "gpt-5.5/high" { - t.Errorf("CurrentModelID = %q, want %q", ev.CurrentModelID, "gpt-5.5/high") - } - if len(ev.ConfigOptions) != 2 { - t.Fatalf("ConfigOptions len = %d, want 2", len(ev.ConfigOptions)) - } - for _, opt := range ev.ConfigOptions { - switch opt.ID { - case "model": - if opt.CurrentValue != "gpt-5.5" { - t.Errorf("model option CurrentValue = %q, want %q", opt.CurrentValue, "gpt-5.5") - } - case "reasoning_effort": - if opt.CurrentValue != reasoningEffortHigh { - t.Errorf("reasoning option CurrentValue = %q, want %q", opt.CurrentValue, reasoningEffortHigh) - } - default: - t.Errorf("unexpected option ID %q in ConfigOptions", opt.ID) - } - } -} - -func TestEmitSetModelEvent_RewritesSplitReasoningOptionsWithSlashInBaseModel(t *testing.T) { - a := newTestAdapter() - - reasoningOptions := []streams.ConfigOptionValue{ - {Name: "Medium", Value: reasoningEffortMedium}, - {Name: "High", Value: reasoningEffortHigh}, - } - cachedModels := []modelInfo{ - {ModelId: "vendor/gpt-5.5/medium", Name: "Vendor GPT-5.5 (medium)"}, - {ModelId: "vendor/gpt-5.5/high", Name: "Vendor GPT-5.5 (high)"}, - } - cachedConfig := []streams.ConfigOption{ - {Type: "select", ID: "model", Category: "model", Name: "Model", CurrentValue: "vendor/gpt-5.5"}, - { - Type: "select", - ID: "reasoning_effort", - Category: "thought_level", - Name: "Reasoning Effort", - CurrentValue: reasoningEffortMedium, - Options: reasoningOptions, - }, - } - - a.emitSetModelEvent("sess-1", "vendor/gpt-5.5/high", cachedModels, cachedConfig) - - ev := findSessionModelsEvent(t, drainEvents(a)) - if ev.CurrentModelID != "vendor/gpt-5.5/high" { - t.Errorf("CurrentModelID = %q, want %q", ev.CurrentModelID, "vendor/gpt-5.5/high") - } - if len(ev.ConfigOptions) != 2 { - t.Fatalf("ConfigOptions len = %d, want 2", len(ev.ConfigOptions)) - } - for _, opt := range ev.ConfigOptions { - switch opt.ID { - case "model": - if opt.CurrentValue != "vendor/gpt-5.5" { - t.Errorf("model option CurrentValue = %q, want %q", opt.CurrentValue, "vendor/gpt-5.5") - } - case "reasoning_effort": - if opt.CurrentValue != reasoningEffortHigh { - t.Errorf("reasoning option CurrentValue = %q, want %q", opt.CurrentValue, reasoningEffortHigh) - } - default: - t.Errorf("unexpected option ID %q in ConfigOptions", opt.ID) - } - } -} - -func TestEmitSetModelEvent_DoesNotSplitSlashModelWithoutReasoningSuffix(t *testing.T) { - a := newTestAdapter() - - reasoningOptions := []streams.ConfigOptionValue{ - {Name: "Low", Value: reasoningEffortLow}, - {Name: "Medium", Value: reasoningEffortMedium}, - {Name: "High", Value: reasoningEffortHigh}, - } - cachedModels := []modelInfo{ - {ModelId: "vendor/gpt-5.5", Name: "Vendor GPT-5.5"}, - } - cachedConfig := []streams.ConfigOption{ - {Type: "select", ID: "model", Category: "model", Name: "Model", CurrentValue: "old-model"}, - { - Type: "select", - ID: "reasoning_effort", - Category: "thought_level", - Name: "Reasoning Effort", - CurrentValue: reasoningEffortMedium, - Options: reasoningOptions, - }, - } - - a.emitSetModelEvent("sess-1", "vendor/gpt-5.5", cachedModels, cachedConfig) - - ev := findSessionModelsEvent(t, drainEvents(a)) - if ev.CurrentModelID != "vendor/gpt-5.5" { - t.Errorf("CurrentModelID = %q, want %q", ev.CurrentModelID, "vendor/gpt-5.5") - } - if len(ev.ConfigOptions) != 2 { - t.Fatalf("ConfigOptions len = %d, want 2", len(ev.ConfigOptions)) - } - for _, opt := range ev.ConfigOptions { - switch opt.ID { - case "model": - if opt.CurrentValue != "vendor/gpt-5.5" { - t.Errorf("model option CurrentValue = %q, want %q", opt.CurrentValue, "vendor/gpt-5.5") - } - case "reasoning_effort": - if opt.CurrentValue != reasoningEffortMedium { - t.Errorf("reasoning option CurrentValue = %q, want %q", opt.CurrentValue, reasoningEffortMedium) - } - default: - t.Errorf("unexpected option ID %q in ConfigOptions", opt.ID) - } - } -} - // TestConfigOptionUpdate_RefreshesCachedConfig pins that an inbound // ConfigOptionUpdate notification refreshes the adapter's availableConfigOptions // cache, so a subsequent SetModel convergence event emits the latest options @@ -696,58 +482,6 @@ func TestConfigOptionUpdate_RefreshesCachedConfig(t *testing.T) { } } -func TestConfigOptionUpdate_ComposesReasoningEffortCurrentModel(t *testing.T) { - a := newTestAdapter() - thoughtCategory := acp.SessionConfigOptionCategoryThoughtLevel - modelOptions := acp.SessionConfigSelectOptionsUngrouped{ - {Name: "GPT-5.5", Value: "gpt-5.5"}, - } - effortOptions := acp.SessionConfigSelectOptionsUngrouped{ - {Name: "Medium", Value: reasoningEffortMedium}, - {Name: "High", Value: reasoningEffortHigh}, - } - - a.mu.Lock() - a.availableModels = []modelInfo{ - {ModelId: "gpt-5.5/medium", Name: "GPT-5.5 (medium)"}, - {ModelId: "gpt-5.5/high", Name: "GPT-5.5 (high)"}, - } - a.mu.Unlock() - - notif := acp.SessionNotification{ - SessionId: "sess-1", - Update: acp.SessionUpdate{ - ConfigOptionUpdate: &acp.SessionConfigOptionUpdate{ - ConfigOptions: []acp.SessionConfigOption{ - {Select: &acp.SessionConfigOptionSelect{ - Type: "select", - Id: "model", - Name: "Model", - CurrentValue: "gpt-5.5", - Options: acp.SessionConfigSelectOptions{Ungrouped: &modelOptions}, - }}, - {Select: &acp.SessionConfigOptionSelect{ - Type: "select", - Id: "reasoning_effort", - Name: "Reasoning Effort", - Category: &thoughtCategory, - CurrentValue: reasoningEffortHigh, - Options: acp.SessionConfigSelectOptions{Ungrouped: &effortOptions}, - }}, - }, - }, - }, - } - - ev := a.convertNotification(notif) - if ev == nil { - t.Fatalf("expected a session_models event from ConfigOptionUpdate") - } - if ev.CurrentModelID != "gpt-5.5/high" { - t.Errorf("event CurrentModelID = %q, want %q", ev.CurrentModelID, "gpt-5.5/high") - } -} - // TestFinalizeSetModel_MethodNoneEmitsNothing pins the contract that when // applySessionModel returns MethodNone (agent supports neither // session/set_config_option nor session/set_model), the adapter must NOT emit @@ -790,3 +524,60 @@ func TestFinalizeSetModel_RealMethodEmitsEvent(t *testing.T) { t.Errorf("CurrentModelID = %q, want %q", ev.CurrentModelID, "claude-opus-4-7") } } + +// TestSetModelThenSetConfigOption_PreservesModel pins the regression where +// changing a non-model option (e.g. reasoning effort) after a SetModel call +// reverted CurrentModelID to the agent's initial default. Root cause: +// emitSetModelEvent rewrote outConfig but never refreshed the adapter's +// availableConfigOptions, so the next emitSetConfigOptionEvent read the stale +// model CurrentValue. Fix: both emit paths now write the rewritten outConfig +// back to the cache. With model and reasoning_effort as fully independent +// options (Codex 0.16.0+ / Auggie 0.27.0+), the model is whatever the +// model-shaped option's CurrentValue says — verbatim. +func TestSetModelThenSetConfigOption_PreservesModel(t *testing.T) { + a := newTestAdapter() + + reasoningOptions := []streams.ConfigOptionValue{ + {Name: "Medium", Value: "medium"}, + {Name: "High", Value: "high"}, + } + cachedModels := []modelInfo{ + {ModelId: "gpt-5.4-mini", Name: "GPT-5.4 Mini"}, + {ModelId: "gpt-5.5", Name: "GPT-5.5"}, + } + // Seed the adapter cache with the agent's initial state — model defaults + // to gpt-5.5 with high effort, mirroring what session/new would surface. + initialConfig := []streams.ConfigOption{ + {Type: "select", ID: "model", Category: "model", Name: "Model", CurrentValue: "gpt-5.5"}, + { + Type: "select", + ID: "reasoning_effort", + Name: "Reasoning Effort", + CurrentValue: "high", + Options: reasoningOptions, + }, + } + a.mu.Lock() + a.availableModels = cachedModels + a.availableConfigOptions = initialConfig + a.mu.Unlock() + + // 1. User selects gpt-5.4-mini — emitSetModelEvent rewrites + // model.CurrentValue to "gpt-5.4-mini" and refreshes the cache. + a.emitSetModelEvent("sess-1", "gpt-5.4-mini", cachedModels, initialConfig) + _ = drainEvents(a) + + // 2. User flips reasoning effort to medium. The cached model option must + // already reflect step 1, so currentModelFromConfig returns "gpt-5.4-mini" + // — NOT the original gpt-5.5. + a.mu.RLock() + cachedAfterSetModel := a.availableConfigOptions + a.mu.RUnlock() + a.emitSetConfigOptionEvent("sess-1", "reasoning_effort", "medium", cachedModels, cachedAfterSetModel) + + ev := findSessionModelsEvent(t, drainEvents(a)) + if ev.CurrentModelID != "gpt-5.4-mini" { + t.Errorf("CurrentModelID = %q, want %q (model must not revert to agent default)", + ev.CurrentModelID, "gpt-5.4-mini") + } +} diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/set_config_option_test.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/set_config_option_test.go index 59d89888be..2f491b1c7c 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/set_config_option_test.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/set_config_option_test.go @@ -4,6 +4,8 @@ import ( "context" "strings" "testing" + + "github.com/kandev/kandev/internal/agentctl/types/streams" ) // TestSetConfigOption_WithoutConnectionReturnsError pins the precondition @@ -21,3 +23,106 @@ func TestSetConfigOption_WithoutConnectionReturnsError(t *testing.T) { t.Errorf("error = %q, want one containing %q", err.Error(), "not initialized") } } + +// TestIsModelConfigID pins the recognizer used by SetConfigOption to decide +// whether a successful set_config_option RPC must also emit a session_models +// convergence event (so the orchestrator persists AgentProfileSnapshot["model"] +// and the selection survives a page refresh). +func TestIsModelConfigID(t *testing.T) { + cachedConfig := []streams.ConfigOption{ + {Type: "select", ID: "model", Name: "Model"}, + {Type: "select", ID: "thought_level", Category: "model", Name: "Thought"}, + {Type: "select", ID: "reasoning_effort", Name: "Reasoning"}, + } + + cases := []struct { + name string + configID string + want bool + }{ + {"well-known model ID matches", "model", true}, + {"custom ID with model category matches", "thought_level", true}, + {"unrelated config ID does not match", "reasoning_effort", false}, + {"unknown ID does not match", "missing", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := isModelConfigID(tc.configID, cachedConfig); got != tc.want { + t.Errorf("isModelConfigID(%q) = %v, want %v", tc.configID, got, tc.want) + } + }) + } + + // Empty cached config still treats the well-known ID as the model so the + // agent can be served before its first ConfigOptionUpdate has refreshed + // the cache. + if !isModelConfigID("model", nil) { + t.Errorf("isModelConfigID(\"model\", nil) = false, want true") + } +} + +// TestEmitSetConfigOptionEvent_RewritesChangedOptionAndKeepsModel pins that +// emitSetConfigOptionEvent (called from SetConfigOption for non-model option +// changes) emits a session_models convergence event with the changed option's +// CurrentValue updated and the existing model's CurrentValue carried through +// unchanged. Without this, the orchestrator's handleSessionModelsEvent never +// runs for non-model changes and reasoning_effort / thought_level updates +// would be lost on page refresh after a backend restart. +func TestEmitSetConfigOptionEvent_RewritesChangedOptionAndKeepsModel(t *testing.T) { + a := newTestAdapter() + cachedModels := []modelInfo{{ModelId: "gpt-5", Name: "GPT-5"}} + cachedConfig := []streams.ConfigOption{ + {Type: "select", ID: "model", Category: "model", Name: "Model", CurrentValue: "gpt-5"}, + {Type: "select", ID: "reasoning_effort", Name: "Reasoning", CurrentValue: "low"}, + } + + a.emitSetConfigOptionEvent("sess-1", "reasoning_effort", "high", cachedModels, cachedConfig) + + events := drainEvents(a) + if len(events) != 1 { + t.Fatalf("got %d events, want 1", len(events)) + } + ev := events[0] + if ev.Type != streams.EventTypeSessionModels { + t.Errorf("event Type = %q, want %q", ev.Type, streams.EventTypeSessionModels) + } + if ev.SessionID != "sess-1" { + t.Errorf("event SessionID = %q, want %q", ev.SessionID, "sess-1") + } + if ev.CurrentModelID != "gpt-5" { + t.Errorf("event CurrentModelID = %q, want %q (carried from existing model option)", ev.CurrentModelID, "gpt-5") + } + + got := map[string]string{} + for _, opt := range ev.ConfigOptions { + got[opt.ID] = opt.CurrentValue + } + if got["reasoning_effort"] != "high" { + t.Errorf("ConfigOptions[reasoning_effort] CurrentValue = %q, want %q", got["reasoning_effort"], "high") + } + if got["model"] != "gpt-5" { + t.Errorf("ConfigOptions[model] CurrentValue = %q, want %q (must not be reset)", got["model"], "gpt-5") + } + + // Cached config must not be mutated — emitSetConfigOptionEvent copies before rewriting. + if cachedConfig[1].CurrentValue != "low" { + t.Errorf("cachedConfig[reasoning_effort] mutated to %q; expected event-local copy only", cachedConfig[1].CurrentValue) + } +} + +// TestEmitSetConfigOptionEvent_SkipsWhenCacheEmpty pins that when the local +// config cache hasn't been seeded yet (e.g. the agent hasn't sent its first +// ConfigOptionUpdate), emitSetConfigOptionEvent returns without emitting any +// event rather than broadcasting a blank session_models frame that would +// temporarily clear the UI's model selector. The agent's own ConfigOptionUpdate +// remains the authoritative event for this path. +func TestEmitSetConfigOptionEvent_SkipsWhenCacheEmpty(t *testing.T) { + a := newTestAdapter() + + a.emitSetConfigOptionEvent("sess-1", "reasoning_effort", "high", nil, nil) + + events := drainEvents(a) + if len(events) != 0 { + t.Fatalf("expected 0 events when cachedConfig is empty; got %d", len(events)) + } +} diff --git a/apps/backend/internal/orchestrator/event_handlers_streaming.go b/apps/backend/internal/orchestrator/event_handlers_streaming.go index e0fd1932de..1e1448d0a3 100644 --- a/apps/backend/internal/orchestrator/event_handlers_streaming.go +++ b/apps/backend/internal/orchestrator/event_handlers_streaming.go @@ -1022,18 +1022,18 @@ func (s *Service) handleAgentCapabilitiesEvent(ctx context.Context, payload *lif } // handleSessionModelsEvent broadcasts session_models events to the WebSocket -// and persists the current model to the session snapshot so it survives page refresh. +// and persists the current model to the session snapshot so the model +// selector survives a page refresh without a flash. func (s *Service) handleSessionModelsEvent(ctx context.Context, payload *lifecycle.AgentStreamEventPayload) { sessionID := payload.SessionID if sessionID == "" || s.eventBus == nil { return } - // Persist the ACP-reported current model to the session snapshot so it's - // available after page refresh (SSR reads from the DB, not runtime state). - if currentModel := payload.Data.CurrentModelID; currentModel != "" { - s.persistSessionModel(ctx, sessionID, currentModel) - } + // Persist the agent-reported current model so SSR can render the model + // selector trigger with the right value on a page reload instead of + // flashing the profile default before the WS catches up. + s.persistSessionModel(ctx, sessionID, payload.Data.CurrentModelID) eventPayload := lifecycle.SessionModelsEventPayload{ TaskID: payload.TaskID, @@ -1053,8 +1053,21 @@ func (s *Service) handleSessionModelsEvent(ctx context.Context, payload *lifecyc _ = s.eventBus.Publish(ctx, subject, bus.NewEvent(events.SessionModelsUpdated, "orchestrator", eventPayload)) } -// persistSessionModel updates the session's AgentProfileSnapshot with the current model. +// persistSessionModel writes the agent-reported current model to the session's +// AgentProfileSnapshot under the `model` key so SSR can render the model +// selector trigger with the right value on a page reload without a flash. +// +// We intentionally only persist the model (not the full set of dynamic config +// options) and intentionally do NOT replay this on backend-restart resume: +// agents that support session/load preserve the value themselves, and replay +// would issue redundant SetModel / SetConfigOption RPCs that cycle the session +// through STARTING / RUNNING and flicker the task into the sidebar's Running +// bucket (see session-resume-keeps-review-state.spec.ts and +// effectiveSessionMode's sibling note in lifecycle/manager_profile.go). func (s *Service) persistSessionModel(ctx context.Context, sessionID, model string) { + if model == "" { + return + } session, err := s.repo.GetTaskSession(ctx, sessionID) if err != nil { return @@ -1062,8 +1075,7 @@ func (s *Service) persistSessionModel(ctx context.Context, sessionID, model stri if session.AgentProfileSnapshot == nil { session.AgentProfileSnapshot = make(map[string]interface{}) } - existing, _ := session.AgentProfileSnapshot["model"].(string) - if existing == model { + if existing, _ := session.AgentProfileSnapshot["model"].(string); existing == model { return } session.AgentProfileSnapshot["model"] = model diff --git a/apps/backend/internal/orchestrator/event_handlers_streaming_test.go b/apps/backend/internal/orchestrator/event_handlers_streaming_test.go index 297fb92bfb..0bf5ca5ff9 100644 --- a/apps/backend/internal/orchestrator/event_handlers_streaming_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_streaming_test.go @@ -459,3 +459,35 @@ func TestSessionStateString(t *testing.T) { require.Equal(t, string(models.TaskSessionStateRunning), sessionStateString(&models.TaskSession{State: models.TaskSessionStateRunning})) } + +// TestPersistSessionModel pins the SSR-side behaviour of the session_models +// event handler: a non-empty agent-reported model is written to +// AgentProfileSnapshot["model"] so the model selector trigger doesn't flash +// the profile default on a page reload before WS state catches up. +func TestPersistSessionModel(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + svc := &Service{logger: testLogger(), repo: repo} + + svc.persistSessionModel(ctx, "s1", "gpt-5.4") + + updated, err := repo.GetTaskSession(ctx, "s1") + require.NoError(t, err) + require.Equal(t, "gpt-5.4", updated.AgentProfileSnapshot["model"]) + + // A no-op write must not touch the DB row, but the visible behaviour is + // the same: the snapshot still carries the previously-set value. + svc.persistSessionModel(ctx, "s1", "gpt-5.4") + again, err := repo.GetTaskSession(ctx, "s1") + require.NoError(t, err) + require.Equal(t, "gpt-5.4", again.AgentProfileSnapshot["model"]) + + // An empty model is a no-op (some agents emit session_models without a + // CurrentModelID before the first ConfigOptionUpdate). The previously + // persisted value must not be cleared. + svc.persistSessionModel(ctx, "s1", "") + preserved, err := repo.GetTaskSession(ctx, "s1") + require.NoError(t, err) + require.Equal(t, "gpt-5.4", preserved.AgentProfileSnapshot["model"]) +} diff --git a/apps/web/components/model-config-selector.tsx b/apps/web/components/model-config-selector.tsx index 33386f8a7b..32a28ad578 100644 --- a/apps/web/components/model-config-selector.tsx +++ b/apps/web/components/model-config-selector.tsx @@ -152,7 +152,7 @@ function ConfigOptionSection({ onChange?: (configId: string, value: string) => void; }) { return ( -