feat: add plugin TransportInterceptor interface and improve fallback logging #593
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (16)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
ui/app/observability/views/observabilityView.tsx (2)
77-93: AddsupportedPlatformsto the dependency array.The effect accesses
supportedPlatforms[0]on line 80 but doesn't includesupportedPlatformsin the dependency array. This can lead to stale closures, especially when the theme changes andsupportedPlatformsis recomputed. The default plugin selection won't update correctly if the theme changes after initial load.Apply this diff to fix the dependency array:
useEffect(() => { if (!plugins || plugins.length === 0) return; if (!selectedPluginId) { setSelectedPluginId(plugins.find((plugin) => plugin.name === supportedPlatforms[0].id)?.name ?? supportedPlatforms[0].id); } else { dispatch( setSelectedPlugin( plugins.find((plugin) => plugin.name === selectedPluginId) ?? { name: selectedPluginId, enabled: false, config: undefined, }, ), ); } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [plugins]); + }, [plugins, supportedPlatforms, selectedPluginId, dispatch, setSelectedPluginId]);
95-109: AddsupportedPlatformsto the dependency array.The effect accesses
supportedPlatforms[0].idon line 107 but doesn't includesupportedPlatformsin the dependency array. When the theme changes andsupportedPlatformsis recomputed, the fallback logic won't use the updated value.Apply this diff to fix the dependency array:
useEffect(() => { if (selectedPluginId) { dispatch( setSelectedPlugin( plugins?.find((plugin) => plugin.name === selectedPluginId) ?? { name: selectedPluginId, enabled: false, config: {}, }, ), ); } else { setSelectedPluginId(supportedPlatforms[0].id); } - }, [selectedPluginId]); + }, [selectedPluginId, supportedPlatforms, plugins, dispatch, setSelectedPluginId]);transports/bifrost-http/lib/config.go (1)
854-867: GetProviderConfigRaw returns a shallow copy; fix the commentMap lookups on struct values return a copy; returning &config points to a copied struct, but slice/map fields inside are shared. Current comment claims “direct reference,” which is misleading.
Apply this diff to clarify:
- // Return direct reference for maximum performance - this is used by Bifrost core - // CRITICAL: Never modify the returned data as it's shared + // Returns a pointer to a shallow copy of the stored config. Slice/map fields + // inside share underlying data with the stored config. Treat as read-only.plugins/governance/main.go (1)
366-369: Compile error: undefined ContextKey; use the same key used in PreHookPostHook calls getStringFromContext with ContextKey(...), which isn’t defined here. PreHook passes schemas.BifrostContextKeyVirtualKeyHeader directly. Do the same.
Apply this diff:
- virtualKey := getStringFromContext(*ctx, ContextKey(schemas.BifrostContextKeyVirtualKeyHeader)) + virtualKey := getStringFromContext(*ctx, schemas.BifrostContextKeyVirtualKeyHeader)
🧹 Nitpick comments (11)
ui/app/observability/views/observabilityView.tsx (2)
24-65: Consider moving the function outside the component.The
supportedPlatformsListfunction is recreated on every render. While the memoization on line 75 prevents unnecessary recalculations of the result, defining the function outside the component would be more idiomatic.Apply this diff to move the function outside:
+type SupportedPlatform = { + id: string; + name: string; + icon: React.ReactNode; + tag?: string; + disabled?: boolean; +}; + +const supportedPlatformsList = (resolvedTheme: string): SupportedPlatform[] => [ + { + id: "otel", + name: "Open Telemetry", + icon: ( + <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 128 128" width={21} height={21}> + <path + fill="#f5a800" + d="M67.648 69.797c-5.246 5.25-5.246 13.758 0 19.008 5.25 5.246 13.758 5.246 19.004 0 5.25-5.25 5.25-13.758 0-19.008-5.246-5.246-13.754-5.246-19.004 0Zm14.207 14.219a6.649 6.649 0 0 1-9.41 0 6.65 6.65 0 0 1 0-9.407 6.649 6.649 0 0 1 9.41 0c2.598 2.586 2.598 6.809 0 9.407ZM86.43 3.672l-8.235 8.234a4.17 4.17 0 0 0 0 5.875l32.149 32.149a4.17 4.17 0 0 0 5.875 0l8.234-8.235c1.61-1.61 1.61-4.261 0-5.87L92.29 3.671a4.159 4.159 0 0 0-5.86 0ZM28.738 108.895a3.763 3.763 0 0 0 0-5.31l-4.183-4.187a3.768 3.768 0 0 0-5.313 0l-8.644 8.649-.016.012-2.371-2.375c-1.313-1.313-3.45-1.313-4.75 0-1.313 1.312-1.313 3.449 0 4.75l14.246 14.242a3.353 3.353 0 0 0 4.746 0c1.3-1.313 1.313-3.45 0-4.746l-2.375-2.375.016-.012Zm0 0" + /> + <path + fill="#425cc7" + d="M72.297 27.313 54.004 45.605c-1.625 1.625-1.625 4.301 0 5.926L65.3 62.824c7.984-5.746 19.18-5.035 26.363 2.153l9.148-9.149c1.622-1.625 1.622-4.297 0-5.922L78.22 27.313a4.185 4.185 0 0 0-5.922 0ZM60.55 67.585l-6.672-6.672c-1.563-1.562-4.125-1.562-5.684 0l-23.53 23.54a4.036 4.036 0 0 0 0 5.687l13.331 13.332a4.036 4.036 0 0 0 5.688 0l15.132-15.157c-3.199-6.609-2.625-14.593 1.735-20.73Zm0 0" + /> + </svg> + ), + tag: "Beta", + }, + { + id: "maxim", + name: "Maxim", + icon: <Image alt="Maxim" src={`/maxim-logo${resolvedTheme === "dark" ? "-dark" : ""}.png`} width={19} height={19} />, + }, + { + id: "datadog", + name: "Datadog", + icon: <Image alt="Datadog" src="/images/datadog-logo.png" width={32} height={32} />, + disabled: true, + }, + { + id: "newrelic", + name: "New Relic", + icon: ( + <svg viewBox="0 0 832.8 959.8" xmlns="http://www.w3.org/2000/svg" width="19" height="19"> + <path d="M672.6 332.3l160.2-92.4v480L416.4 959.8V775.2l256.2-147.6z" fill="#00ac69" /> + <path d="M416.4 184.6L160.2 332.3 0 239.9 416.4 0l416.4 239.9-160.2 92.4z" fill="#1ce783" /> + <path d="M256.2 572.3L0 424.6V239.9l416.4 240v479.9l-160.2-92.2z" fill="#1d252c" /> + </svg> + ), + disabled: true, + }, +]; + export default function ObservabilityView() { const dispatch = useAppDispatch(); const { data: plugins, isLoading } = useGetPluginsQuery(); const [selectedPluginId, setSelectedPluginId] = useQueryState("plugin"); const selectedPlugin = useAppSelector((state) => state.plugin.selectedPlugin); - -type SupportedPlatform = { - id: string; - name: string; - icon: React.ReactNode; - tag?: string; - disabled?: boolean; -}; - -const supportedPlatformsList = (resolvedTheme: string): SupportedPlatform[] => [ - { - id: "otel", - name: "Open Telemetry", - icon: ( - <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 128 128" width={21} height={21}> - <path - fill="#f5a800" - d="M67.648 69.797c-5.246 5.25-5.246 13.758 0 19.008 5.25 5.246 13.758 5.246 19.004 0 5.25-5.25 5.25-13.758 0-19.008-5.246-5.246-13.754-5.246-19.004 0Zm14.207 14.219a6.649 6.649 0 0 1-9.41 0 6.65 6.65 0 0 1 0-9.407 6.649 6.649 0 0 1 9.41 0c2.598 2.586 2.598 6.809 0 9.407ZM86.43 3.672l-8.235 8.234a4.17 4.17 0 0 0 0 5.875l32.149 32.149a4.17 4.17 0 0 0 5.875 0l8.234-8.235c1.61-1.61 1.61-4.261 0-5.87L92.29 3.671a4.159 4.159 0 0 0-5.86 0ZM28.738 108.895a3.763 3.763 0 0 0 0-5.31l-4.183-4.187a3.768 3.768 0 0 0-5.313 0l-8.644 8.649-.016.012-2.371-2.375c-1.313-1.313-3.45-1.313-4.75 0-1.313 1.312-1.313 3.449 0 4.75l14.246 14.242a3.353 3.353 0 0 0 4.746 0c1.3-1.313 1.313-3.45 0-4.746l-2.375-2.375.016-.012Zm0 0" - /> - <path - fill="#425cc7" - d="M72.297 27.313 54.004 45.605c-1.625 1.625-1.625 4.301 0 5.926L65.3 62.824c7.984-5.746 19.18-5.035 26.363 2.153l9.148-9.149c1.622-1.625 1.622-4.297 0-5.922L78.22 27.313a4.185 4.185 0 0 0-5.922 0ZM60.55 67.585l-6.672-6.672c-1.563-1.562-4.125-1.562-5.684 0l-23.53 23.54a4.036 4.036 0 0 0 0 5.687l13.331 13.332a4.036 4.036 0 0 0 5.688 0l15.132-15.157c-3.199-6.609-2.625-14.593 1.735-20.73Zm0 0" - /> - </svg> - ), - tag: "Beta", - }, - { - id: "maxim", - name: "Maxim", - icon: <Image alt="Maxim" src={`/maxim-logo${resolvedTheme === "dark" ? "-dark" : ""}.png`} width={19} height={19} />, - }, - { - id: "datadog", - name: "Datadog", - icon: <Image alt="Datadog" src="/images/datadog-logo.png" width={32} height={32} />, - disabled: true, - }, - { - id: "newrelic", - name: "New Relic", - icon: ( - <svg viewBox="0 0 832.8 959.8" xmlns="http://www.w3.org/2000/svg" width="19" height="19"> - <path d="M672.6 332.3l160.2-92.4v480L416.4 959.8V775.2l256.2-147.6z" fill="#00ac69" /> - <path d="M416.4 184.6L160.2 332.3 0 239.9 416.4 0l416.4 239.9-160.2 92.4z" fill="#1ce783" /> - <path d="M256.2 572.3L0 424.6V239.9l416.4 240v479.9l-160.2-92.2z" fill="#1d252c" /> - </svg> - ), - disabled: true, - }, -];
45-45: Image paths verified; consolidate logo assets All referenced images exist under ui/public. For consistency, consolidate all platform logos in the same directory (e.g., public/images).core/schemas/plugin.go (1)
21-27: Document TransportInterceptor ordering and error semanticsPlease clarify:
- Ordering: Is TransportInterceptor run in plugin registration order (like PreHook)? Is it sequential or can plugins run in parallel?
- Error semantics: Are TransportInterceptor errors treated like hook errors (never surfaced to caller, logged only), or can they short-circuit the HTTP transport?
This reduces ambiguity for plugin authors and keeps behavior consistent with PreHook/PostHook.
transports/bifrost-http/lib/lib.go (1)
7-12: Avoid nil logger usage; provide a safe defaultSet a no-op default to prevent nil derefs if SetLogger isn’t called early enough. Optionally expose GetLogger() that returns the safe logger.
Apply within this file:
-var logger schemas.Logger +var logger schemas.Logger = &noopLogger{}Add in the same package (new code):
// noopLogger implements schemas.Logger with no-ops. type noopLogger struct{} func (*noopLogger) Debug(string, ...any) {} func (*noopLogger) Info(string, ...any) {} func (*noopLogger) Warn(string, ...any) {} func (*noopLogger) Error(string, ...any) {} func (*noopLogger) Fatal(string, ...any) {} func (*noopLogger) SetLevel(schemas.LogLevel) {} func (*noopLogger) SetOutputType(schemas.LoggerOutputType) {}Please confirm all lib logger call sites tolerate nil today; otherwise, adopt the no-op default to be safe.
transports/bifrost-http/handlers/middlewares.go (3)
87-101: Pass full URL to plugins (scheme+host+path+query)Plugins may need the full URL. Prefer FullURI() over RequestURI().
Apply this diff:
- modifiedHeaders, modifiedBody, err := plugin.TransportInterceptor(string(ctx.Request.URI().RequestURI()), headers, requestBody) + modifiedHeaders, modifiedBody, err := plugin.TransportInterceptor(string(ctx.Request.URI().FullURI()), headers, requestBody)
50-61: Make interception independent of governance presenceGating interception on governance being loaded prevents other plugins from using the hook. Consider running interceptors regardless; governance can still act when present.
Apply this diff to remove the gate:
- // If governance plugin is not loaded, skip interception - hasGovernance := false - for _, p := range plugins { - if p.GetName() == governance.PluginName { - hasGovernance = true - break - } - } - if !hasGovernance { - next(ctx) - return - } + // Interceptors run regardless of governance presence
118-121: Treat empty header values as deletionsIf a plugin sets a header to empty string, delete it instead of setting an empty value.
Apply this diff:
- for key, value := range headers { - ctx.Request.Header.Set(key, value) - } + for key, value := range headers { + if value == "" { + ctx.Request.Header.Del(key) + continue + } + ctx.Request.Header.Set(key, value) + }transports/bifrost-http/lib/config.go (2)
118-118: Exported mutex: prefer encapsulation or document usage clearlyExposing Mu invites external locking patterns. If required, document when/how to lock, or add scoped helpers to avoid misuse.
879-905: Returning live slice snapshot; consider copy or document read-only contractGetLoadedPlugins returns the slice from the atomic pointer (read-only snapshot semantics assumed). Callers could mutate elements. Either:
- Document that the returned slice and contained plugins are read-only, or
- Return a copied slice to protect internals (slight alloc cost).
Optional defensive variant:
- if plugins := c.Plugins.Load(); plugins != nil { - return *plugins - } + if plugins := c.Plugins.Load(); plugins != nil { + // Copy slice header and elements (plugins are interfaces) + return append([]schemas.Plugin(nil), (*plugins)...) + }plugins/governance/main.go (2)
35-37: InMemoryStore API: avoid exposing raw maps; ensure thread-safetyReturning a map risks concurrent map read/write if the underlying map is mutated elsewhere. Prefer a safe query method (e.g., HasProvider(provider)) or return a copied map/snapshot.
If keeping GetConfiguredProviders, ensure the implementation returns a snapshot or uses locks to prevent concurrent map iteration panics.
Also applies to: 54-56
159-164: Minor: unnecessary string() conversions and header lookupheader and value are already strings; string(...) casts are redundant. Also consider normalizing once and doing direct lookup if upstream doesn’t already.
- for header, value := range headers { - if strings.ToLower(string(header)) == "x-bf-vk" { - virtualKeyValue = string(value) + for header, value := range headers { + if strings.ToLower(header) == "x-bf-vk" { + virtualKeyValue = value break } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
core/bifrost.go(6 hunks)core/schemas/plugin.go(2 hunks)plugins/governance/main.go(5 hunks)plugins/jsonparser/main.go(1 hunks)plugins/logging/main.go(1 hunks)plugins/maxim/main.go(1 hunks)plugins/mocker/main.go(1 hunks)plugins/otel/main.go(1 hunks)plugins/semanticcache/main.go(1 hunks)plugins/telemetry/main.go(1 hunks)transports/bifrost-http/handlers/middlewares.go(1 hunks)transports/bifrost-http/handlers/server.go(9 hunks)transports/bifrost-http/lib/config.go(15 hunks)transports/bifrost-http/lib/lib.go(1 hunks)transports/bifrost-http/main.go(2 hunks)ui/app/observability/views/observabilityView.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
plugins/maxim/main.go (1)
core/schemas/plugin.go (1)
Plugin(45-71)
plugins/semanticcache/main.go (1)
core/schemas/plugin.go (1)
Plugin(45-71)
core/bifrost.go (1)
core/schemas/bifrost.go (2)
RequestCancelled(520-520)BifrostContextKeyFallbackRequestID(115-115)
transports/bifrost-http/handlers/middlewares.go (4)
plugins/governance/main.go (2)
Config(31-33)PluginName(19-19)transports/bifrost-http/lib/config.go (1)
Config(117-146)transports/bifrost-http/lib/middleware.go (1)
BifrostHTTPMiddleware(7-7)transports/bifrost-http/handlers/utils.go (1)
SendError(25-34)
transports/bifrost-http/main.go (1)
transports/bifrost-http/lib/lib.go (1)
SetLogger(10-12)
transports/bifrost-http/lib/config.go (1)
core/schemas/plugin.go (2)
Plugin(45-71)PluginConfig(75-79)
transports/bifrost-http/lib/lib.go (1)
core/schemas/logger.go (1)
Logger(28-55)
transports/bifrost-http/handlers/server.go (4)
transports/bifrost-http/lib/config.go (1)
Config(117-146)framework/configstore/clientconfig.go (2)
ProviderConfig(43-50)GovernanceConfig(55-61)core/bifrost.go (1)
Init(79-194)transports/bifrost-http/handlers/middlewares.go (1)
TransportInterceptorMiddleware(40-126)
plugins/governance/main.go (4)
core/schemas/bifrost.go (1)
ModelProvider(46-46)transports/bifrost-http/lib/config.go (1)
Config(117-146)framework/configstore/store.go (1)
ConfigStore(16-118)core/schemas/utils.go (1)
ParseModelString(21-34)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (23)
ui/app/observability/views/observabilityView.tsx (3)
8-8: LGTM!The new imports are appropriate for the theme-aware functionality being added.
Also applies to: 13-14
16-22: LGTM!The
SupportedPlatformtype is well-defined with appropriate fields.
115-169: LGTM!The rendering logic correctly uses the theme-aware
supportedPlatformsarray and includes appropriate accessibility attributes.core/bifrost.go (6)
1099-1120: Helpful fallback decision tracingAdditional debug logs for no-error, cancellation, AllowFallbacks=false, and no-fallbacks cases improve diagnosability. LGTM.
1208-1211: Safe request pooling lifecycleDeferring releaseBifrostRequest here is correct; ChannelMessage stores a value copy of BifrostRequest, so fallbacks and streaming remain safe.
1224-1234: Primary error/fallback prep loggingClear traces around primary failure and fallback count are useful. LGTM.
1244-1250: Per-fallback attempt visibilityPer-fallback attempt and nil-skip logs help correlate failures. LGTM.
1275-1277: Streaming path: request release timingDeferring release in streaming path is appropriate; worker goroutines operate on ChannelMessage copies. LGTM.
1889-1890: Clarified release responsibilityComment clarifies request release ownership vs. ChannelMessage lifecycle. LGTM.
transports/bifrost-http/main.go (1)
128-129: Consistent logger wiringlib.SetLogger(logger) aligns transport lib logging with app logger. LGTM.
plugins/logging/main.go (1)
214-218: No-op interceptor to satisfy new interfaceImplementation is correct and preserves existing behavior. LGTM.
plugins/semanticcache/main.go (1)
338-342: No-op interceptor to satisfy new interfaceMatches the new Plugin surface without altering behavior. LGTM.
plugins/mocker/main.go (1)
481-485: No-op interceptor to satisfy new interfaceKeeps existing mocking behavior intact. LGTM.
plugins/jsonparser/main.go (1)
90-94: No-op interceptor to satisfy new interfaceCorrect minimal implementation with no behavior change. LGTM.
core/schemas/plugin.go (1)
49-54: Add context.Context and explicit map mutability guarantees to TransportInterceptor
- Extend
TransportInterceptor(url string, headers map[string]string, body map[string]any)to
TransportInterceptor(ctx context.Context, url string, headers map[string]string, body map[string]any) …
for cancellation, tracing, and per-request values.- Clarify whether plugin implementations must mutate
headers/bodyin place or return new copies (copy-on-write guidance).- If retaining the current signature, document:
- Plugin invocation order (e.g., registration sequence, sequential execution).
- Caller guarantees (e.g., middleware clones inputs to prevent cross-plugin side effects).
Manually verify that all plugin entrypoints define the updated
TransportInterceptorsignature.plugins/maxim/main.go (1)
120-123: No‑op TransportInterceptor looks goodMatches the new interface; safe passthrough.
plugins/otel/main.go (1)
113-116: No‑op TransportInterceptor added correctlyImplements the new hook without changing behavior.
plugins/telemetry/main.go (1)
71-74: No‑op TransportInterceptor added correctlyKeeps plugin behavior unchanged while satisfying the interface.
transports/bifrost-http/handlers/server.go (3)
302-304: Good: atomic publication of loaded pluginsUsing atomic.Pointer for lock-free reads is appropriate here.
342-345: Good: core reload uses the atomic view of pluginsPassing GetLoadedPlugins() keeps core in sync with published plugin state.
576-577: Good middleware chainingCORS wraps TransportInterceptor, ensuring OPTIONS are handled before interception.
transports/bifrost-http/handlers/middlewares.go (1)
79-82: logger availability confirmed: Package-levelloggeris declared in transports/bifrost-http/handlers/init.go; no undefined identifier issues.plugins/governance/main.go (1)
7-10: Go version compliant plugins/governance/go.mod declares Go 1.24.1 (>=1.22), satisfying the requirements for math/rand/v2 and slices.
Pratham-Mishra04
force-pushed
the
10-09-feat_transport_interceptor_method_added_to_plugin_schema_new
branch
from
ed9cd2d to
a570d84
Compare
Pratham-Mishra04
force-pushed
the
10-09-enhancement_extent_websocket_handler_for_general_usage_new
branch
from
60a4d43 to
8283de0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
transports/bifrost-http/handlers/server.go (1)
574-578: Note: Middleware initialization will fail due to missing logger.Line 576 calls
TransportInterceptorMiddleware(s.Config), but as flagged inmiddlewares.go, the middleware uses an undefinedloggervariable. Once that file is fixed to accept a logger parameter, update this line to passlogger.
🧹 Nitpick comments (2)
core/bifrost.go (1)
1224-1224: LGTM! Comprehensive fallback execution logging.The added debug logs provide clear visibility into the fallback decision-making and execution flow, which will aid in debugging and monitoring.
Optional nitpick: The log at line 1249 says "is nil" but technically the fallbackReq (not the provider) is nil. Consider "Skipping fallback: no config for provider %s with model %s" for clarity.
Also applies to: 1229-1234, 1244-1244, 1249-1249
plugins/governance/main.go (1)
157-177: Minor: Redundant string conversions.Lines 160-161:
headerandvaluefrom theAll()iterator are already[]byte(converted to string by the caller), sostring(header)andstring(value)are redundant. The middleware already converts them at lines 67-68 ofmiddlewares.go.This is harmless but can be simplified:
- if strings.ToLower(string(header)) == "x-bf-vk" { - virtualKeyValue = string(value) + if strings.ToLower(header) == "x-bf-vk" { + virtualKeyValue = value
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
core/bifrost.go(6 hunks)core/schemas/plugin.go(2 hunks)plugins/governance/main.go(5 hunks)plugins/jsonparser/main.go(1 hunks)plugins/logging/main.go(1 hunks)plugins/maxim/main.go(1 hunks)plugins/mocker/main.go(1 hunks)plugins/otel/main.go(1 hunks)plugins/semanticcache/main.go(1 hunks)plugins/telemetry/main.go(1 hunks)transports/bifrost-http/handlers/middlewares.go(1 hunks)transports/bifrost-http/handlers/server.go(9 hunks)transports/bifrost-http/lib/config.go(16 hunks)transports/bifrost-http/lib/lib.go(1 hunks)transports/bifrost-http/main.go(2 hunks)ui/app/observability/views/observabilityView.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- plugins/logging/main.go
- plugins/jsonparser/main.go
- plugins/mocker/main.go
- plugins/maxim/main.go
- transports/bifrost-http/lib/lib.go
🧰 Additional context used
🧬 Code graph analysis (4)
transports/bifrost-http/main.go (1)
transports/bifrost-http/lib/lib.go (1)
SetLogger(10-12)
transports/bifrost-http/handlers/middlewares.go (3)
transports/bifrost-http/lib/config.go (1)
Config(117-146)transports/bifrost-http/lib/middleware.go (1)
BifrostHTTPMiddleware(7-7)transports/bifrost-http/handlers/utils.go (1)
SendError(25-34)
transports/bifrost-http/handlers/server.go (3)
transports/bifrost-http/lib/config.go (1)
Config(117-146)framework/configstore/clientconfig.go (2)
ProviderConfig(43-50)GovernanceConfig(55-61)transports/bifrost-http/handlers/middlewares.go (1)
TransportInterceptorMiddleware(40-126)
transports/bifrost-http/lib/config.go (3)
plugins/semanticcache/main.go (2)
Plugin(137-144)Config(25-43)ui/lib/types/plugins.ts (1)
Plugin(6-10)framework/configstore/store.go (1)
ConfigStore(16-118)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (31)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (21)
transports/bifrost-http/main.go (1)
65-65: LGTM! Logger setup for lib package.The lib package import and logger initialization follow the established pattern used for the handlers package.
Also applies to: 128-128
plugins/otel/main.go (1)
113-116: LGTM! No-op TransportInterceptor implementation.The passthrough implementation is appropriate for this plugin, which performs tracing at the PreHook/PostHook stages rather than at the transport layer.
plugins/telemetry/main.go (1)
71-74: LGTM! No-op TransportInterceptor implementation.The passthrough implementation is appropriate for this telemetry plugin, which collects metrics via PreHook/PostHook rather than at the transport layer.
plugins/semanticcache/main.go (1)
338-341: LGTM! No-op TransportInterceptor implementation.The passthrough implementation is appropriate for this semantic cache plugin, which performs caching at the PreHook/PostHook stages.
core/bifrost.go (3)
1208-1208: LGTM! Proper BifrostRequest lifecycle management.The deferred release ensures that the original request object is returned to the pool even on early returns, preventing memory leaks.
1099-1099: LGTM! Enhanced fallback decision logging.These debug logs improve observability by clarifying why fallbacks were skipped, making it easier to debug fallback behavior.
Also applies to: 1105-1105, 1112-1112, 1118-1118
1889-1889: LGTM! Clear documentation of lifecycle responsibility.The updated comment accurately reflects the new lifecycle management where BifrostRequest release is handled via defer in the handle methods rather than in releaseChannelMessage.
core/schemas/plugin.go (2)
21-27: LGTM! Clear plugin execution order documentation.The updated documentation clearly specifies the execution order and scope of each hook, including the new TransportInterceptor which is explicitly noted as HTTP transport only.
49-53: LGTM! Well-documented TransportInterceptor addition.The new interface method is clearly documented and appropriately scoped to HTTP transport only.
Note: The headers signature uses
map[string]stringwhich supports single-value headers only. If multi-value header support is needed in the future (e.g., for multipleSet-Cookieheaders), this would require a signature change tomap[string][]string. The current signature is acceptable for typical JSON API use cases.transports/bifrost-http/handlers/middlewares.go (1)
40-61: LGTM! Clean early-exit pattern.The atomic plugin load and governance check provide efficient guards before processing.
transports/bifrost-http/handlers/server.go (5)
159-168: Lock-free read accepted per team preference.Returns shared map under read lock. As per learnings, the team accepts potential stale reads for hot-path performance.
Based on learnings.
253-306: LGTM! Correct atomic publication.Plugins are loaded into a local slice, then atomically published via
Store(&plugins)at line 303, ensuring lock-free readers see a consistent snapshot.
325-348: LGTM! Atomic plugin read is correct.Line 342 properly uses
GetLoadedPlugins()to retrieve the current plugin snapshot.
362-393: LGTM! Proper CAS retry loop.ReloadPlugin correctly implements compare-and-swap with retry on contention. The sync at line 388 keeps
BifrostHTTPServer.Pluginsconsistent.
395-425: LGTM! Proper CAS retry loop.RemovePlugin correctly implements compare-and-swap with retry on contention. The sync at line 420 keeps
BifrostHTTPServer.Pluginsconsistent.transports/bifrost-http/lib/config.go (4)
138-142: LGTM! Clean atomic plugin storage.Separating runtime
Plugins(atomic pointer) from config-sourcedPluginConfigsis a clear design that supports lock-free reads.
187-189: LGTM! Defensive initialization.Initializing the atomic pointer to an empty slice prevents nil dereferences if code is refactored. Current call sequence (
LoadConfig→LoadPlugins→GetLoadedPlugins) is safe.
883-909: LGTM! Lock-free plugin access helpers.Both methods safely dereference the atomic pointer and provide lock-free access.
IsPluginLoadeduses linear search, which is appropriate for small plugin counts as noted in the doc comment.
860-861: LGTM! Consistent use of exported mutex.All provider methods consistently use
s.Mufor synchronization. Exporting the mutex enables direct access from handlers/plugins as documented.Also applies to: 920-921, 1079-1080, 1099-1100, 1202-1203, 1286-1287, 1311-1312
plugins/governance/main.go (2)
35-37: LGTM! Well-documented interface and initialization.The
InMemoryStoreinterface provides a clean decoupling between governance and transport layers. The Init doc comment thoroughly explains nil-safety and behavior.Also applies to: 60-101
179-259: LGTM! Provider routing logic is sound.The model prefix handling (lines 179-191) was discussed in past comments and confirmed as intentional. The weighted random selection (lines 214-234) and sorted fallback construction (lines 238-256) are both correct.
Based on past review comments.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
transports/bifrost-http/handlers/middlewares.go (1)
103-111: Avoid forcing a body when none existed (already raised previously)Always marshalling back writes "{}"/"null" on empty/unparsed bodies. Prior review suggested only rewriting when the body existed or a plugin modified it. Acknowledging previous discussion; leaving as-is.
🧹 Nitpick comments (4)
transports/bifrost-http/handlers/middlewares.go (3)
51-61: Don’t gate interception on governance plugin presenceThis prevents other plugins (e.g., security) from using TransportInterceptor if governance isn’t loaded. Let all loaded plugins run.
Apply:
- // If governance plugin is not loaded, skip interception - hasGovernance := false - for _, p := range plugins { - if p.GetName() == governance.PluginName { - hasGovernance = true - break - } - } - if !hasGovernance { - next(ctx) - return - } + // Run TransportInterceptor for all loaded plugins
63-73: Possible loss of multi-value headersCollecting into map[string]string collapses duplicates (e.g., Accept, Set-Cookie). If plugins may rely on full fidelity, consider map[string][]string and correspondingly reapply with Add/Set. Otherwise, document the limitation.
75-85: Continue interception even if body isn’t valid JSON; improve logging and nil-safety
- Don’t return early on JSON parse errors; allow header-only interception by passing a nil body to plugins.
- Avoid pre-formatting with fmt.Sprintf; pass format + args and guard logger being nil.
- if len(bodyBytes) > 0 { - if err := json.Unmarshal(bodyBytes, &requestBody); err != nil { - // If body is not valid JSON, log warning and continue without interception - logger.Warn(fmt.Sprintf("TransportInterceptor: Failed to unmarshal request body: %v", err)) - next(ctx) - return - } - } + bodyPresent := len(bodyBytes) > 0 + bodyParsed := false + if bodyPresent { + if err := json.Unmarshal(bodyBytes, &requestBody); err != nil { + if logger != nil { + logger.Warn("TransportInterceptor: Failed to unmarshal request body: %v", err) + } + // Keep requestBody as nil to signal plugins that body wasn't parsed + } else { + bodyParsed = true + } + } @@ - if err != nil { - logger.Warn(fmt.Sprintf("TransportInterceptor: Plugin '%s' returned error: %v", plugin.GetName(), err)) + if err != nil { + if logger != nil { + logger.Warn("TransportInterceptor: Plugin '%s' returned error: %v", plugin.GetName(), err) + } // Continue with unmodified headers/body continue }Additionally, consider passing more context to plugins (e.g., full URI via ctx.URI().FullURI(), method) if useful for decisions.
Also applies to: 87-101
plugins/governance/main.go (1)
159-164: Remove redundant type conversions.Since
headersis typed asmap[string]string, bothheaderandvalueare already strings. Thestring()conversions are redundant and can be removed for cleaner code.Apply this diff:
for header, value := range headers { - if strings.ToLower(string(header)) == "x-bf-vk" { - virtualKeyValue = string(value) + if strings.ToLower(header) == "x-bf-vk" { + virtualKeyValue = value break } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
core/bifrost.go(6 hunks)core/schemas/plugin.go(2 hunks)plugins/governance/main.go(5 hunks)plugins/jsonparser/main.go(1 hunks)plugins/logging/main.go(1 hunks)plugins/maxim/main.go(1 hunks)plugins/mocker/main.go(1 hunks)plugins/otel/main.go(1 hunks)plugins/semanticcache/main.go(1 hunks)plugins/telemetry/main.go(1 hunks)transports/bifrost-http/handlers/middlewares.go(1 hunks)transports/bifrost-http/handlers/server.go(9 hunks)transports/bifrost-http/lib/config.go(16 hunks)transports/bifrost-http/lib/lib.go(1 hunks)transports/bifrost-http/main.go(2 hunks)ui/app/observability/views/observabilityView.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- core/schemas/plugin.go
- core/bifrost.go
- plugins/mocker/main.go
- plugins/semanticcache/main.go
- plugins/maxim/main.go
- plugins/otel/main.go
- plugins/telemetry/main.go
- ui/app/observability/views/observabilityView.tsx
🧰 Additional context used
🧬 Code graph analysis (6)
transports/bifrost-http/handlers/middlewares.go (4)
plugins/governance/main.go (2)
Config(31-33)PluginName(19-19)transports/bifrost-http/lib/config.go (1)
Config(117-146)transports/bifrost-http/lib/middleware.go (1)
BifrostHTTPMiddleware(7-7)transports/bifrost-http/handlers/utils.go (1)
SendError(25-34)
transports/bifrost-http/main.go (1)
transports/bifrost-http/lib/lib.go (1)
SetLogger(10-12)
transports/bifrost-http/handlers/server.go (3)
plugins/governance/main.go (2)
Config(31-33)Init(93-148)transports/bifrost-http/lib/config.go (1)
Config(117-146)transports/bifrost-http/handlers/middlewares.go (1)
TransportInterceptorMiddleware(40-126)
plugins/governance/main.go (3)
core/schemas/bifrost.go (1)
ModelProvider(46-46)transports/bifrost-http/lib/config.go (1)
Config(117-146)core/schemas/utils.go (1)
ParseModelString(21-34)
transports/bifrost-http/lib/config.go (1)
core/schemas/plugin.go (2)
Plugin(45-71)PluginConfig(75-79)
transports/bifrost-http/lib/lib.go (1)
core/schemas/logger.go (1)
Logger(28-55)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (31)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (17)
transports/bifrost-http/main.go (1)
128-129: Good: wires transport logger during initCalling lib.SetLogger(logger) early ensures the transport uses the configured logger.
plugins/logging/main.go (1)
214-217: No-op TransportInterceptor keeps interface compliancePassthrough implementation is correct and side‑effect free.
plugins/jsonparser/main.go (1)
90-93: No-op TransportInterceptor is fineImplements the new interface without altering behavior.
transports/bifrost-http/lib/lib.go (1)
7-12: No nil‐logger risk: SetLogger invoked in main.init before any logger calls
All logger usages in bothlibandhandlersoccur inside functions (noinit()or top‐level calls), andlib.SetLogger/handlers.SetLoggerrun inmain.initbefore any of those functions can execute, so no panics will occur.transports/bifrost-http/handlers/server.go (6)
159-168: LGTM! Design aligns with team's performance priorities.The
GovernanceInMemoryStorecorrectly implements theInMemoryStoreinterface required by the governance plugin. TheGetConfiguredProvidersmethod returns the providers map without copying, which is an intentional performance optimization for read-heavy hot paths.Based on learnings: the team prefers to optimize for performance on hot paths by returning shared maps without copying, accepting the trade-off of potential stale data reads over the overhead of copying.
197-200: LGTM! Correct dependency injection.The
inMemoryStoreis properly instantiated and injected into the governance plugin, enabling transport-level provider validation and routing decisions.
290-303: LGTM! Proper atomic state publication.The plugin loading flow correctly separates configuration (
PluginConfigs) from runtime state (Pluginsatomic pointer). The atomicStoreensures that readers usingGetLoadedPlugins()see a consistent snapshot without locks.
350-393: LGTM! Thread-safe plugin reload with atomic CAS.The
ReloadPluginmethod correctly uses a compare-and-swap retry loop to atomically update the plugin list. The copy-on-write approach (creating a new slice) prevents race conditions, and the CAS ensures that only one update succeeds if multiple goroutines attempt concurrent reloads.
395-425: LGTM! Thread-safe plugin removal with atomic CAS.The
RemovePluginmethod follows the same correct CAS pattern asReloadPlugin, ensuring thread-safe atomic updates to the plugin list. The filtering logic correctly removes the target plugin while preserving others.
576-576: LGTM! Correct middleware integration.The replacement of
VKProviderRoutingMiddlewarewithTransportInterceptorMiddlewarecorrectly integrates the new plugin-based interception hook. This enables the governance plugin (and potentially other plugins) to modify requests at the transport layer before they enter the Bifrost core.plugins/governance/main.go (4)
35-37: LGTM! Clean abstraction for provider config access.The
InMemoryStoreinterface provides a simple, focused contract for the governance plugin to query configured providers without tight coupling to the full config structure.
60-144: LGTM! Well-documented Init with clear nil handling.The updated
Initfunction properly documents the role ofinMemoryStoreand handles nil values gracefully. The documentation clearly explains thatinMemoryStoreis optional and primarily used for transport-level routing, making it safe to omit when using the plugin directly from the Go SDK.
192-234: LGTM! Correct weighted provider selection.The weighted random selection algorithm correctly:
- Filters providers based on
AllowedModels(matching the full model string as designed)- Calculates cumulative weights
- Uses
rand.Float64()for random selection- Includes a safety fallback for floating-point edge cases
235-259: LGTM! Correct fallback array construction.The fallback building logic correctly:
- Prefixes the model with the selected provider
- Respects existing fallbacks if already present
- Sorts allowed providers by weight (descending)
- Excludes the selected provider from fallbacks
- Maintains provider/model format consistency
transports/bifrost-http/lib/config.go (3)
187-189: LGTM! Defensive atomic pointer initialization.Initializing the
Pluginsatomic pointer with an empty slice ensures thatGetLoadedPlugins()never returns nil, even beforeLoadPluginsis called. This is good defensive programming that prevents potential nil pointer issues in edge cases or future refactorings.
887-909: LGTM! Correct lock-free plugin accessors.Both
GetLoadedPlugins()andIsPluginLoaded()correctly use atomic operations for lock-free reads. The linear search inIsPluginLoaded()is appropriate given the small plugin counts (typically 5-10), and the comment clearly explains this design choice.
341-731: LGTM! Clean separation of config source and runtime state.The changes correctly separate
PluginConfigs(configuration source from files/database) fromPlugins(runtime atomic state of loaded plugin instances). This separation enables lock-free reads of runtime state while maintaining mutable configuration data.
akshaydeo
force-pushed
the
10-09-enhancement_extent_websocket_handler_for_general_usage_new
branch
from
8283de0 to
cc60439
Compare
akshaydeo
force-pushed
the
10-09-feat_transport_interceptor_method_added_to_plugin_schema_new
branch
from
a570d84 to
4dc4d58
Compare
akshaydeo
force-pushed
the
10-09-enhancement_extent_websocket_handler_for_general_usage_new
branch
from
cc60439 to
2f34af4
Compare
akshaydeo
force-pushed
the
10-09-feat_transport_interceptor_method_added_to_plugin_schema_new
branch
2 times, most recently
from
273f5c2 to
c67b125
Compare
akshaydeo
force-pushed
the
10-09-enhancement_extent_websocket_handler_for_general_usage_new
branch
from
2f34af4 to
f461587
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/handlers/server.go (1)
350-393: Use atomic config reads for all plugin accesses
Replace directs.Pluginsreferences withs.Config.GetLoadedPlugins()to avoid stale or inconsistent data. Occurrences:
- transports/bifrost-http/handlers/server.go:432, 437, 445 (FindPluginByName calls)
- transports/bifrost-http/handlers/server.go:557 (transport config)
♻️ Duplicate comments (1)
ui/app/observability/views/observabilityView.tsx (1)
73-76: Note: Potential hydration mismatch remains (previously discussed).The
resolvedThemefromuseTheme()can beundefinedduring SSR, which may cause hydration warnings when the theme resolves. While the fallback to"light"helps, the icons are still recreated when the theme resolves.This concern was previously raised and you indicated comfort with the current implementation. If you experience hydration warnings in production, consider adding a mounted state check as suggested in the previous review.
🧹 Nitpick comments (2)
plugins/governance/main.go (1)
155-259: Suggest refactoring: Remove redundant type casts and verify random number generation.The TransportInterceptor logic is correct overall, but there are minor improvements:
- Redundant type casts on lines 160-161:
headerandvalueare already strings, no need to cast:- Random number generation on line 220: Verify you're using
rand/v2consistently (you importedmath/rand/v2at line 7)Apply this diff to remove redundant casts:
for header, value := range headers { - if strings.ToLower(string(header)) == "x-bf-vk" { - virtualKeyValue = string(value) + if strings.ToLower(header) == "x-bf-vk" { + virtualKeyValue = value break } }And verify the
rand.Float64()call on line 220 uses the v2 API correctly. If you need cryptographically secure randomness, consider usingcrypto/randinstead ofmath/rand/v2.transports/bifrost-http/handlers/middlewares.go (1)
50-61: Consider whether to run TransportInterceptors for all plugins.The middleware currently only runs TransportInterceptors when the governance plugin is loaded. If other plugins in the future need transport-level interception (e.g., authentication, logging), this check will prevent them from running.
Consider removing the governance-specific check and running TransportInterceptors for all loaded plugins, or make it configurable. This would be more consistent with the generic plugin architecture:
- // If governance plugin is not loaded, skip interception - hasGovernance := false - for _, p := range plugins { - if p.GetName() == governance.PluginName { - hasGovernance = true - break - } - } - if !hasGovernance { - next(ctx) - return - } + // No early return - all plugins can intercept if neededHowever, if the design intent is to run TransportInterceptors only when governance is present (for performance or policy reasons), the current implementation is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
core/bifrost.go(6 hunks)core/schemas/plugin.go(2 hunks)plugins/governance/main.go(5 hunks)plugins/jsonparser/main.go(1 hunks)plugins/logging/main.go(1 hunks)plugins/maxim/main.go(1 hunks)plugins/mocker/main.go(1 hunks)plugins/otel/main.go(1 hunks)plugins/semanticcache/main.go(1 hunks)plugins/telemetry/main.go(1 hunks)transports/bifrost-http/handlers/middlewares.go(1 hunks)transports/bifrost-http/handlers/server.go(9 hunks)transports/bifrost-http/lib/config.go(16 hunks)transports/bifrost-http/lib/lib.go(1 hunks)transports/bifrost-http/main.go(2 hunks)ui/app/observability/views/observabilityView.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- plugins/jsonparser/main.go
- plugins/mocker/main.go
- plugins/telemetry/main.go
- plugins/logging/main.go
- transports/bifrost-http/lib/lib.go
🧰 Additional context used
🧬 Code graph analysis (8)
transports/bifrost-http/main.go (1)
transports/bifrost-http/lib/lib.go (1)
SetLogger(10-12)
plugins/maxim/main.go (1)
core/schemas/plugin.go (1)
Plugin(45-71)
transports/bifrost-http/handlers/server.go (5)
plugins/governance/main.go (2)
Config(31-33)Init(93-148)transports/bifrost-http/lib/config.go (1)
Config(117-146)framework/configstore/clientconfig.go (2)
ProviderConfig(43-50)GovernanceConfig(55-61)core/bifrost.go (1)
Init(79-194)transports/bifrost-http/handlers/middlewares.go (1)
TransportInterceptorMiddleware(40-126)
core/bifrost.go (1)
core/schemas/bifrost.go (2)
RequestCancelled(520-520)BifrostContextKeyFallbackRequestID(115-115)
transports/bifrost-http/lib/config.go (1)
core/schemas/plugin.go (2)
Plugin(45-71)PluginConfig(75-79)
plugins/semanticcache/main.go (1)
core/schemas/plugin.go (1)
Plugin(45-71)
transports/bifrost-http/handlers/middlewares.go (4)
plugins/governance/main.go (2)
Config(31-33)PluginName(19-19)transports/bifrost-http/lib/config.go (1)
Config(117-146)transports/bifrost-http/lib/middleware.go (1)
BifrostHTTPMiddleware(7-7)transports/bifrost-http/handlers/utils.go (1)
SendError(25-34)
plugins/governance/main.go (4)
core/schemas/bifrost.go (1)
ModelProvider(46-46)transports/bifrost-http/lib/config.go (1)
Config(117-146)framework/configstore/store.go (1)
ConfigStore(16-118)core/schemas/utils.go (1)
ParseModelString(21-34)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (29)
core/schemas/plugin.go (2)
21-27: LGTM! Clear execution order documentation.The documentation clearly explains the plugin execution flow and specifies that TransportInterceptor is HTTP transport-specific. This helps plugin authors understand when each hook is invoked.
49-54: LGTM! Well-designed transport interceptor hook.The TransportInterceptor method signature is well-suited for its purpose:
urlparameter enables routing decisionsheadersandbodyparameters allow modification before core processing- Clear documentation that it's only invoked in HTTP transport, not Go SDK usage
plugins/governance/main.go (3)
7-10: LGTM! Appropriate imports for transport interception logic.The new imports support the TransportInterceptor implementation:
math/rand/v2: Modern random number generation for weighted provider selectionslices: Helper functions for model filteringsort: Sorting fallback providers by weightstrings: String operations for header/model parsing
35-37: LGTM! Minimal interface for provider validation.The
InMemoryStoreinterface provides a lightweight way for the transport layer to access configured providers without requiring the fullConfigStoredependency. This supports the governance plugin's transport-level routing decisions.
60-101: LGTM! Comprehensive documentation and nil-safe initialization.The expanded
Initdocumentation clearly explains:
- Behavior and defaults for all parameters
- Nil-safety guarantees (store, pricingManager, inMemoryStore, config can all be nil)
- When to use/skip transport-level validation (inMemoryStore nil is safe for Go SDK usage)
- Side effects and startup behavior
The implementation properly handles nil config by extracting IsVkMandatory safely.
transports/bifrost-http/main.go (1)
65-65: LGTM! Logger properly wired into lib package.The addition of
lib.SetLogger(logger)ensures consistent logging across the HTTP transport layer and the lib package, supporting the new TransportInterceptorMiddleware and other lib components.Also applies to: 128-128
plugins/otel/main.go (1)
113-116: LGTM! Appropriate no-op implementation.The OTEL plugin correctly implements the
TransportInterceptorinterface with a passthrough, as its observability logic belongs inPreHook/PostHookrather than transport-level interception.plugins/maxim/main.go (1)
120-123: LGTM! Appropriate no-op implementation.The Maxim plugin correctly implements the
TransportInterceptorinterface with a passthrough, as its tracing logic belongs inPreHook/PostHookrather than transport-level interception.plugins/semanticcache/main.go (1)
338-341: LGTM! Appropriate no-op implementation.The semantic cache plugin correctly implements the
TransportInterceptorinterface with a passthrough, as its caching logic operates onBifrostRequestobjects inPreHook/PostHookrather than raw HTTP headers/body.core/bifrost.go (2)
1099-1099: LGTM! Enhanced fallback debugging.The debug logs clearly explain each decision point in the fallback flow:
- When primary succeeds
- When request is cancelled
- When AllowFallbacks is false
- When no fallbacks are configured
This improves observability and helps diagnose unexpected fallback behavior.
Also applies to: 1105-1105, 1112-1112, 1118-1118
1208-1209: LGTM! Proper request lifecycle management with enhanced logging.The changes correctly manage
BifrostRequestlifecycle:
- Deferred release at method entry (lines 1208, 1275) ensures cleanup even on early errors
- Debug logs (lines 1224-1234, 1244, 1249) trace primary/fallback attempts
- Comment at line 1889 clarifies lifecycle ownership
The lifecycle is safe because
prepareFallbackRequestcreates shallow copies of the request before the original is released, so fallback processing operates on independent request objects.Also applies to: 1224-1234, 1244-1244, 1249-1249, 1275-1276, 1889-1889
transports/bifrost-http/handlers/server.go (6)
159-168: LGTM!The
GovernanceInMemoryStorecorrectly implements read access to providers underRLockand returns the shared map without copying, consistent with your stated preference to optimize hot-path performance over strict data consistency. Based on learnings.
197-200: LGTM!Governance plugin initialization correctly instantiates
GovernanceInMemoryStoreand passes it togovernance.Init, enabling the plugin to access provider configurations via the in-memory store.
253-305: LGTM!Plugin loading correctly builds the plugin list locally and publishes it atomically at line 303 via
config.Plugins.Store(&plugins), ensuring readers see a consistent snapshot without intermediate states.
342-342: LGTM!Correctly uses
s.Config.GetLoadedPlugins()to retrieve the current plugin snapshot from the atomic pointer.
395-425: LGTM with observation.The CAS retry loop correctly removes the plugin from the atomic state with proper contention handling. Line 420 synchronizes
s.Pluginsafter successful CAS.Same verification as
ReloadPlugin: ensure readers uses.Config.GetLoadedPlugins()to access the atomic state rather than the synchronizeds.Pluginsfield.
576-576: LGTM!Handler chain correctly wired with
TransportInterceptorMiddleware, replacing the previous VK routing middleware and enabling plugin-driven request interception.transports/bifrost-http/lib/config.go (6)
14-14: LGTM!Import of
sync/atomicand the updated comment at line 116 correctly reflect the use ofatomic.Pointerfor lock-free plugin reads.Also applies to: 116-116
118-118: LGTM!Exporting
Mufor governance plugin access and usingatomic.Pointer[[]schemas.Plugin]for lock-free plugin reads are correct architectural choices.Also applies to: 138-139
141-142: LGTM!
PluginConfigsfield correctly separates plugin configuration data from the runtime plugin instances stored in the atomicPluginspointer.
879-888: LGTM!
GetLoadedPluginscorrectly implements lock-free access to the plugin slice via atomic pointer, returning a safe-to-iterate snapshot or nil.
890-905: LGTM!
IsPluginLoadedcorrectly implements lock-free plugin lookup with acceptable O(n) iteration for typical small plugin counts.
856-857: LGTM!Consistent use of
s.Mu.RLock()for read operations ands.Mu.Lock()for write operations across provider configuration methods ensures thread-safe access.Also applies to: 916-917, 1075-1076, 1095-1096, 1198-1199, 1282-1283, 1307-1308
transports/bifrost-http/handlers/middlewares.go (6)
40-48: LGTM!Middleware correctly retrieves loaded plugins via the lock-free
GetLoadedPlugins()accessor and includes an early-return optimization when no plugins are loaded.
63-72: LGTM!Header parsing correctly uses fasthttp's
Header.All()method (the current, non-deprecated API) to iterate and collect headers with pre-allocated capacity.
74-84: LGTM!Body unmarshaling correctly handles both empty and non-empty request bodies, logging a warning and continuing without interception if JSON parsing fails.
86-101: LGTM!Plugin iteration correctly calls
TransportInterceptoron each plugin, aggregating modifications and gracefully handling errors by logging warnings and continuing with unmodified data.
103-109: LGTM!Body marshaling always writes back the request body (even if originally empty), which is acceptable as plugin logic is responsible for handling semantics. This aligns with your stated preference for simpler reciprocation.
111-121: LGTM!Header update logic correctly removes headers deleted by plugins and applies all modified headers back to the request.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
ui/app/observability/views/observabilityView.tsx (1)
73-75: Past review comment addressed: theme resolution with fallback.The implementation now uses
resolvedTheme || "light"as a fallback, which mitigates the hydration mismatch concern. The file already has the"use client"directive at line 1. This approach is acceptable and avoids the complexity of adding a mounted state check.transports/bifrost-http/handlers/middlewares.go (1)
103-121: Don’t invent a{}body when the request had noneLine 109 always marshals an empty map back into the request, so even a GET with no payload exits this middleware carrying
"{}"andContent-Length: 2. That’s a real behaviour change and can break upstream handlers that rely on the absence of a body. Track whether the original request had bytes or a plugin actually produced a body, and only rewrite in those cases.- // Marshal the body back to JSON - updatedBody, err := json.Marshal(requestBody) - if err != nil { - SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("TransportInterceptor: Failed to marshal request body: %v", err), logger) - return - } - ctx.Request.SetBody(updatedBody) + bodyModified := len(bodyBytes) > 0 + if !bodyModified && len(requestBody) > 0 { + bodyModified = true + } + + if bodyModified { + updatedBody, err := json.Marshal(requestBody) + if err != nil { + SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("TransportInterceptor: Failed to marshal request body: %v", err), logger) + return + } + ctx.Request.SetBody(updatedBody) + }plugins/governance/main.go (1)
179-251: Fix double-prefixed model rewrite when provider changesLine 236 currently concatenates the selected provider with the original
modelStr. When the incoming model already contains an unknown prefix (e.g.,"mistral/mistral-large"), we end up returning"openai/mistral/mistral-large", which breaks downstream routing and AllowedModels checks. Parse out the base model when the existing prefix isn’t recognized, then reuse that base for both the rewritten model and fallbacks.- // Check if model already has provider prefix (contains "/") - if strings.Contains(modelStr, "/") { - provider, _ := schemas.ParseModelString(modelStr, "") + baseModel := modelStr + // Check if model already has provider prefix (contains "/") + if strings.Contains(modelStr, "/") { + provider, extractedModel := schemas.ParseModelString(modelStr, "") // Checking valid provider when store is available; if store is nil, // assume the prefixed model should be left unchanged. if p.inMemoryStore != nil { if _, ok := p.inMemoryStore.GetConfiguredProviders()[provider]; ok { return headers, body, nil } } else { return headers, body, nil } + if extractedModel != "" { + baseModel = extractedModel + } } @@ - body["model"] = string(selectedProvider) + "/" + modelStr + body["model"] = string(selectedProvider) + "/" + baseModel @@ - if config.Provider != string(selectedProvider) { - fallbacks = append(fallbacks, string(schemas.ModelProvider(config.Provider))+"/"+modelStr) + if config.Provider != string(selectedProvider) { + fallbacks = append(fallbacks, string(schemas.ModelProvider(config.Provider))+"/"+baseModel) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
core/bifrost.go(6 hunks)core/schemas/plugin.go(2 hunks)plugins/governance/main.go(5 hunks)plugins/jsonparser/main.go(1 hunks)plugins/logging/main.go(1 hunks)plugins/maxim/main.go(1 hunks)plugins/mocker/main.go(1 hunks)plugins/otel/main.go(1 hunks)plugins/semanticcache/main.go(1 hunks)plugins/telemetry/main.go(1 hunks)transports/bifrost-http/handlers/middlewares.go(1 hunks)transports/bifrost-http/handlers/server.go(9 hunks)transports/bifrost-http/lib/config.go(16 hunks)transports/bifrost-http/lib/lib.go(1 hunks)transports/bifrost-http/main.go(2 hunks)ui/app/observability/views/observabilityView.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- plugins/logging/main.go
- plugins/telemetry/main.go
- plugins/mocker/main.go
- plugins/maxim/main.go
- plugins/semanticcache/main.go
- transports/bifrost-http/main.go
🧰 Additional context used
🧬 Code graph analysis (6)
transports/bifrost-http/handlers/server.go (5)
plugins/governance/main.go (2)
Config(31-33)Init(93-148)transports/bifrost-http/lib/config.go (1)
Config(117-146)framework/configstore/clientconfig.go (2)
ProviderConfig(43-50)GovernanceConfig(55-61)framework/configstore/store.go (1)
ConfigStore(16-118)transports/bifrost-http/handlers/middlewares.go (1)
TransportInterceptorMiddleware(40-126)
transports/bifrost-http/handlers/middlewares.go (4)
plugins/governance/main.go (2)
Config(31-33)PluginName(19-19)transports/bifrost-http/lib/config.go (1)
Config(117-146)transports/bifrost-http/lib/middleware.go (1)
BifrostHTTPMiddleware(7-7)transports/bifrost-http/handlers/utils.go (1)
SendError(25-34)
transports/bifrost-http/lib/lib.go (1)
core/schemas/logger.go (1)
Logger(28-55)
core/bifrost.go (1)
core/schemas/bifrost.go (2)
RequestCancelled(520-520)BifrostContextKeyFallbackRequestID(115-115)
transports/bifrost-http/lib/config.go (2)
core/schemas/plugin.go (2)
Plugin(45-71)PluginConfig(75-79)framework/configstore/store.go (1)
ConfigStore(16-118)
plugins/governance/main.go (4)
core/schemas/bifrost.go (1)
ModelProvider(46-46)transports/bifrost-http/lib/config.go (1)
Config(117-146)framework/configstore/store.go (1)
ConfigStore(16-118)core/schemas/utils.go (1)
ParseModelString(21-34)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (12)
plugins/otel/main.go (1)
113-116: LGTM!The no-op
TransportInterceptorimplementation correctly satisfies the updated Plugin interface. This passthrough approach is appropriate for plugins that don't require transport-layer interception.plugins/jsonparser/main.go (1)
90-93: LGTM!The no-op
TransportInterceptorimplementation is consistent with the pattern used across other plugins and correctly satisfies the interface requirement.core/schemas/plugin.go (1)
49-53: LGTM! Well-documented interface extension.The
TransportInterceptormethod addition is clearly documented with:
- Execution context (HTTP transport layer only, before core processing)
- Purpose (modify raw headers and body before BifrostRequest transformation)
- Return signature (modified headers, body, and error)
The updated execution order documentation (lines 21-26) helps developers understand when this hook runs relative to PreHook/PostHook.
core/bifrost.go (2)
1099-1123: LGTM! Enhanced fallback decision logging.The debug logs provide clear context for each fallback decision point:
- No primary error (line 1099)
- Request cancellation (line 1105)
- AllowFallbacks false (line 1112)
- No fallbacks configured (line 1118)
This tracing will significantly improve troubleshooting of fallback behavior without impacting production performance (debug level only).
1208-1209: LGTM! Proper lifecycle management and per-fallback logging.The
defer releaseBifrostRequest(req)pattern (lines 1208, 1275) ensures proper cleanup on all return paths. This is safe because:
prepareFallbackRequestcreates a copy (lines 1128-1184) rather than modifying the original- The defer only releases the original request after all fallbacks are attempted
- Fallback copies are managed separately
The per-fallback processing logs (lines 1224-1250) provide granular visibility into the fallback flow, including:
- Primary provider outcomes
- Fallback attempt details
- Nil fallback guards
Also applies to: 1224-1250
transports/bifrost-http/handlers/server.go (4)
163-168: GetConfiguredProviders optimization noted.The method returns the underlying
Providersmap without copying while holding the read lock. As documented in the learnings, this is an intentional optimization for read-heavy hot paths, accepting potential stale reads over the overhead of copying.Based on learnings
362-392: LGTM! Thread-safe CAS retry loop for plugin reload.The
ReloadPluginimplementation uses atomicCompareAndSwapwith retry to handle concurrent updates safely. The pattern matches the approach incore/bifrost.go:
- Load current plugins
- Create modified copy
- CAS to atomically update
- Retry on contention
The
BifrostHTTPServer.Pluginsfield is kept in sync (line 388) for consistency.
402-424: LGTM! Thread-safe CAS retry loop for plugin removal.The
RemovePluginimplementation follows the same atomic CAS pattern asReloadPlugin, ensuring safe concurrent updates during plugin removal. The retry loop handles the rare case of contention.
290-303: LGTM! Atomic plugin state publication.The plugin loading flow correctly:
- Loads plugin configs from
config.PluginConfigs(line 290)- Instantiates plugins (lines 290-300)
- Atomically publishes the complete plugin state via
config.Plugins.Store(&plugins)(line 303)This ensures readers always see a consistent plugin snapshot via lock-free reads.
transports/bifrost-http/lib/config.go (3)
138-142: LGTM! Atomic plugin state management.The
Pluginsfield asatomic.Pointer[[]schemas.Plugin]enables lock-free reads on hot paths. The initialization sequence is safe:
LoadConfigsets upPluginConfigs(not yet loaded plugins)LoadPlugins(server.go:303) atomically stores the initialized plugins- Nothing accesses
Pluginsbetween these stepsThe separate
PluginConfigsfield (line 142) holds configuration data for plugin instantiation.
879-905: LGTM! Lock-free plugin accessor methods.Both
GetLoadedPluginsandIsPluginLoadedcorrectly:
- Use lock-free
Load()for atomic reads- Handle nil return from
Load()gracefully- Avoid allocations or locks on hot paths
The linear scan in
IsPluginLoaded(typically 5-10 plugins) is faster than maintaining a separate map for small plugin counts.
118-118: Exported mutex enables governance plugin access.Changing
mutoMu(exported) allows the governance plugin to synchronize access to provider configuration viaGovernanceInMemoryStore.GetConfiguredProviders. This is necessary for the transport interceptor to safely read provider configs.
akshaydeo
force-pushed
the
10-09-enhancement_extent_websocket_handler_for_general_usage_new
branch
from
f461587 to
5bf1842
Compare
akshaydeo
force-pushed
the
10-09-feat_transport_interceptor_method_added_to_plugin_schema_new
branch
from
c67b125 to
701d610
Compare
Merge activity
|
akshaydeo
changed the base branch from
10-09-enhancement_extent_websocket_handler_for_general_usage_new
to
graphite-base/593
akshaydeo
deleted the
10-09-feat_transport_interceptor_method_added_to_plugin_schema_new
branch
…logging (#593) ## Summary Add TransportInterceptor to the Plugin interface, enabling plugins to modify HTTP requests before they enter the Bifrost core. This PR also improves fallback handling with additional debug logging and fixes memory management by ensuring BifrostRequest objects are properly released. ## Changes - Added `TransportInterceptor` method to the Plugin interface to allow plugins to modify raw HTTP headers and body before transformation into BifrostRequest - Implemented TransportInterceptor in the Governance plugin to handle provider routing based on virtual keys - Replaced the VKProviderRoutingMiddleware with a more generic TransportInterceptorMiddleware - Added debug logging for fallback handling to improve troubleshooting - Fixed memory management by ensuring BifrostRequest objects are properly released - Updated plugin documentation to clarify execution order - Made plugin management thread-safe using atomic operations - Updated UI to support theme-aware Maxim logo ## Type of change - [x] Feature - [x] Bug fix - [x] Refactor ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Plugins - [x] UI (Next.js) ## How to test ```sh # Core/Transports go version go test ./... # Test with virtual keys curl -X POST http://localhost:8080/v1/chat/completions \ -H "Content-Type: application/json" \ -H "x-bf-vk: your-virtual-key" \ -d '{"model": "gpt-3.5-turbo", "messages": [{"role": "user", "content": "Hello"}]}' # UI cd ui pnpm i pnpm build ``` ## Breaking changes - [x] No ## Related issues Improves plugin architecture and fixes memory management issues. ## Security considerations The TransportInterceptor provides an additional layer for security plugins to inspect and modify requests before they enter the core system. ## Checklist - [x] I added/updated tests where appropriate - [x] I verified builds succeed (Go and UI)
Summary
Add TransportInterceptor to the Plugin interface, enabling plugins to modify HTTP requests before they enter the Bifrost core. This PR also improves fallback handling with additional debug logging and fixes memory management by ensuring BifrostRequest objects are properly released.
Changes
TransportInterceptormethod to the Plugin interface to allow plugins to modify raw HTTP headers and body before transformation into BifrostRequestType of change
Affected areas
How to test
Breaking changes
Related issues
Improves plugin architecture and fixes memory management issues.
Security considerations
The TransportInterceptor provides an additional layer for security plugins to inspect and modify requests before they enter the core system.
Checklist