diff --git a/apps/backend/internal/agent/hostutility/public.go b/apps/backend/internal/agent/hostutility/public.go index 8a93cc5103..11ce5cc03b 100644 --- a/apps/backend/internal/agent/hostutility/public.go +++ b/apps/backend/internal/agent/hostutility/public.go @@ -6,6 +6,8 @@ import ( "fmt" "time" + "go.uber.org/zap" + "github.com/kandev/kandev/internal/agent/agents" "github.com/kandev/kandev/internal/agent/settings/cliflags" agentctlutil "github.com/kandev/kandev/internal/agentctl/server/utility" @@ -199,6 +201,11 @@ func (m *Manager) resolveModelConfigFlight( if isAuthError(resp.Error) { resolution.Status = StatusAuthRequired } + m.log.Warn("model config resolution failed", + zap.String("agent_type", agentType), + zap.String("model", req.Model), + zap.String("status", string(resolution.Status)), + zap.String("error", resp.Error)) return resolution, nil } diff --git a/apps/backend/internal/agentctl/server/utility/acp_executor.go b/apps/backend/internal/agentctl/server/utility/acp_executor.go index 9f8b80d2d2..7c8980443c 100644 --- a/apps/backend/internal/agentctl/server/utility/acp_executor.go +++ b/apps/backend/internal/agentctl/server/utility/acp_executor.go @@ -796,6 +796,20 @@ func applyProbeModel( return nil, err } if !received { + if method == sessionmodel.MethodSetModel { + // The legacy session/set_model RPC applied the model, but the agent + // surfaces no per-model config options and pushes no follow-up + // config-update notification (e.g. auggie, which advertises a flat + // model list and answers session/set_model with an empty result). + // That is a valid empty resolution, not a failure: the caller keeps + // the session-advertised options. + return nil, nil + } + // A typed session/set_config_option that returns neither inline options + // nor a config-update notification leaves us without an authoritative + // snapshot for the newly selected model. Keeping the pre-switch + // session/new snapshot would report the previous model's options as the + // current configuration, so treat this as a failure. return nil, fmt.Errorf("ACP model selection returned no configuration options") } return updated, nil @@ -904,7 +918,9 @@ func (e *ACPInferenceExecutor) probeACPSessionWithContext( if err != nil { return nil, err } - sessionResp.ConfigOptions = updated + if updated != nil { + sessionResp.ConfigOptions = updated + } } if updated, err := applyProbeConfigOptions( ctx, diff --git a/apps/backend/internal/agentctl/server/utility/acp_probe_capabilities_test.go b/apps/backend/internal/agentctl/server/utility/acp_probe_capabilities_test.go index 7e031cc7e1..bd25f22182 100644 --- a/apps/backend/internal/agentctl/server/utility/acp_probe_capabilities_test.go +++ b/apps/backend/internal/agentctl/server/utility/acp_probe_capabilities_test.go @@ -174,7 +174,15 @@ func TestProbeACPSessionWithContextRejectsProviderWithoutModelSelection(t *testi } } -func TestProbeACPSessionWithContextTimesOutWithoutConfigSnapshot(t *testing.T) { +// TestProbeACPSessionWithContextErrorsWhenConfigOptionYieldsNoSnapshot pins that +// when an agent advertises a typed model config option (so the probe applies the +// model via session/set_config_option) but returns neither inline options nor a +// follow-up config-update notification, the probe fails. Keeping the pre-switch +// session/new snapshot would report the previous model's options as the resolved +// configuration for the newly selected model, so this must stay an error. The +// empty-resolution relaxation is scoped to the legacy session/set_model path +// (auggie); see TestApplyProbeModel_LegacyNoConfigOptionsReturnsEmpty. +func TestProbeACPSessionWithContextErrorsWhenConfigOptionYieldsNoSnapshot(t *testing.T) { c2aR, c2aW := io.Pipe() a2cR, a2cW := io.Pipe() t.Cleanup(func() { 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 557c2b08b5..6e46d661a6 100644 --- a/apps/backend/internal/agentctl/server/utility/model_apply_test.go +++ b/apps/backend/internal/agentctl/server/utility/model_apply_test.go @@ -4,6 +4,7 @@ import ( "context" "reflect" "testing" + "time" acp "github.com/coder/acp-go-sdk" ) @@ -169,6 +170,67 @@ func TestApplySessionModel_NoOpWhenAgentSupportsNeitherSurface(t *testing.T) { } } +// TestApplyProbeModel_LegacyNoConfigOptionsReturnsEmpty pins that when the +// legacy session/set_model RPC succeeds but the agent surfaces no per-model +// config options (and pushes no follow-up config-update notification), the +// probe treats it as a valid empty resolution (nil options, nil error) rather +// than a hard failure. This is the auggie shape: a flat model list with no +// reasoning-effort style selectors to resolve. +func TestApplyProbeModel_LegacyNoConfigOptionsReturnsEmpty(t *testing.T) { + t.Parallel() + + conn := &fakeModelConn{} + state := newACPProbeNotificationState("auggie") + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + options, err := applyProbeModel(ctx, conn, acp.SessionId("sess-1"), "legacy-model", nil, state) + if err != nil { + t.Fatalf("applyProbeModel() error = %v, want nil", err) + } + if options != nil { + t.Fatalf("options = %#v, want nil (empty resolution)", options) + } + wantCalls := []string{"legacy:sess-1:legacy-model"} + if !reflect.DeepEqual(conn.calls, wantCalls) { + t.Fatalf("calls = %#v, want %#v", conn.calls, wantCalls) + } +} + +// TestApplyProbeModel_ConfigOptionNoSnapshotReturnsError pins the other half of +// the narrowing: when the agent advertises a typed model config option (so the +// model is applied via session/set_config_option) but returns neither inline +// options nor a config-update notification, applyProbeModel must fail rather than +// keep the stale pre-switch snapshot. The empty-resolution relaxation is scoped +// to the legacy session/set_model path only. +func TestApplyProbeModel_ConfigOptionNoSnapshotReturnsError(t *testing.T) { + t.Parallel() + + modelCat := acp.SessionConfigOptionCategoryModel + conn := &fakeModelConn{} + state := newACPProbeNotificationState("modern-agent") + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + options, err := applyProbeModel(ctx, conn, acp.SessionId("sess-1"), "model-with-effort", []acp.SessionConfigOption{ + {Select: &acp.SessionConfigOptionSelect{ + Id: "model", + Category: &modelCat, + Type: "select", + }}, + }, state) + if err == nil { + t.Fatalf("applyProbeModel() error = nil, want no-configuration-options error") + } + if options != nil { + t.Fatalf("options = %#v, want nil on error", options) + } + wantCalls := []string{"config:model:model-with-effort"} + 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 {