fix(auth): a provider's first-ever login silently saved nothing

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 <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-08-17 21:39:06 -07:00
parent 7c1d06237c
commit 9eae6b9bc5
9 changed files with 227 additions and 13 deletions

View File

@@ -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.

View File

@@ -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");
}

View File

@@ -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);
}
/*

View File

@@ -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");
}

View File

@@ -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.");
});
});

View File

@@ -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;
}

View File

@@ -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 () => {

View File

@@ -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 });

View File

@@ -218,17 +218,40 @@ class FusionFileAuthStorage implements FusionAuthStorage {
async getApiKey(provider: string, instance?: ProviderInstanceRef): Promise<string | undefined> {
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<StoredCredential | undefined>): Promise<StoredCredential | undefined> {
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" }]; }