feat(onboarding): collect Gemini ACP credentials + ACP agents docs

[simonrosenberg]

What

Two related changes for ACP agents:

1. Docs — a new docs/ACP_AGENTS.md guide explaining what ACP agents are, how to onboard them, how authentication works (subscription login vs API key, and the local auto-detection precedence), and how to switch agent/model later. Linked from the docs index.
2. Feature — onboarding now collects GEMINI_API_KEY + GEMINI_BASE_URL for Gemini CLI, the same way it already does for Claude Code and Codex.

Why the Gemini change

Gemini CLI was the only built-in ACP provider whose onboarding skipped the credentials step, on the premise that it "authenticates via OAuth, not an env-var key." But the SDK has supported gemini-api-key auth since software-agent-sdk #2619 and GEMINI_BASE_URL routing since the registry centralization (#3022) — so the omission left no in-UI way to give a headless/cloud Gemini backend a key (you'd have to hand-create the secret). This closes that gap and brings Gemini to parity.

• ACP_PROVIDER_SECRETS gains a gemini-cli entry (GEMINI_API_KEY masked + optional GEMINI_BASE_URL), reusing the existing generic hint keys.
• Fields flow through the same global-secret path as the other two, so they reach the subprocess env the SDK already reads. No SDK or typescript-client change needed.
• Gemini's Google OAuth login still takes precedence at runtime; the fields are optional/skippable.
• Honest caveat documented: GEMINI_BASE_URL only routes on the API-key path (it's passed as the ACP gateway endpoint), not under the OAuth login.

Docs simplification

Since Gemini is no longer special in onboarding, the guide drops the "Gemini skips the credentials step" framing — the providers table and onboarding steps now treat all three uniformly, while the auth-precedence section (subscription preferred, key fallback) and the two genuine Gemini specifics (Google login is the common no-key local path; base-URL gateway caveat) are retained.

Tests

• Converted the Gemini "skips slide 2" modal test into a "renders the credentials step (no longer skipped)" regression test.
• Added a Gemini field-render case to setup-acp-secrets-step.test.tsx.
• react-router typegen && tsc, eslint, prettier --check, and the onboarding + constants suites (37 tests) pass locally; pre-commit (typecheck:staged + lint-staged) green.

Notes

• This supersedes, for Gemini, the stopgap in feat(onboarding): collect ACP provider API keys and store as secrets #967; the broader provider-driven Authentication panel (feat(settings/agent): provider-driven Authentication section for ACP agents #872) remains the longer-term home for ACP auth.

🤖 Generated with Claude Code

---

🐳 Docker images for this PR

• GHCR package: https://github.com/OpenHands/agent-canvas/pkgs/container/agent-canvas

Component	Value
Image	ghcr.io/openhands/agent-canvas
Architectures	amd64, arm64
Agent Server	ghcr.io/openhands/agent-server:1.24.0-python
Automation	openhands-automation==1.0.0a5
Commit	438bdcc708d2263f9cade37708fe8ae5c60358c4

Pull (multi-arch manifest)

# Multi-arch manifest — Docker automatically pulls the correct architecture
docker pull ghcr.io/openhands/agent-canvas:sha-438bdcc

Run

docker run -it --rm \
  -p 8000:8000 \
  ghcr.io/openhands/agent-canvas:sha-438bdcc

All tags pushed for this build

ghcr.io/openhands/agent-canvas:sha-438bdcc-amd64
ghcr.io/openhands/agent-canvas:docs-acp-agents-amd64
ghcr.io/openhands/agent-canvas:pr-972-amd64
ghcr.io/openhands/agent-canvas:sha-438bdcc-arm64
ghcr.io/openhands/agent-canvas:docs-acp-agents-arm64
ghcr.io/openhands/agent-canvas:pr-972-arm64
ghcr.io/openhands/agent-canvas:sha-438bdcc
ghcr.io/openhands/agent-canvas:docs-acp-agents
ghcr.io/openhands/agent-canvas:pr-972

About Multi-Architecture Support

• Each tag (e.g., sha-438bdcc) is a multi-arch manifest supporting both amd64 and arm64
• Docker automatically pulls the correct architecture for your platform
• Individual architecture tags (e.g., sha-438bdcc-amd64) are also available if needed

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
agent-canvas	Ready	Preview, Comment	Jun 1, 2026 1:12pm

[all-hands-bot]

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

Solid documentation addition — the structure is clear, the Mermaid diagram effectively communicates the delegation model, and the callouts for the @zed-industries/codex-acp vs @openai/codex acp footgun and the subscription-login-wins precedence rule are exactly the kind of gotcha that users would hit silently. Two small suggestions below on authentication discoverability.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

[all-hands-bot]

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

Review: feat(onboarding): collect Gemini ACP credentials + ACP agents docs

Summary: Clean, minimal, and well-motivated. This closes a real gap — Gemini CLI was the only built-in ACP provider that skipped credentials in onboarding, leaving no in-UI path to supply a key in headless/cloud deployments. The implementation change is a single new entry in ACP_PROVIDER_SECRETS; the existing generic logic in onboarding-modal.tsx propagates the rest automatically without any modal code changes.

---

Strengths

• Minimal blast radius. The only functional change is the new "gemini-cli" block in ACP_PROVIDER_SECRETS. showAcpSecretsStep and skipStep2 were already provider-agnostic; they just work.
• Structural consistency. The Gemini entry mirrors the Codex entry exactly — secret: true on the key, no secret flag on the base URL.
• Test quality. Both updated test files are accurate. The setup-acp-secrets-step addition correctly asserts field types (password for the key, text for the URL). The refactored modal test is cleaner — the removed intermediate waitFor for data-current-step="1" was redundant (it is implied by the subsequent waitFor(onboarding-backend-next)).
• Documentation quality. docs/ACP_AGENTS.md is thorough and honest: the GEMINI_BASE_URL routing caveat (API-key path only, not OAuth), the CLAUDE_CONFIG_DIR auto-detection difference, and the Codex wrapper trap are all called out. The Mermaid diagram gives a good overview.
• Comment updates. All three source comments that referenced the old "Gemini skips credentials" behaviour are updated consistently, including the JSDoc in setup-acp-secrets-step.tsx and the getAcpProviderSecrets description.

---

Minor observations

1. skipStep2 dead-code comment — the updated comment reads "No built-in provider hits this today — all three collect a key". True right now, but it could confuse a future contributor who adds a provider without credentials and doesn't spot the mismatch. The phrase "kept for future OAuth-only providers" already appears later in the same comment and conveys the intent better on its own. Not a blocker.

2. Generic hint for GEMINI_BASE_URL — see inline comment.

---

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

[all-hands-bot]

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

Review: feat(onboarding): collect Gemini ACP credentials + ACP agents docs

🟢 Good taste — Clean, minimal, and well-motivated. The functional change is a single new entry in ACP_PROVIDER_SECRETS at src/constants/acp-providers.ts; the existing showAcpSecretsStep / skipStep2 logic in the modal was already provider-agnostic and propagates automatically. No modal code changes were needed.

Note: Prior reviews by all-hands-bot already addressed the main suggestions (the CLAUDE_CONFIG_DIR discoverability wording and the GEMINI_BASE_URL generic hint key). Those threads are resolved or explicitly accepted as non-blocking by the author. The observations below are additive and independent of those.

---

Strengths

• Blast radius is minimal. All three comment blocks that referenced the old "Gemini skips credentials" behaviour are updated consistently across onboarding-modal.tsx, setup-acp-secrets-step.tsx, and acp-providers.ts. No stale documentation anywhere.
• Structural parity with Codex. The new "gemini-cli" block mirrors the "codex" block exactly — secret: true on the key, plain text on the base URL — which is the right call given identical semantics.
• Documentation honesty. docs/ACP_AGENTS.md calls out the two real Gemini gotchas (OAuth login takes precedence, GEMINI_BASE_URL is API-key-path-only) rather than glossing over them. The Mermaid diagram gives a clear mental model up front.
• skipStep2 is correctly preserved. Keeping the skip path generic for future OAuth-only providers is the right design; the alternative (hardcoding Gemini as the only skip case) would have been worse.

---

Minor observations

1. Test: progress step state coverage reduced

See inline comment on onboarding-modal.test.tsx line 329.

2. Docs: npx -y without version pinning

See inline comment on docs/ACP_AGENTS.md line 45.

---

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW
  Purely additive: a new constant entry and docs. No existing credential flows, modal logic, or test coverage is removed. The refactored Gemini modal test replaces a test for behaviour that is now intentionally obsolete, so there is no regression risk there.

VERDICT:
✅ Worth merging. Core change is sound and closes a real gap (no in-UI path to supply a Gemini key in headless/cloud deployments). Minor suggestions are non-blocking.

KEY INSIGHT: The design already handled optional providers generically; this PR simply fills in the missing constant entry, which is exactly the right scope for this change.

---

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.
2. Re-request a review — the reviewer reads guidelines from the PR branch, so your changes take effect immediately.

Was this review helpful? React with 👍 or 👎 to give feedback.

[all-hands-bot]

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

Review: feat(onboarding): collect Gemini ACP credentials + ACP agents docs

Context: Prior all-hands-bot reviews (10:07, 10:27, 11:56 UTC) covered the main observations — generic GEMINI_BASE_URL hint, CLAUDE_CONFIG_DIR discoverability, npx -y version-pinning awareness, and progress-bar state assertions. The author addressed all of them in commits 101bf7fd and 8e24ff8c. This review evaluates the PR at its current HEAD.

---

Summary

Clean, minimal, well-motivated. Gemini CLI was the only built-in ACP provider that skipped credentials in onboarding — leaving no in-UI path to supply an API key for headless/cloud backends. The fix is a single constant entry; the existing provider-agnostic modal logic picks it up automatically. No modal, routing, or SDK changes were needed.

---

What's in the current HEAD

Functional change (src/constants/acp-providers.ts): New "gemini-cli" entry in ACP_PROVIDER_SECRETS with GEMINI_API_KEY (masked) and GEMINI_BASE_URL (plain text), mirroring the Codex entry structurally. The getAcpProviderSecrets JSDoc is updated to remove Gemini from the "returns empty" list. All comment blocks referencing the old "Gemini skips credentials" behaviour are updated consistently across the three source files.

Tests: The Gemini modal test is converted from asserting a skip-to-step-3 path to asserting slide-2 active + both credential fields rendered + 4 progress segments + slide 2 data-state=current. This is stronger coverage than the test it replaces. The setup-acp-secrets-step addition confirms field types (password/text). Both changes are accurate.

Docs (docs/ACP_AGENTS.md): Comprehensive and honest — the two real Gemini gotchas (GEMINI_BASE_URL routes on API-key path only; Google OAuth login auto-detected and preferred) are prominently documented. The CLAUDE_CONFIG_DIR auto-detection difference is correctly flagged as the exception. The npx -y behaviour is noted with a pinning override example. The Mermaid diagram gives a clear mental model.

---

Remaining observations (non-blocking)

1. GEMINI_BASE_URL hint text still generic
The hint_key reuses ONBOARDING$ACP_SECRET_BASE_URL_HINT (same as Claude Code / Codex). If that string says something like "optional base URL override" it is accurate for Gemini too, but it does not surface the OAuth-vs-API-key caveat in-UI. The caveat is documented in the ACP_PROVIDER_SECRETS block comment and in docs/ACP_AGENTS.md, so this is an acceptable v1 trade-off — the prior thread on this was closed as non-blocking. A follow-up could add a Gemini-specific hint key that surfaces the routing caveat inline.

2. skipStep2 comment minor tension
The comment now says "No built-in provider hits this today — all three collect a key" followed by "kept generic for any future OAuth-only provider". These two statements are logically consistent, but the first sentence could mislead a future contributor into thinking the skip path is dead code. Consider trimming it to just the intent: "Kept for future OAuth-only providers; no built-in provider triggers this today." — not a blocker.

---

Risk

🟢 LOW. Purely additive constant entry + docs. No existing credential flows, modal logic, or routing are removed. The only test that changed was already testing behaviour that is now intentionally obsolete.

---

Verdict

✅ Ready to merge. The functional change is exactly the right scope, all prior feedback has been addressed, and the documentation is among the clearest in the project.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

[simonrosenberg]

Addressed the two non-blocking observations from the latest review in ef039a3f:

1. skipStep2 comment — reworded to lead with intent (generic path kept for a future OAuth-only provider) so it doesn't read as dead code.
2. Generic GEMINI_BASE_URL hint — leaving as-is for this PR per your note that it's an acceptable v1 trade-off (caveat is documented in the ACP_PROVIDER_SECRETS comment + docs/ACP_AGENTS.md). A dedicated Gemini hint key that surfaces the OAuth-vs-API-key routing caveat in-UI is a good follow-up to land with the provider-driven Authentication panel (feat(settings/agent): provider-driven Authentication section for ACP agents #872).

[all-hands-bot]

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

Review: feat(onboarding): collect Gemini ACP credentials + ACP agents docs

Context: This is the fifth review pass. Prior all-hands-bot reviews thoroughly covered the main observations — generic GEMINI_BASE_URL hint, CLAUDE_CONFIG_DIR discoverability, npx -y version-pinning awareness, and progress-bar state assertions. All were addressed by the author in 101bf7fd and 8e24ff8c. This review evaluates the PR at its current HEAD (ef039a3f).

---

Summary

Clean, minimal, and fully addressed. Gemini CLI was the only built-in ACP provider with no onboarding credentials step, leaving headless/cloud users with no in-UI path to supply a key. The fix is a single new entry in ACP_PROVIDER_SECRETS; the existing provider-agnostic modal logic (showAcpSecretsStep, skipStep2) picks it up automatically — no conditional changes needed.

---

Code Review

src/constants/acp-providers.ts — The "gemini-cli" block is structurally identical to "codex": secret: true on the API key, no secret flag on the base URL (plain text). Correct. The block comment has been updated to accurately describe all three providers and clearly notes the GEMINI_BASE_URL routing caveat (API-key path only, ignored under OAuth). The getAcpProviderSecrets JSDoc correctly removes Gemini from the "returns empty" list. ✅

src/components/features/onboarding/onboarding-modal.tsx — Comment-only changes updating the slide-2 documentation. The skipStep2 comment now reads "Kept for future OAuth-only providers; no built-in provider triggers this today", which is appropriately forward-looking without implying dead code. ✅

src/components/features/onboarding/steps/setup-acp-secrets-step.tsx — JSDoc and inline comment updated consistently with the new Gemini behavior. ✅

__tests__/components/onboarding/onboarding-modal.test.tsx — The refactored test is stronger than what it replaces: it now asserts slide-2 is active, both Gemini credential fields are rendered, 4 progress segments are in the DOM, and segment 2 carries data-state=current with segment 1 completed. This accurately captures the intended post-change behavior and is a regression-safe baseline. The removed waitFor for data-current-step="1" was redundant (implied by the subsequent waitFor(onboarding-backend-next) check). ✅

__tests__/components/onboarding/setup-acp-secrets-step.test.tsx — The new Gemini case confirms field input types (password for key, text for base URL), consistent with other provider tests. ✅

docs/ACP_AGENTS.md — Comprehensive and honest. The two Gemini gotchas (OAuth login preferred over API key; GEMINI_BASE_URL only routes on API-key path) are prominently documented. The CLAUDE_CONFIG_DIR non-auto-detection exception is correctly flagged as the outlier. The npx -y behavior is noted with a pinning override example. The Mermaid diagram and troubleshooting section add real value. ✅

---

Remaining (pre-existing, accepted as non-blocking)

• GEMINI_BASE_URL hint still reuses the generic ONBOARDING$ACP_SECRET_BASE_URL_HINT — accepted by the author as a follow-up; the routing caveat is covered in the block comment and docs.

---

Risk

🟢 LOW. Purely additive: one new constant entry + one new docs file. No modal routing, credential flows, or existing tests removed. The refactored Gemini modal test replaces an assertion for intentionally obsolete behavior.

---

Verdict

✅ Ready to merge. Functional change is exactly the right scope, all prior feedback has been addressed across multiple commits, and the documentation is among the clearest in the project.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

[simonrosenberg]

Added a concise subscription-auth note to the onboarding Add your API keys step in b1c1af4b (per local UI review): "Already signed in with a subscription or login? Leave these blank — no API key needed." New ONBOARDING$ACP_SECRETS_SUBSCRIPTION_NOTE key across all 15 locales. Verified live in the running app (screenshot attached locally).

[all-hands-bot]

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

Review: feat(onboarding): collect Gemini ACP credentials + ACP agents docs

Context: Previous all-hands-bot reviews (×5) thoroughly covered the main observations — generic GEMINI_BASE_URL hint text, CLAUDE_CONFIG_DIR discoverability, npx -y version-pinning awareness, and progress-bar state assertions. All were addressed in 101bf7fd, 8e24ff8c, and ef039a3f. This review evaluates the one new commit at the current HEAD (b1c1af4b) and confirms the full PR is clean.

---

New in b1c1af4b

src/components/features/onboarding/steps/setup-acp-secrets-step.tsx / src/i18n/translation.json — Adds a subtitle line: "Already signed in with a subscription or login? Leave these blank — no API key needed." This is a well-targeted UX improvement. The per-field hint_key only surfaces in each field's placeholder/tooltip; a step-level note makes the skip-intent visible before a user even focuses a field. The new ONBOARDING$ACP_SECRETS_SUBSCRIPTION_NOTE key ships with 15 locale translations (matching the project's locale coverage) and uses zero dynamic interpolation, so there's no risk of a missing-variable runtime error.

