Skip to content

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

Merged
flora131 merged 1 commit into
mainfrom
fix/oauth-login-refresh-parity
Jul 29, 2026
Merged

fix(auth): never roll back OAuth credentials on failed post-login model refresh#2059
flora131 merged 1 commit into
mainfrom
fix/oauth-login-refresh-parity

Conversation

@flora131

@flora131 flora131 commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

Restores upstream pi's invariant that a completed OAuth login can never be undone by the model-catalog refresh that follows it. This fixes a Linux-reported trap where /login always failed with Model refresh aborted after OAuth login and every send then failed with invalid_grant ("Refresh token not found or invalid").

Root cause

After a successful OAuth login, Atomic ran a full catalog refresh (hard 15 s timeout) and treated an aborted or partially failed result as a login failure, rolling the freshly persisted tokens back to the previous credential (rpc-provider-auth.ts:persistOAuthAndRefresh) or throwing after commit (interactive-auth-login.ts:completeProviderAuthentication). Providers rotate refresh tokens, so the rollback could re-install a server-side-invalidated credential — a permanent invalid_grant that no re-login could escape, since each re-login was rolled back the same way. Any machine with a slow route to catalog endpoints (e.g. pi.dev) hit this deterministically. Upstream pi ignores the refresh outcome after login (ModelRuntime.login), so it cannot enter this state.

Changes

  • rpc-provider-auth.ts: persist the credential, run the post-login refresh best-effort, drop the rollback and the aborted/per-provider-error classification. A thrown refresh still propagates but no longer unpersists tokens.
  • interactive-auth-login.ts: drop the provider-error and refresh-aborted throws after a completed login; models fall back to the cached snapshot.
  • Regression tests updated in rpc-oauth-login.test.ts and interactive-auth-login.test.ts: credentials survive provider-error, thrown, and aborted refreshes; added an aborted-refresh interactive case.
  • Changelog entry under [Unreleased].

Notes

  • Verified with bun run typecheck, bun run lint, bun run check:file-length, and bunx vitest --run test/rpc-oauth-login.test.ts test/interactive-auth-login.test.ts (21/21 pass).
  • Not addressed here (possible follow-up): failed catalog fetches never set lastModified, so they bypass the 4-hour revalidation window and retry on every refresh — identical to upstream, but it keeps /model slow on machines with unreachable catalog endpoints.

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Greptile Summary

This PR preserves newly acquired OAuth credentials when the post-login model-catalog refresh fails.

  • Treats returned provider errors and aborted refresh results as non-fatal in interactive and RPC login flows.
  • Removes credential rollback after the RPC credential commit.
  • Updates regression coverage for provider-error, thrown-error, and aborted-refresh outcomes.
  • Documents the behavior change in the coding-agent changelog.

Confidence Score: 3/5

This PR should not merge until RPC cancellation during the post-commit refresh is distinguished from a successful login without rolling back the rotated credential.

An abort during model refresh resolves as an aborted result, but the changed RPC path ignores that result and returns cancelled false, allowing one login request to produce both a successful cancellation acknowledgment and a successful login response.

Files Needing Attention: packages/coding-agent/src/modes/rpc/rpc-provider-auth.ts

T-Rex T-Rex Logs

What T-Rex did

  • Verified the cancellation flow by executing the real RPC command with the OAuth harness, pausing ModelRegistry.refresh after credential persistence, and confirming cancel_login_provider returned success before the refresh aborted, while the correlated login_provider response showed success with cancelled: false and the OAuth credential remained stored.
  • Observed the test run completed with exit code 0, duration 5.918s, and all Vitest tests passing (2/2 files, 21/21 tests) according to the run log, with no additional setups or services started.

View all artifacts

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
packages/coding-agent/src/modes/rpc/rpc-provider-auth.ts Preserves committed OAuth credentials across refresh failures, but now reports success when cancellation arrives during the post-commit refresh.
packages/coding-agent/src/modes/interactive/interactive-auth-login.ts Makes returned provider errors and aborted refresh results non-fatal after interactive authentication.
packages/coding-agent/test/rpc-oauth-login.test.ts Updates credential-survival regression cases but does not cover cancellation after credential persistence.
packages/coding-agent/test/interactive-auth-login.test.ts Verifies interactive login completion for provider-error and aborted refresh results.
packages/coding-agent/CHANGELOG.md Documents that post-login catalog refresh outcomes no longer roll back OAuth credentials.

