Skip to content

fix(claude-auth): keep refresh failures from clobbering fresh login - #9754

Closed
rodboev wants to merge 1 commit into
stablyai:mainfrom
rodboev:pr/claude-refresh-failure-clobber
Closed

rodboev wants to merge 1 commit into
stablyai:mainfrom
rodboev:pr/claude-refresh-failure-clobber

Conversation

@rodboev

@rodboev rodboev commented Jul 21, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Stop ClaudeRuntimeAuthService from rewriting shared Claude runtime auth with an already-expired managed snapshot after a failed proactive refresh.
  • Keep a failed switch into another expired managed account from leaving the previous runtime footprint active in shared Claude auth.
  • Keep the successful-refresh materialization path and the live-PTY defer/read-back path unchanged. Caller policy stays in ClaudeRuntimeAuthService; ClaudeAccountService and index.ts still route through syncForCurrentSelection() and prepareForRateLimitFetch() without a new bypass.
  • Scope stays limited to the local stale-materialization invariant behind issue [Bug]: anthropic 401 #9582. The broader token-family revocation and multi-writer race remain out of scope.

Refs #9582.

Screenshots

No visual change

Testing

  • pnpm lint
  • pnpm typecheck
  • pnpm test
  • pnpm build
  • Added or updated high-quality tests that would catch regressions, or explained why tests were not needed

Focused validation:

  • pnpm exec vitest run --config config/vitest.config.ts src/main/claude-accounts/runtime-auth-service.test.ts - pass, Tests 100 passed (100)
  • pnpm run typecheck:node - pass, tsc --noEmit -p config/tsconfig.node.json
  • pnpm exec oxlint src/main/claude-accounts/runtime-auth-service.ts src/main/claude-accounts/runtime-auth-service.test.ts - pass, no diagnostics emitted
  • pnpm exec oxfmt --check src/main/claude-accounts/runtime-auth-service.ts src/main/claude-accounts/runtime-auth-service.test.ts - pass, All matched files use the correct format.

AI Review Report

Reviewed surface is limited to the Claude managed-account runtime sync in main-process auth code. The change keeps Linux and Windows runtime-file behavior intact for valid credentials, prevents darwin keychain writes only when the selected managed credential is still expired after refresh, leaves renderer, SSH, remote-runtime, and provider-routing logic untouched, and adds no UI or performance claims.

Security Audit

The change stays inside the existing runtime-auth service, adds no new commands, IPC, or path inputs, and fails closed by refusing to promote an expired managed credential after refresh failure. No new secret surfaces, subprocesses, or trust-boundary crossings are introduced beyond the existing runtime credential and keychain materialization path.

Notes

Attribution: the current-tree reproduction and implementation sketch came from #9582 (comment).

Scope boundary: this PR does not claim to fix the broader token-family revocation or cross-process multi-writer race described in the upstream issue.

Proof points:

  • The refresh-failure repro failed on origin/main, where the stale managed credential overwrote the fresher runtime login, and passed on this branch.
  • A second refresh-failure repro with non-numeric expiresAt metadata also failed on origin/main and preserved the fresher runtime login on this branch.
  • The same repro showed the darwin runtime keychain write on origin/main and no write on this branch.
  • The successful-refresh materialization path still passed on this branch.
  • Switching from one managed account to another no longer leaves the previous runtime footprint active when the incoming expired account fails refresh.
  • The same failed-switch cleanup still restores or clears the previous runtime footprint when the last synced account disappeared from settings before the incoming expired account failed refresh.
  • The live-PTY wiped-runtime boundary still passed on this branch with an expiring managed credential, so deferred refresh does not suppress rematerialization.
  • The live-PTY deferred-refresh boundary still passed on this branch.

ELI5

A failed Claude token refresh could overwrite a good login with an expired snapshot and log you out. Failed refreshes no longer clobber fresh shared auth; successful refresh paths stay the same.

@coderabbitai

coderabbitai Bot commented Jul 21, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: e8155ec9-c173-4ce8-867f-1ee08026d5a0

📥 Commits

Reviewing files that changed from the base of the PR and between 28a31aed8e6e59ecdfc109049abf5ee67e29f742 and 96a40f5.

📒 Files selected for processing (2)
  • src/main/claude-accounts/runtime-auth-service.test.ts
  • src/main/claude-accounts/runtime-auth-service.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/main/claude-accounts/runtime-auth-service.ts
  • src/main/claude-accounts/runtime-auth-service.test.ts

📝 Walkthrough

Walkthrough

Claude runtime synchronization now avoids materializing managed credentials when an OAuth refresh fails and credentials remain expired or have a non-numeric expiration value. Tests cover runtime and persisted state restoration, live-PTY rematerialization behavior, typed expiration mocks, and one-time refresh mock responses.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: preventing refresh failures from clobbering fresh Claude auth.
Description check ✅ Passed All required sections are present and mostly complete, though the AI Review Report could more explicitly detail cross-platform checks.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/main/claude-accounts/runtime-auth-service.test.ts (1)

3872-3883: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Prevent mock state pollution by resetting mocks in a finally block.

In both new tests, if a synchronous error occurs or an assertion fails, the mock reset for isOauthTokenExpiring will not execute. This leaves the mock in a persistently modified state that can cause cascading failures in subsequent tests.

  • src/main/claude-accounts/runtime-auth-service.test.ts#L3872-L3883: wrap lines 3874-3881 in a try block and move the vi.mocked(isOauthTokenExpiring).mockReturnValue(false) reset into a finally block.
  • src/main/claude-accounts/runtime-auth-service.test.ts#L3926-L3937: wrap lines 3928-3935 in a try block and move the vi.mocked(isOauthTokenExpiring).mockReturnValue(false) reset into a finally block.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: fafd6ebb-bbfe-4920-acd6-000124e18c51

📥 Commits

Reviewing files that changed from the base of the PR and between 827cd49 and e6001f78c2e9725a765d9ff98d0521c31636c778.

📒 Files selected for processing (2)
  • src/main/claude-accounts/runtime-auth-service.test.ts
  • src/main/claude-accounts/runtime-auth-service.ts

@rodboev
rodboev force-pushed the pr/claude-refresh-failure-clobber branch from e6001f7 to 28a31ae Compare July 21, 2026 17:00

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/main/claude-accounts/runtime-auth-service.test.ts (1)

3872-3882: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Guard the isOauthTokenExpiring mock reset with try/finally.

beforeEach only calls vi.clearAllMocks(), which won’t restore a mockReturnValue(true). If this test fails before the reset at line 3882, the always-expiring mock can leak into later tests and cause cascading failures.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 00c5b477-5866-4501-8568-575ac1924bfe

📥 Commits

Reviewing files that changed from the base of the PR and between e6001f78c2e9725a765d9ff98d0521c31636c778 and 28a31aed8e6e59ecdfc109049abf5ee67e29f742.

📒 Files selected for processing (2)
  • src/main/claude-accounts/runtime-auth-service.test.ts
  • src/main/claude-accounts/runtime-auth-service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/main/claude-accounts/runtime-auth-service.ts

@AmethystLiang

Copy link
Copy Markdown
Contributor

thanks for reporting. looking into it

@nwparker

Copy link
Copy Markdown
Contributor

Thanks for putting this together, and sorry for the slow turnaround. The area has since been reworked and this is now covered on main by #26801 (per-account CLAUDE_CONFIG_DIR and scoped Keychain items, replacing the old credential swap/OAuth refresh code), so we're closing this as superseded. If you still see the problem on the latest build, please open a fresh report or reopen with a current repro. Thank you for the contribution.

@nwparker nwparker closed this Oct 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants