fix(acp): surface a 'credentials configured' banner state on Docker/cloud (#1244)#1430
fix(acp): surface a 'credentials configured' banner state on Docker/cloud (#1244)#1430georgeglarson wants to merge 7 commits into
Conversation
…#1244) Pure decision matrix mapping the host-login probe + a backend-truthful 'credential is stored' signal to a banner display state. Adds a 'configured' state for the gap that previously rendered nothing on Docker/cloud backends, where the interactive-CLI probe can't run. A stored credential never reports as 'signed-in' — only the probe confirms a host login.
…1244) The auth banner now shows a neutral 'credentials configured' state when the host-login probe can't confirm a login but a credential is stored — accurate on Docker/cloud backends where the interactive-CLI probe goes silent. Distinct from the green 'signed in' banner (which still requires a probe-confirmed host login) so it never overstates the auth state. Adds the ONBOARDING$ACP_CREDENTIALS_CONFIGURED i18n key (all locales). Optional prop, defaults false — no change for existing callers.
…penHands#1244) Derive credentialsConfigured in useAcpCredentialForm (a saved 'secret' field exists — API key, OAuth token, or file blob; a base URL or GCP scalar does not count) and pass it to the auth banner in both the onboarding step and Settings -> Agent. On Docker/cloud backends, where the host-login probe can't run, the banner now reflects configured credentials instead of going silent. Both surfaces stay consistent (shared banner + shared form hook).
…backend (OpenHands#1244) Mock-LLM E2E regression for the OpenHands#1244 banner state. Drives Settings → Agent → ACP → Claude Code, saves a placeholder CLAUDE_CODE_OAUTH_TOKEN, and asserts the neutral 'credentials configured' banner appears while the green 'signed in' banner does not (the honesty guard). The placeholder value is never authenticated — the banner reads only whether the secret name exists in the backend store — so no real or PAYG credential is needed. Runs under both mock-llm configs; the saved-secret path exercises the real agent-server secret store on a kind:'local' backend, the case the host-login probe can't classify. Verified green against examples/acp-docker (agent-server 1.29.0-python).
…enHands#1244) Audit found the configured-banner behaviour was only exercised for the Claude provider on a probe-inconclusive *local* backend, while the issue's headline case is Docker AND cloud. Add: - A cloud-backend test that mocks the cloud secrets boundary (fetchCloudSecrets) and runs the REAL SecretsService.getSecrets, so the cloud routing (kind === 'cloud' -> fetchCloudSecrets) is actually exercised — the path the host-login probe can never reach. Asserts the configured banner appears, fetchCloudSecrets was called, and 'signed in' never shows; plus the empty-store negative. - A Codex test proving the signal is provider-generic (not Claude-specific) and that a multiline file-content blob (CODEX_AUTH_JSON) counts as a credential. Also document that the credentialsConfigured 'secret === true' check is a deliberate strict guard (a field must omit 'secret', never set it false). Full suite 3296 passed; lint clean.
|
@georgeglarson is attempting to deploy a commit to the openhands Team on Vercel. A member of the Team first needs to authorize it. |
…e2e cleanup (OpenHands#1244) cli-review-panel (5 lanes, SECURITY scope, 0 blockers) surfaced two convergent P3s worth closing: - Assert isChecking stays false on a cloud backend in the existing use-acp-auth-status cloud test. The Docker/cloud banner state relies on the gated probe settling false so resolveAcpAuthDisplay falls through to the credentials-configured signal instead of spinning; previously only status and isSupported were pinned. - Add a beforeAll idempotent secret delete to the banner e2e so the no-secret baseline holds even if a prior run's best-effort afterAll cleanup did not complete. afterAll stays best-effort, matching the sibling acp-agent spec. Full suite 3296; lint clean; e2e re-verified green.
8246ea1 to
6b677f8
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes ACP auth banner UX on Docker/cloud backends by adding a new neutral “credentials configured” state (driven by whether a provider credential exists in the backend secret store) to avoid silent/ambiguous “unknown” outcomes from the host-CLI login probe.
Changes:
- Add
resolveAcpAuthDisplay()decision logic and wire a new “configured” banner state intoAcpAuthStatusBanner. - Extend
useAcpCredentialForm()with acredentialsConfiguredsignal derived from stored secrets (credential fields only). - Add unit + component + mock-LLM E2E coverage, including a cloud-backend path test.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/mock-llm/settings/mock-llm-acp-auth-banner.spec.ts | New E2E regression test asserting the neutral “configured” banner appears when a credential is stored and no verified host login is detected. |
| src/utils/acp-auth-display.ts | Adds a small display-state resolver for ACP auth banner precedence. |
| src/i18n/translation.json | Adds localized copy for the new “credentials configured” banner message. |
| src/hooks/use-acp-credential-form.ts | Derives and exposes credentialsConfigured from the backend secret store (credential fields only). |
| src/components/features/settings/acp-credentials-section.tsx | Passes credentialsConfigured into the shared banner component. |
| src/components/features/settings/acp-auth-status-banner.tsx | Renders the new neutral “configured” banner state and centralizes state selection via the resolver. |
| src/components/features/onboarding/steps/setup-acp-secrets-step.tsx | Wires credentialsConfigured into the onboarding surface for consistent behavior. |
| tests/utils/acp-auth-display.test.ts | Unit tests for the banner decision matrix. |
| tests/hooks/query/use-acp-auth-status.test.tsx | Adds an assertion that the probe-gated path never enters “checking”, supporting the new fallback behavior. |
| tests/components/settings/acp-credentials-section.test.tsx | Adds coverage for “configured” state and for excluding non-credential stored fields. |
| tests/components/settings/acp-credentials-section-cloud.test.tsx | New test covering the cloud secrets branch as the source of credentialsConfigured. |
| tests/components/settings/acp-auth-status-banner.test.tsx | New rendering tests for all banner states including precedence. |
| tests/components/onboarding/setup-acp-secrets-step.test.tsx | Adds onboarding coverage for the “configured” banner state. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const checkingBanner = page.getByTestId("settings-acp-auth-checking"); | ||
| await checkingBanner | ||
| .waitFor({ state: "hidden", timeout: 10_000 }) | ||
| .catch(() => {}); | ||
| test.skip( | ||
| (await signedInBanner.count()) > 0, | ||
| "host-login probe reported a verified login; the credentials-configured " + | ||
| "state only applies when the probe is inconclusive (Docker/cloud/CI)", | ||
| ); |
There was a problem hiding this comment.
Good catch. waitFor({ state: "hidden" }) does resolve immediately when the element isnt in the DOM, so the spinner-hidden wait could read the skip-precondition before the probe painted.
It cant flake CI, though. This suite runs against the Docker/mock-llm backend, which has no interactive claude CLI, so the probe always classifies as unknown and the detected banner never renders. signedInBanner.count() is 0 either way. The race only bit the skip path on a host with a real Claude login, which is the graceful-skip the docstring promises but didnt reliably hold.
I went a different route than the suggested "wait for checking or detected to appear", since that one can hang on the unknown path where neither ever paints. The probe POSTs its status command to /api/bash/execute_bash_command, so I settle on that response: armed before the preset selection that fires it, then wait for the spinner to clear before reading the precondition. Deterministic regardless of paint timing.
Verified both branches against a live examples/acp-docker agent-server: stock container (no claude CLI) -> probe unknown -> the spec asserts the configured banner and passes; with a stubbed claude auth status --json returning {"loggedIn": true} -> probe authenticated -> the spec skips gracefully. Pushed in d794e1f.
… banner paint (OpenHands#1244) The skip-precondition waited for the "checking" spinner to hide via waitFor({ state: "hidden" }), which Playwright satisfies by "not in the DOM", so it resolved immediately when the spinner hadn't painted yet and read signedInBanner.count() too early. Harmless on the Docker/mock backend this suite runs against (the probe is always inconclusive there, so the detected banner never appears), but on a host with a real Claude login the probe later reports "signed in" and the spec false-failed instead of skipping, the exact graceful-skip the docstring promises. Settle on the probe's network response instead: it POSTs the provider status command to /api/bash/execute_bash_command, armed before the preset selection that fires it, then wait for the spinner to clear before reading the skip-precondition. Deterministic regardless of paint timing.
HUMAN:
100% real human used in testing. Worked even when some commands were entered using fingers.
AGENT:
Verified end-to-end against a real containerized agent-server, not just unit tests. Commands and results below; manual repro in "How to Test."
Automated
npx vitest run-> 3296 passed / 0 failed (adds resolver matrix, banner, hook, both surfaces, cloud-backend, and a non-Claude provider path).npm run lint(typecheck + eslint + prettier) -> clean.tests/e2e/mock-llm/settings/mock-llm-acp-auth-banner.spec.ts) drives the real UI on akind:"local"Docker backend: select ACP -> Claude Code, save a placeholder credential, assert the "configured" banner appears and the green "signed in" banner does not.Behavioral proof (the part unit tests can't give)
examples/acp-dockeragent-server. With no secret the banner is absent; after saving a credential it shows "configured"; the secret round-trips through the real/api/settings/secretsstore.1 passed.settings-acp-auth-configuredassertion (the testid never renders). Restoring the fix makes it pass again.Why
The ACP auth banner ("already signed in to {provider}") detects login by shelling the provider's interactive CLI (
claude auth status,codex login status, a Gemini creds-file check) through the agent-server bash endpoint. That only tells the truth on a native local backend. A Docker agent-server iskind:"local"but ships only the ACP wrappers (claude-agent-acp), not the interactive CLIs, so the probe hits "command not found", classifies asunknown, and the banner renders nothing, even when the conversation is fully authenticated through the materialized credential. On cloud the probe is gated off entirely, with the same silent result.unknownmeans "couldn't tell," but it rendered identically to "nothing configured." So a Docker or cloud user who has correctly supplied a credential sees no acknowledgement at all.Summary
AcpAuthStatusBannerand the shareduseAcpCredentialForm.Issue Number
#1244
How to Test
Automated (no credentials needed):
Manual, on a real containerized backend:
In the UI: Settings -> Agent -> set Agent to ACP, Preset to Claude Code. Under Credentials, type any value into
CLAUDE_CODE_OAUTH_TOKEN(a placeholder is fine; the banner only checks that a credential is stored, it never reads or authenticates the value, so no real or paid key is required) and Save. The neutral blue "Credentials for Claude Code are configured" banner appears, and the green "signed in" banner does not.Video/Screenshots
red-openhands-agentcanvas1244e2eREDnofix.mp4

green-openhands-agentcanvas1244e2e.mp4
Type
Notes
examples/acp-dockercompose pins1.25.0-python, which predates the frontend's current 1.28.0 minimum, so the default image reads "Disconnected". The override above works. Happy to bump the example default in a separate PR if useful; left out here to keep this change focused.