fix: gate CLI background usage fetch on confirmed-absent OAuth creds - #2813
Merged
Conversation
) * fix: gate CLI background usage fetch on confirmed-absent OAuth creds Add directCredentialIsMissingOverride TaskLocal for deterministic testing and thread an oauthCredentialsConfirmedAbsent check into ClaudeCLIBackgroundAvailability.allowsBackgroundAutoUsageFetch so the CLI background fallback only proceeds when direct OAuth credentials are confirmed absent. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HqBGFKF9M8etkRpgz2bk2Z * fix: honor CLI background revocation before the OAuth absence fallback allowsBackgroundAutoUsageFetch's deadlock-breaker only checked isEstablished(), which drops a marker as soon as it's revoked by a failed foreground fetch. That let the OAuth-absence probe re-permit a background CLI usage attempt on every tick after a failure, defeating the existing revocation/backoff guarantee. Check isRevoked() before falling through to oauthCredentialsConfirmedAbsent so a revoked marker stays denied until the next foreground success. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HqBGFKF9M8etkRpgz2bk2Z * fix: require an identified profile before the OAuth absence fallback allowsBackgroundAutoUsageFetch's deadlock-breaker could still fire for a profile ClaudeAccountProfile.identifiedSessionScope can't identify (missing/malformed/unreadable account config), since isEstablished and the round-1 isRevoked check both silently return false when captureMarker is nil. That let a background CLI fetch launch with no stable account binding, and a failed attempt could never be recorded as a revocation (revoke() needs a marker), so nothing would bound repeated launches. Require a non-nil marker before consulting oauthCredentialsConfirmedAbsent, matching the fail-closed contract identifiedSessionScope already documents for background work. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HqBGFKF9M8etkRpgz2bk2Z --------- Co-authored-by: axisrow <axisrow@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
|
🦞👀 Pull request received. I will update this pull request when review starts. |
![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: 907644516e
ℹ️ 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".
| // is durably dead: it is only recorded by a prior *successful* user-initiated CLI fetch, and a | ||
| // scheduled refresh never reaches user-initiated status. Breaking that deadlock here mirrors | ||
| // explicit OAuth mode's own absence check (`ClaudeOAuthPlanningAvailability`). | ||
| return oauthCredentialsConfirmedAbsent() |
There was a problem hiding this comment.
When a scheduled Auto refresh has an identifiable profile but no local OAuth cache/file, this return bypasses the existing foreground-establishment and .always prompt-policy gates. ClaudeCLIFetchStrategy.fetch then uses the .cli path, which goes directly to the interactive PTY without ClaudeCLIAuthStatusProbe; for a logged-out CLI this can open browser OAuth, and the opaque child can also surface Keychain UI, despite the refresh being in the background and the default policy being onlyOnUserAction. A confirmed absence of CodexBar-readable credentials does not establish that the interactive CLI is safe to launch unattended, so this exception should still require explicit background opt-in or a noninteractive logged-in preflight.
Useful? React with 👍 / 👎.
…back Address Codex's P1 review finding on the upstream PR: a confirmed absence of CodexBar-readable OAuth credentials does not by itself prove the interactive Claude CLI is safe to launch unattended in the background. The deadlock-breaker in allowsBackgroundAutoUsageFetch now requires the same explicit background opt-in (.always prompt policy) that allowsOpaqueChildExecution already requires, instead of relying solely on the credential-absence probe. Update tests to cover both the opted-in and not-opted-in cases. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HqBGFKF9M8etkRpgz2bk2Z
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs real behavior proof before merge. Reviewed August 12, 2026, 11:57 AM ET / 15:57 UTC. ClawSweeper reviewWhat this changesThe PR restricts unattended Claude CLI fallback to confirmed absent OAuth credentials and retains a prior cached-session error when browser-cookie recovery cannot replace it. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 6 items remain Keep open: current main and v0.49.2 still lack the proposed Claude fallback behavior, but the PR continues to mask errors from a successfully discovered replacement browser session. Likely related people: Peter Steinberger and pickaxe (medium confidence). Priority: P1 Review scores
Verification
How this fits togetherCodexBar’s Claude provider selects OAuth, CLI, and browser-cookie sources for manual and scheduled usage refreshes. The changed paths control background CLI eligibility and the error reported after a cached web session expires. flowchart LR
A[Scheduled Claude refresh] --> B[Provider strategy selection]
B --> C[OAuth credential probe]
C --> D[CLI background eligibility]
B --> E[Cached browser session]
E --> F[Browser cookie recovery]
D --> G[Usage result]
F --> G
Decision needed
Why: This changes the background credential and Keychain-interaction boundary; source can verify the guard mechanics, but accepting that consent policy requires maintainer intent. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Return the initial cached-session error only when replacement cookie discovery fails; once a replacement session is found, surface its fetch result while retaining the existing explicit background opt-in boundary. Do we have a high-confidence way to reproduce the issue? Yes. With the existing test seam, invalidate the cached cookie, return a replacement session from browser discovery, and fail its usage request; the changed catch returns the stale error instead of that replacement-session failure. Is this the best way to solve the issue? No. The catch must cover cookie discovery only; a fetch attempted with a discovered replacement session should propagate its own result or error. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against c4ed34d0e44a. 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 (7 earlier review cycles)
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
ClaudeWebAPIFetcher.fetchUsageSerialized cleared a stale cached session cookie on an auth failure and unconditionally fell through to a browser cookie read to recover, exactly the pattern fixed for Ollama in PR steipete#2814. In a background context BrowserCookieAccessGate typically denies that read (no interactive Keychain prompt outside a user-initiated action), so extractSessionKeyInfo finds no session key and the generic 'no session key found' error replaces the original, more informative cached-auth error (e.g. the OAuth-absence message) — surfacing as the same intermittent flicker already reported for Claude. Still attempt browser-cookie recovery after clearing the stale cache, even in a background context — BrowserCookieAccessGate already gates that read on its own no-UI preflight (Safari never needs Keychain decryption, and a Chromium browser with a prior 'Always Allow' Keychain grant is read without a prompt) — so a background attempt is not unconditionally blocked. Only when that attempt also fails do we now surface the original cached-auth error instead of the misleading generic one. Adds ClaudeWebBackgroundRecoveryTests covering: background recovery that finds nothing (original error surfaces), background recovery that succeeds without a prompt (still works), and the equivalent user-initiated case.
Contributor
Author
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
…ests The background-Auto deadlock-breaker requires an identified Claude profile (captureMarker -> identifiedSessionScope -> accountConfigURL). Without CLAUDE_CONFIG_DIR in the test environment, accountConfigURL falls back to the host's real ~/.claude.json, so these tests passed only on machines with a signed-in Claude CLI and failed closed on CI where that file is absent. Give each affected test its own identified profile via an isolated CLAUDE_CONFIG_DIR so they prove the gate's behavior instead of the host's sign-in state.
Merging main pushed ClaudeBaselineCharacterizationTests past the 800-line type_body_length limit; move the explicit source-mode resolution and token heuristic tests into a MARK-ed extension.
Same host-state leak as the baseline suite: without CLAUDE_CONFIG_DIR the deadlock-breaker's marker guard reads the host's real ~/.claude.json, so the test passed only on signed-in dev machines and failed on CI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes two independent causes of the intermittent "Claude OAuth credentials not found. Run
claudeto authenticate." flicker (#2708), both of which surface the same misleading error while the user is actually still authenticated:1. CLI-background deadlock (original scope). After a signed-out/failed background Claude CLI attempt, the background Auto usage-fetch path could stay permanently blocked (or, depending on the exact failure sequence, get silently re-permitted) because the OAuth-absence deadlock-breaker did not consistently account for revoked or unidentified profiles. A fresh sign-in would work briefly and then the provider would fall back to "No available fetch strategy" again.
directCredentialIsMissingOverrideTaskLocal toClaudeOAuthFetchStrategyfor deterministic test overrides.oauthCredentialsConfirmedAbsentcheck intoClaudeCLIBackgroundAvailability.allowsBackgroundAutoUsageFetchso the CLI background fallback only fires when direct OAuth credentials are confirmed absent.2. Background web-cookie recovery (added in this update).
ClaudeWebAPIFetcher.fetchUsageSerialized— the Claude web/cookie usage path — clears a stale cached session cookie on an auth failure and unconditionally falls through to a browser cookie read to recover, the exact same shape as the bug fixed for Ollama in #2814 (ollama.com's equivalent report was #2072). In a background contextBrowserCookieAccessGatetypically denies that read (no interactive Keychain prompt outside a user-initiated action), so the browser lookup finds nothing and a generic "no session key found" error replaces the original, more informative cached-auth error (surfacing as this same flicker). Fixed with the identical pattern used for Ollama:BrowserCookieAccessGatealready gates that read on its own no-UI preflight (Safari never needs Keychain decryption, and a Chromium browser with a prior "Always Allow" Keychain grant is read without a prompt), so a background attempt is not unconditionally blocked.ClaudeWebBackgroundRecoveryTestscovering: background recovery that finds nothing (original error surfaces), background recovery that succeeds without a prompt (still works, proving the fix doesn't block legitimate no-UI recovery), and the equivalent user-initiated case.Reviewed over 3 local review cycles (
/review+ a Codex adversarial-review companion) before opening; two real logic gaps were found and fixed, remaining findings were verified as either duplicates of already-accepted deliberate behavior or non-issues.Closes #2708
Test plan
swift test --filter ClaudeCLIBackgroundAvailabilityTests(14/14 passing)swift test --filter ClaudeWebBackgroundRecoveryTests(3/3 passing)swift test --filter ClaudeWebCookieRenewalTests(15/15 passing, no regressions)make lint(0 violations)swift testsuite (large parallel run is flaky on this machine due to unrelated timing-sensitive tests under CPU load — e.g.AdaptiveRefreshTimerTests,ClaudeOAuthCredentialsStoreNeverPromptCacheTests' subprocess-based tombstone tests; verified these same suites fail identically on the pre-fix commit, i.e. unrelated to this change. All suites touched by this PR pass cleanly in isolation.)🤖 Generated with Claude Code