feat: add Maxim prompt integration and update maxim-go to v0.1.13 #594
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughUpgrades maxim-go to v0.1.13, changes Maxim plugin Init to accept a logger and store it, extends transport interception to enrich chat completions from Maxim headers, updates middleware and server wiring to account for Maxim presence, adjusts tests and UI text, tweaks OTEL docker-compose and Next.js build config. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant HTTP as HTTP Server
participant MW as TransportInterceptorMiddleware
participant Registry as PluginRegistry
participant Maxim as Maxim Plugin
Client->>HTTP: Request (chat/text completion)
HTTP->>MW: Run TransportInterceptorMiddleware
MW->>Registry: Check loaded plugins (governance, maxim)
alt Neither governance nor maxim loaded
MW-->>HTTP: Skip interception
else Maxim present
MW->>Maxim: TransportInterceptor(req)
Maxim->>Maxim: Parse `x-bf-maxim-*` headers
Maxim->>Maxim: Load prompt/version via maxim-go v0.1.13
Maxim->>Maxim: Populate model, messages, params (with nil guards)
Maxim-->>MW: Return modified request/context
end
MW-->>HTTP: Continue handler chain
HTTP-->>Client: Response
sequenceDiagram
autonumber
participant Server as Bifrost Server
participant Logger as Logger
participant Maxim as Maxim Plugin
Server->>Logger: Create logger
Server->>Maxim: Init(config, logger)
Maxim->>Maxim: Store logger on Plugin
Maxim-->>Server: Plugin instance
note right of Maxim: Logger used for debug when log repo ID missing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
plugins/maxim/main.go (5)
134-145: Simplify and harden header matching; remove redundant castsUse EqualFold for case-insensitive match; avoid string() on strings.
- for header, value := range headers { - if strings.ToLower(string(header)) == "x-bf-maxim-prompt-id" { - promptID = string(value) - } - if strings.ToLower(string(header)) == "x-bf-maxim-prompt-version-id" { - promptVersionID = string(value) - } + for h, v := range headers { + if strings.EqualFold(h, "x-bf-maxim-prompt-id") { + promptID = v + } + if strings.EqualFold(h, "x-bf-maxim-prompt-version-id") { + promptVersionID = v + } if promptID != "" && promptVersionID != "" { break } }
151-159: Prefer graceful degradation on Maxim fetch failuresFailing the entire request on GetPromptVersion error may be harsh. Consider logging and passing through unchanged when headers are present but fetch fails.
- promptVersion, err := plugin.mx.GetPromptVersion(promptVersionID, promptID) - if err != nil { - return headers, body, err - } + promptVersion, err := plugin.mx.GetPromptVersion(promptVersionID, promptID) + if err != nil { + if plugin.logger != nil { + plugin.logger.Warn("maxim: failed to fetch prompt version (%s/%s): %v", promptID, promptVersionID, err) + } + return headers, body, nil + }
190-216: Scrub Maxim-specific headers to avoid leaking internals to providersAfter populating the body, drop x-bf-maxim-prompt-id and x-bf-maxim-prompt-version-id from headers.
body["messages"] = messages @@ if params.N != 0 { body["n"] = params.N } - return headers, body, nil + // Remove internal Maxim headers before forwarding + for k := range headers { + if strings.EqualFold(k, "x-bf-maxim-prompt-id") || strings.EqualFold(k, "x-bf-maxim-prompt-version-id") { + delete(headers, k) + } + } + return headers, body, nil
58-66: Avoid variable shadowing of parameter ‘logger’Local var logger returned from mx.GetLogger shadows the Init parameter, reducing clarity.
- logger, err := mx.GetLogger(&logging.LoggerConfig{Id: config.LogRepoID}) + mxLogger, err := mx.GetLogger(&logging.LoggerConfig{Id: config.LogRepoID}) if err != nil { return nil, fmt.Errorf("failed to initialize default logger: %w", err) } - plugin.loggers[config.LogRepoID] = logger + plugin.loggers[config.LogRepoID] = mxLogger
441-449: Align ResponsesRequest text extraction with block Type check (consistency)Chat path checks Type == text; Responses path doesn’t. Make consistent to avoid picking non-text blocks.
- } else if lastMsg.Content.ContentBlocks != nil { + } else if lastMsg.Content.ContentBlocks != nil { // Find the last text content block for i := len(lastMsg.Content.ContentBlocks) - 1; i >= 0; i-- { block := (lastMsg.Content.ContentBlocks)[i] - if block.Text != nil { + if block.Type == schemas.ChatContentBlockTypeText && block.Text != nil { latestMessage = *block.Text break } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
plugins/maxim/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
plugins/maxim/go.mod(1 hunks)plugins/maxim/main.go(6 hunks)plugins/maxim/plugin_test.go(2 hunks)plugins/otel/docker-compose.yml(2 hunks)transports/bifrost-http/handlers/middlewares.go(2 hunks)transports/bifrost-http/handlers/server.go(1 hunks)transports/go.mod(1 hunks)ui/app/observability/views/plugins/maximView.tsx(1 hunks)ui/next.config.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
transports/bifrost-http/handlers/middlewares.go (2)
plugins/maxim/main.go (1)
PluginName(21-21)plugins/governance/main.go (1)
PluginName(19-19)
transports/bifrost-http/handlers/server.go (1)
plugins/maxim/main.go (1)
Init(39-68)
plugins/maxim/plugin_test.go (3)
core/logger.go (1)
NewDefaultLogger(40-49)core/schemas/logger.go (1)
LogLevelDebug(10-10)plugins/maxim/main.go (1)
Init(39-68)
plugins/maxim/main.go (3)
core/schemas/logger.go (1)
Logger(28-55)core/schemas/plugin.go (1)
Plugin(45-71)core/schemas/chatcompletions.go (1)
ChatContentBlockTypeText(272-272)
⏰ 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). (2)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (12)
ui/app/observability/views/plugins/maximView.tsx (1)
43-43: LGTM! Header name updated correctly.The documentation text now correctly references
x-bf-maxim-log-repo-idinstead ofx-bf-log-repo-id, which aligns with the Maxim plugin's expected header naming convention.plugins/otel/docker-compose.yml (3)
67-67: LGTM! Secure default for unsigned plugins.Setting
GF_PLUGINS_ALLOW_LOADING_UNSIGNED_PLUGINSto an empty string disallows unsigned plugins, which is the secure default.
187-190: LGTM! Bifrost metrics endpoint added.The new Prometheus scrape job correctly targets the Bifrost service at
host.docker.internal:8080/metrics.
196-204: Verify that simplified datasource configuration meets requirements.The datasource configuration has been significantly simplified, removing advanced Tempo features like
tracesToMetrics,nodeGraph, andsearchconfigurations. While the basic datasource connectivity remains, these features provided enhanced tracing visualization capabilities.Confirm that the removed Tempo features are not needed for your observability requirements. If you need advanced trace analysis, consider restoring:
tracesToMetrics: Links traces to metrics for performance analysisnodeGraph: Visualizes service dependenciessearch: Enhanced trace search capabilitiestransports/go.mod (1)
75-75: LGTM! Dependency version updated.The maxim-go dependency has been updated to v0.1.13, aligning with the version in plugins/maxim/go.mod.
plugins/maxim/go.mod (1)
9-9: LGTM! Maxim SDK updated to v0.1.13.The dependency update aligns with the PR objectives and supports the new prompt integration features.
Based on learnings
transports/bifrost-http/handlers/server.go (1)
214-214: LGTM! Logger parameter added to Maxim plugin initialization.The Init call has been correctly updated to pass the logger parameter, consistent with the updated plugin interface signature.
plugins/maxim/plugin_test.go (2)
35-35: LGTM! Test initialization updated correctly.The
getPluginfunction now passes a logger toInit, matching the updated plugin interface.
232-232: LGTM! Error test updated with logger parameter.The error path test correctly passes a logger to
Init, ensuring consistent behavior across test scenarios.plugins/maxim/main.go (3)
392-406: Good nil-safety for latest chat content extractionSolid guards for nil content and non-text blocks; fallback to "-".
219-238: Confirm header→context mapping for LogRepoIDKey incomingx-bf-maxim-log-repo-idheaders are mapped toLogRepoIDKeyin transports/bifrost-http/lib/ctx.go; tests in plugin_test.go already cover this.
156-191: Ensure JSON shape of apis.ChoiceMessage matches schemas.ChatMessage expected by the /chat/completions decoder
Verify thatapis.ChoiceMessagemarshals to JSON fieldsname,role, andcontentas defined byschemas.ChatMessageintransports/bifrost-http/handlers/inference.go. If it differs, map toschemas.ChatMessagebefore assigning tobody["messages"].
| // Check if the url is for chat completion | ||
| if !strings.Contains(url, "/chat/completions") { | ||
| return headers, body, nil | ||
| } | ||
|
|
||
| // Check if headers have `x-maxim-prompt-id` and `x-maxim-prompt-version-id` | ||
| var promptID string | ||
| var promptVersionID string | ||
|
|
||
| for header, value := range headers { | ||
| if strings.ToLower(string(header)) == "x-bf-maxim-prompt-id" { | ||
| promptID = string(value) | ||
| } | ||
| if strings.ToLower(string(header)) == "x-bf-maxim-prompt-version-id" { | ||
| promptVersionID = string(value) | ||
| } | ||
|
|
||
| if promptID != "" && promptVersionID != "" { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if promptVersionID == "" || promptID == "" { | ||
| return headers, body, nil | ||
| } | ||
|
|
||
| promptVersion, err := plugin.mx.GetPromptVersion(promptVersionID, promptID) |
There was a problem hiding this comment.
Initialize body map to prevent nil map assignment panic
TransportInterceptor writes to body without ensuring it’s initialized.
Add a nil check early:
func (plugin *Plugin) TransportInterceptor(url string, headers map[string]string, body map[string]any) (map[string]string, map[string]any, error) {
+ if body == nil {
+ body = make(map[string]any)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check if the url is for chat completion | |
| if !strings.Contains(url, "/chat/completions") { | |
| return headers, body, nil | |
| } | |
| // Check if headers have `x-maxim-prompt-id` and `x-maxim-prompt-version-id` | |
| var promptID string | |
| var promptVersionID string | |
| for header, value := range headers { | |
| if strings.ToLower(string(header)) == "x-bf-maxim-prompt-id" { | |
| promptID = string(value) | |
| } | |
| if strings.ToLower(string(header)) == "x-bf-maxim-prompt-version-id" { | |
| promptVersionID = string(value) | |
| } | |
| if promptID != "" && promptVersionID != "" { | |
| break | |
| } | |
| } | |
| if promptVersionID == "" || promptID == "" { | |
| return headers, body, nil | |
| } | |
| promptVersion, err := plugin.mx.GetPromptVersion(promptVersionID, promptID) | |
| func (plugin *Plugin) TransportInterceptor(url string, headers map[string]string, body map[string]any) (map[string]string, map[string]any, error) { | |
| if body == nil { | |
| body = make(map[string]any) | |
| } | |
| // Check if the url is for chat completion | |
| if !strings.Contains(url, "/chat/completions") { | |
| return headers, body, nil | |
| } | |
| // Check if headers have `x-maxim-prompt-id` and `x-maxim-prompt-version-id` | |
| var promptID string | |
| var promptVersionID string | |
| for header, value := range headers { | |
| if strings.ToLower(string(header)) == "x-bf-maxim-prompt-id" { | |
| promptID = string(value) | |
| } | |
| if strings.ToLower(string(header)) == "x-bf-maxim-prompt-version-id" { | |
| promptVersionID = string(value) | |
| } | |
| if promptID != "" && promptVersionID != "" { | |
| break | |
| } | |
| } | |
| if promptVersionID == "" || promptID == "" { | |
| return headers, body, nil | |
| } | |
| promptVersion, err := plugin.mx.GetPromptVersion(promptVersionID, promptID) | |
| // … | |
| } |
🤖 Prompt for AI Agents
In plugins/maxim/main.go around lines 125 to 151, TransportInterceptor may write
into the body map before ensuring it's initialized which can panic; add an early
nil check and initialize body (e.g., if body == nil { body =
make(map[string]interface{}) } or use map[string]any) before any assignments so
subsequent writes are safe, then continue with the existing header checks and
prompt lookup.
| messages := make([]apis.ChoiceMessage, 0, len(promptVersion.Config.Messages)) | ||
|
|
||
| lastResultPayloadIndex := -1 | ||
| for i := len(promptVersion.Config.Messages) - 1; i >= 0; i-- { | ||
| if promptVersion.Config.Messages[i].Payload.ResultPayload != nil { | ||
| lastResultPayloadIndex = i | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if lastResultPayloadIndex != -1 { | ||
| lastResultPayload := promptVersion.Config.Messages[lastResultPayloadIndex].Payload.ResultPayload | ||
| // Append all the input messages from the last result payload | ||
| messages = append(messages, lastResultPayload.Trace.Input.Messages...) | ||
|
|
||
| // Append all the choices from the last result payload | ||
| choices := lastResultPayload.Trace.Output.Choices | ||
| for _, choice := range choices { | ||
| messages = append(messages, choice.Message) | ||
| } | ||
|
|
||
| // Append all messages after the last result payload | ||
| for i := lastResultPayloadIndex + 1; i < len(promptVersion.Config.Messages); i++ { | ||
| msg := promptVersion.Config.Messages[i] | ||
| if msg.Payload.RequestPayload != nil { | ||
| messages = append(messages, *msg.Payload.RequestPayload) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| body["messages"] = messages | ||
|
|
There was a problem hiding this comment.
Handle prompts with no ResultPayload; avoid emitting empty messages
If the prompt has no ResultPayload, messages remains empty, likely breaking the downstream request. Fallback to collecting RequestPayload messages.
- if lastResultPayloadIndex != -1 {
+ if lastResultPayloadIndex != -1 {
// existing logic ...
- }
+ } else {
+ // No previous result; collect all request payload messages
+ for i := 0; i < len(promptVersion.Config.Messages); i++ {
+ msg := promptVersion.Config.Messages[i]
+ if msg.Payload.RequestPayload != nil {
+ messages = append(messages, *msg.Payload.RequestPayload)
+ }
+ }
+ }
body["messages"] = messages📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| messages := make([]apis.ChoiceMessage, 0, len(promptVersion.Config.Messages)) | |
| lastResultPayloadIndex := -1 | |
| for i := len(promptVersion.Config.Messages) - 1; i >= 0; i-- { | |
| if promptVersion.Config.Messages[i].Payload.ResultPayload != nil { | |
| lastResultPayloadIndex = i | |
| break | |
| } | |
| } | |
| if lastResultPayloadIndex != -1 { | |
| lastResultPayload := promptVersion.Config.Messages[lastResultPayloadIndex].Payload.ResultPayload | |
| // Append all the input messages from the last result payload | |
| messages = append(messages, lastResultPayload.Trace.Input.Messages...) | |
| // Append all the choices from the last result payload | |
| choices := lastResultPayload.Trace.Output.Choices | |
| for _, choice := range choices { | |
| messages = append(messages, choice.Message) | |
| } | |
| // Append all messages after the last result payload | |
| for i := lastResultPayloadIndex + 1; i < len(promptVersion.Config.Messages); i++ { | |
| msg := promptVersion.Config.Messages[i] | |
| if msg.Payload.RequestPayload != nil { | |
| messages = append(messages, *msg.Payload.RequestPayload) | |
| } | |
| } | |
| } | |
| body["messages"] = messages | |
| messages := make([]apis.ChoiceMessage, 0, len(promptVersion.Config.Messages)) | |
| lastResultPayloadIndex := -1 | |
| for i := len(promptVersion.Config.Messages) - 1; i >= 0; i-- { | |
| if promptVersion.Config.Messages[i].Payload.ResultPayload != nil { | |
| lastResultPayloadIndex = i | |
| break | |
| } | |
| } | |
| if lastResultPayloadIndex != -1 { | |
| lastResultPayload := promptVersion.Config.Messages[lastResultPayloadIndex].Payload.ResultPayload | |
| // Append all the input messages from the last result payload | |
| messages = append(messages, lastResultPayload.Trace.Input.Messages...) | |
| // Append all the choices from the last result payload | |
| choices := lastResultPayload.Trace.Output.Choices | |
| for _, choice := range choices { | |
| messages = append(messages, choice.Message) | |
| } | |
| // Append all messages after the last result payload | |
| for i := lastResultPayloadIndex + 1; i < len(promptVersion.Config.Messages); i++ { | |
| msg := promptVersion.Config.Messages[i] | |
| if msg.Payload.RequestPayload != nil { | |
| messages = append(messages, *msg.Payload.RequestPayload) | |
| } | |
| } | |
| } else { | |
| // No previous result; collect all request payload messages | |
| for i := 0; i < len(promptVersion.Config.Messages); i++ { | |
| msg := promptVersion.Config.Messages[i] | |
| if msg.Payload.RequestPayload != nil { | |
| messages = append(messages, *msg.Payload.RequestPayload) | |
| } | |
| } | |
| } | |
| body["messages"] = messages |
🤖 Prompt for AI Agents
In plugins/maxim/main.go around lines 160 to 191, the current logic only
populates messages from the last ResultPayload and leaves messages empty when no
ResultPayload exists; update the code so that when lastResultPayloadIndex == -1
you iterate over promptVersion.Config.Messages and append any non-nil
Payload.RequestPayload entries to messages (same structure used later), then set
body["messages"] to that collected slice; this ensures downstream requests
receive RequestPayload-based prompts when no ResultPayload is present.
| // If no log repo ID available, skip logging | ||
| if effectiveLogRepoID == "" { | ||
| plugin.logger.Debug("no log repo ID available, skipping pre-hook for plugin %s", plugin.GetName()) | ||
| return req, nil, nil | ||
| } |
There was a problem hiding this comment.
Guard logger usage to avoid panic when not provided
Init accepts a logger but it may be nil. Calling Debug without a nil check can panic.
Apply this guard:
- plugin.logger.Debug("no log repo ID available, skipping pre-hook for plugin %s", plugin.GetName())
+ if plugin.logger != nil {
+ plugin.logger.Debug("no log repo ID available, skipping pre-hook for plugin %s", plugin.GetName())
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // If no log repo ID available, skip logging | |
| if effectiveLogRepoID == "" { | |
| plugin.logger.Debug("no log repo ID available, skipping pre-hook for plugin %s", plugin.GetName()) | |
| return req, nil, nil | |
| } | |
| // If no log repo ID available, skip logging | |
| if effectiveLogRepoID == "" { | |
| if plugin.logger != nil { | |
| plugin.logger.Debug("no log repo ID available, skipping pre-hook for plugin %s", plugin.GetName()) | |
| } | |
| return req, nil, nil | |
| } |
🤖 Prompt for AI Agents
In plugins/maxim/main.go around lines 300 to 304, the call
plugin.logger.Debug(...) assumes logger is non-nil and can panic if Init
supplied a nil logger; add a nil guard before using the logger (e.g., if
plugin.logger != nil { plugin.logger.Debug(...) }) or use a safe logging helper
that checks for nil, ensuring the debug call is only invoked when logger is
present; keep the existing early return behavior unchanged.
| // If both governance and maxim plugins are not loaded, skip interception | ||
| hasGovernance := false | ||
| hasMaxim := false | ||
| for _, p := range plugins { | ||
| if p.GetName() == governance.PluginName { | ||
| hasGovernance = true | ||
| break | ||
| } | ||
| if p.GetName() == maxim.PluginName { | ||
| hasMaxim = true | ||
| break | ||
| } | ||
| } | ||
| if !hasGovernance { | ||
| if !hasGovernance && !hasMaxim { | ||
| next(ctx) | ||
| return | ||
| } |
There was a problem hiding this comment.
Fix plugin detection logic - break statements prevent checking all plugins.
The current implementation has a critical bug where break statements at lines 57 and 61 cause the loop to exit after finding the first matching plugin. This means if the governance plugin is found first, hasMaxim will never be set to true, and vice versa.
Apply this diff to fix the logic:
- // If both governance and maxim plugins are not loaded, skip interception
hasGovernance := false
hasMaxim := false
for _, p := range plugins {
if p.GetName() == governance.PluginName {
hasGovernance = true
- break
}
if p.GetName() == maxim.PluginName {
hasMaxim = true
- break
}
}
+ // If both governance and maxim plugins are not loaded, skip interception
if !hasGovernance && !hasMaxim {
next(ctx)
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // If both governance and maxim plugins are not loaded, skip interception | |
| hasGovernance := false | |
| hasMaxim := false | |
| for _, p := range plugins { | |
| if p.GetName() == governance.PluginName { | |
| hasGovernance = true | |
| break | |
| } | |
| if p.GetName() == maxim.PluginName { | |
| hasMaxim = true | |
| break | |
| } | |
| } | |
| if !hasGovernance { | |
| if !hasGovernance && !hasMaxim { | |
| next(ctx) | |
| return | |
| } | |
| hasGovernance := false | |
| hasMaxim := false | |
| for _, p := range plugins { | |
| if p.GetName() == governance.PluginName { | |
| hasGovernance = true | |
| } | |
| if p.GetName() == maxim.PluginName { | |
| hasMaxim = true | |
| } | |
| } | |
| // If both governance and maxim plugins are not loaded, skip interception | |
| if !hasGovernance && !hasMaxim { | |
| next(ctx) | |
| return | |
| } |
🤖 Prompt for AI Agents
In transports/bifrost-http/handlers/middlewares.go around lines 51 to 67, the
plugin-detection loop exits too early because the break statements stop scanning
after the first match so the other plugin flag can remain false; modify the loop
to check every plugin (remove the individual break statements) and set
hasGovernance and hasMaxim independently, or alternatively break only when both
flags are true (i.e., continue looping until both are true), so that both
plugins are correctly detected before the subsequent if-check.
| output: "export", | ||
| trailingSlash: true, | ||
| skipTrailingSlashRedirect: true, | ||
| productionBrowserSourceMaps: true, |
There was a problem hiding this comment.
Review production source map exposure.
Enabling productionBrowserSourceMaps: true exposes your source code to end users via browser dev tools. This can reveal implementation details, business logic, and potentially sensitive information.
Consider whether this is necessary for your production deployment. If debugging production issues, consider:
- Using error tracking services (e.g., Sentry) that upload source maps server-side
- Enabling source maps only for staging environments
- Restricting source map access via server configuration
🤖 Prompt for AI Agents
In ui/next.config.ts around line 11, productionBrowserSourceMaps is currently
set to true which exposes source maps in production; change this to be false for
production and enable it conditionally via an environment variable (e.g.,
process.env.ENABLE_PROD_SOURCEMAPS === 'true') or by checking NODE_ENV so only
staging/dev get true; alternatively remove the flag in production and integrate
a server-side source map strategy (upload maps to Sentry or restrict map access
via server config) so source maps are not publicly served in production.
| webpack: (config, { isServer }) => { | ||
| if (!isServer) { | ||
| config.optimization.minimize = false; | ||
| } |
There was a problem hiding this comment.
Disabling minimization increases bundle size significantly.
Setting config.optimization.minimize = false for client builds prevents code minification, which can:
- Increase bundle size by 3-5x
- Slow down page load times
- Expose unobfuscated code to users
This setting is typically reserved for debugging. For production deployments, consider:
- Enabling minimization with source maps for better debugging without bundle size penalties
- Using environment-specific configuration to disable minimization only in development
- If debugging is needed, use browser dev tools with source maps instead
Apply this diff if minimization should remain enabled in production:
- webpack: (config, { isServer }) => {
- if (!isServer) {
- config.optimization.minimize = false;
- }
+ webpack: (config, { isServer }) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| webpack: (config, { isServer }) => { | |
| if (!isServer) { | |
| config.optimization.minimize = false; | |
| } | |
| webpack: (config, { isServer }) => { |
🤖 Prompt for AI Agents
In ui/next.config.ts around lines 28 to 31, the client-side webpack config
force-disables minimization (config.optimization.minimize = false) which bloats
production bundles; revert this for production and only disable minimization in
development. Change the logic to only set minimize = false when
process.env.NODE_ENV === 'development' (or use an isDev flag) and ensure
production builds keep minimization enabled (or simply omit overriding minimize
for client builds), optionally enabling source maps for prod debugging.
Pratham-Mishra04
force-pushed
the
10-09-feat_maxim_prompt_version_support_added_new
branch
from
676c025 to
a1155a5
Compare
Pratham-Mishra04
force-pushed
the
10-09-feat_transport_interceptor_method_added_to_plugin_schema_new
branch
from
ed9cd2d to
a570d84
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/handlers/middlewares.go (1)
81-91: Don’t write an empty{}body when original request had no body.Currently, requests without a body get a
{}body even if plugins make no changes. This can alter semantics and break upstream handlers.Apply these diffs to only write the body when present or modified:
- requestBody := make(map[string]any) - bodyBytes := ctx.Request.Body() - if len(bodyBytes) > 0 { + requestBody := make(map[string]any) + bodyBytes := ctx.Request.Body() + bodyPresent := len(bodyBytes) > 0 + if bodyPresent { if err := json.Unmarshal(bodyBytes, &requestBody); err != nil { // If body is not valid JSON, log warning and continue without interception logger.Warn(fmt.Sprintf("TransportInterceptor: Failed to unmarshal request body: %v", err)) next(ctx) return } }- // Call TransportInterceptor on all plugins + // Call TransportInterceptor on all plugins + bodyModified := false for _, plugin := range plugins { modifiedHeaders, modifiedBody, err := plugin.TransportInterceptor(string(ctx.Request.URI().RequestURI()), headers, requestBody) if err != nil { logger.Warn(fmt.Sprintf("TransportInterceptor: Plugin '%s' returned error: %v", plugin.GetName(), err)) // Continue with unmodified headers/body continue } // Update headers and body with modifications if modifiedHeaders != nil { headers = modifiedHeaders } if modifiedBody != nil { requestBody = modifiedBody + bodyModified = true } }- // Marshal the body back to JSON - updatedBody, err := json.Marshal(requestBody) - if err != nil { - SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("TransportInterceptor: Failed to marshal request body: %v", err), logger) - return - } - ctx.Request.SetBody(updatedBody) + // Marshal body only if present originally or modified by a plugin + if bodyPresent || bodyModified { + updatedBody, err := json.Marshal(requestBody) + if err != nil { + SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("TransportInterceptor: Failed to marshal request body: %v", err), logger) + return + } + ctx.Request.SetBody(updatedBody) + }Also applies to: 92-107, 109-116
♻️ Duplicate comments (3)
transports/bifrost-http/handlers/middlewares.go (1)
55-62: Scan all plugins (or break only when both flags are true).Current breaks stop scanning after first match, which is unnecessary and can confuse readers. Prefer scanning all, or break only when both flags are set.
Apply this diff to simplify and short‑circuit efficiently:
- for _, p := range plugins { - if p.GetName() == governance.PluginName { - hasGovernance = true - break - } - if p.GetName() == maxim.PluginName { - hasMaxim = true - break - } - } + for _, p := range plugins { + switch p.GetName() { + case governance.PluginName: + hasGovernance = true + case maxim.PluginName: + hasMaxim = true + } + if hasGovernance && hasMaxim { + break + } + }ui/next.config.ts (2)
11-11: Avoid exposing production source maps by default.Serve prod source maps only when explicitly enabled (e.g., via env).
Apply:
- productionBrowserSourceMaps: true, + productionBrowserSourceMaps: process.env.ENABLE_PROD_SOURCEMAPS === "true",
28-31: Re-enable client minimization in production.Disabling minification bloats bundles and slows loads. Limit to dev.
Apply:
- webpack: (config, { isServer }) => { - if (!isServer) { - config.optimization.minimize = false; - } + webpack: (config, { isServer }) => { + if (!isServer && process.env.NODE_ENV === "development") { + config.optimization.minimize = false; + }
🧹 Nitpick comments (2)
plugins/otel/docker-compose.yml (2)
67-67: Unsigned plugins env set but empty — likely no-opGF_PLUGINS_ALLOW_LOADING_UNSIGNED_PLUGINS is set to an empty string, which won’t allow any unsigned plugins to load. If you intended to enable specific unsigned plugins, list their IDs or make it configurable.
Example options:
- Make it configurable:
- GF_PLUGINS_ALLOW_LOADING_UNSIGNED_PLUGINS: "" + GF_PLUGINS_ALLOW_LOADING_UNSIGNED_PLUGINS: "${GF_PLUGINS_ALLOW_LOADING_UNSIGNED_PLUGINS:-}"
- Or hardcode specific plugin IDs (comma‑separated), e.g.:
- GF_PLUGINS_ALLOW_LOADING_UNSIGNED_PLUGINS: "" + GF_PLUGINS_ALLOW_LOADING_UNSIGNED_PLUGINS: "your-plugin-id-1,your-plugin-id-2"
196-204: Simplified datasources OK; consider adding minimal Tempo-Prometheus linking (optional)Works as-is. If you want trace↔metric linking and better UX in Grafana, add jsonData to Tempo and Prometheus references. Optional, can defer.
Example (inside grafana-datasources content):
- name: Tempo type: tempo access: proxy url: http://tempo:3200 jsonData: httpMethod: GET tracesToMetrics: datasourceUid: prometheus tags: [{ key: service.name, value: service }]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
plugins/maxim/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
plugins/maxim/go.mod(1 hunks)plugins/maxim/main.go(6 hunks)plugins/maxim/plugin_test.go(2 hunks)plugins/otel/docker-compose.yml(2 hunks)transports/bifrost-http/handlers/middlewares.go(2 hunks)transports/bifrost-http/handlers/server.go(1 hunks)transports/go.mod(1 hunks)ui/app/observability/views/plugins/maximView.tsx(1 hunks)ui/next.config.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- plugins/maxim/go.mod
- transports/go.mod
- plugins/maxim/main.go
- transports/bifrost-http/handlers/server.go
- plugins/maxim/plugin_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
transports/bifrost-http/handlers/middlewares.go (2)
plugins/maxim/main.go (1)
PluginName(21-21)plugins/governance/main.go (1)
PluginName(19-19)
⏰ 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). (2)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (2)
plugins/otel/docker-compose.yml (1)
74-74: Switch to .yaml target looks goodGrafana provisioning supports both .yml and .yaml. Target path change is fine.
ui/app/observability/views/plugins/maximView.tsx (1)
43-43: LGTM — header name updated correctly.Matches backend rename to x-bf-maxim-log-repo-id.
| - job_name: "bifrost" | ||
| static_configs: | ||
| - targets: ["host.docker.internal:8080"] | ||
| metrics_path: "/metrics" | ||
There was a problem hiding this comment.
host.docker.internal target may fail on Linux; add host-gateway mapping
Prometheus scraping host.docker.internal:8080 can fail on Linux unless the special hostname is mapped. Add extra_hosts to the prometheus service to ensure portability.
Apply outside the selected lines in the prometheus service:
services:
prometheus:
image: prom/prometheus:latest
container_name: prometheus
+ extra_hosts:
+ - "host.docker.internal:host-gateway"Alternatively, document that Docker Engine 20.10+ with host-gateway is required, or expose the target as an env var to avoid hardcoding host.docker.internal.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - job_name: "bifrost" | |
| static_configs: | |
| - targets: ["host.docker.internal:8080"] | |
| metrics_path: "/metrics" | |
| services: | |
| prometheus: | |
| image: prom/prometheus:latest | |
| container_name: prometheus | |
| extra_hosts: | |
| - "host.docker.internal:host-gateway" |
🤖 Prompt for AI Agents
In plugins/otel/docker-compose.yml around lines 187-191, the Prometheus job
scrapes host.docker.internal which can fail on Linux; update the prometheus
service (outside the listed job config) to add extra_hosts mapping such as
"host.docker.internal:host-gateway" so the hostname resolves inside containers
(requires Docker Engine 20.10+), or alternatively expose the scrape target via
an environment variable and reference that in the Prometheus config or add a
note in README documenting the host-gateway requirement.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
transports/bifrost-http/handlers/middlewares.go (1)
51-67: Critical: Remove break statements to check all plugins.The
breakstatements at lines 57 and 61 cause the loop to exit after finding the first matching plugin. This means:
- If the governance plugin appears first in the slice,
hasMaximwill never be set to true- Vice versa if maxim appears first
Apply this diff to fix the logic:
- // If both governance and maxim plugins are not loaded, skip interception hasGovernance := false hasMaxim := false for _, p := range plugins { if p.GetName() == governance.PluginName { hasGovernance = true - break } if p.GetName() == maxim.PluginName { hasMaxim = true - break } } + // If both governance and maxim plugins are not loaded, skip interception if !hasGovernance && !hasMaxim { next(ctx) return }plugins/maxim/main.go (2)
123-217: Multiple issues in TransportInterceptor implementation.This new interceptor has several concerns:
Body map safety: The function writes to the
bodymap (lines 157, 190, etc.) without checking if it's nil. While the middleware should initialize it, defensive programming suggests adding a nil check.Empty messages when no ResultPayload: Lines 160-188 only populate
messageswhenlastResultPayloadIndex != -1. If no ResultPayload exists,messagesremains empty and is assigned tobody["messages"]at line 190, potentially breaking downstream requests.Apply these fixes:
func (plugin *Plugin) TransportInterceptor(url string, headers map[string]string, body map[string]any) (map[string]string, map[string]any, error) { + // Ensure body is initialized + if body == nil { + body = make(map[string]any) + } + // Check if the url is for chat completion if !strings.Contains(url, "/chat/completions") { return headers, body, nil }if lastResultPayloadIndex != -1 { // existing logic ... + } else { + // No previous result; collect all request payload messages + for i := 0; i < len(promptVersion.Config.Messages); i++ { + msg := promptVersion.Config.Messages[i] + if msg.Payload.RequestPayload != nil { + messages = append(messages, *msg.Payload.RequestPayload) + } + } } body["messages"] = messages
300-304: Guard logger usage to prevent nil pointer panic.The logger may be nil if not provided during Init. Calling
Debugwithout a nil check will panic.Apply this guard:
// If no log repo ID available, skip logging if effectiveLogRepoID == "" { - plugin.logger.Debug("no log repo ID available, skipping pre-hook for plugin %s", plugin.GetName()) + if plugin.logger != nil { + plugin.logger.Debug("no log repo ID available, skipping pre-hook for plugin %s", plugin.GetName()) + } return req, nil, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
plugins/maxim/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
plugins/maxim/go.mod(1 hunks)plugins/maxim/main.go(6 hunks)plugins/maxim/plugin_test.go(2 hunks)plugins/otel/docker-compose.yml(2 hunks)transports/bifrost-http/handlers/middlewares.go(2 hunks)transports/bifrost-http/handlers/server.go(1 hunks)transports/go.mod(1 hunks)ui/app/observability/views/plugins/maximView.tsx(1 hunks)ui/next.config.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/maxim/go.mod
- ui/next.config.ts
- transports/go.mod
🧰 Additional context used
🧬 Code graph analysis (4)
transports/bifrost-http/handlers/server.go (1)
plugins/maxim/main.go (1)
Init(39-68)
transports/bifrost-http/handlers/middlewares.go (2)
plugins/maxim/main.go (1)
PluginName(21-21)plugins/governance/main.go (1)
PluginName(19-19)
plugins/maxim/plugin_test.go (2)
core/logger.go (1)
NewDefaultLogger(40-49)plugins/maxim/main.go (1)
Init(39-68)
plugins/maxim/main.go (3)
core/schemas/logger.go (1)
Logger(28-55)core/schemas/plugin.go (1)
Plugin(45-71)core/schemas/chatcompletions.go (1)
ChatContentBlockTypeText(272-272)
⏰ 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). (2)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (7)
ui/app/observability/views/plugins/maximView.tsx (1)
43-43: LGTM! Header name is more explicit and properly namespaced.The updated header name
x-bf-maxim-log-repo-idclearly indicates it's Maxim-specific, improving clarity for users.plugins/otel/docker-compose.yml (1)
187-204: Approve datasource simplification and bifrost job addition.The changes look good:
- New bifrost scrape job correctly targets
host.docker.internal:8080with/metricspath- Simplified datasources configuration uses clean YAML format
- Removed extensive Tempo jsonData appears intentional for streamlined setup
plugins/maxim/plugin_test.go (2)
35-35: LGTM! Test correctly updated to pass logger to Init.The test properly initializes a debug logger and passes it to the
Initfunction, aligning with the updated plugin signature.
232-232: LGTM! Error case test correctly updated.The test properly passes the logger parameter when expecting an error from
Init.transports/bifrost-http/handlers/server.go (1)
214-214: LGTM! Server correctly passes logger to Maxim plugin.The update properly passes the logger to the Maxim plugin initialization, enabling debug logging within the plugin.
plugins/maxim/main.go (2)
39-56: LGTM! Init signature correctly updated to accept logger.The function properly validates the config and stores the logger in the Plugin struct.
388-409: LGTM! Content nil handling is robust.The code properly checks for nil content and content blocks, using a placeholder "-" when content is missing. This prevents nil pointer dereferences.
| GF_SECURITY_ADMIN_PASSWORD: admin | ||
| GF_AUTH_ANONYMOUS_ENABLED: "true" | ||
| GF_AUTH_ANONYMOUS_ORG_ROLE: Viewer | ||
| GF_PLUGINS_ALLOW_LOADING_UNSIGNED_PLUGINS: "" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify empty string value for unsigned plugins.
The environment variable GF_PLUGINS_ALLOW_LOADING_UNSIGNED_PLUGINS is set to an empty string. Typically, this variable expects a comma-separated list of plugin IDs to allow, or should be omitted entirely if no unsigned plugins are needed.
Please confirm whether:
- This empty value is intentional to allow all unsigned plugins
- Specific plugin IDs should be listed
- The variable should be removed if unsigned plugins aren't needed
🏁 Script executed:
#!/bin/bash
# Check Grafana documentation for this environment variable
fd -t f -e md -e txt READMELength of output: 145
🌐 Web query:
What values are supported by GF_PLUGINS_ALLOW_LOADING_UNSIGNED_PLUGINS in Grafana's environment variable configuration, and what does an empty string signify?
💡 Result:
-
Supported values: a comma-separated list of plugin identifiers (plugin IDs) to allow loading even if unsigned (e.g. my-plugin,another-plugin). [1][2]
-
Empty string (or omitting the variable) means “no unsigned plugins allowed” — Grafana will not load any unsigned plugins. [1][2]
GF_PLUGINS_ALLOW_LOADING_UNSIGNED_PLUGINS set to empty disables all unsigned plugins
An empty value (or omitting this variable) means Grafana will not load any unsigned plugins. Remove this entry if you don’t use unsigned plugins, or set it to a comma-separated list of plugin IDs you need to allow.
🤖 Prompt for AI Agents
In plugins/otel/docker-compose.yml around line 67,
GF_PLUGINS_ALLOW_LOADING_UNSIGNED_PLUGINS is set to an empty string which
effectively disables all unsigned plugins; either remove this environment
variable entirely if you don't intend to allow unsigned plugins, or replace the
empty value with a comma-separated list of the specific plugin IDs you need to
permit (e.g., pluginA,pluginB) so Grafana will load only the intended unsigned
plugins.
akshaydeo
force-pushed
the
10-09-feat_transport_interceptor_method_added_to_plugin_schema_new
branch
4 times, most recently
from
c67b125 to
701d610
Compare
akshaydeo
changed the base branch from
10-09-feat_transport_interceptor_method_added_to_plugin_schema_new
to
graphite-base/594
Summary
Update the Maxim plugin to support prompt population from Maxim's API using prompt ID and version ID headers, and fix various issues with the plugin's implementation.
Changes
maxim-godependency from v0.1.11 to v0.1.13TransportInterceptorto populate request bodies for chat completion requests using Maxim prompt IDsx-bf-log-repo-idtox-bf-maxim-log-repo-idInitfunction to enable debug loggingType of change
Affected areas
How to test
x-bf-maxim-prompt-idandx-bf-maxim-prompt-version-idBreaking changes
Related issues
N/A
Security considerations
The plugin now retrieves prompt configurations from Maxim's API using the provided API key.
Checklist