Add Grok token refresh - #3473
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
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. |
There was a problem hiding this comment.
🟡 Changes recommended
The refresh flow can send whitespace-padded refresh tokens due to inconsistent trimming/validation, which can cause avoidable refresh failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Grok OIDC access-token refresh support (via xAI’s OAuth token endpoint) and persists refreshed credentials back into auth.json, improving resilience when Grok logins expire.
Changes:
- Introduces
GrokTokenRefresherto POST an OAuth refresh request, parse token responses, and update credential timestamps. - Adds
GrokCredentials.needsRefresh(5-minute leeway) plus aGrokCredentialsStore.savemerge that preserves sibling scopes/unknown fields. - Adds a focused test suite covering refresh request formatting, rotation behavior, rejection handling, and on-disk merge behavior.
File summaries
| File | Description |
|---|---|
| Tests/CodexBarTests/GrokTokenRefresherTests.swift | Adds coverage for refresh request construction, token rotation, refresh gating, and auth.json merge behavior. |
| Sources/CodexBarCore/Providers/Grok/GrokTokenRefresher.swift | Implements the refresh flow, response parsing, and conditional refresh of stored credentials. |
| Sources/CodexBarCore/Providers/Grok/GrokAuth.swift | Adds refresh gating logic and a save path to merge refreshed tokens back into auth.json. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| guard let refreshToken = credentials.refreshToken, !refreshToken.isEmpty else { | ||
| throw RefreshError.missingRefreshToken | ||
| } |
| guard let expiresIn = Self.expiresIn(from: json["expires_in"]), expiresIn > 0 else { | ||
| throw RefreshError.invalidResponse("Missing expires_in") | ||
| } |
![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: 19fec69467
ℹ️ 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".
| teamId: credentials.teamId, | ||
| oidcIssuer: credentials.oidcIssuer, | ||
| oidcClientId: credentials.oidcClientId, | ||
| expiresAt: now.addingTimeInterval(expiresIn), | ||
| createTime: now) |
There was a problem hiding this comment.
When refreshing credentials parsed from a team-scoped auth.json, this initializer omits principalType, so the returned value silently defaults it to nil. Any caller that immediately uses the refreshed credentials will then fail isTeamPrincipal; downstream classification in GrokWebBillingFetcher.classified will no longer convert the team's unsupported billing response into teamUsageUnsupported, preventing the identity-only fallback in GrokWebFetchStrategy.fetch. Pass through credentials.principalType and cover the team refresh case.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
P2
|
Codex review: needs real behavior proof before merge. Reviewed September 7, 2026, 12:04 PM ET / 16:04 UTC. ClawSweeper reviewWhat this changesAdds a Grok token-refresh helper, a five-minute expiry predicate, and six tests, while leaving credential persistence and live usage fetching unchanged. Merge readiness⛔ Blocked before merge - 3 items remain Keep open: the narrowed helper is useful preparatory work, and the earlier persistence and principal findings are resolved. Real xAI exchange proof remains outstanding; repository policy also disables automatic cleanup closure. Likely related people: Vincent Peng and Peter Steinberger (unverified routing candidates). Priority: P2 Review scores
Verification
How this fits togetherCodexBar reads Grok login credentials to fetch usage and display account information. The new standalone helper exchanges a refresh token with xAI and returns renewed credentials, but the existing usage pipeline does not call it. flowchart TD
A[Grok credentials] --> B[Expiry predicate]
A --> C[Standalone refresh helper]
C --> D[xAI token endpoint]
D --> E[Renewed credentials or error]
A --> F[Existing usage pipeline]
F --> G[Usage and account display]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep renewal isolated from CLI-owned credential storage, establish the real exchange contract, and address automatic usage integration separately with explicit credential coordination. Do we have a high-confidence way to reproduce the issue? Not applicable to the narrowed helper addition; source confirms that automatic refresh is absent, but no real token exchange was reproduced. Is this the best way to solve the issue? Yes for the stated preparatory scope: the helper reuses the shared transport and avoids competing credential-file writers. It does not yet solve expired logins in the live usage pipeline. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 170a4d41c6d6. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (4 earlier review cycles)
|
|
@clawsweeper re-review Addressed the P1 stale-refresh and P2 principal findings plus Copilot's trim/message nits. Stale results are now rejected by re-reading auth.json after the network await (intervening login wins, removed file stays removed), covered by new tests. Details in the updated PR body. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review Removed the persistence path per the ownership finding: no more save/stored-refresh helper, so CodexBar never writes auth.json and the final-write race is gone by construction. The PR is now the pure refresh call plus needsRefresh, covered by tests. The remaining live-proof and wiring items are owner follow-ups. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
removed
merge-risk: 🚨 auth-provider
99c86ab to
76bc6e0
Compare
|
Live-load evidence (read-only, real account): the installed 0.56.8 build reads Live renewal proof is intentionally not included: spending the refresh token can invalidate the CLI's stored session, so that step needs the owner's explicit go-ahead. The refresh call itself is covered by stubbed-transport tests. |
|
Thank you for investigating expired Grok sessions. This revision adds an unused refresher without production recovery or persistence, while refresh-token exchange may invalidate the CLI session. Closing the standalone helper; an integrated repair needs a credential-owner coordination contract and actual expired-session recovery proof. |
|
Integrated successor: #3497 (draft). It now calls the installed CLI owner from the production expired-session path, reloads the persisted credentials, and resumes billing. A live xAI refresh rotated both tokens; the CLI subsequently reused the new token successfully. CodexBar does not exchange refresh tokens or write auth.json. I have kept the successor in draft: the credential-owner account-change/logout coordination guarantee is still a release blocker, and the full-suite timeout is disclosed in the validation section. This is not a request to merge the standalone helper. |
Rotates expired Grok OIDC access tokens via the xAI refresh endpoint (pure refresh call plus a needsRefresh predicate with 5-min leeway).
Previously the parsed refresh token was never used, so expired logins just failed.
Ownership note: CodexBar deliberately does not write auth.json — the CLI remains the sole writer, so there is no cross-process write race by construction. Persistence and live fetch wiring are follow-ups once a CLI coordination contract exists.
Verified: GrokTokenRefresherTests 6/6, ProviderArchitectureGatekeeperTests 41/41, SwiftFormat/SwiftLint clean.