From 0f59445d6fa7626adae0ebb8e5d6b7d6c0fecf60 Mon Sep 17 00:00:00 2001 From: Debug Agent Date: Mon, 1 Jun 2026 11:27:52 +0200 Subject: [PATCH 1/2] feat(onboarding): auto-detect ACP subscription/login via /api/acp/auth-status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Onboarding now probes whether the chosen ACP provider's CLI is already authenticated (Claude Pro/Max, ChatGPT, Google subscription — or a pre-set API key) and shows a "✓ you're already logged in" banner instead of unconditionally asking for an API key. Refs OpenHands/agent-canvas#964. Detection rides the agent-server's purpose-built endpoint added in software-agent-sdk#3452, which drives the ACP initialize + session/new handshake server-side (no prompt → no model tokens) and returns authenticated / unauthenticated / unknown. - `AcpService.getAuthStatus(server)` wraps the new `AcpClient` from `@openhands/typescript-client`, keeping all agent-server access typed (the no-direct-agent-server-calls guard stays satisfied). - `useAcpAuthStatus` now calls `GET /api/acp/auth-status` instead of the canvas-only "Phase 0" throwaway-conversation probe (closed PR #968). A failed/unreachable probe falls back to "unknown" so we never falsely claim "not logged in". The hook's public shape is unchanged, so the onboarding banner UI carried over from #968 did not change. - Gated to local backends with credential fields; cached per provider per backend; never blocks the (skippable) step. The `@openhands/typescript-client` dependency is temporarily pinned to the `AcpClient` commit (OpenHands/typescript-client#196) by SHA so this can be validated end-to-end before that PR is released; swap to the released version once published. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../onboarding/onboarding-modal.test.tsx | 11 ++ .../setup-acp-secrets-step.test.tsx | 82 +++++++++- .../hooks/query/use-acp-auth-status.test.tsx | 140 ++++++++++++++++++ package-lock.json | 10 +- package.json | 2 +- src/api/acp-service/acp-service.api.ts | 24 +++ .../features/onboarding/onboarding-modal.tsx | 9 +- .../steps/setup-acp-secrets-step.tsx | 46 ++++++ src/hooks/query/use-acp-auth-status.ts | 98 ++++++++++++ src/i18n/translation.json | 34 +++++ 10 files changed, 448 insertions(+), 8 deletions(-) create mode 100644 __tests__/hooks/query/use-acp-auth-status.test.tsx create mode 100644 src/api/acp-service/acp-service.api.ts create mode 100644 src/hooks/query/use-acp-auth-status.ts diff --git a/__tests__/components/onboarding/onboarding-modal.test.tsx b/__tests__/components/onboarding/onboarding-modal.test.tsx index 883dab5c..5d6de12f 100644 --- a/__tests__/components/onboarding/onboarding-modal.test.tsx +++ b/__tests__/components/onboarding/onboarding-modal.test.tsx @@ -86,6 +86,17 @@ vi.mock("#/hooks/mutation/use-create-conversation", () => ({ }), })); +// The ACP credentials slide runs a login-detection probe (creates a throwaway +// conversation). Stub it here so the modal routing tests don't spin one; the +// probe itself is covered in use-acp-auth-status.test.tsx. +vi.mock("#/hooks/query/use-acp-auth-status", () => ({ + useAcpAuthStatus: () => ({ + status: "unknown", + isChecking: false, + isSupported: false, + }), +})); + function renderModal(onClose = vi.fn()) { const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } }, diff --git a/__tests__/components/onboarding/setup-acp-secrets-step.test.tsx b/__tests__/components/onboarding/setup-acp-secrets-step.test.tsx index af5b1aac..b9808fad 100644 --- a/__tests__/components/onboarding/setup-acp-secrets-step.test.tsx +++ b/__tests__/components/onboarding/setup-acp-secrets-step.test.tsx @@ -10,7 +10,18 @@ import { SetupAcpSecretsStep } from "#/components/features/onboarding/steps/setu import { type OnboardingAgentId } from "#/components/features/onboarding/steps/choose-agent-step"; import { SecretsService } from "#/api/secrets-service"; -function renderStep(providerKey: OnboardingAgentId = "claude-code") { +// The login-detection probe is exercised in its own hook test; here we stub it +// so rendering the step doesn't spin a conversation, and so we can drive the +// banner states directly. +const acpAuthStatusMock = vi.hoisted(() => vi.fn()); +vi.mock("#/hooks/query/use-acp-auth-status", () => ({ + useAcpAuthStatus: (...args: unknown[]) => acpAuthStatusMock(...args), +})); + +function renderStep( + providerKey: OnboardingAgentId = "claude-code", + isActive = true, +) { const onBack = vi.fn(); const onNext = vi.fn(); const user = userEvent.setup(); @@ -23,6 +34,7 @@ function renderStep(providerKey: OnboardingAgentId = "claude-code") { @@ -53,6 +65,11 @@ async function renderWithSavedApiKey() { beforeEach(() => { vi.restoreAllMocks(); __resetActiveStoreForTests(); + acpAuthStatusMock.mockReturnValue({ + status: "unknown", + isChecking: false, + isSupported: false, + }); vi.spyOn(SecretsService, "getSecrets").mockResolvedValue([]); vi.spyOn(SecretsService, "createSecret").mockResolvedValue(); }); @@ -166,4 +183,67 @@ describe("SetupAcpSecretsStep", () => { ); expect(onNext).not.toHaveBeenCalled(); }); + + it("runs the login probe scoped to the active step and provider", () => { + renderStep("claude-code", true); + expect(acpAuthStatusMock).toHaveBeenCalledWith("claude-code", { + enabled: true, + }); + }); + + it("disables the login probe when the step is not active", () => { + renderStep("claude-code", false); + expect(acpAuthStatusMock).toHaveBeenCalledWith("claude-code", { + enabled: false, + }); + }); + + it("shows the 'checking' banner while the login probe is in flight", () => { + acpAuthStatusMock.mockReturnValue({ + status: "unknown", + isChecking: true, + isSupported: true, + }); + renderStep("claude-code"); + + expect( + screen.getByTestId("onboarding-acp-auth-checking"), + ).toBeInTheDocument(); + expect( + screen.queryByTestId("onboarding-acp-auth-detected"), + ).not.toBeInTheDocument(); + }); + + it("shows the 'already signed in' banner when authenticated, keeping the key fields", () => { + acpAuthStatusMock.mockReturnValue({ + status: "authenticated", + isChecking: false, + isSupported: true, + }); + renderStep("claude-code"); + + expect( + screen.getByTestId("onboarding-acp-auth-detected"), + ).toBeInTheDocument(); + // The fields stay visible (now optional) even when already logged in. + expect( + screen.getByTestId("onboarding-acp-secret-ANTHROPIC_API_KEY"), + ).toBeInTheDocument(); + }); + + it("shows no banner when the provider is not authenticated", () => { + acpAuthStatusMock.mockReturnValue({ + status: "unauthenticated", + isChecking: false, + isSupported: true, + }); + renderStep("claude-code"); + + expect( + screen.queryByTestId("onboarding-acp-auth-detected"), + ).not.toBeInTheDocument(); + expect( + screen.queryByTestId("onboarding-acp-auth-checking"), + ).not.toBeInTheDocument(); + }); }); diff --git a/__tests__/hooks/query/use-acp-auth-status.test.tsx b/__tests__/hooks/query/use-acp-auth-status.test.tsx new file mode 100644 index 00000000..94842f26 --- /dev/null +++ b/__tests__/hooks/query/use-acp-auth-status.test.tsx @@ -0,0 +1,140 @@ +import React from "react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { renderHook, waitFor } from "@testing-library/react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { useAcpAuthStatus } from "#/hooks/query/use-acp-auth-status"; +import type { ACPAuthStatusResponse } from "@openhands/typescript-client"; + +// Active backend is swapped per-test (local vs cloud) via this mutable holder. +const backendMock = vi.hoisted(() => ({ + current: { + backend: { id: "local-1", kind: "local" as "local" | "cloud" }, + orgId: null as string | null, + }, +})); +vi.mock("#/contexts/active-backend-context", () => ({ + useActiveBackend: () => backendMock.current, +})); + +const getAuthStatus = vi.hoisted(() => vi.fn()); +vi.mock("#/api/acp-service/acp-service.api", () => ({ + default: { + getAuthStatus: (...args: unknown[]) => getAuthStatus(...args), + }, +})); + +// Build a full ACPAuthStatusResponse with the given status; the hook only +// reads ``.status`` but the service returns the whole record. +function authResponse( + status: ACPAuthStatusResponse["status"], +): ACPAuthStatusResponse { + return { + server: "claude-code", + status, + auth_methods: [], + agent_name: "", + agent_version: "", + detail: null, + }; +} + +function wrapper({ children }: { children: React.ReactNode }) { + const client = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + return {children}; +} + +beforeEach(() => { + vi.clearAllMocks(); + backendMock.current = { + backend: { id: "local-1", kind: "local" }, + orgId: null, + }; +}); + +describe("useAcpAuthStatus", () => { + it("reports authenticated when the probe endpoint says so", async () => { + getAuthStatus.mockResolvedValue(authResponse("authenticated")); + + const { result } = renderHook(() => useAcpAuthStatus("claude-code"), { + wrapper, + }); + + await waitFor(() => expect(result.current.status).toBe("authenticated")); + // The provider key parameterizes the probe. + expect(getAuthStatus).toHaveBeenCalledTimes(1); + expect(getAuthStatus).toHaveBeenCalledWith("claude-code"); + }); + + it("reports unauthenticated when the endpoint reports auth_required", async () => { + getAuthStatus.mockResolvedValue(authResponse("unauthenticated")); + + const { result } = renderHook(() => useAcpAuthStatus("codex"), { wrapper }); + + await waitFor(() => expect(result.current.status).toBe("unauthenticated")); + expect(getAuthStatus).toHaveBeenCalledWith("codex"); + }); + + it("falls back to unknown when the probe call rejects", async () => { + getAuthStatus.mockRejectedValue(new Error("network down")); + + const { result } = renderHook(() => useAcpAuthStatus("claude-code"), { + wrapper, + }); + + // A rejected probe must not be read as "not logged in" — it stays unknown + // so the caller keeps showing the API-key fields. + await waitFor(() => expect(result.current.isChecking).toBe(false)); + expect(result.current.status).toBe("unknown"); + }); + + it("surfaces an unknown status from the endpoint verbatim", async () => { + getAuthStatus.mockResolvedValue(authResponse("unknown")); + + const { result } = renderHook(() => useAcpAuthStatus("claude-code"), { + wrapper, + }); + + await waitFor(() => expect(result.current.isChecking).toBe(false)); + expect(result.current.status).toBe("unknown"); + }); + + it("does not probe on a cloud backend", async () => { + backendMock.current = { + backend: { id: "cloud-1", kind: "cloud" }, + orgId: null, + }; + + const { result } = renderHook(() => useAcpAuthStatus("claude-code"), { + wrapper, + }); + + await Promise.resolve(); + expect(result.current.status).toBe("unknown"); + expect(result.current.isSupported).toBe(false); + expect(getAuthStatus).not.toHaveBeenCalled(); + }); + + it("does not probe for a provider with no credential fields (gemini-cli)", async () => { + const { result } = renderHook(() => useAcpAuthStatus("gemini-cli"), { + wrapper, + }); + + await Promise.resolve(); + expect(result.current.isSupported).toBe(false); + expect(getAuthStatus).not.toHaveBeenCalled(); + }); + + it("does not probe when disabled (e.g. the step is not the active slide)", async () => { + const { result } = renderHook( + () => useAcpAuthStatus("claude-code", { enabled: false }), + { wrapper }, + ); + + await Promise.resolve(); + expect(result.current.status).toBe("unknown"); + expect(getAuthStatus).not.toHaveBeenCalled(); + }); +}); diff --git a/package-lock.json b/package-lock.json index 0e6dbbd5..fbfe5497 100644 --- a/package-lock.json +++ b/package-lock.json @@ -13,7 +13,7 @@ "@microlink/react-json-view": "1.31.20", "@monaco-editor/react": "4.7.0", "@openhands/extensions": "git+https://github.com/OpenHands/extensions.git#e14f740c59b4bfd7369d4bb6aea5eeb33dd05909", - "@openhands/typescript-client": "1.24.3", + "@openhands/typescript-client": "git+https://github.com/OpenHands/typescript-client.git#db948d6767ea05b1f8c21496c53a913cad2c7bc6", "@react-router/node": "7.14.2", "@react-router/serve": "7.14.2", "@tailwindcss/vite": "4.2.4", @@ -3441,7 +3441,7 @@ }, "node_modules/@openhands/extensions": { "version": "0.0.0", - "resolved": "git+https://github.com/OpenHands/extensions.git#e14f740c59b4bfd7369d4bb6aea5eeb33dd05909", + "resolved": "git+ssh://git@github.com/OpenHands/extensions.git#e14f740c59b4bfd7369d4bb6aea5eeb33dd05909", "integrity": "sha512-PC0pmJD1AiNP9aDfdDfDPP1iF40m/QN3vyv/xPN9sigLBtCmNBugSIySWEGVIGydARYkVi+NHL2KorPtlAOFAg==", "license": "MIT", "engines": { @@ -3454,9 +3454,9 @@ } }, "node_modules/@openhands/typescript-client": { - "version": "1.24.3", - "resolved": "https://registry.npmjs.org/@openhands/typescript-client/-/typescript-client-1.24.3.tgz", - "integrity": "sha512-4w5rGbgXSnRKm6DZGedGRMWa4nIyl8/1+lEzinBIjZbdE0MD6+89EAItkk6FTvKU0jdVpoCAtmLWBP3Y/r8ORA==", + "version": "1.24.2", + "resolved": "git+ssh://git@github.com/OpenHands/typescript-client.git#db948d6767ea05b1f8c21496c53a913cad2c7bc6", + "integrity": "sha512-ghv8ZG7i28yNwm4zbvsxGverPPz4ZPFNAJ1cZxoI/NjNjST8OEUK5JM1RCQzWX1qOi2HrJtb+qu8R2LeVT5BPw==", "license": "MIT", "dependencies": { "@openrouter/sdk": "^0.12.35", diff --git a/package.json b/package.json index 32fc798c..c118d18d 100644 --- a/package.json +++ b/package.json @@ -24,7 +24,7 @@ "@microlink/react-json-view": "1.31.20", "@monaco-editor/react": "4.7.0", "@openhands/extensions": "git+https://github.com/OpenHands/extensions.git#e14f740c59b4bfd7369d4bb6aea5eeb33dd05909", - "@openhands/typescript-client": "1.24.3", + "@openhands/typescript-client": "git+https://github.com/OpenHands/typescript-client.git#db948d6767ea05b1f8c21496c53a913cad2c7bc6", "@react-router/node": "7.14.2", "@react-router/serve": "7.14.2", "@tailwindcss/vite": "4.2.4", diff --git a/src/api/acp-service/acp-service.api.ts b/src/api/acp-service/acp-service.api.ts new file mode 100644 index 00000000..9d63f5d2 --- /dev/null +++ b/src/api/acp-service/acp-service.api.ts @@ -0,0 +1,24 @@ +import { AcpClient } from "@openhands/typescript-client/clients"; +import type { ACPAuthStatusResponse } from "@openhands/typescript-client"; +import { getAgentServerClientOptions } from "../agent-server-client-options"; + +/** + * Canvas wrapper over the agent-server ACP routes (``/api/acp``), mirroring the + * other ``*Service`` classes: it owns construction of the typed + * {@link AcpClient} from the active backend's client options, so callers never + * touch the agent-server URL / session key directly (and the + * no-direct-agent-server-calls guard stays satisfied). + */ +class AcpService { + /** + * Probe whether ``server``'s ACP CLI is already authenticated on the active + * agent-server — by a subscription login or a pre-set API key. Drives the ACP + * handshake server-side and sends no prompt, so it spends no model tokens. + * See ``GET /api/acp/auth-status``. + */ + static async getAuthStatus(server: string): Promise { + return new AcpClient(getAgentServerClientOptions()).getAuthStatus(server); + } +} + +export default AcpService; diff --git a/src/components/features/onboarding/onboarding-modal.tsx b/src/components/features/onboarding/onboarding-modal.tsx index f93d9aae..4b89804f 100644 --- a/src/components/features/onboarding/onboarding-modal.tsx +++ b/src/components/features/onboarding/onboarding-modal.tsx @@ -20,6 +20,12 @@ import { getAcpProviderSecrets } from "#/constants/acp-providers"; const TOTAL_STEPS = 4; +// Index of the per-provider setup slide (LLM form for OpenHands, ACP +// credentials for Claude Code / Codex). Named so the slide and the +// ``isActive`` gate that drives the ACP login probe move together — inserting +// a slide before it can't silently fire the probe on the wrong step. +const SETUP_SLIDE_INDEX = 2; + interface SlideProps { /** Index of this slide in the step sequence. */ index: number; @@ -175,12 +181,13 @@ export function OnboardingModal({ onClose }: OnboardingModalProps) { - + {isOpenHands ? ( ) : showAcpSecretsStep ? ( diff --git a/src/components/features/onboarding/steps/setup-acp-secrets-step.tsx b/src/components/features/onboarding/steps/setup-acp-secrets-step.tsx index 2b9aabc5..e8025726 100644 --- a/src/components/features/onboarding/steps/setup-acp-secrets-step.tsx +++ b/src/components/features/onboarding/steps/setup-acp-secrets-step.tsx @@ -2,11 +2,13 @@ import React from "react"; import { useTranslation } from "react-i18next"; import { useQueryClient } from "@tanstack/react-query"; import { AxiosError } from "axios"; +import { Check, Loader2 } from "lucide-react"; import { BrandButton } from "#/components/features/settings/brand-button"; import { SettingsInput } from "#/components/features/settings/settings-input"; import { I18nKey } from "#/i18n/declaration"; import { useCreateSecret } from "#/hooks/mutation/use-create-secret"; import { useSearchSecrets } from "#/hooks/query/use-get-secrets"; +import { useAcpAuthStatus } from "#/hooks/query/use-acp-auth-status"; import { getAcpProviderDisplayName, getAcpProviderSecrets, @@ -25,6 +27,13 @@ interface SetupAcpSecretsStepProps { * form. Providers without a credentials entry (``"openhands"``, * ``"gemini-cli"``) simply yield no fields. */ providerKey: OnboardingAgentId; + /** + * Whether this is the currently visible onboarding slide. The modal mounts + * every slide at once, so we only run the (subprocess-spinning) login probe + * once the user has actually reached this step — by which point the backend + * is confirmed connected. + */ + isActive: boolean; onBack: () => void; onNext: () => void; } @@ -45,6 +54,7 @@ interface SetupAcpSecretsStepProps { */ export function SetupAcpSecretsStep({ providerKey, + isActive, onBack, onNext, }: SetupAcpSecretsStepProps) { @@ -52,6 +62,11 @@ export function SetupAcpSecretsStep({ const queryClient = useQueryClient(); const { mutateAsync: createSecret } = useCreateSecret(); const { data: existingSecrets } = useSearchSecrets(); + // Subscription/login detection via GET /api/acp/auth-status — see issue #964. + const { status: authStatus, isChecking: isCheckingAuth } = useAcpAuthStatus( + providerKey, + { enabled: isActive }, + ); const fields = React.useMemo( () => getAcpProviderSecrets(providerKey), @@ -117,6 +132,37 @@ export function SetupAcpSecretsStep({

