From 9eae6b9bc5dae57d0aba5aaacf774efc92f6ad5f Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 17 Aug 2026 21:39:06 -0700 Subject: [PATCH] fix(auth): a provider's first-ever login silently saved nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Operator could not log in to Anthropic or Codex on a fresh container: every attempt ended "Login did not complete. Please try again.", while the same providers worked flawlessly on their long-lived native install. FusionAuthStorage.modify() is the seam pi persists a COMPLETED LOGIN through (Models.login -> credentials.modify(provider.id, ...) in pi-ai models.js:198). It resolved its write target with `creating: false` and returned before invoking the callback whenever the provider had no credential row yet: const target = this.resolveWriteTarget(provider, current, false); if (!target || !this.credential(target, current)) return { changed: false }; So a first login completed its browser flow, exchanged the code, took and released the lock file, wrote NOTHING, and resolved as success — leaving the dashboard poll to see authenticated:false and report the generic failure. It reproduces only on a store with no existing row, which is why it looked environment-specific: an install that has logged in before takes the same path as a refresh over an existing row and is fine, while every new container, new machine, or wiped ~/.fusion can never complete a first login for ANY provider. Evidence from the operator's container: flow ended with err=None (pi resolved, no error), nothing logged, auth.json still {}, the agent directory's mtime bumped when the lock was taken and released while auth.json itself never changed, and an API-key write — which goes through set(), not modify() — landed immediately. modify() now creates when absent and updates when present; a callback returning undefined still writes nothing, so pi's refresh-bails-out behaviour is unchanged. auth-storage-instances.test.ts asserted the old behaviour, grouping modify() with remove/logout/removeInstance as "non-creating". The removal guarantees are kept; the modify() assertion is inverted, because it encoded the defect. Also surfaces the server's own loginError through a new describeLoginFailure() helper instead of the generic sentence, so an OAuth state mismatch reads as the stale-tab instruction it is. Writing its test caught a bad regex of mine: `code.*expired` matched "OpenAI Codex ... token_expired", a different failure. Verified: the new first-login test fails against the old `creating: false` and passes with the fix; 86 engine auth tests, 238 dashboard auth/dialog tests, and pnpm test:gate all pass. Co-Authored-By: Claude Opus 5 --- .changeset/fix-first-login-never-persists.md | 7 +++ .../app/components/ModelOnboardingModal.tsx | 12 ++++- .../app/components/ProviderLoginDialog.css | 22 ++++++-- .../app/components/SettingsModal.tsx | 8 ++- .../app/utils/__tests__/loginFailure.test.ts | 38 ++++++++++++++ packages/dashboard/app/utils/loginFailure.ts | 42 +++++++++++++++ .../__tests__/auth-storage-instances.test.ts | 29 +++++++++-- .../engine/src/__tests__/auth-storage.test.ts | 51 +++++++++++++++++++ packages/engine/src/auth/auth-storage.ts | 31 +++++++++-- 9 files changed, 227 insertions(+), 13 deletions(-) create mode 100644 .changeset/fix-first-login-never-persists.md create mode 100644 packages/dashboard/app/utils/__tests__/loginFailure.test.ts create mode 100644 packages/dashboard/app/utils/loginFailure.ts diff --git a/.changeset/fix-first-login-never-persists.md b/.changeset/fix-first-login-never-persists.md new file mode 100644 index 0000000000..18af711cd3 --- /dev/null +++ b/.changeset/fix-first-login-never-persists.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Fix a provider's first-ever login silently failing with "Login did not complete" on a fresh install. +category: fix +dev: `FusionAuthStorage.modify()` resolved its write target with `creating: false` and returned before invoking the callback whenever the provider had no credential row yet. That is the seam pi persists a completed login through (`Models.login` -> `credentials.modify(provider.id, ...)`), so a first login finished its OAuth, exchanged the code, took and released the lock file, wrote nothing, and resolved as success — leaving the dashboard poll to report the generic failure. Only reproduces on a store with no existing row, so long-lived installs (where the path is a refresh) were unaffected while every new container/machine/wiped `~/.fusion` could never complete a first login for any provider. Also surfaces the server's own `loginError` through `describeLoginFailure` instead of the generic sentence, so an `OAuth state mismatch` reads as a stale-tab instruction. diff --git a/packages/dashboard/app/components/ModelOnboardingModal.tsx b/packages/dashboard/app/components/ModelOnboardingModal.tsx index ffc7a554f5..3da6a69e2a 100644 --- a/packages/dashboard/app/components/ModelOnboardingModal.tsx +++ b/packages/dashboard/app/components/ModelOnboardingModal.tsx @@ -31,6 +31,7 @@ import { CursorCliProviderCard } from "./CursorCliProviderCard"; import { LlamaCppProviderCard } from "./LlamaCppProviderCard"; import { LoginInstructions } from "./LoginInstructions"; import { ProviderLoginDialog, type ProviderLoginPhase } from "./ProviderLoginDialog"; +import { describeLoginFailure } from "../utils/loginFailure"; import { OAuthManualCodeForm } from "./OAuthManualCodeForm"; import { OnboardingDisclosure } from "./OnboardingDisclosure"; import { CustomProviderForm } from "./CustomProviderForm"; @@ -1552,7 +1553,16 @@ export function ModelOnboardingModal({ } setAuthActionInProgress(null); setLoginOutcomes((prev) => ({ ...prev, [providerId]: "failed" })); - setLoginErrors((prev) => ({ ...prev, [providerId]: t("setup.loginDidNotComplete", "Login did not complete. Please try again.") })); + /* + FNXC:ProviderAuth 2026-08-18-07:10: + PREFER THE SERVER'S REASON. The status row carries why the flow died (`loginError`), and + throwing it away for "Login did not complete. Please try again." is how a real, fixable + cause reached the operator as a shrug: an `OAuth state mismatch` — the pasted URL + belonging to an OLDER sign-in attempt than the one waiting, i.e. a stale provider tab — + is indistinguishable from a network failure under the generic text, and "try again" + reproduces it exactly if they paste from the same stale tab. + */ + setLoginErrors((prev) => ({ ...prev, [providerId]: describeLoginFailure(provider?.loginError) })); clearAuthLoginUiState(); addToast(t("setup.loginDidNotComplete", "Login did not complete. Please try again."), "error"); } diff --git a/packages/dashboard/app/components/ProviderLoginDialog.css b/packages/dashboard/app/components/ProviderLoginDialog.css index d3bd299867..67f86545d3 100644 --- a/packages/dashboard/app/components/ProviderLoginDialog.css +++ b/packages/dashboard/app/components/ProviderLoginDialog.css @@ -126,17 +126,33 @@ is. It carries the same inset as the body and a top divider to separate it from flex-direction: column; gap: var(--space-sm); /* - `var(--modal-padding)` — the SAME inset as .modal-header, the body, and .modal-actions. Using - --space-lg here instead left the paste box and its Submit button hanging closer to the panel edge - than every other row, which is what read as them being flush against it. + FNXC:ProviderAuth 2026-08-18-06:45: + Horizontal inset comes from `--modal-padding` so this region always lines up with .modal-header and + .modal-actions under any theme. Vertical is deliberately LARGER than that token: themes set + `--modal-padding: var(--space-md) var(--space-lg)` (10px at runtime), which is a header-row density + and left this cluster — prompt, textarea, Submit, help text — crowded against its own divider and + the action row. Order matters: `padding` sets both axes from the token, then `padding-block` + replaces only the vertical half. */ padding: var(--modal-padding); + padding-block: var(--space-lg); border-top: 1px solid var(--border); background: var(--surface); } +/* +FNXC:ProviderAuth 2026-08-18-06:45: +The shared form is built for row density inside a provider card: ~6px between its prompt, field and +Submit. As this dialog's primary task it needs breathing room between those three, so open the gaps +here only — the card usages keep their compact rhythm. +*/ .provider-login-dialog__paste .oauth-manual-code { margin-top: 0; + gap: var(--space-md); +} + +.provider-login-dialog__paste .oauth-manual-code__actions { + margin-top: var(--space-xs); } /* diff --git a/packages/dashboard/app/components/SettingsModal.tsx b/packages/dashboard/app/components/SettingsModal.tsx index f134200116..4d4b8dc4f4 100644 --- a/packages/dashboard/app/components/SettingsModal.tsx +++ b/packages/dashboard/app/components/SettingsModal.tsx @@ -73,6 +73,7 @@ import { FileBrowser } from "./FileBrowser"; import { useWorkspaceFileBrowser } from "../hooks/useWorkspaceFileBrowser"; import { FloatingWindow } from "./FloatingWindow"; import { ProviderLoginDialog, type ProviderLoginPhase } from "./ProviderLoginDialog"; +import { describeLoginFailure } from "../utils/loginFailure"; import { ProviderIcon } from "./ProviderIcon"; import { generateUniquePresetId } from "../utils/modelPresets"; import { copyTextToClipboard } from "../utils/copyToClipboard"; @@ -2572,7 +2573,12 @@ export function SettingsModal({ delete pollIntervalRef.current[stateKey]; } setAuthActionInProgress((prev) => { const next = { ...prev }; delete next[stateKey]; return next; }); - setLoginErrors((prev) => ({ ...prev, [stateKey]: t("settings.auth.loginDidNotComplete", "Login did not complete. Please try again.") })); + /* + FNXC:ProviderAuth 2026-08-18-07:10: + Prefer the server's own reason over the generic sentence — an `OAuth state mismatch` + (pasted URL from an older attempt) is actionable, and "try again" alone reproduces it. + */ + setLoginErrors((prev) => ({ ...prev, [stateKey]: describeLoginFailure(provider?.loginError) })); clearAuthLoginUiState(stateKey); addToast(t("settings.auth.loginDidNotComplete", "Login did not complete. Please try again."), "error"); } diff --git a/packages/dashboard/app/utils/__tests__/loginFailure.test.ts b/packages/dashboard/app/utils/__tests__/loginFailure.test.ts new file mode 100644 index 0000000000..4c6e86c678 --- /dev/null +++ b/packages/dashboard/app/utils/__tests__/loginFailure.test.ts @@ -0,0 +1,38 @@ +import { describe, expect, it } from "vitest"; +import { describeLoginFailure } from "../loginFailure"; + +/* +FNXC:ProviderAuth 2026-08-18-07:10: +An operator hit `OAuth state mismatch` — a pasted redirect URL from an older sign-in attempt — and +both login surfaces reported it as "Login did not complete. Please try again.", which describes a +transient failure and prescribes the one action that reproduces it. These pin that the server's +reason survives to the operator, and that the two self-inflicted cases say what to do differently. +*/ +describe("describeLoginFailure", () => { + it("explains a stale-tab state mismatch instead of suggesting a blind retry", () => { + const message = describeLoginFailure("OAuth state mismatch"); + expect(message).toMatch(/earlier sign-in attempt/i); + expect(message).toMatch(/newest tab/i); + expect(message).not.toMatch(/^Login did not complete/); + }); + + it("explains a spent authorization code", () => { + const upstream = "Token exchange request failed. body={\"error\": \"invalid_grant\", \"error_description\": \"Invalid 'code' in request.\"}"; + expect(describeLoginFailure(upstream)).toMatch(/already used or has expired/i); + }); + + it("names a cancellation as one", () => { + expect(describeLoginFailure("This operation was aborted")).toMatch(/cancelled/i); + }); + + it("passes an unrecognized upstream reason through verbatim", () => { + // A specific upstream message always beats replacing it with the generic sentence. + const upstream = "OpenAI Codex token exchange failed (401): token_expired"; + expect(describeLoginFailure(upstream)).toBe(upstream); + }); + + it("falls back to the generic sentence only when the server gave no reason", () => { + expect(describeLoginFailure(undefined)).toBe("Login did not complete. Please try again."); + expect(describeLoginFailure(" ")).toBe("Login did not complete. Please try again."); + }); +}); diff --git a/packages/dashboard/app/utils/loginFailure.ts b/packages/dashboard/app/utils/loginFailure.ts new file mode 100644 index 0000000000..cdcb85df6c --- /dev/null +++ b/packages/dashboard/app/utils/loginFailure.ts @@ -0,0 +1,42 @@ +/* +FNXC:ProviderAuth 2026-08-18-07:10: +Turn a provider login failure into something the operator can act on. + +Both login surfaces used to report every non-completion as "Login did not complete. Please try +again." — including `OAuth state mismatch`, which is not a transient failure at all: it means the +pasted redirect URL came from a DIFFERENT (older) sign-in attempt than the one currently waiting, +so "try again" reproduces it exactly as long as the operator keeps pasting from the same stale +provider tab. That cost a real debugging session. + +The server's own `loginError` is the input; anything unrecognized passes through verbatim, because a +specific upstream message (an invalid_grant body, a token-endpoint status) is always more useful than +the generic sentence it would otherwise be replaced by. +*/ + +const GENERIC_FAILURE = "Login did not complete. Please try again."; + +export function describeLoginFailure(serverReason?: string): string { + const reason = serverReason?.trim(); + if (!reason) { + return GENERIC_FAILURE; + } + + if (/state mismatch/i.test(reason)) { + return "That link is from an earlier sign-in attempt. Close any older provider tabs, then start this login again and paste the URL from the newest tab."; + } + + /* + * Match the authorization-code grant failure only. A loose `code.*expired` also matched + * "OpenAI Codex ... token_expired" — a different failure (a stale stored credential, not a spent + * one-time code) that would then be given the wrong remedy. + */ + if (/invalid_grant|Invalid 'code'|authorization code (?:has )?(?:expired|already)/i.test(reason)) { + return "That authorization code was already used or has expired. Start the login again and paste the fresh redirect URL."; + } + + if (/aborted|cancelled/i.test(reason)) { + return "The login was cancelled before it finished. Start it again."; + } + + return reason; +} diff --git a/packages/engine/src/__tests__/auth-storage-instances.test.ts b/packages/engine/src/__tests__/auth-storage-instances.test.ts index 9052f700b6..456faf150b 100644 --- a/packages/engine/src/__tests__/auth-storage-instances.test.ts +++ b/packages/engine/src/__tests__/auth-storage-instances.test.ts @@ -40,15 +40,36 @@ describe("instance-aware Fusion auth storage", () => { expect(readFileSync(authPath(), "utf8")).toBe(before); expect(statSync(authPath()).mtimeMs).toBe(mtime); }); - it("uses legacy bare creation but non-creating calls do not write absent providers", async () => { + it("uses legacy bare creation but removal calls do not write absent providers", async () => { const store = createFusionAuthStorage(); await store.set("brand-new", credential("new")); expect(JSON.parse(readFileSync(authPath(), "utf8"))).toEqual({ "brand-new": credential("new") }); const before = readFileSync(authPath(), "utf8"); const mtime = statSync(authPath()).mtimeMs; - let invoked = false; await store.remove("absent"); await store.logout("absent"); await store.removeInstance({ providerId: "absent", instanceId: "x" }); - await store.modify("absent", async () => { invoked = true; return credential("bad"); }); - expect(invoked).toBe(false); expect(readFileSync(authPath(), "utf8")).toBe(before); expect(statSync(authPath()).mtimeMs).toBe(mtime); + expect(readFileSync(authPath(), "utf8")).toBe(before); expect(statSync(authPath()).mtimeMs).toBe(mtime); + }); + + /* + FNXC:ProviderAuth 2026-08-18-04:40: + `modify` USED TO BE ASSERTED HERE AS NON-CREATING, alongside remove/logout/removeInstance. That + grouping was wrong: removal calls must not resurrect an absent provider, but `modify` is the seam + pi persists a COMPLETED LOGIN through (`Models.login` -> `credentials.modify(provider.id, ...)`), + and refusing to create meant a provider's first-ever login wrote nothing while resolving as + success — the operator saw "Login did not complete. Please try again." on every fresh install. + Creation is the contract now; declining (returning undefined) still writes nothing. + */ + it("creates an absent provider when the modify callback returns a credential", async () => { + const store = createFusionAuthStorage(); + const before = readFileSync(authPath(), "utf8"); + + let declined = false; + await store.modify("absent", async () => { declined = true; return undefined; }); + expect(declined).toBe(true); + expect(readFileSync(authPath(), "utf8")).toBe(before); + + await store.modify("absent", async (current) => { expect(current).toBeUndefined(); return credential("first-login"); }); + expect(store.get("absent")).toEqual(credential("first-login")); + expect(JSON.parse(readFileSync(authPath(), "utf8")).absent).toEqual(credential("first-login")); }); it("rejects malformed and reserved mutator keys without touching defaults metadata", async () => { diff --git a/packages/engine/src/__tests__/auth-storage.test.ts b/packages/engine/src/__tests__/auth-storage.test.ts index eb39a19577..489e4a821b 100644 --- a/packages/engine/src/__tests__/auth-storage.test.ts +++ b/packages/engine/src/__tests__/auth-storage.test.ts @@ -153,6 +153,57 @@ describe("createFusionAuthStorage", () => { }); }); + + /* + FNXC:ProviderAuth 2026-08-18-04:40: + REGRESSION: a FIRST-EVER login must persist. pi's `Models.login` saves the credential it just + obtained through `credentials.modify(provider.id, ...)`, and this seam used to bail out before + invoking the callback whenever the provider had no row yet — so the OAuth completed, nothing was + written, pi resolved as success, and the dashboard reported "Login did not complete. Please try + again." on a fresh install. It reproduced only with an EMPTY store, which is why every existing + install (where this path is a refresh over an existing row) looked fine. + */ + describe("modify() on a store with no existing credential", () => { + it("creates the credential a first-time login returns", async () => { + const authStorage = createFusionAuthStorage(); + expect(authStorage.get("openai-codex")).toBeUndefined(); + + let sawCurrent: unknown = "callback never ran"; + const written = await authStorage.modify("openai-codex", async (current) => { + sawCurrent = current; + return { type: "oauth", access: "access-token", refresh: "refresh-token", expires: 1 } as never; + }); + + expect(sawCurrent, "callback must run even with nothing stored yet").toBeUndefined(); + expect(written).toBeDefined(); + // Persisted, not just returned: the next read (and the next process) must see it. + expect(authStorage.get("openai-codex")).toMatchObject({ type: "oauth", access: "access-token" }); + expect(JSON.parse(readFileSync(getFusionAuthPath(homeDir), "utf-8"))["openai-codex"]).toMatchObject({ access: "access-token" }); + }); + + it("still writes nothing when the callback declines", async () => { + const authStorage = createFusionAuthStorage(); + + const result = await authStorage.modify("openai-codex", async () => undefined); + + expect(result).toBeUndefined(); + expect(authStorage.get("openai-codex")).toBeUndefined(); + expect(JSON.parse(readFileSync(getFusionAuthPath(homeDir), "utf-8"))).toEqual({}); + }); + + it("updates in place when a credential already exists", async () => { + const authStorage = createFusionAuthStorage(); + await authStorage.set("openai-codex", { type: "oauth", access: "old", refresh: "r", expires: 1 } as never); + + await authStorage.modify("openai-codex", async (current) => { + expect(current).toMatchObject({ access: "old" }); + return { ...(current as object), access: "new" } as never; + }); + + expect(authStorage.get("openai-codex")).toMatchObject({ access: "new" }); + }); + }); + it("writes to Fusion auth and reads legacy Pi auth as fallback", async () => { const legacyAgentDir = join(homeDir, ".pi", "agent"); mkdirSync(legacyAgentDir, { recursive: true }); diff --git a/packages/engine/src/auth/auth-storage.ts b/packages/engine/src/auth/auth-storage.ts index 62d3161b9d..a2b84d1237 100644 --- a/packages/engine/src/auth/auth-storage.ts +++ b/packages/engine/src/auth/auth-storage.ts @@ -218,17 +218,40 @@ class FusionFileAuthStorage implements FusionAuthStorage { async getApiKey(provider: string, instance?: ProviderInstanceRef): Promise { return resolveStoredCredentialApiKey(provider, instance ? this.getInstance(instance) : this.get(provider)); } + /* + FNXC:ProviderAuth 2026-08-18-04:40: + MODIFY MUST BE ABLE TO CREATE. This is the seam pi persists a COMPLETED LOGIN through + (`Models.login` -> `credentials.modify(provider.id, ...)` in pi-ai's models.js), not just the seam + it refreshes an existing token through. + + It used to resolve its write target with `creating: false` and then bail — + `if (!target || !this.credential(target, current)) return { changed: false }` — whenever the + provider had no credential row yet. The callback was never invoked, nothing was written, and pi's + login resolved as a SUCCESS. So a first-ever login for a provider could not be saved: the browser + flow completed, the token exchange succeeded, the lock file was taken and released, `auth.json` + stayed `{}`, and the dashboard's poll saw `authenticated: false` and reported the useless "Login + did not complete. Please try again." + + It looked provider-specific and environment-specific for the worst possible reason: it only + reproduces on a store with NO existing row, so every developer and every long-lived install — where + a row already exists and this path is a plain refresh — works flawlessly, while every fresh install + (a new container, a new machine, a wiped `~/.fusion`) can never complete its first login. + + Create when absent, update when present. A callback returning undefined still writes nothing, so + pi's refresh-bails-out behaviour is unchanged. + */ async modify(provider: string, fn: (current: StoredCredential | undefined) => Promise): Promise { this.assertRefFromKey(provider); return this.withLock(async current => { - const target = this.resolveWriteTarget(provider, current, false); - if (!target || !this.credential(target, current)) return { result: undefined, changed: false }; - const next = await fn(this.credential(target, current)); + const target = this.resolveWriteTarget(provider, current, true); + if (!target) return { result: undefined, changed: false }; + const existing = this.credential(target, current); + const next = await fn(existing); if (next !== undefined) { current[formatProviderInstanceKey(target)] = next; return { result: next, changed: true }; } - return { result: this.credential(target, current), changed: false }; + return { result: existing, changed: false }; }); } getOAuthProviders(): Array<{ id: string; name: string }> { return [{ id: "anthropic", name: "Anthropic" }, { id: "openai-codex", name: "OpenAI Codex" }, { id: "github-copilot", name: "GitHub Copilot" }]; }