fix(auth): cloud provider model mapping, env forwarding, Vertex/Bedrock auth support#2059
fix(auth): cloud provider model mapping, env forwarding, Vertex/Bedrock auth support#2059thedotmack wants to merge 2 commits intomainfrom
Conversation
…ck auth support - #1942: Add Bedrock/Vertex model ID translation (e.g. claude-sonnet-4-6 -> provider format) - #1943: Allowlist CLAUDE_CODE_USE_BEDROCK/VERTEX and AWS_*/Google env vars in env-sanitizer - #1944: Add ANTHROPIC_AUTH_TOKEN to managed credential keys for parse/persist/forward - #1945: Skip OAuth validation when Vertex/Bedrock detected, override auth method to 'api' - #1946: Validate OpenRouter API key before request to prevent cryptic 401 errors Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 51 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThese changes extend Claude Code's configuration and authentication system to support cloud providers (AWS Bedrock and Google Cloud Vertex AI). They add model resolution logic based on cloud provider detection, manage provider-specific credentials through existing configuration systems, forward cloud provider environment variables to spawned SDK processes, and validate auth methods appropriate to each provider. Changes
Sequence Diagram(s)No sequence diagrams generated — changes are primarily configuration initialization, validation logic, and environment variable management without complex multi-step interactions warranting visualization. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code ReviewOverviewThis PR bundles 5 related fixes for cloud provider (Bedrock/Vertex) and auth support: model ID translation, env var forwarding, Strengths
Issues & Suggestions1. Hardcoded version suffixes will go stale (medium) 'claude-sonnet-4-6': 'anthropic.claude-sonnet-4-6-v1:0',
'claude-sonnet-4-6': 'claude-sonnet-4-6@20250514',The 2. Missing current models in the maps (medium)
3. Double
4. False-positive warnings for credential-less auth (low) The Bedrock warning fires when no 5. settings.CLAUDE_MEM_CLAUDE_AUTH_METHOD = 'api';Mutating the settings object passed in is a surprising side effect. A cleaner pattern returns the modified value or takes a 6. This is correct and necessary, but it's worth a comment in the code noting that cloud provider users have reduced env isolation by design. Future reviewers might otherwise flag this as a security regression without context. Test CoverageThe test plan is entirely manual. Given the complexity of the model-mapping logic and the auth override behavior, unit tests would catch regressions cheaply:
SummarySolid fixes for a real pain point — the patterns are consistent and the intent is clear. The two medium items (stale version strings, missing models) are the ones most likely to produce user-facing errors before the next release. |
Greptile SummaryThis PR adds cloud provider (Bedrock/Vertex) awareness to model ID resolution and auth method selection, allowlists cloud credential env vars through the sanitizer, adds
Confidence Score: 4/5Safe to merge after fixing the no-settings-file early-return path in loadFromFile. One P1 bug: first-time users with Bedrock/Vertex enabled won't get model ID translation or auth override because the no-file early-return skips the new cloud provider processing. The other three files are clean. The P2 findings (log spam, misleading credential warning) are non-blocking. src/shared/SettingsDefaultsManager.ts — the no-settings-file early return path needs the same model resolution and auth validation applied in the happy path. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[loadFromFile called] --> B{settings file exists?}
B -- No --> C[applyEnvOverrides defaults]
C --> C1[⚠️ resolveModelForCloudProvider SKIPPED]
C1 --> C2[⚠️ validateCloudProviderAuth SKIPPED]
C2 --> Z[Return settings]
B -- Yes --> D[Read + merge file settings]
D --> E[applyEnvOverrides]
E --> F[resolveModelForCloudProvider]
F --> G{cloud provider detected?}
G -- bedrock --> H[Map model ID via BEDROCK_MODEL_MAP]
G -- vertex --> I[Map model ID via VERTEX_MODEL_MAP]
G -- none --> J[Return unchanged model ID]
H --> K[validateCloudProviderAuth]
I --> K
J --> K
K --> L{auth method = cli?}
L -- Yes + provider set --> M[Override to api]
L -- No --> N[Keep as-is]
M --> Z
N --> Z
B -- Error --> P[applyEnvOverrides getAllDefaults]
P --> Q[resolveModelForCloudProvider]
Q --> R[validateCloudProviderAuth]
R --> Z
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| static get(key: keyof SettingsDefaults): string { | ||
| return process.env[key] ?? this.DEFAULTS[key]; | ||
| const value = process.env[key] ?? this.DEFAULTS[key]; | ||
| // Resolve cloud-provider-specific model ID when reading the model setting | ||
| if (key === 'CLAUDE_MEM_MODEL') { | ||
| return this.resolveModelForCloudProvider(value); | ||
| } | ||
| return value; |
There was a problem hiding this comment.
get('CLAUDE_MEM_MODEL') call
resolveModelForCloudProvider unconditionally calls console.log/console.warn each time it runs, and get('CLAUDE_MEM_MODEL') is the hot path for reading any model setting. Every call throughout the application's lifecycle will emit the mapping message (or warning), flooding the console rather than logging once at startup.
Consider logging only when loadFromFile resolves the model (startup), or use a boolean flag to log the mapping/warning only once.
|
|
||
| if (provider === 'bedrock') { | ||
| // Bedrock uses AWS IAM — OAuth token and ANTHROPIC_API_KEY are not required | ||
| if (!process.env.AWS_ACCESS_KEY_ID && !process.env.AWS_PROFILE && !process.env.AWS_DEFAULT_REGION && !process.env.AWS_REGION) { |
There was a problem hiding this comment.
~/.aws credential configurations
The condition includes !process.env.AWS_DEFAULT_REGION and !process.env.AWS_REGION as part of the "no credentials" check, but those are region vars, not auth credentials. A user whose credentials live in ~/.aws/credentials (the default shared-credential file setup) will have no AWS env vars set at all yet have perfectly valid credentials. The warning will fire as a false positive for this common AWS setup.
Consider narrowing to actual credential indicators only:
| if (!process.env.AWS_ACCESS_KEY_ID && !process.env.AWS_PROFILE && !process.env.AWS_DEFAULT_REGION && !process.env.AWS_REGION) { | |
| if (!process.env.AWS_ACCESS_KEY_ID && !process.env.AWS_PROFILE) { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/SettingsDefaultsManager.ts (1)
323-337:⚠️ Potential issue | 🟠 MajorApply cloud-provider resolution in the no-file branch too.
When
settingsPathdoes not exist, this branch returns right afterapplyEnvOverrides(defaults). Fresh Bedrock/Vertex installs therefore miss both the model remap and thecli→apiauth-method override on first run.🔧 Proposed fix
- return this.applyEnvOverrides(defaults); + const final = this.applyEnvOverrides(defaults); + final.CLAUDE_MEM_MODEL = this.resolveModelForCloudProvider(final.CLAUDE_MEM_MODEL); + this.validateCloudProviderAuth(final); + return final;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/SettingsDefaultsManager.ts` around lines 323 - 337, When settingsPath doesn't exist, after creating defaults and before returning, run the same resolution steps the file-exists branch performs so new installs get cloud-provider resolution, model remap and the cli→api auth-method override; specifically, call the same helper(s) used elsewhere to apply cloud-provider resolution and model-remap/auth-override (rather than only applyEnvOverrides) and then return the resolved settings from this.applyEnvOverrides(defaults). Keep getAllDefaults(), the settingsPath creation code and the console warn, but ensure you invoke the existing resolution routines (the cloud-provider resolution and model remap/auth override helpers used in the other branch) before returning.
🧹 Nitpick comments (1)
src/shared/SettingsDefaultsManager.ts (1)
201-203: Consider de-duplicating these provider logs.These messages are emitted every time settings are loaded, so under the normal
loadFromFile(USER_SETTINGS_PATH)flow they will repeat on ordinary request traffic. A once-per-process guard would keep them useful.Based on learnings,
SettingsDefaultsManager.loadFromFile(USER_SETTINGS_PATH)is intentionally called per request.Also applies to: 234-251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/SettingsDefaultsManager.ts` around lines 201 - 203, The provider mapping log in SettingsDefaultsManager (the block that logs `[SETTINGS] Cloud provider "${provider}" detected — mapping model "${modelId}" → "${mapped}"` when `mapped` is truthy, and the similar logs around the 234-251 region) should be emitted only once per process to avoid noisy repetition; add a module-level guard (e.g., a Set<string> or boolean map keyed by `provider` or `provider|modelId`) and check it before logging, then mark the provider as logged after the first log; update the logging points that reference `provider`, `modelId`, and `mapped` to consult this guard so subsequent calls to `SettingsDefaultsManager.loadFromFile` skip duplicate logs.
🤖 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/services/worker/OpenRouterAgent.ts`:
- Around line 373-380: The current fail-fast guard in OpenRouterAgent.ts checks
only falsy apiKey but allows whitespace-only strings; update the check around
the apiKey variable (the block that throws "OpenRouter API key not
configured...") to treat whitespace-only values as missing by using a trimmed
check (e.g., if (!apiKey || apiKey.trim().length === 0)) so a key of only spaces
will throw the same clear Error; keep the existing error message and throw site
(the same throw in the OpenRouterAgent initialization) unchanged otherwise.
In `@src/shared/EnvManager.ts`:
- Around line 239-242: In buildIsolatedEnv(), remove any ambient/inherited
ANTHROPIC_AUTH_TOKEN from the isolatedEnv before re-injecting the managed
credential: explicitly clear/delete isolatedEnv.ANTHROPIC_AUTH_TOKEN (or avoid
copying it earlier) and then set isolatedEnv.ANTHROPIC_AUTH_TOKEN =
credentials.ANTHROPIC_AUTH_TOKEN only when credentials.ANTHROPIC_AUTH_TOKEN is
present; reference symbols: buildIsolatedEnv, isolatedEnv, and
credentials.ANTHROPIC_AUTH_TOKEN.
In `@src/shared/SettingsDefaultsManager.ts`:
- Around line 191-196: The helper resolveModelForCloudProvider currently treats
only process.env.CLAUDE_MEM_MODEL as explicit but is also called from
loadFromFile on merged settings, causing user-saved CLAUDE_MEM_MODEL from
settings.json to be remapped; change resolveModelForCloudProvider so it only
skips remapping when the incoming modelId equals the hardcoded default (i.e.,
perform mapping only if modelId === <the default constant or literal default
value>), and keep the existing early-return for process.env.CLAUDE_MEM_MODEL;
apply the same fix to the other occurrence referenced around lines 368–370 so
saved settings are not rewritten or warned about.
---
Outside diff comments:
In `@src/shared/SettingsDefaultsManager.ts`:
- Around line 323-337: When settingsPath doesn't exist, after creating defaults
and before returning, run the same resolution steps the file-exists branch
performs so new installs get cloud-provider resolution, model remap and the
cli→api auth-method override; specifically, call the same helper(s) used
elsewhere to apply cloud-provider resolution and model-remap/auth-override
(rather than only applyEnvOverrides) and then return the resolved settings from
this.applyEnvOverrides(defaults). Keep getAllDefaults(), the settingsPath
creation code and the console warn, but ensure you invoke the existing
resolution routines (the cloud-provider resolution and model remap/auth override
helpers used in the other branch) before returning.
---
Nitpick comments:
In `@src/shared/SettingsDefaultsManager.ts`:
- Around line 201-203: The provider mapping log in SettingsDefaultsManager (the
block that logs `[SETTINGS] Cloud provider "${provider}" detected — mapping
model "${modelId}" → "${mapped}"` when `mapped` is truthy, and the similar logs
around the 234-251 region) should be emitted only once per process to avoid
noisy repetition; add a module-level guard (e.g., a Set<string> or boolean map
keyed by `provider` or `provider|modelId`) and check it before logging, then
mark the provider as logged after the first log; update the logging points that
reference `provider`, `modelId`, and `mapped` to consult this guard so
subsequent calls to `SettingsDefaultsManager.loadFromFile` skip duplicate logs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0a933e6-a322-4d4d-ae7a-8f5f13496281
📒 Files selected for processing (4)
src/services/worker/OpenRouterAgent.tssrc/shared/EnvManager.tssrc/shared/SettingsDefaultsManager.tssrc/supervisor/env-sanitizer.ts
…ial checks, env isolation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code ReviewOverviewThis PR adds cloud provider awareness (Bedrock/Vertex AI) for model ID resolution, fixes env var forwarding for AWS/GCP credentials, adds Issues & Suggestions
|
|
Closing to start fresh from main — will redo fixes isolated in Docker container. |
1 participant
Summary
claude-sonnet-4-6→anthropic.claude-sonnet-4-6-v1:0, Vertex maps toclaude-sonnet-4-6@20250514. Warns when no mapping exists.CLAUDE_CODE_USE_BEDROCK,CLAUDE_CODE_USE_VERTEXand AWS/Google credential env vars (AWS_REGION,AWS_ACCESS_KEY_ID,AWS_SECRET_ACCESS_KEY,AWS_SESSION_TOKEN,AWS_PROFILE,GOOGLE_APPLICATION_CREDENTIALS,GOOGLE_CLOUD_PROJECT) in env-sanitizer so they pass through to SDK subprocesses.ANTHROPIC_AUTH_TOKENtoMANAGED_CREDENTIAL_KEYSso it gets parsed from.env, persisted, and forwarded to SDK subprocesses.CLAUDE_MEM_CLAUDE_AUTH_METHODfromclitoapi(skipping OAuth validation). Log clear warnings when expected cloud credentials are missing.Bearer(empty) and getting a cryptic 401.Test plan
CLAUDE_CODE_USE_BEDROCK,AWS_REGION,AWS_ACCESS_KEY_ID, etc.) are forwarded through env-sanitizerCLAUDE_CODE_USE_VERTEX,GOOGLE_APPLICATION_CREDENTIALS,GOOGLE_CLOUD_PROJECT) are forwarded through env-sanitizerANTHROPIC_AUTH_TOKENis parsed from.env, persisted, and forwarded inbuildIsolatedEnvCloses #1942, closes #1943, closes #1944, closes #1945, closes #1946
🤖 Generated with Claude Code