Conversation
|
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 a centralized predicate Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway as Bifrost Gateway
participant Transport as Bifrost Transport
participant AnthropicAPI as Anthropic API
Client->>Gateway: HTTP request (includes User-Agent)
Gateway->>Gateway: Build BifrostContext (ctx)
Gateway->>Gateway: IsClaudeCodeRequest(ctx)?
alt Claude CLI request
Gateway->>Transport: Forward request WITHOUT X-Api-Key (passthrough flag set)
Transport->>AnthropicAPI: Send request (Authorization may be present)
AnthropicAPI-->>Transport: Response / SSE stream (RawResponse possible)
Transport->>Gateway: Return RawResponse (or fallback converted stream)
Gateway-->>Client: Return response/stream (passthrough)
else Non-Claude request
Gateway->>Gateway: Inject X-Api-Key header
Gateway->>Transport: Forward request WITH X-Api-Key
Transport->>AnthropicAPI: Send request
AnthropicAPI-->>Transport: Response / SSE stream
Transport->>Gateway: Convert/process stream into Anthropic responses
Gateway-->>Client: Return processed response/stream
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@core/providers/anthropic/responses.go`:
- Line 14: The file is importing its own package which causes an import cycle;
remove the self-import "github.com/maximhq/bifrost/core/providers/anthropic"
from core/providers/anthropic/responses.go and replace all qualified calls that
use the package prefix (e.g. anthropic.IsClaudeCodeRequest) with the unqualified
function name IsClaudeCodeRequest (the function is declared in utils.go in the
same package); update every occurrence of the qualified call sites in
responses.go to call IsClaudeCodeRequest directly.
akshaydeo
force-pushed
the
02-09-dont_use_forward_api_key_in_case_of_claude_code_request
branch
from
e46deb8 to
2f14280
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@core/providers/anthropic/utils.go`:
- Around line 1593-1600: The integration layer is doing a case-sensitive check
on the raw User-Agent so "Claude-CLI" never sets the context; update the check
in transports/bifrost-http/integrations/anthropic.go (the logic around the
User-Agent check that currently gates setting the BifrostContextKeyUserAgent) to
perform a case-insensitive match (e.g., normalize userAgent with
strings.ToLower() or use a case-insensitive comparison) before deciding to set
the context value; leave IsClaudeCodeRequest in
core/providers/anthropic/utils.go unchanged since it consumes the normalized
context value.
2f14280 to
80728ba
Compare
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)
ui/app/workspace/providers/views/modelProviderConfig.tsx (1)
22-27:⚠️ Potential issue | 🟡 MinorAdd
custom_provider_configpresence to memo dependencies.
Whencustom_provider_configtransitions fromundefinedto a defined object (even withoutis_key_less), the memo logic changes—it switches from returningkeysRequired(provider.name)to returning!(provider.custom_provider_config?.is_key_less ?? false). The current dependency on onlyis_key_lessmisses this object presence change. Include the object itself in the dependency array to capture the transition.✏️ Suggested fix
- }, [provider.name, provider.custom_provider_config?.is_key_less]); + }, [provider.name, provider.custom_provider_config]);
🤖 Fix all issues with AI agents
In `@ui/app/workspace/providers/dialogs/providerConfigSheet.tsx`:
- Around line 81-83: The grid column count is hardcoded as repeat(${tabs.length
+ 3}, 1fr) causing extra empty columns; update the TabsList gridTemplateColumns
to use the actual rendered tab count (e.g., repeat(${tabs.length}, 1fr)) or
compute tabs.length plus the exact number of additional controls if there are
extra action buttons, so the number of columns matches the UI elements; locate
this change in the TabsList usage in providerConfigSheet.tsx.
🧹 Nitpick comments (2)
ui/components/sidebar.tsx (1)
849-862: Indentation inconsistency on cleanup code.The logic is correct, but lines 859-861 have extra indentation compared to the surrounding code within the same block. This is a minor formatting nit.
🧹 Suggested formatting fix
} else if (e.key === "Enter") { e.preventDefault(); const target = navigableItems[focusedIndex]; if (target) { const url = target.queryParam ? `${target.url}?tab=${target.queryParam}` : target.url; if (target.isExternal || e.metaKey || e.ctrlKey) { window.open(url, "_blank"); } else { router.push(url); } - setSearchQuery(""); - setFocusedIndex(-1); - searchInputRef.current?.blur(); + setSearchQuery(""); + setFocusedIndex(-1); + searchInputRef.current?.blur(); }core/providers/anthropic/anthropic.go (1)
866-869: Fix applied correctly; minor trailing whitespace.The logic is correct and consistent. Line 866 has trailing whitespace after the tab character which could be cleaned up for consistency.
🧹 Suggested formatting fix
- - if key.Value.GetValue() != "" && !IsClaudeCodeRequest(ctx) { + if key.Value.GetValue() != "" && !IsClaudeCodeRequest(ctx) {
akshaydeo
force-pushed
the
02-09-dont_use_forward_api_key_in_case_of_claude_code_request
branch
from
80728ba to
c984fd6
Compare
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 (2)
ui/app/workspace/routing-rules/views/routingRulesView.tsx (1)
21-36:⚠️ Potential issue | 🟠 MajorAdd
canEditpermission check to gate the Edit action, consistent with Delete handling.The Edit button in
RoutingRulesTable(line 165) renders unconditionally despiteRbacOperation.Updatebeing defined in the RBAC model. Meanwhile, the Delete button (line 168) correctly respects thecanDeletepermission. Add acanEditcheck inroutingRulesView.tsx(checkingRbacOperation.Update) and pass it toRoutingRulesTableas a prop, then gate the Edit button similar to how Delete is gated. This prevents unauthorized users from seeing and triggering the edit UI.ui/app/workspace/providers/views/modelProviderConfig.tsx (1)
22-27:⚠️ Potential issue | 🟠 Major
showApiKeysmemo can go stale when custom config presence flips.The dependency list only tracks
provider.custom_provider_config?.is_key_less; ifcustom_provider_configis added/removed whileis_key_lessremains undefined, the memo won't recompute and theisKeylessprop passed toModelProviderKeysTableView(line 39) will reflect stale logic. Include the presence check in deps or compute inline.🐛 Proposed fix
- }, [provider.name, provider.custom_provider_config?.is_key_less]); + }, [provider.name, !!provider.custom_provider_config, provider.custom_provider_config?.is_key_less]);
🤖 Fix all issues with AI agents
In `@ui/app/workspace/providers/views/modelProviderKeysTableView.tsx`:
- Around line 111-115: The copy shown when isKeyless is true assumes a header
action exists; update the JSX in the isKeyless rendering block (the conditional
that renders the keyless message) to either (a) check for the presence of
headerActions and render “You can edit the provider configuration using the
button above.” only when headerActions is provided, otherwise render a neutral
message like “You can edit the provider configuration from the provider
settings.”, or (b) enforce headerActions via prop validation/TypeScript types so
callers must supply it; adjust the message logic in the component where
isKeyless is used accordingly (refer to isKeyless and headerActions in
modelProviderKeysTableView.tsx).
🧹 Nitpick comments (1)
transports/bifrost-http/integrations/anthropic.go (1)
115-125: Consider logging marshal failures for observability.When marshaling events for the combined response, errors are silently ignored with
continue. While this is reasonable for streaming (to send as much data as possible), silent failures can make debugging difficult in production.🔧 Add debug logging for marshal failures
for _, event := range anthropicResponse { responseJSON, err := sonic.Marshal(event) if err != nil { + // Log at debug level to aid troubleshooting without noise + if logger := ctx.Value(schemas.BifrostContextKeyLogger); logger != nil { + if l, ok := logger.(schemas.Logger); ok { + l.Debug("failed to marshal stream event", "error", err, "eventType", event.Type) + } + } continue }
akshaydeo
force-pushed
the
02-09-dont_use_forward_api_key_in_case_of_claude_code_request
branch
from
c984fd6 to
670f5b4
Compare
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)
ui/app/workspace/providers/fragments/performanceFormFragment.tsx (1)
44-54:⚠️ Potential issue | 🟠 MajorGuard the reset to avoid overwriting unsaved edits.
This effect resets whenever provider fields change, even if the user has unsaved edits, which can wipe input. Add an isDirty guard (or keepDirty) before resetting.Based on learnings: “In the Bifrost UI, when an external state change may affect a form (e.g., provider changes), do not reset the form or trigger updates if the user has unsaved edits. Check form.formState.isDirty and preserve user edits; only update underlying information or navigate/redirect when the form is clean.”🛠️ Suggested fix
-useEffect(() => { - // Reset form with new provider's concurrency_and_buffer_size when provider changes - form.reset({ - concurrency_and_buffer_size: { - concurrency: provider.concurrency_and_buffer_size?.concurrency ?? DefaultPerformanceConfig.concurrency, - buffer_size: provider.concurrency_and_buffer_size?.buffer_size ?? DefaultPerformanceConfig.buffer_size, - }, - send_back_raw_request: provider.send_back_raw_request ?? false, - send_back_raw_response: provider.send_back_raw_response ?? false, - }); -}, [form, provider.name, provider.concurrency_and_buffer_size, provider.send_back_raw_request, provider.send_back_raw_response]); +useEffect(() => { + if (form.formState.isDirty) return; + // Reset form with new provider's concurrency_and_buffer_size when provider changes + form.reset({ + concurrency_and_buffer_size: { + concurrency: provider.concurrency_and_buffer_size?.concurrency ?? DefaultPerformanceConfig.concurrency, + buffer_size: provider.concurrency_and_buffer_size?.buffer_size ?? DefaultPerformanceConfig.buffer_size, + }, + send_back_raw_request: provider.send_back_raw_request ?? false, + send_back_raw_response: provider.send_back_raw_response ?? false, + }); +}, [ + form, + form.formState.isDirty, + provider.name, + provider.concurrency_and_buffer_size, + provider.send_back_raw_request, + provider.send_back_raw_response, +]);
🤖 Fix all issues with AI agents
In `@ui/components/sidebar.tsx`:
- Around line 218-234: The two window.open calls in handleNavigation and
handleSubItemClick are vulnerable to reverse tabnabbing; add the noopener and
noreferrer protections and centralize the logic by creating a helper (e.g.,
openInNewTab(url: string)) that calls window.open(url, "_blank",
"noopener,noreferrer") and use that helper from both handleNavigation and
handleSubItemClick instead of calling window.open directly. Ensure the helper is
imported/defined in the same module and used both where isExternal/ctrl/meta
clicks are handled so the behavior is consistent and maintainable.
🧹 Nitpick comments (1)
tests/e2e/features/providers/pages/providers.page.ts (1)
402-411: Align the helper comment with the actual interaction.
The comment mentions native value setter/input event dispatch, but the implementation selects and types. Suggest updating the comment to avoid drift.♻️ Suggested comment update
- /** - * Fill a React controlled number input by using the native value setter - * and dispatching an input event. This bypasses React's value tracker - * to reliably update controlled input components. - */ + /** + * Fill a controlled number input by selecting all text and typing sequentially. + * Mirrors user input to ensure React-controlled fields update reliably. + */
akshaydeo
force-pushed
the
02-09-dont_use_forward_api_key_in_case_of_claude_code_request
branch
2 times, most recently
from
74bbd13 to
65e9318
Compare
akshaydeo
force-pushed
the
02-09-dont_use_forward_api_key_in_case_of_claude_code_request
branch
from
65e9318 to
5e5d631
Compare
Merge activity
|
akshaydeo
deleted the
02-09-dont_use_forward_api_key_in_case_of_claude_code_request
branch
1 participant
Summary
Improve Claude Code (claude-cli) integration by refactoring the detection logic and fixing authentication handling for passthrough requests.
Issues
Closes #1551
Changes
IsClaudeCodeRequestutility functionType of change
Affected areas
How to test
Breaking changes
Related issues
Fixes issues with claude-cli authentication and WebSearch tool handling.
Security considerations
Ensures proper authentication header handling for Claude Code requests.
Checklist
docs/contributing/README.mdand followed the guidelines