Skip to content

fix(auth): never roll back OAuth credentials on failed post-login model refresh #2059

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
flora131 merged 1 commit into from
Jul 29, 2026
Merged

fix(auth): never roll back OAuth credentials on failed post-login model refresh #2059

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/coding-agent/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## [Unreleased]

### Fixed

- Fixed OAuth logins being destroyed by an unrelated model-catalog refresh, matching upstream pi's behavior. After a successful `/login`, a timed-out (aborted) or partially failed catalog refresh threw `Model refresh aborted after OAuth login` and rolled the freshly acquired tokens back to the previous credential. Because providers rotate refresh tokens, that rollback could permanently strand a server-side-invalidated credential — every send then failed with `invalid_grant` ("Refresh token not found or invalid") and every re-login was rolled back again, typically on machines with slow routes to catalog endpoints. Freshly persisted OAuth credentials now always survive the post-login refresh: per-provider refresh errors and refresh timeouts no longer fail the login in either the direct interactive or isolated-engine path, and models fall back to the cached snapshot.

## [0.9.11-alpha.7] - 2026-07-28

### Changed
Expand Down
7 changes: 3 additions & 4 deletions packages/coding-agent/src/modes/interactive/interactive-auth-login.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,9 @@ import { isOAuthLoginCancelled } from "../../core/oauth-provider-bridge.ts";

