feat: implement AWS Bedrock integration and update authentication han…#548
feat: implement AWS Bedrock integration and update authentication han…#548tintnc wants to merge 3 commits intositeboon:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds optional AWS Bedrock configuration and Bedrock-aware Claude model resolution; introduces helpers to read ~/.claude/settings.json; propagates an isBedrock flag through server auth responses and frontend types/UI; makes mapCliOptionsToSDK async to await settings-driven resolution. 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 or operation
Server->>SettingsFile: loadClaudeSettingsEnv()
SettingsFile-->>Server: env object (may be {})
alt Bedrock enabled (env or settings)
Server->>Server: resolveClaudeModel(alias, settingsEnv)
Server->>Bedrock: call Bedrock (no API key)
Bedrock-->>Server: respond (method: bedrock)
Server-->>Client: { authenticated: true, method: "bedrock", isBedrock: true }
else Non-Bedrock
Server->>Server: check API key / oauth / settings
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 Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
|
Addressing CodeRabbit review feedback:
|
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
|
@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. |
2 participants
Inspired by: #306
.env.exampleto simplify initial setup.Summary by CodeRabbit
New Features
Documentation