feat(auth): auto-switch provider after successful OAuth login #1008
Conversation
Implement REQ-001: After successful OAuth authentication via /auth <provider> login, automatically switch the active provider to the authenticated provider. - Add switchActiveProvider and getEphemeralSetting imports - Check auth.autoSwitchProvider setting (defaults to true) - On successful auth + switch: show "[OK] Authenticated with X and set as active provider" - If switch fails: gracefully degrade, still report auth success with manual switch hint - If provider already active: just show standard auth success message
Comprehensive tests covering: - Auto-switch after successful OAuth login - Bucket info in success messages - Configurable setting (enable/disable) - Graceful error handling when switch fails - Override behavior for existing provider
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing touches
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/cli/src/ui/commands/__tests__/authCommand.autoswitch.spec.ts (3)
30-33: Consider more specific mock types for better type safety.While using
ReturnType<typeof vi.fn>works, more specific typing would provide better type checking. Consider typing the mocks with their actual function signatures:const mockSwitchActiveProvider = switchActiveProvider as unknown as vi.MockedFunction<typeof switchActiveProvider>; const mockGetEphemeralSetting = getEphemeralSetting as unknown as vi.MockedFunction<typeof getEphemeralSetting>;This provides better autocomplete and type checking in tests while still allowing for mock configuration.
43-48: Consider creating a test helper type for better type safety.The cast
as unknown as OAuthManagerbypasses TypeScript's type checking. While this is common in tests, consider creating a dedicated test helper type:🔎 Proposed refactor for improved type safety
+type MockOAuthManager = Pick<OAuthManager, 'authenticate' | 'getSupportedProviders'>; + beforeEach(() => { vi.clearAllMocks(); mockOAuthManager = { authenticate: vi.fn().mockResolvedValue(undefined), getSupportedProviders: vi .fn() .mockReturnValue(['gemini', 'qwen', 'anthropic', 'codex']), - } as unknown as OAuthManager; + } as MockOAuthManager as OAuthManager;This makes the intended partial mock explicit and easier to maintain.
52-61: Context mock usesas nevercasts, which bypasses type safety.While the minimal mock works for these focused tests, the
as nevercasts can mask issues if the implementation starts using these properties. Consider creating a test helper that provides a more complete mock:🔎 Proposed refactor for a more robust context mock
function createMockContext(): CommandContext { return { services: { config: null, settings: {} as LoadedSettings, git: undefined, logger: { log: vi.fn(), error: vi.fn(), warn: vi.fn(), debug: vi.fn(), } as unknown as Logger, }, ui: { addItem: vi.fn(), clear: vi.fn(), // ... other required properties } as unknown as CommandContext['ui'], session: { stats: {} as SessionStatsState, sessionShellAllowlist: new Set(), }, }; }This makes the mock more maintainable and catches usage of unexpected properties.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/cli/src/ui/commands/__tests__/authCommand.autoswitch.spec.tspackages/cli/src/ui/commands/authCommand.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Don't useany- Always specify proper types. Useunknownif the type is truly unknown and add proper type guards.
Do not useconsole.logorconsole.debug- Use the sophisticated logging system instead. Log files are written to ~/.llxprt/debug/
Fix all linting errors, including warnings aboutanytypes
Files:
packages/cli/src/ui/commands/authCommand.tspackages/cli/src/ui/commands/__tests__/authCommand.autoswitch.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-12-13T16:04:07.079Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-13T16:04:07.079Z
Learning: Auth commands UX: /auth <provider> login [bucket] (eager device flow), /auth <provider> logout <default|bucket|all>, /auth <provider> status; all arguments autocomplete bucket names (including "default").
Applied to files:
packages/cli/src/ui/commands/authCommand.ts
🧬 Code graph analysis (1)
packages/cli/src/ui/commands/__tests__/authCommand.autoswitch.spec.ts (3)
packages/cli/src/runtime/runtimeSettings.ts (1)
switchActiveProvider(1478-1962)packages/cli/src/ui/commands/authCommand.ts (1)
AuthCommandExecutor(184-687)packages/cli/src/ui/commands/types.ts (1)
CommandContext(32-98)
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test (windows-latest, 24.x)
- GitHub Check: Test (ubuntu-latest, 24.x)
- GitHub Check: Test (macos-latest, 24.x)
🔇 Additional comments (5)
packages/cli/src/ui/commands/authCommand.ts (3)
29-32: LGTM! Imports are properly structured.The new imports for
switchActiveProviderandgetEphemeralSettingare correctly added and used in the auto-switch implementation.
427-457: LGTM! Auto-switch logic is well-structured with proper error handling.The implementation correctly:
- Attempts the provider switch after successful authentication
- Differentiates between changed/unchanged provider states with appropriate messaging
- Gracefully handles switch failures without failing the authentication
- Uses the debug logger instead of console.log (per coding guidelines)
The error handling ensures authentication success is preserved even if the auto-switch fails, which is the correct behavior.
423-425: The type safety is already properly implemented.getEphemeralSettingreturnsunknown(notany), and the usage with?? trueprovides an explicit type guard that narrows the result toboolean. No changes are required.packages/cli/src/ui/commands/__tests__/authCommand.autoswitch.spec.ts (2)
12-23: LGTM! Partial mock setup is well-structured.The use of
importOriginalwith proper TypeScript typing ensures that only the necessary functions are mocked while preserving the rest of the module's behavior. This is a good testing practice.
76-191: LGTM! Excellent test coverage with clear requirement mapping.The test suite demonstrates:
- Clear organization by requirements (REQ-001, REQ-008, REQ-010, REQ-004.2)
- Comprehensive coverage of success scenarios, configuration options, error handling, and edge cases
- Proper verification of both function calls and return values
- Good use of descriptive test names that explain the behavior being tested
The tests effectively validate the auto-switch functionality and its integration with the authentication flow.
… mock runtime context - Add continueSession: false to Config constructor in gemini render options tests - Add mock for runtimeSettings module in auth bucket tests to bypass CLI runtime context requirements
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/cli/src/ui/commands/__tests__/authCommand.bucket.spec.ts (1)
21-36: Consider making the mock more flexible for future test scenarios.The hardcoded mock return values (
previousProvider: 'anthropic',nextProvider: 'anthropic',changed: false) work for the current test suite since all tests use the 'anthropic' provider. However, this approach could be brittle if tests are added for other providers or scenarios requiring different mock behaviors.Consider one of these alternatives:
- Make the mock functions configurable per test by exposing them and allowing tests to override behavior when needed
- Use dynamic return values that adapt to the provider being tested
- Add a comment explaining why these specific hardcoded values are safe for this test suite
🔎 Example: Exposing mock for per-test configuration
+// Export mocks for per-test customization +export const mockSwitchActiveProvider = vi.fn().mockResolvedValue({ + changed: false, + previousProvider: 'anthropic', + nextProvider: 'anthropic', +}); +export const mockGetEphemeralSetting = vi.fn().mockReturnValue(true); + vi.mock('../../../runtime/runtimeSettings.js', async (importOriginal) => { const actual = await importOriginal< typeof import('../../../runtime/runtimeSettings.js') >(); return { ...actual, - switchActiveProvider: vi.fn().mockResolvedValue({ - changed: false, - previousProvider: 'anthropic', - nextProvider: 'anthropic', - }), - getEphemeralSetting: vi.fn().mockReturnValue(true), + switchActiveProvider: mockSwitchActiveProvider, + getEphemeralSetting: mockGetEphemeralSetting, }; });Then tests can customize behavior:
beforeEach(() => { mockSwitchActiveProvider.mockResolvedValue({ changed: true, previousProvider: 'gemini', nextProvider: 'anthropic', }); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/cli/src/gemini.renderOptions.test.tsxpackages/cli/src/ui/commands/__tests__/authCommand.bucket.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Don't useany- Always specify proper types. Useunknownif the type is truly unknown and add proper type guards.
Do not useconsole.logorconsole.debug- Use the sophisticated logging system instead. Log files are written to ~/.llxprt/debug/
Fix all linting errors, including warnings aboutanytypes
Files:
packages/cli/src/gemini.renderOptions.test.tsxpackages/cli/src/ui/commands/__tests__/authCommand.bucket.spec.ts
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test (ubuntu-latest, 24.x)
- GitHub Check: Test (windows-latest, 24.x)
- GitHub Check: Test (macos-latest, 24.x)
🔇 Additional comments (2)
packages/cli/src/gemini.renderOptions.test.tsx (2)
86-94: LGTM! Test configuration updated appropriately.The addition of
continueSession: falsealigns the test with the Config constructor requirements without affecting test behavior.
114-122: LGTM! Consistent with the first test case.The configuration parameter is correctly added here as well.
|
discussed in the discord channel with the author. Because we generally want to auth in advance for bucket failover profiles and load balancer profiles and we may auth for GoogleWebSearch and for subagents -- we may not want to switch providers. There is a severe documentation gap after 0.7.0 on how to use
and a general gap on how to set up your workflow. We should address that. I think there is some version of this PR that could be merged once the workflow is figured out with the topics above. For now, I'm converting it to a draft. |
|
Tip For best results, initiate chat on the files or code changes. @acoliver, thank you for the thoughtful analysis. You've identified a critical UX issue with auto-switching in multi-provider workflows. The Core ConcernYou're absolutely right—auto-switching conflicts with several legitimate use cases:
Current State & Potential Paths ForwardThe PR does include Option 1: Change the default
Option 2: Context-aware behavior
Option 3: Explicit switch command
Documentation GapThe identified documentation needs are critical:
I can help draft documentation outlines or examples for these workflows once the desired behavior is settled. RecommendationConverting to draft is the right call. I'd suggest:
Would you like me to help create documentation issues for the workflow gaps, or would you prefer to coordinate that after the auth behavior is finalized? ✏️ Learnings added
🧠 Learnings used |
|
closing for now (feel free to reopen if we come up with a workflow for this) |
2 participants
Summary
Implement automatic provider switching after successful OAuth authentication. When a user logs in with a specific provider via the
/auth <provider>command, the active provider is automatically switched to that authenticated provider.Changes
switchActiveProviderfunction to update the active provider after authenticationgetEphemeralSettingto check theauth.autoSwitchProvidersetting (defaults to true)Features
auth.autoSwitchProvidersettingTesting
Breaking Changes
None
Files Changed
packages/cli/src/ui/commands/authCommand.ts- Core implementationpackages/cli/src/ui/commands/__tests__/authCommand.autoswitch.spec.ts- Test suiteSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.