+ {authStatus === "authenticated" ? ( +
+ + + {t(I18nKey.ONBOARDING$ACP_AUTH_DETECTED, { + provider: providerName, + })} + +
+ ) : isCheckingAuth ? ( +
+ + + {t(I18nKey.ONBOARDING$ACP_AUTH_CHECKING, { + provider: providerName, + })} + +
+ ) : null} +
{fields.map((field) => { const alreadySet = secretExists(field.name); diff --git a/src/hooks/query/use-acp-auth-status.ts b/src/hooks/query/use-acp-auth-status.ts new file mode 100644 index 00000000..4cc26d79 --- /dev/null +++ b/src/hooks/query/use-acp-auth-status.ts @@ -0,0 +1,98 @@ +import { useQuery } from "@tanstack/react-query"; +import AcpService from "#/api/acp-service/acp-service.api"; +import { useActiveBackend } from "#/contexts/active-backend-context"; +import { getAcpProviderSecrets } from "#/constants/acp-providers"; + +export type AcpAuthStatus = "authenticated" | "unauthenticated" | "unknown"; + +/** + * Probe whether the selected ACP provider's CLI is already authenticated on + * the agent-server — by a subscription login (Claude Pro/Max, ChatGPT, Google) + * or a pre-set API key. + * + * Calls the agent-server's purpose-built ``GET /api/acp/auth-status`` endpoint, + * which drives the ACP ``initialize`` + ``session/new`` handshake server-side + * and classifies the outcome — ``session/new`` succeeding ⇒ authenticated, an + * ``auth_required`` error ⇒ unauthenticated, and anything that prevents the + * probe from completing ⇒ unknown. No prompt is sent, so no model tokens are + * spent. + * + * This is "Phase 1" of issue #964: it replaces the canvas-only "Phase 0" + * throwaway-conversation probe (create → observe → delete) now that the + * agent-server (software-agent-sdk #3452) exposes a dedicated endpoint. The + * hook's public shape is unchanged, so the onboarding banner UI that consumes + * it did not need to change. + */ +async function probeAcpAuth(providerKey: string): Promise { + try { + const { status } = await AcpService.getAuthStatus(providerKey); + // ``status`` is already authenticated / unauthenticated / unknown. + return status; + } catch { + // The endpoint is unreachable, rejected the request (e.g. an unknown + // provider → 422), or the agent-server predates the route (→ 404): fall + // back to "unknown" so the caller shows the API-key fields rather than + // falsely claiming "not logged in". + return "unknown"; + } +} + +interface UseAcpAuthStatusOptions { + /** + * Gate the probe to when the consuming surface is actually visible — the + * onboarding modal mounts every slide at once, so without this the probe + * would fire (and spin a subprocess) before the user reaches the step and + * before the backend is confirmed connected. Defaults to ``true``. + */ + enabled?: boolean; +} + +/** + * React Query wrapper around {@link probeAcpAuth}. + * + * Gated to **local backends only**: subscription credentials live wherever the + * agent-server runs, so on a remote/cloud backend they're ~never present and a + * probe would needlessly spin a runtime — there we return ``"unknown"`` and let + * the caller fall back to the (already optional) API-key fields. + * + * The probe spins and kills a subprocess, so the result is cached for the + * session (``staleTime: Infinity``, no refetch on focus/mount) — one probe per + * provider per backend. + */ +export function useAcpAuthStatus( + providerKey: string | null | undefined, + options: UseAcpAuthStatusOptions = {}, +) { + const { enabled = true } = options; + const active = useActiveBackend(); + const isLocal = active.backend.kind === "local"; + const hasCredentials = getAcpProviderSecrets(providerKey).length > 0; + const isSupported = isLocal && hasCredentials; + const queryEnabled = enabled && isSupported && !!providerKey; + + const query = useQuery({ + // ``providerKey`` both discriminates the cache (so switching providers + // re-probes) and parameterizes the probe — ``queryEnabled`` guarantees it + // is non-empty whenever the query runs. + queryKey: ["acp-auth-status", active.backend.id, providerKey], + queryFn: () => probeAcpAuth(providerKey as string), + enabled: queryEnabled, + // ``staleTime: Infinity`` = never re-probe while the result stays cached; + // ``gcTime`` then bounds that to ~15 min after the hook unmounts. So a + // user who dismisses and reopens onboarding >15 min later re-probes — + // intentional: it's a cheap one-off and their login state may have changed. + staleTime: Infinity, + gcTime: 1000 * 60 * 15, + retry: false, + refetchOnWindowFocus: false, + refetchOnMount: false, + }); + + return { + status: query.data ?? "unknown", + /** True while the first probe for this provider is in flight. */ + isChecking: queryEnabled && query.isFetching && query.data === undefined, + /** Whether a probe can run at all on this backend (local + has creds). */ + isSupported, + }; +} diff --git a/src/i18n/translation.json b/src/i18n/translation.json index 3a60b415..e6f85dca 100644 --- a/src/i18n/translation.json +++ b/src/i18n/translation.json @@ -27624,6 +27624,40 @@ "tr": "Zaten kaydedildi — korumak için boş bırakın", "uk": "Уже збережено — залиште порожнім, щоб зберегти" }, + "ONBOARDING$ACP_AUTH_CHECKING": { + "en": "Checking for an existing {{provider}} login…", + "ja": "{{provider}} の既存のログインを確認しています…", + "zh-CN": "正在检查是否已登录 {{provider}}…", + "zh-TW": "正在檢查是否已登入 {{provider}}…", + "ko-KR": "기존 {{provider}} 로그인을 확인하는 중…", + "no": "Ser etter en eksisterende {{provider}}-pålogging …", + "ar": "يتم التحقق من وجود تسجيل دخول حالي إلى {{provider}}…", + "de": "Suche nach einer bestehenden {{provider}}-Anmeldung…", + "fr": "Recherche d'une connexion {{provider}} existante…", + "it": "Verifica di un accesso {{provider}} esistente…", + "pt": "Procurando um login existente do {{provider}}…", + "es": "Comprobando si ya has iniciado sesión en {{provider}}…", + "ca": "S'està comprovant si ja has iniciat la sessió a {{provider}}…", + "tr": "Mevcut bir {{provider}} oturumu denetleniyor…", + "uk": "Перевіряємо наявний вхід у {{provider}}…" + }, + "ONBOARDING$ACP_AUTH_DETECTED": { + "en": "You're already signed in to {{provider}} — adding an API key below is optional.", + "ja": "すでに {{provider}} にサインインしています。下の API キーの追加は任意です。", + "zh-CN": "你已登录 {{provider}} — 下面的 API 密钥为可选项。", + "zh-TW": "你已登入 {{provider}} — 下方的 API 金鑰為選填。", + "ko-KR": "이미 {{provider}}에 로그인되어 있습니다 — 아래 API 키 입력은 선택 사항입니다.", + "no": "Du er allerede logget på {{provider}} – å legge til en API-nøkkel nedenfor er valgfritt.", + "ar": "أنت مسجّل الدخول بالفعل إلى {{provider}} — إضافة مفتاح API أدناه اختيارية.", + "de": "Du bist bereits bei {{provider}} angemeldet – das Hinzufügen eines API-Schlüssels unten ist optional.", + "fr": "Vous êtes déjà connecté à {{provider}} — ajouter une clé API ci-dessous est facultatif.", + "it": "Hai già effettuato l'accesso a {{provider}} — aggiungere una chiave API qui sotto è facoltativo.", + "pt": "Você já está conectado ao {{provider}} — adicionar uma chave de API abaixo é opcional.", + "es": "Ya has iniciado sesión en {{provider}}: añadir una clave de API abajo es opcional.", + "ca": "Ja has iniciat la sessió a {{provider}} — afegir una clau API a sota és opcional.", + "tr": "{{provider}} hesabınızda zaten oturum açtınız — aşağıya bir API anahtarı eklemek isteğe bağlıdır.", + "uk": "Ви вже ввійшли в {{provider}} — додавати ключ API нижче необов'язково." + }, "ERROR$FAILED_TO_LOAD_PROFILE_TRY_AGAIN": { "en": "Failed to load existing profile. Please try again.", "ja": "既存のプロファイルの読み込みに失敗しました。もう一度お試しください。", From f76d648195cf1c51f27af830427c2a406e4b4e66 Mon Sep 17 00:00:00 2001 From: Debug Agent Date: Mon, 1 Jun 2026 17:16:11 +0200 Subject: [PATCH 2/2] feat(onboarding): show the login banner for Gemini (and other OAuth providers) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gemini was the one provider the feature most benefits — it logs in via Google OAuth, so a user with no API key is exactly who should see "✓ you're already signed in" — yet it was never probed. The probe was gated on "does this provider have API-key fields" (`getAcpProviderSecrets(...).length > 0`), and the onboarding flow skipped the whole credentials slide for Gemini. The agent-server can detect Gemini fine (the handshake returns authenticated / auth_required). - `useAcpAuthStatus`: drop the `hasCredentials` gate — eligibility is now just "local backend". The server returns `unknown` (422 → caught) for anything it can't probe, so coupling to key-fields was both wrong (excluded Gemini) and unnecessary. `AcpAuthStatus` now aliases the client's `ACPAuthStatusValue` instead of a hand-redefined union. - Onboarding modal: every non-OpenHands provider now shows slide 2, so the `skipStep2` machinery (nav deltas + progress-bar remapping) is deleted — the flow is a plain 4-step sequence again. - Credentials step: for providers with no key fields it renders a login-status screen — "Sign in to {provider}" + a browser-OAuth subtitle, the "you're already signed in" banner when authenticated, and no inputs. Claude Code / Codex are unchanged (key fields + banner). - New i18n: ACP_LOGIN_TITLE, ACP_LOGIN_SUBTITLE, ACP_AUTH_DETECTED_NO_KEY. Tests: hook now asserts Gemini IS probed; modal asserts slide 2 renders as a key-less login screen (no skip, 4 segments); step asserts the login-only render. Refs OpenHands/agent-canvas#964 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../onboarding/onboarding-modal.test.tsx | 57 +++++++------------ .../setup-acp-secrets-step.test.tsx | 18 ++++++ .../hooks/query/use-acp-auth-status.test.tsx | 12 ++-- .../features/onboarding/onboarding-modal.tsx | 51 +++++------------ .../steps/setup-acp-secrets-step.tsx | 30 +++++++--- src/hooks/query/use-acp-auth-status.ts | 16 ++++-- src/i18n/translation.json | 51 +++++++++++++++++ 7 files changed, 143 insertions(+), 92 deletions(-) diff --git a/__tests__/components/onboarding/onboarding-modal.test.tsx b/__tests__/components/onboarding/onboarding-modal.test.tsx index 5d6de12f..00ff3350 100644 --- a/__tests__/components/onboarding/onboarding-modal.test.tsx +++ b/__tests__/components/onboarding/onboarding-modal.test.tsx @@ -86,9 +86,9 @@ vi.mock("#/hooks/mutation/use-create-conversation", () => ({ }), })); -// The ACP credentials slide runs a login-detection probe (creates a throwaway -// conversation). Stub it here so the modal routing tests don't spin one; the -// probe itself is covered in use-acp-auth-status.test.tsx. +// The ACP credentials slide runs a login-detection probe (calls +// GET /api/acp/auth-status). Stub it here so the modal routing tests don't hit +// the network; the probe itself is covered in use-acp-auth-status.test.tsx. vi.mock("#/hooks/query/use-acp-auth-status", () => ({ useAcpAuthStatus: () => ({ status: "unknown", @@ -294,26 +294,15 @@ describe("OnboardingModal", () => { expect(settings.contains(next)).toBe(false); }); - it("skips the step-2 slide for an ACP agent with no credentials to collect", async () => { + it("shows slide 2 as a login screen (no key fields) for a credential-less ACP agent", async () => { renderModal(); const user = userEvent.setup(); - // Pick Gemini CLI: it authenticates via an interactive OAuth login - // (no env-var API key), so it has no credentials step and slide 2 is - // skipped — unlike Claude Code / Codex, which now render one there. + // Pick Gemini CLI: it authenticates via an interactive OAuth login (no + // env-var API key), but the slide still shows so the "you're already + // signed in" banner can render — the flow no longer skips it. await user.click(screen.getByTestId("onboarding-agent-option-gemini-cli")); await user.click(screen.getByTestId("onboarding-agent-next")); - await waitFor( - () => - expect(screen.getByTestId("onboarding-modal")).toHaveAttribute( - "data-current-step", - "1", - ), - { timeout: 3000 }, - ); - - // Advancing again should jump straight to Say Hello (index 3) and - // bypass slide 2 — Gemini owns its own auth via the OAuth login. await waitFor( () => expect( @@ -323,43 +312,35 @@ describe("OnboardingModal", () => { ); await user.click(screen.getByTestId("onboarding-backend-next")); + // Lands on slide 2 (the ACP step) — not jumped past to Say Hello. await waitFor( () => expect(screen.getByTestId("onboarding-modal")).toHaveAttribute( "data-current-step", - "3", + "2", ), { timeout: 3000 }, ); - // All four slides remain mounted (the rail just translates them); - // the assertion that the LLM step was skipped is that slide 3 (Say - // Hello) is the active one immediately after the backend step, - // *not* slide 2 (LLM). expect(screen.getByTestId("onboarding-slide-2")).toHaveAttribute( - "data-active", - "false", - ); - expect(screen.getByTestId("onboarding-slide-3")).toHaveAttribute( "data-active", "true", ); - - // Progress bar reflects the *visited* step count, not the slide - // index — 3 segments total (not 4), and segment 2 is current (not - // segment 3, which would imply LLM was completed). Without this - // mapping, picking an ACP agent makes the bar show segment 2 as - // "completed" despite the user never visiting it. expect( - screen.queryByTestId("onboarding-progress-step-3"), + screen.getByTestId("onboarding-step-setup-acp-secrets"), + ).toBeInTheDocument(); + // No API-key inputs for Gemini — it's a login-status-only screen. + expect( + screen.queryByTestId("onboarding-acp-secret-GEMINI_API_KEY"), ).not.toBeInTheDocument(); + + // The flow keeps all four progress segments (nothing is skipped). + expect( + screen.getByTestId("onboarding-progress-step-3"), + ).toBeInTheDocument(); expect(screen.getByTestId("onboarding-progress-step-2")).toHaveAttribute( "data-state", "current", ); - expect(screen.getByTestId("onboarding-progress-step-1")).toHaveAttribute( - "data-state", - "completed", - ); }); it("shows the ACP credentials step on slide 2 for Claude Code and saves entered keys as secrets", async () => { diff --git a/__tests__/components/onboarding/setup-acp-secrets-step.test.tsx b/__tests__/components/onboarding/setup-acp-secrets-step.test.tsx index b9808fad..fe964b6b 100644 --- a/__tests__/components/onboarding/setup-acp-secrets-step.test.tsx +++ b/__tests__/components/onboarding/setup-acp-secrets-step.test.tsx @@ -231,6 +231,24 @@ describe("SetupAcpSecretsStep", () => { ).toBeInTheDocument(); }); + it("renders a login-only screen (banner, no key fields) for a credential-less provider", () => { + // Gemini authenticates via browser OAuth — no API-key fields. The step is + // purely a login-status screen and still shows the "signed in" banner. + acpAuthStatusMock.mockReturnValue({ + status: "authenticated", + isChecking: false, + isSupported: true, + }); + renderStep("gemini-cli"); + + expect( + screen.getByTestId("onboarding-acp-auth-detected"), + ).toBeInTheDocument(); + expect( + screen.queryByTestId("onboarding-acp-secret-GEMINI_API_KEY"), + ).not.toBeInTheDocument(); + }); + it("shows no banner when the provider is not authenticated", () => { acpAuthStatusMock.mockReturnValue({ status: "unauthenticated", diff --git a/__tests__/hooks/query/use-acp-auth-status.test.tsx b/__tests__/hooks/query/use-acp-auth-status.test.tsx index 94842f26..85bc9a78 100644 --- a/__tests__/hooks/query/use-acp-auth-status.test.tsx +++ b/__tests__/hooks/query/use-acp-auth-status.test.tsx @@ -117,14 +117,18 @@ describe("useAcpAuthStatus", () => { expect(getAuthStatus).not.toHaveBeenCalled(); }); - it("does not probe for a provider with no credential fields (gemini-cli)", async () => { + it("probes credential-less providers too (e.g. gemini-cli, OAuth login)", async () => { + // Eligibility is not tied to having API-key fields — the server can detect + // subscription/OAuth providers like Gemini, so the hook must still probe. + getAuthStatus.mockResolvedValue(authResponse("authenticated")); + const { result } = renderHook(() => useAcpAuthStatus("gemini-cli"), { wrapper, }); - await Promise.resolve(); - expect(result.current.isSupported).toBe(false); - expect(getAuthStatus).not.toHaveBeenCalled(); + await waitFor(() => expect(result.current.status).toBe("authenticated")); + expect(result.current.isSupported).toBe(true); + expect(getAuthStatus).toHaveBeenCalledWith("gemini-cli"); }); it("does not probe when disabled (e.g. the step is not the active slide)", async () => { diff --git a/src/components/features/onboarding/onboarding-modal.tsx b/src/components/features/onboarding/onboarding-modal.tsx index 4b89804f..07daa3c4 100644 --- a/src/components/features/onboarding/onboarding-modal.tsx +++ b/src/components/features/onboarding/onboarding-modal.tsx @@ -16,7 +16,6 @@ import { CheckBackendStep } from "./steps/check-backend-step"; import { SetupLlmStep } from "./steps/setup-llm-step"; import { SetupAcpSecretsStep } from "./steps/setup-acp-secrets-step"; import { SayHelloStep } from "./steps/say-hello-step"; -import { getAcpProviderSecrets } from "#/constants/acp-providers"; const TOTAL_STEPS = 4; @@ -99,45 +98,21 @@ export function OnboardingModal({ onClose }: OnboardingModalProps) { // * OpenHands → the LLM-setup form (its own LLM config). // * Claude Code / Codex → the ACP secrets form (API key + base URL), since // these providers authenticate via env-var keys. - // * Gemini CLI → nothing: it authenticates through an interactive - // OAuth login, so there's no key to enter and we - // skip the slide entirely. - // ``getAcpProviderSecrets`` returns the field list (empty for Gemini), which - // is what distinguishes the ACP-with-secrets case from the skip case. + // * Gemini CLI → a login-status screen (no key fields; it signs in + // via browser OAuth) so the "you're already signed + // in" banner can still render. + // Every agent has slide-2 content, so the flow is a plain 4-step sequence + // with no skipping. const isOpenHands = selectedAgentId === "openhands"; - const acpSecretFields = getAcpProviderSecrets(selectedAgentId); - const showAcpSecretsStep = !isOpenHands && acpSecretFields.length > 0; - // Skip slide 2 only when there's nothing to show there (an ACP provider - // with no credentials to collect). Skipping keeps the rest of the flow - // intact in both directions (back from SayHello returns to CheckBackend, - // not a dead-end blank page). - const skipStep2 = !isOpenHands && !showAcpSecretsStep; const goNext = React.useCallback( - () => - setCurrentStep((step) => { - const delta = skipStep2 && step === 1 ? 2 : 1; - return Math.min(step + delta, TOTAL_STEPS - 1); - }), - [skipStep2], + () => setCurrentStep((step) => Math.min(step + 1, TOTAL_STEPS - 1)), + [], ); const goBack = React.useCallback( - () => - setCurrentStep((step) => { - const delta = skipStep2 && step === 3 ? 2 : 1; - return Math.max(step - delta, 0); - }), - [skipStep2], + () => setCurrentStep((step) => Math.max(step - 1, 0)), + [], ); - // The progress bar should show the user's actual visited-step count, - // not the underlying index. When slide 2 is skipped: - // * the bar renders 3 segments instead of 4, and - // * the SayHello slide (modal index 3) maps to logical step 2 so - // segment 2 doesn't pop "completed" on a slide the user never saw. - const progressTotal = skipStep2 ? TOTAL_STEPS - 1 : TOTAL_STEPS; - const progressStep = - skipStep2 && currentStep > 1 ? currentStep - 1 : currentStep; - return (
@@ -184,14 +159,14 @@ export function OnboardingModal({ onClose }: OnboardingModalProps) { {isOpenHands ? ( - ) : showAcpSecretsStep ? ( + ) : ( - ) : null} + )} diff --git a/src/components/features/onboarding/steps/setup-acp-secrets-step.tsx b/src/components/features/onboarding/steps/setup-acp-secrets-step.tsx index e8025726..8c127bf4 100644 --- a/src/components/features/onboarding/steps/setup-acp-secrets-step.tsx +++ b/src/components/features/onboarding/steps/setup-acp-secrets-step.tsx @@ -72,6 +72,11 @@ export function SetupAcpSecretsStep({ () => getAcpProviderSecrets(providerKey), [providerKey], ); + // Providers like Gemini authenticate via an interactive browser/OAuth login + // and have no API-key fields. For those the step is purely a login-status + // screen: it shows the "you're signed in" banner (or how to sign in) with no + // inputs to fill. + const hasFields = fields.length > 0; const [values, setValues] = React.useState>({}); const [isSaving, setIsSaving] = React.useState(false); @@ -123,12 +128,20 @@ export function SetupAcpSecretsStep({ >

- {t(I18nKey.ONBOARDING$ACP_SECRETS_TITLE)} + {t( + hasFields + ? I18nKey.ONBOARDING$ACP_SECRETS_TITLE + : I18nKey.ONBOARDING$ACP_LOGIN_TITLE, + { provider: providerName }, + )}

- {t(I18nKey.ONBOARDING$ACP_SECRETS_SUBTITLE, { - provider: providerName, - })} + {t( + hasFields + ? I18nKey.ONBOARDING$ACP_SECRETS_SUBTITLE + : I18nKey.ONBOARDING$ACP_LOGIN_SUBTITLE, + { provider: providerName }, + )}

@@ -144,9 +157,12 @@ export function SetupAcpSecretsStep({ aria-hidden /> - {t(I18nKey.ONBOARDING$ACP_AUTH_DETECTED, { - provider: providerName, - })} + {t( + hasFields + ? I18nKey.ONBOARDING$ACP_AUTH_DETECTED + : I18nKey.ONBOARDING$ACP_AUTH_DETECTED_NO_KEY, + { provider: providerName }, + )}
) : isCheckingAuth ? ( diff --git a/src/hooks/query/use-acp-auth-status.ts b/src/hooks/query/use-acp-auth-status.ts index 4cc26d79..dbf84a44 100644 --- a/src/hooks/query/use-acp-auth-status.ts +++ b/src/hooks/query/use-acp-auth-status.ts @@ -1,9 +1,10 @@ import { useQuery } from "@tanstack/react-query"; +import type { ACPAuthStatusValue } from "@openhands/typescript-client"; import AcpService from "#/api/acp-service/acp-service.api"; import { useActiveBackend } from "#/contexts/active-backend-context"; -import { getAcpProviderSecrets } from "#/constants/acp-providers"; -export type AcpAuthStatus = "authenticated" | "unauthenticated" | "unknown"; +/** Re-exported from the client so the union has a single source of truth. */ +export type AcpAuthStatus = ACPAuthStatusValue; /** * Probe whether the selected ACP provider's CLI is already authenticated on @@ -55,6 +56,12 @@ interface UseAcpAuthStatusOptions { * probe would needlessly spin a runtime — there we return ``"unknown"`` and let * the caller fall back to the (already optional) API-key fields. * + * Eligibility is intentionally *not* tied to whether the provider has API-key + * fields: the server can detect subscription/OAuth providers (e.g. Gemini) + * too, and an unknown/unsupported ``providerKey`` simply comes back as + * ``"unknown"`` (the endpoint 422s → caught below). The caller renders this + * hook only for ACP providers, so any local backend is probeable. + * * The probe spins and kills a subprocess, so the result is cached for the * session (``staleTime: Infinity``, no refetch on focus/mount) — one probe per * provider per backend. @@ -66,8 +73,7 @@ export function useAcpAuthStatus( const { enabled = true } = options; const active = useActiveBackend(); const isLocal = active.backend.kind === "local"; - const hasCredentials = getAcpProviderSecrets(providerKey).length > 0; - const isSupported = isLocal && hasCredentials; + const isSupported = isLocal; const queryEnabled = enabled && isSupported && !!providerKey; const query = useQuery({ @@ -92,7 +98,7 @@ export function useAcpAuthStatus( status: query.data ?? "unknown", /** True while the first probe for this provider is in flight. */ isChecking: queryEnabled && query.isFetching && query.data === undefined, - /** Whether a probe can run at all on this backend (local + has creds). */ + /** Whether a probe can run at all on this backend (local backends only). */ isSupported, }; } diff --git a/src/i18n/translation.json b/src/i18n/translation.json index e6f85dca..e79f4c49 100644 --- a/src/i18n/translation.json +++ b/src/i18n/translation.json @@ -1,4 +1,55 @@ { + "ONBOARDING$ACP_LOGIN_TITLE": { + "en": "Sign in to {{provider}}", + "ja": "Sign in to {{provider}}", + "zh-CN": "Sign in to {{provider}}", + "zh-TW": "Sign in to {{provider}}", + "ko-KR": "Sign in to {{provider}}", + "no": "Sign in to {{provider}}", + "ar": "Sign in to {{provider}}", + "de": "Sign in to {{provider}}", + "fr": "Sign in to {{provider}}", + "it": "Sign in to {{provider}}", + "pt": "Sign in to {{provider}}", + "es": "Sign in to {{provider}}", + "ca": "Sign in to {{provider}}", + "tr": "Sign in to {{provider}}", + "uk": "Sign in to {{provider}}" + }, + "ONBOARDING$ACP_LOGIN_SUBTITLE": { + "en": "{{provider}} signs in through your browser — no API key required. If you're not signed in yet, launch it once in a terminal to log in.", + "ja": "{{provider}} signs in through your browser — no API key required. If you're not signed in yet, launch it once in a terminal to log in.", + "zh-CN": "{{provider}} signs in through your browser — no API key required. If you're not signed in yet, launch it once in a terminal to log in.", + "zh-TW": "{{provider}} signs in through your browser — no API key required. If you're not signed in yet, launch it once in a terminal to log in.", + "ko-KR": "{{provider}} signs in through your browser — no API key required. If you're not signed in yet, launch it once in a terminal to log in.", + "no": "{{provider}} signs in through your browser — no API key required. If you're not signed in yet, launch it once in a terminal to log in.", + "ar": "{{provider}} signs in through your browser — no API key required. If you're not signed in yet, launch it once in a terminal to log in.", + "de": "{{provider}} signs in through your browser — no API key required. If you're not signed in yet, launch it once in a terminal to log in.", + "fr": "{{provider}} signs in through your browser — no API key required. If you're not signed in yet, launch it once in a terminal to log in.", + "it": "{{provider}} signs in through your browser — no API key required. If you're not signed in yet, launch it once in a terminal to log in.", + "pt": "{{provider}} signs in through your browser — no API key required. If you're not signed in yet, launch it once in a terminal to log in.", + "es": "{{provider}} signs in through your browser — no API key required. If you're not signed in yet, launch it once in a terminal to log in.", + "ca": "{{provider}} signs in through your browser — no API key required. If you're not signed in yet, launch it once in a terminal to log in.", + "tr": "{{provider}} signs in through your browser — no API key required. If you're not signed in yet, launch it once in a terminal to log in.", + "uk": "{{provider}} signs in through your browser — no API key required. If you're not signed in yet, launch it once in a terminal to log in." + }, + "ONBOARDING$ACP_AUTH_DETECTED_NO_KEY": { + "en": "You're already signed in to {{provider}} — you're all set.", + "ja": "You're already signed in to {{provider}} — you're all set.", + "zh-CN": "You're already signed in to {{provider}} — you're all set.", + "zh-TW": "You're already signed in to {{provider}} — you're all set.", + "ko-KR": "You're already signed in to {{provider}} — you're all set.", + "no": "You're already signed in to {{provider}} — you're all set.", + "ar": "You're already signed in to {{provider}} — you're all set.", + "de": "You're already signed in to {{provider}} — you're all set.", + "fr": "You're already signed in to {{provider}} — you're all set.", + "it": "You're already signed in to {{provider}} — you're all set.", + "pt": "You're already signed in to {{provider}} — you're all set.", + "es": "You're already signed in to {{provider}} — you're all set.", + "ca": "You're already signed in to {{provider}} — you're all set.", + "tr": "You're already signed in to {{provider}} — you're all set.", + "uk": "You're already signed in to {{provider}} — you're all set." + }, "MAINTENANCE$SCHEDULED_MESSAGE": { "en": "Scheduled maintenance will begin at {{time}}", "ja": "予定されたメンテナンスは{{time}}に開始されます",