The text-[var(--oh-muted)] class is consistent with how other de-emphasized help text is styled in the onboarding flow.

---

Full-PR assessment (current HEAD)

🟢 Good taste — Clean, minimal, and fully addressed. The functional change is a single new entry in ACP_PROVIDER_SECRETS; the modal's existing showAcpSecretsStep / skipStep2 logic picks it up automatically with no conditional changes. The test suite was correctly updated to replace assertions for now-obsolete behavior with positive assertions confirming the new credential fields and 4-segment progress bar. The docs are among the clearest in the project.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW
  Purely additive: one new constant entry, one new docs file, one UX hint, and updated tests. No routing logic or credential flow changed. No existing tests removed; the refactored Gemini modal test replaces an assertion for intentionally obsolete behavior.

VERDICT:
✅ Worth merging — All prior feedback has been addressed across multiple commits. The latest commit adds a clear, well-localised UX hint with no risk.

KEY INSIGHT: Making all three built-in ACP providers structurally uniform (same constant shape, same onboarding path) is the right move — it eliminates the special-case skipStep2 branch for Gemini and makes the modal logic a simple acpSecretFields.length > 0 gate.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

[all-hands-bot]

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

Review: feat(onboarding): collect Gemini ACP credentials + ACP agents docs

Context: This review targets the current HEAD (5dabfe4b) — the most recent commit — and gives an overall PR assessment. Prior all-hands-bot reviews (×6) covered CLAUDE_CONFIG_DIR discoverability, GEMINI_BASE_URL hint caveat, npx -y version-pinning, and progress-bar assertions; all were addressed in 101bf7fd, 8e24ff8c, ef039a3f, and b1c1af4b.

