From c4b3823bebfe2d594d259ee3b8a56dd49541753c Mon Sep 17 00:00:00 2001 From: Debug Agent Date: Fri, 22 May 2026 17:21:06 +0200 Subject: [PATCH 1/6] feat(acp): save concrete model defaults in canvas --- __tests__/api/agent-server-adapter.test.ts | 76 +++++++++----- .../components/chat-input-actions.test.tsx | 28 +++++- .../chat/components/chat-input-model.test.tsx | 56 +++++++++++ .../chat/switch-profile-button.test.tsx | 4 +- .../onboarding/choose-agent-step.test.tsx | 5 +- __tests__/constants/acp-providers.test.ts | 49 ++++++++- __tests__/routes/agent-settings.test.tsx | 56 ++++++++++- src/api/agent-server-adapter.ts | 52 +++++++--- ...agent-server-conversation-service.types.ts | 4 +- .../chat/components/chat-input-actions.tsx | 18 +++- .../chat/components/chat-input-model.tsx | 45 ++++----- .../features/chat/switch-profile-button.tsx | 5 +- src/constants/acp-providers.ts | 70 ++++++++++++- src/i18n/translation.json | 47 ++++++--- src/routes/agent-settings.tsx | 99 ++++++++++++++++--- 15 files changed, 513 insertions(+), 101 deletions(-) diff --git a/__tests__/api/agent-server-adapter.test.ts b/__tests__/api/agent-server-adapter.test.ts index dadff5853..d178a1124 100644 --- a/__tests__/api/agent-server-adapter.test.ts +++ b/__tests__/api/agent-server-adapter.test.ts @@ -554,19 +554,59 @@ describe("toAppConversation", () => { expect(result.llm_model).toBe("claude-sonnet-4-6"); }); - it("marks ACP conversations and nulls llm_model so the chat UI can't mislead", () => { - // The SDK's ACPAgent carries a sentinel ``llm`` (``acp-managed``) for - // cost-attribution only; the *real* model lives on the ACP subprocess via - // ``acp_model`` and isn't surfaced on ``agent.llm.model``. Surfacing the - // sentinel as ``llm_model`` would let SwitchProfileButton render an - // affordance to "change the model" on a Claude-Code conversation while - // the running subprocess kept its own — a confusing silent no-op. + it("marks ACP conversations and surfaces the configured acp_model", () => { + // The SDK's ACPAgent may still carry a sentinel ``llm`` (``acp-managed``) + // for cost-attribution. Consumers should see the concrete ACP model Canvas + // configured, while SwitchProfileButton remains gated by agent_kind. const result = toAppConversation({ ...baseInfo, - agent: { kind: "ACPAgent", llm: { model: "acp-managed" } }, + agent: { + kind: "ACPAgent", + acp_model: "claude-sonnet-4-6", + llm: { model: "acp-managed" }, + }, + }); + expect(result.agent_kind).toBe("acp"); + expect(result.llm_model).toBe("claude-sonnet-4-6"); + }); + + it("prefers ACP runtime model fields over configured acp_model", () => { + const result = toAppConversation({ + ...baseInfo, + current_model_id: "claude-sonnet-4-6", + current_model_name: "Claude Sonnet 4.6", + agent: { + kind: "ACPAgent", + acp_model: "claude-opus-4-7", + llm: { model: "acp-managed" }, + }, }); expect(result.agent_kind).toBe("acp"); - expect(result.llm_model).toBeNull(); + expect(result.llm_model).toBe("Claude Sonnet 4.6"); + }); + + it("does not surface ACP default placeholders when a configured model exists", () => { + const result = toAppConversation({ + ...baseInfo, + current_model_id: "default", + current_model_name: "Default (recommended)", + agent: { + kind: "ACPAgent", + acp_model: "claude-sonnet-4-6", + llm: { model: "acp-managed" }, + }, + }); + expect(result.agent_kind).toBe("acp"); + expect(result.llm_model).toBe("claude-sonnet-4-6"); + }); + + it("falls back to a non-sentinel ACP llm.model for SDKs that mirror acp_model there", () => { + const result = toAppConversation({ + ...baseInfo, + agent: { kind: "ACPAgent", llm: { model: "claude-sonnet-4-6" } }, + }); + expect(result.agent_kind).toBe("acp"); + expect(result.llm_model).toBe("claude-sonnet-4-6"); }); it("surfaces acp_server from tags.acpserver for ACP conversations", () => { @@ -647,8 +687,7 @@ describe("buildRuntimeServicesSystemSuffix", () => { url_from_agent: "http://localhost:18001", api_prefix: "/api/automation", docs_url: "http://localhost:18001/api/automation/docs", - openapi_url: - "http://localhost:18001/api/automation/openapi.json", + openapi_url: "http://localhost:18001/api/automation/openapi.json", auth_env_var: "OPENHANDS_AUTOMATION_API_KEY", }, }, @@ -660,9 +699,7 @@ describe("buildRuntimeServicesSystemSuffix", () => { expect(suffix).toContain("dev:automation"); expect(suffix).toContain("http://localhost:18000"); expect(suffix).toContain("http://localhost:18001"); - expect(suffix).toContain( - "http://localhost:18001/api/automation/docs", - ); + expect(suffix).toContain("http://localhost:18001/api/automation/docs"); expect(suffix).toContain("X-API-Key: $OPENHANDS_AUTOMATION_API_KEY"); expect(suffix).toContain(""); // The "don't guess" line should reference the actual agent-server URL @@ -1007,7 +1044,7 @@ describe("buildStartConversationRequest — ACP discriminator", () => { expect(payload.agent.acp_command).toEqual([]); }); - it("treats acp_model: '' (empty string) as 'no override'", () => { + it("omits acp_model when settings contains an empty string", () => { // The form may carry an empty string after a user clears the model // input; the agent-server expects ``null`` for "use provider default." // Empty strings would pass the spawn but bias model selection on @@ -1028,14 +1065,7 @@ describe("buildStartConversationRequest — ACP discriminator", () => { agent: Record & { acp_model?: unknown }; }; - // ``buildConfiguredAcpAgentSettings`` already filters undefined + - // null; the empty string falls through, which is a known nit. The - // adapter's contract is "forward what settings says"; the - // canonicalisation belongs in the save path - // (``agent-settings.tsx::handleSave`` already does - // ``acpModel.trim() || null``). Pin the current behaviour so a - // future change to either side is a deliberate decision. - expect(payload.agent.acp_model).toBe(""); + expect(payload.agent.acp_model).toBeUndefined(); }); it("ACP → OpenHands → ACP round trip leaves no field leakage", () => { diff --git a/__tests__/components/features/chat/components/chat-input-actions.test.tsx b/__tests__/components/features/chat/components/chat-input-actions.test.tsx index 04bafd7ef..a2ebc143e 100644 --- a/__tests__/components/features/chat/components/chat-input-actions.test.tsx +++ b/__tests__/components/features/chat/components/chat-input-actions.test.tsx @@ -12,7 +12,13 @@ import type { Backend } from "#/api/backend-registry/types"; const useActiveConversationMock = vi.fn< () => { - data: { conversation_id: string; llm_model: string | null } | undefined; + data: + | { + conversation_id: string; + agent_kind?: "openhands" | "acp"; + llm_model: string | null; + } + | undefined; } >(() => ({ data: undefined })); @@ -78,6 +84,26 @@ describe("ChatInputActions", () => { ).not.toBeInTheDocument(); }); + it("renders the static model label for local ACP conversations", () => { + useActiveConversationMock.mockReturnValue({ + data: { + conversation_id: "test-conversation-id", + agent_kind: "acp", + llm_model: "claude-sonnet-4-6", + }, + }); + + renderWithProviders(); + + expect(screen.getByTestId("chat-input-llm-model")).toHaveAttribute( + "title", + "claude-sonnet-4-6", + ); + expect( + screen.queryByTestId("switch-profile-button-stub"), + ).not.toBeInTheDocument(); + }); + it("renders the active conversation model on a cloud backend", () => { setRegisteredBackends([cloudBackend]); setActiveSelection({ backendId: cloudBackend.id }); diff --git a/__tests__/components/features/chat/components/chat-input-model.test.tsx b/__tests__/components/features/chat/components/chat-input-model.test.tsx index 870e326eb..c5cad9a2b 100644 --- a/__tests__/components/features/chat/components/chat-input-model.test.tsx +++ b/__tests__/components/features/chat/components/chat-input-model.test.tsx @@ -62,6 +62,42 @@ describe("ChatInputModel", () => { ).not.toBeInTheDocument(); }); + it("renders an ACP conversation model and links to Agent settings", () => { + useActiveConversationMock.mockReturnValue({ + data: { + conversation_id: "test-conversation-id", + agent_kind: "acp", + llm_model: "claude-sonnet-4-6", + }, + }); + + renderWithProviders(); + + const model = screen.getByTestId("chat-input-llm-model"); + expect(model).toHaveAttribute("title", "claude-sonnet-4-6"); + fireEvent.click(model); + expect(screen.getByRole("link")).toHaveAttribute("href", "/settings/agent"); + }); + + it("does not fall back to the OpenHands settings model for active ACP conversations", () => { + useActiveConversationMock.mockReturnValue({ + data: { + conversation_id: "test-conversation-id", + agent_kind: "acp", + llm_model: null, + }, + }); + useSettingsMock.mockReturnValue({ + data: { llm_model: "openai/gpt-4o" }, + }); + + renderWithProviders(); + + expect( + screen.queryByTestId("chat-input-llm-model"), + ).not.toBeInTheDocument(); + }); + it("falls back to the user's default model from settings when there is no active conversation", () => { // Arrange — home page render: no conversation yet, but the user has // a default model configured. The switcher should still show. @@ -80,6 +116,26 @@ describe("ChatInputModel", () => { ); }); + it("uses the ACP settings model on the home page when ACP is active", () => { + useActiveConversationMock.mockReturnValue({ data: undefined }); + useSettingsMock.mockReturnValue({ + data: { + llm_model: "openai/gpt-4o", + agent_settings: { + agent_kind: "acp", + acp_model: "gemini-2.5-pro", + }, + }, + }); + + renderWithProviders(); + + const model = screen.getByTestId("chat-input-llm-model"); + expect(model).toHaveAttribute("title", "gemini-2.5-pro"); + fireEvent.click(model); + expect(screen.getByRole("link")).toHaveAttribute("href", "/settings/agent"); + }); + it("renders nothing when neither the conversation nor settings provide an llm_model", () => { useActiveConversationMock.mockReturnValue({ data: undefined }); useSettingsMock.mockReturnValue({ data: undefined }); diff --git a/__tests__/components/features/chat/switch-profile-button.test.tsx b/__tests__/components/features/chat/switch-profile-button.test.tsx index 7219fbf27..be0227600 100644 --- a/__tests__/components/features/chat/switch-profile-button.test.tsx +++ b/__tests__/components/features/chat/switch-profile-button.test.tsx @@ -123,7 +123,7 @@ describe("SwitchProfileButton", () => { expect(screen.getByTestId("switch-profile-button")).toBeDisabled(); }); - it("renders nothing for ACP conversations even when profiles exist", () => { + it("renders nothing for ACP conversations even when profiles and a display model exist", () => { // ACPAgent conversations route prompts to a CLI subprocess whose model is // set via ``acp_model`` in Settings → Agent, not by the LLM-profile // picker. Letting the user "switch the LLM" here would silently no-op @@ -133,7 +133,7 @@ describe("SwitchProfileButton", () => { data: { id: "conv-1", agent_kind: "acp", - llm_model: null, + llm_model: "claude-sonnet-4-6", }, }); diff --git a/__tests__/components/onboarding/choose-agent-step.test.tsx b/__tests__/components/onboarding/choose-agent-step.test.tsx index b7dcf9a40..263a94353 100644 --- a/__tests__/components/onboarding/choose-agent-step.test.tsx +++ b/__tests__/components/onboarding/choose-agent-step.test.tsx @@ -168,7 +168,7 @@ describe("ChooseAgentStep", () => { // ``acp_args`` can't survive and concatenate onto the spawn // command at conversation-create time. acp_args: [], - acp_model: null, + acp_model: "claude-sonnet-4-6", }); }); @@ -191,6 +191,9 @@ describe("ChooseAgentStep", () => { expect( (call.agent_settings_diff as Record).acp_server, ).toBe(expected); + expect( + (call.agent_settings_diff as Record).acp_model, + ).toBe(ACP_PROVIDERS.find(({ key }) => key === expected)?.default_model); }); it("rebuilds the diff cleanly when the user flips between ACP providers", async () => { diff --git a/__tests__/constants/acp-providers.test.ts b/__tests__/constants/acp-providers.test.ts index 8c9fe2877..fb61c78cd 100644 --- a/__tests__/constants/acp-providers.test.ts +++ b/__tests__/constants/acp-providers.test.ts @@ -1,5 +1,10 @@ import { describe, expect, it } from "vitest"; -import { getAcpProviderDisplayName } from "#/constants/acp-providers"; +import { + ACP_CUSTOM_PRESET_KEY, + ACP_PROVIDERS, + buildAcpAgentSettingsDiff, + getAcpProviderDisplayName, +} from "#/constants/acp-providers"; describe("getAcpProviderDisplayName", () => { it("resolves the three built-in registry keys to their human names", () => { @@ -28,3 +33,45 @@ describe("getAcpProviderDisplayName", () => { expect(getAcpProviderDisplayName("")).toBeNull(); }); }); + +describe("ACP provider registry", () => { + it("keeps every built-in default model in the UX suggestions", () => { + for (const provider of ACP_PROVIDERS) { + expect(provider.default_model, provider.key).toBeTruthy(); + expect(provider.available_models, provider.key).toBeTruthy(); + expect( + provider.available_models?.some( + (model) => model.id === provider.default_model, + ), + provider.key, + ).toBe(true); + } + }); + + it("does not suggest generic default model placeholders", () => { + for (const provider of ACP_PROVIDERS) { + for (const model of provider.available_models ?? []) { + expect(model.id.toLowerCase()).not.toBe("default"); + expect(model.label.toLowerCase()).not.toContain("default"); + } + } + }); + + it("seeds built-in ACP diffs with the provider default model", () => { + for (const provider of ACP_PROVIDERS) { + expect(buildAcpAgentSettingsDiff(provider.key)).toMatchObject({ + agent_kind: "acp", + acp_server: provider.key, + acp_model: provider.default_model, + }); + } + }); + + it("keeps custom ACP diffs model-optional", () => { + expect(buildAcpAgentSettingsDiff(ACP_CUSTOM_PRESET_KEY)).toMatchObject({ + agent_kind: "acp", + acp_server: ACP_CUSTOM_PRESET_KEY, + acp_model: null, + }); + }); +}); diff --git a/__tests__/routes/agent-settings.test.tsx b/__tests__/routes/agent-settings.test.tsx index 3c7968511..f5045a094 100644 --- a/__tests__/routes/agent-settings.test.tsx +++ b/__tests__/routes/agent-settings.test.tsx @@ -177,6 +177,57 @@ describe("AgentSettingsScreen", () => { expect(modelInput.value).toBe("claude-opus-4-5"); }); + it("defaults built-in ACP providers to a suggested model when none is saved", async () => { + vi.spyOn(SettingsService, "getSettings").mockResolvedValue( + buildSettings({ + agent_settings: { + schema_version: 1, + agent_kind: "acp", + acp_server: "claude-code", + acp_command: [], + acp_model: null, + }, + }), + ); + + renderAgentSettingsScreen(); + + await screen.findByTestId("agent-command-input"); + expect(screen.getByLabelText("SETTINGS$AGENT_MODEL")).toHaveValue( + "Claude Sonnet 4.6", + ); + }); + + it("saves the selected built-in ACP model", async () => { + const user = userEvent.setup(); + vi.spyOn(SettingsService, "getSettings").mockResolvedValue( + buildSettings({ + agent_settings: { + schema_version: 1, + agent_kind: "acp", + acp_server: "claude-code", + acp_command: [], + acp_model: null, + }, + }), + ); + const save = vi.spyOn(SettingsService, "saveSettings"); + + renderAgentSettingsScreen(); + await screen.findByTestId("agent-command-input"); + await user.click(screen.getByLabelText("SETTINGS$AGENT_MODEL")); + await user.click(await screen.findByText("Claude Opus 4.7")); + await user.click(screen.getByTestId("agent-save-button")); + + await waitFor(() => { + expect(save).toHaveBeenCalledTimes(1); + }); + const call = save.mock.calls[0]?.[0] as { + agent_settings_diff?: Record; + }; + expect(call.agent_settings_diff?.acp_model).toBe("claude-opus-4-7"); + }); + it("saves an ACP diff when switching to ACP + Claude Code", async () => { const user = userEvent.setup(); vi.spyOn(SettingsService, "getSettings").mockResolvedValue( @@ -205,6 +256,9 @@ describe("AgentSettingsScreen", () => { expect(commandInput.value).toBe( "npx -y @agentclientprotocol/claude-agent-acp", ); + expect(screen.getByLabelText("SETTINGS$AGENT_MODEL")).toHaveValue( + "Claude Sonnet 4.6", + ); await user.click(screen.getByTestId("agent-save-button")); @@ -225,7 +279,7 @@ describe("AgentSettingsScreen", () => { // ``acp_args`` can't survive and concatenate onto the spawn // command at conversation-create time. acp_args: [], - acp_model: null, + acp_model: "claude-sonnet-4-6", }); }); diff --git a/src/api/agent-server-adapter.ts b/src/api/agent-server-adapter.ts index 5feb1d611..888c35455 100644 --- a/src/api/agent-server-adapter.ts +++ b/src/api/agent-server-adapter.ts @@ -43,15 +43,18 @@ export interface DirectConversationInfo { * conversation runs an ACP CLI subprocess (model selection lives on * the subprocess via ``acp_model``, not on ``agent.llm``); ``"Agent"`` * means the conversation drives an LLM directly through litellm. - * Used by ``toAppConversation`` to null out ``llm_model`` for ACP - * conversations so the chat UI doesn't expose LLM-switch affordances - * that would silently no-op against the running ACP subprocess. + * Used by ``toAppConversation`` to mark ACP conversations so LLM-switch + * affordances stay gated even when ``llm_model`` carries the ACP display + * model. */ kind?: string | null; + acp_model?: string | null; llm?: { model?: string | null; } | null; } | null; + current_model_id?: string | null; + current_model_name?: string | null; workspace?: { working_dir?: string | null; } | null; @@ -242,24 +245,42 @@ export function getDefaultConversationTitle(conversationId: string): string { return `Conversation ${conversationId.slice(0, 5)}`; } +function nonEmptyString(value: unknown): string | null { + if (typeof value !== "string") return null; + const trimmed = value.trim(); + return trimmed.length > 0 ? trimmed : null; +} + +function isAcpDefaultPlaceholder(value: string): boolean { + const normalized = value.trim().toLowerCase(); + return normalized === "default" || normalized === "default (recommended)"; +} + export function toAppConversation( info: DirectConversationInfo, ): AppConversation { const metadata = getStoredConversationMetadata(info.id); - // ACPAgent conversations carry a dummy ``llm`` on the SDK side (the real - // model lives on the ACP subprocess via ``acp_model``), so surfacing - // ``agent.llm.model`` as the conversation's "active LLM" would lie to - // every consumer downstream — most visibly the chat header's - // SwitchProfileButton, which would otherwise let the user switch - // profiles on a Claude-Code conversation while the running subprocess - // keeps its own model. Null at the boundary so no consumer has to - // re-derive the rule. Mirrors OpenHands PR #14401. + // ACPAgent conversations carry a sentinel ``llm`` on older SDKs. Prefer the + // runtime model fields when available, then the configured ``acp_model`` that + // Canvas saves for built-in providers. ``agent_kind`` still gates model + // switching, so surfacing this string is display-only. const isAcp = info.agent?.kind === "ACPAgent"; // Only surface ``acp_server`` for ACP conversations even if the wire // payload accidentally carries an ``acpserver`` tag on an OpenHands // conversation — the chip is identity info for the ACP CLI subprocess, // and showing it on a non-ACP conversation would be a lie. const acpServer = isAcp ? (info.tags?.[ACP_SERVER_TAG_KEY] ?? null) : null; + const acpRuntimeName = nonEmptyString(info.current_model_name); + const acpRuntimeId = nonEmptyString(info.current_model_id); + const acpConfiguredModel = nonEmptyString(info.agent?.acp_model); + const acpLlmModel = nonEmptyString(info.agent?.llm?.model); + const acpDisplayModel = + acpRuntimeName && !isAcpDefaultPlaceholder(acpRuntimeName) + ? acpRuntimeName + : acpRuntimeId && !isAcpDefaultPlaceholder(acpRuntimeId) + ? acpRuntimeId + : (acpConfiguredModel ?? + (acpLlmModel !== "acp-managed" ? acpLlmModel : null)); return { id: info.id, created_by_user_id: null, @@ -275,7 +296,7 @@ export function toAppConversation( agent_kind: isAcp ? "acp" : "openhands", acp_server: acpServer, llm_model: isAcp - ? null + ? acpDisplayModel : (info.agent?.llm?.model ?? DEFAULT_SETTINGS.llm_model), metrics: info.metrics ? { @@ -581,6 +602,13 @@ function buildConfiguredAcpAgentSettings(settings: Settings): SettingsRecord { const payload: SettingsRecord = {}; for (const key of ACP_SETTINGS_KEYS) { if (agentSettings[key] !== undefined && agentSettings[key] !== null) { + if (key === "acp_model") { + const model = nonEmptyString(agentSettings[key]); + if (model) { + payload[key] = model; + } + continue; + } payload[key] = agentSettings[key]; } } diff --git a/src/api/conversation-service/agent-server-conversation-service.types.ts b/src/api/conversation-service/agent-server-conversation-service.types.ts index 3261ed4c9..09243fdfb 100644 --- a/src/api/conversation-service/agent-server-conversation-service.types.ts +++ b/src/api/conversation-service/agent-server-conversation-service.types.ts @@ -127,8 +127,8 @@ export interface AppConversation { * driven Agent, ``"acp"`` for an ACPAgent that delegates to an external * ACP CLI subprocess. Consumers can use this to gate UI affordances that * only make sense for one kind (e.g. the LLM-profile switcher in the chat - * header is a no-op for ACP conversations because model selection lives - * on the subprocess via ``acp_model``, not on ``llm_model``). + * header is a no-op for ACP conversations even though ``llm_model`` may + * carry the ACP subprocess model for display). */ agent_kind?: "openhands" | "acp" | null; /** diff --git a/src/components/features/chat/components/chat-input-actions.tsx b/src/components/features/chat/components/chat-input-actions.tsx index 544cb81b6..491835e86 100644 --- a/src/components/features/chat/components/chat-input-actions.tsx +++ b/src/components/features/chat/components/chat-input-actions.tsx @@ -66,9 +66,17 @@ export function ChatInputActions({ const { data: conversation } = useActiveConversation(); const { backend } = useActiveBackend(); const isCloud = backend.kind === "cloud"; + const isAcpConversation = conversation?.agent_kind === "acp"; const llmDestinationLabel = t( - isCloud ? I18nKey.SETTINGS$LLM_SETTINGS : I18nKey.SETTINGS$LLM_PROFILES, + isAcpConversation + ? I18nKey.SETTINGS$NAV_AGENT + : isCloud + ? I18nKey.SETTINGS$LLM_SETTINGS + : I18nKey.SETTINGS$LLM_PROFILES, ); + const modelDestinationPath = isAcpConversation + ? "/settings/agent" + : "/settings"; const webSocketStatus = useUnifiedWebSocketStatus(); const { curAgentState } = useAgentState(); const { conversationMode, setConversationMode } = useConversationStore(); @@ -449,7 +457,7 @@ export function ChatInputActions({
  • @@ -491,7 +499,11 @@ export function ChatInputActions({ )}
    - {isCloud ? : } + {isCloud || isAcpConversation ? ( + + ) : ( + + )}
    {hasOverflowItems && ( diff --git a/src/components/features/chat/components/chat-input-model.tsx b/src/components/features/chat/components/chat-input-model.tsx index 04b5af5d9..07fcaa551 100644 --- a/src/components/features/chat/components/chat-input-model.tsx +++ b/src/components/features/chat/components/chat-input-model.tsx @@ -28,28 +28,29 @@ export function ChatInputModel() { // Home page has no active conversation; fall back to the user's default // model so the switcher renders consistently across both surfaces. const { data: settings } = useSettings(); - // ACPAgent conversations have no OpenHands LLM (the model lives on the - // ACP subprocess via ``acp_model``), so ``toAppConversation`` writes a - // null ``llm_model`` for them. Don't fall back to ``settings.llm_model`` - // here — that would resurrect the user's *default* OpenHands model on a - // Claude-Code conversation and link to /settings, both of which lie - // about what model is actually running. - // - // On the home screen ``conversation`` is undefined, so we also have to - // consult ``settings.agent_settings.agent_kind`` — that's the kind the - // next-created conversation will inherit. Without the fallback, ACP - // users would still see the LLM-profile control on the home page, - // contradicting the ACP nav gating elsewhere. - const isAcpActive = - conversation?.agent_kind === "acp" || - (!conversation && settings?.agent_settings?.agent_kind === "acp"); - const llmModel = isAcpActive - ? null - : (conversation?.llm_model ?? settings?.llm_model); + // ACP conversations do not use the OpenHands LLM profile. Show the ACP + // model only when the adapter/settings provide one, and link users to Agent + // settings instead of the LLM profile page. + const isActiveAcpConversation = conversation?.agent_kind === "acp"; + const isHomeAcp = + !conversation && settings?.agent_settings?.agent_kind === "acp"; + const settingsAcpModel = + typeof settings?.agent_settings?.acp_model === "string" + ? settings.agent_settings.acp_model.trim() || null + : null; + const llmModel = isActiveAcpConversation + ? conversation?.llm_model + : isHomeAcp + ? settingsAcpModel + : (conversation?.llm_model ?? settings?.llm_model); + const destinationPath = + isActiveAcpConversation || isHomeAcp ? "/settings/agent" : "/settings"; const llmDestinationLabel = t( - backend.kind === "cloud" - ? I18nKey.SETTINGS$LLM_SETTINGS - : I18nKey.SETTINGS$LLM_PROFILES, + isActiveAcpConversation || isHomeAcp + ? I18nKey.SETTINGS$NAV_AGENT + : backend.kind === "cloud" + ? I18nKey.SETTINGS$LLM_SETTINGS + : I18nKey.SETTINGS$LLM_PROFILES, ); const [isPopoverOpen, setIsPopoverOpen] = React.useState(false); @@ -105,7 +106,7 @@ export function ChatInputModel() {
  • setIsPopoverOpen(false)} className="flex h-[30px] items-center gap-2 rounded p-2 leading-5 text-white hover:bg-[var(--oh-interactive-hover)] transition-colors" > diff --git a/src/components/features/chat/switch-profile-button.tsx b/src/components/features/chat/switch-profile-button.tsx index c2f07b68f..a113dc60e 100644 --- a/src/components/features/chat/switch-profile-button.tsx +++ b/src/components/features/chat/switch-profile-button.tsx @@ -34,9 +34,8 @@ export function SwitchProfileButton() { // controlled by ``acp_model`` (set in Settings → Agent), not by the LLM // profile picker. Surfacing the switcher here would let the user "change // the model" while the running subprocess silently keeps its own — a - // confusing no-op. Hide the button instead. ``toAppConversation`` also - // nulls ``llm_model`` on this boundary so any other consumer that reads - // the model directly sees "no model" rather than a misleading value. + // confusing no-op. Hide the button even when ``llm_model`` carries an ACP + // display model for chips/headers. // // On the home screen ``conversation`` is undefined; fall back to // ``settings.agent_settings.agent_kind`` so the picker also hides when diff --git a/src/constants/acp-providers.ts b/src/constants/acp-providers.ts index aa88a54f6..8ac6bb139 100644 --- a/src/constants/acp-providers.ts +++ b/src/constants/acp-providers.ts @@ -43,6 +43,14 @@ export interface ACPProviderConfig { * ``@zed-industries/codex-acp`` (the Zed-shipped wrapper) instead. */ default_command: string[]; + /** + * Canvas-local suggested ACP model IDs. These mirror the current runtime + * picker values for the built-in harnesses, but are not authoritative access + * checks; users can still enter a custom override in Settings -> Agent. + */ + available_models?: ACPModelOption[]; + /** Model ID preselected for built-in providers so Canvas never saves blank. */ + default_model?: string; /** * i18n key for the one-line provider description rendered under the * onboarding tile. Stored on the registry so adding a new ACP @@ -58,6 +66,55 @@ export interface ACPProviderConfig { icon?: ACPProviderIcon; } +export interface ACPModelOption { + /** Exact model ID sent as ``acp_model``. */ + id: string; + /** Human-readable label shown in Settings -> Agent. */ + label: string; +} + +const CODEX_REASONING_EFFORTS = ["low", "medium", "high", "xhigh"] as const; + +function buildCodexModelOptions( + models: Array<{ id: string; label: string }>, +): ACPModelOption[] { + return models.flatMap((model) => + CODEX_REASONING_EFFORTS.map((effort) => ({ + id: `${model.id}/${effort}`, + label: `${model.label} (${effort})`, + })), + ); +} + +const CLAUDE_MODELS: ACPModelOption[] = [ + { id: "claude-sonnet-4-6", label: "Claude Sonnet 4.6" }, + { id: "claude-sonnet-4-6[1m]", label: "Claude Sonnet 4.6 (1M)" }, + { id: "claude-opus-4-7", label: "Claude Opus 4.7" }, + { id: "claude-opus-4-7[1m]", label: "Claude Opus 4.7 (1M)" }, + { id: "claude-haiku-4-5", label: "Claude Haiku 4.5" }, +]; + +const CODEX_MODELS: ACPModelOption[] = buildCodexModelOptions([ + { id: "gpt-5.5", label: "GPT-5.5" }, + { id: "gpt-5.4", label: "GPT-5.4" }, + { id: "gpt-5.4-mini", label: "GPT-5.4 Mini" }, + { id: "gpt-5.3-codex", label: "GPT-5.3 Codex" }, + { id: "gpt-5.2", label: "GPT-5.2" }, +]); + +const GEMINI_MODELS: ACPModelOption[] = [ + { id: "gemini-2.5-pro", label: "Gemini 2.5 Pro" }, + { id: "gemini-2.5-flash", label: "Gemini 2.5 Flash" }, + { id: "gemini-2.5-flash-lite", label: "Gemini 2.5 Flash Lite" }, + { id: "gemini-3-pro-preview", label: "Gemini 3 Pro Preview" }, + { id: "gemini-3-flash-preview", label: "Gemini 3 Flash Preview" }, + { id: "gemini-3.1-pro-preview", label: "Gemini 3.1 Pro Preview" }, + { + id: "gemini-3.1-flash-lite-preview", + label: "Gemini 3.1 Flash Lite Preview", + }, +]; + // Each entry's ``default_command`` is the published-package npx // invocation that speaks the ACP JSON-RPC protocol on stdio. Verified // against the upstream npm registry on the date noted below — if a @@ -71,6 +128,8 @@ export const ACP_PROVIDERS: ACPProviderConfig[] = [ // Verified 2026-05-19. Official Anthropic-maintained ACP wrapper // around the Claude Code CLI. default_command: ["npx", "-y", "@agentclientprotocol/claude-agent-acp"], + available_models: CLAUDE_MODELS, + default_model: "claude-sonnet-4-6", description_key: I18nKey.ONBOARDING$AGENT_CLAUDE_CODE_DESCRIPTION, icon: "claude-code", }, @@ -82,6 +141,8 @@ export const ACP_PROVIDERS: ACPProviderConfig[] = [ // OpenAI Codex CLI — NOT ``@openai/codex acp`` (no ``acp`` // subcommand on that package). default_command: ["npx", "-y", "@zed-industries/codex-acp"], + available_models: CODEX_MODELS, + default_model: "gpt-5.5/medium", description_key: I18nKey.ONBOARDING$AGENT_CODEX_DESCRIPTION, icon: "codex", }, @@ -92,6 +153,8 @@ export const ACP_PROVIDERS: ACPProviderConfig[] = [ // Verified 2026-05-19. Official Google CLI; ``--acp`` switches it // into ACP server mode on stdio. default_command: ["npx", "-y", "@google/gemini-cli", "--acp"], + available_models: GEMINI_MODELS, + default_model: "gemini-2.5-pro", description_key: I18nKey.ONBOARDING$AGENT_GEMINI_CLI_DESCRIPTION, icon: "gemini", }, @@ -170,6 +233,11 @@ export function buildAcpAgentSettingsDiff( return null; } + const model = + options.model === undefined + ? (provider?.default_model ?? null) + : options.model; + // ``acp_args: []`` resets any API-set ``acp_args`` that would // otherwise survive and concatenate to ``acp_command`` at spawn time // (the agent-server merges the two before exec). Callers building the @@ -181,6 +249,6 @@ export function buildAcpAgentSettingsDiff( acp_server: providerKey, acp_command: options.command ?? [], acp_args: [], - acp_model: options.model ?? null, + acp_model: model ?? null, }; } diff --git a/src/i18n/translation.json b/src/i18n/translation.json index 15b28c2e6..ca1b8a319 100644 --- a/src/i18n/translation.json +++ b/src/i18n/translation.json @@ -5133,22 +5133,39 @@ "uk": "Model", "ca": "Model" }, + "SETTINGS$AGENT_CUSTOM_MODEL": { + "en": "Custom model", + "ja": "Custom model", + "zh-CN": "Custom model", + "zh-TW": "Custom model", + "ko-KR": "Custom model", + "no": "Custom model", + "it": "Custom model", + "pt": "Custom model", + "es": "Custom model", + "ar": "Custom model", + "fr": "Custom model", + "tr": "Custom model", + "de": "Custom model", + "uk": "Custom model", + "ca": "Custom model" + }, "SETTINGS$AGENT_MODEL_HINT": { - "en": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.", - "ja": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.", - "zh-CN": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.", - "zh-TW": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.", - "ko-KR": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.", - "no": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.", - "it": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.", - "pt": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.", - "es": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.", - "ar": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.", - "fr": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.", - "tr": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.", - "de": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.", - "uk": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.", - "ca": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default." + "en": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", + "ja": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", + "zh-CN": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", + "zh-TW": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", + "ko-KR": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", + "no": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", + "it": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", + "pt": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", + "es": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", + "ar": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", + "fr": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", + "tr": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", + "de": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", + "uk": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", + "ca": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank." }, "SETTINGS$AGENT_DISABLED_TOOLTIP": { "en": "Disabled while {{agentName}} is the active agent", diff --git a/src/routes/agent-settings.tsx b/src/routes/agent-settings.tsx index bf0f2a059..4d2981073 100644 --- a/src/routes/agent-settings.tsx +++ b/src/routes/agent-settings.tsx @@ -34,6 +34,7 @@ type AgentType = "openhands" | "acp"; const ENABLE_SUB_AGENTS_FIELD_KEY = "enable_sub_agents"; const COMMAND_PLACEHOLDER_FALLBACK = "npx -y "; +const ACP_CUSTOM_MODEL_KEY = "__custom_model__"; function toStringArray(value: unknown): string[] { return Array.isArray(value) @@ -68,6 +69,15 @@ function getEnableSubAgentsValue( return field?.default === true; } +function isKnownAcpModel( + provider: ACPProviderConfig | undefined, + model: string, +): boolean { + return ( + provider?.available_models?.some(({ id }) => id === model.trim()) ?? false + ); +} + function AgentSettingsScreen() { const { t } = useTranslation("openhands"); const { data: settings, isLoading } = useSettings(); @@ -98,6 +108,7 @@ function AgentSettingsScreen() { const [agentType, setAgentType] = useState("openhands"); const [commandText, setCommandText] = useState(""); const [acpModel, setAcpModel] = useState(""); + const [isCustomAcpModel, setIsCustomAcpModel] = useState(false); const [isDirty, setIsDirty] = useState(false); const lastInitializedSettingsRef = useRef(null); @@ -134,13 +145,20 @@ function AgentSettingsScreen() { loadedCommandTextRef.current = renderedCommandText; const savedModel = settings.agent_settings?.acp_model; - setAcpModel(typeof savedModel === "string" ? savedModel : ""); + const normalizedSavedModel = + typeof savedModel === "string" ? savedModel.trim() : ""; + setAcpModel(normalizedSavedModel || provider?.default_model || ""); + setIsCustomAcpModel( + !!normalizedSavedModel && + (!provider || !isKnownAcpModel(provider, normalizedSavedModel)), + ); } else { setAgentType("openhands"); setCommandText(""); setAcpModel(""); loadedAcpServerRef.current = null; loadedCommandTextRef.current = ""; + setIsCustomAcpModel(false); } setIsDirty(false); }, [settings]); @@ -159,6 +177,13 @@ function AgentSettingsScreen() { const selectedProvider = ACP_PROVIDERS.find( ({ key }) => key === selectedPreset, ); + const modelSuggestions = selectedProvider?.available_models ?? []; + const hasModelSuggestions = modelSuggestions.length > 0; + const selectedModelIsSuggestion = isKnownAcpModel(selectedProvider, acpModel); + const selectedModelKey = + isCustomAcpModel || !selectedModelIsSuggestion + ? ACP_CUSTOM_MODEL_KEY + : acpModel; const isDefaultProviderCommand = !!selectedProvider && commandTokens.join(" ") === selectedProvider.default_command.join(" "); @@ -187,9 +212,11 @@ function AgentSettingsScreen() { : selectedProvider && isDefaultProviderCommand ? selectedProvider.key : ACP_CUSTOM_PRESET_KEY; + const effectiveModel = + acpModel.trim() || (selectedProvider?.default_model ?? null); const agentSettingsDiff = buildAcpAgentSettingsDiff(providerKey, { command: useDefault ? [] : commandTokens, - model: acpModel.trim() || null, + model: effectiveModel, allowUnknownServer: preserveUnknownServer, }); @@ -277,7 +304,11 @@ function AgentSettingsScreen() { const preferred = ACP_PROVIDERS[0]; if (preferred) { setCommandText(formatCommand(preferred.default_command)); + setAcpModel(preferred.default_model ?? ""); + setIsCustomAcpModel(false); } + } else if (newType === "openhands") { + setIsCustomAcpModel(false); } setIsDirty(true); }} @@ -325,6 +356,10 @@ function AgentSettingsScreen() { const provider = ACP_PROVIDERS.find(({ key: k }) => k === preset); if (provider) { setCommandText(formatCommand(provider.default_command)); + setAcpModel(provider.default_model ?? ""); + setIsCustomAcpModel(false); + } else if (preset === ACP_CUSTOM_PRESET_KEY) { + setIsCustomAcpModel(true); } setIsDirty(true); }} @@ -350,18 +385,54 @@ function AgentSettingsScreen() {
    - { - setAcpModel(value); - setIsDirty(true); - }} - /> + {hasModelSuggestions && ( + ({ + key: model.id, + label: model.label, + })), + { + key: ACP_CUSTOM_MODEL_KEY, + label: t(I18nKey.SETTINGS$AGENT_PRESET_CUSTOM), + }, + ]} + selectedKey={selectedModelKey} + onSelectionChange={(key) => { + if (!key) return; + const modelKey = String(key); + if (modelKey === ACP_CUSTOM_MODEL_KEY) { + setIsCustomAcpModel(true); + setAcpModel(""); + } else { + setIsCustomAcpModel(false); + setAcpModel(modelKey); + } + setIsDirty(true); + }} + /> + )} + {(!hasModelSuggestions || isCustomAcpModel) && ( + { + setAcpModel(value); + setIsDirty(true); + }} + /> + )} {t(I18nKey.SETTINGS$AGENT_MODEL_HINT)} From 9d1b7c1700f5172ed8f57bbec6e712be4f2f4a4d Mon Sep 17 00:00:00 2001 From: Debug Agent Date: Fri, 22 May 2026 17:38:19 +0200 Subject: [PATCH 2/6] test(acp): verify provider models from subprocesses --- .github/workflows/acp-providers-sync.yml | 8 +- .../onboarding/choose-agent-step.test.tsx | 2 +- __tests__/routes/agent-settings.test.tsx | 6 +- package.json | 1 + scripts/check-acp-provider-models.mjs | 624 ++++++++++++++++++ src/constants/acp-providers.ts | 67 +- 6 files changed, 670 insertions(+), 38 deletions(-) create mode 100644 scripts/check-acp-provider-models.mjs diff --git a/.github/workflows/acp-providers-sync.yml b/.github/workflows/acp-providers-sync.yml index ffb780e1c..560002f72 100644 --- a/.github/workflows/acp-providers-sync.yml +++ b/.github/workflows/acp-providers-sync.yml @@ -4,12 +4,14 @@ name: ACP Providers Sync # (src/constants/acp-providers.ts) and the SDK's source of truth at # openhands-sdk/openhands/sdk/settings/acp_providers.py in # OpenHands/software-agent-sdk. See agent-canvas#587 for the failure mode -# this exists to prevent. +# this exists to prevent. Also launches the built-in ACP subprocesses and +# verifies Canvas's suggested model IDs match their advertised availableModels. on: pull_request: paths: - "src/constants/acp-providers.ts" + - "scripts/check-acp-provider-models.mjs" - "scripts/check-acp-providers-sync.mjs" - ".github/workflows/acp-providers-sync.yml" push: @@ -17,6 +19,7 @@ on: - main paths: - "src/constants/acp-providers.ts" + - "scripts/check-acp-provider-models.mjs" - "scripts/check-acp-providers-sync.mjs" - ".github/workflows/acp-providers-sync.yml" @@ -81,3 +84,6 @@ jobs: # repository_dispatch payload; otherwise the script defaults to "main". ACP_SDK_REF: ${{ inputs.sdk_ref || github.event.client_payload.sdk_ref || '' }} run: node scripts/check-acp-providers-sync.mjs + + - name: Check ACP provider models sync + run: node scripts/check-acp-provider-models.mjs diff --git a/__tests__/components/onboarding/choose-agent-step.test.tsx b/__tests__/components/onboarding/choose-agent-step.test.tsx index 263a94353..4cf9b0e1b 100644 --- a/__tests__/components/onboarding/choose-agent-step.test.tsx +++ b/__tests__/components/onboarding/choose-agent-step.test.tsx @@ -168,7 +168,7 @@ describe("ChooseAgentStep", () => { // ``acp_args`` can't survive and concatenate onto the spawn // command at conversation-create time. acp_args: [], - acp_model: "claude-sonnet-4-6", + acp_model: "sonnet", }); }); diff --git a/__tests__/routes/agent-settings.test.tsx b/__tests__/routes/agent-settings.test.tsx index f5045a094..8daabfa13 100644 --- a/__tests__/routes/agent-settings.test.tsx +++ b/__tests__/routes/agent-settings.test.tsx @@ -216,7 +216,7 @@ describe("AgentSettingsScreen", () => { renderAgentSettingsScreen(); await screen.findByTestId("agent-command-input"); await user.click(screen.getByLabelText("SETTINGS$AGENT_MODEL")); - await user.click(await screen.findByText("Claude Opus 4.7")); + await user.click(await screen.findByText("Claude Haiku 4.5")); await user.click(screen.getByTestId("agent-save-button")); await waitFor(() => { @@ -225,7 +225,7 @@ describe("AgentSettingsScreen", () => { const call = save.mock.calls[0]?.[0] as { agent_settings_diff?: Record; }; - expect(call.agent_settings_diff?.acp_model).toBe("claude-opus-4-7"); + expect(call.agent_settings_diff?.acp_model).toBe("haiku"); }); it("saves an ACP diff when switching to ACP + Claude Code", async () => { @@ -279,7 +279,7 @@ describe("AgentSettingsScreen", () => { // ``acp_args`` can't survive and concatenate onto the spawn // command at conversation-create time. acp_args: [], - acp_model: "claude-sonnet-4-6", + acp_model: "sonnet", }); }); diff --git a/package.json b/package.json index 70b4b1c88..7721dc4e5 100644 --- a/package.json +++ b/package.json @@ -82,6 +82,7 @@ "test:e2e:live": "node --env-file-if-exists=.env tests/e2e/live/scripts/run-live-e2e.mjs", "test:e2e:snapshots": "playwright test tests/e2e/snapshots --project=chromium --retries=0", "test:e2e:snapshots:update": "playwright test tests/e2e/snapshots --project=chromium --update-snapshots", + "test:acp-models": "node scripts/check-acp-provider-models.mjs", "test:coverage": "npm run make-i18n && vitest run --coverage", "dev_wsl": "VITE_WATCH_USE_POLLING=true vite", "preview": "vite preview", diff --git a/scripts/check-acp-provider-models.mjs b/scripts/check-acp-provider-models.mjs new file mode 100644 index 000000000..5cf666f16 --- /dev/null +++ b/scripts/check-acp-provider-models.mjs @@ -0,0 +1,624 @@ +#!/usr/bin/env node + +/** + * Check ACP Provider Models Sync + * + * Launches each built-in ACP subprocess from src/constants/acp-providers.ts, + * creates a minimal ACP session, reads the process-advertised + * models.availableModels, and compares those model IDs to Canvas's + * available_models suggestions. + * + * Canvas intentionally filters generic "default" placeholders such as Claude + * Code's "Default (recommended)" before comparing. The UI must surface concrete + * model choices, not a provider-specific default sentinel. + * + * Usage: + * node scripts/check-acp-provider-models.mjs + * node scripts/check-acp-provider-models.mjs --provider codex + * + * Options: + * --provider Check only one provider key. Can be passed multiple times. + * --timeout-ms Timeout for each ACP request. Default: 30000. + * --help, -h Show help. + * + * Exit codes: + * 0 - Canvas suggestions match the ACP subprocess-advertised model IDs. + * 1 - Drift detected, subprocess error, or parse error. + */ + +import { spawn } from "node:child_process"; +import { + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import readline from "node:readline"; +import { fileURLToPath } from "node:url"; +import process from "node:process"; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const projectRoot = join(__dirname, ".."); +const DEFAULT_TS_PATH = join( + projectRoot, + "src", + "constants", + "acp-providers.ts", +); + +const colors = { + reset: "\x1b[0m", + red: "\x1b[31m", + green: "\x1b[32m", + yellow: "\x1b[33m", + cyan: "\x1b[36m", + dim: "\x1b[2m", +}; + +function parseArgs(argv) { + const args = { + help: false, + providers: [], + timeoutMs: Number(process.env.ACP_MODEL_CHECK_TIMEOUT_MS || 30_000), + tsFile: DEFAULT_TS_PATH, + }; + + for (let i = 0; i < argv.length; i++) { + const arg = argv[i]; + if (arg === "--help" || arg === "-h") { + args.help = true; + } else if (arg === "--provider") { + args.providers.push(argv[++i]); + } else if (arg === "--timeout-ms") { + args.timeoutMs = Number(argv[++i]); + } else if (arg === "--ts-file") { + args.tsFile = argv[++i]; + } else { + throw new Error(`Unknown argument: ${arg}`); + } + } + + if (!Number.isFinite(args.timeoutMs) || args.timeoutMs <= 0) { + throw new Error("--timeout-ms must be a positive number"); + } + + return args; +} + +function showHelp() { + process.stdout.write( + `\nACP Provider Models Sync Check\n\n` + + `Launches each built-in ACP subprocess and verifies that Canvas's\n` + + `available_models suggestions match the model IDs advertised by\n` + + `session/new -> models.availableModels, after filtering generic\n` + + `default placeholders.\n\n` + + `Usage:\n` + + ` node scripts/check-acp-provider-models.mjs [options]\n\n` + + `Options:\n` + + ` --provider Check only one provider key. Repeatable.\n` + + ` --timeout-ms Timeout for each ACP request. Default: 30000.\n` + + ` --ts-file Override TS mirror path.\n` + + ` --help, -h Show this help.\n\n`, + ); +} + +function findMatchingDelimiter(source, openIdx, open, close) { + let depth = 0; + let i = openIdx; + + while (i < source.length) { + const char = source[i]; + + if (char === '"' || char === "'" || char === "`") { + const quote = char; + i++; + while (i < source.length) { + if (source[i] === "\\") { + i += 2; + continue; + } + if (source[i] === quote) { + i++; + break; + } + i++; + } + continue; + } + + if (char === "/" && source[i + 1] === "/") { + while (i < source.length && source[i] !== "\n") i++; + continue; + } + + if (char === "/" && source[i + 1] === "*") { + i += 2; + while ( + i < source.length - 1 && + !(source[i] === "*" && source[i + 1] === "/") + ) { + i++; + } + i += 2; + continue; + } + + if (char === open) depth++; + if (char === close) { + depth--; + if (depth === 0) return i; + } + i++; + } + + return -1; +} + +function extractStringList(source) { + const out = []; + const re = /"((?:\\.|[^"\\])*)"/g; + let match; + while ((match = re.exec(source)) !== null) { + out.push(match[1].replace(/\\(.)/g, "$1")); + } + return out; +} + +function parseModelOptionArrays(source) { + const arrays = new Map(); + const re = /const\s+(\w+)\s*:\s*ACPModelOption\[\]\s*=\s*\[/g; + let match; + + while ((match = re.exec(source)) !== null) { + const name = match[1]; + const equalsIdx = source.indexOf("=", match.index); + const openIdx = source.indexOf("[", equalsIdx); + const closeIdx = findMatchingDelimiter(source, openIdx, "[", "]"); + if (closeIdx === -1) { + throw new Error(`Could not find end of model array ${name}`); + } + + const body = source.slice(openIdx + 1, closeIdx); + const models = splitTopLevelObjects(body).map((modelSource) => { + const id = modelSource.match(/id:\s*"((?:\\.|[^"\\])*)"/)?.[1]; + const label = modelSource.match(/label:\s*"((?:\\.|[^"\\])*)"/)?.[1]; + if (!id || !label) { + throw new Error( + `Could not parse model option in ${name}: ${modelSource}`, + ); + } + return { + id: id.replace(/\\(.)/g, "$1"), + label: label.replace(/\\(.)/g, "$1"), + }; + }); + + arrays.set(name, models); + re.lastIndex = closeIdx + 1; + } + + return arrays; +} + +function splitTopLevelObjects(source) { + const objects = []; + let i = 0; + + while (i < source.length) { + if (source[i] !== "{") { + i++; + continue; + } + + const closeIdx = findMatchingDelimiter(source, i, "{", "}"); + if (closeIdx === -1) { + throw new Error("Could not find end of provider object"); + } + + objects.push(source.slice(i, closeIdx + 1)); + i = closeIdx + 1; + } + + return objects; +} + +function parseCanvasProviders(tsFile) { + const source = readFileSync(tsFile, "utf8"); + const modelArrays = parseModelOptionArrays(source); + const providersStart = source.search( + /export\s+const\s+ACP_PROVIDERS\s*:\s*ACPProviderConfig\[\]\s*=\s*\[/, + ); + if (providersStart === -1) { + throw new Error("Could not find exported ACP_PROVIDERS array"); + } + + const equalsIdx = source.indexOf("=", providersStart); + const openIdx = source.indexOf("[", equalsIdx); + const closeIdx = findMatchingDelimiter(source, openIdx, "[", "]"); + if (closeIdx === -1) { + throw new Error("Could not find end of ACP_PROVIDERS array"); + } + + const providerObjects = splitTopLevelObjects( + source.slice(openIdx + 1, closeIdx), + ); + + return providerObjects + .map((objectSource) => { + const key = objectSource.match(/key:\s*"([^"]+)"/)?.[1]; + const displayName = objectSource.match(/display_name:\s*"([^"]+)"/)?.[1]; + const defaultCommandBody = objectSource.match( + /default_command:\s*\[([\s\S]*?)\]/, + )?.[1]; + const modelArrayName = objectSource.match( + /available_models:\s*(\w+)/, + )?.[1]; + const defaultModel = objectSource.match( + /default_model:\s*"([^"]+)"/, + )?.[1]; + + if (!key || !displayName || !defaultCommandBody) { + throw new Error(`Could not parse provider object:\n${objectSource}`); + } + + return { + key, + display_name: displayName, + default_command: extractStringList(defaultCommandBody), + available_models: modelArrayName + ? (modelArrays.get(modelArrayName) ?? []) + : [], + default_model: defaultModel ?? null, + }; + }) + .filter((provider) => provider.available_models.length > 0); +} + +function isGenericDefaultModel(model) { + const id = String(model.modelId ?? "") + .trim() + .toLowerCase(); + const name = String(model.name ?? "") + .trim() + .toLowerCase(); + return ( + id === "default" || name === "default" || name === "default (recommended)" + ); +} + +function providerEnv(providerKey, tempRoot) { + const env = { + ...process.env, + CI: "1", + NO_COLOR: "1", + }; + + if (providerKey === "claude-code") { + env.CLAUDE_CONFIG_DIR = join(tempRoot, "claude-config"); + } + + if (providerKey === "codex") { + env.CODEX_HOME = join(tempRoot, "codex-home"); + } + + if (providerKey === "gemini-cli") { + env.GEMINI_CLI_HOME = join(tempRoot, "gemini-home"); + env.GEMINI_API_KEY = + process.env.GEMINI_API_KEY || "dummy-acp-model-drift-check"; + } + + return env; +} + +function writeJsonLine(stream, message) { + stream.write(`${JSON.stringify({ jsonrpc: "2.0", ...message })}\n`); +} + +async function waitForExit(child, timeoutMs) { + if (child.exitCode !== null || child.signalCode !== null) return; + + await new Promise((resolve) => { + const timer = setTimeout(resolve, timeoutMs); + child.once("exit", () => { + clearTimeout(timer); + resolve(); + }); + }); +} + +async function terminateProcessGroup(child) { + if (!child.pid) return; + + try { + if (process.platform === "win32") child.kill("SIGTERM"); + else process.kill(-child.pid, "SIGTERM"); + } catch { + // The process may have exited between the response and cleanup. + } + + await waitForExit(child, 1_000); + + if (child.exitCode === null && child.signalCode === null) { + try { + if (process.platform === "win32") child.kill("SIGKILL"); + else process.kill(-child.pid, "SIGKILL"); + } catch { + // Best-effort cleanup. + } + } + + child.stdin?.destroy(); + child.stdout?.destroy(); + child.stderr?.destroy(); +} + +async function fetchRuntimeModels(provider, timeoutMs) { + const tempRoot = mkdtempSync(join(tmpdir(), `acp-models-${provider.key}-`)); + const workspace = join(tempRoot, "workspace"); + mkdirSync(workspace, { recursive: true }); + const command = provider.default_command; + const env = providerEnv(provider.key, tempRoot); + for (const key of ["CLAUDE_CONFIG_DIR", "CODEX_HOME", "GEMINI_CLI_HOME"]) { + if (env[key]) mkdirSync(env[key], { recursive: true }); + } + if (provider.key === "codex") { + writeFileSync( + join(env.CODEX_HOME, "auth.json"), + JSON.stringify({ + auth_mode: "apikey", + OPENAI_API_KEY: + process.env.OPENAI_API_KEY || "dummy-acp-model-drift-check", + }), + ); + } + let stderr = ""; + + const child = spawn(command[0], command.slice(1), { + cwd: workspace, + env, + stdio: ["pipe", "pipe", "pipe"], + detached: process.platform !== "win32", + }); + + const pending = new Map(); + let nextId = 1; + + child.stderr.on("data", (chunk) => { + stderr = `${stderr}${chunk.toString()}`.slice(-8_000); + }); + + child.on("exit", (code, signal) => { + for (const [id, entry] of pending) { + entry.reject( + new Error( + `${provider.key} exited before response ${id} (code=${code}, signal=${signal})\n${stderr}`, + ), + ); + } + pending.clear(); + }); + + readline.createInterface({ input: child.stdout }).on("line", (line) => { + let message; + try { + message = JSON.parse(line); + } catch { + stderr = `${stderr}\n[stdout non-json] ${line}`.slice(-8_000); + return; + } + + if (message.id !== undefined && pending.has(message.id)) { + const entry = pending.get(message.id); + pending.delete(message.id); + entry.resolve(message); + return; + } + + // Some agents may send client-side requests while setting up a session. + // The model list does not depend on these, so return an empty success to + // keep the handshake moving. + if (message.id !== undefined && message.method) { + writeJsonLine(child.stdin, { id: message.id, result: {} }); + } + }); + + function request(method, params) { + const id = nextId++; + writeJsonLine(child.stdin, { id, method, params }); + + return new Promise((resolve, reject) => { + const timer = setTimeout(() => { + pending.delete(id); + reject( + new Error( + `${provider.key} timed out waiting for ${method} after ${timeoutMs}ms\n${stderr}`, + ), + ); + }, timeoutMs); + + pending.set(id, { + resolve: (message) => { + clearTimeout(timer); + resolve(message); + }, + reject: (error) => { + clearTimeout(timer); + reject(error); + }, + }); + }); + } + + try { + const init = await request("initialize", { + protocolVersion: 1, + clientCapabilities: { + auth: { terminal: false }, + fs: { readTextFile: false, writeTextFile: false }, + terminal: false, + }, + clientInfo: { + name: "agent-canvas-acp-model-drift-check", + version: "0.0.0", + }, + }); + if (init.error) { + throw new Error( + `${provider.key} initialize failed: ${JSON.stringify(init.error)}`, + ); + } + + const session = await request("session/new", { + cwd: workspace, + mcpServers: [], + }); + if (session.error) { + throw new Error( + `${provider.key} session/new failed: ${JSON.stringify(session.error)}`, + ); + } + + const models = session.result?.models?.availableModels; + if (!Array.isArray(models)) { + throw new Error( + `${provider.key} did not return models.availableModels from session/new`, + ); + } + + if (session.result?.sessionId) { + await request("session/close", { + sessionId: session.result.sessionId, + }).catch(() => undefined); + } + + return models; + } finally { + await terminateProcessGroup(child); + rmSync(tempRoot, { recursive: true, force: true }); + } +} + +function compareModelIds(provider, runtimeModels) { + const ignored = runtimeModels.filter(isGenericDefaultModel); + const actual = runtimeModels + .filter((model) => !isGenericDefaultModel(model)) + .map((model) => ({ + id: model.modelId, + label: model.name, + })); + const expected = provider.available_models; + const actualIds = actual.map((model) => model.id); + const expectedIds = expected.map((model) => model.id); + const missing = actualIds.filter((id) => !expectedIds.includes(id)); + const extra = expectedIds.filter((id) => !actualIds.includes(id)); + const orderMatches = + actualIds.length === expectedIds.length && + actualIds.every((id, index) => id === expectedIds[index]); + + return { + ok: missing.length === 0 && extra.length === 0 && orderMatches, + actual, + actualIds, + expected, + expectedIds, + extra, + ignored, + missing, + orderMatches, + }; +} + +function formatList(values) { + return values.length > 0 + ? values.map((value) => ` - ${value}`).join("\n") + : " (none)"; +} + +async function main() { + const args = parseArgs(process.argv.slice(2)); + if (args.help) { + showHelp(); + return; + } + + const selected = new Set(args.providers); + const providers = parseCanvasProviders(args.tsFile).filter( + (provider) => selected.size === 0 || selected.has(provider.key), + ); + + if (providers.length === 0) { + throw new Error( + selected.size > 0 + ? `No matching providers found: ${[...selected].join(", ")}` + : "No providers with available_models found", + ); + } + + process.stderr.write( + `${colors.cyan}ACP Provider Models Sync Check${colors.reset}\n`, + ); + + const failures = []; + + for (const provider of providers) { + process.stderr.write( + `${colors.dim}- ${provider.key}: launching ${provider.default_command.join(" ")}${colors.reset}\n`, + ); + + const runtimeModels = await fetchRuntimeModels(provider, args.timeoutMs); + const result = compareModelIds(provider, runtimeModels); + + if (result.ok) { + const ignoredText = + result.ignored.length > 0 + ? `; ignored ${result.ignored.length} default placeholder(s)` + : ""; + process.stderr.write( + ` ${colors.green}ok${colors.reset} ${result.expectedIds.length} model(s)${ignoredText}\n`, + ); + continue; + } + + failures.push({ provider, result }); + process.stderr.write(` ${colors.red}drift detected${colors.reset}\n`); + if (result.missing.length > 0) { + process.stderr.write( + ` ${colors.yellow}Missing from Canvas:${colors.reset}\n${formatList(result.missing)}\n`, + ); + } + if (result.extra.length > 0) { + process.stderr.write( + ` ${colors.yellow}Extra in Canvas:${colors.reset}\n${formatList(result.extra)}\n`, + ); + } + if ( + !result.orderMatches && + result.missing.length === 0 && + result.extra.length === 0 + ) { + process.stderr.write( + ` ${colors.yellow}Order differs.${colors.reset}\n` + + ` Runtime:\n${formatList(result.actualIds)}\n` + + ` Canvas:\n${formatList(result.expectedIds)}\n`, + ); + } + } + + if (failures.length > 0) { + throw new Error( + `${failures.length} ACP provider model list(s) drifted from their subprocess-advertised availableModels`, + ); + } + + process.stderr.write( + `${colors.green}Canvas ACP model suggestions match the ACP subprocess-advertised model IDs.${colors.reset}\n`, + ); +} + +main().catch((error) => { + process.stderr.write(`${colors.red}ERROR:${colors.reset} ${error.message}\n`); + process.exitCode = 1; +}); diff --git a/src/constants/acp-providers.ts b/src/constants/acp-providers.ts index 8ac6bb139..dd91bcfcd 100644 --- a/src/constants/acp-providers.ts +++ b/src/constants/acp-providers.ts @@ -73,46 +73,47 @@ export interface ACPModelOption { label: string; } -const CODEX_REASONING_EFFORTS = ["low", "medium", "high", "xhigh"] as const; - -function buildCodexModelOptions( - models: Array<{ id: string; label: string }>, -): ACPModelOption[] { - return models.flatMap((model) => - CODEX_REASONING_EFFORTS.map((effort) => ({ - id: `${model.id}/${effort}`, - label: `${model.label} (${effort})`, - })), - ); -} - const CLAUDE_MODELS: ACPModelOption[] = [ - { id: "claude-sonnet-4-6", label: "Claude Sonnet 4.6" }, - { id: "claude-sonnet-4-6[1m]", label: "Claude Sonnet 4.6 (1M)" }, - { id: "claude-opus-4-7", label: "Claude Opus 4.7" }, - { id: "claude-opus-4-7[1m]", label: "Claude Opus 4.7 (1M)" }, - { id: "claude-haiku-4-5", label: "Claude Haiku 4.5" }, + { id: "sonnet", label: "Claude Sonnet 4.6" }, + { id: "sonnet[1m]", label: "Claude Sonnet 4.6 (1M)" }, + { id: "haiku", label: "Claude Haiku 4.5" }, ]; -const CODEX_MODELS: ACPModelOption[] = buildCodexModelOptions([ - { id: "gpt-5.5", label: "GPT-5.5" }, - { id: "gpt-5.4", label: "GPT-5.4" }, - { id: "gpt-5.4-mini", label: "GPT-5.4 Mini" }, - { id: "gpt-5.3-codex", label: "GPT-5.3 Codex" }, - { id: "gpt-5.2", label: "GPT-5.2" }, -]); +const CODEX_MODELS: ACPModelOption[] = [ + { id: "gpt-5.5/low", label: "GPT-5.5 (low)" }, + { id: "gpt-5.5/medium", label: "GPT-5.5 (medium)" }, + { id: "gpt-5.5/high", label: "GPT-5.5 (high)" }, + { id: "gpt-5.5/xhigh", label: "GPT-5.5 (xhigh)" }, + { id: "gpt-5.4/low", label: "GPT-5.4 (low)" }, + { id: "gpt-5.4/medium", label: "GPT-5.4 (medium)" }, + { id: "gpt-5.4/high", label: "GPT-5.4 (high)" }, + { id: "gpt-5.4/xhigh", label: "GPT-5.4 (xhigh)" }, + { id: "gpt-5.4-mini/low", label: "GPT-5.4 Mini (low)" }, + { id: "gpt-5.4-mini/medium", label: "GPT-5.4 Mini (medium)" }, + { id: "gpt-5.4-mini/high", label: "GPT-5.4 Mini (high)" }, + { id: "gpt-5.4-mini/xhigh", label: "GPT-5.4 Mini (xhigh)" }, + { id: "gpt-5.3-codex/low", label: "GPT-5.3 Codex (low)" }, + { id: "gpt-5.3-codex/medium", label: "GPT-5.3 Codex (medium)" }, + { id: "gpt-5.3-codex/high", label: "GPT-5.3 Codex (high)" }, + { id: "gpt-5.3-codex/xhigh", label: "GPT-5.3 Codex (xhigh)" }, + { id: "gpt-5.2/low", label: "GPT-5.2 (low)" }, + { id: "gpt-5.2/medium", label: "GPT-5.2 (medium)" }, + { id: "gpt-5.2/high", label: "GPT-5.2 (high)" }, + { id: "gpt-5.2/xhigh", label: "GPT-5.2 (xhigh)" }, +]; const GEMINI_MODELS: ACPModelOption[] = [ - { id: "gemini-2.5-pro", label: "Gemini 2.5 Pro" }, - { id: "gemini-2.5-flash", label: "Gemini 2.5 Flash" }, - { id: "gemini-2.5-flash-lite", label: "Gemini 2.5 Flash Lite" }, - { id: "gemini-3-pro-preview", label: "Gemini 3 Pro Preview" }, - { id: "gemini-3-flash-preview", label: "Gemini 3 Flash Preview" }, - { id: "gemini-3.1-pro-preview", label: "Gemini 3.1 Pro Preview" }, + { id: "auto-gemini-3", label: "Auto (Gemini 3)" }, + { id: "auto-gemini-2.5", label: "Auto (Gemini 2.5)" }, + { id: "gemini-3.1-pro-preview", label: "gemini-3.1-pro-preview" }, + { id: "gemini-3-flash-preview", label: "gemini-3-flash-preview" }, { id: "gemini-3.1-flash-lite-preview", - label: "Gemini 3.1 Flash Lite Preview", + label: "gemini-3.1-flash-lite-preview", }, + { id: "gemini-2.5-pro", label: "gemini-2.5-pro" }, + { id: "gemini-2.5-flash", label: "gemini-2.5-flash" }, + { id: "gemini-2.5-flash-lite", label: "gemini-2.5-flash-lite" }, ]; // Each entry's ``default_command`` is the published-package npx @@ -129,7 +130,7 @@ export const ACP_PROVIDERS: ACPProviderConfig[] = [ // around the Claude Code CLI. default_command: ["npx", "-y", "@agentclientprotocol/claude-agent-acp"], available_models: CLAUDE_MODELS, - default_model: "claude-sonnet-4-6", + default_model: "sonnet", description_key: I18nKey.ONBOARDING$AGENT_CLAUDE_CODE_DESCRIPTION, icon: "claude-code", }, From 0091a9dac128ad526e043c8bd1e548e9da4c5d39 Mon Sep 17 00:00:00 2001 From: Debug Agent Date: Fri, 22 May 2026 21:13:43 +0200 Subject: [PATCH 3/6] feat(acp): per-conversation agent chip + versioned Claude model picker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit User-visible changes: - Conversation cards now show a single inline chip `[brand mark] {model}` on every conversation. ACP cards always render (identity info); OpenHands native cards render whenever `agent.llm.model` is present. New `AgentBrandIcon` covers Claude / Codex / Gemini brand marks with a terminal-glyph fallback; the OpenHands logo is recolored via `[&_path]:fill-current` so it inherits the muted-grey chip color (the shipped SVG hardcodes `fill="white"`). - Settings → Agent dropdown for Claude Code lists 10 versioned options (Opus 4.7, 4.6, 4.6/1M, 4.5, 4.1; Sonnet 4.6, 4.6/1M, 4.5; Haiku 4.5; opusplan). Default switched from `sonnet` to `claude-opus-4-7`. Canonical IDs were verified against the bundled `claude` CLI binary's model registry — the static SDK `.mjs` shims don't carry the full registry. `[1m]` aliases used for 1M-context variants (no canonical `claude-*-1m` IDs ship in the SDK). - `ConversationCardFooter` chip resolves the ACP model string from `current_model_name → current_model_id → agent.acp_model → agent.llm.model` (the `"acp-managed"` sentinel is filtered out), so the chip works whether or not the agent-server populates the SDK runtime fields. - Removed dead `showLlmProfiles` plumbing on `ConversationCard` / `CompactConversationRow` / `ConversationPanel` pass-throughs that used to gate the OpenHands model line behind a metadata-menu toggle. Chip is now always shown when a model exists. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../conversation-card.test.tsx | 108 ++++++++++++++---- .../onboarding/choose-agent-step.test.tsx | 2 +- __tests__/routes/agent-settings.test.tsx | 16 ++- .../compact-conversation-row.tsx | 3 - .../conversation-card-footer.tsx | 67 ++++++----- .../conversation-card/conversation-card.tsx | 7 +- .../conversation-panel/conversation-panel.tsx | 3 - src/components/shared/agent-brand-icon.tsx | 100 ++++++++++++++++ src/constants/acp-brand-marks.ts | 18 +++ src/constants/acp-providers.ts | 38 +++++- src/routes/agent-settings.tsx | 7 +- 11 files changed, 290 insertions(+), 79 deletions(-) create mode 100644 src/components/shared/agent-brand-icon.tsx create mode 100644 src/constants/acp-brand-marks.ts diff --git a/__tests__/components/features/conversation-panel/conversation-card.test.tsx b/__tests__/components/features/conversation-panel/conversation-card.test.tsx index f5687b9de..ac6c79dbe 100644 --- a/__tests__/components/features/conversation-panel/conversation-card.test.tsx +++ b/__tests__/components/features/conversation-panel/conversation-card.test.tsx @@ -597,12 +597,12 @@ describe("ConversationCard", () => { }); }); - describe("ACP agent badge", () => { - it("renders the resolved display name for a known ACP server", () => { - // ``claude-code`` resolves through the ACP_PROVIDERS registry to the - // human display name "Claude Code". The badge always renders for - // ACP conversations — it's identity info, not gated by the LLM- - // profile preference. + describe("Agent chip", () => { + it("renders the brand mark + model for an ACP conversation with a model", () => { + // PR 730 wires ``current_model_name``/``current_model_id``/configured + // ``acp_model`` into ``llm_model`` on the adapter so ACP conversations + // arrive at the card with a concrete model string. The chip shows the + // resolved Claude brand mark + that model text. renderWithProviders( { lastUpdatedAt="2021-10-01T12:00:00Z" agentKind="acp" acpServer="claude-code" + llmModel="sonnet" />, ); - const badge = screen.getByTestId("conversation-card-acp-badge"); - expect(badge).toHaveTextContent("Claude Code"); + const chip = screen.getByTestId("conversation-card-agent-chip"); + expect(chip).toHaveTextContent("sonnet"); + expect(chip).toHaveAttribute("title", "Claude Code · sonnet"); + expect( + within(chip).getByTestId("agent-brand-icon-claude-code"), + ).toBeInTheDocument(); }); - it("falls back to the generic 'ACP' label when the server key is unknown", () => { - // The Custom-command preset uses ``acp_server: "custom"`` (and - // future ACP servers Canvas's registry doesn't know about look the - // same here) — the resolver returns null and the chip shows the - // generic ``CONVERSATION$ACP_AGENT_GENERIC`` translation. + it("falls back to the provider display name for an ACP conversation with no model", () => { + // No ``llm_model`` (older agent-server, no SDK runtime fields, no + // configured ``acp_model``) — the chip still renders for identity, with + // the provider name as the text and the brand mark as the icon. + renderWithProviders( + , + ); + + const chip = screen.getByTestId("conversation-card-agent-chip"); + expect(chip).toHaveTextContent("Claude Code"); + expect(chip).toHaveAttribute("title", "Claude Code"); + expect( + within(chip).getByTestId("agent-brand-icon-claude-code"), + ).toBeInTheDocument(); + }); + + it("falls back to the generic terminal glyph when the server key is unknown", () => { + // ``custom`` (and any future ACP server Canvas doesn't know yet) maps + // to the fallback ``cli-generic`` icon and the generic "ACP" label. renderWithProviders( { />, ); - const badge = screen.getByTestId("conversation-card-acp-badge"); - expect(badge).toHaveTextContent("ACP"); + const chip = screen.getByTestId("conversation-card-agent-chip"); + expect(chip).toHaveTextContent("ACP"); + expect( + within(chip).getByTestId("agent-brand-icon-generic"), + ).toBeInTheDocument(); }); - it("falls back to the generic 'ACP' label when the server key is null", () => { + it("falls back to the generic terminal glyph when the server key is null", () => { // ACP conversations missing the ``acpserver`` tag (older clients, - // raw API writes) still get a chip — the goal is "this is an ACP - // conversation" first, exact provider second. + // raw API writes) still get a chip — identity first, exact provider + // second. renderWithProviders( { />, ); - const badge = screen.getByTestId("conversation-card-acp-badge"); - expect(badge).toHaveTextContent("ACP"); + const chip = screen.getByTestId("conversation-card-agent-chip"); + expect(chip).toHaveTextContent("ACP"); + expect( + within(chip).getByTestId("agent-brand-icon-generic"), + ).toBeInTheDocument(); }); - it("does not render the badge for OpenHands conversations", () => { - // The OpenHands rendering path must be untouched — even if a stray - // ``acp_server`` value somehow reaches the prop, the chip stays - // hidden because ``agentKind !== "acp"``. + it("renders the OpenHands logo + model name for native conversations", () => { + // OpenHands native conversations show the same chip pattern as ACP + // cards: the OpenHands logo + the raw ``agent.llm.model`` string. No + // user-preference gate — surfacing the model is identity info, same + // as the ACP path. A stray ``acp_server`` value on an OpenHands card + // must not flip the icon to the Claude/Codex/Gemini brand mark. renderWithProviders( { lastUpdatedAt="2021-10-01T12:00:00Z" agentKind="openhands" acpServer="claude-code" + llmModel="claude-sonnet-4" + />, + ); + + const chip = screen.getByTestId("conversation-card-agent-chip"); + expect(chip).toHaveTextContent("claude-sonnet-4"); + expect(chip).toHaveAttribute("title", "claude-sonnet-4"); + expect( + within(chip).getByTestId("agent-brand-icon-openhands"), + ).toBeInTheDocument(); + expect( + within(chip).queryByTestId("agent-brand-icon-claude-code"), + ).not.toBeInTheDocument(); + }); + + it("hides the chip for OpenHands conversations with no model", () => { + // No model string and no ACP server — nothing to display, so the + // chip collapses entirely rather than showing a bare logo. + renderWithProviders( + , ); expect( - screen.queryByTestId("conversation-card-acp-badge"), + screen.queryByTestId("conversation-card-agent-chip"), ).not.toBeInTheDocument(); }); }); diff --git a/__tests__/components/onboarding/choose-agent-step.test.tsx b/__tests__/components/onboarding/choose-agent-step.test.tsx index 4cf9b0e1b..fd16ae31f 100644 --- a/__tests__/components/onboarding/choose-agent-step.test.tsx +++ b/__tests__/components/onboarding/choose-agent-step.test.tsx @@ -168,7 +168,7 @@ describe("ChooseAgentStep", () => { // ``acp_args`` can't survive and concatenate onto the spawn // command at conversation-create time. acp_args: [], - acp_model: "sonnet", + acp_model: "claude-opus-4-7", }); }); diff --git a/__tests__/routes/agent-settings.test.tsx b/__tests__/routes/agent-settings.test.tsx index 8daabfa13..bd27f4b6e 100644 --- a/__tests__/routes/agent-settings.test.tsx +++ b/__tests__/routes/agent-settings.test.tsx @@ -152,6 +152,10 @@ describe("AgentSettingsScreen", () => { }); it("shows the ACP form when the active agent_kind is acp", async () => { + // Use a model ID that isn't in CLAUDE_MODELS so the form falls through to + // the custom-input branch — that's the path this test is asserting (saved + // value round-trips into the visible input). Known IDs go through the + // dropdown instead and are covered separately. vi.spyOn(SettingsService, "getSettings").mockResolvedValue( buildSettings({ agent_settings: { @@ -159,7 +163,7 @@ describe("AgentSettingsScreen", () => { agent_kind: "acp", acp_server: "claude-code", acp_command: ["npx", "-y", "@agentclientprotocol/claude-agent-acp"], - acp_model: "claude-opus-4-5", + acp_model: "my-pinned-fork-model", }, }), ); @@ -174,7 +178,7 @@ describe("AgentSettingsScreen", () => { const modelInput = screen.getByTestId( "agent-model-input", ) as HTMLInputElement; - expect(modelInput.value).toBe("claude-opus-4-5"); + expect(modelInput.value).toBe("my-pinned-fork-model"); }); it("defaults built-in ACP providers to a suggested model when none is saved", async () => { @@ -194,7 +198,7 @@ describe("AgentSettingsScreen", () => { await screen.findByTestId("agent-command-input"); expect(screen.getByLabelText("SETTINGS$AGENT_MODEL")).toHaveValue( - "Claude Sonnet 4.6", + "Claude Opus 4.7", ); }); @@ -225,7 +229,7 @@ describe("AgentSettingsScreen", () => { const call = save.mock.calls[0]?.[0] as { agent_settings_diff?: Record; }; - expect(call.agent_settings_diff?.acp_model).toBe("haiku"); + expect(call.agent_settings_diff?.acp_model).toBe("claude-haiku-4-5"); }); it("saves an ACP diff when switching to ACP + Claude Code", async () => { @@ -257,7 +261,7 @@ describe("AgentSettingsScreen", () => { "npx -y @agentclientprotocol/claude-agent-acp", ); expect(screen.getByLabelText("SETTINGS$AGENT_MODEL")).toHaveValue( - "Claude Sonnet 4.6", + "Claude Opus 4.7", ); await user.click(screen.getByTestId("agent-save-button")); @@ -279,7 +283,7 @@ describe("AgentSettingsScreen", () => { // ``acp_args`` can't survive and concatenate onto the spawn // command at conversation-create time. acp_args: [], - acp_model: "sonnet", + acp_model: "claude-opus-4-7", }); }); diff --git a/src/components/features/conversation-panel/compact-conversation-row.tsx b/src/components/features/conversation-panel/compact-conversation-row.tsx index 1875053ff..ad32aa311 100644 --- a/src/components/features/conversation-panel/compact-conversation-row.tsx +++ b/src/components/features/conversation-panel/compact-conversation-row.tsx @@ -21,7 +21,6 @@ interface CompactConversationRowProps { onClose?: () => void; showRepositoryMetadata?: boolean; llmModel?: string | null; - showLlmProfiles?: boolean; agentKind?: "openhands" | "acp" | null; acpServer?: string | null; } @@ -44,7 +43,6 @@ export function CompactConversationRow({ onClose, showRepositoryMetadata = true, llmModel = null, - showLlmProfiles = false, agentKind = null, acpServer = null, }: CompactConversationRowProps) { @@ -70,7 +68,6 @@ export function CompactConversationRow({ workspaceWorkingDir={workspaceWorkingDir} showRepositoryMetadata={showRepositoryMetadata} llmModel={llmModel} - showLlmModel={showLlmProfiles} agentKind={agentKind} acpServer={acpServer} /> diff --git a/src/components/features/conversation-panel/conversation-card/conversation-card-footer.tsx b/src/components/features/conversation-panel/conversation-card/conversation-card-footer.tsx index 8ab26ab31..2c75a03db 100644 --- a/src/components/features/conversation-panel/conversation-card/conversation-card-footer.tsx +++ b/src/components/features/conversation-panel/conversation-card/conversation-card-footer.tsx @@ -5,7 +5,12 @@ import { I18nKey } from "#/i18n/declaration"; import { RepositorySelection } from "#/api/open-hands.types"; import { ExecutionStatus } from "#/types/agent-server/core/base/common"; import { isExecutionPaused } from "#/utils/status"; -import { getAcpProviderDisplayName } from "#/constants/acp-providers"; +import { + getAcpProviderDisplayName, + resolveAcpProviderIcon, + type ACPProviderIcon, +} from "#/constants/acp-providers"; +import { AgentBrandIcon } from "#/components/shared/agent-brand-icon"; import { ConversationRepoLink } from "./conversation-repo-link"; import { NoRepository } from "./no-repository"; @@ -18,13 +23,11 @@ interface ConversationCardFooterProps { showRepositoryMetadata?: boolean; showTimestamp?: boolean; llmModel?: string | null; - showLlmModel?: boolean; /** - * High-level kind of the conversation's agent. The ACP-agent chip is - * only rendered when this is ``"acp"``. The OpenHands rendering path - * is intentionally untouched — for OpenHands conversations the chip is - * suppressed regardless of any ``acpServer`` value (defensive against - * stray wire tags on non-ACP conversations). + * High-level kind of the conversation's agent. Drives the chip's icon: + * the OpenHands logo for native conversations and the resolved ACP brand + * mark for ACP conversations. Defensive against stray ``acpServer`` + * values reaching an OpenHands card. */ agentKind?: "openhands" | "acp" | null; /** @@ -32,9 +35,7 @@ interface ConversationCardFooterProps { * ``"gemini-cli"`` / unknown / null). Resolved to a human display name * via {@link getAcpProviderDisplayName}; unknown / null falls back to * a generic "ACP" label so a Custom-command preset still produces a - * useful chip. Always shown for ACP conversations — this is identity - * info, not gated by the ``showLlmModel`` preference (which is about - * LLM model strings, an orthogonal concern). + * useful chip. */ acpServer?: string | null; } @@ -48,7 +49,6 @@ export function ConversationCardFooter({ showRepositoryMetadata = true, showTimestamp = true, llmModel, - showLlmModel = false, agentKind = null, acpServer = null, }: ConversationCardFooterProps) { @@ -56,11 +56,27 @@ export function ConversationCardFooter({ const isPaused = isExecutionPaused(executionStatus); - const acpDisplayName = - agentKind === "acp" - ? (getAcpProviderDisplayName(acpServer) ?? - t(I18nKey.CONVERSATION$ACP_AGENT_GENERIC)) - : null; + // Single inline chip per conversation: [brand mark] {model text}. Always + // shown when there's something meaningful to render — OpenHands shows the + // logo + ``agent.llm.model``; ACP shows the provider brand mark + model + // resolved through PR 730's adapter chain, falling back to the provider + // display name when no model is available so the chip never collapses to + // icon-only. + let chip: { kind: ACPProviderIcon; text: string; tooltip: string } | null = + null; + if (agentKind === "acp") { + const providerName = + getAcpProviderDisplayName(acpServer) ?? + t(I18nKey.CONVERSATION$ACP_AGENT_GENERIC); + const text = llmModel ?? providerName; + chip = { + kind: resolveAcpProviderIcon(acpServer), + text, + tooltip: llmModel ? `${providerName} · ${llmModel}` : providerName, + }; + } else if (llmModel) { + chip = { kind: "openhands", text: llmModel, tooltip: llmModel }; + } return (
    - {acpDisplayName ? ( + {chip ? (
    - {acpDisplayName} + + {chip.text}
    ) : null} - {showLlmModel && llmModel ? ( - - {llmModel} - - ) : null}
    diff --git a/src/components/features/conversation-panel/conversation-panel.tsx b/src/components/features/conversation-panel/conversation-panel.tsx index 4c9c29fa8..8f7f756ab 100644 --- a/src/components/features/conversation-panel/conversation-panel.tsx +++ b/src/components/features/conversation-panel/conversation-panel.tsx @@ -408,7 +408,6 @@ export function ConversationPanel({ onClose={onClose} showRepositoryMetadata={showRepoBranchMetadata} llmModel={conversation.llm_model} - showLlmProfiles={showLlmProfiles} agentKind={conversation.agent_kind} acpServer={conversation.acp_server} /> @@ -451,7 +450,6 @@ export function ConversationPanel({ } showRepositoryMetadata={showRepoBranchMetadata} llmModel={conversation.llm_model} - showLlmProfiles={showLlmProfiles} agentKind={conversation.agent_kind} acpServer={conversation.acp_server} /> @@ -467,7 +465,6 @@ export function ConversationPanel({ onClose, openContextMenuId, showRepoBranchMetadata, - showLlmProfiles, ], ); diff --git a/src/components/shared/agent-brand-icon.tsx b/src/components/shared/agent-brand-icon.tsx new file mode 100644 index 000000000..c853e627b --- /dev/null +++ b/src/components/shared/agent-brand-icon.tsx @@ -0,0 +1,100 @@ +import OpenHandsLogo from "#/assets/branding/openhands-logo.svg?react"; +import TerminalIcon from "#/icons/terminal.svg?react"; +import { + CLAUDE_CODE_MARK_PATH, + CLAUDE_CODE_VIEWBOX, + CODEX_MARK_PATH, + CODEX_VIEWBOX, + GEMINI_MARK_PATH, + GEMINI_VIEWBOX, +} from "#/constants/acp-brand-marks"; +import type { ACPProviderIcon } from "#/constants/acp-providers"; +import { cn } from "#/utils/utils"; + +interface AgentBrandIconProps { + kind: ACPProviderIcon; + size?: number; + className?: string; + "data-testid"?: string; +} + +export function AgentBrandIcon({ + kind, + size = 12, + className, + "data-testid": testId, +}: AgentBrandIconProps) { + if (kind === "openhands") { + // The shipped SVG paints every path ``fill="white"``. Override with + // ``fill-current`` on the descendant ```` nodes so the logo + // inherits the chip's text color. + return ( + + ); + } + if (kind === "claude-code") { + return ( + + + + ); + } + if (kind === "codex") { + return ( + + + + ); + } + if (kind === "gemini") { + return ( + + + + ); + } + return ( + + ); +} diff --git a/src/constants/acp-brand-marks.ts b/src/constants/acp-brand-marks.ts new file mode 100644 index 000000000..ea510d7d4 --- /dev/null +++ b/src/constants/acp-brand-marks.ts @@ -0,0 +1,18 @@ +// Brand-mark SVG path data for the agent harnesses we recognise on a +// conversation chip. Kept as raw path strings (drawn with ``currentColor`` +// so callers can size & colour them via Tailwind) rather than embedded +// ```` files to avoid an extra asset round-trip and so the icons can +// be reused inline at multiple sizes (onboarding tile, conversation chip, +// settings preview). + +export const CLAUDE_CODE_MARK_PATH = + "m19.6 66.5 19.7-11 .3-1-.3-.5h-1l-3.3-.2-11.2-.3L14 53l-9.5-.5-2.4-.5L0 49l.2-1.5 2-1.3 2.9.2 6.3.5 9.5.6 6.9.4L38 49.1h1.6l.2-.7-.5-.4-.4-.4L29 41l-10.6-7-5.6-4.1-3-2-1.5-2-.6-4.2 2.7-3 3.7.3.9.2 3.7 2.9 8 6.1L37 36l1.5 1.2.6-.4.1-.3-.7-1.1L33 25l-6-10.4-2.7-4.3-.7-2.6c-.3-1-.4-2-.4-3l3-4.2L28 0l4.2.6L33.8 2l2.6 6 4.1 9.3L47 29.9l2 3.8 1 3.4.3 1h.7v-.5l.5-7.2 1-8.7 1-11.2.3-3.2 1.6-3.8 3-2L61 2.6l2 2.9-.3 1.8-1.1 7.7L59 27.1l-1.5 8.2h.9l1-1.1 4.1-5.4 6.9-8.6 3-3.5L77 13l2.3-1.8h4.3l3.1 4.7-1.4 4.9-4.4 5.6-3.7 4.7-5.3 7.1-3.2 5.7.3.4h.7l12-2.6 6.4-1.1 7.6-1.3 3.5 1.6.4 1.6-1.4 3.4-8.2 2-9.6 2-14.3 3.3-.2.1.2.3 6.4.6 2.8.2h6.8l12.6 1 3.3 2 1.9 2.7-.3 2-5.1 2.6-6.8-1.6-16-3.8-5.4-1.3h-.8v.4l4.6 4.5 8.3 7.5L89 80.1l.5 2.4-1.3 2-1.4-.2-9.2-7-3.6-3-8-6.8h-.5v.7l1.8 2.7 9.8 14.7.5 4.5-.7 1.4-2.6 1-2.7-.6-5.8-8-6-9-4.7-8.2-.5.4-2.9 30.2-1.3 1.5-3 1.2-2.5-2-1.4-3 1.4-6.2 1.6-8 1.3-6.4 1.2-7.9.7-2.6v-.2H49L43 72l-9 12.3-7.2 7.6-1.7.7-3-1.5.3-2.8L24 86l10-12.8 6-7.9 4-4.6-.1-.5h-.3L17.2 77.4l-4.7.6-2-2 .2-3 1-1 8-5.5Z"; +export const CLAUDE_CODE_VIEWBOX = "0 0 100 100"; + +export const CODEX_MARK_PATH = + "M4.04286 0.228393C4.52451 0.0304495 5.0488 -0.0409817 5.56586 0.0208928C6.23236 0.0973928 6.82636 0.380893 7.34786 0.870893C7.35488 0.877545 7.36344 0.882351 7.37278 0.884881C7.38212 0.887412 7.39194 0.887588 7.40136 0.885393C8.10536 0.712393 8.78236 0.773393 9.43186 1.06839L9.46336 1.08339L9.54036 1.12139C10.2189 1.47289 10.7054 2.00639 10.9994 2.72039C11.1384 3.05989 11.2084 3.41439 11.2099 3.78339C11.2197 4.05816 11.1893 4.33288 11.1199 4.59889C11.1164 4.61245 11.1165 4.62665 11.12 4.6402C11.1235 4.65374 11.1303 4.66618 11.1399 4.67639C11.5329 5.0749 11.8063 5.57572 11.9289 6.12189C12.1214 7.07239 11.9239 7.92939 11.3374 8.69189L11.2464 8.80189C10.8579 9.24669 10.3481 9.56836 9.77936 9.72739C9.76694 9.73097 9.75556 9.73747 9.74617 9.74634C9.73677 9.75521 9.72964 9.7662 9.72536 9.77839C9.59786 10.1464 9.46986 10.4604 9.23186 10.7744C8.63236 11.5654 7.75086 12.0054 6.75786 11.9999C5.96636 11.9959 5.26486 11.7064 4.65286 11.1319C4.64358 11.1234 4.63225 11.1174 4.61998 11.1146C4.6077 11.1118 4.59491 11.1123 4.58286 11.1159C4.32386 11.1994 4.06286 11.2114 3.78086 11.2084C3.33033 11.2048 2.88658 11.0984 2.48336 10.8974C2.0613 10.688 1.6939 10.3831 1.41036 10.0069C1.30886 9.87239 1.20836 9.74589 1.13486 9.59639C1.03349 9.39033 0.95066 9.17565 0.887357 8.95489C0.754446 8.45324 0.751521 7.92599 0.878857 7.42289C0.882974 7.41102 0.884341 7.39837 0.882857 7.38589C0.88038 7.37348 0.873877 7.36223 0.864357 7.35389C0.556147 7.04213 0.320543 6.66619 0.174357 6.25289C0.0775698 5.99842 0.0213841 5.73031 0.00785682 5.45839C-0.0163229 5.10033 0.0153902 4.74069 0.101857 4.39239C0.326857 3.65039 0.756357 3.06839 1.39036 2.64589C1.53136 2.55189 1.66536 2.47889 1.79136 2.42689C1.93436 2.36689 2.07786 2.31689 2.22186 2.27489C2.23216 2.27184 2.24153 2.26626 2.24913 2.25866C2.25672 2.25107 2.2623 2.24169 2.26536 2.23139C2.37455 1.83888 2.56235 1.47264 2.81736 1.15489C3.15736 0.731893 3.56586 0.422893 4.04286 0.228393ZM3.64086 4.15339C3.58503 4.05573 3.49269 3.98424 3.38415 3.95465C3.27562 3.92507 3.15977 3.93981 3.06211 3.99564C2.96444 4.05147 2.89295 4.14381 2.86337 4.25235C2.83378 4.36088 2.84853 4.47673 2.90436 4.57439L3.75136 6.05689L2.90736 7.48089C2.85561 7.57738 2.84315 7.69014 2.87257 7.7956C2.902 7.90106 2.97104 7.99108 3.06526 8.04684C3.15949 8.1026 3.27162 8.1198 3.37823 8.09484C3.48484 8.06988 3.57768 8.00469 3.63736 7.91289L4.60736 6.27689C4.64561 6.21237 4.66609 6.13887 4.66671 6.06386C4.66732 5.98886 4.64805 5.91503 4.61086 5.84989L3.64086 4.15339ZM6.36386 7.27339C6.25583 7.27982 6.15434 7.32727 6.08012 7.40603C6.00591 7.48479 5.96458 7.58892 5.96458 7.69714C5.96458 7.80536 6.00591 7.90949 6.08012 7.98826C6.15434 8.06702 6.25583 8.11446 6.36386 8.12089H8.78786C8.89675 8.1156 8.99943 8.06862 9.07462 7.98969C9.14982 7.91075 9.19176 7.80591 9.19176 7.69689C9.19176 7.58787 9.14982 7.48303 9.07462 7.4041C8.99943 7.32516 8.89675 7.27818 8.78786 7.27289H6.36386V7.27339Z"; +export const CODEX_VIEWBOX = "0 0 12 12"; + +export const GEMINI_MARK_PATH = + "M12 0C12.904 6.056 17.944 11.096 24 12C17.944 12.904 12.904 17.944 12 24C11.096 17.944 6.056 12.904 0 12C6.056 11.096 11.096 6.056 12 0Z"; +export const GEMINI_VIEWBOX = "0 0 24 24"; diff --git a/src/constants/acp-providers.ts b/src/constants/acp-providers.ts index dd91bcfcd..28318bf80 100644 --- a/src/constants/acp-providers.ts +++ b/src/constants/acp-providers.ts @@ -73,10 +73,25 @@ export interface ACPModelOption { label: string; } +// Canonical model IDs the Claude Code CLI binary's model registry recognises +// (verified by string-scanning v2.1.146 of the bundled ``claude`` native +// binary). ``[1m]`` is the SDK-documented 1M-context suffix; we use the +// version-agnostic alias so the option auto-tracks the newest 1M-capable +// model. ``opusplan`` routes planning to Opus and execution to Sonnet. +// Availability for any of these ultimately depends on the user's Anthropic +// plan tier — surfacing them here matches what the CLI *accepts*, not what +// every account can actually invoke. const CLAUDE_MODELS: ACPModelOption[] = [ - { id: "sonnet", label: "Claude Sonnet 4.6" }, + { id: "claude-opus-4-7", label: "Claude Opus 4.7" }, + { id: "claude-opus-4-6", label: "Claude Opus 4.6" }, + { id: "opus[1m]", label: "Claude Opus 4.6 (1M)" }, + { id: "claude-opus-4-5", label: "Claude Opus 4.5" }, + { id: "claude-opus-4-1-20250805", label: "Claude Opus 4.1" }, + { id: "claude-sonnet-4-6", label: "Claude Sonnet 4.6" }, { id: "sonnet[1m]", label: "Claude Sonnet 4.6 (1M)" }, - { id: "haiku", label: "Claude Haiku 4.5" }, + { id: "claude-sonnet-4-5", label: "Claude Sonnet 4.5" }, + { id: "claude-haiku-4-5", label: "Claude Haiku 4.5" }, + { id: "opusplan", label: "Opus (plan) + Sonnet (execute)" }, ]; const CODEX_MODELS: ACPModelOption[] = [ @@ -130,7 +145,7 @@ export const ACP_PROVIDERS: ACPProviderConfig[] = [ // around the Claude Code CLI. default_command: ["npx", "-y", "@agentclientprotocol/claude-agent-acp"], available_models: CLAUDE_MODELS, - default_model: "sonnet", + default_model: "claude-opus-4-7", description_key: I18nKey.ONBOARDING$AGENT_CLAUDE_CODE_DESCRIPTION, icon: "claude-code", }, @@ -187,6 +202,23 @@ export function getAcpProviderDisplayName( return found ? found.display_name : null; } +/** + * Resolve an ACP provider registry key to the icon discriminator the + * conversation chip should render alongside the model text. + * + * Falls back to {@link ACP_PROVIDER_FALLBACK_ICON} for ``"custom"``, + * unknown keys, or a missing key — the chip then shows a neutral + * terminal glyph that still communicates "this is an ACP conversation" + * without claiming a brand identity we don't know. + */ +export function resolveAcpProviderIcon( + key: string | null | undefined, +): ACPProviderIcon { + if (!key) return ACP_PROVIDER_FALLBACK_ICON; + const found = ACP_PROVIDERS.find((p) => p.key === key); + return found?.icon ?? ACP_PROVIDER_FALLBACK_ICON; +} + /** * Build the ``agent_settings_diff`` payload PATCH /api/settings expects * for the agent-kind/provider choice the user just made. diff --git a/src/routes/agent-settings.tsx b/src/routes/agent-settings.tsx index 4d2981073..3102ba1f6 100644 --- a/src/routes/agent-settings.tsx +++ b/src/routes/agent-settings.tsx @@ -212,11 +212,12 @@ function AgentSettingsScreen() { : selectedProvider && isDefaultProviderCommand ? selectedProvider.key : ACP_CUSTOM_PRESET_KEY; - const effectiveModel = - acpModel.trim() || (selectedProvider?.default_model ?? null); + // ``model: undefined`` lets buildAcpAgentSettingsDiff seed the provider's + // ``default_model`` for built-in keys; for the custom preset it falls + // through to ``null`` since custom has no default. const agentSettingsDiff = buildAcpAgentSettingsDiff(providerKey, { command: useDefault ? [] : commandTokens, - model: effectiveModel, + model: acpModel.trim() || undefined, allowUnknownServer: preserveUnknownServer, }); From cf6fa37d65c37d4f304f49b79e2cfaf08b67f826 Mon Sep 17 00:00:00 2001 From: Debug Agent Date: Fri, 22 May 2026 21:13:59 +0200 Subject: [PATCH 4/6] refactor(acp): adapter cleanup + drop unauthenticated model-check script MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Extract `resolveAcpDisplayModel(info)` from the triple-nested ternary in `toAppConversation`. Same precedence chain (runtime name → runtime id → configured model → llm.model minus sentinel), just expressed once with a loop and named conditions. - Name the `"acp-managed"` literal as `ACP_MANAGED_SENTINEL` (exported) and the `"default" / "default (recommended)"` strings as `ACP_DEFAULT_PLACEHOLDERS`. The sentinel was referenced from 7 places across source and tests. - Trim `DirectConversationInfo.agent.kind` JSDoc to 3 lines (was 7). - Delete `scripts/check-acp-provider-models.mjs` and revert the CI step + triggers in `.github/workflows/acp-providers-sync.yml`. The script spawned each ACP wrapper unauthenticated and validated Canvas's lists against the wrapper's fallback model set — which is only 3 models for Claude Code (sonnet / sonnet[1m] / haiku) regardless of what the wrapper actually accepts. The check flagged every legitimately-added Opus entry as drift. Removing it; followup tracked in #740 for moving the lists into `@openhands/typescript-client`. - Remove `test:acp-models` npm script (only invoked the deleted file). Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/acp-providers-sync.yml | 8 +- package.json | 1 - scripts/check-acp-provider-models.mjs | 624 ----------------------- src/api/agent-server-adapter.ts | 51 +- 4 files changed, 31 insertions(+), 653 deletions(-) delete mode 100644 scripts/check-acp-provider-models.mjs diff --git a/.github/workflows/acp-providers-sync.yml b/.github/workflows/acp-providers-sync.yml index 560002f72..ffb780e1c 100644 --- a/.github/workflows/acp-providers-sync.yml +++ b/.github/workflows/acp-providers-sync.yml @@ -4,14 +4,12 @@ name: ACP Providers Sync # (src/constants/acp-providers.ts) and the SDK's source of truth at # openhands-sdk/openhands/sdk/settings/acp_providers.py in # OpenHands/software-agent-sdk. See agent-canvas#587 for the failure mode -# this exists to prevent. Also launches the built-in ACP subprocesses and -# verifies Canvas's suggested model IDs match their advertised availableModels. +# this exists to prevent. on: pull_request: paths: - "src/constants/acp-providers.ts" - - "scripts/check-acp-provider-models.mjs" - "scripts/check-acp-providers-sync.mjs" - ".github/workflows/acp-providers-sync.yml" push: @@ -19,7 +17,6 @@ on: - main paths: - "src/constants/acp-providers.ts" - - "scripts/check-acp-provider-models.mjs" - "scripts/check-acp-providers-sync.mjs" - ".github/workflows/acp-providers-sync.yml" @@ -84,6 +81,3 @@ jobs: # repository_dispatch payload; otherwise the script defaults to "main". ACP_SDK_REF: ${{ inputs.sdk_ref || github.event.client_payload.sdk_ref || '' }} run: node scripts/check-acp-providers-sync.mjs - - - name: Check ACP provider models sync - run: node scripts/check-acp-provider-models.mjs diff --git a/package.json b/package.json index 7721dc4e5..70b4b1c88 100644 --- a/package.json +++ b/package.json @@ -82,7 +82,6 @@ "test:e2e:live": "node --env-file-if-exists=.env tests/e2e/live/scripts/run-live-e2e.mjs", "test:e2e:snapshots": "playwright test tests/e2e/snapshots --project=chromium --retries=0", "test:e2e:snapshots:update": "playwright test tests/e2e/snapshots --project=chromium --update-snapshots", - "test:acp-models": "node scripts/check-acp-provider-models.mjs", "test:coverage": "npm run make-i18n && vitest run --coverage", "dev_wsl": "VITE_WATCH_USE_POLLING=true vite", "preview": "vite preview", diff --git a/scripts/check-acp-provider-models.mjs b/scripts/check-acp-provider-models.mjs deleted file mode 100644 index 5cf666f16..000000000 --- a/scripts/check-acp-provider-models.mjs +++ /dev/null @@ -1,624 +0,0 @@ -#!/usr/bin/env node - -/** - * Check ACP Provider Models Sync - * - * Launches each built-in ACP subprocess from src/constants/acp-providers.ts, - * creates a minimal ACP session, reads the process-advertised - * models.availableModels, and compares those model IDs to Canvas's - * available_models suggestions. - * - * Canvas intentionally filters generic "default" placeholders such as Claude - * Code's "Default (recommended)" before comparing. The UI must surface concrete - * model choices, not a provider-specific default sentinel. - * - * Usage: - * node scripts/check-acp-provider-models.mjs - * node scripts/check-acp-provider-models.mjs --provider codex - * - * Options: - * --provider Check only one provider key. Can be passed multiple times. - * --timeout-ms Timeout for each ACP request. Default: 30000. - * --help, -h Show help. - * - * Exit codes: - * 0 - Canvas suggestions match the ACP subprocess-advertised model IDs. - * 1 - Drift detected, subprocess error, or parse error. - */ - -import { spawn } from "node:child_process"; -import { - mkdirSync, - mkdtempSync, - readFileSync, - rmSync, - writeFileSync, -} from "node:fs"; -import { tmpdir } from "node:os"; -import { dirname, join } from "node:path"; -import readline from "node:readline"; -import { fileURLToPath } from "node:url"; -import process from "node:process"; - -const __dirname = dirname(fileURLToPath(import.meta.url)); -const projectRoot = join(__dirname, ".."); -const DEFAULT_TS_PATH = join( - projectRoot, - "src", - "constants", - "acp-providers.ts", -); - -const colors = { - reset: "\x1b[0m", - red: "\x1b[31m", - green: "\x1b[32m", - yellow: "\x1b[33m", - cyan: "\x1b[36m", - dim: "\x1b[2m", -}; - -function parseArgs(argv) { - const args = { - help: false, - providers: [], - timeoutMs: Number(process.env.ACP_MODEL_CHECK_TIMEOUT_MS || 30_000), - tsFile: DEFAULT_TS_PATH, - }; - - for (let i = 0; i < argv.length; i++) { - const arg = argv[i]; - if (arg === "--help" || arg === "-h") { - args.help = true; - } else if (arg === "--provider") { - args.providers.push(argv[++i]); - } else if (arg === "--timeout-ms") { - args.timeoutMs = Number(argv[++i]); - } else if (arg === "--ts-file") { - args.tsFile = argv[++i]; - } else { - throw new Error(`Unknown argument: ${arg}`); - } - } - - if (!Number.isFinite(args.timeoutMs) || args.timeoutMs <= 0) { - throw new Error("--timeout-ms must be a positive number"); - } - - return args; -} - -function showHelp() { - process.stdout.write( - `\nACP Provider Models Sync Check\n\n` + - `Launches each built-in ACP subprocess and verifies that Canvas's\n` + - `available_models suggestions match the model IDs advertised by\n` + - `session/new -> models.availableModels, after filtering generic\n` + - `default placeholders.\n\n` + - `Usage:\n` + - ` node scripts/check-acp-provider-models.mjs [options]\n\n` + - `Options:\n` + - ` --provider Check only one provider key. Repeatable.\n` + - ` --timeout-ms Timeout for each ACP request. Default: 30000.\n` + - ` --ts-file Override TS mirror path.\n` + - ` --help, -h Show this help.\n\n`, - ); -} - -function findMatchingDelimiter(source, openIdx, open, close) { - let depth = 0; - let i = openIdx; - - while (i < source.length) { - const char = source[i]; - - if (char === '"' || char === "'" || char === "`") { - const quote = char; - i++; - while (i < source.length) { - if (source[i] === "\\") { - i += 2; - continue; - } - if (source[i] === quote) { - i++; - break; - } - i++; - } - continue; - } - - if (char === "/" && source[i + 1] === "/") { - while (i < source.length && source[i] !== "\n") i++; - continue; - } - - if (char === "/" && source[i + 1] === "*") { - i += 2; - while ( - i < source.length - 1 && - !(source[i] === "*" && source[i + 1] === "/") - ) { - i++; - } - i += 2; - continue; - } - - if (char === open) depth++; - if (char === close) { - depth--; - if (depth === 0) return i; - } - i++; - } - - return -1; -} - -function extractStringList(source) { - const out = []; - const re = /"((?:\\.|[^"\\])*)"/g; - let match; - while ((match = re.exec(source)) !== null) { - out.push(match[1].replace(/\\(.)/g, "$1")); - } - return out; -} - -function parseModelOptionArrays(source) { - const arrays = new Map(); - const re = /const\s+(\w+)\s*:\s*ACPModelOption\[\]\s*=\s*\[/g; - let match; - - while ((match = re.exec(source)) !== null) { - const name = match[1]; - const equalsIdx = source.indexOf("=", match.index); - const openIdx = source.indexOf("[", equalsIdx); - const closeIdx = findMatchingDelimiter(source, openIdx, "[", "]"); - if (closeIdx === -1) { - throw new Error(`Could not find end of model array ${name}`); - } - - const body = source.slice(openIdx + 1, closeIdx); - const models = splitTopLevelObjects(body).map((modelSource) => { - const id = modelSource.match(/id:\s*"((?:\\.|[^"\\])*)"/)?.[1]; - const label = modelSource.match(/label:\s*"((?:\\.|[^"\\])*)"/)?.[1]; - if (!id || !label) { - throw new Error( - `Could not parse model option in ${name}: ${modelSource}`, - ); - } - return { - id: id.replace(/\\(.)/g, "$1"), - label: label.replace(/\\(.)/g, "$1"), - }; - }); - - arrays.set(name, models); - re.lastIndex = closeIdx + 1; - } - - return arrays; -} - -function splitTopLevelObjects(source) { - const objects = []; - let i = 0; - - while (i < source.length) { - if (source[i] !== "{") { - i++; - continue; - } - - const closeIdx = findMatchingDelimiter(source, i, "{", "}"); - if (closeIdx === -1) { - throw new Error("Could not find end of provider object"); - } - - objects.push(source.slice(i, closeIdx + 1)); - i = closeIdx + 1; - } - - return objects; -} - -function parseCanvasProviders(tsFile) { - const source = readFileSync(tsFile, "utf8"); - const modelArrays = parseModelOptionArrays(source); - const providersStart = source.search( - /export\s+const\s+ACP_PROVIDERS\s*:\s*ACPProviderConfig\[\]\s*=\s*\[/, - ); - if (providersStart === -1) { - throw new Error("Could not find exported ACP_PROVIDERS array"); - } - - const equalsIdx = source.indexOf("=", providersStart); - const openIdx = source.indexOf("[", equalsIdx); - const closeIdx = findMatchingDelimiter(source, openIdx, "[", "]"); - if (closeIdx === -1) { - throw new Error("Could not find end of ACP_PROVIDERS array"); - } - - const providerObjects = splitTopLevelObjects( - source.slice(openIdx + 1, closeIdx), - ); - - return providerObjects - .map((objectSource) => { - const key = objectSource.match(/key:\s*"([^"]+)"/)?.[1]; - const displayName = objectSource.match(/display_name:\s*"([^"]+)"/)?.[1]; - const defaultCommandBody = objectSource.match( - /default_command:\s*\[([\s\S]*?)\]/, - )?.[1]; - const modelArrayName = objectSource.match( - /available_models:\s*(\w+)/, - )?.[1]; - const defaultModel = objectSource.match( - /default_model:\s*"([^"]+)"/, - )?.[1]; - - if (!key || !displayName || !defaultCommandBody) { - throw new Error(`Could not parse provider object:\n${objectSource}`); - } - - return { - key, - display_name: displayName, - default_command: extractStringList(defaultCommandBody), - available_models: modelArrayName - ? (modelArrays.get(modelArrayName) ?? []) - : [], - default_model: defaultModel ?? null, - }; - }) - .filter((provider) => provider.available_models.length > 0); -} - -function isGenericDefaultModel(model) { - const id = String(model.modelId ?? "") - .trim() - .toLowerCase(); - const name = String(model.name ?? "") - .trim() - .toLowerCase(); - return ( - id === "default" || name === "default" || name === "default (recommended)" - ); -} - -function providerEnv(providerKey, tempRoot) { - const env = { - ...process.env, - CI: "1", - NO_COLOR: "1", - }; - - if (providerKey === "claude-code") { - env.CLAUDE_CONFIG_DIR = join(tempRoot, "claude-config"); - } - - if (providerKey === "codex") { - env.CODEX_HOME = join(tempRoot, "codex-home"); - } - - if (providerKey === "gemini-cli") { - env.GEMINI_CLI_HOME = join(tempRoot, "gemini-home"); - env.GEMINI_API_KEY = - process.env.GEMINI_API_KEY || "dummy-acp-model-drift-check"; - } - - return env; -} - -function writeJsonLine(stream, message) { - stream.write(`${JSON.stringify({ jsonrpc: "2.0", ...message })}\n`); -} - -async function waitForExit(child, timeoutMs) { - if (child.exitCode !== null || child.signalCode !== null) return; - - await new Promise((resolve) => { - const timer = setTimeout(resolve, timeoutMs); - child.once("exit", () => { - clearTimeout(timer); - resolve(); - }); - }); -} - -async function terminateProcessGroup(child) { - if (!child.pid) return; - - try { - if (process.platform === "win32") child.kill("SIGTERM"); - else process.kill(-child.pid, "SIGTERM"); - } catch { - // The process may have exited between the response and cleanup. - } - - await waitForExit(child, 1_000); - - if (child.exitCode === null && child.signalCode === null) { - try { - if (process.platform === "win32") child.kill("SIGKILL"); - else process.kill(-child.pid, "SIGKILL"); - } catch { - // Best-effort cleanup. - } - } - - child.stdin?.destroy(); - child.stdout?.destroy(); - child.stderr?.destroy(); -} - -async function fetchRuntimeModels(provider, timeoutMs) { - const tempRoot = mkdtempSync(join(tmpdir(), `acp-models-${provider.key}-`)); - const workspace = join(tempRoot, "workspace"); - mkdirSync(workspace, { recursive: true }); - const command = provider.default_command; - const env = providerEnv(provider.key, tempRoot); - for (const key of ["CLAUDE_CONFIG_DIR", "CODEX_HOME", "GEMINI_CLI_HOME"]) { - if (env[key]) mkdirSync(env[key], { recursive: true }); - } - if (provider.key === "codex") { - writeFileSync( - join(env.CODEX_HOME, "auth.json"), - JSON.stringify({ - auth_mode: "apikey", - OPENAI_API_KEY: - process.env.OPENAI_API_KEY || "dummy-acp-model-drift-check", - }), - ); - } - let stderr = ""; - - const child = spawn(command[0], command.slice(1), { - cwd: workspace, - env, - stdio: ["pipe", "pipe", "pipe"], - detached: process.platform !== "win32", - }); - - const pending = new Map(); - let nextId = 1; - - child.stderr.on("data", (chunk) => { - stderr = `${stderr}${chunk.toString()}`.slice(-8_000); - }); - - child.on("exit", (code, signal) => { - for (const [id, entry] of pending) { - entry.reject( - new Error( - `${provider.key} exited before response ${id} (code=${code}, signal=${signal})\n${stderr}`, - ), - ); - } - pending.clear(); - }); - - readline.createInterface({ input: child.stdout }).on("line", (line) => { - let message; - try { - message = JSON.parse(line); - } catch { - stderr = `${stderr}\n[stdout non-json] ${line}`.slice(-8_000); - return; - } - - if (message.id !== undefined && pending.has(message.id)) { - const entry = pending.get(message.id); - pending.delete(message.id); - entry.resolve(message); - return; - } - - // Some agents may send client-side requests while setting up a session. - // The model list does not depend on these, so return an empty success to - // keep the handshake moving. - if (message.id !== undefined && message.method) { - writeJsonLine(child.stdin, { id: message.id, result: {} }); - } - }); - - function request(method, params) { - const id = nextId++; - writeJsonLine(child.stdin, { id, method, params }); - - return new Promise((resolve, reject) => { - const timer = setTimeout(() => { - pending.delete(id); - reject( - new Error( - `${provider.key} timed out waiting for ${method} after ${timeoutMs}ms\n${stderr}`, - ), - ); - }, timeoutMs); - - pending.set(id, { - resolve: (message) => { - clearTimeout(timer); - resolve(message); - }, - reject: (error) => { - clearTimeout(timer); - reject(error); - }, - }); - }); - } - - try { - const init = await request("initialize", { - protocolVersion: 1, - clientCapabilities: { - auth: { terminal: false }, - fs: { readTextFile: false, writeTextFile: false }, - terminal: false, - }, - clientInfo: { - name: "agent-canvas-acp-model-drift-check", - version: "0.0.0", - }, - }); - if (init.error) { - throw new Error( - `${provider.key} initialize failed: ${JSON.stringify(init.error)}`, - ); - } - - const session = await request("session/new", { - cwd: workspace, - mcpServers: [], - }); - if (session.error) { - throw new Error( - `${provider.key} session/new failed: ${JSON.stringify(session.error)}`, - ); - } - - const models = session.result?.models?.availableModels; - if (!Array.isArray(models)) { - throw new Error( - `${provider.key} did not return models.availableModels from session/new`, - ); - } - - if (session.result?.sessionId) { - await request("session/close", { - sessionId: session.result.sessionId, - }).catch(() => undefined); - } - - return models; - } finally { - await terminateProcessGroup(child); - rmSync(tempRoot, { recursive: true, force: true }); - } -} - -function compareModelIds(provider, runtimeModels) { - const ignored = runtimeModels.filter(isGenericDefaultModel); - const actual = runtimeModels - .filter((model) => !isGenericDefaultModel(model)) - .map((model) => ({ - id: model.modelId, - label: model.name, - })); - const expected = provider.available_models; - const actualIds = actual.map((model) => model.id); - const expectedIds = expected.map((model) => model.id); - const missing = actualIds.filter((id) => !expectedIds.includes(id)); - const extra = expectedIds.filter((id) => !actualIds.includes(id)); - const orderMatches = - actualIds.length === expectedIds.length && - actualIds.every((id, index) => id === expectedIds[index]); - - return { - ok: missing.length === 0 && extra.length === 0 && orderMatches, - actual, - actualIds, - expected, - expectedIds, - extra, - ignored, - missing, - orderMatches, - }; -} - -function formatList(values) { - return values.length > 0 - ? values.map((value) => ` - ${value}`).join("\n") - : " (none)"; -} - -async function main() { - const args = parseArgs(process.argv.slice(2)); - if (args.help) { - showHelp(); - return; - } - - const selected = new Set(args.providers); - const providers = parseCanvasProviders(args.tsFile).filter( - (provider) => selected.size === 0 || selected.has(provider.key), - ); - - if (providers.length === 0) { - throw new Error( - selected.size > 0 - ? `No matching providers found: ${[...selected].join(", ")}` - : "No providers with available_models found", - ); - } - - process.stderr.write( - `${colors.cyan}ACP Provider Models Sync Check${colors.reset}\n`, - ); - - const failures = []; - - for (const provider of providers) { - process.stderr.write( - `${colors.dim}- ${provider.key}: launching ${provider.default_command.join(" ")}${colors.reset}\n`, - ); - - const runtimeModels = await fetchRuntimeModels(provider, args.timeoutMs); - const result = compareModelIds(provider, runtimeModels); - - if (result.ok) { - const ignoredText = - result.ignored.length > 0 - ? `; ignored ${result.ignored.length} default placeholder(s)` - : ""; - process.stderr.write( - ` ${colors.green}ok${colors.reset} ${result.expectedIds.length} model(s)${ignoredText}\n`, - ); - continue; - } - - failures.push({ provider, result }); - process.stderr.write(` ${colors.red}drift detected${colors.reset}\n`); - if (result.missing.length > 0) { - process.stderr.write( - ` ${colors.yellow}Missing from Canvas:${colors.reset}\n${formatList(result.missing)}\n`, - ); - } - if (result.extra.length > 0) { - process.stderr.write( - ` ${colors.yellow}Extra in Canvas:${colors.reset}\n${formatList(result.extra)}\n`, - ); - } - if ( - !result.orderMatches && - result.missing.length === 0 && - result.extra.length === 0 - ) { - process.stderr.write( - ` ${colors.yellow}Order differs.${colors.reset}\n` + - ` Runtime:\n${formatList(result.actualIds)}\n` + - ` Canvas:\n${formatList(result.expectedIds)}\n`, - ); - } - } - - if (failures.length > 0) { - throw new Error( - `${failures.length} ACP provider model list(s) drifted from their subprocess-advertised availableModels`, - ); - } - - process.stderr.write( - `${colors.green}Canvas ACP model suggestions match the ACP subprocess-advertised model IDs.${colors.reset}\n`, - ); -} - -main().catch((error) => { - process.stderr.write(`${colors.red}ERROR:${colors.reset} ${error.message}\n`); - process.exitCode = 1; -}); diff --git a/src/api/agent-server-adapter.ts b/src/api/agent-server-adapter.ts index 888c35455..3c5d6d712 100644 --- a/src/api/agent-server-adapter.ts +++ b/src/api/agent-server-adapter.ts @@ -39,13 +39,9 @@ export interface DirectConversationInfo { } | null; agent?: { /** - * Pydantic discriminator from the SDK union. ``"ACPAgent"`` means the - * conversation runs an ACP CLI subprocess (model selection lives on - * the subprocess via ``acp_model``, not on ``agent.llm``); ``"Agent"`` - * means the conversation drives an LLM directly through litellm. - * Used by ``toAppConversation`` to mark ACP conversations so LLM-switch - * affordances stay gated even when ``llm_model`` carries the ACP display - * model. + * Pydantic discriminator from the SDK union: ``"ACPAgent"`` for ACP CLI + * subprocesses (model lives on the subprocess via ``acp_model``), + * ``"Agent"`` for direct litellm. Read by {@link toAppConversation}. */ kind?: string | null; acp_model?: string | null; @@ -251,9 +247,33 @@ function nonEmptyString(value: unknown): string | null { return trimmed.length > 0 ? trimmed : null; } +// SDK placeholder strings the ACP wrapper returns before the user has +// chosen a real model — surfacing either would lie about what's running. +const ACP_DEFAULT_PLACEHOLDERS = new Set(["default", "default (recommended)"]); function isAcpDefaultPlaceholder(value: string): boolean { - const normalized = value.trim().toLowerCase(); - return normalized === "default" || normalized === "default (recommended)"; + return ACP_DEFAULT_PLACEHOLDERS.has(value.trim().toLowerCase()); +} + +// Sentinel ``agent.llm.model`` returned by older SDKs for ACP conversations +// in lieu of a real model. Suppressed at the display boundary. +export const ACP_MANAGED_SENTINEL = "acp-managed"; + +/** + * Resolve the model string to surface on an ACP conversation, preferring + * (in order) SDK runtime fields → Canvas-configured ``acp_model`` → + * ``agent.llm.model`` (unless it's the {@link ACP_MANAGED_SENTINEL}). + * Placeholder strings like ``"Default (recommended)"`` are skipped so the + * chip never shows a generic label when a concrete one is reachable. + */ +function resolveAcpDisplayModel(info: DirectConversationInfo): string | null { + for (const candidate of [info.current_model_name, info.current_model_id]) { + const value = nonEmptyString(candidate); + if (value && !isAcpDefaultPlaceholder(value)) return value; + } + const configured = nonEmptyString(info.agent?.acp_model); + if (configured) return configured; + const llmModel = nonEmptyString(info.agent?.llm?.model); + return llmModel === ACP_MANAGED_SENTINEL ? null : llmModel; } export function toAppConversation( @@ -270,17 +290,6 @@ export function toAppConversation( // conversation — the chip is identity info for the ACP CLI subprocess, // and showing it on a non-ACP conversation would be a lie. const acpServer = isAcp ? (info.tags?.[ACP_SERVER_TAG_KEY] ?? null) : null; - const acpRuntimeName = nonEmptyString(info.current_model_name); - const acpRuntimeId = nonEmptyString(info.current_model_id); - const acpConfiguredModel = nonEmptyString(info.agent?.acp_model); - const acpLlmModel = nonEmptyString(info.agent?.llm?.model); - const acpDisplayModel = - acpRuntimeName && !isAcpDefaultPlaceholder(acpRuntimeName) - ? acpRuntimeName - : acpRuntimeId && !isAcpDefaultPlaceholder(acpRuntimeId) - ? acpRuntimeId - : (acpConfiguredModel ?? - (acpLlmModel !== "acp-managed" ? acpLlmModel : null)); return { id: info.id, created_by_user_id: null, @@ -296,7 +305,7 @@ export function toAppConversation( agent_kind: isAcp ? "acp" : "openhands", acp_server: acpServer, llm_model: isAcp - ? acpDisplayModel + ? resolveAcpDisplayModel(info) : (info.agent?.llm?.model ?? DEFAULT_SETTINGS.llm_model), metrics: info.metrics ? { From 610e3a4eaa7281ba6ffa742e943119877bc1a637 Mon Sep 17 00:00:00 2001 From: Debug Agent Date: Fri, 22 May 2026 21:39:29 +0200 Subject: [PATCH 5/6] fix(acp): plug review-flagged gaps in model resolution + chip path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five behavioral fixes + two refactors from review feedback on PR #730: - F1: ``normalizeAgent`` and ``requireDirectConversationInfo`` were dropping ``agent.acp_model``, ``current_model_id``, and ``current_model_name`` on the way in from the wire — so the chip resolver only worked in unit tests (which build ``DirectConversationInfo`` in-process) and silently fell back to provider-name labels in production. Normalizer now preserves all three. - F2: ChatInputActions gated rendering ChatInputModel on ``isCloud || conversation?.agent_kind === "acp"``, so on a local home screen with no active conversation it always picked SwitchProfileButton. SwitchProfileButton then hid itself for ACP — net result: the home-ACP model label never appeared. Added ``isHomeAcp`` derivation from ``settings.agent_settings.agent_kind`` so the chat input picks ChatInputModel in that case too. - F3: Switching the preset dropdown to Custom set ``isCustomAcpModel`` but left ``acpModel`` untouched, so a user moving Claude Code → Custom + typing a custom command could save ``acp_model: "claude-opus-4-7"`` on an unrelated wrapper. Now clears ``acpModel`` on Custom selection. - F4: ``buildConfiguredAcpAgentSettings`` was stripping null / empty ``acp_model`` and not falling back to ``provider.default_model``, so existing users with ``acp_model: null`` saw the new registered default in Settings → Agent but their next conversation still started with the agent-server's own default (UI/runtime mismatch). Conversation creation now substitutes the provider default for empty values, matching what the form displays. - F5: ``isAcpDefaultPlaceholder`` was applied only to runtime fields, not to the ``configured`` (``agent.acp_model``) or ``sdkLlm`` (``agent.llm.model``) fallback rungs of the precedence chain. Older settings that persisted the literal ``"Default (recommended)"`` could surface it on chips. Filter now applies uniformly. - R1: All five surfaces (Settings form, conversation creation, ChatInputModel, conversation adapter, chip) now route through one helper, ``resolveEffectiveAcpModel({ runtimeName, runtimeId, configured, sdkLlm, providerDefault })`` in ``acp-providers.ts``. Placeholder + sentinel filtering live in one place. ``providerDefault`` is opt-in — chip omits it (don't lie about what's running), settings / creation / chat-input pass it (silently substitute the registry default). - R2: ``ACPProviderIcon`` no longer includes ``"openhands"`` — it's ACP-only again. Reintroduced ``AgentBrandIconKind = "openhands" | ACPProviderIcon`` for surfaces that can render either harness's mark. Tests: ``acp-server-conversation-service.test.ts`` now exercises the wire path with the new fields. ``agent-server-adapter.test.ts`` adds two cases covering placeholder filtering on configured + sdkLlm rungs. ``agent-settings.test.tsx`` adds a Custom-preset-clears-default regression. ``chat-input-model.test.tsx`` swaps the "home + ACP renders nothing" assertion for "home + ACP shows the provider default" — that's the new correct behavior. Co-Authored-By: Claude Opus 4.7 (1M context) --- __tests__/api/agent-server-adapter.test.ts | 62 +++++++++++-- .../agent-server-conversation-service.test.ts | 63 +++++++++++++ .../chat/components/chat-input-model.test.tsx | 26 +++--- __tests__/routes/agent-settings.test.tsx | 55 ++++++++++++ src/api/agent-server-adapter.ts | 89 ++++++++----------- .../agent-server-conversation-service.api.ts | 20 ++++- .../chat/components/chat-input-actions.tsx | 19 ++-- .../chat/components/chat-input-model.tsx | 40 ++++++--- .../conversation-card-footer.tsx | 13 ++- .../onboarding/steps/choose-agent-step.tsx | 4 +- src/components/shared/agent-brand-icon.tsx | 10 ++- src/constants/acp-providers.ts | 59 +++++++++++- src/routes/agent-settings.tsx | 5 ++ 13 files changed, 366 insertions(+), 99 deletions(-) diff --git a/__tests__/api/agent-server-adapter.test.ts b/__tests__/api/agent-server-adapter.test.ts index d178a1124..144b3c883 100644 --- a/__tests__/api/agent-server-adapter.test.ts +++ b/__tests__/api/agent-server-adapter.test.ts @@ -609,6 +609,33 @@ describe("toAppConversation", () => { expect(result.llm_model).toBe("claude-sonnet-4-6"); }); + it("filters ACP default placeholders surfaced via the configured acp_model", () => { + // Older settings may have persisted the SDK's literal "default" string + // into ``acp_model``. Surfacing it on the chip would lie about what's + // running — the placeholder filter is applied to every candidate, not + // just the runtime fields. + const result = toAppConversation({ + ...baseInfo, + agent: { + kind: "ACPAgent", + acp_model: "Default (recommended)", + llm: { model: "acp-managed" }, + }, + }); + expect(result.agent_kind).toBe("acp"); + expect(result.llm_model).toBeNull(); + }); + + it("filters ACP default placeholders surfaced via agent.llm.model", () => { + // Same defense, one rung lower in the precedence chain. + const result = toAppConversation({ + ...baseInfo, + agent: { kind: "ACPAgent", llm: { model: "default" } }, + }); + expect(result.agent_kind).toBe("acp"); + expect(result.llm_model).toBeNull(); + }); + it("surfaces acp_server from tags.acpserver for ACP conversations", () => { // The ``acpserver`` conversation tag is stamped at create time // (``buildStartConversationRequest``) but never previously plumbed @@ -1044,12 +1071,14 @@ describe("buildStartConversationRequest — ACP discriminator", () => { expect(payload.agent.acp_command).toEqual([]); }); - it("omits acp_model when settings contains an empty string", () => { + it("seeds the provider default when settings contains an empty acp_model", () => { // The form may carry an empty string after a user clears the model - // input; the agent-server expects ``null`` for "use provider default." - // Empty strings would pass the spawn but bias model selection on - // some providers (e.g. claude-agent-acp's _meta would set - // ``model: ''``). + // input. Older behavior left ``acp_model`` absent and relied on the + // agent-server's own default; the registry-default path + // (resolveEffectiveAcpModel) is now authoritative on Canvas's side, + // so an empty string resolves to the provider's ``default_model`` + // before the request leaves the client. Keeps the displayed Settings + // → Agent default in sync with what the runtime actually starts. const payload = buildStartConversationRequest({ settings: { ...DEFAULT_SETTINGS, @@ -1065,6 +1094,29 @@ describe("buildStartConversationRequest — ACP discriminator", () => { agent: Record & { acp_model?: unknown }; }; + expect(payload.agent.acp_model).toBe("claude-opus-4-7"); + }); + + it("omits acp_model for the custom preset when none is configured", () => { + // The Custom preset has no registered ``default_model``, so an empty + // ``acp_model`` falls through to ``undefined`` — the agent-server then + // applies its own default. Distinct from the built-in providers + // which substitute their registry default. + const payload = buildStartConversationRequest({ + settings: { + ...DEFAULT_SETTINGS, + agent_settings: { + schema_version: 1, + agent_kind: "acp", + acp_server: "custom", + acp_command: ["my-custom-acp"], + acp_model: "", + }, + }, + }) as { + agent: Record & { acp_model?: unknown }; + }; + expect(payload.agent.acp_model).toBeUndefined(); }); diff --git a/__tests__/api/agent-server-conversation-service.test.ts b/__tests__/api/agent-server-conversation-service.test.ts index 14b460a3a..74fbef56c 100644 --- a/__tests__/api/agent-server-conversation-service.test.ts +++ b/__tests__/api/agent-server-conversation-service.test.ts @@ -502,6 +502,69 @@ describe("AgentServerConversationService", () => { ); }); + it("preserves the new ACP model fields through the wire normalizer", async () => { + // Direct adapter tests pass DirectConversationInfo objects in-process + // and so can't catch the case where the wire-format normalizer + // (``normalizeAgent`` + ``requireDirectConversationInfo``) drops the + // newly-added ACP fields. Exercises the full HTTP -> AppConversation + // path so the chip's model resolution actually has the inputs it + // needs on a real local-backend fetch. + mockHttpGet.mockResolvedValue({ + data: [ + { + id: "conv-acp-model-wire", + created_at: "2024-01-01", + updated_at: "2024-01-01", + agent: { + kind: "ACPAgent", + acp_model: "claude-opus-4-7", + llm: { model: "acp-managed" }, + }, + current_model_id: "claude-opus-4-7", + current_model_name: "Claude Opus 4.7", + tags: { acpserver: "claude-code" }, + }, + ], + }); + + const [conversation] = + await AgentServerConversationService.batchGetAppConversations([ + "conv-acp-model-wire", + ]); + + // ``current_model_name`` wins the precedence chain in the adapter. + expect(conversation?.agent_kind).toBe("acp"); + expect(conversation?.llm_model).toBe("Claude Opus 4.7"); + }); + + it("falls back to acp_model when SDK runtime fields are absent on the wire", async () => { + // Older agent-servers don't populate ``current_model_*``. The + // adapter must still surface a model on the chip — falling through + // to ``agent.acp_model`` (the Canvas-configured value). + mockHttpGet.mockResolvedValue({ + data: [ + { + id: "conv-acp-fallback", + created_at: "2024-01-01", + updated_at: "2024-01-01", + agent: { + kind: "ACPAgent", + acp_model: "claude-sonnet-4-6", + llm: { model: "acp-managed" }, + }, + tags: { acpserver: "claude-code" }, + }, + ], + }); + + const [conversation] = + await AgentServerConversationService.batchGetAppConversations([ + "conv-acp-fallback", + ]); + + expect(conversation?.llm_model).toBe("claude-sonnet-4-6"); + }); + it("extracts the acpserver tag from the wire payload for the sidebar chip", async () => { // The agent-server stamps ``tags.acpserver`` at conversation create // time (see ``buildStartConversationRequest``); the read path diff --git a/__tests__/components/features/chat/components/chat-input-model.test.tsx b/__tests__/components/features/chat/components/chat-input-model.test.tsx index c5cad9a2b..3feab4770 100644 --- a/__tests__/components/features/chat/components/chat-input-model.test.tsx +++ b/__tests__/components/features/chat/components/chat-input-model.test.tsx @@ -172,27 +172,29 @@ describe("ChatInputModel", () => { ).not.toBeInTheDocument(); }); - it("renders nothing on the home page when ACP is the default agent", () => { - // Home-screen gating: no active conversation, so the - // per-conversation ``agent_kind`` check can't catch this case. - // Fall back to ``settings.agent_settings.agent_kind`` — that's - // the kind the next-created conversation will inherit. Showing - // the LLM picker here would put up a control that becomes a - // silent no-op the moment the user sends their first message. + it("shows the provider default on the home page when ACP is the default agent and no model is saved", () => { + // Home-screen gating: no active conversation and no saved ``acp_model``. + // The next-created conversation will inherit the provider's + // ``default_model`` (see buildConfiguredAcpAgentSettings), so the picker + // shows that same default — matching what the runtime will actually + // start. Picker links to /settings/agent (not /settings) since + // ``settings.llm_model`` doesn't apply to ACP. useActiveConversationMock.mockReturnValue({ data: undefined }); useSettingsMock.mockReturnValue({ data: { agent_settings: { agent_kind: "acp", acp_server: "claude-code" }, - // settings.llm_model is still set (the user has an OpenHands - // default configured), but agent_kind=acp wins. + // settings.llm_model is set (user has an OpenHands default + // configured), but agent_kind=acp suppresses it. llm_model: "anthropic/claude-sonnet-4-20250514", }, }); renderWithProviders(); - expect( - screen.queryByTestId("chat-input-llm-model"), - ).not.toBeInTheDocument(); + const model = screen.getByTestId("chat-input-llm-model"); + // Claude Code's registered default; see CLAUDE_MODELS in acp-providers.ts. + expect(model).toHaveAttribute("title", "claude-opus-4-7"); + fireEvent.click(model); + expect(screen.getByRole("link")).toHaveAttribute("href", "/settings/agent"); }); }); diff --git a/__tests__/routes/agent-settings.test.tsx b/__tests__/routes/agent-settings.test.tsx index bd27f4b6e..ca1656fb1 100644 --- a/__tests__/routes/agent-settings.test.tsx +++ b/__tests__/routes/agent-settings.test.tsx @@ -232,6 +232,61 @@ describe("AgentSettingsScreen", () => { expect(call.agent_settings_diff?.acp_model).toBe("claude-haiku-4-5"); }); + it("clears the model when switching from a built-in provider to Custom", async () => { + // F3 from review: built-ins seed ``acp_model`` to their registered + // ``default_model`` on load. Picking Custom must not leak that built-in + // default into custom settings — otherwise a user choosing Custom from + // Claude Code would silently save ``acp_model: "claude-opus-4-7"`` on an + // unrelated wrapper. + const user = userEvent.setup(); + vi.spyOn(SettingsService, "getSettings").mockResolvedValue( + buildSettings({ + agent_settings: { + schema_version: 1, + agent_kind: "acp", + acp_server: "claude-code", + acp_command: [], + acp_model: null, + }, + }), + ); + const save = vi.spyOn(SettingsService, "saveSettings"); + + renderAgentSettingsScreen(); + await screen.findByTestId("agent-command-input"); + // Form loads with the Claude Code default visible. + expect(screen.getByLabelText("SETTINGS$AGENT_MODEL")).toHaveValue( + "Claude Opus 4.7", + ); + + // Switch to the Custom preset, then enter a different command — the + // form's ``selectedPreset`` re-derives from the command text, so the + // save path only treats it as Custom once the command no longer matches + // a built-in provider's default. + await user.click(screen.getByTestId("agent-preset-selector")); + await user.click( + await screen.findByRole("option", { name: "SETTINGS$AGENT_PRESET_CUSTOM" }), + ); + const commandInput = screen.getByTestId( + "agent-command-input", + ) as HTMLTextAreaElement; + await user.clear(commandInput); + await user.type(commandInput, "my-custom-acp --flag"); + + await user.click(screen.getByTestId("agent-save-button")); + await waitFor(() => { + expect(save).toHaveBeenCalledTimes(1); + }); + + const call = save.mock.calls[0]?.[0] as { + agent_settings_diff?: Record; + }; + // Custom preset has no registered default — saved acp_model must be null, + // not the inherited Claude Opus default. + expect(call.agent_settings_diff?.acp_server).toBe("custom"); + expect(call.agent_settings_diff?.acp_model).toBeNull(); + }); + it("saves an ACP diff when switching to ACP + Claude Code", async () => { const user = userEvent.setup(); vi.spyOn(SettingsService, "getSettings").mockResolvedValue( diff --git a/src/api/agent-server-adapter.ts b/src/api/agent-server-adapter.ts index 3c5d6d712..f3d769cec 100644 --- a/src/api/agent-server-adapter.ts +++ b/src/api/agent-server-adapter.ts @@ -1,7 +1,10 @@ import { DEFAULT_SETTINGS } from "#/services/settings"; import { ExecutionStatus } from "#/types/agent-server/core"; import { Settings, SettingsValue } from "#/types/settings"; -import { ACP_PROVIDERS } from "#/constants/acp-providers"; +import { + ACP_PROVIDERS, + resolveEffectiveAcpModel, +} from "#/constants/acp-providers"; import { getAgentServerClientOptions } from "./agent-server-client-options"; import { isAgentServerToolAvailable } from "./agent-server-compatibility"; import { getAgentServerWorkingDir } from "./agent-server-config"; @@ -241,41 +244,6 @@ export function getDefaultConversationTitle(conversationId: string): string { return `Conversation ${conversationId.slice(0, 5)}`; } -function nonEmptyString(value: unknown): string | null { - if (typeof value !== "string") return null; - const trimmed = value.trim(); - return trimmed.length > 0 ? trimmed : null; -} - -// SDK placeholder strings the ACP wrapper returns before the user has -// chosen a real model — surfacing either would lie about what's running. -const ACP_DEFAULT_PLACEHOLDERS = new Set(["default", "default (recommended)"]); -function isAcpDefaultPlaceholder(value: string): boolean { - return ACP_DEFAULT_PLACEHOLDERS.has(value.trim().toLowerCase()); -} - -// Sentinel ``agent.llm.model`` returned by older SDKs for ACP conversations -// in lieu of a real model. Suppressed at the display boundary. -export const ACP_MANAGED_SENTINEL = "acp-managed"; - -/** - * Resolve the model string to surface on an ACP conversation, preferring - * (in order) SDK runtime fields → Canvas-configured ``acp_model`` → - * ``agent.llm.model`` (unless it's the {@link ACP_MANAGED_SENTINEL}). - * Placeholder strings like ``"Default (recommended)"`` are skipped so the - * chip never shows a generic label when a concrete one is reachable. - */ -function resolveAcpDisplayModel(info: DirectConversationInfo): string | null { - for (const candidate of [info.current_model_name, info.current_model_id]) { - const value = nonEmptyString(candidate); - if (value && !isAcpDefaultPlaceholder(value)) return value; - } - const configured = nonEmptyString(info.agent?.acp_model); - if (configured) return configured; - const llmModel = nonEmptyString(info.agent?.llm?.model); - return llmModel === ACP_MANAGED_SENTINEL ? null : llmModel; -} - export function toAppConversation( info: DirectConversationInfo, ): AppConversation { @@ -304,8 +272,17 @@ export function toAppConversation( pr_number: [], agent_kind: isAcp ? "acp" : "openhands", acp_server: acpServer, + // Chip path: no ``providerDefault`` — the chip must distinguish + // "no concrete model" (fall back to the provider display name in + // ConversationCardFooter) from "default" (would lie about what's + // running on the subprocess). llm_model: isAcp - ? resolveAcpDisplayModel(info) + ? resolveEffectiveAcpModel({ + runtimeName: info.current_model_name, + runtimeId: info.current_model_id, + configured: info.agent?.acp_model, + sdkLlm: info.agent?.llm?.model, + }) : (info.agent?.llm?.model ?? DEFAULT_SETTINGS.llm_model), metrics: info.metrics ? { @@ -610,14 +587,8 @@ function buildConfiguredAcpAgentSettings(settings: Settings): SettingsRecord { // ``buildStartConversationRequest``). const payload: SettingsRecord = {}; for (const key of ACP_SETTINGS_KEYS) { + if (key === "acp_model") continue; if (agentSettings[key] !== undefined && agentSettings[key] !== null) { - if (key === "acp_model") { - const model = nonEmptyString(agentSettings[key]); - if (model) { - payload[key] = model; - } - continue; - } payload[key] = agentSettings[key]; } } @@ -632,18 +603,30 @@ function buildConfiguredAcpAgentSettings(settings: Settings): SettingsRecord { // command for every built-in preset, while leaving ``acp_server: custom`` // (and any unknown key) untouched — those genuinely require the user's // ``acp_command`` entry. + const serverKey = + typeof agentSettings.acp_server === "string" + ? agentSettings.acp_server + : undefined; + const provider = ACP_PROVIDERS.find(({ key }) => key === serverKey); const cmd = payload.acp_command; const isEmpty = Array.isArray(cmd) && cmd.length === 0; const noCommand = cmd === undefined; - if (isEmpty || noCommand) { - const serverKey = - typeof agentSettings.acp_server === "string" - ? agentSettings.acp_server - : undefined; - const provider = ACP_PROVIDERS.find(({ key }) => key === serverKey); - if (provider) { - payload.acp_command = [...provider.default_command]; - } + if ((isEmpty || noCommand) && provider) { + payload.acp_command = [...provider.default_command]; + } + + // Saved settings may carry ``acp_model: null`` (existing users predating + // the default-model registry, or savedfields that the agent-server stripped). + // Fall back to the provider's ``default_model`` so the conversation starts + // with whatever the Settings → Agent UI shows — without that, the form's + // displayed default would silently not take effect at runtime until the + // user re-saved the page. + const effectiveModel = resolveEffectiveAcpModel({ + configured: agentSettings.acp_model as string | null | undefined, + providerDefault: provider?.default_model, + }); + if (effectiveModel) { + payload.acp_model = effectiveModel; } return payload; diff --git a/src/api/conversation-service/agent-server-conversation-service.api.ts b/src/api/conversation-service/agent-server-conversation-service.api.ts index fe463c99f..a0f707261 100644 --- a/src/api/conversation-service/agent-server-conversation-service.api.ts +++ b/src/api/conversation-service/agent-server-conversation-service.api.ts @@ -129,11 +129,17 @@ function normalizeAgent(value: unknown): DirectConversationInfo["agent"] { ? { model: stringOrNull(value.llm.model) } : null; // ``kind`` is the SDK's pydantic discriminator (``"Agent"`` vs ``"ACPAgent"``); - // ``toAppConversation`` reads it to derive ``agent_kind`` and to gate the - // ACP-server chip + ``llm_model`` null-out. Preserving it here makes the - // wire path agree with the unit-test path that builds ``DirectConversationInfo`` + // ``toAppConversation`` reads it to derive ``agent_kind``. ``acp_model`` is + // the Canvas-configured model on the ACPAgent — preserved so the conversation + // adapter and the conversation chip can fall back to it when the SDK runtime + // model fields aren't populated. Preserving these here makes the wire path + // agree with the unit-test path that builds ``DirectConversationInfo`` // directly (e.g. ``__tests__/api/agent-server-adapter.test.ts``). - return { kind: stringOrNull(value.kind), llm }; + return { + kind: stringOrNull(value.kind), + acp_model: stringOrNull(value.acp_model), + llm, + }; } function normalizeWorkspace( @@ -214,6 +220,12 @@ function requireDirectConversationInfo(item: unknown): DirectConversationInfo { agent: normalizeAgent(item.agent), workspace: normalizeWorkspace(item.workspace), tags: normalizeTags(item.tags), + // SDK-runtime ACP model fields (populated when the agent-server supports + // ``ConversationInfo.current_model_*``). Consumed by the conversation + // adapter to drive the per-card chip's model text. Older agent-servers + // omit these — adapter handles ``undefined`` / ``null`` gracefully. + current_model_id: stringOrNull(item.current_model_id), + current_model_name: stringOrNull(item.current_model_name), }; } diff --git a/src/components/features/chat/components/chat-input-actions.tsx b/src/components/features/chat/components/chat-input-actions.tsx index 491835e86..86acda0f2 100644 --- a/src/components/features/chat/components/chat-input-actions.tsx +++ b/src/components/features/chat/components/chat-input-actions.tsx @@ -21,6 +21,7 @@ import { usePauseConversation } from "#/hooks/mutation/use-pause-conversation"; import { useResumeConversation } from "#/hooks/mutation/use-resume-conversation"; import { useActiveBackend } from "#/contexts/active-backend-context"; import { useActiveConversation } from "#/hooks/query/use-active-conversation"; +import { useSettings } from "#/hooks/query/use-settings"; import { useConversationNameContextMenu } from "#/hooks/use-conversation-name-context-menu"; import { useConversationStore } from "#/stores/conversation-store"; import { useAgentState } from "#/hooks/use-agent-state"; @@ -65,18 +66,26 @@ export function ChatInputActions({ const { conversationId } = useOptionalConversationId(); const { data: conversation } = useActiveConversation(); const { backend } = useActiveBackend(); + const { data: settings } = useSettings(); const isCloud = backend.kind === "cloud"; const isAcpConversation = conversation?.agent_kind === "acp"; + // Home screen: no active conversation yet, but Settings → Agent may already + // be configured for ACP. The chat input on the home page should reflect + // that — the next-created conversation will inherit it — so route the model + // label to ChatInputModel (which knows how to show the ACP model) instead + // of SwitchProfileButton (which would hide itself for ACP and leave the + // home screen with no model affordance at all). + const isHomeAcp = + !conversation && settings?.agent_settings?.agent_kind === "acp"; + const isAcpContext = isAcpConversation || isHomeAcp; const llmDestinationLabel = t( - isAcpConversation + isAcpContext ? I18nKey.SETTINGS$NAV_AGENT : isCloud ? I18nKey.SETTINGS$LLM_SETTINGS : I18nKey.SETTINGS$LLM_PROFILES, ); - const modelDestinationPath = isAcpConversation - ? "/settings/agent" - : "/settings"; + const modelDestinationPath = isAcpContext ? "/settings/agent" : "/settings"; const webSocketStatus = useUnifiedWebSocketStatus(); const { curAgentState } = useAgentState(); const { conversationMode, setConversationMode } = useConversationStore(); @@ -499,7 +508,7 @@ export function ChatInputActions({
    )}
    - {isCloud || isAcpConversation ? ( + {isCloud || isAcpContext ? ( ) : ( diff --git a/src/components/features/chat/components/chat-input-model.tsx b/src/components/features/chat/components/chat-input-model.tsx index 07fcaa551..863205143 100644 --- a/src/components/features/chat/components/chat-input-model.tsx +++ b/src/components/features/chat/components/chat-input-model.tsx @@ -8,6 +8,10 @@ import { ContextMenu } from "#/ui/context-menu"; import { Divider } from "#/ui/divider"; import { I18nKey } from "#/i18n/declaration"; import { useActiveBackend } from "#/contexts/active-backend-context"; +import { + ACP_PROVIDERS, + resolveEffectiveAcpModel, +} from "#/constants/acp-providers"; import { cn } from "#/utils/utils"; import React from "react"; import { useTranslation } from "react-i18next"; @@ -28,21 +32,33 @@ export function ChatInputModel() { // Home page has no active conversation; fall back to the user's default // model so the switcher renders consistently across both surfaces. const { data: settings } = useSettings(); - // ACP conversations do not use the OpenHands LLM profile. Show the ACP - // model only when the adapter/settings provide one, and link users to Agent - // settings instead of the LLM profile page. + // ACP conversations do not use the OpenHands LLM profile. Resolve the model + // label through the shared helper so the displayed value matches what the + // conversation-creation path will actually send to the agent-server (the + // helper applies provider defaults + filters out the SDK ``"default"`` + // placeholders + the ``"acp-managed"`` sentinel). const isActiveAcpConversation = conversation?.agent_kind === "acp"; const isHomeAcp = !conversation && settings?.agent_settings?.agent_kind === "acp"; - const settingsAcpModel = - typeof settings?.agent_settings?.acp_model === "string" - ? settings.agent_settings.acp_model.trim() || null - : null; - const llmModel = isActiveAcpConversation - ? conversation?.llm_model - : isHomeAcp - ? settingsAcpModel - : (conversation?.llm_model ?? settings?.llm_model); + const acpProvider = isHomeAcp + ? ACP_PROVIDERS.find( + ({ key }) => key === settings?.agent_settings?.acp_server, + ) + : undefined; + let llmModel: string | null | undefined; + if (isActiveAcpConversation) { + llmModel = conversation?.llm_model; + } else if (isHomeAcp) { + llmModel = resolveEffectiveAcpModel({ + configured: + typeof settings?.agent_settings?.acp_model === "string" + ? settings.agent_settings.acp_model + : null, + providerDefault: acpProvider?.default_model, + }); + } else { + llmModel = conversation?.llm_model ?? settings?.llm_model; + } const destinationPath = isActiveAcpConversation || isHomeAcp ? "/settings/agent" : "/settings"; const llmDestinationLabel = t( diff --git a/src/components/features/conversation-panel/conversation-card/conversation-card-footer.tsx b/src/components/features/conversation-panel/conversation-card/conversation-card-footer.tsx index 2c75a03db..4d101a971 100644 --- a/src/components/features/conversation-panel/conversation-card/conversation-card-footer.tsx +++ b/src/components/features/conversation-panel/conversation-card/conversation-card-footer.tsx @@ -8,9 +8,11 @@ import { isExecutionPaused } from "#/utils/status"; import { getAcpProviderDisplayName, resolveAcpProviderIcon, - type ACPProviderIcon, } from "#/constants/acp-providers"; -import { AgentBrandIcon } from "#/components/shared/agent-brand-icon"; +import { + AgentBrandIcon, + type AgentBrandIconKind, +} from "#/components/shared/agent-brand-icon"; import { ConversationRepoLink } from "./conversation-repo-link"; import { NoRepository } from "./no-repository"; @@ -62,8 +64,11 @@ export function ConversationCardFooter({ // resolved through PR 730's adapter chain, falling back to the provider // display name when no model is available so the chip never collapses to // icon-only. - let chip: { kind: ACPProviderIcon; text: string; tooltip: string } | null = - null; + let chip: { + kind: AgentBrandIconKind; + text: string; + tooltip: string; + } | null = null; if (agentKind === "acp") { const providerName = getAcpProviderDisplayName(acpServer) ?? diff --git a/src/components/features/onboarding/steps/choose-agent-step.tsx b/src/components/features/onboarding/steps/choose-agent-step.tsx index 313dcb585..4f9773d51 100644 --- a/src/components/features/onboarding/steps/choose-agent-step.tsx +++ b/src/components/features/onboarding/steps/choose-agent-step.tsx @@ -12,8 +12,8 @@ import { ACP_PROVIDER_FALLBACK_ICON, ACP_PROVIDERS, buildAcpAgentSettingsDiff, - type ACPProviderIcon, } from "#/constants/acp-providers"; +import { type AgentBrandIconKind } from "#/components/shared/agent-brand-icon"; import { displayErrorToast, displaySuccessToast, @@ -35,7 +35,7 @@ const CODEX_MARK_PATH = const GEMINI_MARK_PATH = "M12 0C12.904 6.056 17.944 11.096 24 12C17.944 12.904 12.904 17.944 12 24C11.096 17.944 6.056 12.904 0 12C6.056 11.096 11.096 6.056 12 0Z"; -function getAgentOptionIcon(id: string): ACPProviderIcon { +function getAgentOptionIcon(id: string): AgentBrandIconKind { if (id === "openhands") return "openhands"; return ( diff --git a/src/components/shared/agent-brand-icon.tsx b/src/components/shared/agent-brand-icon.tsx index c853e627b..204dde003 100644 --- a/src/components/shared/agent-brand-icon.tsx +++ b/src/components/shared/agent-brand-icon.tsx @@ -11,8 +11,16 @@ import { import type { ACPProviderIcon } from "#/constants/acp-providers"; import { cn } from "#/utils/utils"; +/** + * Icons the conversation chip + onboarding tiles can render. Strictly broader + * than {@link ACPProviderIcon} — that type covers ACP CLI subprocesses only + * (Claude Code, Codex, Gemini, generic terminal fallback), whereas this type + * additionally includes the native OpenHands harness. + */ +export type AgentBrandIconKind = "openhands" | ACPProviderIcon; + interface AgentBrandIconProps { - kind: ACPProviderIcon; + kind: AgentBrandIconKind; size?: number; className?: string; "data-testid"?: string; diff --git a/src/constants/acp-providers.ts b/src/constants/acp-providers.ts index 28318bf80..e388f4a58 100644 --- a/src/constants/acp-providers.ts +++ b/src/constants/acp-providers.ts @@ -1,7 +1,6 @@ import { I18nKey } from "#/i18n/declaration"; export type ACPProviderIcon = - | "openhands" | "claude-code" | "codex" | "gemini" @@ -9,6 +8,64 @@ export type ACPProviderIcon = export const ACP_PROVIDER_FALLBACK_ICON: ACPProviderIcon = "cli-generic"; +// SDK placeholder strings the ACP wrapper returns before the user has +// chosen a real model — surfacing either would lie about what's running. +export const ACP_DEFAULT_PLACEHOLDERS = new Set([ + "default", + "default (recommended)", +]); + +// Sentinel ``agent.llm.model`` returned by older SDKs for ACP conversations +// in lieu of a real model. Suppressed at every consumer that resolves a +// display string. +export const ACP_MANAGED_SENTINEL = "acp-managed"; + +/** + * Filter for "real" ACP model strings — non-empty, not the SDK's "default" + * placeholder, not the legacy ``acp-managed`` sentinel. Returns the trimmed + * value on success, ``null`` otherwise. + */ +function realAcpModel(value: unknown): string | null { + if (typeof value !== "string") return null; + const trimmed = value.trim(); + if (!trimmed) return null; + if (ACP_DEFAULT_PLACEHOLDERS.has(trimmed.toLowerCase())) return null; + if (trimmed === ACP_MANAGED_SENTINEL) return null; + return trimmed; +} + +/** + * Single source of truth for resolving the model string to surface for an + * ACP conversation/settings context. Consumed by the conversation adapter + * (chip text), the conversation-creation path (concrete ``acp_model`` + * payload), the Settings → Agent form (initial value), and the chat-input + * model label. + * + * Precedence: SDK runtime fields → user-configured ``acp_model`` → + * legacy ``agent.llm.model`` → provider default (when ``providerDefault`` + * is passed). Pass ``providerDefault`` only on surfaces that should + * silently substitute the registry default; omit it for the conversation + * chip, which must distinguish "no concrete model" from "default". + */ +export function resolveEffectiveAcpModel(inputs: { + runtimeName?: string | null; + runtimeId?: string | null; + configured?: string | null; + sdkLlm?: string | null; + providerDefault?: string | null; +}): string | null { + for (const candidate of [ + inputs.runtimeName, + inputs.runtimeId, + inputs.configured, + inputs.sdkLlm, + ]) { + const value = realAcpModel(candidate); + if (value) return value; + } + return inputs.providerDefault ?? null; +} + /** * Built-in ACP (Agent Client Protocol) provider registry. * diff --git a/src/routes/agent-settings.tsx b/src/routes/agent-settings.tsx index 3102ba1f6..75cd757b3 100644 --- a/src/routes/agent-settings.tsx +++ b/src/routes/agent-settings.tsx @@ -360,6 +360,11 @@ function AgentSettingsScreen() { setAcpModel(provider.default_model ?? ""); setIsCustomAcpModel(false); } else if (preset === ACP_CUSTOM_PRESET_KEY) { + // Switching to Custom must clear any built-in default the + // user just left — otherwise saving the custom command + // silently leaks e.g. ``claude-opus-4-7`` into + // ``acp_model`` for an unrelated wrapper. + setAcpModel(""); setIsCustomAcpModel(true); } setIsDirty(true); From c11c8c3ebc4781a4699671cc6ebd93235137ca27 Mon Sep 17 00:00:00 2001 From: Debug Agent Date: Sat, 23 May 2026 11:46:19 +0200 Subject: [PATCH 6/6] fix(acp): translate new agent-settings keys + harmonize Gemini labels MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the latest review pass: - Translates ``SETTINGS$AGENT_CUSTOM_MODEL`` and ``SETTINGS$AGENT_MODEL_HINT`` into all 14 non-English locales (ja, zh-CN, zh-TW, ko-KR, no, it, pt, es, ar, fr, tr, de, uk, ca). Both keys previously fell back to English text on every non-English client, blocking the model dropdown's hint copy and Custom-model label from being legible. - Harmonizes the Gemini model labels with the Claude / Codex pattern — raw IDs like ``gemini-3.1-pro-preview`` become ``Gemini 3.1 Pro (preview)`` so the three providers read consistently in the dropdown. - Adds provenance notes to ``CODEX_MODELS`` and ``GEMINI_MODELS`` mirroring the Claude one — naming the upstream source the list was extracted from and pointing at agent-canvas#740 (the long-term "move ACP model lists to ``@openhands/typescript-client``" plan). Co-Authored-By: Claude Opus 4.7 (1M context) --- src/constants/acp-providers.ts | 24 +++++++++++---- src/i18n/translation.json | 56 +++++++++++++++++----------------- 2 files changed, 46 insertions(+), 34 deletions(-) diff --git a/src/constants/acp-providers.ts b/src/constants/acp-providers.ts index e388f4a58..f090069c6 100644 --- a/src/constants/acp-providers.ts +++ b/src/constants/acp-providers.ts @@ -151,6 +151,12 @@ const CLAUDE_MODELS: ACPModelOption[] = [ { id: "opusplan", label: "Opus (plan) + Sonnet (execute)" }, ]; +// Model IDs accepted by the ``@zed-industries/codex-acp`` wrapper, mirroring +// the Codex CLI's own ``/model`` picker. Format is ``/`` +// where the trailing tier (``low``/``medium``/``high``/``xhigh``) hints the +// reasoning effort for that turn. Sourced from the Codex CLI's documented +// runtime options as of 2026-05-22 — see ``acp_model`` registry tracker +// in agent-canvas#740 for the long-term plan. const CODEX_MODELS: ACPModelOption[] = [ { id: "gpt-5.5/low", label: "GPT-5.5 (low)" }, { id: "gpt-5.5/medium", label: "GPT-5.5 (medium)" }, @@ -174,18 +180,24 @@ const CODEX_MODELS: ACPModelOption[] = [ { id: "gpt-5.2/xhigh", label: "GPT-5.2 (xhigh)" }, ]; +// Model IDs accepted by ``@google/gemini-cli --acp``. The ``auto-gemini-*`` +// entries delegate version selection to the CLI's router; the explicit +// ``gemini-3.1-*`` / ``gemini-2.5-*`` entries pin to a specific snapshot. +// Sourced from the Gemini CLI's documented model list as of 2026-05-22 — +// see agent-canvas#740 for the long-term plan to move this registry +// upstream. const GEMINI_MODELS: ACPModelOption[] = [ { id: "auto-gemini-3", label: "Auto (Gemini 3)" }, { id: "auto-gemini-2.5", label: "Auto (Gemini 2.5)" }, - { id: "gemini-3.1-pro-preview", label: "gemini-3.1-pro-preview" }, - { id: "gemini-3-flash-preview", label: "gemini-3-flash-preview" }, + { id: "gemini-3.1-pro-preview", label: "Gemini 3.1 Pro (preview)" }, + { id: "gemini-3-flash-preview", label: "Gemini 3 Flash (preview)" }, { id: "gemini-3.1-flash-lite-preview", - label: "gemini-3.1-flash-lite-preview", + label: "Gemini 3.1 Flash Lite (preview)", }, - { id: "gemini-2.5-pro", label: "gemini-2.5-pro" }, - { id: "gemini-2.5-flash", label: "gemini-2.5-flash" }, - { id: "gemini-2.5-flash-lite", label: "gemini-2.5-flash-lite" }, + { id: "gemini-2.5-pro", label: "Gemini 2.5 Pro" }, + { id: "gemini-2.5-flash", label: "Gemini 2.5 Flash" }, + { id: "gemini-2.5-flash-lite", label: "Gemini 2.5 Flash Lite" }, ]; // Each entry's ``default_command`` is the published-package npx diff --git a/src/i18n/translation.json b/src/i18n/translation.json index ca1b8a319..8cde8d7fb 100644 --- a/src/i18n/translation.json +++ b/src/i18n/translation.json @@ -5135,37 +5135,37 @@ }, "SETTINGS$AGENT_CUSTOM_MODEL": { "en": "Custom model", - "ja": "Custom model", - "zh-CN": "Custom model", - "zh-TW": "Custom model", - "ko-KR": "Custom model", - "no": "Custom model", - "it": "Custom model", - "pt": "Custom model", - "es": "Custom model", - "ar": "Custom model", - "fr": "Custom model", - "tr": "Custom model", - "de": "Custom model", - "uk": "Custom model", - "ca": "Custom model" + "ja": "カスタムモデル", + "zh-CN": "自定义模型", + "zh-TW": "自訂模型", + "ko-KR": "사용자 정의 모델", + "no": "Egendefinert modell", + "it": "Modello personalizzato", + "pt": "Modelo personalizado", + "es": "Modelo personalizado", + "ar": "نموذج مخصص", + "fr": "Modèle personnalisé", + "tr": "Özel model", + "de": "Benutzerdefiniertes Modell", + "uk": "Користувацька модель", + "ca": "Model personalitzat" }, "SETTINGS$AGENT_MODEL_HINT": { "en": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", - "ja": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", - "zh-CN": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", - "zh-TW": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", - "ko-KR": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", - "no": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", - "it": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", - "pt": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", - "es": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", - "ar": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", - "fr": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", - "tr": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", - "de": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", - "uk": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank.", - "ca": "Choose a suggested ACP model or enter a custom model override. Built-in providers save a concrete model instead of leaving the model blank." + "ja": "推奨される ACP モデルを選択するか、カスタムモデルの上書きを入力してください。組み込みプロバイダーはモデルを空白のままにせず、具体的なモデルを保存します。", + "zh-CN": "选择一个建议的 ACP 模型，或输入自定义模型覆盖。内置提供商会保存一个具体的模型，而不是将模型留空。", + "zh-TW": "選擇一個建議的 ACP 模型，或輸入自訂模型覆寫。內建供應商會儲存具體的模型，而不是將模型留空。", + "ko-KR": "추천 ACP 모델을 선택하거나 사용자 정의 모델 재정의를 입력하세요. 기본 제공 공급자는 모델을 비워두지 않고 구체적인 모델을 저장합니다.", + "no": "Velg en foreslått ACP-modell eller skriv inn en egendefinert modelloverstyring. Innebygde leverandører lagrer en konkret modell i stedet for å la modellen være tom.", + "it": "Scegli un modello ACP suggerito o inserisci una sostituzione personalizzata. I provider integrati salvano un modello concreto invece di lasciarlo vuoto.", + "pt": "Escolha um modelo ACP sugerido ou insira uma substituição de modelo personalizada. Os provedores integrados salvam um modelo concreto em vez de deixá-lo em branco.", + "es": "Elige un modelo ACP sugerido o introduce una sustitución de modelo personalizada. Los proveedores integrados guardan un modelo concreto en lugar de dejarlo en blanco.", + "ar": "اختر نموذج ACP مقترحًا أو أدخل تجاوزًا لنموذج مخصص. تقوم المزودات المدمجة بحفظ نموذج محدد بدلًا من ترك النموذج فارغًا.", + "fr": "Choisissez un modèle ACP suggéré ou saisissez un remplacement de modèle personnalisé. Les fournisseurs intégrés enregistrent un modèle concret plutôt que de laisser le champ vide.", + "tr": "Önerilen bir ACP modelini seçin veya özel bir model geçersiz kılma değeri girin. Yerleşik sağlayıcılar, modeli boş bırakmak yerine somut bir model kaydeder.", + "de": "Wähle ein vorgeschlagenes ACP-Modell aus oder gib eine benutzerdefinierte Modellüberschreibung ein. Integrierte Anbieter speichern ein konkretes Modell, anstatt das Modellfeld leer zu lassen.", + "uk": "Виберіть рекомендовану модель ACP або введіть користувацьке перевизначення моделі. Вбудовані постачальники зберігають конкретну модель замість того, щоб залишати поле порожнім.", + "ca": "Tria un model ACP suggerit o introdueix una substitució de model personalitzada. Els proveïdors integrats desen un model concret en lloc de deixar-lo en blanc." }, "SETTINGS$AGENT_DISABLED_TOOLTIP": { "en": "Disabled while {{agentName}} is the active agent",