From 0c0d9e299b56999c53f0525c23dc19e7448f5293 Mon Sep 17 00:00:00 2001 From: Adam Dalloul <47503782+Adam-Dalloul@users.noreply.github.com> Date: Thu, 10 Sep 2026 08:37:20 -0700 Subject: [PATCH 1/3] fix(agents): keep the provider you picked across an auth-method change Leaving "model_provider" auth mode drops `draft.modelProviderId`, so a save in another mode cannot persist a binding. Coming back to provider mode therefore arrives with no binding and falls into the auto-select, which took the head of the provider list. That list is ordered by row id, so the agent was silently rebound to its OLDEST provider, and the rebind rewrites the draft's model fields, its env text and its config text with that provider's values. With two providers pointing at different models, switching to the second one showed and saved the first one's model name. The panel now remembers the provider each agent was last bound to and prefers it, falling back to the head of the list only for a first-time pick or when the remembered provider is gone. Reported in #628. --- .../settings/acp-agent-settings.test.tsx | 38 +++++++++++++ .../settings/acp-agent-settings.tsx | 53 +++++++++++++++++-- 2 files changed, 86 insertions(+), 5 deletions(-) diff --git a/src/components/settings/acp-agent-settings.test.tsx b/src/components/settings/acp-agent-settings.test.tsx index 4532f2ec38..13aa41e00a 100644 --- a/src/components/settings/acp-agent-settings.test.tsx +++ b/src/components/settings/acp-agent-settings.test.tsx @@ -23,6 +23,7 @@ import { patchCodexConfigTomlText, patchEnvByImportantKey, patchImportantConfigText, + providerToRebindTo, codexSandboxSeedsAcpPreset, rebaseDeepSeekDraft, setAdapterChannel, @@ -35,6 +36,7 @@ import type { AcpAgentInfo, AdapterInfo, AgentType, + ModelProviderInfo, PreflightResult, } from "@/lib/types" @@ -148,6 +150,42 @@ function codexSandboxDraft( } } +// #628: providers arrive ordered by row id, so falling straight to the head of +// the list rebound the agent to its OLDEST provider whenever the auth-mode +// dropdown round-tripped through another mode, and the rebind copies that +// provider's model names over the one the user was on. +describe("providerToRebindTo", () => { + function provider(id: number): ModelProviderInfo { + return { + id, + name: `provider-${id}`, + agent_type: "claude_code" as AgentType, + api_url: "", + api_key: "", + model: null, + } as ModelProviderInfo + } + + it("returns to the provider the user was on, not the first one", () => { + const available = [provider(1), provider(2)] + expect(providerToRebindTo(available, 2)?.id).toBe(2) + }) + + it("falls back to the head for a first-time pick", () => { + const available = [provider(1), provider(2)] + expect(providerToRebindTo(available, null)?.id).toBe(1) + expect(providerToRebindTo(available, undefined)?.id).toBe(1) + }) + + it("falls back to the head when the remembered provider is gone", () => { + expect(providerToRebindTo([provider(3), provider(4)], 2)?.id).toBe(3) + }) + + it("has nothing to bind to when no provider exists", () => { + expect(providerToRebindTo([], 2)).toBeNull() + }) +}) + describe("buildCodexSandboxConfig — Codex sandbox/approval save patch", () => { // The core contract. The panel also sends the raw config.toml text and the // backend applies this patch last, so a field the user did not move must not diff --git a/src/components/settings/acp-agent-settings.tsx b/src/components/settings/acp-agent-settings.tsx index b046daa8b5..da8cbb5baa 100644 --- a/src/components/settings/acp-agent-settings.tsx +++ b/src/components/settings/acp-agent-settings.tsx @@ -3870,6 +3870,28 @@ export function buildAcpAdapterCheck( } } +/** + * Which provider an agent should land on when it returns to "model_provider" + * auth mode with no binding in the draft. + * + * `remembered` is the user's own last pick for this agent (or the binding + * already saved on it). Going straight to `available[0]` is #628: the list + * arrives ordered by row id, so an auth-mode round trip silently rebound the + * agent to its OLDEST provider, and the rebind copies that provider's model + * names into the draft, the env text and the config text. A remembered + * provider that is no longer in the list (deleted, or the agent changed) falls + * back to the head, which is the pre-existing behaviour for a first-time pick. + */ +export function providerToRebindTo( + available: readonly ModelProviderInfo[], + remembered: number | null | undefined +): ModelProviderInfo | null { + if (available.length === 0) return null + return ( + available.find((provider) => provider.id === remembered) ?? available[0] + ) +} + // `uvReady` reports whether the uv runtime (uvx) is installed — only meaningful // for uvx agents (custom Python-package agents; built-in Hermes moved to the // npm bridge). Derived from the uv preflight check by the caller. uvx agents @@ -5471,6 +5493,14 @@ export function AcpAgentSettings() { ) }, [modelProviders, selectedAgent]) + // The provider each agent was last bound to, remembered across auth-mode + // changes. The auth-mode handlers drop `draft.modelProviderId` whenever the + // mode leaves "model_provider", so that a save in another mode cannot + // persist a binding. Without this memory, coming BACK to provider mode falls + // through to the auto-select below, which had no record of the user's own + // choice. See `providerToRebindTo`. + const lastBoundProviderRef = useRef>>({}) + const selectedNeedsModelProvider = useMemo(() => { if (!selectedDraft) return false if (!selectedAgent) return false @@ -5902,6 +5932,9 @@ export function AcpAgentSettings() { (providerIdStr: string) => { if (!selectedAgent || !selectedDraft) return const providerId = providerIdStr ? Number(providerIdStr) : null + if (providerId != null) { + lastBoundProviderRef.current[selectedAgent.agent_type] = providerId + } const provider = providerId ? modelProviders.find((p) => p.id === providerId) : null @@ -6113,15 +6146,25 @@ export function AcpAgentSettings() { [selectedAgent, selectedDraft, modelProviders, updateSelectedDraft] ) - // Auto-select the first available provider when the user switches an agent to - // "model_provider" auth mode and hasn't picked one yet. If the list is empty, - // the existing "noModelProviderAvailable" hint handles the empty state. + // Auto-select a provider when the user switches an agent to + // "model_provider" auth mode and the draft holds no binding. If the list is + // empty, the existing "noModelProviderAvailable" hint handles the empty + // state. The user's own last pick wins over the head of the list, because an + // auth-mode round trip lands here too and rebinding to whichever provider + // happens to be first copies ITS model names over the one the user was + // actually on (#628). useEffect(() => { if (!selectedNeedsModelProvider) return if (selectedDraft?.modelProviderId != null) return - if (selectedModelProviders.length === 0) return - handleModelProviderSelect(String(selectedModelProviders[0].id)) + if (!selectedAgent) return + const remembered = + lastBoundProviderRef.current[selectedAgent.agent_type] ?? + selectedAgent.model_provider_id + const target = providerToRebindTo(selectedModelProviders, remembered) + if (!target) return + handleModelProviderSelect(String(target.id)) }, [ + selectedAgent, selectedNeedsModelProvider, selectedDraft?.modelProviderId, selectedModelProviders, From 348725b940bc32c57dd8a91c48274991709d79d3 Mon Sep 17 00:00:00 2001 From: xintaofei Date: Tue, 29 Sep 2026 07:23:05 +0800 Subject: [PATCH 2/3] fix(agents): let a saved provider binding outrank the head on rebind The auto-select resolved `lastPick ?? savedBinding` before looking at the list, so a last pick whose provider had since gone hid a saved binding that was still good and fell straight to the head. `providerToRebindTo` now owns the whole order - last pick, then saved binding, then head - taking the first candidate that is still listed, and its tests pin that order. The comments no longer attribute this to #628: the settings in that report carry the new provider's pinned model keys and only the old provider's ANTHROPIC_DEFAULT_*_MODEL_NAME display keys, which this path never touches. --- .../settings/acp-agent-settings.test.tsx | 44 ++++++++++++------- .../settings/acp-agent-settings.tsx | 42 ++++++++++-------- 2 files changed, 52 insertions(+), 34 deletions(-) diff --git a/src/components/settings/acp-agent-settings.test.tsx b/src/components/settings/acp-agent-settings.test.tsx index 13aa41e00a..ab941d169d 100644 --- a/src/components/settings/acp-agent-settings.test.tsx +++ b/src/components/settings/acp-agent-settings.test.tsx @@ -150,39 +150,53 @@ function codexSandboxDraft( } } -// #628: providers arrive ordered by row id, so falling straight to the head of -// the list rebound the agent to its OLDEST provider whenever the auth-mode -// dropdown round-tripped through another mode, and the rebind copies that -// provider's model names over the one the user was on. +// Providers arrive ordered by row id, so falling straight to the head of the +// list rebound the agent to its OLDEST provider whenever the auth-mode dropdown +// round-tripped through another mode, and the rebind copies that provider's +// model names over the one the user was on. The rendered round trip is pinned +// in acp-agent-settings.provider-rebind.test.tsx. describe("providerToRebindTo", () => { function provider(id: number): ModelProviderInfo { return { id, name: `provider-${id}`, - agent_type: "claude_code" as AgentType, api_url: "", api_key: "", + api_key_masked: "", + agent_type: "claude_code", model: null, - } as ModelProviderInfo + created_at: "", + updated_at: "", + } } + const available = [provider(1), provider(2), provider(3)] + + it("returns to the user's last pick, not the head", () => { + expect(providerToRebindTo(available, 2, null)?.id).toBe(2) + }) + + it("prefers the last pick over the binding saved on the agent", () => { + expect(providerToRebindTo(available, 3, 2)?.id).toBe(3) + }) + + it("returns to the saved binding when nothing was picked in the panel", () => { + expect(providerToRebindTo(available, undefined, 2)?.id).toBe(2) + }) - it("returns to the provider the user was on, not the first one", () => { - const available = [provider(1), provider(2)] - expect(providerToRebindTo(available, 2)?.id).toBe(2) + it("skips a last pick that is gone and returns to the saved binding", () => { + expect(providerToRebindTo(available, 9, 2)?.id).toBe(2) }) it("falls back to the head for a first-time pick", () => { - const available = [provider(1), provider(2)] - expect(providerToRebindTo(available, null)?.id).toBe(1) - expect(providerToRebindTo(available, undefined)?.id).toBe(1) + expect(providerToRebindTo(available, undefined, null)?.id).toBe(1) }) - it("falls back to the head when the remembered provider is gone", () => { - expect(providerToRebindTo([provider(3), provider(4)], 2)?.id).toBe(3) + it("falls back to the head when every candidate is gone", () => { + expect(providerToRebindTo([provider(3), provider(4)], 2, 1)?.id).toBe(3) }) it("has nothing to bind to when no provider exists", () => { - expect(providerToRebindTo([], 2)).toBeNull() + expect(providerToRebindTo([], 2, 2)).toBeNull() }) }) diff --git a/src/components/settings/acp-agent-settings.tsx b/src/components/settings/acp-agent-settings.tsx index da8cbb5baa..71e6b4b705 100644 --- a/src/components/settings/acp-agent-settings.tsx +++ b/src/components/settings/acp-agent-settings.tsx @@ -3874,22 +3874,25 @@ export function buildAcpAdapterCheck( * Which provider an agent should land on when it returns to "model_provider" * auth mode with no binding in the draft. * - * `remembered` is the user's own last pick for this agent (or the binding - * already saved on it). Going straight to `available[0]` is #628: the list - * arrives ordered by row id, so an auth-mode round trip silently rebound the - * agent to its OLDEST provider, and the rebind copies that provider's model - * names into the draft, the env text and the config text. A remembered - * provider that is no longer in the list (deleted, or the agent changed) falls - * back to the head, which is the pre-existing behaviour for a first-time pick. + * In order: `lastPick`, the user's own last pick for this agent in this panel; + * `savedBinding`, the provider the agent is bound to on disk; then the head of + * the list. Each candidate counts only while it is still listed (not deleted, + * not moved to another agent), so a stale pick falls through to a saved + * binding that is still good rather than straight to the head. Going straight + * to `available[0]` rebound the agent to its OLDEST provider (the list arrives + * ordered by row id) whenever the auth-mode dropdown round-tripped through + * another mode, and the rebind copies that provider's model names into the + * draft, the env text and the config text. The head stays the fallback for a + * first-time pick. */ export function providerToRebindTo( available: readonly ModelProviderInfo[], - remembered: number | null | undefined + lastPick: number | null | undefined, + savedBinding: number | null | undefined ): ModelProviderInfo | null { - if (available.length === 0) return null - return ( - available.find((provider) => provider.id === remembered) ?? available[0] - ) + const listed = (id: number | null | undefined) => + id == null ? undefined : available.find((provider) => provider.id === id) + return listed(lastPick) ?? listed(savedBinding) ?? available[0] ?? null } // `uvReady` reports whether the uv runtime (uvx) is installed — only meaningful @@ -6149,18 +6152,19 @@ export function AcpAgentSettings() { // Auto-select a provider when the user switches an agent to // "model_provider" auth mode and the draft holds no binding. If the list is // empty, the existing "noModelProviderAvailable" hint handles the empty - // state. The user's own last pick wins over the head of the list, because an - // auth-mode round trip lands here too and rebinding to whichever provider - // happens to be first copies ITS model names over the one the user was - // actually on (#628). + // state. The user's own last pick (then the saved binding) wins over the head + // of the list, because an auth-mode round trip lands here too and rebinding + // to whichever provider happens to be first copies ITS model names over the + // one the user was actually on. useEffect(() => { if (!selectedNeedsModelProvider) return if (selectedDraft?.modelProviderId != null) return if (!selectedAgent) return - const remembered = - lastBoundProviderRef.current[selectedAgent.agent_type] ?? + const target = providerToRebindTo( + selectedModelProviders, + lastBoundProviderRef.current[selectedAgent.agent_type], selectedAgent.model_provider_id - const target = providerToRebindTo(selectedModelProviders, remembered) + ) if (!target) return handleModelProviderSelect(String(target.id)) }, [ From 27edaf2e56cf9e568b2168fb5b7615da67d88660 Mon Sep 17 00:00:00 2001 From: xintaofei Date: Tue, 29 Sep 2026 07:23:05 +0800 Subject: [PATCH 3/3] test(agents): drive the provider rebind round trip through the panel The helper test cannot see the wiring: the panel recording the user's pick and handing the saved binding to the auto-select. These render the real settings panel, move the Claude auth mode away from "model_provider" and back, and check where the binding lands - the unsaved pick, then the saved binding, with the pick winning - and that Save persists that provider's model to both the env and the native config. --- ...cp-agent-settings.provider-rebind.test.tsx | 206 ++++++++++++++++++ 1 file changed, 206 insertions(+) create mode 100644 src/components/settings/acp-agent-settings.provider-rebind.test.tsx diff --git a/src/components/settings/acp-agent-settings.provider-rebind.test.tsx b/src/components/settings/acp-agent-settings.provider-rebind.test.tsx new file mode 100644 index 0000000000..fddf07b8c1 --- /dev/null +++ b/src/components/settings/acp-agent-settings.provider-rebind.test.tsx @@ -0,0 +1,206 @@ +/** + * Leaving "model_provider" auth mode drops the draft's provider binding (a save + * in another mode must not persist one), so coming back falls into the + * auto-select. That auto-select used to take the head of the provider list — + * the OLDEST provider, since the list is ordered by row id — and the rebind is + * provider-authoritative: it rewrites the model fields, the env text and the + * config text from whichever provider it lands on. These drive the real panel + * through that round trip and pin where it lands: the user's own last pick, + * then the binding saved on the agent, and only then the head. + */ +import { render, screen, waitFor, within } from "@testing-library/react" +import userEvent from "@testing-library/user-event" +import { NextIntlClientProvider } from "next-intl" +import { beforeEach, describe, expect, it, vi } from "vitest" + +import enMessages from "@/i18n/messages/en.json" +import { + acpListAgents, + acpUpdateAgentConfig, + acpUpdateAgentEnv, + listModelProviders, +} from "@/lib/api" +import type { AcpAgentInfo, ModelProviderInfo } from "@/lib/types" + +import { AcpAgentSettings } from "./acp-agent-settings" + +vi.mock("@/lib/api", async (importOriginal) => { + const actual = await importOriginal>() + // Every function is stubbed so nothing here can reach the real transport; + // the calls the panel makes on mount that these tests don't need (preflight, + // catalogs) simply never settle. + return Object.fromEntries( + Object.entries(actual).map(([name, value]) => [ + name, + typeof value === "function" ? vi.fn(() => new Promise(() => {})) : value, + ]) + ) +}) +vi.mock("next/navigation", () => { + const params = new URLSearchParams() + return { useSearchParams: () => params } +}) + +function claudeAgent(overrides: Partial = {}): AcpAgentInfo { + return { + agent_type: "claude_code", + skills_capable: true, + registry_id: "claude-code", + registry_version: "1.0.0", + supports_custom_version: false, + name: "Claude Code", + description: "", + available: true, + distribution_type: "npx", + is_acp_adapter: true, + custom_source: null, + enabled: true, + sort_order: 0, + installed_version: null, + host_tools_agent_mode: false, + env: {}, + config_json: null, + config_file_path: null, + opencode_auth_json: null, + codex_auth_json: null, + cline_secrets_json: null, + codex_config_toml: null, + codex_model_catalog: null, + codex_sandbox_settings: null, + grok_config_toml: null, + grok_settings: null, + hermes_config_yaml: null, + cursor_cli_config_json: null, + cursor_settings: null, + model_provider_id: null, + icon_url: null, + ...overrides, + } +} + +function provider(id: number, letter: string): ModelProviderInfo { + return { + id, + name: `Provider ${letter}`, + api_url: `https://gateway-${letter.toLowerCase()}.test`, + api_key: `key-${letter.toLowerCase()}`, + api_key_masked: "", + agent_type: "claude_code", + model: JSON.stringify({ main: `model-${letter.toLowerCase()}` }), + created_at: "", + updated_at: "", + } +} + +// Row-id order, as `list_all` returns them: A is the oldest, so the head. +const PROVIDERS = [provider(1, "A"), provider(2, "B"), provider(3, "C")] + +function renderPanel() { + return render( + + + + ) +} + +/** The select that sits under a visible `