---

New in 5dabfe4b — Registry-derived credential fields

This commit replaces the hand-maintained ACP_PROVIDER_SECRETS map with a dynamic derivation from getClientAcpProvider(), pulling api_key_env_var and base_url_env_var directly out of the SDK registry (mirrored by @openhands/typescript-client). It's a clean architectural improvement with two concrete wins:

1. No drift. The env-var names previously lived in two places (the registry and the Canvas constant). A registry rename would leave Canvas silently showing stale field names until someone noticed. With derivation, the TypeScript client propagates any rename at compile time.
2. Future providers are free. Any new provider added to the SDK registry with an api_key_env_var gets a credentials step in Canvas automatically, with no parallel Canvas update required.

The implementation handles edge cases correctly: null / undefined key → []; missing api_key_env_var → no password field; missing base_url_env_var → no text field. The secret: true on API keys is hardcoded and correct — API keys must always be masked regardless of provider.

The trade-off (SDK renames now propagate automatically) is the right one: renaming a provider's env var in the SDK is already a breaking change for anyone depending on the subprocess environment, so Canvas should track it.

---

Full-PR assessment (current HEAD 5dabfe4b)

🟢 Good taste — Each commit in this series is focused and well-motivated. The functional change (Gemini CLI gets GEMINI_API_KEY + GEMINI_BASE_URL in onboarding) is small; the rest is documentation, test corrections, UX polish, and a clean architectural refactor. The modal's existing showAcpSecretsStep / skipStep2 logic required zero conditional changes because the gate was already acpSecretFields.length > 0. Making all three built-in ACP providers structurally uniform was the right call.

