Fix Claude source mode fallback on successful CLI login (#2403) - #2414
Fix Claude source mode fallback on successful CLI login (#2403)#2414Yuxin-Qiao wants to merge 3 commits into
Conversation
![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: d1dc6bd8e5
ℹ️ 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".
| let metadata = self.store.metadata(for: .claude) | ||
| self.settings.setProviderEnabled(provider: .claude, metadata: metadata, enabled: true) | ||
| if self.settings.claudeUsageDataSource == .web { | ||
| self.settings.claudeUsageDataSource = .auto |
There was a problem hiding this comment.
When a saved Claude token account contains a web/session cookie, this assignment changes an explicit web configuration to auto but leaves that token account selected. On the next app refresh, auto mode can pick the newly logged-in Claude Code OAuth/CLI credential before the selected account’s web cookie, while refresh publication still treats the selected token account as current and labels/caches that ambient snapshot under the saved account. In that configuration, users can see the CLI account’s usage under the selected web account; either clear/switch the selected token account or avoid auto-switching while a selected web-cookie account owns the refresh.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9471a060c9
ℹ️ 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".
| let metadata = self.store.metadata(for: .claude) | ||
| self.settings.setProviderEnabled(provider: .claude, metadata: metadata, enabled: true) | ||
| if self.settings.claudeUsageDataSource == .web { | ||
| self.settings.claudeUsageDataSource = .auto |
There was a problem hiding this comment.
When a user had explicitly selected Web but also has a Claude Admin API key saved or in the environment, this switch to Auto means the refresh triggered after a successful Claude Code login no longer uses the credential they just logged in with: ClaudeProviderDescriptor.resolveStrategies returns the admin API strategy immediately for Auto + admin key before reaching the OAuth/CLI planner. In that configuration the menu shows Admin API spend instead of the Claude Code account the login flow just authenticated; consider switching to a Claude Code-specific source or suppressing the admin-key auto shortcut for this post-login migration.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs real behavior proof before merge. Reviewed August 1, 2026, 5:46 AM ET / 09:46 UTC. ClawSweeper reviewWhat this changesThe PR changes a successful Claude CLI login from explicit Web to explicit OAuth, updates its login-flow policy test, and adds a Linux subprocess launch retry. Merge readiness⛔ Blocked until stronger real behavior proof is added - 10 items remain Keep this PR open: current Priority: P1 Review scores
Verification
How this fits togetherClaude login enables the Claude provider and stores a usage-source preference. The Claude provider descriptor then turns that preference and available credentials into ordered fetch strategies whose usage result is shown in CodexBar’s menu. flowchart LR
A[Claude CLI login] --> B[Login flow]
B --> C[Claude source preference]
C --> D[Provider strategy planner]
D --> E[OAuth or CLI usage fetch]
E --> F[Usage store]
F --> G[Menu bar usage display]
Decision needed
Why: Changing the stored preference affects account identity and provider routing; source inspection proves strict OAuth is wrong for the reported failure, but it does not establish which persistent account-selection policy maintainers want. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Define and implement a post-login source policy that preserves the authenticated Claude account and a CLI fallback under unavailable Keychain access, restore stable test cleanup, and prove the full transition with redacted runtime evidence. Do we have a high-confidence way to reproduce the issue? Yes for the branch defect: the changed login path writes Is this the best way to solve the issue? No. Strict OAuth cannot solve the stated unavailable-Keychain fallback, and a correct replacement must also preserve the intended selected-account and Admin API routing policy. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 299385a7a8f1. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (11 earlier review cycles; latest 8 shown)
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
60e9173 to
987f6d6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 987f6d696d
ℹ️ 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".
| if self.settings.claudeUsageDataSource == .web { | ||
| self.settings.claudeUsageDataSource = .oauth |
There was a problem hiding this comment.
When Claude is disabled—the setup used by both login-flow tests and a normal sign-in onboarding path—replacing the unconditional setProviderEnabled(..., enabled: true) call with this source-only update leaves ProviderConfig.enabled false. Changing claudeUsageDataSource does not alter enablement, so the flow reports success and posts a notification while Claude remains hidden and is not refreshed; the new isProviderEnabledCached assertions consequently fail. Restore provider enablement independently of the selected source.
Useful? React with 👍 / 👎.
| #expect(actions.contains { | ||
| $0.0 == "Re-login at claude.ai" && $0.1 == .loginToProvider(url: "https://claude.ai/") |
There was a problem hiding this comment.
After the successful flow changes this test's source from .web to .oauth, shouldOpenBrowserForWebSessionError rejects the source because it only accepts .auto or .web. Injecting a web unauthorized error therefore cannot produce the asserted “Re-login at claude.ai” action, so this test will fail once the earlier enablement assertion is repaired and does not validate the new routing behavior; assert the OAuth behavior or verify that the obsolete web action is absent instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eaf2a3ef70
ℹ️ 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".
| let metadata = self.store.metadata(for: .claude) | ||
| self.settings.setProviderEnabled(provider: .claude, metadata: metadata, enabled: true) | ||
| if self.settings.claudeUsageDataSource == .web { | ||
| self.settings.claudeUsageDataSource = .oauth |
There was a problem hiding this comment.
When “Disable Keychain access” or the no-prompt OAuth policy is enabled, claude auth login --claudeai can still succeed because it runs as an external CLI process, but this assignment then forces the provider into strict OAuth mode. ClaudeOAuthFetchStrategy cannot obtain the newly stored Keychain credential in that configuration, and its shouldFallback implementation only permits the working CLI strategy in .auto mode, so the refresh immediately fails despite the successful login. Keep a CLI fallback for this case rather than unconditionally selecting .oauth.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c526a05781
ℹ️ 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".
| account: fetcher.loadAccountInfo(), | ||
| updater: DisabledUpdaterController(), | ||
| preferencesSelection: PreferencesSelection(), | ||
| statusBar: .system) |
There was a problem hiding this comment.
On macOS test runs, this now constructs a controller against NSStatusBar.system for every source mode without calling releaseStatusItemsForTesting; the removed withStatusItemControllerForTesting helper deferred exactly that cleanup, and StatusItemController.deinit does not remove its status items. These iterations can therefore leave real status items registered and pollute later headless AppKit tests, so retain the helper or add equivalent deferred teardown.
AGENTS.md reference: AGENTS.md:L27-L27
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
d0d9b6d to
8ef8607
Compare
clawsweeper
Bot
removed
the
merge-risk: 🚨 automation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f60d474e7
ℹ️ 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".
| let desc = error.localizedDescription | ||
| if desc.contains("Text file busy") || desc.contains("ETXTBSY"), attempt < 4 { |
There was a problem hiding this comment.
When another process has the executable open for writing on Linux, Foundation wraps ETXTBSY as an outer NSCocoaErrorDomain error whose localizedDescription only reports Cocoa error 256; code 26 and “Text file busy” reside in NSUnderlyingError. Consequently neither substring matches, the loop exits after its first attempt, and this still returns launchFailed instead of retrying. Inspect the underlying NSError for NSPOSIXErrorDomain/ETXTBSY, and cover the busy-executable race with a focused test.
AGENTS.md reference: AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
1 participant
Summary
Claude CLI logins with
claudeUsageDataSource=.webcaused immediate disconnection because the OAuth flow would fail due to Keychain issues, and there was no fallback to CLI. Additionally, there was a test lifecycle regression in the status item controller.Changes Made
claudeUsageDataSourcefrom.webto.autoafter a successful CLI login. This ensures the system falls back to CLI if OAuth fails due to Keychain issues.withStatusItemControllerForTestingteardown to prevent status item pollution.DEBUG-onlyresolveStrategiesForTestinghook for testing strategy planning.Runtime Behavior Proof: Claude CLI Login Fallback
Scenario: CLI Login with Keychain Issues
Step 1: User logs in via Claude CLI
Step 2: CodexBar detects login
Step 3: OAuth attempt
Step 4: Fallback behavior
Source Strategy Resolution:
Runtime Log Evidence (After Fix):
Verification
swift test --filter ClaudeLoginFlowPolicyTests✅swift test --filter ClaudeIssue2403ReproductionTests✅make check✅ (0 format/lint violations)Fixes #2403