refactor: remove Anthropic OAuth and use generic settings#1375
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughRemoved Anthropic OAuth support and UI, converted provider handling to API-key-only via the Anthropic SDK, added a migration to normalize legacy Anthropic providers, removed related types/actions/strings, and added tests for the API-only behavior and migration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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: b59f23a0a9
ℹ️ 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".
Comment on lines
+134
to
+135
| delete normalized.authMode | ||
| delete normalized.oauthToken |
There was a problem hiding this comment.
If an existing install was using Anthropic via OAuth only, deleting authMode/oauthToken here leaves the provider enabled but with no usable credential. AnthropicProvider.init() now bails out when apiKey is empty, while BaseLLMProvider.loadCachedModels() still exposes the cached Anthropic models, so after upgrade the provider remains selectable and every request fails with Anthropic client is not initialized. Please either disable Anthropic or force reconfiguration when migrating these OAuth-only configs so existing users do not end up with a silently broken provider.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/presenter/llmProviderPresenter/providers/anthropicProvider.ts (1)
37-42:⚠️ Potential issue | 🟠 MajorSplit trace headers from auth headers.
This helper now returns the live
x-api-key, and Line 662 reuses it for request tracing. That pushes the secret into non-network code unnecessarily and makes any trace sink a credential-bearing path.🔐 Suggested change
private buildAnthropicApiKeyHeaders(): Record<string, string> { return { 'Content-Type': 'application/json', 'anthropic-version': '2023-06-01', 'x-api-key': this.provider.apiKey || process.env.ANTHROPIC_API_KEY || 'MISSING_API_KEY' } } + + private buildAnthropicTraceHeaders(): Record<string, string> { + return { + ...this.buildAnthropicApiKeyHeaders(), + 'x-api-key': '[REDACTED]' + } + }await this.emitRequestTrace(modelConfig, { endpoint: this.buildAnthropicEndpoint(), - headers: this.buildAnthropicApiKeyHeaders(), + headers: this.buildAnthropicTraceHeaders(), body: streamParams })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/llmProviderPresenter/providers/anthropicProvider.ts` around lines 37 - 42, The helper buildAnthropicApiKeyHeaders currently returns headers including the live x-api-key which is later reused for tracing; split auth and trace headers so credentials are never placed into trace paths: keep a method (e.g., buildAnthropicApiKeyHeaders or buildAnthropicAuthHeaders) that returns only auth-related headers including 'x-api-key' for network requests, and add a separate method (e.g., buildAnthropicTraceHeaders or buildAnthropicNonAuthHeaders) that returns traceable headers like 'Content-Type' and 'anthropic-version' but omits 'x-api-key'; update call sites that previously reused buildAnthropicApiKeyHeaders for traces to use the new trace-only method and ensure only the auth-header method is passed to actual HTTP request logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/llmProviderPresenter/providers/anthropicProvider.ts`:
- Around line 61-64: The console.log at the proxy handling in
anthropicProvider.ts leaks possible proxy credentials by printing proxyUrl;
instead modify the proxy-logging to avoid sensitive userinfo: when proxyUrl is
truthy, detect and redact userinfo (or simply log that a proxy is enabled)
before calling new ProxyAgent and assigning fetchOptions.dispatcher; reference
the proxyUrl variable, ProxyAgent construction, and fetchOptions.dispatcher to
locate the change and ensure no full URL is written to logs.
---
Outside diff comments:
In `@src/main/presenter/llmProviderPresenter/providers/anthropicProvider.ts`:
- Around line 37-42: The helper buildAnthropicApiKeyHeaders currently returns
headers including the live x-api-key which is later reused for tracing; split
auth and trace headers so credentials are never placed into trace paths: keep a
method (e.g., buildAnthropicApiKeyHeaders or buildAnthropicAuthHeaders) that
returns only auth-related headers including 'x-api-key' for network requests,
and add a separate method (e.g., buildAnthropicTraceHeaders or
buildAnthropicNonAuthHeaders) that returns traceable headers like 'Content-Type'
and 'anthropic-version' but omits 'x-api-key'; update call sites that previously
reused buildAnthropicApiKeyHeaders for traces to use the new trace-only method
and ensure only the auth-header method is passed to actual HTTP request logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0758352e-467d-4e54-8c1c-8676cfe6de1d
📒 Files selected for processing (25)
src/main/presenter/anthropicOAuth.tssrc/main/presenter/configPresenter/index.tssrc/main/presenter/llmProviderPresenter/providers/anthropicProvider.tssrc/main/presenter/oauthPresenter.tssrc/renderer/settings/components/AnthropicProviderSettingsDetail.vuesrc/renderer/settings/components/ModelProviderSettings.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/stores/providerStore.tssrc/shared/provider-operations.tssrc/shared/types/presenters/legacy.presenters.d.tssrc/shared/types/presenters/llmprovider.presenter.d.tstest/main/presenter/configPresenter/anthropicProviderMigration.test.tstest/main/presenter/llmProviderPresenter/anthropicProvider.test.tstest/renderer/components/ModelProviderSettings.test.ts
💤 Files with no reviewable changes (20)
- src/shared/provider-operations.ts
- src/shared/types/presenters/llmprovider.presenter.d.ts
- src/renderer/settings/components/ModelProviderSettings.vue
- src/renderer/src/i18n/zh-TW/settings.json
- src/renderer/src/i18n/fr-FR/settings.json
- src/renderer/src/stores/providerStore.ts
- src/renderer/src/i18n/ru-RU/settings.json
- src/main/presenter/oauthPresenter.ts
- src/shared/types/presenters/legacy.presenters.d.ts
- src/renderer/src/i18n/fa-IR/settings.json
- src/renderer/src/i18n/da-DK/settings.json
- src/renderer/src/i18n/ko-KR/settings.json
- src/renderer/src/i18n/pt-BR/settings.json
- src/renderer/src/i18n/zh-CN/settings.json
- src/renderer/settings/components/AnthropicProviderSettingsDetail.vue
- src/renderer/src/i18n/he-IL/settings.json
- src/renderer/src/i18n/en-US/settings.json
- src/renderer/src/i18n/zh-HK/settings.json
- src/renderer/src/i18n/ja-JP/settings.json
- src/main/presenter/anthropicOAuth.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/main/presenter/configPresenter/anthropicProviderMigration.test.ts`:
- Line 1: The “no credential” test should explicitly control ANTHROPIC_API_KEY
to be deterministic: capture the original value at test start (e.g., const orig
= process.env.ANTHROPIC_API_KEY), delete or set process.env.ANTHROPIC_API_KEY =
undefined (or use delete process.env.ANTHROPIC_API_KEY) before running the
specific test that assumes no key, then restore the original value in afterEach
(or finally) to avoid leaking state; update the
anthropicProviderMigration.test.ts test that references ANTHROPIC_API_KEY so it
stores, clears, runs the assertion, and restores the env variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 625067a4-2550-4bf2-a81e-c5b8c60b2477
📒 Files selected for processing (3)
src/main/presenter/configPresenter/index.tssrc/main/presenter/llmProviderPresenter/providers/anthropicProvider.tstest/main/presenter/configPresenter/anthropicProviderMigration.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/main/presenter/llmProviderPresenter/providers/anthropicProvider.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/presenter/configPresenter/index.ts
| @@ -0,0 +1,152 @@ | |||
| import { afterEach, describe, expect, it, vi } from 'vitest' | |||
Contributor
There was a problem hiding this comment.
Make the “no credential” test deterministic across environments.
Line 40 captures host env state, but the test at Line 51 assumes no ANTHROPIC_API_KEY. If CI/local env already has that variable, this case can fail intermittently.
Proposed fix
-import { afterEach, describe, expect, it, vi } from 'vitest'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
describe('normalizeAnthropicProviderForApiOnly', () => {
const originalEnvKey = process.env.ANTHROPIC_API_KEY
+ beforeEach(() => {
+ delete process.env.ANTHROPIC_API_KEY
+ })
+
afterEach(() => {
if (originalEnvKey === undefined) {
delete process.env.ANTHROPIC_API_KEY
return
}Also applies to: 40-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/main/presenter/configPresenter/anthropicProviderMigration.test.ts` at
line 1, The “no credential” test should explicitly control ANTHROPIC_API_KEY to
be deterministic: capture the original value at test start (e.g., const orig =
process.env.ANTHROPIC_API_KEY), delete or set process.env.ANTHROPIC_API_KEY =
undefined (or use delete process.env.ANTHROPIC_API_KEY) before running the
specific test that assumes no key, then restore the original value in afterEach
(or finally) to avoid leaking state; update the
anthropicProviderMigration.test.ts test that references ANTHROPIC_API_KEY so it
stores, clears, runs the assertion, and restores the env variable.
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
1 participant
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 by CodeRabbit
Breaking Changes
Improvements