feat: implement AWS Bedrock integration and update authentication han…#548
Open
tintnc wants to merge 6 commits intositeboon:mainfrom
Open
feat: implement AWS Bedrock integration and update authentication han…#548tintnc wants to merge 6 commits intositeboon:mainfrom
tintnc wants to merge 6 commits intositeboon:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds commented AWS Bedrock guidance to Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant SettingsFile as "~/.claude/settings.json"
participant Bedrock
rect rgba(200,200,255,0.5)
Client->>Server: request Claude status / operation
Server->>SettingsFile: loadClaudeSettingsEnv()
SettingsFile-->>Server: returns env object (or {})
alt Bedrock enabled (env or process.env)
Server->>Server: resolveClaudeModel(alias, settingsEnv)
Server->>Bedrock: call Bedrock endpoint (bedrock auth path)
Bedrock-->>Server: respond (method: "bedrock")
Server-->>Client: { authenticated: true, method: "bedrock", isBedrock: true }
else Non-Bedrock
Server->>Server: check API key / OAuth / settings.json creds
Server-->>Client: { authenticated:*, method:*, isBedrock: false }
end
end
Possibly related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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}
Contributor
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 (2)
src/components/settings/hooks/useSettingsController.ts (1)
276-284:⚠️ Potential issue | 🟡 MinorMissing
isBedrock: falsein failed response path.When
!response.ok, the status object is missing theisBedrockfield. SincesetAuthStatusByProviderreplaces the entire object (not merges), this could leaveisBedrockundefined. Other paths at lines 287-294 and 297-303 correctly include it.🔧 Proposed fix
if (!response.ok) { setAuthStatusByProvider(provider, { authenticated: false, email: null, loading: false, error: 'Failed to check authentication status', + isBedrock: false, }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/settings/hooks/useSettingsController.ts` around lines 276 - 284, The failure branch that runs when response.ok is false calls setAuthStatusByProvider(provider, {...}) but omits the isBedrock property; update that object to include isBedrock: false (matching the other failure/success branches) so setAuthStatusByProvider receives a complete status shape for provider and won’t leave isBedrock undefined.server/routes/cli-auth.js (1)
166-172:⚠️ Potential issue | 🟡 MinorMissing
isBedrock: falsein ANTHROPIC_API_KEY path.This return path is missing the
isBedrockfield, while other api_key paths (lines 177-184, 186-193) include it. For consistency and to avoid potential undefined values, add the field.🔧 Proposed fix
if (process.env.ANTHROPIC_API_KEY && process.env.ANTHROPIC_API_KEY.trim()) { return { authenticated: true, email: 'API Key Auth', - method: 'api_key' + method: 'api_key', + isBedrock: false }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/cli-auth.js` around lines 166 - 172, The ANTHROPIC_API_KEY return path is missing the isBedrock property; update the object returned inside the conditional that checks process.env.ANTHROPIC_API_KEY (the block returning authenticated: true, email: 'API Key Auth', method: 'api_key') to include isBedrock: false so it matches the other api_key return paths and avoids undefined values.
🧹 Nitpick comments (2)
server/claude-sdk.js (2)
472-485:loadClaudeSettingsEnvis also duplicated incli-auth.js.Similar to
isTruthyValue, this function is duplicated inserver/routes/cli-auth.js(lines 111-125). Both implementations read from the same path and have identical logic. Consider extracting to a shared module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/claude-sdk.js` around lines 472 - 485, Duplicate implementation of loadClaudeSettingsEnv exists; extract it into a single shared module (e.g., create a new module that exports loadClaudeSettingsEnv) and replace both local copies with imports. Preserve the original behavior (async function, reading path.join(os.homedir(), '.claude', 'settings.json'), JSON.parse, returning parsed.env when an object, catching errors and returning {}), export it as a named function (loadClaudeSettingsEnv) from the new module, update the callers (the implementations currently in claude-sdk.js and cli-auth.js) to import that function and remove the duplicate code blocks.
463-470: Consider extractingisTruthyValueto a shared utility.This helper is duplicated in
server/routes/cli-auth.js(lines 9-16). Extracting it to a shared utility module would reduce duplication and ensure consistent behavior.♻️ Suggested refactor
Create a shared utility file:
// server/utils/env-helpers.js export function isTruthyValue(value) { if (typeof value !== 'string') { return false; } const normalized = value.trim().toLowerCase(); return normalized === '1' || normalized === 'true' || normalized === 'yes' || normalized === 'on'; }Then import in both files:
import { isTruthyValue } from './utils/env-helpers.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/claude-sdk.js` around lines 463 - 470, The helper isTruthyValue is duplicated; extract it into a shared utility module (e.g., env-helpers) as a named export isTruthyValue, remove the local definitions in the modules that currently define it, and update those modules to import { isTruthyValue } from the new utility; ensure the exported function preserves the same string-check behavior and replace the local calls with the imported function everywhere the duplicate existed (including the other module that currently contains lines 9-16).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@server/routes/cli-auth.js`:
- Around line 166-172: The ANTHROPIC_API_KEY return path is missing the
isBedrock property; update the object returned inside the conditional that
checks process.env.ANTHROPIC_API_KEY (the block returning authenticated: true,
email: 'API Key Auth', method: 'api_key') to include isBedrock: false so it
matches the other api_key return paths and avoids undefined values.
In `@src/components/settings/hooks/useSettingsController.ts`:
- Around line 276-284: The failure branch that runs when response.ok is false
calls setAuthStatusByProvider(provider, {...}) but omits the isBedrock property;
update that object to include isBedrock: false (matching the other
failure/success branches) so setAuthStatusByProvider receives a complete status
shape for provider and won’t leave isBedrock undefined.
---
Nitpick comments:
In `@server/claude-sdk.js`:
- Around line 472-485: Duplicate implementation of loadClaudeSettingsEnv exists;
extract it into a single shared module (e.g., create a new module that exports
loadClaudeSettingsEnv) and replace both local copies with imports. Preserve the
original behavior (async function, reading path.join(os.homedir(), '.claude',
'settings.json'), JSON.parse, returning parsed.env when an object, catching
errors and returning {}), export it as a named function (loadClaudeSettingsEnv)
from the new module, update the callers (the implementations currently in
claude-sdk.js and cli-auth.js) to import that function and remove the duplicate
code blocks.
- Around line 463-470: The helper isTruthyValue is duplicated; extract it into a
shared utility module (e.g., env-helpers) as a named export isTruthyValue,
remove the local definitions in the modules that currently define it, and update
those modules to import { isTruthyValue } from the new utility; ensure the
exported function preserves the same string-check behavior and replace the local
calls with the imported function everywhere the duplicate existed (including the
other module that currently contains lines 9-16).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 73fe83d1-5fe0-4878-b1bb-d84fbc8a59be
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
.env.exampleserver/claude-sdk.jsserver/routes/cli-auth.jssrc/components/settings/constants/constants.tssrc/components/settings/hooks/useSettingsController.tssrc/components/settings/types/types.tssrc/components/settings/view/tabs/agents-settings/sections/content/AccountContent.tsx
Author
|
Addressing CodeRabbit review feedback:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/utils/env-helpers.js (1)
25-38: Consider caching to avoid redundant file reads during a single request.Per context snippets,
loadClaudeSettingsEnv()is called from bothcheckClaudeCredentials()(cli-auth.js:125) andmapCliOptionsToSDK()(claude-sdk.js:196). A single request flow that checks auth and then processes through the SDK will read~/.claude/settings.jsontwice.While the current implementation is correct and OS-level file caching mitigates most overhead, a simple module-level cache with TTL would reduce I/O and ensure consistent snapshots within a request.
💡 Optional: Add simple caching
+let cachedSettingsEnv = null; +let cacheTimestamp = 0; +const CACHE_TTL_MS = 5000; // 5 seconds + export async function loadClaudeSettingsEnv() { + const now = Date.now(); + if (cachedSettingsEnv && (now - cacheTimestamp) < CACHE_TTL_MS) { + return cachedSettingsEnv; + } + try { const settingsPath = path.join(os.homedir(), '.claude', 'settings.json'); const content = await fs.readFile(settingsPath, 'utf8'); const settings = JSON.parse(content); if (settings?.env && typeof settings.env === 'object') { + cachedSettingsEnv = settings.env; + cacheTimestamp = now; return settings.env; } } catch { // Ignore missing or malformed settings and fall back to empty. } + cachedSettingsEnv = {}; + cacheTimestamp = now; return {}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/env-helpers.js` around lines 25 - 38, loadClaudeSettingsEnv currently rereads ~/.claude/settings.json on every call causing duplicate I/O when invoked by checkClaudeCredentials and mapCliOptionsToSDK; add a simple module-level cache (e.g., cachedEnv and cachedAt) and a TTL (e.g., 1–5 seconds) so loadClaudeSettingsEnv returns the cached env if cachedAt is recent, otherwise reads the file, updates cachedEnv and cachedAt, and returns the result; preserve current error swallowing and fallback-to-empty behavior and ensure cache stores the parsed settings.env object (or {}).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/utils/env-helpers.js`:
- Around line 25-38: loadClaudeSettingsEnv currently rereads
~/.claude/settings.json on every call causing duplicate I/O when invoked by
checkClaudeCredentials and mapCliOptionsToSDK; add a simple module-level cache
(e.g., cachedEnv and cachedAt) and a TTL (e.g., 1–5 seconds) so
loadClaudeSettingsEnv returns the cached env if cachedAt is recent, otherwise
reads the file, updates cachedEnv and cachedAt, and returns the result; preserve
current error swallowing and fallback-to-empty behavior and ensure cache stores
the parsed settings.env object (or {}).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9b153b0b-0d85-4f7b-8e2e-f789649eb8c5
📒 Files selected for processing (4)
server/claude-sdk.jsserver/routes/cli-auth.jsserver/utils/env-helpers.jssrc/components/settings/hooks/useSettingsController.ts
Contributor
|
@tintnc can you look at the PR again and the claude agent sdk requirements for env variables? AWS_ACCESS_KEY_ID=your-access-key-idAWS_SECRET_ACCESS_KEY=your-secret-access-keyOnce you are done, please put your PR back to open so we can have a look at it. |
tintnc
force-pushed
the
feat/aws_bedrock_support
branch
from
cd67122 to
d5cd3d4
Compare
Author
|
Thanks @viper151, I have updated accordingly for what need to be used on the |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 58-63: Remove the unused AWS_BEARER_TOKEN_BEDROCK entry and its
explanatory comment from .env.example: delete the line defining
AWS_BEARER_TOKEN_BEDROCK and any lines that suggest using a bearer token for
Bedrock authentication, and ensure the remaining comments reference only
standard AWS SDK auth (e.g., AWS_REGION, AWS_PROFILE,
AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY or IAM roles) and the
CLAUDE_CODE_USE_BEDROCK flag so users are not misled about supported
authentication methods.
In `@server/claude-sdk.js`:
- Around line 478-504: In resolveClaudeModel, a user-supplied options.model
passed as modelAlias can be overridden by environment-derived explicitModel when
bedrock is enabled; to fix, add an early check in resolveClaudeModel that if
modelAlias is truthy and not one of the special alias tokens
('haiku','opus','opusplan','sonnet','sonnet[1m]') then return modelAlias
(requestedModel) immediately so a direct CLI/API model selection always has
highest precedence before falling back to explicitModel, explicitFastModel,
sonnetDefault, opusDefault, or haikuDefault.
In `@server/routes/cli-auth.js`:
- Around line 127-130: The bedrock enablement check is duplicated (the local
bedrockEnabled variable using isTruthyValue and settingsEnv) and should be
centralized: add a new helper function isBedrockEnabled(settingsEnv = {}) in
server/utils/env-helpers.js that returns the existing expression
(isTruthyValue(process.env.CLAUDE_CODE_USE_BEDROCK) ||
isTruthyValue(settingsEnv.CLAUDE_CODE_USE_BEDROCK)), then update the usage in
server/routes/cli-auth.js (replace the bedrockEnabled declaration) and in
server/claude-sdk.js (replace the resolver’s bedrock check) to import and call
isBedrockEnabled so both files rely on the same implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7a45a4d5-833a-41ba-8b82-c6ce7a5b3d66
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
.env.exampleserver/claude-sdk.jsserver/routes/cli-auth.jsserver/utils/env-helpers.jssrc/components/settings/constants/constants.tssrc/components/settings/hooks/useSettingsController.tssrc/components/settings/types/types.tssrc/components/settings/view/tabs/agents-settings/sections/content/AccountContent.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/settings/types/types.ts
- server/utils/env-helpers.js
- src/components/settings/hooks/useSettingsController.ts
Comment on lines
+127
to
+130
| // Priority 0: Bedrock mode bypasses Claude OAuth checks. | ||
| const bedrockEnabled = isTruthyValue(process.env.CLAUDE_CODE_USE_BEDROCK) | ||
| || isTruthyValue(settingsEnv.CLAUDE_CODE_USE_BEDROCK); | ||
| if (bedrockEnabled) { |
Contributor
There was a problem hiding this comment.
Bedrock enablement logic is duplicated and can diverge from SDK behavior.
This inline check can drift from the resolver used in server/claude-sdk.js, causing status/UI to report Bedrock while SDK model resolution behaves differently. Centralize Bedrock detection in one shared helper and reuse it in both files.
Proposed fix
-import { isTruthyValue, loadClaudeSettingsEnv } from '../utils/env-helpers.js';
+import { isBedrockEnabled, loadClaudeSettingsEnv } from '../utils/env-helpers.js';
@@
- const bedrockEnabled = isTruthyValue(process.env.CLAUDE_CODE_USE_BEDROCK)
- || isTruthyValue(settingsEnv.CLAUDE_CODE_USE_BEDROCK);
+ const bedrockEnabled = isBedrockEnabled(settingsEnv);// server/utils/env-helpers.js
export function isBedrockEnabled(settingsEnv = {}) {
return isTruthyValue(process.env.CLAUDE_CODE_USE_BEDROCK)
|| isTruthyValue(settingsEnv.CLAUDE_CODE_USE_BEDROCK);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/routes/cli-auth.js` around lines 127 - 130, The bedrock enablement
check is duplicated (the local bedrockEnabled variable using isTruthyValue and
settingsEnv) and should be centralized: add a new helper function
isBedrockEnabled(settingsEnv = {}) in server/utils/env-helpers.js that returns
the existing expression (isTruthyValue(process.env.CLAUDE_CODE_USE_BEDROCK) ||
isTruthyValue(settingsEnv.CLAUDE_CODE_USE_BEDROCK)), then update the usage in
server/routes/cli-auth.js (replace the bedrockEnabled declaration) and in
server/claude-sdk.js (replace the resolver’s bedrock check) to import and call
isBedrockEnabled so both files rely on the same implementation.
viper151
requested changes
Mar 17, 2026
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
2 participants
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.
Inspired by: #306
.env.exampleto simplify initial setup.Summary by CodeRabbit
New Features
Documentation