fix(acp): persist session model and drop composite id logic (#1327)

This commit is contained in:
Carlos Florêncio
2026-06-11 11:07:22 +01:00
committed by GitHub
parent b0482285aa
commit 00500cca05
11 changed files with 543 additions and 439 deletions
@@ -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)
@@ -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.
@@ -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()
@@ -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,
}
@@ -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")
}
}
@@ -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))
}
}
@@ -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
@@ -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"])
}
@@ -152,7 +152,7 @@ function ConfigOptionSection({
onChange?: (configId: string, value: string) => void;
}) {
return (
<div className="space-y-1.5">
<div className="space-y-1.5" data-testid={`config-option-section-${option.id}`}>
<div className="text-[0.6875rem] font-medium uppercase tracking-wide text-muted-foreground">
{option.name}
</div>
@@ -143,3 +143,113 @@ test.describe("Chat model selector — RPC failure", () => {
await expect(trigger).toContainText("Mock Fast", { timeout: 5_000 });
});
});
/**
* Verifies that a model change selected via the chat-input model selector
* survives a full page reload. The regression this guards against: the
* SetConfigOption RPC path used by mock-agent did not emit the session_models
* convergence event, so the orchestrator never persisted the new model to the
* session snapshot and SSR re-served the pre-change model on reload.
*/
test.describe("Chat model selector — persistence", () => {
test("model change persists across page reload", async ({ testPage, apiClient, seedData }) => {
const task = await apiClient.createTaskWithAgent(
seedData.workspaceId,
"Model Selector Persistence Test",
seedData.agentProfileId,
{
description: "/e2e:simple-message",
workflow_id: seedData.workflowId,
workflow_step_id: seedData.startStepId,
repository_ids: [seedData.repositoryId],
},
);
await testPage.goto(`/t/${task.id}`);
const session = new SessionPage(testPage);
await session.waitForLoad();
await expect(session.idleInput()).toBeVisible({ timeout: 30_000 });
const trigger = testPage.getByRole("button", { name: "Session model settings" });
await expect(trigger).toContainText("Mock Fast", { timeout: 15_000 });
// Switch from Mock Fast to Mock Smart. mock-agent routes model changes
// through POST /set-config-option — the same path the persistence bug
// lived on (SetConfigOption did not emit the session_models convergence
// event, so the orchestrator never wrote the new model to the DB).
await trigger.click();
await testPage.getByRole("option", { name: /Mock Smart/ }).click();
await expect(trigger).toContainText("Mock Smart", { timeout: 5_000 });
await testPage.reload();
await session.waitForLoad();
await expect(session.idleInput()).toBeVisible({ timeout: 30_000 });
// After reload the trigger label comes from SSR-hydrated session state.
// If SetConfigOption fails to emit the convergence event, the orchestrator
// never persists the change and SSR re-serves "Mock Fast" — this assertion
// is the regression guard.
await expect(trigger).toContainText("Mock Smart", { timeout: 15_000 });
await expect(trigger).not.toContainText("Mock Fast");
});
});
/**
* Verifies the chat-input model selector's popover stays open after picking a
* model when extra config options (reasoning effort, thought level, …) are
* present on the same popover. Picking a model is rarely the last thing a user
* wants to do when there are other knobs in the same surface — they typically
* want to adjust effort too. Auto-closing on model select forces a second open.
*
* mock-agent ships with model + effort config options, so the popover renders
* the Effort section alongside the model list — exactly the layout this guard
* protects.
*/
test.describe("Chat model selector — popover open/close behavior", () => {
test("stays open after picking a model when extra config options exist", async ({
testPage,
apiClient,
seedData,
}) => {
const task = await apiClient.createTaskWithAgent(
seedData.workspaceId,
"Model Selector Stay Open Test",
seedData.agentProfileId,
{
description: "/e2e:simple-message",
workflow_id: seedData.workflowId,
workflow_step_id: seedData.startStepId,
repository_ids: [seedData.repositoryId],
},
);
await testPage.goto(`/t/${task.id}`);
const session = new SessionPage(testPage);
await session.waitForLoad();
await expect(session.idleInput()).toBeVisible({ timeout: 30_000 });
const trigger = testPage.getByRole("button", { name: "Session model settings" });
await expect(trigger).toContainText("Mock Fast", { timeout: 15_000 });
await trigger.click();
// The effort config section is the rendered marker that the popover
// content is mounted — it sits below the model list inside the same
// PopoverContent. Selecting by testid mirrors the suite-wide convention
// (apps/web/e2e/README.md).
const effortSection = testPage.getByTestId("config-option-section-effort");
await expect(effortSection).toBeVisible({ timeout: 5_000 });
await testPage.getByRole("option", { name: /Mock Smart/ }).click();
// The trigger label updates optimistically — wait for that so we know the
// selection round-tripped through the handler.
await expect(trigger).toContainText("Mock Smart", { timeout: 5_000 });
// Popover must still be open so the user can also pick an effort level
// without re-opening. The effort section is rendered only when the
// PopoverContent is mounted (Radix unmounts it on close).
await expect(effortSection).toBeVisible();
});
});
+51 -16
View File
@@ -58,6 +58,12 @@ type BuildSessionPageStateParams = {
agents: Awaited<ReturnType<typeof listAgents>>;
repositories: Awaited<ReturnType<typeof listRepositories>>["repositories"];
allSessions: TaskSession[];
// Full session payload (with agent_profile_snapshot) for the active sessionId,
// when available. The list endpoint returns lightweight summaries without the
// snapshot, which would force the model selector to fall back to the agent's
// default model on SSR — visible as a brief flash of the wrong model before
// the WS-driven cached state arrives.
activeSession: TaskSession | null;
workspaces: Awaited<ReturnType<typeof listWorkspaces>>["workspaces"];
workflows: Awaited<ReturnType<typeof listWorkflows>>["workflows"];
turns: Awaited<ReturnType<typeof listSessionTurns>>["turns"];
@@ -139,9 +145,16 @@ function buildResourceState(p: BuildSessionPageStateParams) {
}
function buildSessionState(p: BuildSessionPageStateParams) {
const { task, sessionId, allSessions, turns } = p;
const { task, sessionId, allSessions, activeSession, turns } = p;
// Prefer the full active session payload (with agent_profile_snapshot) over
// its summary entry in allSessions so the model selector can resolve the
// persisted model on first render without flashing the agent default.
const itemsBySessionId = Object.fromEntries(allSessions.map((s) => [s.id, s]));
if (activeSession?.id) {
itemsBySessionId[activeSession.id] = activeSession;
}
return {
taskSessions: { items: Object.fromEntries(allSessions.map((s) => [s.id, s])) },
taskSessions: { items: itemsBySessionId },
taskSessionsByTask: {
itemsByTaskId: { [task.id]: allSessions },
loadingByTaskId: { [task.id]: false },
@@ -181,23 +194,23 @@ export type FetchedSessionData = {
};
export async function fetchSessionData(sessionId: string): Promise<FetchedSessionData> {
const [allSessionsResponse, task] = await (async () => {
// We need task + sessions; caller provides sessionId
const { fetchTaskSession } = await import("@/lib/api");
const sessionResponse = await fetchTaskSession(sessionId, { cache: "no-store" });
const session = sessionResponse.session;
if (!session?.task_id) throw new Error("No task_id found for session");
const t = await fetchTask(session.task_id, { cache: "no-store" });
const sessResp = await listTaskSessions(session.task_id, { cache: "no-store" });
return [sessResp, t] as const;
})();
const { fetchTaskSession } = await import("@/lib/api");
const sessionResponse = await fetchTaskSession(sessionId, { cache: "no-store" });
const activeSession = sessionResponse.session ?? null;
if (!activeSession?.task_id) throw new Error("No task_id found for session");
const [task, allSessionsResponse] = await Promise.all([
fetchTask(activeSession.task_id, { cache: "no-store" }),
listTaskSessions(activeSession.task_id, { cache: "no-store" }),
]);
return fetchSessionDataFromTask(task, sessionId, allSessionsResponse);
return fetchSessionDataFromTask(task, sessionId, allSessionsResponse, activeSession);
}
export async function fetchSessionDataForTask(taskId: string): Promise<FetchedSessionData> {
const task = await fetchTask(taskId, { cache: "no-store" });
const allSessionsResponse = await listTaskSessions(taskId, { cache: "no-store" });
const [task, allSessionsResponse] = await Promise.all([
fetchTask(taskId, { cache: "no-store" }),
listTaskSessions(taskId, { cache: "no-store" }),
]);
const sessions = allSessionsResponse.sessions ?? [];
const sessionId = task.primary_session_id ?? sessions[0]?.id;
@@ -207,7 +220,26 @@ export async function fetchSessionDataForTask(taskId: string): Promise<FetchedSe
return fetchTaskDataOnly(task, allSessionsResponse);
}
return fetchSessionDataFromTask(task, sessionId, allSessionsResponse);
// Refetch the active session via the single-session endpoint to get
// agent_profile_snapshot, which the list endpoint strips. See
// BuildSessionPageStateParams.activeSession for the SSR-flicker rationale.
// A failure here (auth, 5xx, timeout) degrades gracefully to the original
// initial-flash behaviour but should surface in server logs so silent
// 401/500s are debuggable.
const { fetchTaskSession } = await import("@/lib/api");
const sessionResponse = await fetchTaskSession(sessionId, { cache: "no-store" }).catch((e) => {
console.warn(
"[session-page-state] failed to fetch active session snapshot; SSR will fall back to summary entry",
e,
);
return null;
});
return fetchSessionDataFromTask(
task,
sessionId,
allSessionsResponse,
sessionResponse?.session ?? null,
);
}
async function fetchTaskDataOnly(
@@ -246,6 +278,7 @@ async function fetchTaskDataOnly(
agents,
repositories,
allSessions,
activeSession: null,
workspaces,
workflows,
turns: [],
@@ -316,6 +349,7 @@ async function fetchSessionDataFromTask(
task: Task,
sessionId: string,
allSessionsResponse: Awaited<ReturnType<typeof listTaskSessions>>,
activeSession: TaskSession | null,
): Promise<FetchedSessionData> {
// User shells are env-scoped — look up this session's task_environment_id
// from the already-fetched session list. Sessions w/o env (legacy) skip
@@ -369,6 +403,7 @@ async function fetchSessionDataFromTask(
agents,
repositories,
allSessions,
activeSession,
workspaces,
workflows,
turns,