-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: warn users when ANTHROPIC_API_KEY conflicts with Claude Code subscription #7197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…scription - Add environment variable detection in ClaudeCodeHandler constructor - Show warning notification when ANTHROPIC_API_KEY is detected - Add i18n translations for the warning message - Include "Learn More" action that links to Claude Code authentication docs - Add comprehensive tests for the new functionality Fixes #7196
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code is like debugging in a mirror - everything looks backwards but the bugs are still mine.
| "learn_more": "Learn More" | ||
| }, | ||
| "actions": { | ||
| "learn_more": "Learn More" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice we're adding a new "actions" object at lines 138-140, but there's already a "buttons" object at lines 133-137 that contains "learn_more". Is this intentional duplication? Could we consolidate by using the existing "buttons.learn_more" translation key instead to avoid redundancy?
|
|
||
| private async checkForAnthropicApiKey() { | ||
| // Only check and warn once per session to avoid annoying the user | ||
| if (!ClaudeCodeHandler.hasShownApiKeyWarning && process.env.ANTHROPIC_API_KEY) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The static flag approach works for preventing repeated warnings per session, but have you considered making this dismissible with a "Don't show again" option that persists? Some users might intentionally use both ANTHROPIC_API_KEY and Claude Code subscription for different purposes.
| ) | ||
| } | ||
| } catch { | ||
| // Silently ignore any errors from the warning dialog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This catch block silently swallows all errors. While I understand we don't want the warning to break the extension, should we at least log errors for debugging purposes? Something like:
| this.checkForAnthropicApiKey() | ||
| } | ||
|
|
||
| private async checkForAnthropicApiKey() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a comment here explaining why this check is necessary and linking to issue #7196 for future maintainers. This would help document the context behind this warning system.
|
|
||
| afterEach(() => { | ||
| // Restore original environment | ||
| process.env = originalEnv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on properly saving and restoring process.env in beforeEach/afterEach! Consider adding a test that explicitly verifies the environment is restored correctly after the tests run, just to ensure the cleanup is working as expected.
|
See #7196 (comment) |
Summary
This PR addresses issue #7196 by adding a warning notification when users have an
ANTHROPIC_API_KEYenvironment variable set while using the Claude Code API provider. This helps prevent confusion for Claude Max/Pro subscription users who may experience authentication errors due to the environment variable taking precedence over their subscription login.Problem
When users with Claude Max/Pro subscriptions correctly configure the extension to use Claude Code, the presence of an
ANTHROPIC_API_KEYenvironment variable causes the CLI to use that key instead of the subscription authentication. This results in misleading "Credit balance is too low" errors for users who have valid subscriptions.Solution
ClaudeCodeHandlerconstructorANTHROPIC_API_KEYis detectedChanges
checkForAnthropicApiKey()method to detect and warn about the environment variableTesting
Screenshots
The warning message will appear as a VSCode notification:
With a "Learn More" button that links to the Claude Code authentication documentation.
Fixes #7196
Important
Adds a warning in
ClaudeCodeHandlerforANTHROPIC_API_KEYconflicts with Claude Max/Pro subscriptions, with tests and documentation link.ClaudeCodeHandlerconstructor whenANTHROPIC_API_KEYis set, potentially conflicting with Claude Max/Pro subscription.checkForAnthropicApiKey()method inclaude-code.tsfor environment variable detection and warning.common.jsonwith new warning and action strings.claude-code.spec.tsfor warning display, single session warning, and "Learn More" link functionality.This description was created by
for 8ad7bdc. You can customize this summary. It will automatically update as commits are pushed.