fix(oauth): clear the Anthropic refresh intent after a transient failure - #3111
Conversation
(cherry picked from commit 3e7c5fd)
A timeout, lost response, unreadable body, or credential-store failure can happen after Anthropic consumed and rotated a refresh token. Clearing the durable intent lets the next attempt replay the old token, risking reuse handling and forced reauthentication. The store's uncertain flag does not cover these request outcomes. Track the pre-dispatch boundary explicitly. Clear a non-terminal intent only before dispatch or after the adapter reports an explicit non-success HTTP response; retain it after all other post-dispatch outcomes and after provider success followed by persistence failure. Intent cleanup is secondary to the refresh result. A failed unlink now leaves the replay guard in place without replacing the original provider error, pre-dispatch abort, terminal login result, or successfully persisted credential. Cover definite 503 retry, uncertain post-dispatch failure, pre-dispatch abort, cleanup failure, post-provider persistence failure, and post-persist cleanup failure. (cherry picked from commit 9b55278)
Persist an attempt-scoped cleanup-pending marker before returning a definitive rejection or pre-dispatch abort. Serialize all refresh-intent mutations with the existing SQLite config transaction, refuse guard overwrites, and retry only the exact safe cleanup before provider redispatch. Preserve ordinary intents for timeouts and post-provider persistence uncertainty. (cherry picked from commit 97a547c)
…edential When Anthropic adopts a newer Claude Code credential, mergeAccountCredential persists it before the observed refresh intent is cleaned up. Both exact-match and generation-based cleanup rethrow non-ENOENT unlink errors, so a locked or read-only intent file turned a successful adoption into a rejected refresh even though the credential was already durable on disk. Those two post-commit sites now use the existing best-effort helper, which logs and preserves the replay guard instead of throwing. The two remaining throwing calls are unchanged: their return values gate stale-flight handling before a refresh is dispatched, so their failures must still surface. The refresh-intent file also carried its own snapshot type and identity check that duplicated the OAuth file lock's snapshot/sameSnapshot pair, differing only in a field name. Both are compare-and-swap decisions on secret-adjacent files, so they now share one definition rather than two copies that can drift. Regression: a cleanup failure during disk-credential adoption still resolves with the committed token, attempts cleanup, performs no network refresh, and does not mark the account needsReauth. Reverting the first hunk fails it with a rejected promise. (cherry picked from commit f45fb1c)
(cherry picked from commit eb4639b)
(cherry picked from commit 5ca2172)
(cherry picked from commit 66780c6)
(cherry picked from commit 60c9d76)
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughOAuth refresh handling now persists validated attempt metadata and cleanup markers under a shared mutation lock. Anthropic refreshes distinguish definitive rejection from uncertain outcomes, use compare-and-swap cleanup, and preserve replay guards when provider state is unknown. Tests cover failure, recovery, and race conditions. ChangesOAuth refresh durability
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to This PR improves recovery from stale Anthropic refresh markers, but it can clear replay protection after a 5xx response even when the provider may already have consumed the token, potentially causing token reuse and reauthentication; cleanup persistence failures can also leave an account blocked. Merge should wait for these recovery and response-classification cases to be addressed or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant RefreshFlow
participant IntentStore
participant AnthropicProvider
participant CredentialStore
RefreshFlow->>IntentStore: Write refresh intent with attemptId
RefreshFlow->>IntentStore: Mark cleanup pending before dispatch
RefreshFlow->>AnthropicProvider: Dispatch refresh request
AnthropicProvider-->>RefreshFlow: Return credential or definitive rejection
RefreshFlow->>CredentialStore: Persist refreshed credential
RefreshFlow->>IntentStore: Clear intent with compare-and-swap checks
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Title checkExplanation The title describes the OAuth refresh-intent change and identifies Anthropic and transient failures. It is related to the primary changes, although it does not mention the important distinction between safe cleanup and uncertain post-dispatch outcomes.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
리뷰 · 우선순위 70 / 80설명 이 PR은 기여자 @luvs01 의 #2989(maintainer-sponsored)를 지금 지금 체크아웃의 기존 테스트 변경량이 라인 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
Administrator bypass and security-review record\n\nOwner authorization was explicit in Codex session 01a05a34-1e3a-73f2-8607-15e517cbec11 on 2026-09-01 KST. Exact head: c418819. Current exact-head check rollup: 0 failing, 0 pending. Independent security review PASS: refresh-intent cleanup adds no token logging, replay bypass, unsafe race, or secret exposure; all three changed files were covered. Using the maintain/admin pull_request bypass documented in MAINTAINERS.md because the author cannot approve their own PR. The bypass is recorded; it does not waive CI or security review. |
2 participants
Summary
Carries #2989 (author @luvs01,
maintainer-sponsored) rebased onto currentdev. All eight commits are the author's and needed no changes.refreshAnthropicAccountWithLockwrites a durable refresh intent before dispatching, then rethrows non-terminal errors without clearing it (src/oauth/index.ts:681-702ondev).terminal()covers only 400/401 plus grant/revoke codes, so a plain 503 left the marker on disk. The next attempt saw a same-generation intent and treated the token as possibly consumed —OAuthLoginRequiredError, manual reauth, for one transient upstream failure.The intent is now cleared when the refresh either never reached the provider or was definitively answered, and kept when the outcome is genuinely uncertain. Cleanup is durable and resumable, so a failed unlink is retried rather than masking a credential that did land.
Why the existing test did not catch this
Anthropic transient failures do not mark needsReauth(tests/oauth-refresh.test.ts:500-508ondev) asserts only the first throw. It never re-entersrefreshAnthropicAccountWithLock, and re-entry is where the stale intent does its damage — so it stays green on the broken path. The added tests drive the second attempt.Verification
Both directions of the new condition are mutation-checked, each restored:
if (attemptIntent))an Anthropic refresh with an uncertain post-dispatch outcome preserves its intentand the persistence-failure replay guardif (false && ...))a definitive transient Anthropic HTTP failure leaves the account refreshableand the three cleanup-retry testsTwo mutations, two disjoint failure sets: over-clearing risks replaying a token that may already have rotated, under-clearing is the reported outage. Both sides are guarded, which is the property this change needs to have.
Security note
This touches credential refresh and OAuth state, so it needs the security review
MAINTAINERS.mdrequires. The PR carriesmaintainer-sponsoredfrom #2989. Nothing here logs or serializes a token;definitivelyAnsweredreads onlyAnthropicTokenError.httpStatus, whichpostJsonattaches on an explicit non-success response.Checklist
bun x tsc --noEmitcleanTriaged in the 2026-08-31 non-priority-70 bug round; supersedes #2989 by rebase only.
Summary by CodeRabbit