Preserve externally refreshed Gemini credentials - #75
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
@codex review |
|
@coderabbitai review |
📝 WalkthroughWalkthroughGemini token refresh now re-reads credentials before persistence and preserves newer externally written tokens. A deterministic test covers the in-flight update, token usage, metadata, and permissions. The changelog documents the fix. ChangesGemini credential refresh
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0489715e1b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard let latestCredentials = GeminiAuthFileStore.readCredentials(at: oauthFilePath) else { | ||
| return .transient | ||
| } | ||
| if latestCredentials != credentials { |
There was a problem hiding this comment.
When the Gemini CLI refreshes this OAuth file while CodexBar's refresh request is in flight, the POST made with the old refresh token can come back as 400/invalid_grant after the file already contains a fresh access/refresh token. Because this re-read only happens after a successful token response, that race still returns rejected and tells the user to re-authenticate instead of preferring the updated local CLI credentials.
AGENTS.md reference: AGENTS.md:L20-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 4674ec9. The provider now re-reads credentials immediately after the awaited token request, before interpreting HTTP or OAuth errors, and adopts a fresh external access token. It still re-checks immediately before persistence to cover a later write. Added a deterministic 400/invalid_grant race test; the full ./test.sh suite passes (207 tests).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4674ec9a14
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard let accessToken = latest.accessToken, | ||
| !accessToken.isEmpty, | ||
| !latest.shouldRefresh(at: now()) else { | ||
| return .unusable | ||
| } | ||
| return .valid(accessToken) |
There was a problem hiding this comment.
When this refresh is forced after a quota 401, the original.accessToken has already been rejected, but this helper treats any parsed credential change as usable as long as the on-disk access token is non-expired. If Gemini CLI or another credential writer changes only id_token, client metadata, refresh token, or expiry while leaving access_token unchanged during the token request, we return the same bearer that just got a 401, discard the successful refresh response, and the retry cannot refresh again because canRefresh is already false. Please only adopt the external update when it provides a different access token in the forced/rejected-token path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 2ae3932. Forced refreshes now adopt an external update only when it supplies a different valid access token, so metadata-only or refresh-token-only changes cannot reuse the bearer that just received a 401. Added a regression test covering that forced-refresh path; ./test.sh passes 208 tests.
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ae393202c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| switch externalCredentialUpdate( | ||
| since: credentials, | ||
| requiresDifferentAccessToken: force | ||
| ) { |
There was a problem hiding this comment.
When Gemini CLI writes oauth_creds.json in the window after this re-read returns .unchanged but before the following writeCredentials call, the newer tokens are still overwritten: writeCredentials rereads the file only to merge metadata, then unconditionally replaces the token fields with updated built from the old snapshot. This leaves the same in-flight refresh race this change is trying to prevent; the compare/merge needs to happen in the write path, or otherwise be made atomic with the write, so a post-check external update is not clobbered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 0ab86fd. The provider now uses a conditional credential-store write: under NSFileCoordinator, the store re-reads and compares the on-disk credentials with the request snapshot immediately in the write path. If they changed, it returns and preserves the external credentials instead of writing the stale refresh result. Added a deterministic post-check update regression; ./test.sh passes 209 tests.
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ab86fdc8a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard let accessToken = latest.accessToken, | ||
| !accessToken.isEmpty, | ||
| !latest.shouldRefresh(at: now()), | ||
| !requiresDifferentAccessToken || accessToken != original.accessToken else { |
There was a problem hiding this comment.
When the Gemini CLI refreshes during this request and writes a fresh access_token/expiry_date but no refresh_token, this guard classifies that file as .valid, so the callers return early and skip the merge path that preserves credentials.refreshToken. The app then leaves an access-only credential file behind; after that access token expires, resolveAccessToken rejects the account because there is no refresh token, forcing the user to re-authenticate even though the original refresh token was still available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in the latest commit. External credential adoption now conditionally writes back only missing refresh and ID tokens from the original snapshot while preserving the new external access token, expiry, and unrelated metadata. A regression covers an external access-token refresh that omits both fields; ./test.sh passes 210 tests.
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Closes #66
Summary
Verification
./test.sh(210 tests, 0 failures; release artifact and changelog smoke tests passed)git diff --checkNote
Preserve externally refreshed Gemini credentials during in-flight token refresh
GeminiUsageProvider.resolveAccessTokenchecks whether the Gemini credentials file was updated externally (e.g. by the Gemini CLI) and adopts the external tokens instead of overwriting them.GeminiAuthFileStore(writeCredentials(ifUnchangedFrom:)) usesNSFileCoordinatorto serialize writes and returns the newer on-disk credentials if the file changed since the expected snapshot, rather than clobbering them.writeCredentialsnow throws additional errors (invalidCredentialFile,unableToCoordinateWrite) and all credential writes go throughNSFileCoordinator.Macroscope summarized 7b3190d.