Fix Grok browser cookie refresh - #2458
Conversation
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs maintainer review before merge. Reviewed July 28, 2026, 2:04 AM ET / 06:04 UTC. ClawSweeper reviewWhat this changesThis PR allows an explicit Grok CLI cookie refresh to import and cache a validated browser session, then reuses it for later app and CLI usage fetches while evicting it only after authentication failures. Merge readinessKeep this PR open for normal maintainer review. It has focused implementation, regression coverage, and convincing redacted macOS proof; the remaining merge-critical point is confirming that the expanded Priority: P2 Review scores
Verification
How this fits togetherCodexBar’s Grok usage provider combines local Grok credentials with an optional browser-backed session to fetch billing data. The changed path decides whether a request may read browser cookies, validates and caches that session, and returns full usage or an identity-only fallback. flowchart LR
A[CLI or app request] --> B[Interaction context]
B --> C{Cached Grok session}
C -->|Valid| D[Billing usage fetch]
C -->|Missing or auth failed| E{Browser import allowed}
E -->|Explicit refresh or app| F[Validate browser session]
E -->|Background or ordinary CLI| G[Credential fallback]
F --> H[Store validated session]
H --> D
D --> I[Usage or identity-only result]
Decision needed
Why: The patch is narrow and proven, but this condition becomes a shared authorization boundary whose safety depends on all present and future producers preserving its direct-user-action meaning. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Keep browser-cookie import restricted to the app and explicit refresh actions, retain cache reuse for background requests, and preserve the documented authentication-only eviction behavior with the added regression coverage. Do we have a high-confidence way to reproduce the issue? Yes, with medium confidence: the supplied final-branch macOS transcripts exercise the reported cache miss, explicit refresh, cache commit, stale-session eviction, and later cache reuse; the base-path behavior is also directly reflected by the changed provider gate. Is this the best way to solve the issue? Yes, conditionally: caching only validated sessions and granting browser import for an explicit refresh is a narrow solution, provided maintainers accept the shared AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 5e233b2efcb2. 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
Bot
added
proof: sufficient
![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: 9e3ada765d
ℹ️ 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".
| credentials: browserCredentials) | ||
| return (snapshot, cached.sourceLabel, false) | ||
| } catch { | ||
| guard Self.isCookieAuthenticationFailure(error) else { throw error } |
There was a problem hiding this comment.
When auth.json contains a non-expired team principal and the cached cookie has expired, the cookie-plus-bearer attempt can return teamUsageUnsupported while the subsequent cookie-only attempt returns 401. fetchValidCookieHeader prioritizes the saved team error, so this guard treats the cached attempt as non-authentication-related and exits before importing the newly signed-in browser session. Regular app refreshes then remain stuck on the stale cache and publish identity-only data until the user runs an explicit cookie refresh; preserve the team error only after fresh browser sessions have also been attempted.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦐 gold shrimp
A cached team-limited session whose trailing cookie-only attempt fails with a non-authentication error (gRPC 9 "No personal team") must keep degrading to identity-only data instead of failing outright and pinning a cache entry that authentication-failure eviction can never remove. Trailing errors now win over the team classification only when they are authentication failures, so stale cached sessions still evict and re-import. Also reads the cached entry from the conditional-mutation observation (single Keychain read, matching the Claude web fetcher), documents the browser-cookie import contract and cache behavior, and resets the display cache in the cookie tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the remaining review items in 87cffde and refreshed the PR body with final-head proof: Security: Cached team-cookie fallback (both directions). On top of the earlier fix that lets a stale team cookie surface its trailing 401 for eviction, 87cffde keeps Final-head live proof. The PR body now shows, on binary @clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. |
clawsweeper
Bot
added
proof: sufficient
|
Merged after full verification. Per the owner-approved semantics: an explicit CLI cookie-refresh counts as consent to Chromium import, validated Grok sessions are cached in Keychain and reused for background/CLI fetches, and only authentication failures evict (team-unsupported responses keep the cache). Review confirmed the shared CookieHeaderCache conditional-mutation pattern and that fresh imports stay consent-gated. Exact-head CI: all checks green (https://github.com/steipete/CodexBar/actions/runs/30333376489, shard 1 completed after runner backlog). Autoreview clean. Thanks @olddonkey — nice first contribution! |
2 participants
Summary
teamUsageUnsupportedclassification for cached sessions whose trailing cookie-only attempt fails with a non-authentication error, so team principals continue degrading to identity-only data instead of pinning an ineradicable failing cache entry.Root cause
The generic cookie refresh command marks provider work as user initiated, but the Grok strategy still rejected every CLI browser import unless CODEXBAR_ALLOW_BROWSER_COOKIE_IMPORT was set manually. After a browser session did validate, Grok also did not store it in CookieHeaderCache, so the refresh command had no staged cookie to commit and background fetches could not reuse the session behind the Chromium Keychain gate.
Two follow-up fixes hardened the cached path:
teamUsageUnsupportedclassification and never evict. The cached path now prefers a trailing authentication failure so stale sessions evict and re-import.teamUsageUnsupportedis preserved.Security note:
userInitiatedimport boundaryProducers of
ProviderInteractionContext.userInitiatedwere audited for this change: in the CLI, only the explicitcookie refreshcommand sets it (CLICookieCommand.performCookieRefreshes);usageandserveexplicitly bind.background. In the app, every setter is a direct UI gesture, and the app runtime was already permitted viaruntime == .app, so the new clause only affects explicit CLI refreshes. Scheduled/background work inherits the.backgroundtask-local default. The contract is now documented oncanImportBrowserCookies.Live behavior proof
Validated on macOS with the final branch binary (
87cffdea) and a real signed-in Chrome profile. Cookie values, usage values, reset timestamps, and account data are redacted.1. Ordinary CLI reuses the validated cached session without browser-import opt-in
No browser-cookie import or Keychain-attempt log was emitted.
2. A stale cached session is evicted on a real authentication failure
For this check, the cached entry's
cookieHeaderwas replaced with an invalid value (entry shape untouched); the Chrome source session was left signed in.grok.com rejected the invalid session with an authentication failure, the entry was evicted, and the run fell through to the auth-file probe (absent on this machine). The next run proves the eviction and that ordinary CLI work stays gated — it exited 1 with no browser-cookie import or Keychain-attempt log:
3. Explicit refresh imports, validates, and commits a fresh browser session
4. The committed session is reused by the next ordinary CLI fetch
The team-cookie matrix (trailing 401 evicts; trailing gRPC 9 keeps the team classification) is covered by unit tests; no team account was available for a live team run.
Validation
swift test --filter "GrokWebBillingFetcherTests|CLICookieRefreshTests"(52 tests in 2 suites passed on87cffde)make check(clean: 0 violations in 1581 files)make testwas previously blocked at group 48/61 by four unrelated singularization expectations in SessionEquivalentForecastTests (1 windowsversus1 window); this PR does not touch the forecast implementation or tests.