fix(claude): detect macOS Keychain authentication - #1033
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesClaude CLI authentication
Sequence Diagram(s)sequenceDiagram
participant ClaudeProviderAuth
participant ClaudeCLI
participant CredentialFiles
ClaudeProviderAuth->>ClaudeCLI: claude auth status --json
ClaudeCLI-->>ClaudeProviderAuth: Authentication JSON or error
alt CLI status is available
ClaudeProviderAuth-->>ClaudeProviderAuth: Return CLI-derived status
else CLI status is unavailable
ClaudeProviderAuth->>CredentialFiles: Read legacy credentials
CredentialFiles-->>ClaudeProviderAuth: Credential data or error
end
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/modules/providers/list/claude/claude-auth.provider.ts (1)
29-37: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle
spawnSyncresults incheckInstalled()
spawnSyncreturns an error object for a missing executable instead of throwing, so thistry/catchstill reports the CLI as installed. Checkresult.errorandresult.statusbefore returningtrue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/modules/providers/list/claude/claude-auth.provider.ts` around lines 29 - 37, Update checkInstalled() to capture the result of spawnSync and return true only when no result.error exists and the process exits successfully (status 0). Preserve the existing false result for failures, including missing executables and nonzero exit statuses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@server/modules/providers/list/claude/claude-auth.provider.ts`:
- Around line 29-37: Update checkInstalled() to capture the result of spawnSync
and return true only when no result.error exists and the process exits
successfully (status 0). Preserve the existing false result for failures,
including missing executables and nonzero exit statuses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 17a1d3ce-6a45-40d7-9b07-19624df019c8
📒 Files selected for processing (2)
server/modules/providers/list/claude/claude-auth.provider.tsserver/modules/providers/tests/claude-auth.test.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Closes #556
What changed
On macOS, CloudCLI can report Claude as disconnected even while conversations work because Claude Code stores OAuth credentials in Keychain rather than
~/.claude/.credentials.json. Authentication status now comes fromclaude auth status --json, with the existing environment, settings, and legacy credential-file paths retained for compatibility.Why
Claude Code itself is the reliable authority for whether its credentials are usable. Asking the CLI avoids duplicating platform-specific credential-storage logic or attempting to read Keychain directly, and keeps CloudCLI aligned with the authentication source Claude actually uses at runtime.
The status response is parsed even when Claude exits with status 1, because that is its documented logged-out result rather than an unsupported-command failure. Timeouts remain distinguishable from a genuine logged-out response so Keychain-only users are not silently misreported.
This is a deliberately narrow follow-up to #881. That PR also changed the login command and refresh behavior; maintainers requested separate changes and noted that the existing
/loginflow exposes additional authentication choices. Those behaviors remain untouched here.Out of scope
The login command, available login methods, and modal refresh behavior are unchanged.
Before and after
Before
After
Test plan
npx tsx --tsconfig=server/tsconfig.json --test server/modules/providers/tests/claude-auth.test.tsnpm run typechecknpm run lint(passes with the repository's existing warnings)npm run build(passes with the repository's existing CSS and bundle-size warnings)authenticated: truewith methodcli:claude.aifor a macOS Keychain-backed loginSummary by CodeRabbit