Risk: 🟢 LOW. Purely additive at the feature level. The refactor in 5dabfe4b is behavior-preserving (env-var names match the removed literals exactly, confirmed by the tests). No routing logic changed; no secrets handling changed.

VERDICT: ✅ Ready to merge.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

[all-hands-bot]

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

Review: feat(onboarding): collect Gemini ACP credentials + ACP agents docs

Context: This is the eighth review pass on this PR. Prior all-hands-bot reviews (×7) covered the full feature and refactor — GEMINI_BASE_URL caveat, npx -y version-pinning, CLAUDE_CONFIG_DIR discoverability, registry-derived credential fields, and the UX note about subscription / OAuth logins. All prior observations were addressed across the commit series (101bf7fd, 8e24ff8c, ef039a3f, b1c1af4b, 5dabfe4b). The previous review at 5dabfe4b declared this Ready to merge.

This review covers only the new commit 849663e0.

---

849663e0 — docs(acp): correct Claude Code subscription login detection

A targeted documentation correction to docs/ACP_AGENTS.md. The prior text incorrectly claimed:

• Claude Code's login requires CLAUDE_CONFIG_DIR to be set and is not auto-detected.
• A subscription login always wins over an API key (false for Claude Code — it's actually the opposite).

The correction is factually accurate:

• Claude Code stores its OAuth token in the macOS Keychain (or ~/.claude/.credentials.json on Linux) and auto-detects it with no CLAUDE_CONFIG_DIR needed.
• CLAUDE_CONFIG_DIR only relocates the config/session-history directory (settings, session history), not the credential store. Its real use case is containers or multi-account setups.
• Claude Code's actual precedence is the reverse of Codex and Gemini: if ANTHROPIC_API_KEY is present it wins; the OAuth token is the fallback. The corrected text captures this nuance correctly.
• The CLAUDE_CONFIG_DIR + SDK key-stripping behaviour for headless OAuth-forced setups is a genuine (if uncommon) edge case and is correctly retained.

The table reformulation ("Subscription login (auto-detected)" / "API key") and the rewritten callout are clear improvements. No code changes; no test changes needed.

---

Full-PR assessment (current HEAD 849663e0)

🟢 Good taste — Two related, well-scoped changes (Gemini CLI credentials parity + ACP docs) with a clean refactor (registry-derived fields) layered in. Each commit is focused. The functional gap (no in-UI path for a headless Gemini backend to receive a key) is real and the fix is minimal. The latest commit closes a factual inaccuracy in the docs about Claude Code's authentication flow.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW — Additive feature on a previously skipped onboarding slide; registry-derived fields are behavior-preserving (confirmed by tests); latest commit is documentation-only.

VERDICT: ✅ Ready to merge.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

[all-hands-bot]

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

Review: feat(onboarding): collect Gemini ACP credentials + ACP agents docs

Context: All five prior inline review threads are resolved, and prior all-hands-bot reviews covered GEMINI_BASE_URL hint specificity, CLAUDE_CONFIG_DIR discoverability, npx -y version-pinning, and progress-bar state assertions. This is an independent review of the PR at current HEAD (849663e0).

---

Core architectural change — registry-derived credential fields (src/constants/acp-providers.ts)

The replacement of the hardcoded ACP_PROVIDER_SECRETS map with dynamic derivation from getClientAcpProvider() is the standout architectural improvement. The previous design required a parallel Canvas update every time the SDK registry added or renamed a provider — a silent drift risk. The new implementation propagates env-var names at compile time: a registry rename becomes a TypeScript error rather than a stale field name in the UI.

Edge cases are handled correctly:

• null/undefined key → [] (no credentials step)
• api_key_env_var absent → no password field
• base_url_env_var absent → no URL field
• secret: true hardcoded for all API keys — correct, independent of provider

The trade-off (SDK renames now auto-propagate to the form) is the right one: any such rename is already a breaking change for the subprocess environment.

---

Gemini credential parity (src/constants/acp-providers.ts, onboarding-modal.tsx)

The gap is cleanly closed. getAcpProviderSecrets("gemini-cli") now returns [GEMINI_API_KEY (masked), GEMINI_BASE_URL (text)] via registry derivation, requiring zero changes to the modal's showAcpSecretsStep / skipStep2 logic — the existing acpSecretFields.length > 0 check handles it automatically.

The retained skipStep2 path remains correct for any future OAuth-only provider whose registry entry carries no api_key_env_var.

---

UX — subscription note (setup-acp-secrets-step.tsx, translation.json)

The new ONBOARDING$ACP_SECRETS_SUBSCRIPTION_NOTE paragraph is well-placed (after the per-provider description, before the form fields) and appropriately styled (text-[var(--oh-muted)] matches existing help text). It correctly applies to all three built-in providers — Claude Code and Codex users with subscription logins equally benefit from knowing the fields are optional.

The 15-locale coverage matches the project's existing i18n breadth. Zero dynamic interpolation means no runtime variable-missing risk.

Minor nuance (non-blocking, for awareness): Claude Code's auth precedence is inverted from the others (ANTHROPIC_API_KEY wins over the OAuth login if both are present), whereas the note says "Already signed in? Leave these blank." This is fine for the onboarding context — the note advises users not to enter a key, not making claims about precedence when both exist simultaneously. The precedence detail is already accurately captured in docs/ACP_AGENTS.md. No change needed.

---

Documentation (docs/ACP_AGENTS.md)

Strong addition. Highlights:

• Mermaid architecture diagram communicates the Canvas → Server → ACP subprocess boundary effectively
• The explicit @openai/codex acp anti-pattern callout is genuinely useful — this footgun causes silent handshake deadlocks
• The Claude Code credential detection correction (849663e0) is factually accurate: Keychain / ~/.claude/.credentials.json auto-detected with no CLAUDE_CONFIG_DIR needed; CLAUDE_CONFIG_DIR only relocates config/session-history
• The CLAUDE_CONFIG_DIR + SDK key-stripping edge case for forced-OAuth headless setups is correctly retained
• npx -y version-pinning awareness note added to the providers table ✓
• Troubleshooting section covers the main real-world failure modes (deadlocked handshake, auth errors, model not found, per-backend agent choice)

---

Tests

• onboarding-modal.test.tsx: correctly converted from "skips slide 2" to "renders credential step", with positive assertions on onboarding-acp-secret-GEMINI_API_KEY, onboarding-acp-secret-GEMINI_BASE_URL, slide-2 active, and the 4-segment progress-bar state machine (step 2 current, step 1 completed)
• setup-acp-secrets-step.test.tsx: new Gemini case confirms GEMINI_API_KEY renders as type="password" and GEMINI_BASE_URL as type="text" ✓

---

Summary

🟢 Ready to merge. The functional change is minimal and well-motivated: one registry-derived field list replaces a hand-maintained map, closing the Gemini credential gap with no downstream code changes required. All prior feedback has been addressed. Tests guard both the new behavior and the state machine.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW — purely additive (new doc, new i18n key, one new registry-derived lookup). No routing logic, credential flow, or auth path changed.

---

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

[github-actions]

📸 Snapshot Test Report

✅ All snapshots match the main branch baselines.

Category	Count
🔴 Changed	0
🆕 New	0
✅ Unchanged	73
Total	73

✅ Unchanged snapshots (73)

archived-conversation

• conversation-panel-with-archived-badges
• conversation-view-archived
• conversation-view-sandbox-error

automations

• automations-delete-modal
• automations-list-active-inactive
• automations-no-automations
• automations-search-no-results

backends-extended

• backend-add-blank-disabled
• backend-add-cloud-advanced-open
• backend-add-cloud-no-key-disabled
• backend-add-cloud-with-key-enabled
• backend-add-form-partially-filled
• backend-add-invalid-url-disabled
• backend-add-local-ready
• backend-add-name-only-disabled
• backend-add-two-column-layout
• backend-add-whitespace-host-disabled
• backend-after-switch
• backend-cancel-nothing-saved
• backend-dropdown-two-backends
• backend-edit-prefilled
• backend-manage-after-removal
• backend-manage-two-listed
• backend-remove-cancelled
• backend-remove-confirmation
• backend-switch-overlay

backends

• backend-add-modal
• backend-manage-modal
• backend-selector-open

changes-tab

• changes-deleted-file
• changes-diff-viewer
• changes-empty

collapsible-thinking

• reasoning-content-collapsed
• reasoning-content-expanded
• think-action-collapsed
• think-action-expanded

mcp-page

• mcp-custom-server-1-editor-open
• mcp-custom-server-2-url-filled
• mcp-custom-server-3-all-filled
• mcp-custom-server-4-installed
• mcp-custom-server-editor
• mcp-empty-installed
• mcp-search-filtered
• mcp-slack-install-1-marketplace
• mcp-slack-install-2-modal
• mcp-slack-install-3-filled
• mcp-slack-install-4-installed

onboarding

• onboarding-step-0-choose-agent
• onboarding-step-1-check-backend
• onboarding-step-2-setup-llm
• onboarding-step-3-say-hello

projects-workspace-browser

• projects-workspace-browser

settings-page

• add-backend-modal
• analytics-consent-modal
• home-screen
• settings-app-page
• settings-page

settings-secrets

• secrets-add-form-filled
• secrets-add-form
• secrets-after-save
• secrets-delete-confirm
• secrets-list

settings-verification

• condenser-settings
• verification-settings-off
• verification-settings-on

sidebar

• sidebar-collapsed
• sidebar-conversation-panel
• sidebar-filter-menu

skills-page

• skills-empty
• skills-loaded
• skills-no-match
• skills-search-filtered
• skills-type-filter

---

Generated by the Snapshot Tests workflow. This comment was created by an AI agent (OpenHands) on behalf of the repo maintainers.