feat(onboarding): detect ACP login client-side via the bash endpoint (no SDK changes)#1002
feat(onboarding): detect ACP login client-side via the bash endpoint (no SDK changes)#1002simonrosenberg wants to merge 5 commits into
Conversation
Auto-detect whether the chosen ACP provider (Claude Code / Codex / Gemini) is already signed in and show a "✓ you're already signed in" banner instead of unconditionally asking for an API key. Refs #964. This is a 100% canvas implementation — no SDK endpoint, no SDK release, no version pin. Detection is gated to local backends (where the provider CLIs and credential files actually live) and runs the provider's own status command through the existing agent-server bash endpoint, classifying the output: - Claude Code → `claude auth status --json` (read `loggedIn`; exits non-zero when logged out, so we read the JSON, not the exit code). - Codex → `codex login status` ("Logged in …" / "Not logged in" — on stderr, so both streams are checked). - Gemini → has no status command (browser OAuth), so check its credentials file `~/.gemini/oauth_creds.json`. Anything that can't be classified — CLI not installed ("command not found"), unexpected output, or the bash call failing — is reported as `unknown`, so onboarding falls back to the API-key fields rather than a false banner. Verified live end-to-end through the Vite proxy → bash endpoint: all three authenticated on a logged-in machine; a CLI stripped from PATH (exit 127) → `unknown`. - `AcpService.getAuthStatus(server)` (`BashClient`-based) returns the classified status; `useAcpAuthStatus` is gated to local backends and no longer keys off whether the provider has API-key fields, so Gemini is detected too. - The credentials step renders a login-status screen (banner, no inputs) for key-less providers like Gemini; the onboarding flow no longer skips slide 2. - New i18n: ACP_AUTH_DETECTED, ACP_AUTH_CHECKING, ACP_LOGIN_TITLE, ACP_LOGIN_SUBTITLE, ACP_AUTH_DETECTED_NO_KEY. Tests: `acp-service.api.test.ts` covers each provider's classifier incl. the missing-CLI → `unknown` path (the "no available ACP process" case); hook/step/ modal tests cover gating, the Gemini login-only screen, and no-skip routing. Supersedes the protocol-probe approach (software-agent-sdk#3452 / typescript-client#196 / the #971 wiring), which can be closed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
✅ Mock-LLM Docker E2E Test Results12/12 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
all-hands-bot
left a comment
There was a problem hiding this comment.
Clean, well-scoped implementation. The detection logic is correctly fail-safe — any error, missing CLI, or unrecognised output falls back to unknown and shows the API-key fields rather than a misleading banner. Removing the slide-skip logic simplifies the modal significantly and the layered test coverage is excellent.
Three things worth addressing before merge, plus one note:
- Test file placement —
src/api/acp-service/acp-service.api.test.tslives insidesrc/while every other test file in this PR and the project lives under__tests__/. Should be__tests__/api/acp-service/acp-service.api.test.ts(see inline). retry: falseis dead code —probeAcpAuthcatches all exceptions internally and always resolves, so React Query will never see a rejection to retry. Harmless today, but it could mislead a future author who removes the innertry/catchexpecting this option to protect them (see inline).classifyGeminisubstring match — The command controls the output precisely so no false positive is realistically possible, but comparing the trimmed value exactly would be more defensive (see inline).- i18n note — all 14 non-English locales duplicate the English strings. Normal for a new feature, but worth a follow-up issue so translated users aren't silently shown English.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
❌ Mock-LLM Docker E2E Test Results11/12 passed · 1 failed Commit:
🔍 Failure details (1)❌ mock-llm-conversation.spec.ts › mock-LLM agent-server conversation › step 4: resume conversation from sidebar after navigating awayPosted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
- Move acp-service.api.test.ts under __tests__/ for consistency with the project's test layout. - classifyGemini: exact (trimmed) match on present/absent instead of substring, so stray output falls through to "unknown". - Document why `retry: false` co-exists with probeAcpAuth's internal catch.
|
Addressed all three review threads in |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
There was a problem hiding this comment.
All three issues from the previous review have been addressed in 7a4544d — the test file is in the right place, retry: false has a clarifying comment, and classifyGemini now uses exact-string comparison. The code is clean, the fail-safe unknown fallback is applied consistently end-to-end, and removing the skip-slide logic is a genuine simplification.
One minor comment below on a stale code comment; everything else is ready to merge.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
✅ Mock-LLM Docker E2E Test Results12/12 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
The endpoint design was replaced by bash-command detection via AcpService; update the comment in setup-acp-secrets-step to match.
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
There was a problem hiding this comment.
Clean implementation that achieves the stated goal — detecting ACP provider login client-side via bash commands, with a robust fail-safe unknown fallback throughout. All issues from the previous two review rounds have been addressed. The removal of the Gemini slide-skip logic is a genuine simplification, and the three-layer test pyramid (service → hook → component) gives solid confidence. Two small suggestions below.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
❌ Mock-LLM Docker E2E Test Results11/12 passed · 1 failed Commit:
🔍 Failure details (1)❌ mock-llm-conversation.spec.ts › mock-LLM agent-server conversation › step 4: resume conversation from sidebar after navigating awayPosted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
- classifyGemini reads only stdout (the echo writes there); avoids a stray stderr line turning a real result into unknown. - Shorten probe timeout 30s -> 10s; the detection commands are fast local checks, so a long onboarding spinner isn't warranted.
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
There was a problem hiding this comment.
All feedback from the three previous review rounds has been addressed — the test file is in the right place, classifyGemini reads only stdout with exact-string matching, retry: false has its explanatory comment, and the stale GET /api/acp/auth-status mention is gone. The implementation is clean and the fail-safe unknown fallback is applied consistently end-to-end.
One minor gap noted below on the translation strings. Everything else is ready to merge.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
✅ Mock-LLM Docker E2E Test Results12/12 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
📸 Snapshot Test Report✅ All snapshots match the main branch baselines.
✅ Unchanged snapshots (73)
Generated by the Snapshot Tests workflow. This comment was created by an AI agent (OpenHands) on behalf of the repo maintainers. |
Summary
Auto-detect whether the chosen ACP provider (Claude Code / Codex / Gemini) is already signed in during onboarding, and show a "✓ you're already signed in" banner instead of unconditionally asking for an API key. Refs #964.
100% canvas — no SDK endpoint, no SDK release, no version pin. Detection is gated to local backends (where the provider CLIs and credential files live) and runs each provider's own status command through the existing agent-server bash endpoint, classifying the output.
How each provider is detected
BashClient.executeCommand)claude auth status --jsonloggedIn(exits non-zero when logged out, so we read the JSON, not the exit code)codex login statustest -f ~/.gemini/oauth_creds.jsonAnything that can't be classified — CLI not installed ("command not found"), unexpected output, or the bash call failing — is reported as
unknown, so onboarding falls back to the API-key fields rather than a false banner.Why this instead of a server-side probe
The earlier approach added an
/acp/auth-statusendpoint to the SDK (an ACPsession/newhandshake) + a typed client + a SHA pin. But the banner is local-only, so the protocol approach's headline benefit (topology-agnostic, works in cloud/docker) is unused — we only ever probe the user's own machine, where the CLIs and creds reliably exist. This collapses the whole feature to one canvas PR against the released client.Honest trade-offs: Gemini is a credentials-file check (existence-based), Codex parses text output, and detection runs shell via the bash endpoint. All reliable locally; see discussion in #964.
Changes
AcpService.getAuthStatus(server)(BashClient-based) → classifiedAcpAuthStatus.useAcpAuthStatusgated to local backends; no longer keys off API-key fields, so Gemini is detected too.Tests / validation
acp-service.api.test.ts: each provider's classifier incl. missing-CLI →unknown(the "no available ACP process" path); hook/step/modal tests cover gating, the Gemini login-only screen, and no-skip routing. typecheck + eslint + 40 tests green.authenticatedon a logged-in machine; a CLI stripped fromPATH(exit 127) →unknown.Supersedes
The protocol-probe approach — software-agent-sdk#3452, typescript-client#196, and the #971 wiring — can be closed in favor of this.
🤖 Generated with Claude Code
🐳 Docker images for this PR
• GHCR package: https://github.com/OpenHands/agent-canvas/pkgs/container/agent-canvas
ghcr.io/openhands/agent-canvasghcr.io/openhands/agent-server:1.24.0-pythonopenhands-automation==1.0.0a5be1eeb008cd48c0976e6bd1cc678cb9aa4c07465Pull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-be1eeb0Run
All tags pushed for this build
About Multi-Architecture Support
sha-be1eeb0) is a multi-arch manifest supporting both amd64 and arm64sha-be1eeb0-amd64) are also available if needed