diff --git a/apps/backend/cmd/mock-agent/main.go b/apps/backend/cmd/mock-agent/main.go index ae4f5ca916..5e14e912ff 100644 --- a/apps/backend/cmd/mock-agent/main.go +++ b/apps/backend/cmd/mock-agent/main.go @@ -85,10 +85,11 @@ func (a *mockAgent) Initialize(_ context.Context, _ acp.InitializeRequest) (acp. // NewSession creates a new conversation session. // MCP servers from the ACP request are registered so callMCPTool can use them. -// The Models and Modes fields advertise available capabilities so the host -// utility capability probe can populate them in the cache — this is what makes -// the utility-agents settings page show model and mode options for mock-agent -// in E2E, and lets profile-mode tests select a non-default mode. +// The Modes field and the model-shaped entry in ConfigOptions advertise +// available capabilities so the host utility capability probe can populate +// them in the cache — this is what makes the utility-agents settings page +// show model and mode options for mock-agent in E2E, and lets profile-mode +// tests select a non-default mode. func (a *mockAgent) NewSession(_ context.Context, req acp.NewSessionRequest) (acp.NewSessionResponse, error) { sid := acp.SessionId(fmt.Sprintf("mock-session-%d", os.Getpid())) a.mu.Lock() @@ -106,25 +107,15 @@ func (a *mockAgent) NewSession(_ context.Context, req acp.NewSessionRequest) (ac return acp.NewSessionResponse{ SessionId: sid, - Models: mockSessionModels(), Modes: mockSessionModes(), ConfigOptions: mockSessionConfigOptions(), }, nil } -// mockSessionModels returns the mock agent's advertised model list for ACP -// session responses. Two models are exposed so tests can verify both -// selection and default behavior (mock-fast is the default). -func mockSessionModels() *acp.SessionModelState { - fastDesc := "Fast mock model for testing" - smartDesc := "Smart mock model for testing" - return &acp.SessionModelState{ - CurrentModelId: "mock-fast", - AvailableModels: []acp.ModelInfo{ - {ModelId: "mock-fast", Name: "Mock Fast", Description: &fastDesc}, - {ModelId: "mock-smart", Name: "Mock Smart", Description: &smartDesc}, - }, - } +// Logout terminates the current authenticated session. The mock agent has no +// persistent auth state so this is a no-op. +func (a *mockAgent) Logout(_ context.Context, _ acp.LogoutRequest) (acp.LogoutResponse, error) { + return acp.LogoutResponse{}, nil } // mockSessionModes returns the mock agent's advertised session-mode list for diff --git a/apps/backend/go.mod b/apps/backend/go.mod index 20ab0865d8..ccbd5de885 100644 --- a/apps/backend/go.mod +++ b/apps/backend/go.mod @@ -5,7 +5,7 @@ go 1.26.0 require ( github.com/UserExistsError/conpty v0.1.4 github.com/bmatcuk/doublestar/v4 v4.10.0 - github.com/coder/acp-go-sdk v0.13.0 + github.com/coder/acp-go-sdk v0.13.5 github.com/creack/pty v1.1.21 github.com/docker/docker v28.5.2+incompatible github.com/docker/go-connections v0.6.0 @@ -115,3 +115,5 @@ require ( gopkg.in/ini.v1 v1.67.0 // indirect gotest.tools/v3 v3.5.2 // indirect ) + +replace github.com/coder/acp-go-sdk => github.com/kdlbs/acp-go-sdk v0.13.6-0.20260609161428-91d5e43d6599 diff --git a/apps/backend/go.sum b/apps/backend/go.sum index 077ec789d3..c21d8b82ee 100644 --- a/apps/backend/go.sum +++ b/apps/backend/go.sum @@ -72,8 +72,6 @@ github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDk github.com/cncf/udpa/go v0.0.0-20191209042840-269d4d468f6f/go.mod h1:M8M6+tZqaGXZJjfX53e64911xZQV5JYwmTeXPW+k8Sc= github.com/cncf/udpa/go v0.0.0-20200629203442-efcf912fb354/go.mod h1:WmhPx2Nbnhtbo57+VJT5O0JRkEi1Wbu0z5j0R8u5Hbk= github.com/cncf/udpa/go v0.0.0-20201120205902-5459f2c99403/go.mod h1:WmhPx2Nbnhtbo57+VJT5O0JRkEi1Wbu0z5j0R8u5Hbk= -github.com/coder/acp-go-sdk v0.13.0 h1:IAKBDIbe/iBfKAGikeIndzb8fowt4ioD+gCtSU4HwMA= -github.com/coder/acp-go-sdk v0.13.0/go.mod h1:yKzM/3R9uELp4+nBAwwtkS0aN1FOFjo11CNPy37yFko= github.com/containerd/errdefs v1.0.0 h1:tg5yIfIlQIrxYtu9ajqY42W3lpS19XqdxRQeEwYG8PI= github.com/containerd/errdefs v1.0.0/go.mod h1:+YBYIdtsnF4Iw6nWZhJcqGSg/dwvV7tyJ/kCkyJ2k+M= github.com/containerd/errdefs/pkg v0.3.0 h1:9IKJ06FvyNlexW690DXuQNx2KA2cUJXx151Xdx3ZPPE= @@ -219,6 +217,8 @@ github.com/json-iterator/go v1.1.12 h1:PV8peI4a0ysnczrg+LtxykD8LfKY9ML6u2jnxaEnr github.com/json-iterator/go v1.1.12/go.mod h1:e30LSqwooZae/UwlEbR2852Gd8hjQvJoHmT4TnhNGBo= github.com/jstemmer/go-junit-report v0.0.0-20190106144839-af01ea7f8024/go.mod h1:6v2b51hI/fHJwM22ozAgKL4VKDeJcHhJFhtBdhmNjmU= github.com/jstemmer/go-junit-report v0.9.1/go.mod h1:Brl9GWCQeLvo8nXZwPNNblvFj/XSXhF0NWZEnDohbsk= +github.com/kdlbs/acp-go-sdk v0.13.6-0.20260609161428-91d5e43d6599 h1:VeNo6BcOkY18Hj9HopG7/3t5PpD4ocmuxMIuGSobzLQ= +github.com/kdlbs/acp-go-sdk v0.13.6-0.20260609161428-91d5e43d6599/go.mod h1:yKzM/3R9uELp4+nBAwwtkS0aN1FOFjo11CNPy37yFko= github.com/kevinburke/ssh_config v1.6.0 h1:J1FBfmuVosPHf5GRdltRLhPJtJpTlMdKTBjRgTaQBFY= github.com/kevinburke/ssh_config v1.6.0/go.mod h1:q2RIzfka+BXARoNexmF9gkxEX7DmvbW9P4hIVx2Kg4M= github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck= diff --git a/apps/backend/internal/agent/runtime/lifecycle/session.go b/apps/backend/internal/agent/runtime/lifecycle/session.go index 59a544dd41..83a7e04969 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/session.go +++ b/apps/backend/internal/agent/runtime/lifecycle/session.go @@ -133,6 +133,19 @@ func (sm *SessionManager) createOrLoadSession( if err == nil { return sessionID, nil } + // If the underlying ACP connection is dead (peer disconnected, context + // cancelled), session/new on the same client will return the same + // transport error — falling back just emits a noisy duplicate failure + // and delays the FAILED transition. Short-circuit so the caller can + // rebuild the connection (next resume cycle gets a fresh agentctl + // instance and a fresh ACP connection). + if isTransportDeadErr(err) { + sm.logger.Warn("session/load failed at transport layer, not retrying with session/new", + zap.String("agent_type", agentConfig.ID()), + zap.String("existing_session_id", existingSessionID), + zap.String("reason", err.Error())) + return "", err + } // session/load can fail for reasons that don't justify aborting the // session: agent doesn't support the method (capability mismatch / // method not found), the upstream agent CLI no longer recognises the @@ -726,3 +739,27 @@ func isAgentStreamNotConnectedErr(err error) bool { } return strings.Contains(err.Error(), "agent stream not connected") } + +// isTransportDeadErr reports whether a session/load failure is caused by the +// underlying ACP connection being gone rather than an agent-side error. The +// coder/acp-go-sdk surfaces this as a JSON-RPC internal-error whose data map +// carries the canonical phrase "peer disconnected before response" (also +// emitted while waiting for pre-response notifications). The error reaches us +// as a string through the agentctl WS layer, so we match the phrase. +// "connection closed" is the SDK's own cause string emitted from +// shutdownReceive — pulling double duty as a fallback for paths where the +// peer-disconnected wrapping isn't applied. Canonical context cancellation +// errors short-circuit too: the caller's ctx going down means session/new +// retry will fail for the same reason, so treat it as transport-dead. +func isTransportDeadErr(err error) bool { + if err == nil { + return false + } + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return true + } + msg := err.Error() + return strings.Contains(msg, "peer disconnected") || + strings.Contains(msg, "connection closed") || + strings.Contains(msg, "notification queue overflow") +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/session_test.go b/apps/backend/internal/agent/runtime/lifecycle/session_test.go index 0d2088996a..e0e97482eb 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/session_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/session_test.go @@ -622,6 +622,74 @@ func TestIsMethodNotFoundErr(t *testing.T) { } } +func TestIsTransportDeadErr(t *testing.T) { + tests := []struct { + name string + err error + expected bool + }{ + { + name: "peer disconnected before response", + err: fmt.Errorf("session/load failed: %w", fmt.Errorf("peer disconnected before response")), + expected: true, + }, + { + name: "peer disconnected while waiting for pre-response notifications", + err: fmt.Errorf("peer disconnected while waiting for pre-response notifications"), + expected: true, + }, + { + name: "connection closed cause", + err: fmt.Errorf("load session failed: connection closed"), + expected: true, + }, + { + name: "notification queue overflow", + err: fmt.Errorf("load session failed: notification queue overflow"), + expected: true, + }, + { + name: "context canceled", + err: context.Canceled, + expected: true, + }, + { + name: "context deadline exceeded wrapped", + err: fmt.Errorf("load session failed: %w", context.DeadlineExceeded), + expected: true, + }, + { + name: "method not found is not transport-dead", + err: &acp.RequestError{Code: -32601, Message: "Method not found"}, + expected: false, + }, + { + name: "session unknown is not transport-dead", + err: &acp.RequestError{Code: -32002, Message: "Resource not found"}, + expected: false, + }, + { + name: "nil error", + err: nil, + expected: false, + }, + { + name: "unrelated error", + err: fmt.Errorf("some other failure"), + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := isTransportDeadErr(tt.err) + if got != tt.expected { + t.Errorf("isTransportDeadErr(%v) = %v, want %v", tt.err, got, tt.expected) + } + }) + } +} + func TestInitializeSession_LoadsExistingSession(t *testing.T) { mock := newMockAgentServer(t) defer mock.Close() 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 a106f81a9e..7754eaeb4a 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go @@ -7,6 +7,8 @@ import ( "fmt" "io" "log/slog" + "os" + "strconv" "sync" "time" @@ -60,11 +62,47 @@ const wakeupPromptTimeout = 30 * time.Minute // notifQueueCapacity sizes the buffered channel that feeds the update // worker. 4096 covers any realistic session/load replay burst (the failure // case that motivated this was ~5-10k notifications spread over several -// seconds, well within the worker's sub-microsecond drain rate). Set big -// enough that the SDK's 1024-slot upstream queue stays near-empty under -// burst, small enough that worst-case memory is bounded. +// seconds, well within the worker's sub-microsecond drain rate). This is +// the internal hand-off between the SDK's update handler and our worker; +// it sits in front of the larger acpNotifQueueDefault SDK inbound queue and +// can be smaller because the worker drains it well under SDK fill rate. const notifQueueCapacity = 4096 +// acpNotifQueueDefault is the per-connection capacity passed to the SDK's +// inbound notification queue. Larger than the SDK's own default (1024) so +// long session/load replays (auggie 500+ exchanges, claude with heavy tool +// use) don't overflow and tear the connection down. The SDK still bounds +// memory to (capacity * avg notification size). +const acpNotifQueueDefault = 16384 + +// acpNotifQueueMin / acpNotifQueueMax clamp KANDEV_ACP_NOTIF_QUEUE so a +// misconfigured value can't either re-introduce the overflow (too low) or +// blow the heap (too high). +const ( + acpNotifQueueMin = 1024 + acpNotifQueueMax = 131072 +) + +// acpNotifQueueCapacity returns the per-connection inbound notification queue +// capacity, honoring KANDEV_ACP_NOTIF_QUEUE when set and parseable. +func acpNotifQueueCapacity() int { + raw := os.Getenv("KANDEV_ACP_NOTIF_QUEUE") + if raw == "" { + return acpNotifQueueDefault + } + n, err := strconv.Atoi(raw) + if err != nil || n <= 0 { + return acpNotifQueueDefault + } + if n < acpNotifQueueMin { + return acpNotifQueueMin + } + if n > acpNotifQueueMax { + return acpNotifQueueMax + } + return n +} + // AgentInfo contains information about the connected agent. type AgentInfo struct { Name string `json:"name"` @@ -170,7 +208,7 @@ type Adapter struct { // Available models from the most recent session creation/load. // Used by SetModel to validate the requested model exists. - availableModels []acp.ModelInfo + availableModels []modelInfo // usageDelta tracks the running cumulative `usage_update.used` and // the most recent USD cost reported per session. codex-acp emits no @@ -310,9 +348,18 @@ func (a *Adapter) Initialize(ctx context.Context) error { acpclient.WithPermissionHandler(a.handlePermissionRequest), ) - // Create ACP SDK connection - a.acpConn = acp.NewClientSideConnection(a.acpClient, a.stdin, a.stdout) + // Create ACP SDK connection. Raise the inbound notification queue cap + // to acpNotifQueueDefault (well above the SDK's built-in default) so + // long session/load replays don't overflow and tear the connection + // down. The internal notifQueueCapacity channel sits in front of this + // queue and is drained by our update worker. Requires a coder/acp-go-sdk + // fork with WithMaxQueuedNotifications; see go.mod replace directive. + notifQueueCap := acpNotifQueueCapacity() + a.acpConn = acp.NewClientSideConnection(a.acpClient, a.stdin, a.stdout, + acp.WithMaxQueuedNotifications(notifQueueCap)) a.acpConn.SetLogger(slog.Default().With("component", "acp-conn")) + a.logger.Debug("ACP connection notification queue sized", + zap.Int("capacity", notifQueueCap)) // Perform ACP handshake - this exchanges capabilities with the agent ctx, span := shared.TraceProtocolRequest(ctx, shared.ProtocolACP, a.agentID, "initialize") 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 91885efa15..3687f20e6e 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 @@ -55,7 +55,12 @@ func (a *Adapter) NewSession(ctx context.Context, mcpServers []types.McpServer) a.mu.Lock() a.sessionID = string(resp.SessionId) sessionID := a.sessionID - initialModels := initialSessionModelState(resp.Models, resp.Meta, resp.ConfigOptions) + // Reset session-scoped model caches before computing the new session's + // state so a session without a model surface can't reuse the previous + // session's models / configOptions for validation in SetModel. + a.availableModels = nil + a.availableConfigOptions = nil + initialModels := initialSessionModelState(resp.Meta, resp.ConfigOptions, resp.LegacyModels) if initialModels != nil { a.availableModels = initialModels.AvailableModels } @@ -70,8 +75,7 @@ func (a *Adapter) NewSession(ctx context.Context, mcpServers []types.McpServer) a.emitInitialModeState(resp.Modes) } - // Emit session models if the agent returned model state, or if it exposes - // model selection only through configOptions. + // Emit session models when the session exposes a model-shaped config option. if initialModels != nil { a.emitSessionModels(sessionID, initialModels, resp.Meta, resp.ConfigOptions) } @@ -91,16 +95,32 @@ func (a *Adapter) NewSession(ctx context.Context, mcpServers []types.McpServer) return sessionID, nil } +// initialSessionModelState resolves the initial model state for a session. +// Returns nil when no model-shaped surface exists, signalling that the agent +// doesn't advertise model selection on this session. +// +// Precedence (in order): +// 1. Typed ConfigOptions list with category="model" (v0.13.4+ agents). +// 2. Pre-v0.13.5 top-level `models` field (e.g. auggie 0.29.x), exposed by the +// kdlbs fork as acp.LegacyModels. Reached even when configOptions carries +// non-model entries (e.g. `category="mode"`) so an agent that mixes typed +// mode options with a legacy models block still surfaces its models. +// 3. _meta-only ConfigOption stub for legacy agents that surface options +// under `_meta.configOptions` (returns an empty state so emitSessionModels +// still fires; the event's ConfigOptions list is filled from _meta there). func initialSessionModelState( - models *acp.SessionModelState, meta map[string]any, configOptions []acp.SessionConfigOption, -) *acp.SessionModelState { - if models != nil { - return models + legacy *acp.LegacyModels, +) *sessionModelState { + if state := modelsFromConfigOptions(configOptions); state != nil { + return state } - if hasModelConfigOption(sessionConfigOptions(meta, configOptions)) { - return &acp.SessionModelState{} + if state := modelsFromLegacy(legacy); state != nil { + return state + } + if hasModelConfigOption(extractConfigOptions(meta)) { + return &sessionModelState{} } return nil } @@ -299,7 +319,11 @@ func (a *Adapter) LoadSession(ctx context.Context, sessionID string, mcpServers a.mu.Lock() a.sessionID = sessionID - initialModels := initialSessionModelState(resp.Models, resp.Meta, resp.ConfigOptions) + // Reset session-scoped model caches so a load that lands on a session + // without a model surface can't reuse the previous session's data. + a.availableModels = nil + a.availableConfigOptions = nil + initialModels := initialSessionModelState(resp.Meta, resp.ConfigOptions, resp.LegacyModels) if initialModels != nil { a.availableModels = initialModels.AvailableModels } @@ -399,8 +423,8 @@ func (a *Adapter) emitInitialModeState(modes *acp.SessionModeState) { } // emitSessionModels emits a session_models event from the session response. -func (a *Adapter) emitSessionModels(sessionID string, models *acp.SessionModelState, meta map[string]any, acpConfigOptions []acp.SessionConfigOption) { - currentModelID := string(models.CurrentModelId) +func (a *Adapter) emitSessionModels(sessionID string, models *sessionModelState, meta map[string]any, acpConfigOptions []acp.SessionConfigOption) { + currentModelID := models.CurrentModelId // Prefer typed config options from the response; fall back to _meta // extraction for older agents. configOptions := sessionConfigOptions(meta, acpConfigOptions) @@ -448,7 +472,7 @@ func (a *Adapter) emitSessionModels(sessionID string, models *acp.SessionModelSt // — this prevents a downstream consumer that reads ConfigOptions[model] // .CurrentValue (codex-style agents surface the current model there) from // disagreeing with the CurrentModelID emitted on the same event. -func (a *Adapter) emitSetModelEvent(sessionID, modelID string, cachedModels []acp.ModelInfo, cachedConfig []streams.ConfigOption) { +func (a *Adapter) emitSetModelEvent(sessionID, modelID string, cachedModels []modelInfo, cachedConfig []streams.ConfigOption) { outConfig := cachedConfig if len(cachedConfig) > 0 { // Shallow copy: only CurrentValue (a string) is rewritten below, so @@ -488,7 +512,7 @@ func (a *Adapter) emitSetModelEvent(sessionID, modelID string, cachedModels []ac } // resolveCurrentModelFromConfig extracts current model ID from configOptions. -func resolveCurrentModelFromConfig(options []streams.ConfigOption, available []acp.ModelInfo) string { +func resolveCurrentModelFromConfig(options []streams.ConfigOption, available []modelInfo) string { modelID := "" reasoningEffort := "" for _, opt := range options { @@ -554,12 +578,12 @@ func splitReasoningModelID(modelID string, options []streams.ConfigOption) (stri return baseModelID, reasoningEffort, true } -func modelIDExists(modelID string, available []acp.ModelInfo) bool { +func modelIDExists(modelID string, available []modelInfo) bool { if len(available) == 0 { return false } for _, model := range available { - if string(model.ModelId) == modelID { + if model.ModelId == modelID { return true } } @@ -619,7 +643,7 @@ func (a *Adapter) SetModel(ctx context.Context, modelID string) error { if len(available) > 0 { found := false for _, m := range available { - if string(m.ModelId) == modelID { + if m.ModelId == modelID { found = true break } @@ -636,9 +660,26 @@ func (a *Adapter) SetModel(ctx context.Context, modelID string) error { if err != nil { return fmt.Errorf("set session model failed via %s: %w", method, err) } + a.finalizeSetModel(method, sessionID, modelID, available, cachedConfig) + return nil +} + +// finalizeSetModel emits the post-apply convergence event when applySessionModel +// actually performed a switch. MethodNone means the agent supports neither the +// typed session/set_config_option nor the legacy session/set_model RPC, so no +// switch happened — skip the reset and the emit to avoid lying to the frontend. +func (a *Adapter) finalizeSetModel( + method sessionmodel.Method, + sessionID string, + modelID string, + available []modelInfo, + cachedConfig []streams.ConfigOption, +) { + if method == sessionmodel.MethodNone { + return + } a.resetContextWindowMaxSize(sessionID) a.emitSetModelEvent(sessionID, modelID, available, cachedConfig) - return nil } func applySessionModel( diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/load_burst_test.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/load_burst_test.go index f359ed14a3..438ce8d15a 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/load_burst_test.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/load_burst_test.go @@ -33,6 +33,10 @@ func (burstAgent) ListSessions(context.Context, acp.ListSessionsRequest) (acp.Li return acp.ListSessionsResponse{}, nil } +func (burstAgent) Logout(context.Context, acp.LogoutRequest) (acp.LogoutResponse, error) { + return acp.LogoutResponse{}, nil +} + func (burstAgent) NewSession(context.Context, acp.NewSessionRequest) (acp.NewSessionResponse, error) { return acp.NewSessionResponse{}, nil } diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/meta_convert.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/meta_convert.go index 614d90abed..7374eca648 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/meta_convert.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/meta_convert.go @@ -68,16 +68,16 @@ func extractTerminalAuth(meta map[string]any) *streams.TerminalAuth { return ta } -// convertSessionModels converts ACP model info to stream types, +// convertSessionModels converts kandev-local model info to stream types, // normalizing known _meta patterns (copilotUsage) while preserving raw _meta. -func convertSessionModels(models []acp.ModelInfo) []streams.SessionModelInfo { +func convertSessionModels(models []modelInfo) []streams.SessionModelInfo { if len(models) == 0 { return nil } result := make([]streams.SessionModelInfo, 0, len(models)) for _, m := range models { info := streams.SessionModelInfo{ - ModelID: string(m.ModelId), + ModelID: m.ModelId, Name: m.Name, Description: derefStr(m.Description), Meta: toStringMap(m.Meta), diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/model_apply_test.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/model_apply_test.go index fc0f326f89..8db8f5f3ab 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/model_apply_test.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/model_apply_test.go @@ -2,6 +2,7 @@ package acp import ( "context" + "errors" "reflect" "testing" @@ -11,7 +12,7 @@ import ( type fakeModelApplier struct { configErr error - modelErr error + legacyErr error calls []string } @@ -21,8 +22,8 @@ func (f *fakeModelApplier) SetSessionConfigOption(_ context.Context, req sdk.Set } func (f *fakeModelApplier) UnstableSetSessionModel(_ context.Context, req sdk.UnstableSetSessionModelRequest) (sdk.UnstableSetSessionModelResponse, error) { - f.calls = append(f.calls, "model:"+string(req.ModelId)) - return sdk.UnstableSetSessionModelResponse{}, f.modelErr + f.calls = append(f.calls, "legacy:"+string(req.SessionId)+":"+req.ModelId) + return sdk.UnstableSetSessionModelResponse{}, f.legacyErr } func TestApplySessionModel_UsesConfigOptionForModelConfig(t *testing.T) { @@ -46,7 +47,33 @@ func TestApplySessionModel_UsesConfigOptionForModelConfig(t *testing.T) { } } -func TestApplySessionModel_FallsBackToSetModelWhenSetConfigOptionMissing(t *testing.T) { +// TestApplySessionModel_PropagatesSetConfigOptionError pins that non +// MethodNotFound errors from the underlying SetSessionConfigOption RPC bubble +// up unchanged when the session advertises a model-shaped config option. +func TestApplySessionModel_PropagatesSetConfigOptionError(t *testing.T) { + t.Parallel() + + boom := errors.New("boom") + conn := &fakeModelApplier{configErr: boom} + _, err := applySessionModel(context.Background(), conn, "sess-1", "claude-opus-4-8", []streams.ConfigOption{{ + ID: "model", + Category: "model", + }}) + + if !errors.Is(err, boom) { + t.Fatalf("applySessionModel() error = %v, want boom", err) + } + wantCalls := []string{"config:model:claude-opus-4-8"} + if !reflect.DeepEqual(conn.calls, wantCalls) { + t.Fatalf("calls = %#v, want %#v", conn.calls, wantCalls) + } +} + +// TestApplySessionModel_SetConfigOptionMethodNotFoundFallsBack pins the +// partial-migration case: when the agent advertises the typed model-shaped +// config option but hasn't wired up the session/set_config_option handler, +// applySessionModel falls through to the legacy session/set_model RPC. +func TestApplySessionModel_SetConfigOptionMethodNotFoundFallsBack(t *testing.T) { t.Parallel() conn := &fakeModelApplier{configErr: sdk.NewMethodNotFound(sdk.AgentMethodSessionSetConfigOption)} @@ -55,13 +82,35 @@ func TestApplySessionModel_FallsBackToSetModelWhenSetConfigOptionMissing(t *test Category: "model", }}) + if err != nil { + t.Fatalf("applySessionModel() error = %v, want nil after legacy fallback", err) + } + if method != "session/set_model" { + t.Fatalf("method = %q, want session/set_model", method) + } + wantCalls := []string{"config:model:claude-opus-4-8", "legacy:sess-1:claude-opus-4-8"} + if !reflect.DeepEqual(conn.calls, wantCalls) { + t.Fatalf("calls = %#v, want %#v", conn.calls, wantCalls) + } +} + +// TestApplySessionModel_FallsBackToLegacySetModel pins the SDK-side fallback +// path: when the session has no model-shaped config option (e.g. auggie 0.29.x +// which surfaces models via the legacy top-level `models` field), the +// dispatcher uses the kdlbs-fork-restored session/set_model RPC instead. +func TestApplySessionModel_FallsBackToLegacySetModel(t *testing.T) { + t.Parallel() + + conn := &fakeModelApplier{} + method, err := applySessionModel(context.Background(), conn, "sess-1", "claude-opus-4-7", nil) + if err != nil { t.Fatalf("applySessionModel() error = %v", err) } if method != "session/set_model" { t.Fatalf("method = %q, want session/set_model", method) } - wantCalls := []string{"config:model:claude-opus-4-8", "model:claude-opus-4-8"} + wantCalls := []string{"legacy:sess-1:claude-opus-4-7"} if !reflect.DeepEqual(conn.calls, wantCalls) { t.Fatalf("calls = %#v, want %#v", conn.calls, wantCalls) } diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/model_types.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/model_types.go new file mode 100644 index 0000000000..9b3a94728e --- /dev/null +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/model_types.go @@ -0,0 +1,108 @@ +package acp + +import ( + acp "github.com/coder/acp-go-sdk" +) + +// modelInfo is the kandev-internal representation of a single model advertised +// by an ACP session. v0.13.0 of the SDK exposed an equivalent acp.ModelInfo +// type; v0.13.5 removed it in favor of routing model selection through +// SessionConfigOption with category="model". We keep the local shape so the +// rest of the adapter (caching, convergence events, fallback resolution) +// doesn't have to spread the configOption walk across every call site. +type modelInfo struct { + ModelId string + Name string + Description *string + Meta map[string]any +} + +// sessionModelState mirrors the removed v0.13.0 acp.SessionModelState shape. +// Populated by initialSessionModelState from the session response's typed +// SessionConfigOption list (with _meta fallback for legacy agents). +type sessionModelState struct { + CurrentModelId string + AvailableModels []modelInfo +} + +// modelsFromConfigOptions extracts the current model id and the available +// model list from a typed SessionConfigOption slice, looking for the option +// whose Select.Category is "model". Returns nil when no such option exists. +func modelsFromConfigOptions(opts []acp.SessionConfigOption) *sessionModelState { + for _, opt := range opts { + if opt.Select == nil { + continue + } + sel := opt.Select + isModel := string(sel.Id) == configOptionIDModel || + (sel.Category != nil && string(*sel.Category) == configOptionIDModel) + if !isModel { + continue + } + state := &sessionModelState{ + CurrentModelId: string(sel.CurrentValue), + AvailableModels: modelsFromSelectOptions(sel.Options), + } + return state + } + return nil +} + +// modelsFromSelectOptions flattens the ungrouped/grouped variants of a +// SessionConfigSelectOptions payload into a flat []modelInfo. Grouped options +// are walked in declaration order so the resulting slice preserves the agent's +// presentation order. +func modelsFromSelectOptions(opts acp.SessionConfigSelectOptions) []modelInfo { + if opts.Ungrouped != nil { + return convertSelectOptionList(*opts.Ungrouped) + } + if opts.Grouped != nil { + var out []modelInfo + for _, group := range *opts.Grouped { + out = append(out, convertSelectOptionList(group.Options)...) + } + return out + } + return nil +} + +// modelsFromLegacy extracts the session model state from the pre-v0.13.5 +// top-level `models` field still emitted by agents (e.g. auggie 0.29.x) that +// haven't migrated to the SessionConfigOption(category="model") surface. +// Upstream removed this field from the SDK struct in v0.13.5; the kdlbs fork +// restores read-only parsing via acp.LegacyModels. Returns nil when the field +// is absent or carries no models. +func modelsFromLegacy(legacy *acp.LegacyModels) *sessionModelState { + if legacy == nil || len(legacy.AvailableModels) == 0 { + return nil + } + out := make([]modelInfo, 0, len(legacy.AvailableModels)) + for _, m := range legacy.AvailableModels { + out = append(out, modelInfo{ + ModelId: m.ModelId, + Name: m.Name, + Description: m.Description, + Meta: m.Meta, + }) + } + return &sessionModelState{ + CurrentModelId: legacy.CurrentModelId, + AvailableModels: out, + } +} + +func convertSelectOptionList(in []acp.SessionConfigSelectOption) []modelInfo { + if len(in) == 0 { + return nil + } + out := make([]modelInfo, 0, len(in)) + for _, o := range in { + out = append(out, modelInfo{ + ModelId: string(o.Value), + Name: o.Name, + Description: o.Description, + Meta: o.Meta, + }) + } + return out +} 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 03f0408e80..2b20dac73f 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 @@ -4,6 +4,7 @@ import ( "testing" "github.com/coder/acp-go-sdk" + "github.com/kandev/kandev/internal/agentctl/sessionmodel" "github.com/kandev/kandev/internal/agentctl/types/streams" ) @@ -22,10 +23,10 @@ func findSessionModelsEvent(t *testing.T, events []AgentEvent) AgentEvent { // auggieLikeModels mimics Auggie's response: empty CurrentModelId with an // alphabetically-sorted list whose [0] is a pseudo-agent ("Build Analyzer"). -func auggieLikeModels() *acp.SessionModelState { - return &acp.SessionModelState{ +func auggieLikeModels() *sessionModelState { + return &sessionModelState{ CurrentModelId: "", - AvailableModels: []acp.ModelInfo{ + AvailableModels: []modelInfo{ {ModelId: "build-fix-gpt5-2-responses-high-200k-v1-c4-p2-agent", Name: "Build Analyzer"}, {ModelId: "claude-opus-4-7", Name: "Opus 4.7"}, }, @@ -79,9 +80,9 @@ func TestEmitSessionModels_EmptyCurrentIDFromConfigOption(t *testing.T) { // selectable ID "gpt-5.5/medium". func TestEmitSessionModels_EmptyCurrentIDComposesReasoningEffort(t *testing.T) { a := newTestAdapter() - models := &acp.SessionModelState{ + models := &sessionModelState{ CurrentModelId: "", - AvailableModels: []acp.ModelInfo{ + AvailableModels: []modelInfo{ {ModelId: "gpt-5.5/low", Name: "GPT-5.5 (low)"}, {ModelId: "gpt-5.5/medium", Name: "GPT-5.5 (medium)"}, }, @@ -124,9 +125,9 @@ func TestEmitSessionModels_EmptyCurrentIDComposesReasoningEffortFromTypedOptions {Name: "Low", Value: reasoningEffortLow}, {Name: "Medium", Value: reasoningEffortMedium}, } - models := &acp.SessionModelState{ + models := &sessionModelState{ CurrentModelId: "", - AvailableModels: []acp.ModelInfo{ + AvailableModels: []modelInfo{ {ModelId: "gpt-5.5/low", Name: "GPT-5.5 (low)"}, {ModelId: "gpt-5.5/medium", Name: "GPT-5.5 (medium)"}, }, @@ -175,7 +176,7 @@ func TestInitialSessionModelState_UsesConfigOptionsWithoutModels(t *testing.T) { }}, } - models := initialSessionModelState(nil, nil, configOptions) + models := initialSessionModelState(nil, configOptions, nil) if models == nil { t.Fatal("initialSessionModelState returned nil for configOptions-only response") } @@ -208,7 +209,7 @@ func TestInitialSessionModelState_UsesMetaConfigOptionsWithoutModels(t *testing. }, } - models := initialSessionModelState(nil, meta, nil) + models := initialSessionModelState(meta, nil, nil) if models == nil { t.Fatal("initialSessionModelState returned nil for meta configOptions-only response") } @@ -245,12 +246,53 @@ func TestInitialSessionModelState_IgnoresNonModelConfigOptions(t *testing.T) { }}, } - if models := initialSessionModelState(nil, nil, configOptions); models != nil { + if models := initialSessionModelState(nil, configOptions, nil); models != nil { t.Fatalf("initialSessionModelState returned %+v for non-model configOptions", models) } } -func TestInitialSessionModelState_UsesTypedConfigOptionPrecedence(t *testing.T) { +// TestInitialSessionModelState_NonModelConfigOptionsFallThroughToLegacy +// pins that an agent emitting typed configOptions[category="mode"] alongside +// the legacy top-level `models` field still surfaces its models — the typed +// non-model option must not block the LegacyModels tier. +func TestInitialSessionModelState_NonModelConfigOptionsFallThroughToLegacy(t *testing.T) { + modeCategory := acp.SessionConfigOptionCategoryMode + modeOptions := acp.SessionConfigSelectOptionsUngrouped{ + {Name: "Read Only", Value: "read-only"}, + } + configOptions := []acp.SessionConfigOption{ + {Select: &acp.SessionConfigOptionSelect{ + Type: "select", + Id: "mode", + Name: "Approval Preset", + Category: &modeCategory, + CurrentValue: "read-only", + Options: acp.SessionConfigSelectOptions{Ungrouped: &modeOptions}, + }}, + } + legacy := &acp.LegacyModels{ + CurrentModelId: "claude-opus-4-7", + AvailableModels: []acp.LegacyModelInfo{ + {ModelId: "claude-opus-4-7", Name: "Opus 4.7"}, + }, + } + + models := initialSessionModelState(nil, configOptions, legacy) + if models == nil { + t.Fatal("initialSessionModelState returned nil; expected legacy fallback") + } + if models.CurrentModelId != "claude-opus-4-7" { + t.Errorf("CurrentModelId = %q, want claude-opus-4-7", models.CurrentModelId) + } + if len(models.AvailableModels) != 1 { + t.Fatalf("AvailableModels = %d, want 1", len(models.AvailableModels)) + } +} + +// TestInitialSessionModelState_NonModelConfigOptionsAllowMetaFallback pins +// that the _meta tier-3 fallback still fires when typed configOptions has +// only non-model entries and no LegacyModels are present. +func TestInitialSessionModelState_NonModelConfigOptionsAllowMetaFallback(t *testing.T) { modeCategory := acp.SessionConfigOptionCategoryMode modeOptions := acp.SessionConfigSelectOptionsUngrouped{ {Name: "Read Only", Value: "read-only"}, @@ -277,8 +319,82 @@ func TestInitialSessionModelState_UsesTypedConfigOptionPrecedence(t *testing.T) }, } - if models := initialSessionModelState(nil, meta, configOptions); models != nil { - t.Fatalf("initialSessionModelState returned %+v for non-model typed configOptions", models) + models := initialSessionModelState(meta, configOptions, nil) + if models == nil { + t.Fatal("initialSessionModelState returned nil; expected _meta fallback stub") + } + if len(models.AvailableModels) != 0 { + t.Errorf("AvailableModels = %d, want 0 (stub state)", len(models.AvailableModels)) + } +} + +func TestInitialSessionModelState_FallsBackToLegacyModels(t *testing.T) { + // auggie 0.29.x emits the pre-v0.13.5 top-level `models` field and does + // NOT surface a SessionConfigOption(category="model"). The kdlbs SDK fork + // restores read-only parsing as acp.LegacyModels; the third-tier fallback + // in initialSessionModelState must turn that into a fully populated state. + desc := "Anthropic Claude Opus 4.7" + legacy := &acp.LegacyModels{ + CurrentModelId: "claude-opus-4-7", + AvailableModels: []acp.LegacyModelInfo{ + {ModelId: "claude-opus-4-7", Name: "Opus 4.7", Description: &desc}, + {ModelId: "claude-sonnet-4-5", Name: "Sonnet 4.5"}, + }, + } + + models := initialSessionModelState(nil, nil, legacy) + if models == nil { + t.Fatal("initialSessionModelState returned nil for legacy-models-only response") + } + if models.CurrentModelId != "claude-opus-4-7" { + t.Errorf("CurrentModelId = %q, want claude-opus-4-7", models.CurrentModelId) + } + if len(models.AvailableModels) != 2 { + t.Fatalf("AvailableModels len = %d, want 2", len(models.AvailableModels)) + } + if models.AvailableModels[0].Name != "Opus 4.7" { + t.Errorf("AvailableModels[0].Name = %q, want Opus 4.7", models.AvailableModels[0].Name) + } + + a := newTestAdapter() + a.emitSessionModels("sess-1", models, nil, nil) + ev := findSessionModelsEvent(t, drainEvents(a)) + if ev.CurrentModelID != "claude-opus-4-7" { + t.Errorf("event CurrentModelID = %q, want claude-opus-4-7", ev.CurrentModelID) + } + if len(ev.SessionModels) != 2 { + t.Fatalf("event SessionModels len = %d, want 2", len(ev.SessionModels)) + } +} + +func TestInitialSessionModelState_TypedConfigOptionsBeatLegacyModels(t *testing.T) { + // When both surfaces are present, the typed configOptions[category=model] + // list wins — matches the documented precedence. + modelCategory := acp.SessionConfigOptionCategoryModel + modelOptions := acp.SessionConfigSelectOptionsUngrouped{ + {Name: "GPT-5.5", Value: "gpt-5.5"}, + } + configOptions := []acp.SessionConfigOption{ + {Select: &acp.SessionConfigOptionSelect{ + Type: "select", + Id: "model", + Name: "Model", + Category: &modelCategory, + CurrentValue: "gpt-5.5", + Options: acp.SessionConfigSelectOptions{Ungrouped: &modelOptions}, + }}, + } + legacy := &acp.LegacyModels{ + CurrentModelId: "claude-opus-4-7", + AvailableModels: []acp.LegacyModelInfo{{ModelId: "claude-opus-4-7", Name: "Opus 4.7"}}, + } + + models := initialSessionModelState(nil, configOptions, legacy) + if models == nil { + t.Fatal("initialSessionModelState returned nil") + } + if models.CurrentModelId != "gpt-5.5" { + t.Errorf("CurrentModelId = %q, want gpt-5.5 (typed must win over legacy)", models.CurrentModelId) } } @@ -287,7 +403,7 @@ func TestResolveCurrentModelFromConfig_ComposesReasoningEffort(t *testing.T) { {Type: "select", ID: "model", Category: "model", CurrentValue: "gpt-5.5"}, {Type: "select", ID: "reasoning_effort", Category: "thought_level", CurrentValue: reasoningEffortMedium}, } - available := []acp.ModelInfo{ + available := []modelInfo{ {ModelId: "gpt-5.5/low", Name: "GPT-5.5 (low)"}, {ModelId: "gpt-5.5/medium", Name: "GPT-5.5 (medium)"}, } @@ -303,7 +419,7 @@ func TestResolveCurrentModelFromConfig_PrefersReasoningModelWhenBaseAlsoAvailabl {Type: "select", ID: "model", Category: "model", CurrentValue: "gpt-5.5"}, {Type: "select", ID: "reasoning_effort", Category: "thought_level", CurrentValue: reasoningEffortMedium}, } - available := []acp.ModelInfo{ + available := []modelInfo{ {ModelId: "gpt-5.5", Name: "GPT-5.5"}, {ModelId: "gpt-5.5/medium", Name: "GPT-5.5 (medium)"}, } @@ -318,9 +434,9 @@ func TestResolveCurrentModelFromConfig_PrefersReasoningModelWhenBaseAlsoAvailabl // when the agent populates CurrentModelId, we propagate it verbatim. func TestEmitSessionModels_NonEmptyCurrentIDPreserved(t *testing.T) { a := newTestAdapter() - models := &acp.SessionModelState{ + models := &sessionModelState{ CurrentModelId: "claude-opus-4-7", - AvailableModels: []acp.ModelInfo{ + AvailableModels: []modelInfo{ {ModelId: "claude-opus-4-7", Name: "Opus 4.7"}, }, } @@ -340,7 +456,7 @@ func TestEmitSessionModels_NonEmptyCurrentIDPreserved(t *testing.T) { func TestEmitSetModelEvent_EmitsSessionModelsWithCachedState(t *testing.T) { a := newTestAdapter() - cachedModels := []acp.ModelInfo{ + cachedModels := []modelInfo{ {ModelId: "claude-opus-4-7", Name: "Opus 4.7"}, {ModelId: "build-analyzer", Name: "Build Analyzer"}, } @@ -397,7 +513,7 @@ func TestEmitSetModelEvent_RewritesSplitReasoningOptions(t *testing.T) { {Name: "Medium", Value: reasoningEffortMedium}, {Name: "High", Value: reasoningEffortHigh}, } - cachedModels := []acp.ModelInfo{ + cachedModels := []modelInfo{ {ModelId: "gpt-5.5/medium", Name: "GPT-5.5 (medium)"}, {ModelId: "gpt-5.5/high", Name: "GPT-5.5 (high)"}, } @@ -445,7 +561,7 @@ func TestEmitSetModelEvent_RewritesSplitReasoningOptionsWithSlashInBaseModel(t * {Name: "Medium", Value: reasoningEffortMedium}, {Name: "High", Value: reasoningEffortHigh}, } - cachedModels := []acp.ModelInfo{ + 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)"}, } @@ -494,7 +610,7 @@ func TestEmitSetModelEvent_DoesNotSplitSlashModelWithoutReasoningSuffix(t *testi {Name: "Medium", Value: reasoningEffortMedium}, {Name: "High", Value: reasoningEffortHigh}, } - cachedModels := []acp.ModelInfo{ + cachedModels := []modelInfo{ {ModelId: "vendor/gpt-5.5", Name: "Vendor GPT-5.5"}, } cachedConfig := []streams.ConfigOption{ @@ -592,7 +708,7 @@ func TestConfigOptionUpdate_ComposesReasoningEffortCurrentModel(t *testing.T) { } a.mu.Lock() - a.availableModels = []acp.ModelInfo{ + a.availableModels = []modelInfo{ {ModelId: "gpt-5.5/medium", Name: "GPT-5.5 (medium)"}, {ModelId: "gpt-5.5/high", Name: "GPT-5.5 (high)"}, } @@ -631,3 +747,46 @@ func TestConfigOptionUpdate_ComposesReasoningEffortCurrentModel(t *testing.T) { 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 +// a session_models convergence event nor reset the cached context-window size, +// because no switch actually happened. +func TestFinalizeSetModel_MethodNoneEmitsNothing(t *testing.T) { + a := newTestAdapter() + + cachedModels := []modelInfo{{ModelId: "claude-opus-4-7", Name: "Opus 4.7"}} + cachedConfig := []streams.ConfigOption{ + {Type: "select", ID: "model", Name: "Model", CurrentValue: "old-model"}, + } + + a.finalizeSetModel(sessionmodel.MethodNone, "sess-1", "claude-opus-4-7", cachedModels, cachedConfig) + + events := drainEvents(a) + for _, ev := range events { + if ev.Type == streams.EventTypeSessionModels { + t.Fatalf("expected no session_models event, got %+v", ev) + } + } +} + +// TestFinalizeSetModel_RealMethodEmitsEvent pins the positive path: when +// applySessionModel reports a real RPC was used (MethodSetConfigOption or +// MethodSetModel), the adapter must emit a session_models event so the +// frontend converges on the new model. +func TestFinalizeSetModel_RealMethodEmitsEvent(t *testing.T) { + a := newTestAdapter() + + cachedModels := []modelInfo{{ModelId: "claude-opus-4-7", Name: "Opus 4.7"}} + cachedConfig := []streams.ConfigOption{ + {Type: "select", ID: "model", Name: "Model", CurrentValue: "old-model"}, + } + + a.finalizeSetModel(sessionmodel.MethodSetConfigOption, "sess-1", "claude-opus-4-7", cachedModels, cachedConfig) + + ev := findSessionModelsEvent(t, drainEvents(a)) + if ev.CurrentModelID != "claude-opus-4-7" { + t.Errorf("CurrentModelID = %q, want %q", ev.CurrentModelID, "claude-opus-4-7") + } +} diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/wakeup_concurrency_test.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/wakeup_concurrency_test.go index 77ef21f99c..d9234effd8 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/wakeup_concurrency_test.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/wakeup_concurrency_test.go @@ -63,6 +63,9 @@ func (f *concurrencyFakeAgent) SetSessionConfigOption(_ context.Context, _ acp.S func (f *concurrencyFakeAgent) SetSessionMode(_ context.Context, _ acp.SetSessionModeRequest) (acp.SetSessionModeResponse, error) { return acp.SetSessionModeResponse{}, nil } +func (f *concurrencyFakeAgent) Logout(_ context.Context, _ acp.LogoutRequest) (acp.LogoutResponse, error) { + return acp.LogoutResponse{}, nil +} // setupConcurrencyFakeAgent wires an adapter to a blocking fake agent and // registers cleanup immediately so early t.Fatal paths cannot leak pipes or diff --git a/apps/backend/internal/agentctl/server/utility/acp_executor.go b/apps/backend/internal/agentctl/server/utility/acp_executor.go index 407075943a..389e422d4f 100644 --- a/apps/backend/internal/agentctl/server/utility/acp_executor.go +++ b/apps/backend/internal/agentctl/server/utility/acp_executor.go @@ -547,24 +547,15 @@ func buildInitProbeFields(initResp acp.InitializeResponse) *ProbeResponse { // applySessionProbeFields populates models and modes from an ACP session/new response. // -// The legacy `models`/`modes` fields were marked UNSTABLE in the ACP spec and -// newer agents (e.g. claude-agent-acp v0.42+) drop them in favour of the -// general `configOptions[]` carrier with `category: model | mode`. Read the -// legacy fields first, then fall back to configOptions when they are absent -// so both shapes work. +// As of acp-go-sdk v0.13.5 the legacy unstable `models` field on +// NewSessionResponse was removed upstream; model and mode selection are +// surfaced through the typed `configOptions[]` carrier with +// `category: model | mode`. The kdlbs fork restores read-only parsing of the +// top-level `models` field via acp.LegacyModels for agents that haven't +// migrated yet (auggie 0.29.x). The legacy `modes` field is still present on +// the SDK type and we keep populating from it for older agents. func applySessionProbeFields(out *ProbeResponse, sessionResp acp.NewSessionResponse) { out.ConfigOptions = probeConfigOptions(sessionResp.ConfigOptions) - if sessionResp.Models != nil { - out.CurrentModelID = string(sessionResp.Models.CurrentModelId) - for _, m := range sessionResp.Models.AvailableModels { - out.Models = append(out.Models, ProbeModel{ - ID: string(m.ModelId), - Name: m.Name, - Description: derefString(m.Description), - Meta: m.Meta, - }) - } - } if sessionResp.Modes != nil { out.CurrentModeID = string(sessionResp.Modes.CurrentModeId) for _, m := range sessionResp.Modes.AvailableModes { @@ -576,19 +567,34 @@ func applySessionProbeFields(out *ProbeResponse, sessionResp acp.NewSessionRespo }) } } - // Gate the fallback on the legacy field being absent (nil) rather than - // producing an empty slice. A non-nil `Models` with an empty - // `AvailableModels` is schema-valid and would otherwise mix sources: - // `CurrentModelID` set from the legacy field, then immediately - // overwritten by the configOptions fallback. Same for modes. - if sessionResp.Models == nil { - applyConfigOptionsAsModels(out, sessionResp.ConfigOptions) + applyConfigOptionsAsModels(out, sessionResp.ConfigOptions) + if len(out.Models) == 0 { + applyLegacyModelsFallback(out, sessionResp.LegacyModels) } if sessionResp.Modes == nil { applyConfigOptionsAsModes(out, sessionResp.ConfigOptions) } } +// applyLegacyModelsFallback fills out.Models / out.CurrentModelID from the +// pre-v0.13.5 top-level `models` field still emitted by agents like +// auggie 0.29.x. Only invoked when typed configOptions[category=model] did +// not produce a model list, so the new surface always wins when present. +func applyLegacyModelsFallback(out *ProbeResponse, legacy *acp.LegacyModels) { + if legacy == nil || len(legacy.AvailableModels) == 0 { + return + } + out.CurrentModelID = legacy.CurrentModelId + for _, m := range legacy.AvailableModels { + out.Models = append(out.Models, ProbeModel{ + ID: m.ModelId, + Name: m.Name, + Description: derefString(m.Description), + Meta: m.Meta, + }) + } +} + func probeConfigOptions(opts []acp.SessionConfigOption) []ProbeConfigOption { out := make([]ProbeConfigOption, 0, len(opts)) for _, opt := range opts { diff --git a/apps/backend/internal/agentctl/server/utility/acp_executor_test.go b/apps/backend/internal/agentctl/server/utility/acp_executor_test.go index 0f9b9861fa..5235afa8ed 100644 --- a/apps/backend/internal/agentctl/server/utility/acp_executor_test.go +++ b/apps/backend/internal/agentctl/server/utility/acp_executor_test.go @@ -251,81 +251,80 @@ func TestApplySessionProbeFields_FallsBackToConfigOptions(t *testing.T) { } } -// The legacy `models` field still wins when present so existing agents are -// unaffected; configOptions is only consulted as a fallback. -func TestApplySessionProbeFields_PrefersLegacyModelsField(t *testing.T) { +// auggie 0.29.x hasn't migrated to configOptions[category=model] and still +// emits the pre-v0.13.5 top-level `models` field. The kdlbs SDK fork keeps +// parsing it as acp.LegacyModels; the probe must fall through to that surface +// so the inference-agents endpoint still surfaces auggie's model list. +func TestApplySessionProbeFields_FallsBackToLegacyModels(t *testing.T) { t.Parallel() - modelCat := acp.SessionConfigOptionCategoryModel + opusDesc := "Great for complex coding" resp := acp.NewSessionResponse{ - Models: &acp.SessionModelState{ - CurrentModelId: "legacy", - AvailableModels: []acp.ModelInfo{ - {ModelId: "legacy", Name: "Legacy"}, + LegacyModels: &acp.LegacyModels{ + CurrentModelId: "claude-opus-4-7", + AvailableModels: []acp.LegacyModelInfo{ + {ModelId: "claude-opus-4-7", Name: "Opus 4.7", Description: &opusDesc}, + {ModelId: "claude-sonnet-4-5", Name: "Sonnet 4.5"}, }, }, - ConfigOptions: []acp.SessionConfigOption{ - {Select: &acp.SessionConfigOptionSelect{ - Category: &modelCat, - CurrentValue: "fallback", - Options: acp.SessionConfigSelectOptions{Ungrouped: &acp.SessionConfigSelectOptionsUngrouped{ - {Value: "fallback", Name: "Fallback"}, - }}, - Type: "select", - }}, - }, } out := &ProbeResponse{} applySessionProbeFields(out, resp) - if got, want := out.CurrentModelID, "legacy"; got != want { + if got, want := out.CurrentModelID, "claude-opus-4-7"; got != want { t.Fatalf("CurrentModelID = %q, want %q", got, want) } + if got, want := len(out.Models), 2; got != want { + t.Fatalf("len(Models) = %d, want %d", got, want) + } + if got, want := out.Models[0].ID, "claude-opus-4-7"; got != want { + t.Fatalf("Models[0].ID = %q, want %q", got, want) + } + if got, want := out.Models[0].Description, "Great for complex coding"; got != want { + t.Fatalf("Models[0].Description = %q, want %q", got, want) + } +} + +// When both surfaces are present the typed configOptions list wins — the +// legacy field is only a fallback for unmigrated agents. +func TestApplySessionProbeFields_TypedConfigOptionsBeatLegacyModels(t *testing.T) { + t.Parallel() + + modelCat := acp.SessionConfigOptionCategoryModel + resp := acp.NewSessionResponse{ + ConfigOptions: []acp.SessionConfigOption{ + {Select: &acp.SessionConfigOptionSelect{ + Category: &modelCat, + CurrentValue: "opus", + Id: "model", + Name: "Model", + Options: acp.SessionConfigSelectOptions{Ungrouped: &acp.SessionConfigSelectOptionsUngrouped{ + {Value: "opus", Name: "Opus"}, + }}, + Type: "select", + }}, + }, + LegacyModels: &acp.LegacyModels{ + CurrentModelId: "claude-opus-4-7", + AvailableModels: []acp.LegacyModelInfo{{ModelId: "claude-opus-4-7", Name: "Opus 4.7"}}, + }, + } + + out := &ProbeResponse{} + applySessionProbeFields(out, resp) + + if got, want := out.CurrentModelID, "opus"; got != want { + t.Fatalf("CurrentModelID = %q, want %q (typed configOptions must win)", got, want) + } if got, want := len(out.Models), 1; got != want { t.Fatalf("len(Models) = %d, want %d", got, want) } - if got, want := out.Models[0].ID, "legacy"; got != want { + if got, want := out.Models[0].ID, "opus"; got != want { t.Fatalf("Models[0].ID = %q, want %q", got, want) } } -// A non-nil `Models` struct with an empty `AvailableModels` slice is -// schema-valid. The fallback must NOT fire in that case — otherwise -// `CurrentModelID` set from the legacy field gets clobbered by the -// configOptions value, mixing sources. -func TestApplySessionProbeFields_LegacyEmptyModelsBlocksFallback(t *testing.T) { - t.Parallel() - - modelCat := acp.SessionConfigOptionCategoryModel - resp := acp.NewSessionResponse{ - Models: &acp.SessionModelState{ - CurrentModelId: "legacy-current", - AvailableModels: nil, - }, - ConfigOptions: []acp.SessionConfigOption{ - {Select: &acp.SessionConfigOptionSelect{ - Category: &modelCat, - CurrentValue: "fallback-current", - Options: acp.SessionConfigSelectOptions{Ungrouped: &acp.SessionConfigSelectOptionsUngrouped{ - {Value: "fallback-current", Name: "Fallback"}, - }}, - Type: "select", - }}, - }, - } - - out := &ProbeResponse{} - applySessionProbeFields(out, resp) - - if got, want := out.CurrentModelID, "legacy-current"; got != want { - t.Fatalf("CurrentModelID = %q, want %q (legacy must win, fallback must not fire)", got, want) - } - if len(out.Models) != 0 { - t.Fatalf("Models = %+v, want empty (fallback should be skipped when legacy field is non-nil)", out.Models) - } -} - // Grouped select-option payloads are flattened group-by-group so the // fallback works regardless of whether the agent groups its options. func TestApplySessionProbeFields_FlattensGroupedConfigOptions(t *testing.T) { diff --git a/apps/backend/internal/agentctl/server/utility/model_apply_test.go b/apps/backend/internal/agentctl/server/utility/model_apply_test.go index ae3db86619..6a81c6c208 100644 --- a/apps/backend/internal/agentctl/server/utility/model_apply_test.go +++ b/apps/backend/internal/agentctl/server/utility/model_apply_test.go @@ -18,7 +18,7 @@ func (f *fakeModelConn) SetSessionConfigOption(_ context.Context, req acp.SetSes } func (f *fakeModelConn) UnstableSetSessionModel(_ context.Context, req acp.UnstableSetSessionModelRequest) (acp.UnstableSetSessionModelResponse, error) { - f.calls = append(f.calls, "model:"+string(req.ModelId)) + f.calls = append(f.calls, "legacy:"+string(req.SessionId)+":"+req.ModelId) return acp.UnstableSetSessionModelResponse{}, nil } @@ -47,7 +47,13 @@ func TestApplySessionModel_UsesConfigOptionWhenSessionAdvertisesModelOption(t *t } } -func TestApplySessionModel_UsesSetModelWithoutModelConfigOption(t *testing.T) { +// TestApplySessionModel_FallsBackToLegacySetModel pins that when the session +// advertises no model-shaped config option, the dispatcher falls through to +// the pre-v0.13.5 session/set_model RPC restored by the kdlbs acp-go-sdk +// fork. This keeps model switching working for unmigrated agents like +// auggie 0.29.x, which surface their models via the legacy top-level +// `models` field on session/new instead of a typed config option. +func TestApplySessionModel_FallsBackToLegacySetModel(t *testing.T) { t.Parallel() conn := &fakeModelConn{} @@ -59,8 +65,51 @@ func TestApplySessionModel_UsesSetModelWithoutModelConfigOption(t *testing.T) { if method != "session/set_model" { t.Fatalf("method = %q, want session/set_model", method) } - wantCalls := []string{"model:legacy-model"} + wantCalls := []string{"legacy:sess-1:legacy-model"} if !reflect.DeepEqual(conn.calls, wantCalls) { t.Fatalf("calls = %#v, want %#v", conn.calls, wantCalls) } } + +// TestApplySessionModel_NoOpWhenAgentSupportsNeitherSurface pins that when +// the session has no model-shaped config option AND the legacy +// session/set_model RPC returns JSON-RPC -32601 (method not found), the +// dispatcher treats the request as a clean no-op (empty method, nil error). +// This is how agents that support no model-selection surface at all signal +// "I don't support model selection". +func TestApplySessionModel_NoOpWhenAgentSupportsNeitherSurface(t *testing.T) { + t.Parallel() + + conn := &fakeModelConnLegacyErr{ + err: acp.NewMethodNotFound(acp.LegacyAgentMethodSessionSetModel), + } + method, err := applySessionModel(context.Background(), conn, "sess-1", "legacy-model", nil) + + if err != nil { + t.Fatalf("applySessionModel() error = %v, want nil", err) + } + if method != "" { + t.Fatalf("method = %q, want empty (MethodNone)", method) + } + wantCalls := []string{"legacy:sess-1:legacy-model"} + if !reflect.DeepEqual(conn.calls, wantCalls) { + t.Fatalf("calls = %#v, want %#v", conn.calls, wantCalls) + } +} + +// fakeModelConnLegacyErr returns a configurable error from the legacy +// session/set_model RPC. Used to exercise the -32601 no-op path. +type fakeModelConnLegacyErr struct { + err error + calls []string +} + +func (f *fakeModelConnLegacyErr) SetSessionConfigOption(_ context.Context, req acp.SetSessionConfigOptionRequest) (acp.SetSessionConfigOptionResponse, error) { + f.calls = append(f.calls, "config:"+string(req.ValueId.ConfigId)+":"+string(req.ValueId.Value)) + return acp.SetSessionConfigOptionResponse{}, nil +} + +func (f *fakeModelConnLegacyErr) UnstableSetSessionModel(_ context.Context, req acp.UnstableSetSessionModelRequest) (acp.UnstableSetSessionModelResponse, error) { + f.calls = append(f.calls, "legacy:"+string(req.SessionId)+":"+req.ModelId) + return acp.UnstableSetSessionModelResponse{}, f.err +} diff --git a/apps/backend/internal/agentctl/sessionmodel/sessionmodel.go b/apps/backend/internal/agentctl/sessionmodel/sessionmodel.go index e7de36fc21..d69d871d34 100644 --- a/apps/backend/internal/agentctl/sessionmodel/sessionmodel.go +++ b/apps/backend/internal/agentctl/sessionmodel/sessionmodel.go @@ -17,8 +17,13 @@ type Method string const ( MethodNone Method = "" - MethodSetModel Method = "session/set_model" MethodSetConfigOption Method = "session/set_config_option" + // MethodSetModel is the pre-v0.13.5 unstable session/set_model RPC. + // Used as a fallback for agents (e.g. auggie 0.29.x) that surface their + // model list via the legacy top-level `models` field on session/new + // instead of SessionConfigOption(category="model"). The RPC is provided + // by the kdlbs acp-go-sdk fork (ClientSideConnection.UnstableSetSessionModel). + MethodSetModel Method = "session/set_model" ) // ConfigOption is the subset of ACP session config options needed to decide @@ -35,16 +40,23 @@ type Request struct { ConfigOptions []ConfigOption } -// Applier performs the actual ACP calls. Implementations wrap either the ACP +// Applier performs the actual ACP call. Implementations wrap either the ACP // SDK connection or the agentctl websocket client. type Applier interface { SetConfigOption(ctx context.Context, sessionID, configID, value string) error - SetModel(ctx context.Context, sessionID, modelID string) error + // SetModelLegacy issues the pre-v0.13.5 unstable session/set_model RPC. + // Used only when the session advertises no model-shaped config option; + // implementations should return a JSON-RPC -32601 error (recognized by + // IsMethodNotFound) when the agent doesn't implement the legacy surface, + // so Apply can treat the model change as a clean no-op. + SetModelLegacy(ctx context.Context, sessionID, modelID string) error } // SDKConn is the subset of the ACP SDK connection used to apply model changes. type SDKConn interface { SetSessionConfigOption(context.Context, acp.SetSessionConfigOptionRequest) (acp.SetSessionConfigOptionResponse, error) + // UnstableSetSessionModel is the legacy session/set_model RPC restored by + // the kdlbs acp-go-sdk fork for compatibility with unmigrated agents. UnstableSetSessionModel(context.Context, acp.UnstableSetSessionModelRequest) (acp.UnstableSetSessionModelResponse, error) } @@ -64,10 +76,10 @@ func (a SDKApplier) SetConfigOption(ctx context.Context, sessionID, configID, va return err } -func (a SDKApplier) SetModel(ctx context.Context, sessionID, modelID string) error { +func (a SDKApplier) SetModelLegacy(ctx context.Context, sessionID, modelID string) error { _, err := a.Conn.UnstableSetSessionModel(ctx, acp.UnstableSetSessionModelRequest{ SessionId: acp.SessionId(sessionID), - ModelId: acp.UnstableModelId(modelID), + ModelId: modelID, }) return err } @@ -92,27 +104,37 @@ func ApplySDKFromACP( }) } -// Apply chooses the model-switching mechanism supported by the session. Agents -// that expose a model-shaped config option (Codex, Cursor, recent Claude) are -// configured through session/set_config_option. If that RPC is not implemented, -// we fall back to the older unstable session/set_model call. +// Apply selects the model by routing through the typed +// session/set_config_option RPC when the session advertises a model-shaped +// config option. When that option is absent — or when the modern RPC returns +// JSON-RPC -32601 because a partially-migrated agent advertises the option +// without implementing the handler — falls through to the legacy +// session/set_model RPC. A -32601 from the legacy call is treated as the +// agent declaring "I don't support model selection", and Apply returns +// MethodNone with no error so callers can no-op cleanly. func Apply(ctx context.Context, applier Applier, req Request) (Method, error) { if req.ModelID == "" { return MethodNone, nil } if configID, ok := modelConfigID(req.ConfigOptions); ok { - if err := applier.SetConfigOption(ctx, req.SessionID, configID, req.ModelID); err != nil { - if !IsMethodNotFound(err) { - return MethodSetConfigOption, err - } - } else { + err := applier.SetConfigOption(ctx, req.SessionID, configID, req.ModelID) + if err == nil { return MethodSetConfigOption, nil } + if !IsMethodNotFound(err) { + return MethodSetConfigOption, err + } + // Agent advertises the typed option but its handler isn't wired up + // (partial migration); fall through to the legacy RPC below. } - if err := applier.SetModel(ctx, req.SessionID, req.ModelID); err != nil { - return MethodSetModel, err + err := applier.SetModelLegacy(ctx, req.SessionID, req.ModelID) + if err == nil { + return MethodSetModel, nil } - return MethodSetModel, nil + if IsMethodNotFound(err) { + return MethodNone, nil + } + return MethodSetModel, err } // FromACP converts typed ACP SDK options to the shared strategy shape. diff --git a/apps/backend/internal/agentctl/sessionmodel/sessionmodel_test.go b/apps/backend/internal/agentctl/sessionmodel/sessionmodel_test.go index 613c717758..1a6a174167 100644 --- a/apps/backend/internal/agentctl/sessionmodel/sessionmodel_test.go +++ b/apps/backend/internal/agentctl/sessionmodel/sessionmodel_test.go @@ -11,7 +11,7 @@ import ( type fakeApplier struct { configErr error - modelErr error + legacyErr error calls []string } @@ -20,12 +20,12 @@ func (f *fakeApplier) SetConfigOption(_ context.Context, _ string, configID, val return f.configErr } -func (f *fakeApplier) SetModel(_ context.Context, _, modelID string) error { - f.calls = append(f.calls, "model:"+modelID) - return f.modelErr +func (f *fakeApplier) SetModelLegacy(_ context.Context, sessionID, modelID string) error { + f.calls = append(f.calls, "legacy:"+sessionID+":"+modelID) + return f.legacyErr } -func TestApply_PrefersModelConfigOption(t *testing.T) { +func TestApply_UsesModelConfigOption(t *testing.T) { t.Parallel() applier := &fakeApplier{} @@ -50,38 +50,17 @@ func TestApply_PrefersModelConfigOption(t *testing.T) { } } -func TestApply_FallsBackToSetModelWhenConfigOptionMethodMissing(t *testing.T) { - t.Parallel() - - applier := &fakeApplier{configErr: acp.NewMethodNotFound(acp.AgentMethodSessionSetConfigOption)} - method, err := Apply(context.Background(), applier, Request{ - SessionID: "sess-1", - ModelID: "claude-opus-4-8", - ConfigOptions: []ConfigOption{{ - ID: "model", - Category: "model", - }}, - }) - - if err != nil { - t.Fatalf("Apply() error = %v", err) - } - if method != MethodSetModel { - t.Fatalf("method = %q, want %q", method, MethodSetModel) - } - wantCalls := []string{"config:model:claude-opus-4-8", "model:claude-opus-4-8"} - if !reflect.DeepEqual(applier.calls, wantCalls) { - t.Fatalf("calls = %#v, want %#v", applier.calls, wantCalls) - } -} - -func TestApply_UsesSetModelWhenNoModelConfigOption(t *testing.T) { +// TestApply_FallsBackToLegacySetModel verifies the legacy fallback path: +// when the session advertises no model-shaped config option, Apply issues +// the pre-v0.13.5 session/set_model RPC restored by the kdlbs fork. This +// keeps model switching working for unmigrated agents (e.g. auggie 0.29.x). +func TestApply_FallsBackToLegacySetModel(t *testing.T) { t.Parallel() applier := &fakeApplier{} method, err := Apply(context.Background(), applier, Request{ SessionID: "sess-1", - ModelID: "legacy-model", + ModelID: "claude-opus-4-7", ConfigOptions: []ConfigOption{{ID: "mode", Category: "mode"}}, }) @@ -91,13 +70,62 @@ func TestApply_UsesSetModelWhenNoModelConfigOption(t *testing.T) { if method != MethodSetModel { t.Fatalf("method = %q, want %q", method, MethodSetModel) } - wantCalls := []string{"model:legacy-model"} + wantCalls := []string{"legacy:sess-1:claude-opus-4-7"} if !reflect.DeepEqual(applier.calls, wantCalls) { t.Fatalf("calls = %#v, want %#v", applier.calls, wantCalls) } } -func TestApply_DoesNotFallbackForInvalidConfigOptionValue(t *testing.T) { +// TestApply_LegacyMethodNotFoundIsNoOp pins that when the legacy +// session/set_model RPC returns JSON-RPC -32601, Apply treats the request as +// a clean no-op (MethodNone, nil error). This is how agents that support +// neither surface (no config option AND no legacy RPC) signal "I don't +// support model selection". +func TestApply_LegacyMethodNotFoundIsNoOp(t *testing.T) { + t.Parallel() + + notFound := acp.NewMethodNotFound(acp.LegacyAgentMethodSessionSetModel) + applier := &fakeApplier{legacyErr: notFound} + method, err := Apply(context.Background(), applier, Request{ + SessionID: "sess-1", + ModelID: "some-model", + ConfigOptions: []ConfigOption{{ID: "mode", Category: "mode"}}, + }) + + if err != nil { + t.Fatalf("Apply() error = %v, want nil", err) + } + if method != MethodNone { + t.Fatalf("method = %q, want %q", method, MethodNone) + } + wantCalls := []string{"legacy:sess-1:some-model"} + if !reflect.DeepEqual(applier.calls, wantCalls) { + t.Fatalf("calls = %#v, want %#v", applier.calls, wantCalls) + } +} + +// TestApply_PropagatesLegacySetModelError pins that non-32601 errors from +// the legacy RPC bubble up unchanged with MethodSetModel attribution. +func TestApply_PropagatesLegacySetModelError(t *testing.T) { + t.Parallel() + + invalid := acp.NewInvalidParams(map[string]any{"message": "Unknown model"}) + applier := &fakeApplier{legacyErr: invalid} + method, err := Apply(context.Background(), applier, Request{ + SessionID: "sess-1", + ModelID: "bogus", + ConfigOptions: []ConfigOption{{ID: "mode", Category: "mode"}}, + }) + + if !errors.Is(err, invalid) { + t.Fatalf("Apply() error = %v, want %v", err, invalid) + } + if method != MethodSetModel { + t.Fatalf("method = %q, want %q", method, MethodSetModel) + } +} + +func TestApply_PropagatesSetConfigOptionError(t *testing.T) { t.Parallel() invalid := acp.NewInvalidParams(map[string]any{"message": "Invalid model value"}) @@ -122,3 +150,33 @@ func TestApply_DoesNotFallbackForInvalidConfigOptionValue(t *testing.T) { t.Fatalf("calls = %#v, want %#v", applier.calls, wantCalls) } } + +// TestApply_SetConfigOptionMethodNotFoundFallsBackToLegacy pins the +// partial-migration fallthrough: when the session advertises a model-shaped +// config option but the agent answers session/set_config_option with -32601, +// Apply falls through to the legacy session/set_model RPC instead of +// surfacing the error. +func TestApply_SetConfigOptionMethodNotFoundFallsBackToLegacy(t *testing.T) { + t.Parallel() + + applier := &fakeApplier{configErr: acp.NewMethodNotFound(acp.AgentMethodSessionSetConfigOption)} + method, err := Apply(context.Background(), applier, Request{ + SessionID: "sess-1", + ModelID: "claude-opus-4-7", + ConfigOptions: []ConfigOption{{ + ID: "model", + Category: "model", + }}, + }) + + if err != nil { + t.Fatalf("Apply() error = %v, want nil", err) + } + if method != MethodSetModel { + t.Fatalf("method = %q, want %q", method, MethodSetModel) + } + wantCalls := []string{"config:model:claude-opus-4-7", "legacy:sess-1:claude-opus-4-7"} + if !reflect.DeepEqual(applier.calls, wantCalls) { + t.Fatalf("calls = %#v, want %#v", applier.calls, wantCalls) + } +} diff --git a/apps/backend/internal/orchestrator/event_handlers_agent.go b/apps/backend/internal/orchestrator/event_handlers_agent.go index 1d48417278..c3cba9a0fb 100644 --- a/apps/backend/internal/orchestrator/event_handlers_agent.go +++ b/apps/backend/internal/orchestrator/event_handlers_agent.go @@ -907,6 +907,23 @@ func (s *Service) handleAgentStopped(ctx context.Context, data watcher.AgentEven // on that self-inflicted stop would abort the retry. The loop is freed on // ready/completed (success), cancel, and exhaustion instead. + // Drop stopped events that belong to a previous (rotated) execution. The + // session might already be running a fresh resume cycle; flipping its state + // to CANCELLED based on the corpse of the prior execution poisons the + // recovery (the new cycle's session/load succeeds against a session that + // looks terminal to the rest of the system). Mirrors the rotation guard in + // handleAgentCompleted. + if s.agentManager != nil && data.AgentExecutionID != "" && data.SessionID != "" { + if liveExecID, _ := s.agentManager.GetExecutionIDForSession(ctx, data.SessionID); liveExecID != "" && liveExecID != data.AgentExecutionID { + s.logger.Info("ignoring agent.stopped for non-active (rotated) execution", + zap.String("task_id", data.TaskID), + zap.String("session_id", data.SessionID), + zap.String("event_execution_id", data.AgentExecutionID), + zap.String("live_execution_id", liveExecID)) + return + } + } + // Complete the current turn if there is one s.completeTurnForSession(ctx, data.SessionID) diff --git a/apps/backend/internal/orchestrator/event_handlers_test.go b/apps/backend/internal/orchestrator/event_handlers_test.go index 99195fbaab..eb131b8eae 100644 --- a/apps/backend/internal/orchestrator/event_handlers_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_test.go @@ -1650,6 +1650,44 @@ func TestHandleAgentStopped_PreservesRecoveryState(t *testing.T) { models.TaskSessionStateIdle, updated.State) } }) + + // Rotated-execution regression: a previous cycle's agent.stopped event must + // not flip the session to CANCELLED when a fresh resume cycle has already + // taken over (executors_running.agent_execution_id rotated). Without this + // guard, an intermittent ACP notification-queue overflow on cycle 1 + // produces a late stopped event whose state mutation poisons the in- + // progress cycle 2 — the user sees the task transition to FAILED even + // though cycle 2 loaded the session cleanly. See discussion in the cycle + // 1/2/3 analysis (task a263caf5). + t.Run("drops stopped events from rotated executions", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + + // Make sure session is in RUNNING — the state we'd clobber if the + // guard didn't fire. + session, _ := repo.GetTaskSession(ctx, "s1") + session.State = models.TaskSessionStateRunning + _ = repo.UpdateTaskSession(ctx, session) + + // Live execution is exec-2 (cycle 2). The stale event below carries + // exec-1 (cycle 1). + seedExecutorRunning(t, repo, "s1", "t1", "exec-2") + + agentMgr := &mockAgentManager{repoForExecutionLookup: repo} + svc := createTestServiceWithAgent(repo, newMockStepGetter(), newMockTaskRepo(), agentMgr) + + svc.handleAgentStopped(ctx, watcher.AgentEventData{ + TaskID: "t1", + SessionID: "s1", + AgentExecutionID: "exec-1", + }) + + updated, _ := repo.GetTaskSession(ctx, "s1") + if updated.State != models.TaskSessionStateRunning { + t.Errorf("expected state to remain %q (rotated event ignored), got %q", + models.TaskSessionStateRunning, updated.State) + } + }) } // waitForStopCall polls until the mock agent manager has received at least one diff --git a/apps/web/components/model-config-selector.tsx b/apps/web/components/model-config-selector.tsx index 2b2f46b497..33386f8a7b 100644 --- a/apps/web/components/model-config-selector.tsx +++ b/apps/web/components/model-config-selector.tsx @@ -1,6 +1,6 @@ "use client"; -import { memo, useCallback, useState } from "react"; +import { memo, useState } from "react"; import { IconCheck, IconChevronDown } from "@tabler/icons-react"; import { cn } from "@/lib/utils"; @@ -207,13 +207,14 @@ export const ModelConfigSelector = memo(function ModelConfigSelector({ const currentModelValue = modelConfig?.currentValue || currentModel || ""; const label = resolveTriggerLabel(modelOptions, currentModel, modelConfig, configOptions); - const onModelSelect = useCallback( - (value: string) => { - if (!value) return; - onModelChange(value); - }, - [onModelChange], - ); + const hasExtraConfigOptions = extraConfigOptions.length > 0; + const onModelSelect = (value: string) => { + if (!value) return; + onModelChange(value); + if (!hasExtraConfigOptions) { + setOpen(false); + } + }; const triggerClassName = variant === "compact" diff --git a/apps/web/components/task/model-selector.tsx b/apps/web/components/task/model-selector.tsx index 1bc0f464c2..35772f47f6 100644 --- a/apps/web/components/task/model-selector.tsx +++ b/apps/web/components/task/model-selector.tsx @@ -1,6 +1,6 @@ "use client"; -import { memo, useCallback, useMemo } from "react"; +import { memo, useCallback, useMemo, useRef } from "react"; import { configOptionToModelOptions, @@ -11,6 +11,7 @@ import { usableConfigOptions, } from "@/components/model-config-selector"; import { useAppStore } from "@/components/state-provider"; +import { useToast } from "@/components/toast-provider"; import { useAvailableAgents } from "@/hooks/domains/settings/use-available-agents"; import { useSettingsData } from "@/hooks/domains/settings/use-settings-data"; import { setSessionConfigOption, setSessionModel } from "@/lib/api/domains/session-api"; @@ -20,6 +21,12 @@ import type { SessionModelEntry, } from "@/lib/state/slices/session-runtime/types"; +type SessionModelsEntry = { + currentModelId: string; + models: SessionModelEntry[]; + configOptions: ConfigOptionEntry[]; +}; + type ModelSelectorProps = { sessionId: string | null; }; @@ -127,6 +134,99 @@ function resolveAvailableModels({ return resolveStaticModels(settingsAgents, profileId, availableAgents); } +function describeError(err: unknown): string { + return err instanceof Error ? err.message : "Unknown error"; +} + +/** Builds model/config change handlers with optimistic update + error toast + revert. */ +function useModelChangeHandlers( + configOptions: SelectConfigOption[], + sessionModelsData: SessionModelsEntry | undefined, +) { + const activeModels = useAppStore((state) => state.activeModel.bySessionId); + const setActiveModel = useAppStore((state) => state.setActiveModel); + const setSessionModels = useAppStore((state) => state.setSessionModels); + const { toast } = useToast(); + // Per-session monotonic request id so a stale failure doesn't clobber a + // newer successful selection (rapid A -> B -> C where B fails). + const latestReqId = useRef>({}); + + const updateLocalConfig = useCallback( + (sid: string, configId: string, value: string) => { + if (!sessionModelsData) return; + setSessionModels(sid, { + ...sessionModelsData, + currentModelId: nextCurrentModelId(sessionModelsData, configId, value), + configOptions: updateConfigOptionValue(sessionModelsData.configOptions, configId, value), + }); + }, + [sessionModelsData, setSessionModels], + ); + + const onFail = useCallback( + ( + sid: string, + reqId: number, + previousActive: string, + previousModels: SessionModelsEntry | undefined, + ) => + (err: unknown) => { + if (latestReqId.current[sid] !== reqId) return; + console.error("[ModelSelector] model change failed:", err); + setActiveModel(sid, previousActive); + if (previousModels) setSessionModels(sid, previousModels); + toast({ + title: "Failed to change model", + description: describeError(err), + variant: "error", + }); + }, + [setActiveModel, setSessionModels, toast], + ); + + const nextReqId = useCallback((sid: string) => { + const id = (latestReqId.current[sid] ?? 0) + 1; + latestReqId.current[sid] = id; + return id; + }, []); + + const handleModelChange = useCallback( + (sid: string, modelId: string) => { + const reqId = nextReqId(sid); + const fail = onFail(sid, reqId, activeModels[sid] ?? "", sessionModelsData); + setActiveModel(sid, modelId); + const modelConfig = configOptions.find(isModelConfigOption); + if (modelConfig) { + updateLocalConfig(sid, modelConfig.id, modelId); + setSessionConfigOption(sid, modelConfig.id, modelId).catch(fail); + return; + } + setSessionModel(sid, modelId).catch(fail); + }, + [ + activeModels, + configOptions, + nextReqId, + onFail, + sessionModelsData, + setActiveModel, + updateLocalConfig, + ], + ); + + const handleConfigChange = useCallback( + (sid: string, configId: string, value: string) => { + const reqId = nextReqId(sid); + const fail = onFail(sid, reqId, activeModels[sid] ?? "", sessionModelsData); + updateLocalConfig(sid, configId, value); + setSessionConfigOption(sid, configId, value).catch(fail); + }, + [activeModels, nextReqId, onFail, sessionModelsData, updateLocalConfig], + ); + + return { handleModelChange, handleConfigChange }; +} + /** Resolves available models, config options and current model from store state. */ function useModelSelectorState(sessionId: string | null) { useSettingsData(true); @@ -134,8 +234,6 @@ function useModelSelectorState(sessionId: string | null) { const settingsAgents = useAppStore((state) => state.settingsAgents.items); const taskSessions = useAppStore((state) => state.taskSessions.items); const activeModels = useAppStore((state) => state.activeModel.bySessionId); - const setActiveModel = useAppStore((state) => state.setActiveModel); - const setSessionModels = useAppStore((state) => state.setSessionModels); const { items: availableAgents } = useAvailableAgents(); const sessionModelsData = useAppStore((state) => sessionId ? state.sessionModels.bySessionId[sessionId] : undefined, @@ -170,44 +268,9 @@ function useModelSelectorState(sessionId: string | null) { ); const modelOptions = buildModelOptions(availableModels, currentModel); - const updateLocalConfig = useCallback( - (sid: string, configId: string, value: string) => { - if (!sessionModelsData) return; - setSessionModels(sid, { - ...sessionModelsData, - currentModelId: nextCurrentModelId(sessionModelsData, configId, value), - configOptions: updateConfigOptionValue(sessionModelsData.configOptions, configId, value), - }); - }, - [sessionModelsData, setSessionModels], - ); - - const handleModelChange = useCallback( - (sid: string, modelId: string) => { - setActiveModel(sid, modelId); - const modelConfig = configOptions.find(isModelConfigOption); - if (modelConfig) { - updateLocalConfig(sid, modelConfig.id, modelId); - setSessionConfigOption(sid, modelConfig.id, modelId).catch((err) => { - console.error("[ModelSelector] set-config-option API failed:", err); - }); - return; - } - setSessionModel(sid, modelId).catch((err) => { - console.error("[ModelSelector] set-model API failed:", err); - }); - }, - [configOptions, setActiveModel, updateLocalConfig], - ); - - const handleConfigChange = useCallback( - (sid: string, configId: string, value: string) => { - updateLocalConfig(sid, configId, value); - setSessionConfigOption(sid, configId, value).catch((err) => { - console.error("[ModelSelector] set-config-option API failed:", err); - }); - }, - [updateLocalConfig], + const { handleModelChange, handleConfigChange } = useModelChangeHandlers( + configOptions, + sessionModelsData, ); return { currentModel, modelOptions, configOptions, handleModelChange, handleConfigChange }; diff --git a/apps/web/e2e/tests/chat/model-selector-error.spec.ts b/apps/web/e2e/tests/chat/model-selector-error.spec.ts new file mode 100644 index 0000000000..08ffe63c44 --- /dev/null +++ b/apps/web/e2e/tests/chat/model-selector-error.spec.ts @@ -0,0 +1,145 @@ +import { test, expect } from "../../fixtures/test-base"; +import { SessionPage } from "../../pages/session-page"; + +/** + * Verifies the chat-input model selector's failure path: + * when the backend rejects a model change, the UI shows a toast carrying the + * backend error message and the trigger label reverts to the previous model. + * + * mock-agent exposes "model" as a config option, so changing the model goes + * through POST /set-config-option. Agents without a model config option use + * POST /set-model — both routes are stubbed for resilience. + */ +test.describe("Chat model selector — RPC failure", () => { + test("shows error toast and reverts trigger label when model change fails", async ({ + testPage, + apiClient, + seedData, + }) => { + const task = await apiClient.createTaskWithAgent( + seedData.workspaceId, + "Model Selector Error 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).toBeVisible({ timeout: 15_000 }); + // mock-agent ships with model="mock-fast" + effort="medium", so the + // composite trigger label starts as "Mock Fast / Medium". + await expect(trigger).toContainText("Mock Fast", { timeout: 15_000 }); + + const backendErrorMessage = "mock backend rejected model change"; + const fail = (route: import("@playwright/test").Route) => + route.fulfill({ + status: 500, + contentType: "application/json", + body: JSON.stringify({ error: backendErrorMessage }), + }); + await testPage.route("**/set-model", fail); + await testPage.route("**/set-config-option", fail); + + await trigger.click(); + const smartRow = testPage.getByRole("option", { name: /Mock Smart/ }); + await expect(smartRow).toBeVisible({ timeout: 5_000 }); + await smartRow.click(); + + const toast = testPage + .getByTestId("toast-message") + .filter({ hasText: "Failed to change model" }); + await expect(toast).toBeVisible({ timeout: 5_000 }); + await expect(toast).toContainText(backendErrorMessage); + + // After revert the trigger label should match the original (model + extras). + await expect(trigger).toContainText("Mock Fast", { timeout: 5_000 }); + await expect(trigger).not.toContainText("Mock Smart"); + }); + + test("stale failure does not revert a newer successful change", async ({ + testPage, + apiClient, + seedData, + }) => { + const task = await apiClient.createTaskWithAgent( + seedData.workspaceId, + "Model Selector Race 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 }); + + // Hold the first config-option request open so we can fire a second one + // before the first settles. Resolve the first as a 500 after the second + // has already been issued — the stale rejection must NOT clobber the + // newer optimistic state. + let releaseFirst: (() => void) | null = null; + const firstHeld = new Promise((resolve) => { + releaseFirst = resolve; + }); + // Resolves once the first (stale) request has fully completed its + // route.fulfill, so the test can deterministically await propagation + // instead of using a fixed sleep. + let firstSettled: (() => void) | null = null; + const firstSettledPromise = new Promise((resolve) => { + firstSettled = resolve; + }); + let callCount = 0; + await testPage.route("**/set-config-option", async (route) => { + callCount += 1; + if (callCount === 1) { + await firstHeld; + await route.fulfill({ + status: 500, + contentType: "application/json", + body: JSON.stringify({ error: "stale failure" }), + }); + firstSettled?.(); + return; + } + return route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify({ ok: true }), + }); + }); + + await trigger.click(); + await testPage.getByRole("option", { name: /Mock Smart/ }).click(); + // Re-open and pick again — second request will succeed. mock-agent only + // ships two models (Mock Fast / Mock Smart), so we go back to Mock Fast. + await trigger.click(); + await testPage.getByRole("option", { name: /Mock Fast/ }).click(); + + // Now release the first (stale) request — its 500 rejection should be + // swallowed (no toast). + releaseFirst?.(); + await firstSettledPromise; + await expect(testPage.getByTestId("toast-message")).toHaveCount(0); + // Trigger should still reflect the newer (successful) selection. + await expect(trigger).toContainText("Mock Fast", { timeout: 5_000 }); + }); +});