From eb6dffdbc6fac93f810f910b2d0415b23ad6fb6c Mon Sep 17 00:00:00 2001 From: Jiayuan Zhang Date: Wed, 17 Jun 2026 08:23:20 +0200 Subject: [PATCH] MUL-3341: clear incompatible model on runtime switch Closes MUL-3341 --- server/internal/handler/agent.go | 33 +++-- .../internal/handler/agent_thinking_test.go | 132 ++++++++++++++++++ server/pkg/agent/models.go | 61 ++++++++ server/pkg/agent/models_test.go | 72 ++++++++++ 4 files changed, 290 insertions(+), 8 deletions(-) diff --git a/server/internal/handler/agent.go b/server/internal/handler/agent.go index 68e825e0ac..4f37fcab45 100644 --- a/server/internal/handler/agent.go +++ b/server/internal/handler/agent.go @@ -1059,6 +1059,7 @@ func (h *Handler) UpdateAgent(w http.ResponseWriter, r *http.Request) { // request doesn't move the agent, we still need to load the *current* // runtime to validate a thinking_level change. Resolve once and reuse. targetRuntimeID := existing.RuntimeID + targetProvider := "" if req.RuntimeID != nil { runtimeUUID, ok := parseUUIDOrBadRequest(w, *req.RuntimeID, "runtime_id") if !ok { @@ -1086,6 +1087,7 @@ func (h *Handler) UpdateAgent(w http.ResponseWriter, r *http.Request) { params.RuntimeID = runtime.ID params.RuntimeMode = pgtype.Text{String: runtime.RuntimeMode, Valid: true} targetRuntimeID = runtime.ID + targetProvider = runtime.Provider } if req.Visibility != nil { params.Visibility = pgtype.Text{String: *req.Visibility, Valid: true} @@ -1098,6 +1100,13 @@ func (h *Handler) UpdateAgent(w http.ResponseWriter, r *http.Request) { } if req.Model != nil { params.Model = pgtype.Text{String: *req.Model, Valid: true} + } else if req.RuntimeID != nil && existing.Model.Valid && agent.ModelKnownIncompatibleWithProvider(targetProvider, existing.Model.String) { + // Model is runtime-native. When moving an agent across known provider + // families and the caller did not choose a replacement model, clear the + // old value so the new runtime falls back to its own default instead of + // receiving an obvious foreign model ID (e.g. Claude Code -> Codex). + // Unknown/custom model strings are preserved by the helper. + params.Model = pgtype.Text{String: "", Valid: true} } // thinking_level handling (MUL-2339). Tri-state semantics: @@ -1121,10 +1130,14 @@ func (h *Handler) UpdateAgent(w http.ResponseWriter, r *http.Request) { // Need the target runtime's provider to validate. Re-fetch only when // we haven't already loaded it above (i.e. the request didn't change // runtime_id), to keep the no-change path one DB roundtrip. - provider, ok := h.resolveAgentProvider(r, existing.WorkspaceID, targetRuntimeID) - if !ok { - writeError(w, http.StatusInternalServerError, "failed to resolve runtime for thinking_level validation") - return + provider := targetProvider + if provider == "" { + var ok bool + provider, ok = h.resolveAgentProvider(r, existing.WorkspaceID, targetRuntimeID) + if !ok { + writeError(w, http.StatusInternalServerError, "failed to resolve runtime for thinking_level validation") + return + } } if !agent.IsKnownThinkingValue(provider, value) { writeError(w, http.StatusBadRequest, fmt.Sprintf("thinking_level %q is not a recognised value for runtime %q", value, provider)) @@ -1140,10 +1153,14 @@ func (h *Handler) UpdateAgent(w http.ResponseWriter, r *http.Request) { // literal-invalid, never silently coerce. The caller can either // pass `thinking_level: ""` to clear or pick a value valid for the // new runtime. - provider, ok := h.resolveAgentProvider(r, existing.WorkspaceID, targetRuntimeID) - if !ok { - writeError(w, http.StatusInternalServerError, "failed to resolve runtime for thinking_level validation") - return + provider := targetProvider + if provider == "" { + var ok bool + provider, ok = h.resolveAgentProvider(r, existing.WorkspaceID, targetRuntimeID) + if !ok { + writeError(w, http.StatusInternalServerError, "failed to resolve runtime for thinking_level validation") + return + } } if !agent.IsKnownThinkingValue(provider, existing.ThinkingLevel.String) { writeError(w, http.StatusBadRequest, fmt.Sprintf( diff --git a/server/internal/handler/agent_thinking_test.go b/server/internal/handler/agent_thinking_test.go index 0119814fde..0b31230019 100644 --- a/server/internal/handler/agent_thinking_test.go +++ b/server/internal/handler/agent_thinking_test.go @@ -295,6 +295,117 @@ func TestUpdateAgent_RuntimeSwitch_PreservesValidValueRejectsInvalid(t *testing. }) } +// TestUpdateAgent_RuntimeSwitch_ClearsKnownIncompatibleModel covers the +// runtime/model persistence bug from MUL-3341: a runtime_id-only PATCH used +// to preserve a provider-native model string, so switching a Claude Code +// agent to Codex could leave agent.model = "claude-..." and fail at task +// execution. Unknown custom models are intentionally preserved because the +// API supports manual entries and cannot prove they are invalid. +func TestUpdateAgent_RuntimeSwitch_ClearsKnownIncompatibleModel(t *testing.T) { + if testHandler == nil { + t.Skip("database not available") + } + + ctx := context.Background() + claudeRuntimeID := createClaudeProviderRuntime(t) + codexRuntimeID := createCodexProviderRuntime(t) + + t.Cleanup(func() { + testPool.Exec(ctx, `DELETE FROM agent WHERE workspace_id = $1 AND name LIKE 'runtime-model-switch-%'`, testWorkspaceID) + }) + + t.Run("runtime-only switch clears known foreign model", func(t *testing.T) { + agentID := createAgentOnRuntimeWithModel(t, "runtime-model-switch-clear", claudeRuntimeID, "claude-sonnet-4-6") + body := map[string]any{ + "runtime_id": codexRuntimeID, + } + w := httptest.NewRecorder() + req := withURLParam(newRequest(http.MethodPatch, "/api/agents/"+agentID, body), "id", agentID) + testHandler.UpdateAgent(w, req) + if w.Code != http.StatusOK { + t.Fatalf("expected 200 switching runtime with incompatible model, got %d: %s", w.Code, w.Body.String()) + } + var resp map[string]any + _ = json.NewDecoder(w.Body).Decode(&resp) + if resp["model"] != "" { + t.Errorf("expected model cleared across Claude->Codex runtime switch, got %v", resp["model"]) + } + }) + + t.Run("runtime-only switch clears provider-prefixed model not accepted by target", func(t *testing.T) { + agentID := createAgentOnRuntimeWithModel(t, "runtime-model-switch-prefixed", claudeRuntimeID, "openai/gpt-4o") + body := map[string]any{ + "runtime_id": codexRuntimeID, + } + w := httptest.NewRecorder() + req := withURLParam(newRequest(http.MethodPatch, "/api/agents/"+agentID, body), "id", agentID) + testHandler.UpdateAgent(w, req) + if w.Code != http.StatusOK { + t.Fatalf("expected 200 switching runtime with provider-prefixed model, got %d: %s", w.Code, w.Body.String()) + } + var resp map[string]any + _ = json.NewDecoder(w.Body).Decode(&resp) + if resp["model"] != "" { + t.Errorf("expected provider-prefixed model cleared across runtime switch, got %v", resp["model"]) + } + }) + + t.Run("runtime-only switch keeps exact target accepted model", func(t *testing.T) { + agentID := createAgentOnRuntimeWithModel(t, "runtime-model-switch-accepted", claudeRuntimeID, "gpt-5.5") + body := map[string]any{ + "runtime_id": codexRuntimeID, + } + w := httptest.NewRecorder() + req := withURLParam(newRequest(http.MethodPatch, "/api/agents/"+agentID, body), "id", agentID) + testHandler.UpdateAgent(w, req) + if w.Code != http.StatusOK { + t.Fatalf("expected 200 preserving exact target accepted model, got %d: %s", w.Code, w.Body.String()) + } + var resp map[string]any + _ = json.NewDecoder(w.Body).Decode(&resp) + if resp["model"] != "gpt-5.5" { + t.Errorf("expected exact target model preserved, got %v", resp["model"]) + } + }) + + t.Run("explicit replacement model wins during switch", func(t *testing.T) { + agentID := createAgentOnRuntimeWithModel(t, "runtime-model-switch-replace", claudeRuntimeID, "claude-sonnet-4-6") + body := map[string]any{ + "runtime_id": codexRuntimeID, + "model": "gpt-5.5", + } + w := httptest.NewRecorder() + req := withURLParam(newRequest(http.MethodPatch, "/api/agents/"+agentID, body), "id", agentID) + testHandler.UpdateAgent(w, req) + if w.Code != http.StatusOK { + t.Fatalf("expected 200 with explicit replacement model, got %d: %s", w.Code, w.Body.String()) + } + var resp map[string]any + _ = json.NewDecoder(w.Body).Decode(&resp) + if resp["model"] != "gpt-5.5" { + t.Errorf("expected explicit model to be persisted, got %v", resp["model"]) + } + }) + + t.Run("unknown custom model is preserved", func(t *testing.T) { + agentID := createAgentOnRuntimeWithModel(t, "runtime-model-switch-custom", claudeRuntimeID, "private-lab-model") + body := map[string]any{ + "runtime_id": codexRuntimeID, + } + w := httptest.NewRecorder() + req := withURLParam(newRequest(http.MethodPatch, "/api/agents/"+agentID, body), "id", agentID) + testHandler.UpdateAgent(w, req) + if w.Code != http.StatusOK { + t.Fatalf("expected 200 preserving unknown custom model, got %d: %s", w.Code, w.Body.String()) + } + var resp map[string]any + _ = json.NewDecoder(w.Body).Decode(&resp) + if resp["model"] != "private-lab-model" { + t.Errorf("expected unknown custom model preserved, got %v", resp["model"]) + } + }) +} + // createCodexProviderRuntime mirrors createClaudeProviderRuntime but for // the codex provider, so runtime-switch tests can exercise a real // cross-provider transition. @@ -370,3 +481,24 @@ func createAgentOnRuntime(t *testing.T, name, runtimeID, level string) string { }) return agentID } + +func createAgentOnRuntimeWithModel(t *testing.T, name, runtimeID, model string) string { + t.Helper() + var agentID string + err := testPool.QueryRow(context.Background(), ` + INSERT INTO agent ( + workspace_id, name, description, runtime_mode, runtime_config, + runtime_id, visibility, max_concurrent_tasks, owner_id, + instructions, custom_env, custom_args, model + ) + VALUES ($1, $2, '', 'cloud', '{}'::jsonb, $3, 'private', 1, $4, '', '{}'::jsonb, '[]'::jsonb, $5) + RETURNING id + `, testWorkspaceID, name, runtimeID, testUserID, model).Scan(&agentID) + if err != nil { + t.Fatalf("create agent on runtime %s with model %s: %v", runtimeID, model, err) + } + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM agent WHERE id = $1`, agentID) + }) + return agentID +} diff --git a/server/pkg/agent/models.go b/server/pkg/agent/models.go index bfc9e1076d..7c99216df8 100644 --- a/server/pkg/agent/models.go +++ b/server/pkg/agent/models.go @@ -172,6 +172,67 @@ func ModelSelectionSupported(providerType string) bool { return true } +// ModelKnownIncompatibleWithProvider reports whether a saved model is a known +// mismatch for a target runtime provider. For first-party providers with +// maintained static catalogs, compatibility is exact: the model must be one of +// the IDs that runtime advertises. Unknown/custom model strings still return +// false because the UI and CLI allow manual entries and the server should not +// erase values it cannot confidently classify. +func ModelKnownIncompatibleWithProvider(providerType, model string) bool { + model = strings.TrimSpace(model) + if model == "" { + return false + } + + accepted, ok := acceptedModelIDsForProvider(providerType) + if !ok { + return false + } + if accepted[model] { + return false + } + return isRuntimeSpecificModelID(model) +} + +func acceptedModelIDsForProvider(providerType string) (map[string]bool, bool) { + switch { + case providerType == "claude": + return modelIDSet(claudeStaticModels()), true + case providerType == "codex": + return modelIDSet(codexStaticModels()), true + case providerType == "gemini": + return modelIDSet(geminiStaticModels()), true + default: + return nil, false + } +} + +func modelIDSet(models []Model) map[string]bool { + out := make(map[string]bool, len(models)) + for _, m := range models { + out[m.ID] = true + } + return out +} + +func isRuntimeSpecificModelID(model string) bool { + if strings.Contains(model, "/") { + return true + } + return modelHasKnownPrefix(model) || + modelIDSet(claudeStaticModels())[model] || + modelIDSet(codexStaticModels())[model] || + modelIDSet(geminiStaticModels())[model] +} + +func modelHasKnownPrefix(model string) bool { + return strings.HasPrefix(model, "claude-") || + strings.HasPrefix(model, "gpt-") || + strings.HasPrefix(model, "gemini-") || + strings.HasPrefix(model, "auto-gemini-") || + isOpenAIReasoningSeriesID(model) +} + // cachedDiscovery invokes fn and caches the result for modelCacheTTL. // The cache is keyed on providerType only; callers that need to // distinguish discovery by host/user should include that in the key diff --git a/server/pkg/agent/models_test.go b/server/pkg/agent/models_test.go index 45273dd8c7..04e38df28c 100644 --- a/server/pkg/agent/models_test.go +++ b/server/pkg/agent/models_test.go @@ -149,6 +149,78 @@ func TestCodexStaticModelsExposesGPT55(t *testing.T) { } } +func TestModelKnownIncompatibleWithProvider(t *testing.T) { + cases := []struct { + name string + provider string + model string + want bool + }{ + { + name: "claude model is incompatible with codex", + provider: "codex", + model: "claude-sonnet-4-6", + want: true, + }, + { + name: "codex model is compatible with codex", + provider: "codex", + model: "gpt-5.5", + want: false, + }, + { + name: "codex model is incompatible with claude", + provider: "claude", + model: "o3", + want: true, + }, + { + name: "exact claude model is compatible with claude", + provider: "claude", + model: "claude-opus-4-7", + want: false, + }, + { + name: "provider-prefixed openai model is incompatible with codex", + provider: "codex", + model: "openai/gpt-4o", + want: true, + }, + { + name: "provider-prefixed anthropic model is incompatible with claude", + provider: "claude", + model: "anthropic/claude-opus-4.7", + want: true, + }, + { + name: "known openai-looking model outside codex catalog is incompatible", + provider: "codex", + model: "gpt-99", + want: true, + }, + { + name: "unknown custom model is not classified", + provider: "codex", + model: "private-lab-model", + want: false, + }, + { + name: "unknown target provider does not clear", + provider: "opencode", + model: "claude-sonnet-4-6", + want: false, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := ModelKnownIncompatibleWithProvider(tc.provider, tc.model); got != tc.want { + t.Fatalf("ModelKnownIncompatibleWithProvider(%q, %q) = %v, want %v", tc.provider, tc.model, got, tc.want) + } + }) + } +} + func TestInferCopilotProvider(t *testing.T) { cases := map[string]string{ "gpt-5.5": "openai",