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 { LlamaCppProviderCard } from "./LlamaCppProviderCard";
import { LoginInstructions } from "./LoginInstructions"; import { LoginInstructions } from "./LoginInstructions";
import { ProviderLoginDialog, type ProviderLoginPhase } from "./ProviderLoginDialog"; import { ProviderLoginDialog, type ProviderLoginPhase } from "./ProviderLoginDialog";
import { describeLoginFailure } from "../utils/loginFailure";
import { OAuthManualCodeForm } from "./OAuthManualCodeForm"; import { OAuthManualCodeForm } from "./OAuthManualCodeForm";
import { OnboardingDisclosure } from "./OnboardingDisclosure"; import { OnboardingDisclosure } from "./OnboardingDisclosure";
import { CustomProviderForm } from "./CustomProviderForm"; import { CustomProviderForm } from "./CustomProviderForm";
@@ -1552,7 +1553,16 @@ export function ModelOnboardingModal({
} }
setAuthActionInProgress(null); setAuthActionInProgress(null);
setLoginOutcomes((prev) => ({ ...prev, [providerId]: "failed" })); 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(); clearAuthLoginUiState();
addToast(t("setup.loginDidNotComplete", "Login did not complete. Please try again."), "error"); 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; flex-direction: column;
gap: var(--space-sm); gap: var(--space-sm);
/* /*
`var(--modal-padding)` — the SAME inset as .modal-header, the body, and .modal-actions. Using FNXC:ProviderAuth 2026-08-18-06:45:
--space-lg here instead left the paste box and its Submit button hanging closer to the panel edge Horizontal inset comes from `--modal-padding` so this region always lines up with .modal-header and
than every other row, which is what read as them being flush against it. .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: var(--modal-padding);
padding-block: var(--space-lg);
border-top: 1px solid var(--border); border-top: 1px solid var(--border);
background: var(--surface); 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 { .provider-login-dialog__paste .oauth-manual-code {
margin-top: 0; 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 { useWorkspaceFileBrowser } from "../hooks/useWorkspaceFileBrowser";
import { FloatingWindow } from "./FloatingWindow"; import { FloatingWindow } from "./FloatingWindow";
import { ProviderLoginDialog, type ProviderLoginPhase } from "./ProviderLoginDialog"; import { ProviderLoginDialog, type ProviderLoginPhase } from "./ProviderLoginDialog";
import { describeLoginFailure } from "../utils/loginFailure";
import { ProviderIcon } from "./ProviderIcon"; import { ProviderIcon } from "./ProviderIcon";
import { generateUniquePresetId } from "../utils/modelPresets"; import { generateUniquePresetId } from "../utils/modelPresets";
import { copyTextToClipboard } from "../utils/copyToClipboard"; import { copyTextToClipboard } from "../utils/copyToClipboard";
@@ -2572,7 +2573,12 @@ export function SettingsModal({
delete pollIntervalRef.current[stateKey]; delete pollIntervalRef.current[stateKey];
} }
setAuthActionInProgress((prev) => { const next = { ...prev }; delete next[stateKey]; return next; }); 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); clearAuthLoginUiState(stateKey);
addToast(t("settings.auth.loginDidNotComplete", "Login did not complete. Please try again."), "error"); 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); 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(); const store = createFusionAuthStorage();
await store.set("brand-new", credential("new")); await store.set("brand-new", credential("new"));
expect(JSON.parse(readFileSync(authPath(), "utf8"))).toEqual({ "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; 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.remove("absent"); await store.logout("absent"); await store.removeInstance({ providerId: "absent", instanceId: "x" });
await store.modify("absent", async () => { invoked = true; return credential("bad"); }); expect(readFileSync(authPath(), "utf8")).toBe(before); expect(statSync(authPath()).mtimeMs).toBe(mtime);
expect(invoked).toBe(false); 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 () => { 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 () => { it("writes to Fusion auth and reads legacy Pi auth as fallback", async () => {
const legacyAgentDir = join(homeDir, ".pi", "agent"); const legacyAgentDir = join(homeDir, ".pi", "agent");
mkdirSync(legacyAgentDir, { recursive: true }); mkdirSync(legacyAgentDir, { recursive: true });

View File

@@ -218,17 +218,40 @@ class FusionFileAuthStorage implements FusionAuthStorage {
async getApiKey(provider: string, instance?: ProviderInstanceRef): Promise<string | undefined> { async getApiKey(provider: string, instance?: ProviderInstanceRef): Promise<string | undefined> {
return resolveStoredCredentialApiKey(provider, instance ? this.getInstance(instance) : this.get(provider)); 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> { async modify(provider: string, fn: (current: StoredCredential | undefined) => Promise<StoredCredential | undefined>): Promise<StoredCredential | undefined> {
this.assertRefFromKey(provider); this.assertRefFromKey(provider);
return this.withLock(async current => { return this.withLock(async current => {
const target = this.resolveWriteTarget(provider, current, false); const target = this.resolveWriteTarget(provider, current, true);
if (!target || !this.credential(target, current)) return { result: undefined, changed: false }; if (!target) return { result: undefined, changed: false };
const next = await fn(this.credential(target, current)); const existing = this.credential(target, current);
const next = await fn(existing);
if (next !== undefined) { if (next !== undefined) {
current[formatProviderInstanceKey(target)] = next; current[formatProviderInstanceKey(target)] = next;
return { result: next, changed: true }; 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" }]; } getOAuthProviders(): Array<{ id: string; name: string }> { return [{ id: "anthropic", name: "Anthropic" }, { id: "openai-codex", name: "OpenAI Codex" }, { id: "github-copilot", name: "GitHub Copilot" }]; }