Sequence Diagram

sequenceDiagram
  participant Client
  participant RPC as RpcProviderAuth
  participant Store as Auth Storage
  participant Registry as Model Registry
  Client->>RPC: login_provider
  RPC->>Store: Persist new OAuth credential
  RPC->>Registry: refresh(signal)
  Client->>RPC: cancel_login_provider
  RPC->>Registry: Abort signal
  Registry-->>RPC: "{ aborted: true }"
  RPC-->>Client: Login success, cancelled: false
Loading
Prompt To Fix All With AI
### Issue 1
packages/coding-agent/src/modes/rpc/rpc-provider-auth.ts:136-137
**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.

Reviews (1): Last reviewed commit: "fix(auth): never roll back OAuth credent..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

…el refresh

Match upstream pi's invariant: once fresh OAuth tokens are persisted, the
catalog-refresh outcome must not fail the login or touch the stored
credential.

Previously, a timed-out (aborted) or partially failed model-catalog
refresh after a successful /login threw 'Model refresh aborted after
OAuth login' and rolled the fresh tokens back to the previous
credential. Because providers rotate refresh tokens, the rollback could
re-install a server-side-invalidated credential, permanently stranding
the user with 'invalid_grant' on every send while every re-login was
rolled back again. Machines with slow routes to catalog endpoints hit
this deterministically.

- rpc-provider-auth: persist the credential, run the refresh
  best-effort, and drop the rollback and aborted/error classification
  (a thrown refresh still propagates but no longer unpersists tokens)
- interactive-auth-login: drop the provider-error and aborted throws
  from the post-login refresh; models fall back to the cached snapshot
- update RPC and interactive regression tests to lock in credential
  survival; add an aborted-refresh interactive case

Assistant-model: Claude Fable 5
Comment on lines +136 to +137
await session.modelRegistry.refresh();
session.refreshCurrentModelFromRegistry();

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.

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.

@flora131
flora131 merged commit 86dd67d into main Jul 29, 2026
10 of 11 checks passed
flora131 added a commit that referenced this pull request Jul 29, 2026
All five conflicts resolve to the branch side: the ModelRuntime
migration supersedes the #2059 versions of these files while keeping
its credential-survival invariant and changelog entry.

