otel plugin, dynamic plugin reloads, new UI changes and a few bug fixes #470
Conversation
|
Warning Rate limit exceeded@akshaydeo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (25)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughIntroduces context-aware RDB stores (Postgres/SQLite), a new HTTP server with middleware and UI, OTEL plugin, streaming accumulator, plugin hot-reload, and extensive schema refactors (pointer-to-slice changes). Updates pricing sync, governance, logging, and handlers to use context. Adds UI for observability, restructures virtual key governance, and broadens config schema. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Main as main.go
participant Server as BifrostHTTPServer
participant Cfg as Config (lib)
participant Store as ConfigStore (RDB)
participant Router as HTTP Router
participant UI as UI Handler
Main->>Server: NewBifrostHTTPServer(version, uiFS)
Main->>Server: Bootstrap(ctx)
activate Server
Server->>Cfg: LoadConfig(ctx)
Cfg->>Store: NewConfigStore(ctx, config)
Store-->>Cfg: instance
Server->>Server: InitializeTelemetry()
Server->>Router: RegisterRoutes(ctx, middlewares...)
Server->>UI: RegisterUIHandler(middlewares...)
deactivate Server
Main->>Server: Start()
Server-->>Main: serve + graceful shutdown on signals
sequenceDiagram
autonumber
participant Client as HTTP Client
participant MW as VKProviderRoutingMiddleware
participant Store as ConfigStore
participant Bifrost as Core Bifrost
participant Provider as Model Provider
Client->>MW: POST /api/chat (x-bf-vk, model)
MW->>Store: GetVirtualKeyByValue(ctx, vk)
Store-->>MW: VK + ProviderConfigs
MW->>MW: Filter configs by allowed models
MW->>MW: Weighted-select provider/model
MW->>Bifrost: Forward request (rewritten model)
Bifrost->>Provider: Execute request
Provider-->>Bifrost: Response/Stream
Bifrost-->>Client: Response/Stream
sequenceDiagram
autonumber
participant Core as Bifrost
participant Acc as Streaming Accumulator
participant Log as Logging Plugin
participant OTEL as OTEL Plugin
Core->>Acc: ProcessStreamingResponse(ctx, chunk)
Acc-->>Core: Delta/Final ProcessedStreamResponse
opt Delta
Core->>Log: PostHook(ctx, delta)
Core->>OTEL: PostHook(ctx, delta)
end
opt Final
Acc->>Acc: Assemble complete message, usage, cost
Core->>Log: PostHook(ctx, final)
Core->>OTEL: PostHook(ctx, final)
end
sequenceDiagram
autonumber
participant API as /api/plugins
participant PH as PluginsHandler
participant Store as ConfigStore
participant Core as Bifrost
API->>PH: PUT /plugins/{name} (config, enabled)
PH->>Store: UpdatePlugin(ctx, name, config, enabled)
Store-->>PH: OK
PH->>Core: ReloadPlugin(ctx, name, config)
alt Reload OK
Core-->>PH: OK
PH-->>API: 200
else Reload Fail
Core-->>PH: error
PH-->>API: 200 (partial success message)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180+ minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
transports/bifrost-http/handlers/handlers.go (1)
13-16: Guard against nil logger assignment (and ensure it’s always set before use).If
SetLogger(nil)is called (or never called in tests), anylogger.*call in this package will panic.Apply this diff:
func SetLogger(l schemas.Logger) { - logger = l + if l == nil { + return + } + logger = l }Please confirm all entry points (including tests/benchmarks) call
handlers.SetLogger(...)beforeBootstrap(); otherwise we should add a package‑local no‑op logger as a safe default.framework/configstore/tables.go (1)
123-123: DB default for MaxRequestBodySizeMB: confirm migration semantics and backward compat.Tag
gorm:"default:100"sets a DB default for new rows; existing rows with0remain0. Givenfasthttp.Server.MaxRequestBodySizeuses this value, verify legacy configs don’t unintentionally become “unlimited” or too restrictive.
- Ensure AutoMigrate adds the default to the column on supported DBs.
- Decide semantics for
0(unlimited vs. invalid). If invalid, clamp or backfill during load.transports/bifrost-http/handlers/server.go (7)
84-86: Avoid hard‑coded default path; use the constant.Use
DefaultAppDirinstead of"./bifrost-data"for consistency.Apply this diff:
- if appDir != "" && appDir != "./bifrost-data" { + if appDir != "" && appDir != DefaultAppDir { return appDir }
128-150: CORS: addVary: Originto prevent cache poisoning across origins.Responses can be cached and served to a different origin without
Vary.Apply this diff:
return func(ctx *fasthttp.RequestCtx) { origin := string(ctx.Request.Header.Peek("Origin")) // Check if origin is allowed (localhost always allowed + configured origins) if IsOriginAllowed(origin, config.ClientConfig.AllowedOrigins) { ctx.Response.Header.Set("Access-Control-Allow-Origin", origin) } + ctx.Response.Header.Add("Vary", "Origin")
214-221: UI: addX-Content-Type-Options: nosniff.Small hardening to prevent MIME sniffing.
Apply this diff:
ctx.SetContentType(contentType) + ctx.Response.Header.Set("X-Content-Type-Options", "nosniff")
342-345: Redundantplugin.Enabledcheck.Already filtered at Lines 321–323.
Apply this diff:
- case semanticcache.PluginName: - if !plugin.Enabled { - logger.Debug("semantic cache plugin is disabled, skipping initialization") - continue - } + case semanticcache.PluginName:
448-450: Validate/clamp request body size to avoid accidental unlimited or overflow.Protect against
<=0(unlimited?) and absurdly large sizes.Apply this diff:
- s.server = &fasthttp.Server{ - Handler: corsHandler, - MaxRequestBodySize: config.ClientConfig.MaxRequestBodySizeMB * 1024 * 1024, - } + sizeMB := config.ClientConfig.MaxRequestBodySizeMB + if sizeMB <= 0 { + sizeMB = 100 // safe default; matches DB default + } + const maxAllowedMB = 1024 // 1 GiB cap; adjust as needed + if sizeMB > maxAllowedMB { + logger.Warn("max request body size clamped from %dMB to %dMB", sizeMB, maxAllowedMB) + sizeMB = maxAllowedMB + } + s.server = &fasthttp.Server{ + Handler: corsHandler, + MaxRequestBodySize: sizeMB * 1024 * 1024, + }
465-469: Startup log message is misleading on bind failure.Log “starting” before
ListenAndServe; success can be implied afterwards or by lack of error.Apply this diff:
- logger.Info("successfully started bifrost, serving UI on http://%s:%s", s.Host, s.Port) + logger.Info("starting bifrost, serving UI on http://%s:%s", s.Host, s.Port)
471-505: Optional: honor context cancellation in addition to OS signals.Allows callers/tests to stop the server via
ctx.Apply this diff:
// Wait for either termination signal or server error select { - case sig := <-sigChan: + case sig := <-sigChan: logger.Info("received signal %v, initiating graceful shutdown...", sig) + case <-ctx.Done(): + logger.Info("context cancelled, initiating graceful shutdown...") // Create shutdown context with timeout shutdownCtx, cancel := context.WithTimeout(ctx, 30*time.Second)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
framework/configstore/tables.go(2 hunks)transports/bifrost-http/handlers/handlers.go(1 hunks)transports/bifrost-http/handlers/server.go(1 hunks)transports/bifrost-http/main.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
transports/bifrost-http/handlers/handlers.go (1)
core/schemas/logger.go (1)
Logger(28-55)
transports/bifrost-http/handlers/server.go (17)
core/schemas/logger.go (4)
LogLevelInfo(11-11)LoggerOutputTypeJSON(21-21)LogLevel(6-6)Logger(28-55)transports/bifrost-http/handlers/websocket.go (2)
WebSocketHandler(26-34)NewWebSocketHandler(37-46)transports/bifrost-http/handlers/utils.go (2)
IsOriginAllowed(74-82)SendError(27-36)framework/configstore/clientconfig.go (1)
ClientConfig(28-38)transports/bifrost-http/lib/config.go (1)
LoadConfig(164-732)transports/bifrost-http/lib/account.go (1)
NewBaseAccount(20-24)plugins/telemetry/setup.go (1)
InitPrometheusMetrics(57-175)plugins/logging/main.go (1)
LoggerPlugin(117-132)transports/bifrost-http/handlers/logging.go (2)
LoggingHandler(19-22)NewLoggingHandler(25-30)transports/bifrost-http/handlers/governance.go (2)
GovernanceHandler(20-25)NewGovernanceHandler(28-39)transports/bifrost-http/handlers/providers.go (1)
NewProviderHandler(28-34)transports/bifrost-http/handlers/completions.go (1)
NewCompletionHandler(33-39)transports/bifrost-http/handlers/mcp.go (1)
NewMCPHandler(24-30)transports/bifrost-http/handlers/integrations.go (1)
NewIntegrationHandler(23-36)transports/bifrost-http/handlers/config.go (1)
NewConfigHandler(26-32)transports/bifrost-http/handlers/plugins.go (1)
NewPluginsHandler(19-24)transports/bifrost-http/handlers/cache.go (2)
CacheHandler(10-13)NewCacheHandler(15-25)
transports/bifrost-http/main.go (3)
transports/bifrost-http/handlers/server.go (7)
BifrostHTTPServer(46-61)DefaultHost(38-38)NewBifrostHTTPServer(64-75)DefaultPort(39-39)DefaultAppDir(40-40)DefaultLogLevel(41-41)DefaultLogOutputStyle(42-42)core/schemas/logger.go (2)
LogLevel(6-6)LoggerOutputType(17-17)transports/bifrost-http/handlers/handlers.go (1)
SetLogger(14-16)
🔇 Additional comments (4)
transports/bifrost-http/handlers/handlers.go (1)
3-6: Logger DI entrypoint looks good.Clear, minimal surface for wiring a logger into handlers.
framework/configstore/tables.go (1)
788-791: Trigger DDL whitespace tweak — no behavioral change.Looks fine.
transports/bifrost-http/main.go (2)
121-124: Logger flag wiring LGTM.Level and output style are applied before server bootstrap; handlers logger is injected.
111-112: Sane host fallback.Defaults to
handlers.DefaultHostwhenBIFROST_HOSTis empty.
akshaydeo
force-pushed
the
09-15-seggreate_http_server_into_a_separate_struct
branch
from
5e9747a to
c697a9b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
transports/bifrost-http/main.go (1)
115-121: Shadowing bug resolved—now assigning to the package-levelserver.This fixes the earlier nil deref risk at
server.Bootstrap().
🧹 Nitpick comments (5)
transports/bifrost-http/handlers/init.go (1)
13-16: Guard against nil logger injection.A nil logger will panic at first use. Add a defensive check.
func SetLogger(l schemas.Logger) { - logger = l + if l == nil { + panic("handlers.SetLogger: nil logger") + } + logger = l }transports/bifrost-http/main.go (4)
87-88: Prevent panic for longVersionstrings.
strings.Repeatpanics on negative counts ifVersionexceeds the line width. Clamp padding.- versionLine := fmt.Sprintf("║%s%s%s║", strings.Repeat(" ", (61-2-len(Version))/2), Version, strings.Repeat(" ", (61-2-len(Version)+1)/2)) + width := 61 - 2 + pad := width - len(Version) + if pad < 0 { + pad = 0 + } + left := pad / 2 + right := pad - left + versionLine := fmt.Sprintf("║%s%s%s║", strings.Repeat(" ", left), Version, strings.Repeat(" ", right))
123-126: Validate log flags before applying.If an invalid value is passed, the logger may end up with an undefined level/style. Add validation with fallback to defaults.
flag.Parse() - // Configure logger from flags + // Validate and normalize logger flags before configuring + switch schemas.LoggerOutputType(server.LogOutputStyle) { + case schemas.LoggerOutputTypeJSON, schemas.LoggerOutputTypePretty: + default: + fmt.Printf("warning: invalid -log-style=%q; defaulting to %s\n", server.LogOutputStyle, handlers.DefaultLogOutputStyle) + server.LogOutputStyle = handlers.DefaultLogOutputStyle + } + switch schemas.LogLevel(server.LogLevel) { + case schemas.LogLevelDebug, schemas.LogLevelInfo, schemas.LogLevelWarn, schemas.LogLevelError: + default: + fmt.Printf("warning: invalid -log-level=%q; defaulting to %s\n", server.LogLevel, handlers.DefaultLogLevel) + server.LogLevel = handlers.DefaultLogLevel + } + // Configure logger from flags logger.SetOutputType(schemas.LoggerOutputType(server.LogOutputStyle)) logger.SetLevel(schemas.LogLevel(server.LogLevel))
54-65: Graceful shutdown with OS signals.Use a signal-aware context so the server can shut down cleanly on SIGINT/SIGTERM.
import ( "context" "embed" "flag" "fmt" "os" + "os/signal" "strings" + "syscall" bifrost "github.com/maximhq/bifrost/core" schemas "github.com/maximhq/bifrost/core/schemas" "github.com/maximhq/bifrost/transports/bifrost-http/handlers" )- ctx := context.Background() + ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) + defer stop()Also applies to: 132-132
133-141: Error logging path is fine; considerFatalif it exits.If the logger’s
Fatalhandles exit, you can replaceError+os.Exit(1)withFatalfor consistency. Optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
framework/configstore/tables.go(2 hunks)transports/bifrost-http/handlers/handlers.go(0 hunks)transports/bifrost-http/handlers/init.go(1 hunks)transports/bifrost-http/handlers/server.go(1 hunks)transports/bifrost-http/main.go(3 hunks)
💤 Files with no reviewable changes (1)
- transports/bifrost-http/handlers/handlers.go
🚧 Files skipped from review as they are similar to previous changes (2)
- framework/configstore/tables.go
- transports/bifrost-http/handlers/server.go
🧰 Additional context used
🧬 Code graph analysis (2)
transports/bifrost-http/handlers/init.go (1)
core/schemas/logger.go (1)
Logger(28-55)
transports/bifrost-http/main.go (3)
transports/bifrost-http/handlers/server.go (7)
BifrostHTTPServer(46-64)DefaultHost(38-38)NewBifrostHTTPServer(67-78)DefaultPort(39-39)DefaultAppDir(40-40)DefaultLogLevel(41-41)DefaultLogOutputStyle(42-42)core/schemas/logger.go (2)
LogLevel(6-6)LoggerOutputType(17-17)transports/bifrost-http/handlers/init.go (1)
SetLogger(14-16)
🔇 Additional comments (1)
transports/bifrost-http/main.go (1)
112-121: Flag defaults and env override look good.Using
BIFROST_HOSTas the default with explicit flag overrides is clear and predictable.
akshaydeo
force-pushed
the
09-15-seggreate_http_server_into_a_separate_struct
branch
from
c697a9b to
b21428b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
transports/bifrost-http/main.go (1)
115-121: Shadowing fix confirmed — good catch.You now assign to the package-level
serverinstead of creating a new local with:=. This removes the nil-deref risk when callingserver.Bootstrap()/Start().transports/bifrost-http/handlers/server.go (1)
363-371: Don’t Fatal on semantic cache config (graceful degrade).Marshalling/unmarshalling errors for an optional plugin shouldn’t crash the server. Log and continue.
- configBytes, err := json.Marshal(plugin.Config) - if err != nil { - logger.Fatal("failed to marshal semantic cache config: %v", err) - } - if err := json.Unmarshal(configBytes, &semCacheConfig); err != nil { - logger.Fatal("failed to unmarshal semantic cache config: %v", err) - } + configBytes, err := json.Marshal(plugin.Config) + if err != nil { + logger.Error("failed to marshal semantic cache config: %v", err) + continue + } + if err := json.Unmarshal(configBytes, &semCacheConfig); err != nil { + logger.Error("failed to unmarshal semantic cache config: %v", err) + continue + }
🧹 Nitpick comments (3)
transports/bifrost-http/handlers/server.go (3)
131-152: CORS: add Vary and echo requested headers for robust preflight.Add Vary headers to keep caches correct and reflect
Access-Control-Request-Headersto avoid preflight failures with custom headers.return func(ctx *fasthttp.RequestCtx) { origin := string(ctx.Request.Header.Peek("Origin")) + // Help caches and proxies + ctx.Response.Header.Add("Vary", "Origin") + ctx.Response.Header.Add("Vary", "Access-Control-Request-Method") + ctx.Response.Header.Add("Vary", "Access-Control-Request-Headers") @@ - ctx.Response.Header.Set("Access-Control-Allow-Headers", "Content-Type, Authorization, X-Requested-With") + // Echo requested headers when provided; fall back to a safe default + if reqH := string(ctx.Request.Header.Peek("Access-Control-Request-Headers")); reqH != "" { + ctx.Response.Header.Set("Access-Control-Allow-Headers", reqH) + } else { + ctx.Response.Header.Set("Access-Control-Allow-Headers", "Content-Type, Authorization, X-Requested-With, Accept") + }
465-468: Set server timeouts to harden against slowloris and stuck clients.Fasthttp defaults leave timeouts unset. Add sane timeouts.
s.server = &fasthttp.Server{ Handler: corsHandler, MaxRequestBodySize: s.config.ClientConfig.MaxRequestBodySizeMB * 1024 * 1024, + ReadTimeout: 30 * time.Second, + WriteTimeout: 2 * time.Minute, + IdleTimeout: 2 * time.Minute, + // Optional hardening: + // MaxRequestsPerConn: 0, // unlimited; set if needed + // ReduceMemoryUsage: true, }
473-476: Improve startup log for 0.0.0.0/IPv6 listeners.Log both the bind address and a friendly URL (localhost) when binding to all interfaces.
- logger.Info("successfully started bifrost, serving UI on http://%s:%s", s.Host, s.Port) + uiHost := s.Host + if s.Host == "0.0.0.0" || s.Host == "::" { + uiHost = "localhost" + } + logger.Info("successfully started bifrost (listening on %s). UI: http://%s:%s", serverAddr, uiHost, s.Port)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
framework/configstore/tables.go(2 hunks)transports/bifrost-http/handlers/handlers.go(0 hunks)transports/bifrost-http/handlers/init.go(1 hunks)transports/bifrost-http/handlers/server.go(1 hunks)transports/bifrost-http/main.go(3 hunks)
💤 Files with no reviewable changes (1)
- transports/bifrost-http/handlers/handlers.go
🚧 Files skipped from review as they are similar to previous changes (2)
- transports/bifrost-http/handlers/init.go
- framework/configstore/tables.go
🧰 Additional context used
🧬 Code graph analysis (2)
transports/bifrost-http/main.go (3)
transports/bifrost-http/handlers/server.go (7)
BifrostHTTPServer(46-64)DefaultHost(38-38)NewBifrostHTTPServer(67-77)DefaultPort(39-39)DefaultAppDir(40-40)DefaultLogLevel(41-41)DefaultLogOutputStyle(42-42)core/schemas/logger.go (2)
LogLevel(6-6)LoggerOutputType(17-17)transports/bifrost-http/handlers/init.go (1)
SetLogger(9-11)
transports/bifrost-http/handlers/server.go (15)
core/schemas/logger.go (4)
LogLevelInfo(11-11)LoggerOutputTypeJSON(21-21)LogLevel(6-6)Logger(28-55)transports/bifrost-http/handlers/websocket.go (2)
WebSocketHandler(26-34)NewWebSocketHandler(37-46)transports/bifrost-http/handlers/utils.go (1)
IsOriginAllowed(74-82)framework/configstore/clientconfig.go (2)
ClientConfig(28-38)GovernanceConfig(54-60)transports/bifrost-http/lib/config.go (1)
LoadConfig(164-732)plugins/telemetry/setup.go (1)
InitPrometheusMetrics(57-175)transports/bifrost-http/handlers/logging.go (2)
LoggingHandler(19-22)NewLoggingHandler(25-30)transports/bifrost-http/handlers/governance.go (2)
GovernanceHandler(20-25)NewGovernanceHandler(28-39)transports/bifrost-http/handlers/providers.go (1)
NewProviderHandler(28-34)transports/bifrost-http/handlers/completions.go (1)
NewCompletionHandler(33-39)transports/bifrost-http/handlers/mcp.go (1)
NewMCPHandler(24-30)transports/bifrost-http/handlers/integrations.go (1)
NewIntegrationHandler(23-36)transports/bifrost-http/handlers/config.go (1)
NewConfigHandler(26-32)transports/bifrost-http/handlers/plugins.go (1)
NewPluginsHandler(19-24)transports/bifrost-http/handlers/cache.go (2)
CacheHandler(10-13)NewCacheHandler(15-25)
🔇 Additional comments (3)
transports/bifrost-http/main.go (2)
124-127: Logger DI into handlers is clean.Flag-driven level/output applied before
handlers.SetLogger(logger)keeps server internals decoupled from main.
133-141: Error paths look correct.Bootstrap/start errors are logged and exit with non‑zero status.
transports/bifrost-http/handlers/server.go (1)
255-256: No action required —versionis a package-level var.Declaration found at transports/bifrost-http/handlers/init.go:5 (
var version string). Theversion = s.Versionassignment in server.go writes to that package-level variable; no shadowing in handlers.
akshaydeo
force-pushed
the
09-15-seggreate_http_server_into_a_separate_struct
branch
2 times, most recently
from
692bee5 to
73cf382
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
transports/bifrost-http/handlers/server.go (3)
88-91: Honor provided appDir and use the DefaultAppDir constant (don’t special‑case "./bifrost-data").Treating "./bifrost-data" as “not provided” diverges from the flag’s displayed default and can break setups. Only fall back when appDir == "" and avoid hardcoded strings.
Apply:
- if appDir != "" && appDir != "./bifrost-data" { + if appDir != "" { return appDir } @@ - if configDir == "" { - configDir = "./bifrost-data" - } + if configDir == "" { + configDir = DefaultAppDir + }Also applies to: 114-116
321-329: Don’t Fatal on semantic cache config errors; log and skip the plugin.A misconfigured optional plugin shouldn’t crash the server.
Apply:
- configBytes, err := json.Marshal(plugin.Config) - if err != nil { - logger.Fatal("failed to marshal semantic cache config: %v", err) - } - if err := json.Unmarshal(configBytes, &semCacheConfig); err != nil { - logger.Fatal("failed to unmarshal semantic cache config: %v", err) - } + configBytes, err := json.Marshal(plugin.Config) + if err != nil { + logger.Error("failed to marshal semantic cache config: %v", err) + continue + } + if err := json.Unmarshal(configBytes, &semCacheConfig); err != nil { + logger.Error("failed to unmarshal semantic cache config: %v", err) + continue + }
382-385: Don’t Fatal on governance handler init; log and skip routes.Governance is optional; avoid taking down the server on handler construction failure.
Apply:
- if err != nil { - logger.Fatal("failed to initialize governance handler: %v", err) - } + if err != nil { + logger.Error("failed to initialize governance handler: %v", err) + governanceHandler = nil + }
🧹 Nitpick comments (7)
transports/bifrost-http/handlers/server.go (7)
134-151: CORS: set Allow‑Credentials only when origin is allowed; add Vary: Origin; use method constant.Prevents misleading headers and improves cache correctness.
Apply:
return func(ctx *fasthttp.RequestCtx) { origin := string(ctx.Request.Header.Peek("Origin")) - // Check if origin is allowed (localhost always allowed + configured origins) - if IsOriginAllowed(origin, config.ClientConfig.AllowedOrigins) { - ctx.Response.Header.Set("Access-Control-Allow-Origin", origin) - } + allowed := IsOriginAllowed(origin, config.ClientConfig.AllowedOrigins) + if allowed && origin != "" { + ctx.Response.Header.Set("Access-Control-Allow-Origin", origin) + ctx.Response.Header.Set("Access-Control-Allow-Credentials", "true") + ctx.Response.Header.Add("Vary", "Origin") + } else { + ctx.Response.Header.Del("Access-Control-Allow-Credentials") + } ctx.Response.Header.Set("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, OPTIONS") ctx.Response.Header.Set("Access-Control-Allow-Headers", "Content-Type, Authorization, X-Requested-With") - ctx.Response.Header.Set("Access-Control-Allow-Credentials", "true") ctx.Response.Header.Set("Access-Control-Max-Age", "86400") // Handle preflight OPTIONS requests - if string(ctx.Method()) == "OPTIONS" { + if string(ctx.Method()) == fasthttp.MethodOptions { ctx.SetStatusCode(fasthttp.StatusOK) return }
243-244: Guard against nil router in RegisterRoute.If RegisterRoute is called before initializeRoutes, this will panic.
Apply:
func (s *BifrostHTTPServer) RegisterRoute(method string, path string, handler fasthttp.RequestHandler) { - s.router.Handle(method, path, handler) + if s.router == nil { + s.router = router.New() + } + s.router.Handle(method, path, handler) }
510-513: Add sensible server timeouts and clamp MaxRequestBodySize.Prevents slowloris and protects memory. Keep values configurable later.
Apply:
- s.server = &fasthttp.Server{ - Handler: corsHandler, - MaxRequestBodySize: s.config.ClientConfig.MaxRequestBodySizeMB * 1024 * 1024, - } + mb := s.config.ClientConfig.MaxRequestBodySizeMB + if mb <= 0 { + mb = 100 // fallback to 100MB + } + s.server = &fasthttp.Server{ + Handler: corsHandler, + MaxRequestBodySize: mb * 1024 * 1024, + ReadTimeout: 30 * time.Second, + WriteTimeout: 30 * time.Second, + IdleTimeout: 120 * time.Second, + }
518-521: Log “starting” before ListenAndServe; report success after bind.Avoids claiming success if bind fails.
Apply:
- logger.Info("successfully started bifrost, serving UI on http://%s:%s", s.Host, s.Port) - if err := s.server.ListenAndServe(serverAddr); err != nil { + logger.Info("starting bifrost on http://%s:%s ...", s.Host, s.Port) + if err := s.server.ListenAndServe(serverAddr); err != nil { errChan <- err }
503-507: Stop signal notifications on exit to avoid leaks.Small hygiene improvement.
Apply:
signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM) + defer signal.Stop(sigChan)
190-194: Avoid echoing request paths in 404 bodies (reduce info leakage).Return a generic message; details are already in logs.
Apply:
- ctx.SetStatusCode(fasthttp.StatusNotFound) - ctx.SetBodyString("404 - Static asset not found: " + requestPath) + ctx.SetStatusCode(fasthttp.StatusNotFound) + ctx.SetBodyString("404 - Static asset not found")- ctx.SetStatusCode(fasthttp.StatusNotFound) - ctx.SetBodyString("404 - File not found") + ctx.SetStatusCode(fasthttp.StatusNotFound) + ctx.SetBodyString("404 - File not found")Also applies to: 213-216
125-129: Use errors.As to detect AlreadyRegisteredError.Type assertion may miss non-identical concrete types; errors.As is more robust.
Apply:
- if err := prometheus.Register(collector); err != nil { - if _, ok := err.(prometheus.AlreadyRegisteredError); !ok { - logger.Error("failed to register prometheus collector: %v", err) - } - } + if err := prometheus.Register(collector); err != nil { + var are prometheus.AlreadyRegisteredError + if !errors.As(err, &are) { + logger.Error("failed to register prometheus collector: %v", err) + } + }Note: add
errorsimport.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
framework/configstore/tables.go(2 hunks)transports/bifrost-http/handlers/handlers.go(0 hunks)transports/bifrost-http/handlers/init.go(1 hunks)transports/bifrost-http/handlers/server.go(1 hunks)transports/bifrost-http/main.go(3 hunks)
💤 Files with no reviewable changes (1)
- transports/bifrost-http/handlers/handlers.go
🚧 Files skipped from review as they are similar to previous changes (3)
- transports/bifrost-http/handlers/init.go
- framework/configstore/tables.go
- transports/bifrost-http/main.go
🧰 Additional context used
🧬 Code graph analysis (1)
transports/bifrost-http/handlers/server.go (8)
core/schemas/logger.go (2)
LogLevel(6-6)Logger(28-55)transports/bifrost-http/handlers/websocket.go (2)
WebSocketHandler(26-34)NewWebSocketHandler(37-46)core/bifrost.go (1)
Bifrost(33-48)transports/bifrost-http/handlers/utils.go (1)
IsOriginAllowed(74-82)framework/configstore/clientconfig.go (2)
ClientConfig(28-38)GovernanceConfig(54-60)transports/bifrost-http/lib/config.go (1)
LoadConfig(164-732)transports/bifrost-http/lib/account.go (1)
NewBaseAccount(20-24)plugins/telemetry/setup.go (1)
InitPrometheusMetrics(57-175)
🔇 Additional comments (1)
transports/bifrost-http/handlers/server.go (1)
456-465: OK — package 'version' present and logger set before usevar version and SetLogger are defined in transports/bifrost-http/handlers/init.go; main.init calls handlers.SetLogger(logger) in transports/bifrost-http/main.go before server.Bootstrap and there are no handlers package init() functions that log.
akshaydeo
force-pushed
the
09-15-seggreate_http_server_into_a_separate_struct
branch
from
73cf382 to
d2f95cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
transports/bifrost-http/handlers/server.go (3)
85-87: Honor any provided app-dir and use constants (no hardcoded "./bifrost-data").- if appDir != "" && appDir != "./bifrost-data" { + if appDir != "" { return appDir } @@ - if configDir == "" { - configDir = "./bifrost-data" - } + if configDir == "" { + configDir = DefaultAppDir + }Also applies to: 111-113
174-181: Don’t Fatal on logging plugin init; degrade gracefully and guard usage.- loggingPlugin, err = logging.Init(logger, config.LogsStore, pricingManager) - if err != nil { - logger.Fatal("failed to initialize logging plugin: %v", err) - } - plugins = append(plugins, loggingPlugin) + loggingPlugin, err = logging.Init(logger, config.LogsStore, pricingManager) + if err != nil { + logger.Error("failed to initialize logging plugin: %v", err) + } else { + plugins = append(plugins, loggingPlugin) + }
235-241: Don’t Fatal on semantic cache config marshal/unmarshal; skip plugin.- configBytes, err := json.Marshal(plugin.Config) - if err != nil { - logger.Fatal("failed to marshal semantic cache config: %v", err) - } - if err := json.Unmarshal(configBytes, &semCacheConfig); err != nil { - logger.Fatal("failed to unmarshal semantic cache config: %v", err) - } + configBytes, err := json.Marshal(plugin.Config) + if err != nil { + logger.Error("failed to marshal semantic cache config: %v", err) + continue + } + if err := json.Unmarshal(configBytes, &semCacheConfig); err != nil { + logger.Error("failed to unmarshal semantic cache config: %v", err) + continue + }
🧹 Nitpick comments (7)
transports/bifrost-http/handlers/ui.go (4)
26-30: Support HEAD for UI routes (no response body).Browsers and CDNs issue HEADs; serve headers without a body.
func (h *UIHandler) RegisterRoutes(router *router.Router) { router.GET("/", h.serveDashboard) router.GET("/{filepath:*}", h.serveDashboard) + router.HEAD("/", h.serveDashboard) + router.HEAD("/{filepath:*}", h.serveDashboard) }And near the send step:
- // Send the file content - ctx.SetBody(data) + // Send the file content (suppress body for HEAD) + if string(ctx.Method()) == "HEAD" { + ctx.Response.Header.SetContentLength(len(data)) + return + } + ctx.SetBody(data)
58-59: Use filepath.Ext to detect extensions (handles edge cases).- // Check if this is a static asset request (has file extension) - hasExtension := strings.Contains(filepath.Base(cleanPath), ".") + // Check if this is a static asset request (has file extension) + ext := filepath.Ext(cleanPath) + hasExtension := ext != ""Then remove the later re‑declaration of ext:
- // Set content type based on file extension - ext := filepath.Ext(cleanPath) + // Set content type based on file extension
64-71: Set Content-Type on 404s to avoid defaulting to text/html.if hasExtension { ctx.SetStatusCode(fasthttp.StatusNotFound) - ctx.SetBodyString("404 - Static asset not found: " + requestPath) + ctx.SetContentType("text/plain; charset=utf-8") + ctx.SetBodyString("404 - Static asset not found") return }
88-91: Consistent 404 body and type for non-asset misses.} else { ctx.SetStatusCode(fasthttp.StatusNotFound) - ctx.SetBodyString("404 - File not found") + ctx.SetContentType("text/plain; charset=utf-8") + ctx.SetBodyString("404 - File not found") return }transports/bifrost-http/handlers/server.go (3)
135-148: CORS: add Vary: Origin and include Accept in allowed headers.if IsOriginAllowed(origin, config.ClientConfig.AllowedOrigins) { ctx.Response.Header.Set("Access-Control-Allow-Origin", origin) + ctx.Response.Header.Add("Vary", "Origin") } - ctx.Response.Header.Set("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, OPTIONS") - ctx.Response.Header.Set("Access-Control-Allow-Headers", "Content-Type, Authorization, X-Requested-With") + ctx.Response.Header.Set("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, OPTIONS") + ctx.Response.Header.Set("Access-Control-Allow-Headers", "Accept, Content-Type, Authorization, X-Requested-With") ctx.Response.Header.Set("Access-Control-Allow-Credentials", "true") ctx.Response.Header.Set("Access-Control-Max-Age", "86400")
317-331: Register UI catch-all last to avoid accidental shadowing of future routes.- uiHandler.RegisterRoutes(s.router) ... - // Add Prometheus /metrics endpoint - s.router.GET("/metrics", fasthttpadaptor.NewFastHTTPHandler(promhttp.Handler())) + // Add Prometheus /metrics endpoint early + s.router.GET("/metrics", fasthttpadaptor.NewFastHTTPHandler(promhttp.Handler())) + // Register UI routes last (catch-all) + uiHandler.RegisterRoutes(s.router)Although fasthttp/router prefers static routes over wildcards, placing the catch‑all last reduces risk as routes evolve.
121-127: Robust AlreadyRegistered check (pointer-safe).- if err := prometheus.Register(collector); err != nil { - if _, ok := err.(prometheus.AlreadyRegisteredError); !ok { - logger.Error("failed to register prometheus collector: %v", err) - } - } + if err := prometheus.Register(collector); err != nil { + var are prometheus.AlreadyRegisteredError + if !errors.As(err, &are) { + logger.Error("failed to register prometheus collector: %v", err) + } + }(Remember to import errors.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
framework/configstore/tables.go(2 hunks)transports/bifrost-http/handlers/handlers.go(0 hunks)transports/bifrost-http/handlers/init.go(1 hunks)transports/bifrost-http/handlers/server.go(1 hunks)transports/bifrost-http/handlers/ui.go(1 hunks)transports/bifrost-http/main.go(3 hunks)
💤 Files with no reviewable changes (1)
- transports/bifrost-http/handlers/handlers.go
🚧 Files skipped from review as they are similar to previous changes (2)
- transports/bifrost-http/handlers/init.go
- framework/configstore/tables.go
🧰 Additional context used
🧬 Code graph analysis (3)
transports/bifrost-http/handlers/server.go (14)
core/schemas/logger.go (4)
LogLevelInfo(11-11)LoggerOutputTypeJSON(21-21)LogLevel(6-6)Logger(28-55)transports/bifrost-http/handlers/websocket.go (2)
WebSocketHandler(26-34)NewWebSocketHandler(37-46)transports/bifrost-http/handlers/utils.go (2)
IsOriginAllowed(74-82)SendError(27-36)transports/bifrost-http/handlers/logging.go (2)
LoggingHandler(19-22)NewLoggingHandler(25-30)transports/bifrost-http/handlers/governance.go (2)
GovernanceHandler(20-25)NewGovernanceHandler(28-39)transports/bifrost-http/handlers/providers.go (1)
NewProviderHandler(29-35)transports/bifrost-http/handlers/completions.go (1)
NewCompletionHandler(33-39)transports/bifrost-http/handlers/mcp.go (1)
NewMCPHandler(24-30)transports/bifrost-http/handlers/integrations.go (1)
NewIntegrationHandler(23-36)transports/bifrost-http/handlers/config.go (1)
NewConfigHandler(26-32)transports/bifrost-http/handlers/plugins.go (1)
NewPluginsHandler(19-24)transports/bifrost-http/handlers/ui.go (1)
NewUIHandler(20-24)transports/bifrost-http/handlers/cache.go (2)
CacheHandler(10-13)NewCacheHandler(15-25)transports/bifrost-http/lib/config.go (1)
LoadConfig(164-732)
transports/bifrost-http/main.go (3)
transports/bifrost-http/handlers/server.go (7)
BifrostHTTPServer(45-64)DefaultHost(37-37)NewBifrostHTTPServer(67-77)DefaultPort(38-38)DefaultAppDir(39-39)DefaultLogLevel(40-40)DefaultLogOutputStyle(41-41)core/schemas/logger.go (2)
LogLevel(6-6)LoggerOutputType(17-17)transports/bifrost-http/handlers/init.go (1)
SetLogger(9-11)
transports/bifrost-http/handlers/ui.go (1)
transports/bifrost-http/handlers/server.go (1)
RegisterRoutes(273-337)
🔇 Additional comments (3)
transports/bifrost-http/main.go (2)
73-73: Good fix: avoid shadowing the package-level server.
130-142: Bootstrap/Start error handling looks solid.Logs and exits cleanly on failures.
transports/bifrost-http/handlers/server.go (1)
359-359: Confirmed: package-levelversionvariable exists — no action required.
Foundvar version stringin transports/bifrost-http/handlers/init.go (line 5).
akshaydeo
force-pushed
the
09-15-seggreate_http_server_into_a_separate_struct
branch
from
d2f95cc to
edaffe9
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
akshaydeo
force-pushed
the
09-15-seggreate_http_server_into_a_separate_struct
branch
from
edaffe9 to
7f08f2d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
transports/bifrost-http/handlers/server.go (4)
83-116: Honor provided app-dir (including "./bifrost-data"); don’t special‑case the default.Treating the default as “not provided” silently relocates config to OS dirs and diverges from flag help. Use provided value when non‑empty; only fall back when empty. Also, use
DefaultAppDirconstant for the final fallback.func GetDefaultConfigDir(appDir string) string { // If appDir is provided, use it directly - if appDir != "" && appDir != "./bifrost-data" { + if appDir != "" { return appDir } @@ // If we couldn't determine the config directory, fall back to current directory if configDir == "" { - configDir = "./bifrost-data" + configDir = DefaultAppDir }
167-174: Don’t Fatal on logging plugin init; degrade gracefully.A misconfigured optional plugin shouldn’t crash the server. Log and skip instead; guard subsequent usage (already nil‑checked via
loggerPlugin != nil).if config.ClientConfig.EnableLogging && config.LogsStore != nil { // Use dedicated logs database with high-scale optimizations loggingPlugin, err = logging.Init(logger, config.LogsStore, pricingManager) if err != nil { - logger.Fatal("failed to initialize logging plugin: %v", err) - } - plugins = append(plugins, loggingPlugin) + logger.Error("failed to initialize logging plugin: %v", err) + } else { + plugins = append(plugins, loggingPlugin) + } }
225-235: Don’t Fatal on semantic cache config marshal/unmarshal; skip plugin and continue.Crashing here takes down the whole server for an optional plugin.
if plugin.Config != nil { configBytes, err := json.Marshal(plugin.Config) if err != nil { - logger.Fatal("failed to marshal semantic cache config: %v", err) + logger.Error("failed to marshal semantic cache config: %v", err) + continue } if err := json.Unmarshal(configBytes, &semCacheConfig); err != nil { - logger.Fatal("failed to unmarshal semantic cache config: %v", err) + logger.Error("failed to unmarshal semantic cache config: %v", err) + continue } }
4-31: Restore signal handling; current code never subscribes to SIGINT/SIGTERM.
signal.Notifyis commented out and imports are missing, so graceful shutdown won’t trigger.import ( "context" "embed" "encoding/json" "fmt" "net" "os" + "os/signal" "path/filepath" "runtime" "strings" + "syscall" "time" @@ - // Initializing server signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM) + // Watch for termination signals + signal.Notify(sigChan, os.Interrupt, syscall.SIGTERM)Also applies to: 416-420
🧹 Nitpick comments (5)
transports/bifrost-http/handlers/ui.go (2)
57-59: Fix extension detection to support dotfiles like "/.well-known".
strings.Contains(filepath.Base(cleanPath), ".")misclassifies directories starting with a dot as “files with extension”, preventing SPA fallback. Use filepath.Ext instead.- // Check if this is a static asset request (has file extension) - hasExtension := strings.Contains(filepath.Base(cleanPath), ".") + // Check if this is a static asset request (has file extension) + hasExtension := filepath.Ext(cleanPath) != ""
94-101: Add MIME fallbacks for JS/CSS/SVG/JSON to avoid octet-stream.
mime.TypeByExtensioncan return empty for common types in some environments; JS served as octet-stream breaks execution.ext := filepath.Ext(cleanPath) contentType := mime.TypeByExtension(ext) if contentType == "" { - contentType = "application/octet-stream" + switch ext { + case ".js": + contentType = "application/javascript" + case ".css": + contentType = "text/css; charset=utf-8" + case ".svg": + contentType = "image/svg+xml" + case ".json": + contentType = "application/json" + case ".txt", ".map": + contentType = "text/plain; charset=utf-8" + default: + contentType = "application/octet-stream" + } } ctx.SetContentType(contentType)transports/bifrost-http/handlers/server.go (3)
398-403: Check RegisterRoutes error before registering UI.If routing init fails, we shouldn’t proceed to register UI, which can leave partial state.
- err = s.RegisterRoutes(s.ctx) - // Register UI handler - s.RegisterUIHandler() - if err != nil { - return fmt.Errorf("failed to initialize routes: %v", err) - } + err = s.RegisterRoutes(s.ctx) + if err != nil { + return fmt.Errorf("failed to initialize routes: %v", err) + } + // Register UI handler last + s.RegisterUIHandler()
128-151: Add Vary: Origin to CORS responses.When echoing Origin, add Vary to prevent cache misdelivery across different origins.
func CorsMiddleware(config *lib.Config, next fasthttp.RequestHandler) fasthttp.RequestHandler { return func(ctx *fasthttp.RequestCtx) { origin := string(ctx.Request.Header.Peek("Origin")) + // Ensure caches vary by Origin + ctx.Response.Header.Add("Vary", "Origin") + // Check if origin is allowed (localhost always allowed + configured origins) if IsOriginAllowed(origin, config.ClientConfig.AllowedOrigins) { ctx.Response.Header.Set("Access-Control-Allow-Origin", origin) }
289-297: Comment mismatch about WebSocket vs UI ordering.Code registers WS routes before UI (UI is registered in Bootstrap after routes). Update the comment to reflect actual ordering to avoid future regressions.
- // Websocket handler needs to go below UI handler + // Websocket handler is registered before UI; UI is registered last in Bootstrap
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
framework/configstore/tables.go(2 hunks)transports/bifrost-http/handlers/handlers.go(0 hunks)transports/bifrost-http/handlers/init.go(1 hunks)transports/bifrost-http/handlers/server.go(1 hunks)transports/bifrost-http/handlers/ui.go(1 hunks)transports/bifrost-http/handlers/websocket.go(4 hunks)transports/bifrost-http/main.go(3 hunks)
💤 Files with no reviewable changes (1)
- transports/bifrost-http/handlers/handlers.go
🚧 Files skipped from review as they are similar to previous changes (3)
- framework/configstore/tables.go
- transports/bifrost-http/handlers/init.go
- transports/bifrost-http/handlers/websocket.go
🧰 Additional context used
🧬 Code graph analysis (2)
transports/bifrost-http/handlers/server.go (18)
core/schemas/logger.go (4)
LogLevelInfo(11-11)LoggerOutputTypeJSON(21-21)LogLevel(6-6)Logger(28-55)transports/bifrost-http/handlers/utils.go (1)
IsOriginAllowed(74-82)framework/configstore/clientconfig.go (2)
ClientConfig(28-38)GovernanceConfig(54-60)transports/bifrost-http/handlers/logging.go (2)
LoggingHandler(19-22)NewLoggingHandler(25-30)transports/bifrost-http/handlers/governance.go (2)
GovernanceHandler(20-25)NewGovernanceHandler(28-39)transports/bifrost-http/handlers/cache.go (2)
CacheHandler(10-13)NewCacheHandler(15-25)transports/bifrost-http/handlers/websocket.go (2)
WebSocketHandler(27-36)NewWebSocketHandler(39-49)transports/bifrost-http/handlers/providers.go (1)
NewProviderHandler(29-35)transports/bifrost-http/handlers/completions.go (1)
NewCompletionHandler(33-39)transports/bifrost-http/handlers/mcp.go (1)
NewMCPHandler(24-30)transports/bifrost-http/handlers/integrations.go (1)
NewIntegrationHandler(23-36)transports/bifrost-http/handlers/config.go (1)
NewConfigHandler(26-32)transports/bifrost-http/handlers/plugins.go (1)
NewPluginsHandler(19-24)plugins/telemetry/setup.go (1)
InitPrometheusMetrics(57-175)transports/bifrost-http/handlers/ui.go (1)
NewUIHandler(20-24)transports/bifrost-http/handlers/init.go (1)
SetVersion(14-16)transports/bifrost-http/lib/config.go (1)
LoadConfig(164-732)transports/bifrost-http/lib/account.go (1)
NewBaseAccount(20-24)
transports/bifrost-http/main.go (3)
transports/bifrost-http/handlers/server.go (7)
BifrostHTTPServer(43-63)DefaultHost(35-35)NewBifrostHTTPServer(66-76)DefaultPort(36-36)DefaultAppDir(37-37)DefaultLogLevel(38-38)DefaultLogOutputStyle(39-39)core/schemas/logger.go (2)
LogLevel(6-6)LoggerOutputType(17-17)transports/bifrost-http/handlers/init.go (1)
SetLogger(9-11)
🔇 Additional comments (2)
transports/bifrost-http/handlers/ui.go (1)
26-30: Confirm UI routes are registered last.Catch‑all GET “/{filepath:*}” must be registered after API routes to avoid shadowing. Server.RegisterUIHandler appears to do this; please keep that invariant.
transports/bifrost-http/main.go (1)
113-121: LGTM: server instance bound correctly; shadowing bug resolved.Global
serveris initialized once and flags bind to the same instance. Bootstrap/Start flow reads clean.
akshaydeo
force-pushed
the
09-15-seggreate_http_server_into_a_separate_struct
branch
from
7f08f2d to
76c79e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
transports/bifrost-http/handlers/init.go (1)
13-16: Duplicate version source — remove globalversion/SetVersionor prove it’s still needed.You now pass
Versionon the server struct and also mirror it into a package-global. Prefer a single source of truth to avoid divergence. If unused, delete the global and the setter; otherwise document why both are necessary.Apply:
-var version string @@ -// SetVersion sets the version for the application. -func SetVersion(v string) { - version = v -}And remove the
SetVersioncall in server bootstrap (see server.go fix below).transports/bifrost-http/handlers/server.go (6)
165-174: Don’t Fatal on optional logging plugin init; degrade gracefully.A failed optional plugin shouldn’t crash the server. Log and continue.
- loggingPlugin, err = logging.Init(logger, config.LogsStore, pricingManager) - if err != nil { - logger.Fatal("failed to initialize logging plugin: %v", err) - } - plugins = append(plugins, loggingPlugin) + loggingPlugin, err = logging.Init(logger, config.LogsStore, pricingManager) + if err != nil { + logger.Error("failed to initialize logging plugin: %v", err) + } else { + plugins = append(plugins, loggingPlugin) + }
227-237: Don’t Fatal on semantic cache config (un)marshal. Log and skip plugin.Crashing here is disproportionate; continue startup without this plugin.
if plugin.Config != nil { configBytes, err := json.Marshal(plugin.Config) if err != nil { - logger.Fatal("failed to marshal semantic cache config: %v", err) + logger.Error("failed to marshal semantic cache config: %v", err) + break } if err := json.Unmarshal(configBytes, &semCacheConfig); err != nil { - logger.Fatal("failed to unmarshal semantic cache config: %v", err) + logger.Error("failed to unmarshal semantic cache config: %v", err) + break } }
366-366: RemoveSetVersioncall after consolidating version source.If you drop the global, this call should go too.
- SetVersion(s.Version) + // Version is available on s.Version; avoid global duplication.
83-88: Honor provided appDir; don’t treat "./bifrost-data" as “not provided”.Current logic overrides an explicit default value with OS path, changing behavior.
- if appDir != "" && appDir != "./bifrost-data" { + if appDir != "" { return appDir }
418-421: Graceful shutdown:signal.Notifyis commented; add imports and enable it.Without this, SIGINT/SIGTERM won’t be handled.
import ( "context" "embed" "encoding/json" "fmt" "net" "os" + "os/signal" "path/filepath" "runtime" "strings" "time" + "syscall" @@ - // Initializing server signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM) + // Initialize signal handling for graceful shutdown + signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM) + defer signal.Stop(sigChan)
197-215: Maxim plugin ignores plugin.Config → breaking change; support both config and env.Prefer values from
plugin.Configwhen provided; fall back to env.- case maxim.PluginName: - if os.Getenv("MAXIM_LOG_REPO_ID") == "" { - logger.Warn("maxim log repo id is required to initialize maxim plugin") - continue - } - if os.Getenv("MAXIM_API_KEY") == "" { - logger.Warn("maxim api key is required in environment variable MAXIM_API_KEY to initialize maxim plugin") - continue - } - maximPlugin, err := maxim.Init(maxim.Config{ - ApiKey: os.Getenv("MAXIM_API_KEY"), - LogRepoId: os.Getenv("MAXIM_LOG_REPO_ID"), - }) + case maxim.PluginName: + var mcfg maxim.Config + // Prefer explicit config when present + if plugin.Config != nil { + if b, err := json.Marshal(plugin.Config); err != nil { + logger.Warn("maxim: failed to marshal config (falling back to env): %v", err) + } else if err := json.Unmarshal(b, &mcfg); err != nil { + logger.Warn("maxim: failed to unmarshal config (falling back to env): %v", err) + } + } + // Env fallbacks + if mcfg.ApiKey == "" { + mcfg.ApiKey = os.Getenv("MAXIM_API_KEY") + } + if mcfg.LogRepoId == "" { + mcfg.LogRepoId = os.Getenv("MAXIM_LOG_REPO_ID") + } + // Validate required fields + if mcfg.ApiKey == "" || mcfg.LogRepoId == "" { + logger.Warn("maxim: api key and log repo id are required; skipping initialization") + continue + } + maximPlugin, err := maxim.Init(mcfg) if err != nil { logger.Warn("failed to initialize maxim plugin: %v", err) } else { plugins = append(plugins, maximPlugin) }
🧹 Nitpick comments (6)
transports/bifrost-http/handlers/websocket.go (1)
224-226: Fix misleading log message on ctx cancel.This is the WS heartbeat goroutine, not the “webserver”.
- h.logger.Info("got context cancel(), stopping webserver") + h.logger.Info("context canceled; stopping websocket heartbeat")transports/bifrost-http/handlers/server.go (5)
109-112: Use the declared default constant instead of hardcoded string.- if configDir == "" { - configDir = "./bifrost-data" - } + if configDir == "" { + configDir = DefaultAppDir + }
217-221: Redundantif !plugin.Enabledinside case.Already filtered above; remove dead branch.
- case semanticcache.PluginName: - if !plugin.Enabled { - logger.Debug("semantic cache plugin is disabled, skipping initialization") - continue - } + case semanticcache.PluginName:
400-405: Return on routing init error before registering UI.Avoid partially-initialized router state.
- err = s.RegisterRoutes(s.ctx) - // Register UI handler - s.RegisterUIHandler() - if err != nil { + err = s.RegisterRoutes(s.ctx) + if err != nil { return fmt.Errorf("failed to initialize routes: %v", err) } + // Register UI handler + s.RegisterUIHandler()
428-439: Prefer ShutdownWithContext if available.If your fasthttp version supports it, honor
shutdownCtxtimeout.- if err := s.Server.Shutdown(); err != nil { + if err := s.Server.Shutdown(); err != nil { // or: s.Server.ShutdownWithContext(shutdownCtx) logger.Error("error during graceful shutdown: %v", err) } else { logger.Info("server gracefully shutdown") }
128-151: CORS: add Vary: Origin for cache correctness.Prevents intermediaries from serving cached responses across origins.
if IsOriginAllowed(origin, config.ClientConfig.AllowedOrigins) { ctx.Response.Header.Set("Access-Control-Allow-Origin", origin) } + ctx.Response.Header.Add("Vary", "Origin")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
framework/configstore/tables.go(2 hunks)transports/bifrost-http/handlers/handlers.go(0 hunks)transports/bifrost-http/handlers/init.go(1 hunks)transports/bifrost-http/handlers/server.go(1 hunks)transports/bifrost-http/handlers/ui.go(1 hunks)transports/bifrost-http/handlers/websocket.go(4 hunks)transports/bifrost-http/main.go(3 hunks)
💤 Files with no reviewable changes (1)
- transports/bifrost-http/handlers/handlers.go
🚧 Files skipped from review as they are similar to previous changes (3)
- framework/configstore/tables.go
- transports/bifrost-http/handlers/ui.go
- transports/bifrost-http/main.go
🧰 Additional context used
🧬 Code graph analysis (3)
transports/bifrost-http/handlers/init.go (1)
core/schemas/logger.go (1)
Logger(28-55)
transports/bifrost-http/handlers/websocket.go (2)
plugins/logging/utils.go (1)
LogManager(11-20)core/schemas/logger.go (1)
Logger(28-55)
transports/bifrost-http/handlers/server.go (19)
core/schemas/logger.go (4)
LogLevelInfo(11-11)LoggerOutputTypeJSON(21-21)LogLevel(6-6)Logger(28-55)core/bifrost.go (1)
Bifrost(33-48)transports/bifrost-http/handlers/utils.go (1)
IsOriginAllowed(74-82)framework/configstore/clientconfig.go (2)
ClientConfig(28-38)GovernanceConfig(54-60)transports/bifrost-http/handlers/logging.go (2)
LoggingHandler(19-22)NewLoggingHandler(25-30)transports/bifrost-http/handlers/governance.go (2)
GovernanceHandler(20-25)NewGovernanceHandler(28-39)transports/bifrost-http/handlers/cache.go (2)
CacheHandler(10-13)NewCacheHandler(15-25)transports/bifrost-http/handlers/websocket.go (2)
WebSocketHandler(27-36)NewWebSocketHandler(39-49)transports/bifrost-http/handlers/providers.go (1)
NewProviderHandler(29-35)transports/bifrost-http/handlers/completions.go (1)
NewCompletionHandler(33-39)transports/bifrost-http/handlers/mcp.go (1)
NewMCPHandler(24-30)transports/bifrost-http/handlers/integrations.go (1)
NewIntegrationHandler(23-36)transports/bifrost-http/handlers/config.go (1)
NewConfigHandler(26-32)transports/bifrost-http/handlers/plugins.go (1)
NewPluginsHandler(19-24)plugins/telemetry/setup.go (1)
InitPrometheusMetrics(57-175)transports/bifrost-http/handlers/ui.go (1)
NewUIHandler(20-24)transports/bifrost-http/handlers/init.go (1)
SetVersion(14-16)transports/bifrost-http/lib/config.go (1)
LoadConfig(164-732)transports/bifrost-http/lib/account.go (1)
NewBaseAccount(20-24)
🔇 Additional comments (1)
transports/bifrost-http/handlers/init.go (1)
5-11: SetLogger is invoked in main — no immediate nil-panic risk. handlers.SetLogger(logger) is called at transports/bifrost-http/main.go:127.
akshaydeo
force-pushed
the
09-15-seggreate_http_server_into_a_separate_struct
branch
from
76c79e0 to
98ed454
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
transports/bifrost-http/handlers/ui.go (1)
57-58: Consider using path/filepath.Ext for consistency.While this check works, using
filepath.Ext()would be more idiomatic and handle edge cases better.-// Check if this is a static asset request (has file extension) -hasExtension := strings.Contains(filepath.Base(cleanPath), ".") +// Check if this is a static asset request (has file extension) +hasExtension := filepath.Ext(cleanPath) != ""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
framework/configstore/tables.go(2 hunks)transports/bifrost-http/handlers/handlers.go(0 hunks)transports/bifrost-http/handlers/init.go(1 hunks)transports/bifrost-http/handlers/server.go(1 hunks)transports/bifrost-http/handlers/ui.go(1 hunks)transports/bifrost-http/handlers/websocket.go(4 hunks)transports/bifrost-http/main.go(3 hunks)
💤 Files with no reviewable changes (1)
- transports/bifrost-http/handlers/handlers.go
🚧 Files skipped from review as they are similar to previous changes (2)
- framework/configstore/tables.go
- transports/bifrost-http/handlers/init.go
🧰 Additional context used
🧬 Code graph analysis (3)
transports/bifrost-http/handlers/server.go (17)
core/schemas/logger.go (4)
LogLevelInfo(11-11)LoggerOutputTypeJSON(21-21)LogLevel(6-6)Logger(28-55)core/bifrost.go (1)
Bifrost(33-48)transports/bifrost-http/handlers/utils.go (2)
IsOriginAllowed(74-82)SendError(27-36)framework/configstore/clientconfig.go (2)
ClientConfig(28-38)GovernanceConfig(54-60)transports/bifrost-http/handlers/logging.go (2)
LoggingHandler(19-22)NewLoggingHandler(25-30)transports/bifrost-http/handlers/governance.go (2)
GovernanceHandler(20-25)NewGovernanceHandler(28-39)transports/bifrost-http/handlers/cache.go (2)
CacheHandler(10-13)NewCacheHandler(15-25)transports/bifrost-http/handlers/websocket.go (2)
WebSocketHandler(27-36)NewWebSocketHandler(39-49)transports/bifrost-http/handlers/providers.go (1)
NewProviderHandler(29-35)transports/bifrost-http/handlers/completions.go (1)
NewCompletionHandler(33-39)transports/bifrost-http/handlers/mcp.go (1)
NewMCPHandler(24-30)transports/bifrost-http/handlers/integrations.go (1)
NewIntegrationHandler(23-36)transports/bifrost-http/handlers/config.go (1)
NewConfigHandler(26-32)transports/bifrost-http/handlers/plugins.go (1)
NewPluginsHandler(19-24)plugins/telemetry/setup.go (1)
InitPrometheusMetrics(57-175)transports/bifrost-http/lib/config.go (1)
LoadConfig(164-737)transports/bifrost-http/lib/account.go (1)
NewBaseAccount(20-24)
transports/bifrost-http/main.go (2)
transports/bifrost-http/handlers/server.go (7)
BifrostHTTPServer(43-63)DefaultHost(35-35)NewBifrostHTTPServer(66-76)DefaultPort(36-36)DefaultAppDir(37-37)DefaultLogLevel(38-38)DefaultLogOutputStyle(39-39)transports/bifrost-http/handlers/init.go (1)
SetLogger(9-11)
transports/bifrost-http/handlers/websocket.go (2)
plugins/logging/utils.go (1)
LogManager(11-20)core/schemas/logger.go (1)
Logger(28-55)
🔇 Additional comments (18)
transports/bifrost-http/handlers/websocket.go (3)
28-28: LGTM!The addition of the context field to the WebSocketHandler struct enables proper lifecycle management and context cancellation.
39-39: LGTM!The constructor now properly accepts a context parameter, which aligns with the new context-aware design pattern.
224-226: LGTM!The context cancellation handling is properly implemented. The log message clearly indicates the shutdown reason and the function returns immediately upon context cancellation.
transports/bifrost-http/handlers/ui.go (3)
19-24: LGTM!Simple constructor that properly initializes the UIHandler with the embedded file system.
102-109: Good cache strategy implementation.The cache headers are appropriately set: long-term caching for static assets in
_next/static/, no-cache for HTML files to ensure fresh content, and moderate caching for other assets.
32-48: Verify Next.js RSC.txt→/index.txtmapping.
Repo search found no Next.js/RSC references; the only .txt-specific handling is in transports/bifrost-http/handlers/ui.go (lines 32–48). Confirm this special-case is intentional and document/add tests or remove if unnecessary.transports/bifrost-http/main.go (4)
73-73: LGTM!The package-level server variable is now properly declared as a pointer to
BifrostHTTPServer.
115-115: LGTM!The server is now properly initialized using assignment (
=) rather than declaration (:=), which correctly assigns to the package-level variable instead of shadowing it.
124-127: LGTM!The logger configuration is now properly driven by the server's properties, and the logger is correctly set up in the handlers package.
133-142: LGTM!The main function is now properly streamlined, delegating all startup responsibilities to the server's Bootstrap and Start methods. Error handling is clear and appropriate.
transports/bifrost-http/handlers/server.go (8)
42-63: LGTM!The BifrostHTTPServer struct is well-designed with clear separation of concerns. All necessary fields are present for managing the server lifecycle, configuration, and dependencies.
217-220: Redundant disabled plugin check.The outer loop already filters out disabled plugins, so the inner check
if !plugin.Enabledis redundant.Remove the redundant check:
case semanticcache.PluginName: - if !plugin.Enabled { - logger.Debug("semantic cache plugin is disabled, skipping initialization") - continue - }
292-299: Ensure WebSocket handler is properly guarded.The code creates a WebSocket handler only when the logging plugin is available, which is correct. The heartbeat is started immediately, which aligns with the server lifecycle.
65-76: Constructor lacks logger parameter, causing potential nil panics.The constructor doesn't accept or initialize a logger, but many methods in this file use the package-global
loggervariable. This creates a dependency on external initialization and potential nil pointer panics.Consider updating the constructor to accept a logger parameter:
-func NewBifrostHTTPServer(version string, uiContent embed.FS) *BifrostHTTPServer { +func NewBifrostHTTPServer(version string, uiContent embed.FS, logger schemas.Logger) *BifrostHTTPServer { + SetLogger(logger) // Set the package-global logger immediately return &BifrostHTTPServer{ Version: version, UIContent: uiContent, + Logger: logger, // Store on struct for future use Port: DefaultPort, Host: DefaultHost, AppDir: DefaultAppDir, LogLevel: DefaultLogLevel, LogOutputStyle: DefaultLogOutputStyle, } }
84-87: Default app-dir logic inconsistency.The logic treats the literal
"./bifrost-data"string as if the flag was not provided, which is inconsistent with the flag's displayed default and could break existing setups.Apply this fix:
- if appDir != "" && appDir != "./bifrost-data" { + if appDir != "" { return appDir }And at line 111:
- if configDir == "" { - configDir = "./bifrost-data" - } + if configDir == "" { + configDir = DefaultAppDir + }
169-172: Logging plugin initialization should degrade gracefully.The code currently calls
logger.Fatal()when logging plugin initialization fails, which crashes the server. This should degrade gracefully since logging is optional.Apply this fix:
- loggingPlugin, err = logging.Init(logger, config.LogsStore, pricingManager) - if err != nil { - logger.Fatal("failed to initialize logging plugin: %v", err) - } - plugins = append(plugins, loggingPlugin) + loggingPlugin, err = logging.Init(logger, config.LogsStore, pricingManager) + if err != nil { + logger.Error("failed to initialize logging plugin: %v", err) + } else { + plugins = append(plugins, loggingPlugin) + }
230-236: Semantic cache config marshaling should not Fatal.Using
logger.Fatal()for optional plugin configuration errors will crash the entire server. This should degrade gracefully.Apply this fix:
configBytes, err := json.Marshal(plugin.Config) if err != nil { - logger.Fatal("failed to marshal semantic cache config: %v", err) + logger.Error("failed to marshal semantic cache config: %v", err) + continue } if err := json.Unmarshal(configBytes, &semCacheConfig); err != nil { - logger.Fatal("failed to unmarshal semantic cache config: %v", err) + logger.Error("failed to unmarshal semantic cache config: %v", err) + continue }
418-420: Missing signal.Notify call prevents graceful shutdown.The comment on line 420 shows that
signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM)is commented out or missing, which means the server won't handle termination signals properly. The required importsos/signalandsyscallare also missing.Based on standard Go signal handling patterns, add the missing imports and signal notification:
import ( "context" "embed" "encoding/json" "fmt" "net" "os" + "os/signal" "path/filepath" "runtime" "strings" + "syscall" "time"// Create channels for signal and error handling sigChan := make(chan os.Signal, 1) errChan := make(chan error, 1) - // Initializing server signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM) + // Register for signal notifications + signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM)
akshaydeo
force-pushed
the
09-15-seggreate_http_server_into_a_separate_struct
branch
from
98ed454 to
ac5abfd
Compare
akshaydeo
force-pushed
the
09-15-seggreate_http_server_into_a_separate_struct
branch
from
21675b2 to
e41d9ec
Compare
akshaydeo
force-pushed
the
09-15-seggreate_http_server_into_a_separate_struct
branch
from
e41d9ec to
5f6b769
Compare
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/app/teams-customers/views/customerDialog.tsx (1)
184-245: Remove duplicate budget sections
Two blocks underisEditing && customer?.budget(lines 184–218 and 220–245) render the same budget information twice. Consolidate into a single section or remove the redundant block.core/bifrost.go (1)
1246-1263: Preserve post-hook order withpreCount.
pluginCount := len(*bifrost.plugins.Load())re-reads the global slice after queuing. If a concurrent reload removes the plugin before the response arrives,len(*bifrost.plugins.Load())becomes smaller andRunPostHooksruns withrunFrom == 0, so the plugin that already ran a PreHook never gets its PostHook. Pass the originalpreCounthere (and in the streaming error branch) so we always unwind exactly the hooks that ran.- var resp *schemas.BifrostResponse - pluginCount := len(*bifrost.plugins.Load()) + var resp *schemas.BifrostResponse select { case result = <-msg.Response: latency := time.Since(startTime).Milliseconds() if result.ExtraFields.Latency == nil { result.ExtraFields.Latency = Ptr(float64(latency)) } - resp, bifrostErr := pipeline.RunPostHooks(&ctx, result, nil, pluginCount) + resp, bifrostErr := pipeline.RunPostHooks(&ctx, result, nil, preCount) @@ case bifrostErrVal := <-msg.Err: bifrostErrPtr := &bifrostErrVal - resp, bifrostErrPtr = pipeline.RunPostHooks(&ctx, nil, bifrostErrPtr, pluginCount) + resp, bifrostErrPtr = pipeline.RunPostHooks(&ctx, nil, bifrostErrPtr, preCount) @@ - recoveredResp, recoveredErr := pipeline.RunPostHooks(&ctx, nil, &bifrostErrVal, len(*bifrost.plugins.Load())) + recoveredResp, recoveredErr := pipeline.RunPostHooks(&ctx, nil, &bifrostErrVal, preCount)Also applies to: 1370-1376
♻️ Duplicate comments (17)
ui/app/logs/views/speechView.tsx (1)
95-97: AudioPlayer renders with empty src during streaming.This change removes the conditional rendering check for
isStreaming, causing theAudioPlayerto always render when the Speech Output section is visible. WhenisStreamingis true butspeechOutputis null orspeechOutput.audiois empty, the player receives an empty string as thesrc, which may cause display issues or errors despite the error boundary.This concern was previously flagged in the existing review comment.
plugins/maxim/main.go (1)
454-459: Guard against nilbifrostErr.Errorbefore dereferencing.Line 453 checks if
bifrostErr != nil, but lines 455-457 dereferencebifrostErr.Errorfields without verifying thatbifrostErr.Erroritself is non-nil. IfbifrostErr.Erroris nil, this will panic.Apply this diff to add the missing nil check:
- if bifrostErr != nil { + if bifrostErr != nil && bifrostErr.Error != nil { genErr := logging.GenerationError{ Message: bifrostErr.Error.Message, Code: bifrostErr.Error.Code, Type: bifrostErr.Error.Type, } logger.SetGenerationError(generationID, &genErr)Alternatively, if
bifrostErrcan have a nilErrorfield but you still want to log something, construct a safe fallbackGenerationErrorusing available data (e.g.,bifrostErr.Messageor a default message).ui/app/_fallbacks/enterprise/views/contactUsView.tsx (1)
31-31: Mark decorative icon as hidden from screen readers.The
ArrowUpRighticon should be marked as decorative to prevent screen readers from announcing it separately, as suggested in a previous review.Apply this diff:
- Read more <ArrowUpRight className="text-muted-foreground h-3 w-3" /> + Read more <ArrowUpRight className="text-muted-foreground h-3 w-3" aria-hidden="true" />plugins/logging/changelog.md (1)
4-4: Remove duplicate "Fix:"Line 4 repeats "Fix:" twice. This was flagged in a previous review.
Apply this diff:
-- Fix: Fix capturing Bifrost-specific errors in logs (e.g. provider not configured) +- Fix: Capture Bifrost-specific errors in logs (e.g., provider not configured)transports/bifrost-http/handlers/middlewares_test.go (1)
27-28: Rebind loop variable to avoid closure capture issue.The subtest closure captures the loop variable
origin, causing all subtests to execute with the final slice value instead of the intended per-iteration value.Rebind the variable before creating the subtest:
for _, origin := range localhostOrigins { + origin := origin // rebind for closure t.Run(origin, func(t *testing.T) {Alternatively, pass the value as a parameter to an immediately-invoked function.
transports/config.schema.json (1)
607-614: Allow IPv6 collector endpoints.
collector_urlstill uses^[^:\\s]+:\\d+$, so[::1]:4317and other bracketed IPv6 hosts fail validation. Please widen the pattern as previously suggested.- { - "pattern": "^[^:\\s]+:\\d+$" - } + { + "pattern": "^(\\[[0-9A-Fa-f:.%]+\\]|[^\\[\\]:\\s]+):\\d+$" + }transports/bifrost-http/handlers/plugins.go (1)
190-207: Return 500 on real lookup errors.The fallback branch treats every non-
ErrNotFounderror as a 404, masking DB/connection failures and incorrectly telling clients the plugin is missing. Only respond with 404 when the not-found sentinel matches; otherwise propagate a 500.- } else { - h.logger.Error("failed to get plugin: %v", err) - SendError(ctx, 404, "Plugin not found", h.logger) - return - } + } else { + h.logger.Error("failed to get plugin: %v", err) + SendError(ctx, fasthttp.StatusInternalServerError, "Failed to retrieve plugin", h.logger) + return + }framework/go.mod (1)
15-16: LGTM! Dependency upgrades aligned.The upgrades to gorm v1.31.0, addition of Postgres driver v1.6.0, and updates to gRPC/protobuf dependencies are consistent across modules. Per the past review comment, ensure
go mod tidyis run to syncgo.sum.Based on learnings
Also applies to: 91-96
plugins/logging/pool.go (1)
20-24: Restore StreamUpdateData reset to avoid stale pooled state.
putLogMessagenow leavesmsg.StreamUpdateDatauntouched. Because log messages are pooled, reusing an instance with a non-nilStreamUpdateDataleaks the previous request’s stream payload into the next caller. We still need to nil that pointer when returning the message to the pool.Apply this diff to clear the field:
// Don't reset UpdateData and StreamUpdateData here since they're returned // to their own pools in the defer function - just clear the pointers - msg.UpdateData = nil + msg.UpdateData = nil + msg.StreamUpdateData = nilplugins/otel/grpc.go (1)
19-23: Default to TLS (or require explicit opt-in for insecure transport).The gRPC client still dials with
insecure.NewCredentials(), so every OTLP export runs unencrypted. This was already flagged earlier and remains unresolved. Please make TLS the default (system roots or configured certs) and only fall back to insecure when an explicit “allow_insecure” config is set.-import ( - "context" - - collectorpb "go.opentelemetry.io/proto/otlp/collector/trace/v1" - "google.golang.org/grpc" - "google.golang.org/grpc/credentials/insecure" -) +import ( + "context" + "crypto/tls" + + collectorpb "go.opentelemetry.io/proto/otlp/collector/trace/v1" + "google.golang.org/grpc" + "google.golang.org/grpc/credentials" + "google.golang.org/grpc/credentials/insecure" +) @@ -func NewOtelClientGRPC(endpoint string) (*OtelClientGRPC, error) { - conn, err := grpc.NewClient(endpoint, grpc.WithTransportCredentials(insecure.NewCredentials())) +func NewOtelClientGRPC(endpoint string, allowInsecure bool) (*OtelClientGRPC, error) { + creds := grpc.WithTransportCredentials(credentials.NewTLS(&tls.Config{})) + if allowInsecure { + creds = grpc.WithTransportCredentials(insecure.NewCredentials()) + } + + conn, err := grpc.NewClient(endpoint, creds)(call sites must pass the new flag/config and choose
allowInsecureonly for trusted local setups.)ui/app/observability/fragments/otelFormFragment.tsx (1)
181-184: Reset must keepotel_configin a valid shape.
otelFormSchemarequiresotel_config, but the Reset handler sets it toundefined, immediately invalidating the form and blocking users from saving defaults. Reset should restore the schema-compliant defaults instead of wiping the object.- form.reset({ - enabled: false, - otel_config: undefined, - }); + form.reset({ + enabled: false, + otel_config: { + collector_url: "", + trace_type: "otel", + protocol: "http", + }, + });framework/pricing/main.go (1)
108-110: Use a background context for the sync worker
context.WithCancel(ctx)still ties the long-lived worker to the init context, so any cancellation of the caller (startup, CLI, tests) tears down the sync loop immediately. Please derive the worker context fromcontext.Background()(optionally wrapped in your own cancel) before launching the goroutine.framework/streaming/accumulator.go (1)
279-293: Return pooled chunks through the reset helpersInside
Cleanupwe’re pushing cached chunks straight back into the pools viachatStreamChunkPool.Put/audioStreamChunkPool.Put/transcriptionStreamChunkPool.Put. Those methods bypass the zeroing logic input{Type}StreamChunk, so prior request state (cache debug, cost, errors, etc.) bleeds into the next borrower. Please funnel everything through the helper so each chunk is scrubbed before reuse.Suggested fix:
- for _, chunk := range accumulator.ChatStreamChunks { - a.chatStreamChunkPool.Put(chunk) - } - for _, chunk := range accumulator.TranscriptionStreamChunks { - a.transcriptionStreamChunkPool.Put(chunk) - } - for _, chunk := range accumulator.AudioStreamChunks { - a.audioStreamChunkPool.Put(chunk) - } + for _, chunk := range accumulator.ChatStreamChunks { + a.putChatStreamChunk(chunk) + } + for _, chunk := range accumulator.TranscriptionStreamChunks { + a.putTranscriptionStreamChunk(chunk) + } + for _, chunk := range accumulator.AudioStreamChunks { + a.putAudioStreamChunk(chunk) + }plugins/governance/main.go (2)
203-206: Fix virtual key lookup in PostHook.
ContextKey(schemas.BifrostContextKeyVirtualKeyHeader)creates a different key type than the one used to populate the context, sovirtualKeycomes back empty and the usage update is skipped. That breaks rate-limit/budget tracking for every request. Please query the context with the sameschemas.BifrostContextKeyVirtualKeyHeadervalue you already use inPreHook.- virtualKey := getStringFromContext(*ctx, ContextKey(schemas.BifrostContextKeyVirtualKeyHeader)) + virtualKey := getStringFromContext(*ctx, schemas.BifrostContextKeyVirtualKeyHeader)
299-301: Avoid canceling usage updates during shutdown.
Cleanup()callsp.cancelFunc()before the goroutine completes, so every in-flightUpdateUsagesees a cancelled context and drops the write—usage, rate limits, and budgets all go stale. Detach these writes from the plugin’s cancellable context (give them their own background/timeout context) so they can flush even while shutdown proceeds.- p.tracker.UpdateUsage(p.ctx, usageUpdate) + updateCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + p.tracker.UpdateUsage(updateCtx, usageUpdate)Add the corresponding
timeimport at the top of the file.plugins/logging/main.go (1)
333-339: Do not bubble logging failures back to the caller.
PostHookstill returns the accumulator error, so a transient logging failure aborts the user request. Log and continue instead; keep the hook non-blocking.- streamResponse, err := p.accumulator.ProcessStreamingResponse(ctx, result, bifrostErr) + streamResponse, err := p.accumulator.ProcessStreamingResponse(ctx, result, bifrostErr) if err != nil { - p.logger.Error("failed to process streaming response: %v", err) - return result, bifrostErr, err + p.logger.Error("failed to process streaming response: %v", err) + return result, bifrostErr, nil }transports/bifrost-http/lib/config.go (1)
544-555: Keep auto-detected providers when seeding the DB.When
configData.Providersis nil we auto-detect intoconfig.Providers, but the subsequentUpdateProvidersConfigcall still writes the original (empty)processedProvidersmap, wiping the detected providers from the store.Assign the detected providers before persisting, e.g.
- if config.ConfigStore != nil { + if config.ConfigStore != nil { logger.Debug("updating providers config in store") - err = config.ConfigStore.UpdateProvidersConfig(ctx, processedProviders) + if len(processedProviders) == 0 { + processedProviders = config.Providers + } + err = config.ConfigStore.UpdateProvidersConfig(ctx, processedProviders)
🧹 Nitpick comments (6)
ui/app/logs/views/logsTable.tsx (1)
81-82: Consider extracting the magic value for maintainability.The value
calc(100vh-20rem)is hardcoded in two places. If this constraint needs adjustment in the future, both locations would require updates.Consider extracting this to a constant or CSS variable:
const TABLE_MAX_HEIGHT = "max-h-[calc(100vh-20rem)]";Then use:
-<div className="rounded-sm border max-h-[calc(100vh-20rem)]"> - <Table containerClassName="max-h-[calc(100vh-20rem)]"> +<div className={`rounded-sm border ${TABLE_MAX_HEIGHT}`}> + <Table containerClassName={TABLE_MAX_HEIGHT}>transports/bifrost-http/handlers/middlewares_test.go (1)
19-25: Add IPv6 localhost and HTTPS 0.0.0.0 test cases.The localhost origins list is missing common development variants:
http://[::1]:<port>(IPv6 localhost HTTP)https://[::1]:<port>(IPv6 localhost HTTPS)https://0.0.0.0:<port>(HTTPS on all interfaces)Add these test cases to ensure comprehensive localhost origin coverage:
localhostOrigins := []string{ "http://localhost:3000", "https://localhost:3000", "http://127.0.0.1:8080", "http://0.0.0.0:5000", "https://127.0.0.1:3000", + "http://[::1]:3000", + "https://[::1]:3000", + "https://0.0.0.0:5000", }Ensure that the
isLocalhostOriginhelper intransports/bifrost-http/handlers/utils.gotreats these origins as local.core/providers/utils.go (1)
135-166: Match skip list against canonical header names.
skipHeadersis documented as canonicalized, but the comparison still uses the originalkey. IfextraHeaderscontains a lowercase entry (e.g.,"x-custom-header"), the skip check breaks because"x-custom-header"≠"X-Custom-Header", so the header slips through. Compare againstcanonicalKeyin both functions to keep the skip list effective.- if skipHeaders != nil { - if slices.Contains(skipHeaders, key) { + if skipHeaders != nil { + if slices.Contains(skipHeaders, canonicalKey) {Apply the same change in
setExtraHeadersHTTP.framework/docker-compose.yml (1)
6-7: Consider externalizing credentials for flexibility.Hardcoded credentials (
bifrost/bifrost_password) are acceptable for local development but consider documenting that these should be overridden via environment variables in shared or production-like environments.framework/logstore/postgres.go (1)
23-40: Consider validating config fields before use.The function does not validate that required config fields (Host, Port, User, DBName, SSLMode) are non-empty before attempting to connect. This could lead to obscure connection errors.
Add validation:
if config.Host == "" || config.Port == "" || config.User == "" || config.DBName == "" { return nil, fmt.Errorf("missing required Postgres config fields") }ui/app/virtual-keys/page.tsx (1)
16-32: Consider consolidating error handling.The three separate
useEffecthooks for error handling could be consolidated into a single effect or a custom hook to reduce code duplication.Example:
useEffect(() => { if (vkError) toast.error(`Failed to load virtual keys: ${getErrorMessage(vkError)}`); if (teamsError) toast.error(`Failed to load teams: ${getErrorMessage(teamsError)}`); if (customersError) toast.error(`Failed to load customers: ${getErrorMessage(customersError)}`); }, [vkError, teamsError, customersError]);
akshaydeo
force-pushed
the
09-15-seggreate_http_server_into_a_separate_struct
branch
from
0829918 to
4aba40f
Compare
akshaydeo
force-pushed
the
09-15-seggreate_http_server_into_a_separate_struct
branch
from
4aba40f to
025f8a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/schemas/chatcompletions.go (1)
232-243: Clear staleChatMessageContentstate on UnmarshalDecoding a string payload sets
ContentStrbut leaves any previousContentBlocksslice in place (and vice-versa). When the struct is reused—which is common during streaming decodes—you end up with both fields non-nil, soMarshalJSONfails the mutual-exclusion check and callers see stale blocks. Please nil the opposite field in each branch.Apply this diff:
@@ var stringContent string if err := sonic.Unmarshal(data, &stringContent); err == nil { mc.ContentStr = &stringContent + mc.ContentBlocks = nil return nil } @@ var arrayContent []ChatContentBlock if err := sonic.Unmarshal(data, &arrayContent); err == nil { mc.ContentBlocks = arrayContent + mc.ContentStr = nil return nil }
♻️ Duplicate comments (2)
plugins/otel/go.mod (1)
7-11: Update core dependency to match other modules.This module requires
core v1.1.34whileplugins/loggingusesv1.1.37. Align to the latest version used across the repo to prevent API drift.plugins/otel/converter.go (1)
120-126: Format embedding values as floats, not integers.Embedding values are floats but are formatted as integers using
%d, which truncates decimal values and causes data loss.Apply this diff:
if req.Input.Embedding != nil { embedding := make([]string, len(req.Input.Embedding)) for i, v := range req.Input.Embedding { - embedding[i] = fmt.Sprintf("%d", v) + embedding[i] = fmt.Sprintf("%f", v) } params = append(params, kvStr("gen_ai.input.embedding", strings.Join(embedding, ","))) }
🧹 Nitpick comments (1)
ui/app/teams-customers/page.tsx (1)
58-75: Expose active tab state to assistive tech.Right now the buttons don’t convey which view is selected, so screen readers announce identical controls. Adding a
role="tablist"wrapper andaria-pressed(oraria-current) on the active button restores the lost semantics from the old Tabs component.- <div className="flex min-w-[200px] flex-col gap-1 rounded-md bg-zinc-50/50 p-4 dark:bg-zinc-800/20"> + <div + className="flex min-w-[200px] flex-col gap-1 rounded-md bg-zinc-50/50 p-4 dark:bg-zinc-800/20" + role="tablist" + aria-label="Teams and customers views" + > {["teams", "customers"].map((tab) => ( <button key={tab} className={cn( "mb-1 flex w-full items-center gap-2 rounded-sm border px-3 py-1.5 text-sm", activeTab === tab ? "bg-secondary opacity-100 hover:opacity-100" : "hover:bg-secondary cursor-pointer border-transparent opacity-100 hover:border", )} onClick={() => setActiveTab(tab)} type="button" + aria-pressed={activeTab === tab} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
plugins/logging/go.sumis excluded by!**/*.sumplugins/otel/go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
core/schemas/chatcompletions.go(12 hunks)core/schemas/providers/anthropic/chat.go(18 hunks)core/schemas/providers/anthropic/responses.go(21 hunks)core/schemas/providers/bedrock/responses.go(6 hunks)core/schemas/providers/cohere/responses.go(8 hunks)core/schemas/utils.go(0 hunks)plugins/logging/go.mod(2 hunks)plugins/otel/converter.go(1 hunks)plugins/otel/go.mod(1 hunks)tests/core-providers/scenarios/multiple_images.go(1 hunks)tests/core-providers/scenarios/reasoning.go(2 hunks)tests/core-providers/scenarios/response_validation.go(3 hunks)tests/core-providers/scenarios/utils.go(7 hunks)ui/app/teams-customers/page.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- core/schemas/utils.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/schemas/providers/bedrock/responses.go
🧰 Additional context used
🧬 Code graph analysis (9)
tests/core-providers/scenarios/response_validation.go (1)
core/schemas/chatcompletions.go (1)
ChatAssistantMessage(296-300)
core/schemas/providers/anthropic/chat.go (2)
core/schemas/providers/anthropic/types.go (5)
AnthropicMessageRoleAssistant(52-52)AnthropicContentBlock(116-126)AnthropicContent(62-65)AnthropicMessage(56-59)AnthropicMessageError(273-276)core/schemas/chatcompletions.go (2)
ChatAssistantMessage(296-300)ChatMessageContent(204-207)
tests/core-providers/scenarios/utils.go (2)
core/schemas/chatcompletions.go (2)
ChatContentBlock(260-267)ChatAssistantMessage(296-300)core/schemas/responses.go (1)
ResponsesMessageContentBlock(527-538)
ui/app/teams-customers/page.tsx (3)
ui/lib/utils.ts (1)
cn(4-6)ui/app/teams-customers/views/teamsTable.tsx (1)
TeamsTable(34-204)ui/app/teams-customers/views/customerTable.tsx (1)
CustomersTable(34-218)
tests/core-providers/scenarios/multiple_images.go (1)
core/schemas/chatcompletions.go (1)
ChatContentBlock(260-267)
core/schemas/providers/anthropic/responses.go (3)
core/schemas/utils.go (1)
SafeExtractStringSlice(432-460)core/schemas/responses.go (2)
ResponsesMessageContentBlock(527-538)ResponsesFunctionToolCallOutput(860-863)core/schemas/providers/anthropic/types.go (2)
AnthropicContent(62-65)AnthropicContentBlock(116-126)
plugins/otel/converter.go (4)
plugins/otel/types.go (13)
KeyValue(21-21)AnyValue(24-24)StringValue(27-27)IntValue(30-30)DoubleValue(33-33)BoolValue(36-36)ArrayValue(39-39)ArrayValueValue(42-42)ListValue(45-45)KeyValueList(48-48)ResourceSpan(9-9)ScopeSpan(12-12)Span(15-15)core/schemas/bifrost.go (13)
BifrostSpeechRequest(152-158)BifrostEmbeddingRequest(144-150)BifrostTextCompletionRequest(120-126)BifrostChatRequest(128-134)BifrostTranscriptionRequest(160-166)BifrostRequest(15-27)RequestType(89-89)TextCompletionRequest(92-92)ChatCompletionRequest(93-93)ChatCompletionStreamRequest(94-94)TranscriptionStreamRequest(101-101)SpeechStreamRequest(99-99)BifrostResponse(177-192)core/schemas/chatcompletions.go (5)
ChatMessageRoleUser(186-186)ChatMessageRoleAssistant(185-185)ChatMessageRoleSystem(187-187)ChatMessageRoleTool(188-188)ChatMessageRoleDeveloper(189-189)framework/pricing/main.go (1)
PricingManager(22-38)
core/schemas/providers/cohere/responses.go (1)
core/schemas/utils.go (1)
SafeExtractStringSlice(432-460)
core/schemas/chatcompletions.go (2)
core/schemas/providers/gemini/types.go (2)
FileData(1073-1081)Type(778-778)ui/lib/types/logs.ts (2)
Citation(227-234)Function(138-142)
⏰ 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). (50)
- 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
- 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 (18)
core/schemas/providers/cohere/responses.go (5)
35-65: LGTM! Pointer-to-value slice refactor is correct.The migration from pointer-based to value-based slices for
stopsequences (line 35) andTools(line 65) is implemented correctly.SafeExtractStringSlicehandles both[]stringand[]*stringinputs safely, and the direct slice assignment on line 65 is protected by the length check on line 64.
173-190: LGTM! Content block iteration refactored correctly.The direct iteration over
msg.Content.ContentBlocks(line 173) and passing the slice directly to the conversion function (line 190) are both correct. The nil checks on lines 172 and 189 ensure these operations are safe.
228-257: LGTM! Tool call handling updated consistently.The direct slice assignments for
ToolCalls(line 228) and content block conversions (line 257) align with the pointer-to-value refactor. Both are protected by appropriate nil checks on lines 227 and 256.
333-353: LGTM! Output conversion logic correctly refactored.The direct iterations over
BlocksContent(line 333) andToolCalls(line 353), along with the direct slice assignment (line 337), are all safe. Theelse-ifbranch on line 331 ensuresBlocksContentis non-nil, and line 352 protects theToolCallsiteration.
35-353: Slice field types validated
CohereChatRequest.Tools, CohereMessage.ToolCalls, and ResponsesMessageContent.ContentBlocks are all defined as slice types, so no further refactor is needed.core/schemas/providers/anthropic/chat.go (5)
34-45: LGTM! Pointer-to-slice refactoring simplifies the code.The change from iterating over
*request.System.ContentBlocksto direct slice access (request.System.ContentBlocks) removes unnecessary pointer indirection. The nil check at line 32 remains appropriate for slices, and the direct assignment at line 45 (ContentBlocks: contentBlocks) is cleaner than the previous pointer-based approach.
65-126: LGTM! Content block and tool call conversions updated consistently.The refactoring successfully removes pointer indirection for
ContentBlocks(lines 65, 101, 120) andToolCalls(line 126). The iteration patterns and assignments are correct, and the nil checks at lines 60, 100 remain valid for slices.
163-186: LGTM! Tools conversion simplified with direct slice access.The change from iterating over a pointer (
*request.Tools) to direct slice access (line 163) and the direct assignment at line 376 (Tools = tools) are correct and consistent with the overall refactoring.
423-528: LGTM! Message conversion logic handles direct slices correctly.The refactoring updates multiple conversion paths:
- System content blocks (lines 423-433)
- Tool result content blocks (lines 451-463)
- Regular message content blocks (lines 489-513)
- Final content assignment (line 528)
All iterations and assignments follow the direct-slice pattern consistently. The nil checks remain appropriate, and the slice construction logic is correct.
582-594: LGTM! Response conversion updated to use direct slices.The changes at lines 582 and 594 correctly iterate over
ContentBlocksandToolCallsas direct slices. The conversion logic remains semantically correct.core/schemas/providers/anthropic/responses.go (8)
38-38: LGTM! Stop sequences assigned as direct slice value.The change from pointer-to-slice to direct slice assignment (
params.ExtraParams["stop"] = request.StopSequences) is correct and consistent with the refactoring pattern.
53-90: LGTM! System content and tools conversion simplified.The direct iteration over
request.System.ContentBlocks(line 53) andrequest.Tools(line 82) removes unnecessary pointer indirection. The conversion logic remains correct.
132-147: LGTM! Stop sequences and tools extraction updated correctly.The usage of
SafeExtractStringSliceat line 132 correctly returns a slice, and the direct assignment ofanthropicToolsat line 147 (Tools = anthropicTools) is consistent with the refactoring.
295-366: LGTM! Message conversion handles direct slices correctly.The refactoring updates content block iterations and assignments throughout the conversion logic. The patterns are consistent and correct.
447-454: LGTM! Good use of defensive copying to avoid aliasing issues.The pattern of explicitly copying slices before assignment (e.g.,
copied := make([]AnthropicContentBlock, len(pendingToolCalls)); copy(copied, pendingToolCalls)) prevents unintended mutations whenpendingToolCallsis reused across iterations. This is good defensive programming practice.Also applies to: 587-595, 727-734
466-513: LGTM! System and regular message content blocks handled correctly.The direct iteration over content blocks (lines 466, 506) and direct slice assignments (lines 473, 513) are consistent with the refactoring. The nil checks remain appropriate for slices.
540-552: LGTM! Content block aliasing is safe in this context.The aliasing of
targetMsg.Content.ContentBlocksat line 540 is safe because:
targetMsgis a valid pointer (either to existing element or newly appended element)- The append at line 548 may reallocate, but the result is reassigned
- The entire
Contentstruct is replaced at line 551, ensuring the new slice is usedThis differs from the defensive copying used with
pendingToolCallsbecausecontentBlocksis built up once and immediately assigned, not reused across iterations.
621-635: LGTM! Tool output and content block conversions updated consistently.The direct iteration over content blocks (lines 621, 894, 943) and direct slice assignments (lines 628, 635, 962) follow the refactoring pattern correctly. The conversion logic remains semantically correct.
Also applies to: 894-900, 943-962
akshaydeo
force-pushed
the
09-15-seggreate_http_server_into_a_separate_struct
branch
from
025f8a9 to
29fb578
Compare
akshaydeo
force-pushed
the
09-15-seggreate_http_server_into_a_separate_struct
branch
from
29fb578 to
feddb47
Compare
akshaydeo
force-pushed
the
09-15-seggreate_http_server_into_a_separate_struct
branch
from
feddb47 to
6110981
Compare
2 participants
Summary
Refactors the HTTP server implementation by extracting it into a dedicated
BifrostHTTPServerstruct in a new file, improving code organization and maintainability.Changes
server.gofile with a dedicatedBifrostHTTPServerstruct to encapsulate server functionalityMaxRequestBodySizeMBin theTableClientConfigstructSetLoggerfunction to the handlers package to allow dependency injection of the loggerTableVirtualKeystructType of change
Affected areas
How to test
Breaking changes
Related issues
Improves code organization and maintainability
Security considerations
No security implications as this is a refactoring of existing code.
Checklist
docs/contributing/README.mdand followed the guidelinesNote
Adds OTEL tracing plugin with UI, dynamic plugin reload/remove, a refactored HTTP server, context-aware stores, and several core/plugin fixes (latency, governance/logging, Maxim).
plugins/otelwith HTTP/gRPC emit, GenAI attrs, TTL map; config schema and example docker-compose.coresupports atomic plugin list,ReloadPlugin/RemovePlugin; HTTP API to manage plugins.BifrostHTTPServer(server.go), middleware chaining (CORS),SetLogger, route registrations updated.context.Contextto config/log store APIs, Close() methods; migrations use ctx; defaultMaxRequestBodySizeMB=100.atomic.Pointer; latency captured asint64; Bedrock latency type aligned; new streaming accumulator structs.APIKey,LogRepoID) and PostHook simplified.transports/config.schema.jsonplugin array + OTEL schema; bumpedgorm, deps; misc fixes (Makefile cleanup, not-found page).Written by Cursor Bugbot for commit cd8a4fb. This will update automatically on new commits. Configure here.