Add one-click browser account connect for OpenAI, Anthropic, and Google#68
Add one-click browser account connect for OpenAI, Anthropic, and Google#68YishayPo wants to merge 1 commit into
Conversation
|
@yaacovcorcos is there a way to test this before merging to master, how do you build an executable application locally? |
yaacovcorcos
left a comment
There was a problem hiding this comment.
Thanks, Yishay. The product direction is valuable: a clean-machine path from install to the provider's supported sign-in and then verified readiness. I am requesting changes because this PR currently combines several independent, high-risk changes, and the review found confirmed upgrade, permission, authentication, and model-option regressions that green CI does not cover.
Please do not continue repairing this as one mega-PR. Split it into as many independently reviewable PRs as needed. A sensible sequence is:
- One-click connection orchestration only, reusing the existing managed installer and each provider's official login implementation.
- A versioned provider-identity migration that preserves the released meaning of persisted
geminivalues and introduces a distinct identity for native Gemini. - Native Gemini ACP adapter core: sessions, permission policy, Plan-mode behavior, model catalog/options, health, and focused tests.
- Managed Gemini runtime distribution only after the artifact trust/provenance problem is resolved; otherwise require an external official install.
- Onboarding separately: true first-install gating, modal arbitration, platform filtering, truthful credential copy, and browser tests.
- Usage reporting separately, and only through a Google-supported API and credential boundary. The current CLI OAuth piggyback must not ship.
- Remaining settings, plugin-library, icon, and profile integration in small bounded PRs where appropriate.
The blocking findings are in the inline comments. Each replacement PR should include released-version upgrade fixtures where persistence changes, negative permission tests, exact model-option encode/decode/dispatch tests, concurrency/cancellation coverage for connection flows, and clean-machine plus upgrade-path evidence. Please include packaged/manual evidence for user-facing provider flows; compile-only browser fixtures are not sufficient.
Once split, I am happy to review the smaller PRs independently. Do not merge this head while any of the blocking findings remains.
3b0bf44 to
968fced
Compare
There was a problem hiding this comment.
Thanks for reducing the branch, Yishay. I re-reviewed the current head 968fcedd. The removed Gemini-specific findings from the earlier review are now outdated; the remaining one-click connection work is valuable and worth finishing.
I am still requesting changes because the current head retains several release-blocking authentication, lifecycle, onboarding, and integration issues. Please use this as the current checklist:
-
Remove the native Codex/OpenAI OAuth implementation. Delete
openaiNativeOAuth.tsand its dedicated flow tests. The one-click sequence should be: install or resolve the managed Codex runtime → invoke the officialcodex loginbrowser flow → verify authentication → require a non-empty model catalog before reporting success. Native OAuth only gains parallel install/authentication and tighter callback-page control. Those modest UX gains do not justify making Scient responsible for OpenAI tokens, Codex's evolvingauth.jsonschema, keyring/file/ephemeral credential-store selection, auth restrictions, proxies/routes, and concurrent-login behavior. The existing implementation demonstrably replaces unrelated seeded credential fields, and the active inline thread also identifies callback-state and premature-success bugs. The user can still have a one-button browser-login experience through the official CLI. -
Make install-to-login chaining operation-owned and failure-safe. Bind the chain to the exact provider installation
operationId, keep it alive until the server accepts the login operation, define cancellation/replacement semantics, and expose terminal failure/retry state even when the dialog is closed. Starting provider B must not overwrite provider A, and stale completion must not trigger or clear another operation. -
Harden or separate onboarding while keeping “Skip for now.” It needs durable first-install versus upgrade provenance, centralized startup-modal arbitration, platform-aware provider filtering, and truthful credential copy. Existing users must not receive first-run onboarding merely because the new local-storage key is absent. Keep the Skip for now action: skipping onboarding and providing permanent ways to connect later are separate product concerns. After a user skips with no provider connected, attempting to send a message must explain that no provider is connected, preserve the draft, and offer a direct Connect a provider action. The provider/model selector must clearly show disconnected or uninstalled providers and provide inline Connect or Set up actions. Settings → Providers must remain permanently available as another connection entry point. Please add component/browser coverage for fresh install, upgrade, already-connected, skipped/completed, contextual send recovery, selector connection, More Providers, and competing startup-dialog states.
-
Remove unrelated changes from this PR. The raw ACP
session/set_modelfallback and settings/composer refactors are not required for one-click connection or onboarding. Move the ACP compatibility behavior into a separate typed, capability-gated, directly tested PR. Keep the low-risk refactors separate as well so this authentication change remains reviewable. -
Rebase onto current
maincarefully. The branch currently conflicts in__root.tsx. Main intentionally removedAppSnapWelcomeDialogat startup; the resolution must preserve that removal and must not reintroduce the startup snapshot notification. -
Complete failure-path and user-journey evidence. Add coverage for installed and uninstalled providers, clean install → official browser login → account/model verification, browser-open failure with manual retry, auth denial/cancellation, install failure, verification failure, concurrent providers, stale events, dialog close/reopen, onboarding upgrade behavior, skipped onboarding followed by a send attempt, selector-driven connection, and preserving an unsent draft through connection cancellation or failure. Include packaged/manual evidence on the supported desktop platforms for the final user-facing flow; green unit tests alone do not prove the browser/CLI integration.
-
Update the PR description. It still says the reduced head adds Gemini CLI, while the current 17-file diff does not. Fill in What Changed/Why, document the remaining scope, and include screenshots plus a short interaction recording for onboarding and one-click connection.
The target outcome is excellent: one click from an unconfigured provider to its official browser sign-in, followed by verified account and model readiness. The changes above preserve that product value while keeping credentials owned by the provider CLI and making failures recoverable and observable. Please do not merge the current head until the active current-head threads and the inline findings below are addressed.
|
@YishayPo Product clarification for onboarding: please keep “Skip for now.” I removed the earlier inline objection to that action and updated the main review. Skipping onboarding and having permanent ways to connect later are separate concerns. The desired behavior after a user skips without connecting a provider is:
The onboarding modal can remain lightweight and dismissible. The rest of the app must still make the next required action obvious at the moment the user tries to use a provider. |
968fced to
12986cf
Compare
|
Rebased onto #103 "Harden provider installation and sign-in" landed while this was in review and re-implemented the install/sign-in chain server-side ( In scope now (+535 / -33, 16 files):
Removed per your review:
CI note: one browser test ( |
12986cf to
8a354eb
Compare
…onnect Layers on top of #103's server-side install/sign-in chain. Fixes the Codex zip extraction hang (entry.buffer + a bounded timeout), surfaces the provider CLI's real error on failed sign-in, shows download progress, and adds a per-provider disconnect. The client-side connect chain is dropped as redundant with #103.
8a354eb to
4f2b362
Compare
Superseded by the re-scoped and force-pushed PR. Every finding in this review applies to an obsolete head and is no longer a merge blocker. The current head should be evaluated independently against current CI and current-head findings.
What changed
One click takes an unconfigured provider from "not installed" to a verified, ready account:
The install→sign-in handoff is an operation-owned chain: bound to the exact managed-install
operationId, so concurrent providers never interfere, stale completions can't trigger/clear the
wrong flow, and the chain survives closing the dialog. Terminal failures are observable (Activity
Center) and retryable even with the dialog closed. Credentials stay with each provider's own CLI.
Scope
In this PR: one-click install-and-connect orchestration, per-provider operation-owned chains,
failure/retry surfacing, presentation logic + unit/browser coverage.
Not in this PR (follow-ups): first-run onboarding (preserved on
onboarding-connect-accounts-wip);ACP session/set_model fallback; settings/composer refactors; native OpenAI OAuth (dropped for the
official
codex loginflow).