Assistant-model: Claude Fable 5
flora131 added a commit that referenced this pull request Jul 29, 2026
…del-provider parity (#2064)

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

Match upstream pi's invariant: once fresh OAuth tokens are persisted, the
catalog-refresh outcome must not fail the login or touch the stored
credential.

Previously, a timed-out (aborted) or partially failed model-catalog
refresh after a successful /login threw 'Model refresh aborted after
OAuth login' and rolled the fresh tokens back to the previous
credential. Because providers rotate refresh tokens, the rollback could
re-install a server-side-invalidated credential, permanently stranding
the user with 'invalid_grant' on every send while every re-login was
rolled back again. Machines with slow routes to catalog endpoints hit
this deterministically.

- rpc-provider-auth: persist the credential, run the refresh
  best-effort, and drop the rollback and aborted/error classification
  (a thrown refresh still propagates but no longer unpersists tokens)
- interactive-auth-login: drop the provider-error and aborted throws
  from the post-login refresh; models fall back to the cached snapshot
- update RPC and interactive regression tests to lock in credential
  survival; add an aborted-refresh interactive case

Assistant-model: Claude Fable 5

* feat(coding-agent)!: adopt pi ModelRuntime architecture

Replace the fork-era registry engine with pi's provider-owned runtime and thin extension facade. Remove legacy OAuth registration and layered models configuration while preserving Atomic path, auth, RPC, and isolated-engine compatibility.

Assistant-model: Claude Opus 5

* fix(coding-agent): persist RPC credentials and restore coverage

Persist the full RPC credential union through ModelRuntime and refresh the catalog before returning. Restore Atomic-specific retry, compaction, footer, OAuth cancellation, migration, and usage-accounting regression coverage removed during the runtime parity port.

Assistant-model: Claude Opus 5

* fix(coding-agent): restore runtime compatibility coverage

Restore isolated RPC model refresh credential reload, explicit force and timeout forwarding, auth-storage transactional behavior, and SDK/RPC regression coverage while documenting pi parity changes.

Assistant-model: Claude Opus 5

* test(coding-agent): restore SDK session selection coverage

Assistant-model: Claude Opus 5

* fix(coding-agent): refresh isolated OAuth credentials

Reload frontend credential state after isolated login and logout so auth selectors reflect stored credentials. Transport and honor OAuth login labels and callback-server metadata, restoring reachable manual-code input.

Restore credential reload regression coverage, isolate RPC persistence tests from ambient auth, and document the AuthStorage and ModelRegistry API reductions.

Assistant-model: Claude Opus 5

* fix(coding-agent): restore builtin OAuth routing

Restore callback-server and login-label metadata for builtin OAuth providers so manual redirect login remains reachable. Keep logout and isolated auth status aligned with stored credentials and engine-published availability, and restore deterministic RPC prompt-lock coverage.

Assistant-model: Claude Opus 5

* fix(coding-agent): correct model config compatibility

Correct the models.json migration docs, preserve refresh option coverage, label logout credentials by stored type, and honor truthy offline flags.

Assistant-model: Claude Opus 5

* fix(coding-agent): restore custom model session resolution

Fix the tautological restored-model provider guard and restore the positive registered-provider regression coverage. Preserve readStoredCredential's non-throwing malformed-file contract and split ModelRuntime snapshot helpers for file-length headroom. Correct models.json migration documentation and project-scoped configuration coverage.

Assistant-model: Claude Opus 5

* fix(coding-agent): restore runtime model compatibility

Route session restoration through the restored-model resolver and narrow
unknown-model reconstruction to supported dynamic and configured providers.
Restore discriminating SDK and retired-model regression coverage.

Use the extension-facing model registry for workflow catalogs, guard the
advisory Anthropic auth check from stale credential failures, and restore the
full RPC OAuth login coverage including correlated callback dispatch.

Assistant-model: Claude Opus 5

* fix(coding-agent): correct interactive auth status

Show the Anthropic subscription warning once from stored OAuth status without triggering refresh-capable auth lookup, while keeping lookup failures non-fatal. Label login status from the stored credential type and restore regression assertions for both behaviors.

Document the removed CreateAgentSessionOptions authStorage and modelRegistry overrides.

Assistant-model: Claude Opus 5

* fix(coding-agent): restore login and startup parity

Keep thrown post-login catalog refresh failures visible without rolling back persisted OAuth credentials, while preserving successful handling of returned provider errors and aborted results. Restore the PR #2059 regression assertion and propagate interactive refresh failures.

Disable network catalog refresh during --api-key startup, migrate shipped SDK examples and documentation to ModelRuntime, and restore session-service startup timing spans.

Assistant-model: Claude Opus 5

* fix(tui): port pi's exact /model selector refresh behavior

The migrated selector diverged from pi: it dropped pi's selector-owned
15s refresh timeout, routed failures through the status line with
'showing available models' wording, rendered the snapshot through an
async promise chain, and passed allowNetwork although the runtime owns
offline gating. Port pi's implementation verbatim: synchronous snapshot
render, selector-owned timeout with 'Model refresh timed out; showing
cached models.', failures on the error surface, muted status color, and
a signal-only refresh call. Tests updated to pi semantics plus a fake-
timer timeout case and a runtime-owned network-gating assertion.

Assistant-model: Claude Fable 5

* fix(llama): adopt pi's provider-owned llama.cpp extension

The migration left the bundled llama.cpp extension on Atomic's legacy
ProviderConfig registration with a hardcoded 'local' API key, so the new
ModelRuntime treated the provider as configured for everyone: every
/model refresh attempted http://127.0.0.1:8080 and unconfigured users
saw 'Could not refresh llama.cpp; showing cached models.'

Port pi's provider.ts verbatim: native Provider registration with
provider-owned auth whose check/resolve return undefined until
LLAMA_BASE_URL or a stored login configures a server, so refresh skips
the provider entirely when unconfigured. Includes pi's /login flow
(server URL + optional API key). Ports pi's llama-extension test suite
(one assertion adapted to Atomic's unified pending-registration list),
rewrites the root parity tests against the native surface with a new
dormant-until-configured regression, and corrects the changelog's
refresh-timeout entry to pi's real semantics (selector-owned 15s
timeout).

Assistant-model: Claude Fable 5
@flora131
flora131 deleted the fix/oauth-login-refresh-parity branch August 14, 2026 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant