diff --git a/__tests__/api/acp-service/acp-service.api.test.ts b/__tests__/api/acp-service/acp-service.api.test.ts new file mode 100644 index 00000000..d35a2b6c --- /dev/null +++ b/__tests__/api/acp-service/acp-service.api.test.ts @@ -0,0 +1,130 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { BashOutput } from "@openhands/typescript-client"; +import AcpService from "#/api/acp-service/acp-service.api"; + +// Capture the command the service runs and control the BashOutput it sees. +const executeCommand = vi.hoisted(() => vi.fn()); +vi.mock("@openhands/typescript-client/clients", () => ({ + BashClient: class { + executeCommand = executeCommand; + }, +})); +vi.mock("#/api/agent-server-client-options", () => ({ + getAgentServerClientOptions: () => ({ + host: "http://localhost", + workingDir: "/", + }), +})); + +function bashOutput(partial: Partial): BashOutput { + return { + id: "1", + timestamp: "2026-01-01T00:00:00Z", + command_id: "c1", + order: 0, + exit_code: 0, + stdout: null, + stderr: null, + kind: "BashOutput", + ...partial, + } as BashOutput; +} + +beforeEach(() => vi.clearAllMocks()); + +describe("AcpService.getAuthStatus", () => { + describe("claude-code (claude auth status --json)", () => { + it("runs the right command and maps loggedIn:true → authenticated", async () => { + executeCommand.mockResolvedValue( + bashOutput({ + stdout: JSON.stringify({ loggedIn: true, authMethod: "claude.ai" }), + }), + ); + await expect(AcpService.getAuthStatus("claude-code")).resolves.toBe( + "authenticated", + ); + expect(executeCommand).toHaveBeenCalledWith( + "claude auth status --json", + undefined, + expect.any(Number), + ); + }); + + it("maps loggedIn:false (even with a non-zero exit) → unauthenticated", async () => { + executeCommand.mockResolvedValue( + bashOutput({ + stdout: JSON.stringify({ loggedIn: false }), + exit_code: 1, + }), + ); + await expect(AcpService.getAuthStatus("claude-code")).resolves.toBe( + "unauthenticated", + ); + }); + + it("→ unknown when the CLI is missing (exit 127, no JSON on stdout)", async () => { + // The "no available ACP process / CLI not installed" path. + executeCommand.mockResolvedValue( + bashOutput({ + exit_code: 127, + stderr: "env: claude: No such file or directory", + }), + ); + await expect(AcpService.getAuthStatus("claude-code")).resolves.toBe( + "unknown", + ); + }); + }); + + describe("codex (codex login status)", () => { + it("→ authenticated even though the CLI writes to stderr", async () => { + executeCommand.mockResolvedValue( + bashOutput({ stderr: "Logged in using ChatGPT\n" }), + ); + await expect(AcpService.getAuthStatus("codex")).resolves.toBe( + "authenticated", + ); + }); + + it("→ unauthenticated on 'Not logged in'", async () => { + executeCommand.mockResolvedValue( + bashOutput({ stderr: "Not logged in\n" }), + ); + await expect(AcpService.getAuthStatus("codex")).resolves.toBe( + "unauthenticated", + ); + }); + + it("→ unknown when the CLI is missing", async () => { + executeCommand.mockResolvedValue( + bashOutput({ exit_code: 127, stderr: "codex: command not found" }), + ); + await expect(AcpService.getAuthStatus("codex")).resolves.toBe("unknown"); + }); + }); + + describe("gemini-cli (credentials file check)", () => { + it("→ authenticated when the creds file is present", async () => { + executeCommand.mockResolvedValue(bashOutput({ stdout: "present\n" })); + await expect(AcpService.getAuthStatus("gemini-cli")).resolves.toBe( + "authenticated", + ); + // Probes the OAuth creds file, not a (nonexistent) gemini status command. + expect(executeCommand.mock.calls[0][0]).toContain("oauth_creds.json"); + }); + + it("→ unauthenticated when the creds file is absent", async () => { + executeCommand.mockResolvedValue(bashOutput({ stdout: "absent\n" })); + await expect(AcpService.getAuthStatus("gemini-cli")).resolves.toBe( + "unauthenticated", + ); + }); + }); + + it("→ unknown for an unprobeable provider, without running any command", async () => { + await expect(AcpService.getAuthStatus("openhands")).resolves.toBe( + "unknown", + ); + expect(executeCommand).not.toHaveBeenCalled(); + }); +}); diff --git a/__tests__/components/onboarding/onboarding-modal.test.tsx b/__tests__/components/onboarding/onboarding-modal.test.tsx index 883dab5c..1a635f73 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 (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", + isChecking: false, + isSupported: false, + }), +})); + function renderModal(onClose = vi.fn()) { const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } }, @@ -283,26 +294,14 @@ 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 with Gemini's credential fields", 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: its key/base-URL come from the SDK registry like the + // other providers, so the slide shows the GEMINI_API_KEY field. 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( @@ -312,43 +311,36 @@ 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", ); + expect( + screen.getByTestId("onboarding-step-setup-acp-secrets"), + ).toBeInTheDocument(); + // Gemini exposes credential fields (GEMINI_API_KEY), derived from the SDK + // registry like Claude Code / Codex. + expect( + screen.getByTestId("onboarding-acp-secret-GEMINI_API_KEY"), + ).toBeInTheDocument(); - // 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. + // The flow keeps all four progress segments (nothing is skipped). expect( - screen.queryByTestId("onboarding-progress-step-3"), - ).not.toBeInTheDocument(); + 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 af5b1aac..eb8ebc57 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,86 @@ 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("renders Gemini's credential fields and the 'signed in' banner together", () => { + // Gemini's key/base-URL come from the SDK registry like the others, so the + // step shows the GEMINI_API_KEY field AND the detection banner (its Google + // login takes precedence, but a key can still be entered). + acpAuthStatusMock.mockReturnValue({ + status: "authenticated", + isChecking: false, + isSupported: true, + }); + renderStep("gemini-cli"); + + expect( + screen.getByTestId("onboarding-acp-auth-detected"), + ).toBeInTheDocument(); + expect( + screen.getByTestId("onboarding-acp-secret-GEMINI_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..6ffca8e0 --- /dev/null +++ b/__tests__/hooks/query/use-acp-auth-status.test.tsx @@ -0,0 +1,131 @@ +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"; + +// 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, +})); + +// AcpService.getAuthStatus resolves an AcpAuthStatus string (it runs the +// detection command + classifies internally; that logic is unit-tested in +// acp-service.api.test.ts). +const getAuthStatus = vi.hoisted(() => vi.fn()); +vi.mock("#/api/acp-service/acp-service.api", () => ({ + default: { + getAuthStatus: (...args: unknown[]) => getAuthStatus(...args), + }, +})); + +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 says so", async () => { + getAuthStatus.mockResolvedValue("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 probe says so", async () => { + getAuthStatus.mockResolvedValue("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 probe verbatim", async () => { + getAuthStatus.mockResolvedValue("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("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("authenticated"); + + const { result } = renderHook(() => useAcpAuthStatus("gemini-cli"), { + wrapper, + }); + + 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 () => { + 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/src/api/acp-service/acp-service.api.ts b/src/api/acp-service/acp-service.api.ts new file mode 100644 index 00000000..a7327754 --- /dev/null +++ b/src/api/acp-service/acp-service.api.ts @@ -0,0 +1,110 @@ +import { BashClient } from "@openhands/typescript-client/clients"; +import type { BashOutput } from "@openhands/typescript-client"; +import { getAgentServerClientOptions } from "../agent-server-client-options"; + +export type AcpAuthStatus = "authenticated" | "unauthenticated" | "unknown"; + +// Hard cap (seconds) for a single detection command. These are fast local +// status checks (no npx download), so 10s is ample for a healthy CLI while +// keeping the onboarding spinner short before falling back to "unknown". +const PROBE_TIMEOUT_SECONDS = 10; + +interface AcpAuthProbe { + /** Shell command run on the (local) agent-server host to detect login. */ + command: string; + /** Classify the command's output into an auth status. */ + classify: (out: BashOutput) => AcpAuthStatus; +} + +/** Combined stdout+stderr — CLIs are inconsistent about which stream they use + * (e.g. ``codex login status`` writes to stderr). */ +function streams(out: BashOutput): string { + return `${out.stdout ?? ""}\n${out.stderr ?? ""}`; +} + +// Claude Code: ``claude auth status --json`` prints {"loggedIn": bool, …}. The +// CLI exits non-zero when logged out, so we read the JSON, not the exit code. +// No parseable ``loggedIn`` (e.g. the CLI isn't installed → "command not +// found", empty stdout) ⇒ unknown, so onboarding shows the API-key fields +// rather than guessing. +function classifyClaude(out: BashOutput): AcpAuthStatus { + let parsed: unknown; + try { + parsed = JSON.parse((out.stdout ?? "").trim()); + } catch { + return "unknown"; + } + const loggedIn = (parsed as { loggedIn?: unknown } | null)?.loggedIn; + if (typeof loggedIn === "boolean") { + return loggedIn ? "authenticated" : "unauthenticated"; + } + return "unknown"; +} + +// Codex: ``codex login status`` prints "Logged in using …" / "Not logged in" +// (to stderr), so check both streams. Match "not logged in" first since it +// contains the "logged in" substring. Neither phrase (e.g. CLI missing) ⇒ +// unknown. +function classifyCodex(out: BashOutput): AcpAuthStatus { + const text = streams(out).toLowerCase(); + if (text.includes("not logged in")) return "unauthenticated"; + if (text.includes("logged in")) return "authenticated"; + return "unknown"; +} + +// Gemini CLI signs in via Google OAuth and has no status command, so we check +// its credentials file. The command echoes present/absent and exits 0 either +// way; anything else (a shell failure) ⇒ unknown. +function classifyGemini(out: BashOutput): AcpAuthStatus { + // The command echoes exactly `present` / `absent` to stdout, so match trimmed + // stdout exactly. Reading only stdout (not stderr) means a stray shell + // warning can't turn a real result into `unknown`. + const text = (out.stdout ?? "").trim(); + if (text === "present") return "authenticated"; + if (text === "absent") return "unauthenticated"; + return "unknown"; +} + +// Per-provider login detection, keyed by ``acp_server`` / OnboardingAgentId. +// Providers absent here (OpenHands, custom, unknown) report ``unknown``. +const ACP_AUTH_PROBES: Record = { + "claude-code": { + command: "claude auth status --json", + classify: classifyClaude, + }, + codex: { + command: "codex login status", + classify: classifyCodex, + }, + "gemini-cli": { + command: + 'test -f "$HOME/.gemini/oauth_creds.json" && echo present || echo absent', + classify: classifyGemini, + }, +}; + +/** + * Detects whether the selected ACP provider is already logged in — entirely + * client-side, with **no dedicated agent-server endpoint**. It runs the + * provider's own status command (or, for Gemini, a credentials-file check) + * through the existing agent-server bash endpoint and classifies the output. + * + * Gated by the caller to **local backends**: the command runs wherever the + * agent-server runs, and on a user's own machine the provider CLIs and + * credential files live at their standard paths. No prompt is sent, so no model + * tokens are spent. A provider that can't be classified — CLI not installed, + * unexpected output, or the bash call failing — comes back as ``unknown`` so + * onboarding falls back to the API-key fields rather than a misleading banner. + */ +class AcpService { + static async getAuthStatus(server: string): Promise { + const probe = ACP_AUTH_PROBES[server]; + if (!probe) return "unknown"; + const out = await new BashClient( + getAgentServerClientOptions(), + ).executeCommand(probe.command, undefined, PROBE_TIMEOUT_SECONDS); + return probe.classify(out); + } +} + +export default AcpService; diff --git a/src/components/features/onboarding/onboarding-modal.tsx b/src/components/features/onboarding/onboarding-modal.tsx index f93d9aae..79c94bbc 100644 --- a/src/components/features/onboarding/onboarding-modal.tsx +++ b/src/components/features/onboarding/onboarding-modal.tsx @@ -16,10 +16,15 @@ 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; +// 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; @@ -88,50 +93,20 @@ export function OnboardingModal({ onClose }: OnboardingModalProps) { const [selectedAgentId, setSelectedAgentId] = React.useState("openhands"); - // Slide index 2 is the "provider credentials" slot. Its content depends on - // the chosen agent: - // * 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. + // Slide index 2 is the "provider credentials" slot: + // * OpenHands → the LLM-setup form (its own LLM config). + // * Any ACP provider (Claude Code / Codex / Gemini) → the ACP credentials + // form: API key + optional base URL, with a login-detection banner. 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 (
@@ -175,16 +150,17 @@ 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 2b9aabc5..60a7d745 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, @@ -22,29 +24,39 @@ interface SetupAcpSecretsStepProps { /** ACP provider whose credentials we're collecting (e.g. ``"claude-code"``). * Typed as {@link OnboardingAgentId} — the same type the onboarding modal * tracks — so a mistyped key is a compile error rather than a silently empty - * form. Providers without a credentials entry (``"openhands"``, - * ``"gemini-cli"``) simply yield no fields. */ + * form. Providers without a credentials entry (``"openhands"``) 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; } /** - * Onboarding credentials step for ACP providers that authenticate via an - * env-var API key (Claude Code, Codex). The fields are derived from + * Onboarding credentials step for ACP providers that expose an env-var API key + * (Claude Code, Codex, Gemini CLI). The fields are derived from * {@link getAcpProviderSecrets}; each one maps 1:1 to a **global secret** * whose name equals the env var the agent-server exports into the provider * subprocess. Saving here is therefore the same as adding the secret under * Settings → Secrets — it shows up there afterwards. * - * The step is intentionally skippable: a user may authenticate Claude Code via - * a subscription login, or already have the env var set on the backend, so we - * never block "Next" on a value. Empty fields are simply not written; a field - * whose secret already exists shows an "already saved" placeholder and is left - * untouched unless the user types a replacement. + * The step is intentionally skippable: a user may authenticate via a + * subscription / OAuth login (a Claude login, or Gemini's Google login), or + * already have the env var set on the backend, so we never block "Next" on a + * value — and the login probe shows a "you're already signed in" banner when it + * detects one. Empty fields are simply not written; a field whose secret + * already exists shows an "already saved" placeholder and is left untouched + * unless the user types a replacement. */ export function SetupAcpSecretsStep({ providerKey, + isActive, onBack, onNext, }: SetupAcpSecretsStepProps) { @@ -52,6 +64,12 @@ export function SetupAcpSecretsStep({ const queryClient = useQueryClient(); const { mutateAsync: createSecret } = useCreateSecret(); const { data: existingSecrets } = useSearchSecrets(); + // Login detection via AcpService (provider status commands run through the + // agent-server bash endpoint) — see issue #964. + const { status: authStatus, isChecking: isCheckingAuth } = useAcpAuthStatus( + providerKey, + { enabled: isActive }, + ); const fields = React.useMemo( () => getAcpProviderSecrets(providerKey), @@ -115,8 +133,46 @@ export function SetupAcpSecretsStep({ provider: providerName, })}

+ {authStatus !== "authenticated" && ( + // When already signed in, the success banner below already says to + // leave the fields blank, so this general reminder would be redundant. +

+ {t(I18nKey.ONBOARDING$ACP_SECRETS_SUBSCRIPTION_NOTE)} +

+ )} + {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/constants/acp-providers.ts b/src/constants/acp-providers.ts index cc21df58..5b37947e 100644 --- a/src/constants/acp-providers.ts +++ b/src/constants/acp-providers.ts @@ -190,50 +190,45 @@ export interface ACPProviderSecretField { hint_key: I18nKey; } -// Credentials Canvas prompts for during onboarding, keyed by ACP registry key. -// Only providers that authenticate through an env-var API key appear here: -// Claude Code (Anthropic) and Codex (OpenAI). Gemini CLI authenticates via an -// interactive OAuth login rather than a static key, so it has no entry and its -// onboarding credentials step is skipped. Every field is optional (the step is -// skippable): the API keys render masked, the base-URL entries are plain-text -// overrides for proxies/gateways. A provider with no entry simply shows no -// credentials step. -const ACP_PROVIDER_SECRETS: Record = { - "claude-code": [ - { - name: "ANTHROPIC_API_KEY", - secret: true, - hint_key: I18nKey.ONBOARDING$ACP_SECRET_API_KEY_HINT, - }, - { - name: "ANTHROPIC_BASE_URL", - hint_key: I18nKey.ONBOARDING$ACP_SECRET_BASE_URL_HINT, - }, - ], - codex: [ - { - name: "OPENAI_API_KEY", - secret: true, - hint_key: I18nKey.ONBOARDING$ACP_SECRET_API_KEY_HINT, - }, - { - name: "OPENAI_BASE_URL", - hint_key: I18nKey.ONBOARDING$ACP_SECRET_BASE_URL_HINT, - }, - ], -}; - /** * List the credentials Canvas should prompt for when onboarding the given ACP - * provider. Returns ``[]`` for OpenHands, the ``"custom"`` preset, providers - * that don't use a static API key (Gemini CLI), and any unknown key — callers - * treat an empty list as "no credentials step for this provider". + * provider, derived from the SDK registry's ``api_key_env_var`` / + * ``base_url_env_var`` (mirrored via ``@openhands/typescript-client``) rather + * than a hand-maintained per-provider list — so the field names track the SDK + * as providers are added or renamed, with no parallel copy to drift. Each field + * name equals the env var the agent-server exports into the provider subprocess + * (which is what makes a saved secret reach the CLI); the API key renders + * masked, the base URL plain-text. All three built-ins (Claude Code, Codex, + * Gemini CLI) expose an API key + optional base URL. + * + * Every field is optional — the step is skippable — and a subscription / OAuth + * login takes precedence over a key at runtime (most relevant for Gemini, whose + * Google login is the common local path). + * + * Returns ``[]`` for OpenHands, the ``"custom"`` preset, any unknown key, and a + * future OAuth-only provider whose registry entry has no ``api_key_env_var`` — + * callers treat an empty list as "no credentials step for this provider". */ export function getAcpProviderSecrets( key: string | null | undefined, ): ACPProviderSecretField[] { - if (!key) return []; - return ACP_PROVIDER_SECRETS[key] ?? []; + const info = key ? getClientAcpProvider(key) : null; + if (!info) return []; + const fields: ACPProviderSecretField[] = []; + if (info.api_key_env_var) { + fields.push({ + name: info.api_key_env_var, + secret: true, + hint_key: I18nKey.ONBOARDING$ACP_SECRET_API_KEY_HINT, + }); + } + if (info.base_url_env_var) { + fields.push({ + name: info.base_url_env_var, + hint_key: I18nKey.ONBOARDING$ACP_SECRET_BASE_URL_HINT, + }); + } + return fields; } /** 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..7e7ed1bc --- /dev/null +++ b/src/hooks/query/use-acp-auth-status.ts @@ -0,0 +1,98 @@ +import { useQuery } from "@tanstack/react-query"; +import AcpService, { + type AcpAuthStatus, +} from "#/api/acp-service/acp-service.api"; +import { useActiveBackend } from "#/contexts/active-backend-context"; + +export type { AcpAuthStatus }; + +/** + * Probe whether the selected ACP provider is already authenticated on the + * (local) agent-server — by a subscription login (Claude Pro/Max, ChatGPT, + * Google) or a pre-set API key. + * + * Detection is entirely client-side: {@link AcpService.getAuthStatus} runs the + * provider's own status command (Claude: ``claude auth status``; Codex: + * ``codex login status``; Gemini: a credentials-file check) through the + * agent-server bash endpoint and classifies the output — no dedicated + * endpoint, no prompt, no model tokens. Anything it can't classify (CLI not + * installed, unexpected output, the bash call failing) is ``unknown``. + */ +async function probeAcpAuth(providerKey: string): Promise { + try { + return await AcpService.getAuthStatus(providerKey); + } catch { + // The bash endpoint is unreachable or errored: 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**: the detection command runs wherever the + * agent-server runs, and a provider CLI / credentials file is only reliably + * present on the user's own machine. On a remote/cloud backend they're ~never + * there, so we skip the probe, 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: subscription/OAuth providers (e.g. Gemini) are detectable too, and an + * unknown ``providerKey`` simply classifies as ``"unknown"``. The caller renders + * this hook only for ACP providers, so any local backend is probeable. + * + * The probe runs a subprocess on the agent-server, 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 isSupported = isLocal; + 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, + // ``probeAcpAuth`` always resolves (it catches internally → "unknown"), so + // this is redundant today — kept as a guard so the probe still never retries + // if that inner catch is ever removed. + 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 backends only). */ + isSupported, + }; +} diff --git a/src/i18n/translation.json b/src/i18n/translation.json index 05701c38..81f7e37e 100644 --- a/src/i18n/translation.json +++ b/src/i18n/translation.json @@ -1,4 +1,55 @@ { + "ONBOARDING$ACP_SECRETS_SUBSCRIPTION_NOTE": { + "ar": "Already signed in with a subscription or login? Leave these blank — no API key needed.", + "ca": "Already signed in with a subscription or login? Leave these blank — no API key needed.", + "de": "Already signed in with a subscription or login? Leave these blank — no API key needed.", + "en": "Already signed in with a subscription or login? Leave these blank — no API key needed.", + "es": "Already signed in with a subscription or login? Leave these blank — no API key needed.", + "fr": "Already signed in with a subscription or login? Leave these blank — no API key needed.", + "it": "Already signed in with a subscription or login? Leave these blank — no API key needed.", + "ja": "Already signed in with a subscription or login? Leave these blank — no API key needed.", + "ko-KR": "Already signed in with a subscription or login? Leave these blank — no API key needed.", + "no": "Already signed in with a subscription or login? Leave these blank — no API key needed.", + "pt": "Already signed in with a subscription or login? Leave these blank — no API key needed.", + "tr": "Already signed in with a subscription or login? Leave these blank — no API key needed.", + "uk": "Already signed in with a subscription or login? Leave these blank — no API key needed.", + "zh-CN": "Already signed in with a subscription or login? Leave these blank — no API key needed.", + "zh-TW": "Already signed in with a subscription or login? Leave these blank — no API key needed." + }, + "ONBOARDING$ACP_AUTH_DETECTED": { + "ar": "You're already signed in to {{provider}} — you can leave the fields below blank.", + "ca": "You're already signed in to {{provider}} — you can leave the fields below blank.", + "de": "You're already signed in to {{provider}} — you can leave the fields below blank.", + "en": "You're already signed in to {{provider}} — you can leave the fields below blank.", + "es": "You're already signed in to {{provider}} — you can leave the fields below blank.", + "fr": "You're already signed in to {{provider}} — you can leave the fields below blank.", + "it": "You're already signed in to {{provider}} — you can leave the fields below blank.", + "ja": "You're already signed in to {{provider}} — you can leave the fields below blank.", + "ko-KR": "You're already signed in to {{provider}} — you can leave the fields below blank.", + "no": "You're already signed in to {{provider}} — you can leave the fields below blank.", + "pt": "You're already signed in to {{provider}} — you can leave the fields below blank.", + "tr": "You're already signed in to {{provider}} — you can leave the fields below blank.", + "uk": "You're already signed in to {{provider}} — you can leave the fields below blank.", + "zh-CN": "You're already signed in to {{provider}} — you can leave the fields below blank.", + "zh-TW": "You're already signed in to {{provider}} — you can leave the fields below blank." + }, + "ONBOARDING$ACP_AUTH_CHECKING": { + "ar": "Checking for an existing {{provider}} login…", + "ca": "Checking for an existing {{provider}} login…", + "de": "Checking for an existing {{provider}} login…", + "en": "Checking for an existing {{provider}} login…", + "es": "Checking for an existing {{provider}} login…", + "fr": "Checking for an existing {{provider}} login…", + "it": "Checking for an existing {{provider}} login…", + "ja": "Checking for an existing {{provider}} login…", + "ko-KR": "Checking for an existing {{provider}} login…", + "no": "Checking for an existing {{provider}} login…", + "pt": "Checking for an existing {{provider}} login…", + "tr": "Checking for an existing {{provider}} login…", + "uk": "Checking for an existing {{provider}} login…", + "zh-CN": "Checking for an existing {{provider}} login…", + "zh-TW": "Checking for an existing {{provider}} login…" + }, "MAINTENANCE$SCHEDULED_MESSAGE": { "en": "Scheduled maintenance will begin at {{time}}", "ja": "予定されたメンテナンスは{{time}}に開始されます",