InteractiveModeBase.prototype.completeProviderAuthentication = async function(this: InteractiveModeBase, providerId: string, providerName: string, authType: "oauth" | "api_key", previousModel: Model<Api> | undefined, options: { modelsRefreshed?: boolean } = {}): Promise<void> {
if (!options.modelsRefreshed) {
const result = await this.session.modelRegistry.refresh();
const providerError = result.errors.get(providerId);
if (providerError) throw providerError;
if (result.aborted) throw new Error(`Model refresh aborted after authenticating ${providerName}`);
// Upstream pi parity: a failed or timeout-aborted catalog refresh after a
// completed login is non-fatal; models fall back to the cached snapshot.
await this.session.modelRegistry.refresh();
}

const actionLabel =
Expand Down
26 changes: 8 additions & 18 deletions packages/coding-agent/src/modes/rpc/rpc-provider-auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,26 +125,16 @@ export class RpcProviderAuth {
credential: AuthCredential,
signal: AbortSignal,
): Promise<void> {
const storage = session.modelRegistry.authStorage;
const previous = storage.get(provider);
const credentialStore = storage.asCredentialStore();
const credentialStore = session.modelRegistry.authStorage.asCredentialStore();
if (signal.aborted) throw normalizeOAuthLoginError(signal.reason, signal);
await credentialStore.modify(provider, async () => credential);
try {
if (signal.aborted) throw normalizeOAuthLoginError(signal.reason, signal);
const result = await session.modelRegistry.refresh();
const failures = [...result.errors.entries()];
if (failures.length > 0) {
throw new Error(failures.map(([id, error]) => `${id}: ${error.message}`).join("; "));
}
if (signal.aborted) throw normalizeOAuthLoginError(signal.reason, signal);
if (result.aborted) throw new Error("Model refresh aborted after OAuth login");
session.refreshCurrentModelFromRegistry();
} catch (error) {
if (previous === undefined) await credentialStore.delete(provider);
else await credentialStore.modify(provider, async () => previous);
throw error;
}
// Upstream pi parity: once fresh OAuth tokens are persisted, the catalog
// refresh outcome (per-provider errors or a timeout-aborted result) must
// not fail the login or touch the stored credential. Refresh tokens are
// rotated by providers, so rolling back here would re-install a
// server-side-invalidated credential (permanent invalid_grant).
await session.modelRegistry.refresh();
session.refreshCurrentModelFromRegistry();
Comment on lines +136 to +137

@greptile-apps greptile-apps Bot Jul 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Cancellation reports login success

When cancel_login_provider arrives after the credential is persisted but during the catalog refresh, refresh() returns an aborted result normally and this path continues to report cancelled: false. The client consequently receives both a successful cancellation acknowledgment and a successful login response.

Artifacts

Repro: focused Vitest race harness using the real RPC command handler

  • Evidence file captured while the check ran.

Repro: Vitest configuration used to execute the focused artifact test

  • Evidence file captured while the check ran.

Repro: verbose execution trace showing refresh start, successful cancellation acknowledgement, aborted refresh, and login success with cancelled false

  • The full command output behind this check.

View artifacts

T-Rex Ran code and verified through T-Rex

Prompt To Fix With AI 
This is a comment left during a code review.
Path: packages/coding-agent/src/modes/rpc/rpc-provider-auth.ts
Line: 136-137

Comment:
**Cancellation reports login success**

When `cancel_login_provider` arrives after the credential is persisted but during the catalog refresh, `refresh()` returns an aborted result normally and this path continues to report `cancelled: false`. The client consequently receives both a successful cancellation acknowledgment and a successful login response.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

@flora131 flora131 Jul 29, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid race on this branch, resolved in the stacked #2064 which rewrites this file for the ModelRuntime migration: loginOAuth now re-checks controller.signal.aborted after modelRuntime.login() returns and reports cancelled: true for a cancel landing during the post-persist catalog refresh. The persisted tokens are deliberately kept in that window — rolling them back is the invalid_grant trap this PR removes (providers rotate refresh tokens, so a rollback can strand a server-side-invalidated credential). Fixing it here separately would conflict with the stacked rewrite for an identical end state, so it lands via #2064.

}

private catalog(session: AgentSession): RpcModelCatalog {
Expand Down
60 changes: 32 additions & 28 deletions packages/coding-agent/test/interactive-auth-login.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,34 +209,38 @@ describe("post-login model refresh", () => {
});
}

it("keeps a provider-specific model refresh failure visible after credential commit", async () => {
const refreshFailure = new Error("catalog unavailable");
const showStatus = vi.fn();
const harness = {
session: {
modelRegistry: {
refresh: async () => ({ aborted: false, errors: new Map([["corp-oauth", refreshFailure]]) }),
getAvailable: () => [],
for (const outcome of [
{ label: "reports provider errors", result: { aborted: false, errors: new Map([["corp-oauth", new Error("catalog unavailable")]]) } },
{ label: "aborts on timeout", result: { aborted: true, errors: new Map() } },
]) {
it(`completes login when the post-login model refresh ${outcome.label}`, async () => {
const showStatus = vi.fn();
const harness = {
session: {
modelRegistry: {
refresh: async () => outcome.result,
getAvailable: () => [],
},
},
},
updateAvailableProviderCount: vi.fn(),
setupAutocompleteProvider: vi.fn(),
footer: { invalidate: vi.fn() },
updateEditorBorderColor: vi.fn(),
showStatus,
showError: vi.fn(),
maybeWarnAboutAnthropicSubscriptionAuth: vi.fn(),
checkDaxnutsEasterEgg: vi.fn(),
};
const complete = InteractiveModeBase.prototype.completeProviderAuthentication as (
this: typeof harness,
providerId: string,
providerName: string,
authType: "oauth" | "api_key",
previousModel: Model<Api> | undefined,
) => Promise<void>;
updateAvailableProviderCount: vi.fn(),
setupAutocompleteProvider: vi.fn(),
footer: { invalidate: vi.fn() },
updateEditorBorderColor: vi.fn(),
showStatus,
showError: vi.fn(),
maybeWarnAboutAnthropicSubscriptionAuth: vi.fn(),
checkDaxnutsEasterEgg: vi.fn(),
};
const complete = InteractiveModeBase.prototype.completeProviderAuthentication as (
this: typeof harness,
providerId: string,
providerName: string,
authType: "oauth" | "api_key",
previousModel: Model<Api> | undefined,
) => Promise<void>;

await expect(complete.call(harness, "corp-oauth", "Corp OAuth", "oauth", undefined)).rejects.toBe(refreshFailure);
expect(showStatus).not.toHaveBeenCalled();
});
await complete.call(harness, "corp-oauth", "Corp OAuth", "oauth", undefined);
expect(showStatus).toHaveBeenCalledWith(expect.stringContaining("Logged in to Corp OAuth"));
});
}
});
25 changes: 13 additions & 12 deletions packages/coding-agent/test/rpc-oauth-login.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ describe("isolated engine OAuth", () => {
expect(caught).toMatchObject({ message: "Login cancelled", cause: abort });
expect({ applied, reloaded }).toEqual({ applied: 0, reloaded: 0 });
});
it("rolls back the acquired credential when post-login model refresh fails", async () => {
it("keeps the acquired credential when post-login model refresh reports provider errors", async () => {
const { harness, runtime } = await createRuntimeHarness();
harness.authStorage.set("corp-oauth", { type: "api_key", key: "previous" });
harness.session.modelRegistry.registerProvider("corp-oauth", {
Expand All @@ -368,10 +368,9 @@ describe("isolated engine OAuth", () => {
getApiKey: (credential) => credential.access,
},
});
const refreshFailure = new Error("catalog unavailable");
harness.session.modelRegistry.refresh = async () => ({
aborted: false,
errors: new Map([["corp-oauth", refreshFailure]]),
errors: new Map([["corp-oauth", new Error("catalog unavailable")]]),
});
const handler = createRpcCommandHandler({
runtimeHost: runtime,
Expand All @@ -381,13 +380,14 @@ describe("isolated engine OAuth", () => {
output: () => {},
});

await expect(handler({
const response = await handler({
id: "login", type: "login_provider", provider: "corp-oauth", authType: "oauth",
})).rejects.toThrow("catalog unavailable");
expect(harness.authStorage.get("corp-oauth")).toEqual({ type: "api_key", key: "previous" });
});
expect(response).toMatchObject({ success: true, data: { provider: "corp-oauth", cancelled: false } });
expect(harness.authStorage.get("corp-oauth")).toMatchObject({ type: "oauth", access: "new-secret" });
});

it("keeps a post-acquisition AbortError from model refresh visible", async () => {
it("keeps a thrown post-login model refresh failure visible without rolling back", async () => {
const { harness, runtime } = await createRuntimeHarness();
harness.authStorage.set("corp-oauth", { type: "api_key", key: "previous" });
harness.session.modelRegistry.registerProvider("corp-oauth", {
Expand All @@ -411,10 +411,10 @@ describe("isolated engine OAuth", () => {
await expect(handler({
id: "login", type: "login_provider", provider: "corp-oauth", authType: "oauth",
})).rejects.toThrow("refresh transport aborted");
expect(harness.authStorage.get("corp-oauth")).toEqual({ type: "api_key", key: "previous" });
expect(harness.authStorage.get("corp-oauth")).toMatchObject({ type: "oauth", access: "new-secret" });
});

it("rolls back when model refresh reports an aborted result", async () => {
it("completes login and keeps the credential when model refresh reports an aborted result", async () => {
const { harness, runtime } = await createRuntimeHarness();
harness.authStorage.set("corp-oauth", { type: "api_key", key: "previous" });
harness.session.modelRegistry.registerProvider("corp-oauth", {
Expand All @@ -434,9 +434,10 @@ describe("isolated engine OAuth", () => {
output: () => {},
});

await expect(handler({
const response = await handler({
id: "login", type: "login_provider", provider: "corp-oauth", authType: "oauth",
})).rejects.toThrow("Model refresh aborted after OAuth login");
expect(harness.authStorage.get("corp-oauth")).toEqual({ type: "api_key", key: "previous" });
});
expect(response).toMatchObject({ success: true, data: { provider: "corp-oauth", cancelled: false } });
expect(harness.authStorage.get("corp-oauth")).toMatchObject({ type: "oauth", access: "new-secret" });
});
});
Loading