enhancement: configuration updates in maxim plugin#471
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughReplace Maxim plugin constructor with a config-based Init enabling per-repo loggers and header/context repo selection; update PreHook/PostHook for per-repo traces; transport stores/excludes Maxim repo-id header; UI adds Maxim toggle and debounced config; docs updated for Maxim-based observability and Go Init now requires context; README removed and tests updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Admin/UI
participant Startup as Bifrost Startup
participant Maxim as Maxim Plugin
participant Cfg as Plugin Config
rect rgba(200,230,255,0.3)
note over Startup,Cfg: Config-based plugin init
UI->>Startup: Save plugin config (api_key, log_repo_id)
Startup->>Cfg: Load plugin.Config (JSON)
Startup->>Maxim: Init(Config)
Maxim-->>Startup: Plugin instance ("maxim")
end
sequenceDiagram
autonumber
participant Client as Client
participant HTTP as Bifrost HTTP Transport
participant Ctx as Context
participant Maxim as Maxim Plugin
participant Repo as Repo Logger
participant MX as Maxim SDK
rect rgba(220,255,220,0.35)
note over Client,HTTP: Incoming request
Client->>HTTP: HTTP request (+ optional x-bf-maxim-log-repo-id)
HTTP->>Ctx: Store repo-id in context (skip as tag)
end
rect rgba(255,245,200,0.5)
note over Maxim: PreHook
HTTP->>Maxim: PreHook(ctx, request)
Maxim->>Maxim: Determine repo (header > default > skip)
alt repo selected
Maxim->>Repo: getOrCreateLogger(repo-id)
Repo->>MX: start trace/generation
else no repo
Maxim-->>HTTP: Skip logging
end
end
rect rgba(255,230,230,0.5)
note over Maxim: PostHook
HTTP->>Maxim: PostHook(ctx, response/error)
alt repo in use
Repo->>MX: end generation/trace
Maxim->>Repo: flush
else
Maxim-->>HTTP: No-op
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
TejasGhatte
changed the base branch from
09-13-enhancement_forward_tags_in_maxim_logs
to
graphite-base/471
TejasGhatte
force-pushed
the
graphite-base/471
branch
from
c2ccac0 to
26a4cfa
Compare
TejasGhatte
force-pushed
the
09-13-enhancement_configuration_updates_in_maxim_plugin
branch
from
39c3b6b to
0be13b3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (11)
ui/lib/types/config.ts (2)
216-216: Avoid dual sources of truth for enabling Maxim.If plugin enablement is controlled via the Plugins API (enabled flag per plugin), consider removing CoreConfig.enable_maxim to prevent config drift.
239-244: MaximConfig matches Go JSON tags; optional default helper.Type shape looks correct and consistent with
json:"api_key"andjson:"log_repo_id". Consider exporting a DefaultMaximConfig here for reuse in the UI.ui/app/config/views/pluginsForm.tsx (1)
502-512: Replace cache-related notes with Maxim-specific guidance.These notes are for semantic cache, not Maxim. Suggest mentioning the multi-repo header usage.
Apply this diff:
-<ul className="text-muted-foreground list-inside list-disc text-xs"> - <li> - You can pass <b>x-bf-cache-ttl</b> header with requests to use request-specific TTL. - </li> - <li> - You can pass <b>x-bf-cache-threshold</b> header with requests to use request-specific similarity threshold. - </li> -</ul> +<ul className="text-muted-foreground list-inside list-disc text-xs"> + <li> + To log to multiple repositories for a single request, send one or more <b>x-bf-maxim-log-repo-id</b> headers. + </li> + <li> + If no header is provided, the default <b>log_repo_id</b> from configuration will be used (if set). + </li> +</ul>plugins/maxim/main.go (3)
29-62: Minor: error text should match JSON field name.Current message says "apiKey is not set". Prefer "api_key is not set" for consistency with config.
Apply this diff:
- if config.ApiKey == "" { - return nil, fmt.Errorf("apiKey is not set") - } + if config.ApiKey == "" { + return nil, fmt.Errorf("api_key is not set") + }
451-457: Reduce lock hold time during Flush.Hold the map read lock only to snapshot pointers, then flush outside to avoid blocking writers during potentially slow I/O.
Apply this diff:
- // Flush all loggers - plugin.loggerMutex.RLock() - for _, logger := range plugin.loggers { - logger.Flush() - } - plugin.loggerMutex.RUnlock() + // Flush all loggers without holding the lock during I/O + plugin.loggerMutex.RLock() + loggers := make([]*logging.Logger, 0, len(plugin.loggers)) + for _, l := range plugin.loggers { + loggers = append(loggers, l) + } + plugin.loggerMutex.RUnlock() + for _, logger := range loggers { + logger.Flush() + }Repeat the same pattern in Cleanup().
21-27: Go naming nit: prefer ID over Id.Consider
LogRepoIDfor the exported field (JSON tag remainslog_repo_id). This aligns with Go’s initialism conventions.transports/go.mod (1)
117-117: Local replace is fine for the monorepo; avoid shipping it in releases.Keep this replace for local dev, but ensure it’s removed (or conditioned) before tagging/publishing to avoid breaking external consumers’ module resolution.
Suggested annotation:
+// Local development only: use in-repo Maxim plugin. Remove before release. replace github.com/maximhq/bifrost/plugins/maxim => ../plugins/maximVerification: confirm CI/release jobs either strip this line or build from the monorepo root where the relative path exists.
transports/bifrost-http/lib/ctx.go (2)
118-121: Collecting multiple x-bf-maxim-log-repo-id headers: trim and de‑dupe values.Prevents accidental whitespace issues and header duplication from bloating context.
Apply:
- if labelName == "log-repo-id" { - // Collect all log repo IDs in a slice to support multiple repos per request - maximLogRepoIDs = append(maximLogRepoIDs, string(value)) - } + if labelName == "log-repo-id" { + // Collect all log repo IDs; trim and de-duplicate + v := strings.TrimSpace(string(value)) + if v != "" { + // lazy-init a small set alongside the slice for O(1) de-dupe + if seen == nil { + seen = make(map[string]struct{}, 4) + } + if _, ok := seen[v]; !ok { + maximLogRepoIDs = append(maximLogRepoIDs, v) + seen[v] = struct{}{} + } + } + }Add near the slice declaration:
- // Initialize slice for collecting log repo IDs - var maximLogRepoIDs []string + // Initialize slice and set for collecting log repo IDs + var maximLogRepoIDs []string + var seen map[string]struct{}
207-210: Context write LGTM; consider a soft cap to avoid header abuse.Optionally clamp to a small max (e.g., 10) to prevent pathological header spam from impacting memory.
Example:
- if len(maximLogRepoIDs) > 0 { + if n := len(maximLogRepoIDs); n > 0 { + if n > 10 { + maximLogRepoIDs = maximLogRepoIDs[:10] + } bifrostCtx = context.WithValue(bifrostCtx, maxim.ContextKey(maxim.LogRepoIDsKey), maximLogRepoIDs) }transports/bifrost-http/main.go (2)
420-429: Config decoding via JSON round‑trip works; consider mapstructure for fewer allocations and better errors.Keeps parity with semantic cache path, but mapstructure.Decode would avoid marshal/unmarshal and give field‑path errors.
If you decide to switch later:
import "github.com/mitchellh/mapstructure" var maximConfig maxim.Config if plugin.Config != nil { if err := mapstructure.Decode(plugin.Config, &maximConfig); err != nil { logger.Fatal("failed to decode maxim config: %v", err) } }
431-436: Init(Config) path LGTM; improve warning to guide ops.When Init fails due to missing api_key, suggest including a hint to set plugins[].config.api_key or disable the plugin.
- logger.Warn("failed to initialize maxim plugin: %v", err) + logger.Warn("failed to initialize maxim plugin: %v (set plugins[].config.api_key and optional log_repo_id, or disable the plugin)", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
plugins/maxim/README.md(1 hunks)plugins/maxim/main.go(9 hunks)plugins/maxim/plugin_test.go(1 hunks)transports/bifrost-http/lib/ctx.go(3 hunks)transports/bifrost-http/main.go(1 hunks)transports/bifrost-http/tmp-maxim/config.json(1 hunks)transports/go.mod(1 hunks)ui/app/config/views/pluginsForm.tsx(7 hunks)ui/lib/types/config.ts(2 hunks)ui/lib/types/plugins.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
transports/bifrost-http/lib/ctx.go (1)
plugins/maxim/main.go (7)
GenerationIDKey(76-76)TraceIDKey(74-74)SessionIDKey(73-73)TraceNameKey(75-75)GenerationNameKey(77-77)ContextKey(67-67)LogRepoIDsKey(79-79)
plugins/maxim/plugin_test.go (1)
plugins/maxim/main.go (2)
Init(37-62)Config(24-27)
plugins/maxim/main.go (3)
transports/bifrost-http/lib/config.go (1)
Config(113-136)transports/bifrost-http/lib/ctx.go (1)
ContextKey(68-68)core/schemas/bifrost.go (4)
EmbeddingInput(126-128)ModelChatMessageRoleUser(31-31)SpeechInput(131-136)TranscriptionInput(193-199)
transports/bifrost-http/main.go (3)
plugins/maxim/main.go (3)
PluginName(19-19)Config(24-27)Init(37-62)plugins/semanticcache/main.go (3)
PluginName(144-144)Config(25-42)Init(262-319)transports/bifrost-http/lib/config.go (1)
Config(113-136)
ui/app/config/views/pluginsForm.tsx (3)
ui/lib/types/config.ts (3)
MaximConfig(240-243)CacheConfig(224-237)ModelProviderName(12-12)ui/lib/types/plugins.ts (1)
MAXIM_PLUGIN(4-4)ui/components/ui/input.tsx (1)
Input(7-21)
🔇 Additional comments (4)
ui/app/config/views/pluginsForm.tsx (1)
112-135: Toggle handler looks solid.Create/update flow and toasts match existing semantic cache pattern.
plugins/maxim/main.go (2)
18-20: Plugin renamed to "maxim" — good alignment with UI constant.
367-375: Generation on existing trace across new repos — confirm behavior.If a trace ID is provided via context but wasn’t created in a specific repo, does
AddGenerationToTraceauto-create the trace or no-op? If not, we should ensure a trace exists per repo before adding a generation.Would you like me to add a guard that creates a trace per repo when
traceIDexists but the repo has no trace state?transports/bifrost-http/main.go (1)
414-415: Nice touch: explicit debug when a plugin is disabled.Improves startup transparency without noisy logs.
TejasGhatte
force-pushed
the
09-13-enhancement_configuration_updates_in_maxim_plugin
branch
from
0be13b3 to
dd6071d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugins/maxim/main.go (1)
290-308: Don’t log raw Embedding/Speech/Transcription payloads (binary/large/PII).
Current code logs full structs includingTranscriptionInput.File []byte. Replace with metadata-only logging.Apply this diff:
-} else if req.Input.EmbeddingInput != nil { - requestType = "embedding" - messages = append(messages, logging.CompletionRequest{ - Role: string(schemas.ModelChatMessageRoleUser), - Content: req.Input.EmbeddingInput, - }) -} else if req.Input.SpeechInput != nil { - requestType = "speech" - messages = append(messages, logging.CompletionRequest{ - Role: string(schemas.ModelChatMessageRoleUser), - Content: req.Input.SpeechInput, - }) -} else if req.Input.TranscriptionInput != nil { - requestType = "transcription" - messages = append(messages, logging.CompletionRequest{ - Role: string(schemas.ModelChatMessageRoleUser), - Content: req.Input.TranscriptionInput, - }) +} else if req.Input.EmbeddingInput != nil { + requestType = "embedding" + latestMessage = fmt.Sprintf("embedding texts: %d item(s)", len(req.Input.EmbeddingInput.Texts)) + messages = append(messages, logging.CompletionRequest{ + Role: string(schemas.ModelChatMessageRoleUser), + Content: map[string]any{ + "type": "embedding", + "count": len(req.Input.EmbeddingInput.Texts), + }, + }) +} else if req.Input.SpeechInput != nil { + requestType = "speech" + latestMessage = "speech synthesis request" + messages = append(messages, logging.CompletionRequest{ + Role: string(schemas.ModelChatMessageRoleUser), + Content: map[string]any{ + "type": "speech", + "voice": req.Input.SpeechInput.VoiceConfig, + "response_type": req.Input.SpeechInput.ResponseFormat, + }, + }) +} else if req.Input.TranscriptionInput != nil { + requestType = "transcription" + latestMessage = "transcription request" + messages = append(messages, logging.CompletionRequest{ + Role: string(schemas.ModelChatMessageRoleUser), + Content: map[string]any{ + "type": "transcription", + "file_bytes": len(req.Input.TranscriptionInput.File), + "language": req.Input.TranscriptionInput.Language, + "response_type": req.Input.TranscriptionInput.ResponseFormat, + "format": req.Input.TranscriptionInput.Format, + }, + })
🧹 Nitpick comments (4)
plugins/maxim/main.go (4)
37-61: Init flow is reasonable; default logger eager-init is fine.
Minor nit: error text says “apiKey”; consider “ApiKey” for consistency.
116-140: Repo ID merge/dedupe is correct.
Optional: acceptcontext.Context(not pointer) since values are immutable; would simplify call sites.
330-340: Bound trace input preview to avoid giant log entries.
CaplatestMessagebeforeSetInput.Apply this diff:
- trace := logger.Trace(&traceConfig) - trace.SetInput(latestMessage) + trace := logger.Trace(&traceConfig) + inputPreview := latestMessage + if inputPreview != "" { + r := []rune(inputPreview) + if len(r) > 512 { + inputPreview = string(r[:512]) + } + } + trace.SetInput(inputPreview)
411-457: Swallowed logger creation errors; and flushing all loggers each request is heavy.
- At minimum, log/debug the repo ID on failure to help ops triage.
- Flush only the loggers used for this request (not the entire map).
Apply this diff for targeted flush:
- // Flush all loggers - plugin.loggerMutex.RLock() - for _, logger := range plugin.loggers { - logger.Flush() - } - plugin.loggerMutex.RUnlock() + // Flush only the loggers used in this request + plugin.loggerMutex.RLock() + for _, id := range effectiveLogRepoIDs { + if logger, ok := plugin.loggers[id]; ok { + logger.Flush() + } + } + plugin.loggerMutex.RUnlock()Optionally, add a debug log on
getOrCreateLoggererror paths here and in PreHook.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
plugins/maxim/README.md(1 hunks)plugins/maxim/main.go(9 hunks)plugins/maxim/plugin_test.go(1 hunks)transports/bifrost-http/lib/ctx.go(3 hunks)transports/bifrost-http/main.go(1 hunks)transports/go.mod(1 hunks)ui/app/config/views/pluginsForm.tsx(7 hunks)ui/lib/types/config.ts(2 hunks)ui/lib/types/plugins.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- plugins/maxim/README.md
- ui/lib/types/config.ts
- transports/go.mod
- plugins/maxim/plugin_test.go
- ui/lib/types/plugins.ts
- ui/app/config/views/pluginsForm.tsx
- transports/bifrost-http/main.go
- transports/bifrost-http/lib/ctx.go
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/maxim/main.go (4)
transports/bifrost-http/lib/config.go (1)
Config(113-136)ui/lib/types/plugins.ts (1)
Plugin(6-10)transports/bifrost-http/lib/ctx.go (1)
ContextKey(68-68)core/schemas/bifrost.go (4)
EmbeddingInput(126-128)ModelChatMessageRoleUser(31-31)SpeechInput(131-136)TranscriptionInput(193-199)
🔇 Additional comments (8)
plugins/maxim/main.go (8)
18-20: Plugin name change looks good.
Constant is clear and aligned with the UI naming.
21-27: Config struct + JSON tags: LGTM.
Fields match the PR objectives (api_key, log_repo_id).
96-109: Thread-safe logger registry: LGTM.
Map+RWMutex is appropriate; pointer vs value mutex is fine here.
142-169: Double‑checked locking is correct.
No races on the map; good error wrap on create failure.
199-206: Skip when no repos: good guard.
Avoids overhead when not configured.
367-375: Add generation across repos: LGTM.
Shared IDs across repos are acceptable since repos are scoped.
463-469: Cleanup flush: LGTM.
Reasonable finalization.
64-80: Incorrect — context key type mismatch claim is unsupportedplugins/maxim/main.go defines
type ContextKey stringand exports SessionIDKey/TraceIDKey/GenerationIDKey/etc as that type; transports/bifrost-http/lib/ctx.go consistently usesmaxim.ContextKey(...)andstring(maxim.<Key>)when comparing/setting, so keys are compatible. Do not apply the suggested string-constants diff. Locations: plugins/maxim/main.go:67–80; transports/bifrost-http/lib/ctx.go:98–106,209.Likely an incorrect or invalid review comment.
TejasGhatte
force-pushed
the
09-13-enhancement_configuration_updates_in_maxim_plugin
branch
from
dd6071d to
0bcba23
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ui/app/config/views/pluginsForm.tsx (1)
497-503: Fix duplicate input IDs and mask the API key for security.The API Key input has
id="api_key"but it should match the label'shtmlForattribute for proper accessibility. Also, sensitive information like API keys should be masked.Apply this diff to fix the accessibility and security issues:
-<Label htmlFor="api_key">API Key*</Label> +<Label htmlFor="maxim_api_key">API Key*</Label> <Input - id="api_key" + id="maxim_api_key" + type="password" placeholder="your-maxim-api-key" value={maximConfig.api_key} onChange={(e) => debouncedUpdateMaximConfig({ api_key: e.target.value })} />
🧹 Nitpick comments (5)
ui/app/config/views/pluginsForm.tsx (1)
112-134: Remove redundant comment that duplicates the semantic cache toggle.Line 112 has the same comment as line 88, but this is for the Maxim toggle implementation.
Apply this diff to fix the comment:
-// Handle semantic cache toggle (create or update) +// Handle Maxim toggle (create or update)plugins/maxim/main.go (2)
194-200: Consider adding debug logging when skipping due to no log repo.When no log repo ID is available and logging is skipped, it might be helpful to add debug logging for troubleshooting.
// If no log repo ID available, skip logging if effectiveLogRepoID == "" { + if plugin.mx != nil { + logger.Debug("Skipping Maxim logging: no log repo ID available") + } return req, nil, nil }
449-454: Optimize logger flushing by tracking which loggers need flushing.Currently all loggers are flushed on every request completion. Consider tracking which loggers were used in the request to minimize unnecessary flush operations.
Consider maintaining a set of used logger IDs in the context and only flushing those specific loggers to reduce overhead in high-traffic scenarios.
plugins/maxim/plugin_test.go (1)
232-244: Consider mocking the Maxim SDK for complete test coverage.The test skips actual initialization for valid configs. Consider using a mock or test double for the Maxim SDK to achieve better test coverage without requiring real API keys.
Consider introducing an interface for the Maxim SDK initialization to enable dependency injection and proper mocking in tests. This would allow testing the full initialization flow including the default logger creation path.
transports/bifrost-http/main.go (1)
414-414: Consider using a constant for the log message.The debug log message could use a more descriptive format.
-logger.Debug("plugin %s is disabled, skipping initialization", plugin.Name) +logger.Debug("Plugin '%s' is disabled in configuration, skipping initialization", plugin.Name)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
plugins/maxim/README.md(2 hunks)plugins/maxim/main.go(9 hunks)plugins/maxim/plugin_test.go(5 hunks)transports/bifrost-http/lib/ctx.go(1 hunks)transports/bifrost-http/main.go(1 hunks)transports/go.mod(1 hunks)ui/app/config/views/pluginsForm.tsx(9 hunks)ui/lib/types/config.ts(2 hunks)ui/lib/types/plugins.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- transports/go.mod
- ui/lib/types/plugins.ts
- transports/bifrost-http/lib/ctx.go
- plugins/maxim/README.md
- ui/lib/types/config.ts
🧰 Additional context used
🧬 Code graph analysis (4)
plugins/maxim/plugin_test.go (1)
plugins/maxim/main.go (5)
Init(37-62)Config(24-27)Plugin(104-109)LogRepoIDKey(79-79)PluginName(19-19)
ui/app/config/views/pluginsForm.tsx (2)
ui/lib/types/config.ts (3)
MaximConfig(240-243)CacheConfig(224-237)ModelProviderName(12-12)ui/lib/types/plugins.ts (1)
MAXIM_PLUGIN(4-4)
plugins/maxim/main.go (4)
transports/bifrost-http/lib/config.go (1)
Config(113-136)ui/lib/types/plugins.ts (1)
Plugin(6-10)transports/bifrost-http/lib/ctx.go (1)
ContextKey(68-68)core/schemas/bifrost.go (5)
EmbeddingInput(126-128)ModelChatMessageRoleUser(31-31)SpeechInput(131-136)VoiceConfig(143-146)TranscriptionInput(193-199)
transports/bifrost-http/main.go (2)
plugins/maxim/main.go (3)
PluginName(19-19)Config(24-27)Init(37-62)transports/bifrost-http/lib/config.go (1)
Config(113-136)
🔇 Additional comments (7)
ui/app/config/views/pluginsForm.tsx (1)
200-231: LGTM! Proper debouncing implementation with separate timers.The implementation correctly uses separate debounce timers for cache and Maxim configurations, preventing race conditions. The error handling with config reversion and cleanup on unmount are properly implemented.
plugins/maxim/main.go (2)
285-321: LGTM! Proper sanitization of sensitive input data.The implementation correctly sanitizes embedding, speech, and transcription inputs by logging only metadata (counts, sizes, presence flags) instead of raw content. This prevents sensitive data leakage while maintaining useful debugging information.
137-164: Thread-safe logger creation with proper double-check locking.The
getOrCreateLoggerimplementation correctly uses the double-check locking pattern for thread-safe logger creation, minimizing lock contention by using read locks for lookups and write locks only for creation.plugins/maxim/plugin_test.go (3)
32-35: LGTM! Consistent use of MAXIM_LOG_REPO_ID environment variable.The test correctly uses
MAXIM_LOG_REPO_IDwhich aligns with the JSON field namelog_repo_idin the Config struct.
89-90: LGTM! Proper use of t.Fatalf for test failures.The test correctly uses
t.Fatalfinstead oflog.Fatalffor test failures, ensuring proper test runner attribution.Also applies to: 101-102
130-195: Well-structured test coverage for log repo selection logic.The test comprehensively covers all priority scenarios: header precedence, default fallback, and skip-logging cases. The test structure with table-driven tests is clean and maintainable.
transports/bifrost-http/main.go (1)
419-436: LGTM! Config-based plugin initialization with proper error handling.The transition from environment-based to config-based initialization is well implemented. The JSON marshaling/unmarshaling approach provides a clean way to convert the generic config to the typed Maxim config.
TejasGhatte
force-pushed
the
09-13-enhancement_configuration_updates_in_maxim_plugin
branch
from
0bcba23 to
6b057da
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/main.go (1)
187-195: CORS blocks Maxim/semantic-cache custom headers (preflight fails).Allow requested headers dynamically to support x-bf-maxim-, x-bf-cache-, etc. Otherwise browser clients can’t send these headers.
Apply:
- ctx.Response.Header.Set("Access-Control-Allow-Headers", "Content-Type, Authorization, X-Requested-With") + // Reflect requested headers to support custom plugin headers (x-bf-*) + reqHeaders := string(ctx.Request.Header.Peek("Access-Control-Request-Headers")) + allow := "Content-Type, Authorization, X-Requested-With" + if reqHeaders != "" { + allow = allow + ", " + reqHeaders + } + ctx.Response.Header.Set("Access-Control-Allow-Headers", allow) + ctx.Response.Header.Add("Vary", "Origin") + ctx.Response.Header.Add("Vary", "Access-Control-Request-Headers")
♻️ Duplicate comments (1)
ui/app/config/views/pluginsForm.tsx (1)
496-503: Mask API key input and avoid generic id.Use password type and a unique id; discourage autocomplete.
Apply:
-<Label htmlFor="api_key">Maxim API Key</Label> -<Input - id="api_key" - placeholder="your-maxim-api-key" +<Label htmlFor="maxim_api_key">Maxim API Key</Label> +<Input + id="maxim_api_key" + type="password" + autoComplete="new-password" + placeholder="your-maxim-api-key" value={maximConfig.api_key} onChange={(e) => debouncedUpdateMaximConfig({ api_key: e.target.value })} />
🧹 Nitpick comments (10)
transports/bifrost-http/main.go (2)
420-428: Don’t crash the server on malformed plugin config; skip plugin instead.Marshalling errors should degrade per‑plugin, not fatal the whole process.
Apply:
- if err != nil { - logger.Fatal("failed to marshal maxim config: %v", err) - } + if err != nil { + logger.Error("failed to marshal maxim config: %v (skipping maxim)", err) + break + } - if err := json.Unmarshal(configBytes, &maximConfig); err != nil { - logger.Fatal("failed to unmarshal maxim config: %v", err) - } + if err := json.Unmarshal(configBytes, &maximConfig); err != nil { + logger.Error("failed to unmarshal maxim config: %v (skipping maxim)", err) + break + }
431-436: Log successful Maxim initialization for parity with semantic cache.Adds operational visibility.
Apply:
} else { loadedPlugins = append(loadedPlugins, maximPlugin) + logger.Info("successfully initialized maxim") }plugins/maxim/README.md (2)
61-68: Document multi‑repo: header may be repeated to log to multiple repos.Improves clarity on the new capability.
Apply:
-3. Override repository per request (optional): +3. Override repository per request (optional, supports multiple repos): ```bash - # Use specific repository for this request - curl -H "x-bf-maxim-log-repo-id: specific-repo-123" \ + # Log to multiple repositories by repeating the header + curl \ + -H "x-bf-maxim-log-repo-id: repo-1" \ + -H "x-bf-maxim-log-repo-id: repo-2" \ -H "Authorization: Bearer your-api-key" \ -X POST http://localhost:8080/v1/chat/completions \ -d '{"model": "gpt-4", "messages": [...]}'--- `90-99`: **Avoid encouraging pasting tokens in docs; use env var substitution.** Minor hardening for readers following examples. Apply: ```diff - docker run -d \ + export OPENAI_API_KEY=your_openai_api_key + docker run -d \ -p 8080:8080 \ -v $(pwd):/app/data \ -e APP_PORT=8080 \ -e MAXIM_API_KEY \ -e MAXIM_LOG_REPO_ID \ + -e OPENAI_API_KEY \ bifrost-transportdocs/features/plugins/maxim.mdx (2)
105-116: Add multi‑repo header example.Reflects new feature.
Apply:
# Override with specific repository curl -X POST http://localhost:8080/v1/chat/completions \ -H "Authorization: Bearer your-api-key" \ -H "x-bf-maxim-log-repo-id: project-specific-repo" \ -d '{"model": "gpt-4", "messages": [...]}' + +# Log to multiple repositories +curl -X POST http://localhost:8080/v1/chat/completions \ + -H "Authorization: Bearer your-api-key" \ + -H "x-bf-maxim-log-repo-id: repo-1" \ + -H "x-bf-maxim-log-repo-id: repo-2" \ + -d '{"model": "gpt-4", "messages": [...]}'
141-147: Docs: prefer env var tokens to avoid copy‑pasting secrets.Apply:
-curl -X POST http://localhost:8080/v1/chat/completions \ - -H "x-bf-maxim-trace-id: custom-trace-123" \ - -H "x-bf-maxim-generation-id: custom-gen-456" \ - -H "x-bf-maxim-session-id: user-session-789" \ - -d '{"model": "gpt-4", "messages": [...]}' +export OPENAI_API_KEY=your_openai_api_key +curl -X POST http://localhost:8080/v1/chat/completions \ + -H "Authorization: Bearer $OPENAI_API_KEY" \ + -H "x-bf-maxim-trace-id: custom-trace-123" \ + -H "x-bf-maxim-generation-id: custom-gen-456" \ + -H "x-bf-maxim-session-id: user-session-789" \ + -d '{"model": "gpt-4", "messages": [...]}'ui/app/config/views/pluginsForm.tsx (4)
200-231: Debounced Maxim revert uses stale state; capture prevConfig.Reverting to
maximConfiginside the timeout can restore a newer, incorrect state.Apply:
-const debouncedUpdateMaximConfig = useCallback( - (updates: Partial<MaximConfig>) => { - // Update local state immediately for responsive UI - const newConfig = { ...maximConfig, ...updates }; - setMaximConfig(newConfig); +const debouncedUpdateMaximConfig = useCallback( + (updates: Partial<MaximConfig>) => { + const prevConfig = maximConfig; + const newConfig = { ...prevConfig, ...updates }; + setMaximConfig(newConfig); @@ - .catch((error) => { + .catch((error) => { toast.error("Failed to update Maxim configuration"); - // Revert on error - use the newConfig that was captured in closure - setMaximConfig(maximConfig); + // Revert to the previous config snapshot + setMaximConfig(prevConfig); });
112-134: Gate enabling Maxim when API key is empty.Prevents “enabled but not initialized” confusion.
Apply:
const handleMaximToggle = async (enabled: boolean) => { try { + if (enabled && !maximConfig.api_key) { + toast.error("Please enter a Maxim API key before enabling."); + return; + } if (maximPlugin) {
505-512: Repo ID isn’t required; remove “*” and clarify optionality.Aligns with plugin behavior (header override or skip).
Apply:
-<Label htmlFor="log_repo_id">Log Repo ID*</Label> +<Label htmlFor="log_repo_id">Log Repo ID</Label> <Input id="log_repo_id" placeholder="your-log-repo-id" value={maximConfig.log_repo_id} onChange={(e) => debouncedUpdateMaximConfig({ log_repo_id: e.target.value })} />
516-533: Notes: mention multi‑repo via repeated header and conditional logging.Small UX clarity win.
Apply:
<ul className="text-muted-foreground list-inside list-disc text-xs"> @@ - <li> - You can override the default repository per request using the <b>x-bf-maxim-log-repo-id</b> header. - </li> + <li> + Override per request using <b>x-bf-maxim-log-repo-id</b>; repeat the header to log to multiple repositories. + </li> <li> - All requests and responses will be logged to your Maxim repository when enabled. + If no repo is set in config or headers, logging is skipped. </li> </ul>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
docs/docs.json(1 hunks)docs/features/plugins/maxim.mdx(1 hunks)plugins/maxim/README.md(3 hunks)plugins/maxim/main.go(8 hunks)plugins/maxim/plugin_test.go(5 hunks)transports/bifrost-http/lib/ctx.go(1 hunks)transports/bifrost-http/main.go(1 hunks)transports/go.mod(1 hunks)ui/app/config/views/pluginsForm.tsx(9 hunks)ui/lib/types/config.ts(2 hunks)ui/lib/types/plugins.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- ui/lib/types/config.ts
- transports/go.mod
- ui/lib/types/plugins.ts
- transports/bifrost-http/lib/ctx.go
- plugins/maxim/plugin_test.go
- plugins/maxim/main.go
🧰 Additional context used
🧬 Code graph analysis (2)
transports/bifrost-http/main.go (2)
plugins/maxim/main.go (3)
PluginName(19-19)Config(24-27)Init(37-62)transports/bifrost-http/lib/config.go (1)
Config(113-136)
ui/app/config/views/pluginsForm.tsx (6)
ui/lib/types/config.ts (3)
MaximConfig(240-243)CacheConfig(224-237)ModelProviderName(12-12)ui/lib/types/plugins.ts (1)
MAXIM_PLUGIN(4-4)ui/components/ui/switch.tsx (1)
Switch(36-36)ui/components/ui/separator.tsx (1)
Separator(43-43)ui/components/ui/label.tsx (1)
Label(21-21)ui/components/ui/input.tsx (1)
Input(7-21)
🪛 Gitleaks (8.27.2)
docs/features/plugins/maxim.mdx
[high] 107-108: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 112-113: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
plugins/maxim/README.md
[high] 64-65: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🔇 Additional comments (1)
docs/docs.json (1)
101-104: LGTM: navigation entry added for Maxim docs.Placement and JSON are valid.
TejasGhatte
force-pushed
the
09-13-enhancement_configuration_updates_in_maxim_plugin
branch
from
6b057da to
75bed46
Compare
TejasGhatte
changed the base branch from
graphite-base/471
to
09-16-feat_api_key_support_added_for_bedrock_provider
TejasGhatte
force-pushed
the
09-13-enhancement_configuration_updates_in_maxim_plugin
branch
2 times, most recently
from
cd367e4 to
f99b9e3
Compare
TejasGhatte
force-pushed
the
09-13-enhancement_configuration_updates_in_maxim_plugin
branch
2 times, most recently
from
85ec908 to
d6b2896
Compare
Pratham-Mishra04
changed the base branch from
09-16-feat_api_key_support_added_for_bedrock_provider
to
graphite-base/471
TejasGhatte
force-pushed
the
09-13-enhancement_configuration_updates_in_maxim_plugin
branch
from
d6b2896 to
d659da9
Compare
TejasGhatte
force-pushed
the
graphite-base/471
branch
from
4b396fd to
47c6332
Compare
Pratham-Mishra04
force-pushed
the
graphite-base/471
branch
from
47c6332 to
83da938
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (18)
plugins/maxim/README.md (3)
61-68: Avoid showing bearer tokens in docs; use env placeholders.The curl example uses an Authorization header with a token-like value, which static analysis flags. Prefer environment placeholders to discourage copy‑pasting real secrets.
Apply:
- curl -H "x-bf-maxim-log-repo-id: specific-repo-123" \ - -H "Authorization: Bearer your-api-key" \ + curl -H "x-bf-maxim-log-repo-id: specific-repo-123" \ + -H "Authorization: Bearer $OPENAI_API_KEY" \ -X POST http://localhost:8080/v1/chat/completions \ -d '{"model": "gpt-4", "messages": [...]}'
59-59: CLI flag should be double-dashed.Elsewhere you mention “--plugins”; here it’s “-plugins”. Align to the double-dash form to avoid user confusion.
- e.g., `npx -y @maximhq/bifrost -plugins maxim` + e.g., `npx -y @maximhq/bifrost --plugins maxim`
34-46: Docs claim single-repo override but PR summary mentions multi-repo headers.If multi-repo per request via repeated x-bf-maxim-log-repo-id is supported, add an example; if not, clarify it’s single-header only. Keep README consistent with actual behavior.
Example to add (only if supported):
+### Target multiple repositories in one request +You can pass the `x-bf-maxim-log-repo-id` header multiple times: + +```bash +curl -X POST http://localhost:8080/v1/chat/completions \ + -H "x-bf-maxim-log-repo-id: repo1" \ + -H "x-bf-maxim-log-repo-id: repo2" \ + -H "Authorization: Bearer $OPENAI_API_KEY" \ + -d '{"model":"gpt-4","messages":[...]}' +```docs/features/plugins/maxim.mdx (4)
101-112: Mask tokens in curl examples.Use env placeholders to avoid users pasting real tokens.
- -H "Authorization: Bearer your-api-key" \ + -H "Authorization: Bearer $OPENAI_API_KEY" \
136-144: Mask tokens in trace header example as well.Same guidance for all curl blocks: prefer
$OPENAI_API_KEYover literal values.
165-176: Custom tag examples: apply same token masking pattern.Keep token usage consistent across docs to reduce the chance of accidental leakage.
95-112: If multi-repo headers are supported, show the repeated-header pattern.Add an example with two
x-bf-maxim-log-repo-idheaders; otherwise explicitly state that only the first header is used today.docs/features/observability.mdx (4)
71-73: Tab label mismatch: JSON config, not environment variables.This tab shows a config.json snippet, but the intro says “configure via environment variables.” Adjust the sentence for accuracy.
-For HTTP transport, configure via environment variables: +For HTTP transport, configure via config.json:
124-133: Mask tokens in curl examples.Replace literal bearer with env placeholder.
- -H "Authorization: Bearer your-api-key" \ + -H "Authorization: Bearer $OPENAI_API_KEY" \
159-164: Repeat token masking in all curl blocks.Apply the same
$OPENAI_API_KEYsubstitution here for consistency and safety.
128-133: Consider adding a multi-repo request example (if supported).Show repeated
x-bf-maxim-log-repo-idheaders, or state single‑repo only if multi is not yet implemented.ui/app/config/views/pluginsForm.tsx (4)
496-503: Mask the API key input.Prevent shoulder‑surfing; also add autocomplete hint.
-<Label htmlFor="api_key">Maxim API Key</Label> -<Input - id="api_key" - placeholder="your-maxim-api-key" +<Label htmlFor="api_key">Maxim API Key*</Label> +<Input + id="api_key" + type="password" + autoComplete="new-password" + placeholder="your-maxim-api-key" value={maximConfig.api_key} onChange={(e) => debouncedUpdateMaximConfig({ api_key: e.target.value })} />
505-512: “Log Repo ID” is optional in backend; remove the asterisk.Docs and types mark it optional. Align the UI label.
-<Label htmlFor="log_repo_id">Log Repo ID*</Label> +<Label htmlFor="log_repo_id">Log Repo ID (optional)</Label>
200-231: Debounce revert uses stale state; capture prev config.On failure you revert to
maximConfigfrom the closure, which may already include the failed change. CaptureprevConfigbefore setting and revert to it in the catch.const debouncedUpdateMaximConfig = useCallback( (updates: Partial<MaximConfig>) => { - // Update local state immediately for responsive UI - const newConfig = { ...maximConfig, ...updates }; + // Update local state immediately for responsive UI + const prevConfig = maximConfig; + const newConfig = { ...maximConfig, ...updates }; setMaximConfig(newConfig); // Clear previous timeout if (maximDebounceTimeoutRef.current) { clearTimeout(maximDebounceTimeoutRef.current); } // Only save to backend if plugin is enabled, with debouncing if (maximPlugin?.enabled) { maximDebounceTimeoutRef.current = setTimeout(() => { updatePlugin({ name: MAXIM_PLUGIN, data: { enabled: true, config: newConfig }, }) .unwrap() .then(() => { toast.success("Maxim configuration updated successfully"); }) .catch((error) => { toast.error("Failed to update Maxim configuration"); - // Revert on error - use the newConfig that was captured in closure - setMaximConfig(maximConfig); + // Revert on error + setMaximConfig(prevConfig); }); }, 500); // 500ms debounce } }, [maximConfig, maximPlugin?.enabled, updatePlugin], );
472-481: Optional: block enabling until API key is present.Prevent a predictable failure path and reduce churn in toast/errors.
Example:
-<Switch +<Switch id="enable-maxim" size="md" checked={isMaximEnabled} - disabled={providersLoading || providers.length === 0} + disabled={providersLoading || providers.length === 0 || !maximConfig.api_key}plugins/maxim/main.go (3)
251-278: Avoid logging non-text/binary content blocks verbatim.
message.Contentmay include image/audio blocks; forwarding raw blocks can bloat logs or leak sensitive data. Consider transformingmessagesto include only sanitized text (e.g., last N chars) and metadata (counts, types), mirroring the prior change you made for embeddings/speech/transcription.I can propose a safe content filter if helpful.
412-418: Nit: don’t hold the map lock while flushing I/O.
Flush()likely performs I/O. Copy the map under a read lock, then release the lock before iterating to reduce contention with writers.- plugin.loggerMutex.RLock() - for _, logger := range plugin.loggers { - logger.Flush() - } - plugin.loggerMutex.RUnlock() + plugin.loggerMutex.RLock() + loggers := make([]*logging.Logger, 0, len(plugin.loggers)) + for _, l := range plugin.loggers { loggers = append(loggers, l) } + plugin.loggerMutex.RUnlock() + for _, logger := range loggers { + logger.Flush() + }Also applies to: 423-431
39-41: Error message consistency.Use a clearer message that matches the config field naming.
- return nil, fmt.Errorf("apiKey is not set") + return nil, fmt.Errorf("api_key is required")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
docs/docs.json(1 hunks)docs/features/observability.mdx(1 hunks)docs/features/plugins/maxim.mdx(1 hunks)plugins/maxim/README.md(3 hunks)plugins/maxim/main.go(8 hunks)plugins/maxim/plugin_test.go(5 hunks)transports/bifrost-http/lib/ctx.go(1 hunks)transports/bifrost-http/main.go(1 hunks)ui/app/config/views/pluginsForm.tsx(9 hunks)ui/lib/types/config.ts(2 hunks)ui/lib/types/plugins.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- ui/lib/types/plugins.ts
- transports/bifrost-http/lib/ctx.go
- transports/bifrost-http/main.go
- ui/lib/types/config.ts
- docs/docs.json
- plugins/maxim/plugin_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
plugins/maxim/main.go (3)
transports/bifrost-http/lib/config.go (1)
Config(113-136)ui/lib/types/plugins.ts (1)
Plugin(6-10)transports/bifrost-http/lib/ctx.go (1)
ContextKey(68-68)
ui/app/config/views/pluginsForm.tsx (2)
ui/lib/types/config.ts (3)
MaximConfig(240-243)CacheConfig(224-237)ModelProviderName(12-12)ui/lib/types/plugins.ts (1)
MAXIM_PLUGIN(4-4)
🪛 Gitleaks (8.27.2)
docs/features/plugins/maxim.mdx
[high] 103-104: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 108-109: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
plugins/maxim/README.md
[high] 64-65: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
⏰ 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). (6)
- 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 (1)
plugins/maxim/main.go (1)
69-80: Incorrect — the context-key type mismatch claim is falsetransports/bifrost-http/lib/ctx.go stores the header using maxim.ContextKey(labelName) and checks string(maxim.LogRepoIDKey), while plugins/maxim/main.go defines and reads LogRepoIDKey (maxim.ContextKey). The x-bf-maxim-log-repo-id header is being written with the same typed key and will be visible to the plugin (see transports/bifrost-http/lib/ctx.go ≈ lines 95–116 and plugins/maxim/main.go ≈ lines 64–80, 121–125).
Likely an incorrect or invalid review comment.
TejasGhatte
force-pushed
the
09-13-enhancement_configuration_updates_in_maxim_plugin
branch
from
53da1d7 to
4632e8c
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)
plugins/maxim/README.md (1)
102-116: Use typed keys in Go example; string keys won’t work.Align with package’s ContextKey usage.
- ctx = context.WithValue(ctx, "generation-id", "123") + ctx = context.WithValue(ctx, maxim.GenerationIDKey, "123")Also note: use maxim.TraceIDKey / maxim.SessionIDKey for trace/session if needed.
♻️ Duplicate comments (6)
ui/app/config/views/pluginsForm.tsx (1)
496-503: Mask the API key input and set a unique id.Repeat of earlier review; still unmasked. Use type="password" (and consider autocomplete="off").
-<Label htmlFor="api_key">Maxim API Key</Label> -<Input - id="api_key" - placeholder="your-maxim-api-key" +<Label htmlFor="maxim_api_key">Maxim API Key</Label> +<Input + id="maxim_api_key" + type="password" + autoComplete="off" + placeholder="your-maxim-api-key" value={maximConfig.api_key} onChange={(e) => debouncedUpdateMaximConfig({ api_key: e.target.value })} />plugins/maxim/main.go (5)
380-417: Complete/end generations and traces across all repos; flush only once.Use repoIDs; avoid early returns; optionally flush only involved loggers to reduce overhead.
-// Get effective log repo ID for this request -effectiveLogRepoID := plugin.getEffectiveLogRepoID(ctxRef) +repoIDs := plugin.getEffectiveLogRepoIDs(ctxRef) @@ - if ok && effectiveLogRepoID != "" { - // Process generation completion in the effective log repository - logger, err := plugin.getOrCreateLogger(effectiveLogRepoID) - if err == nil { + if ok && len(repoIDs) > 0 { + for _, rid := range repoIDs { + if logger, err := plugin.getOrCreateLogger(rid); err == nil { if bifrostErr != nil { genErr := logging.GenerationError{ Message: bifrostErr.Error.Message, Code: bifrostErr.Error.Code, Type: bifrostErr.Error.Type, } logger.SetGenerationError(generationID, &genErr) } else if res != nil { logger.AddResultToGeneration(generationID, res) } - logger.EndGeneration(generationID) - } - } + logger.EndGeneration(generationID) + } + } } @@ - if ok && effectiveLogRepoID != "" { - // End trace in the effective log repository - logger, err := plugin.getOrCreateLogger(effectiveLogRepoID) - if err == nil { - logger.EndTrace(traceID) - } - } + if ok && len(repoIDs) > 0 { + for _, rid := range repoIDs { + if logger, err := plugin.getOrCreateLogger(rid); err == nil { + logger.EndTrace(traceID) + } + } + }Optional (performance): keep current “flush all” or track involved logger IDs in ctx and flush those only.
116-135: Implement multi-repo selection (headers can repeat).Current helper returns a single repo ID; PR goal requires fan‑out to multiple repos. Return []string, accept both string and []string from context, and include default (deduped, order-preserving).
-// getEffectiveLogRepoID determines which single log repo ID to use based on priority: +// getEffectiveLogRepoIDs determines which log repo IDs to use based on priority: // 1. Header log repo ID (if provided) // 2. Default log repo ID from config (if configured) -// 3. Empty string (skip logging) -func (plugin *Plugin) getEffectiveLogRepoID(ctx *context.Context) string { - // Check for header log repo ID first (highest priority) - if ctx != nil { - if headerRepoID, ok := (*ctx).Value(LogRepoIDKey).(string); ok && headerRepoID != "" { - return headerRepoID - } - } - // Fall back to default log repo ID from config - if plugin.defaultLogRepoId != "" { - return plugin.defaultLogRepoId - } - // Return empty string if neither header nor default is available - return "" -} +// 3. Empty (skip logging) +func (plugin *Plugin) getEffectiveLogRepoIDs(ctx *context.Context) []string { + ids := make([]string, 0, 3) + if ctx != nil { + if v := (*ctx).Value(LogRepoIDKey); v != nil { + switch t := v.(type) { + case string: + if t != "" { ids = append(ids, t) } + case []string: + ids = append(ids, t...) + } + } + // Fallbacks for transports/SDKs that set plain string keys + for _, k := range []any{"log-repo-id", "x-bf-maxim-log-repo-id"} { + if v := (*ctx).Value(k); v != nil { + if s, ok := v.(string); ok && s != "" { ids = append(ids, s) } + if ss, ok := v.([]string); ok { ids = append(ids, ss...) } + } + } + } + if plugin.defaultLogRepoId != "" { + ids = append(ids, plugin.defaultLogRepoId) + } + // de-dup while preserving order + seen := map[string]struct{}{} + uniq := make([]string, 0, len(ids)) + for _, id := range ids { + if id == "" { continue } + if _, ok := seen[id]; !ok { + seen[id] = struct{}{} + uniq = append(uniq, id) + } + } + return uniq +}
194-201: Use multi-repo list and short-circuit when none.Swap single-id logic with list returned by getEffectiveLogRepoIDs.
-// Get effective log repo ID (header > default > skip) -effectiveLogRepoID := plugin.getEffectiveLogRepoID(ctx) -// If no log repo ID available, skip logging -if effectiveLogRepoID == "" { +repoIDs := plugin.getEffectiveLogRepoIDs(ctx) +if len(repoIDs) == 0 { return req, nil, nil }
307-313: Create trace in each selected repo.Fan-out to all repo IDs.
-// Create trace in the effective log repository -logger, err := plugin.getOrCreateLogger(effectiveLogRepoID) -if err == nil { - trace := logger.Trace(&traceConfig) - trace.SetInput(latestMessage) -} +for _, rid := range repoIDs { + if logger, err := plugin.getOrCreateLogger(rid); err == nil { + trace := logger.Trace(&traceConfig) + trace.SetInput(latestMessage) + } +}
340-345: Add generation in each selected repo.-// Add generation to the effective log repository -logger, err := plugin.getOrCreateLogger(effectiveLogRepoID) -if err == nil { - logger.AddGenerationToTrace(traceID, &generationConfig) -} +for _, rid := range repoIDs { + if logger, err := plugin.getOrCreateLogger(rid); err == nil { + logger.AddGenerationToTrace(traceID, &generationConfig) + } +}
🧹 Nitpick comments (11)
docs/features/observability.mdx (3)
71-86: Fix heading: this block is config.json, not env vars.The text says “configure via environment variables” but shows JSON config. Adjust to avoid confusion.
Apply:
-For HTTP transport, configure via environment variables: +For HTTP transport, configure via config.json:
124-131: Avoid literal-looking tokens in docs to silence secret scanners.Gitleaks flagged Authorization header examples. Use an env var placeholder.
- -H "Authorization: Bearer your-api-key" \ + -H "Authorization: Bearer $BIFROST_API_KEY" \Also applies to: 129-133
180-186: Align “Supported Request Types” with implementation.Docs list Chat/Text only, while PR summary mentions embedding/speech/transcription. Either add those rows (with sanitization notes) or clarify they’re not yet supported.
ui/app/config/views/pluginsForm.tsx (4)
167-197: Fix stale rollback in debounced cache updates.On failure you revert to
cacheConfigcaptured from closure, which may already contain newer edits. Capture a snapshot before updating and revert to it.const debouncedUpdateCacheConfig = useCallback( (updates: Partial<CacheConfig>) => { - // Update local state immediately for responsive UI - const newConfig = { ...cacheConfig, ...updates }; + const prevConfig = cacheConfig; // snapshot + const newConfig = { ...cacheConfig, ...updates }; setCacheConfig(newConfig); @@ - .catch((error) => { + .catch((error) => { toast.error("Failed to update cache configuration"); - // Revert on error - setCacheConfig(cacheConfig); + // Revert on error + setCacheConfig(prevConfig); });
200-231: Fix stale rollback in debounced Maxim updates.Same race as cache path; revert to a captured
prevConfiginstead of the closure-boundmaximConfig.const debouncedUpdateMaximConfig = useCallback( (updates: Partial<MaximConfig>) => { - // Update local state immediately for responsive UI - const newConfig = { ...maximConfig, ...updates }; + const prevConfig = maximConfig; // snapshot + const newConfig = { ...maximConfig, ...updates }; setMaximConfig(newConfig); @@ - .catch((error) => { + .catch((error) => { toast.error("Failed to update Maxim configuration"); // Revert on error - use the newConfig that was captured in closure - setMaximConfig(maximConfig); + setMaximConfig(prevConfig); });
505-512: Make “Log Repo ID” optional in UI to match docs/config.Label shows required (“*”) but field is optional per docs and config tags.
-<Label htmlFor="log_repo_id">Log Repo ID*</Label> +<Label htmlFor="log_repo_id">Log Repo ID</Label>
186-195: Reduce toast spam for debounced saves.Success toasts fire after every debounced keystroke. Consider replacing with a subtle “Saved” status or throttled toasts.
Also applies to: 218-227
plugins/maxim/main.go (2)
203-236: Accept raw string context keys as fallback (SDK ergonomics).Docs currently suggest plain strings. Either update docs (recommended) or accept both typed and raw keys to reduce friction.
- if existingTraceID, ok := (*ctx).Value(TraceIDKey).(string); ok && existingTraceID != "" { + existingTraceID, ok := (*ctx).Value(TraceIDKey).(string) + if !ok || existingTraceID == "" { + if s, ok2 := (*ctx).Value("trace-id").(string); ok2 { existingTraceID = s } + } + if existingTraceID != "" { traceID = existingTraceID } @@ - if existingSessionID, ok := (*ctx).Value(SessionIDKey).(string); ok && existingSessionID != "" { + if existingSessionID, ok := (*ctx).Value(SessionIDKey).(string); !ok || existingSessionID == "" { + if s, ok2 := (*ctx).Value("session-id").(string); ok2 { existingSessionID = s } + } + if existingSessionID != "" { sessionID = existingSessionID } @@ - if existingTraceName, ok := (*ctx).Value(TraceNameKey).(string); ok && existingTraceName != "" { + if existingTraceName, ok := (*ctx).Value(TraceNameKey).(string); !ok || existingTraceName == "" { + if s, ok2 := (*ctx).Value("trace-name").(string); ok2 { existingTraceName = s } + } + if existingTraceName != "" { traceName = existingTraceName } @@ - if existingGenerationName, ok := (*ctx).Value(GenerationNameKey).(string); ok && existingGenerationName != "" { + if existingGenerationName, ok := (*ctx).Value(GenerationNameKey).(string); !ok || existingGenerationName == "" { + if s, ok2 := (*ctx).Value("generation-name").(string); ok2 { existingGenerationName = s } + } + if existingGenerationName != "" { generationName = existingGenerationName }
243-285: Consider truncating logged input to prevent oversized logs/PII.Protect dashboards and storage from huge payloads (e.g., long prompts). Truncate latestMessage (e.g., 4–8KB) with an ellipsis; keep full request in messages if necessary and allowed.
- latestMessage = *req.Input.TextCompletionInput + const maxLen = 4096 + s := *req.Input.TextCompletionInput + if len(s) > maxLen { + latestMessage = s[:maxLen] + "…" + } else { + latestMessage = s + }plugins/maxim/README.md (2)
34-46: Call out multi-repo header repetition.Add example showing repeated x-bf-maxim-log-repo-id to hit multiple repos.
-This allows you to: +This allows you to: - Set a default repository in your plugin configuration - - Override it per-request using HTTP headers + - Override it per-request using HTTP headers (repeat the header to target multiple repositories) - Automatically skip logging when no repository is configured
62-69: Safer Authorization header placeholder and multi-repo example.Switch to env var placeholder and add a second header.
- curl -H "x-bf-maxim-log-repo-id: specific-repo-123" \ - -H "Authorization: Bearer your-api-key" \ + curl -H "x-bf-maxim-log-repo-id: repo-1" \ + -H "x-bf-maxim-log-repo-id: repo-2" \ + -H "Authorization: Bearer $BIFROST_API_KEY" \ -X POST http://localhost:8080/v1/chat/completions \ -d '{"model": "gpt-4", "messages": [...]}'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/docs.json(1 hunks)docs/features/observability.mdx(1 hunks)plugins/maxim/README.md(3 hunks)plugins/maxim/main.go(8 hunks)plugins/maxim/plugin_test.go(5 hunks)transports/bifrost-http/lib/ctx.go(1 hunks)transports/bifrost-http/main.go(1 hunks)ui/app/config/views/pluginsForm.tsx(9 hunks)ui/lib/types/config.ts(2 hunks)ui/lib/types/plugins.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- ui/lib/types/config.ts
- ui/lib/types/plugins.ts
- transports/bifrost-http/lib/ctx.go
- docs/docs.json
- transports/bifrost-http/main.go
- plugins/maxim/plugin_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
plugins/maxim/main.go (3)
transports/bifrost-http/lib/config.go (1)
Config(113-136)ui/lib/types/plugins.ts (1)
Plugin(6-10)transports/bifrost-http/lib/ctx.go (1)
ContextKey(68-68)
ui/app/config/views/pluginsForm.tsx (2)
ui/lib/types/config.ts (3)
MaximConfig(240-243)CacheConfig(224-237)ModelProviderName(12-12)ui/lib/types/plugins.ts (1)
MAXIM_PLUGIN(4-4)
🪛 Gitleaks (8.27.2)
plugins/maxim/README.md
[high] 64-65: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
docs/features/observability.mdx
[high] 124-125: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 129-130: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
⏰ 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). (27)
- 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 (2)
plugins/maxim/README.md (2)
16-23: API usage looks good.Init now accepts Config; example matches code.
125-129: Keep env var names consistent.You standardized on MAXIM_LOG_REPO_ID elsewhere; this block matches—good. Also consider noting it’s optional.
TejasGhatte
force-pushed
the
09-13-enhancement_configuration_updates_in_maxim_plugin
branch
from
4632e8c to
c908094
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/maxim/main.go (1)
14-16: Name collision: importing maxim-go without alias breaks compilation.Current code imports
github.com/maximhq/maxim-goasmaximinside packagemaxim, causing symbol shadowing. Calls likemaxim.Initandmaxim.StrPtrresolve to this package, not the SDK. This won’t compile. Alias the SDK import and update references.Apply:
-import ( +import ( "context" "encoding/json" "fmt" - "sync" + "sync" "github.com/google/uuid" "github.com/maximhq/bifrost/core/schemas" - "github.com/maximhq/maxim-go" + mx "github.com/maximhq/maxim-go" "github.com/maximhq/maxim-go/logging" ) @@ -func Init(config Config) (schemas.Plugin, error) { +func Init(config Config) (schemas.Plugin, error) { @@ - mx := maxim.Init(&maxim.MaximSDKConfig{ApiKey: config.ApiKey}) + sdk := mx.Init(&mx.MaximSDKConfig{ApiKey: config.ApiKey}) @@ plugin := &Plugin{ - mx: mx, + mx: sdk, defaultLogRepoId: config.LogRepoId, loggers: make(map[string]*logging.Logger), loggerMutex: &sync.RWMutex{}, } @@ -type Plugin struct { - mx *maxim.Maxim +type Plugin struct { + mx *mx.Maxim defaultLogRepoId string loggers map[string]*logging.Logger loggerMutex *sync.RWMutex } @@ - Name: maxim.StrPtr(name), + Name: mx.StrPtr(name),Also applies to: 43-50, 105-109, 297-301
♻️ Duplicate comments (1)
docs/features/observability.mdx (1)
145-155: Use typed context keys from the maxim package; string keys won’t match.
context.WithValue(ctx, "trace-id", ...)won’t be read by the plugin (typed key mismatch). Usemaxim.TraceIDKey,maxim.GenerationIDKey, andmaxim.SessionIDKey.Apply:
-// Set custom trace ID -ctx = context.WithValue(ctx, "trace-id", "custom-trace-123") +// Set custom trace ID +ctx = context.WithValue(ctx, maxim.TraceIDKey, "custom-trace-123") -// Set custom generation ID -ctx = context.WithValue(ctx, "generation-id", "custom-gen-456") +// Set custom generation ID +ctx = context.WithValue(ctx, maxim.GenerationIDKey, "custom-gen-456") -// Set custom session ID -ctx = context.WithValue(ctx, "session-id", "user-session-789") +// Set custom session ID +ctx = context.WithValue(ctx, maxim.SessionIDKey, "user-session-789")Also add a short note: “If using the Go SDK directly, use maxim.*Key typed constants; plain string keys won’t match.”
🧹 Nitpick comments (7)
docs/features/observability.mdx (2)
123-133: Replace literal API tokens in curl with env var to avoid secret scanners.Use
$BIFROST_API_KEYinstead of a literal token; Gitleaks flagged this.Apply:
- -H "Authorization: Bearer your-api-key" \ + -H "Authorization: Bearer $BIFROST_API_KEY" \
112-119: Clarify single‑repo behavior (no multi‑repo fan‑out).Per current implementation, only one
x-bf-maxim-log-repo-idis honored (header overrides default). Add a note that repeated headers are not fanned out to multiple repos.Proposed note after the priority list:
+> Note: Only a single `x-bf-maxim-log-repo-id` value is honored per request. Repeating the header does not fan out logs to multiple repositories.plugins/maxim/README.md (1)
61-68: Avoid literal tokens in docs and clarify single‑repo header behavior.
- Replace
your-api-keywith$BIFROST_API_KEYto avoid secret scanner noise.- Add a note that only one
x-bf-maxim-log-repo-idis honored (no multi‑repo fan‑out).Apply:
- curl -H "x-bf-maxim-log-repo-id: specific-repo-123" \ - -H "Authorization: Bearer your-api-key" \ + curl -H "x-bf-maxim-log-repo-id: specific-repo-123" \ + -H "Authorization: Bearer $BIFROST_API_KEY" \ -X POST http://localhost:8080/v1/chat/completions \ -d '{"model": "gpt-4", "messages": [...]}' + + # Note: Only a single x-bf-maxim-log-repo-id is honored per request.plugins/maxim/main.go (3)
412-417: Don’t hold the RW lock while flushing; copy loggers then flush.
Flush()may block; holding the read lock can starve writers creating new loggers. Copy pointers under RLock, unlock, then flush.Apply:
- plugin.loggerMutex.RLock() - for _, logger := range plugin.loggers { - logger.Flush() - } - plugin.loggerMutex.RUnlock() + plugin.loggerMutex.RLock() + loggers := make([]*logging.Logger, 0, len(plugin.loggers)) + for _, l := range plugin.loggers { + loggers = append(loggers, l) + } + plugin.loggerMutex.RUnlock() + for _, l := range loggers { + l.Flush() + }Repeat the same pattern in Cleanup().
Also applies to: 424-429
307-344: Swallowing logger errors silently — log at debug level.If
getOrCreateLoggerfails, it’s silently ignored. Consider emitting a debug warning with repo ID and error to aid ops.Example:
- if err == nil { + if err == nil { trace := logger.Trace(&traceConfig) trace.SetInput(latestMessage) - } + } else { + // TODO: replace with structured logger if available + fmt.Printf("[maxim] warn: repo %s trace init failed: %v\n", effectiveLogRepoID, err) + }(and similarly in PostHook)
Also applies to: 381-410
251-285: Consider trimming or capping logged message content.Logging full chat/text content may be large/PII-heavy. Consider truncating
latestMessageand/or messages for safer defaults (configurable).transports/go.mod (1)
80-80: Remove indirect github.com/maximhq/maxim-go from transports/go.mod (rungo mod tidy)transports does not import github.com/maximhq/maxim-go directly — it's imported by plugins/maxim (plugins/maxim/main.go) and declared in plugins/maxim/go.mod; run
go mod tidyin transports to drop the indirect entry at transports/go.mod:80, or keep it only if you intentionally want to pin the transitive version.
📜 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 (12)
docs/docs.json(1 hunks)docs/features/observability.mdx(1 hunks)plugins/maxim/README.md(3 hunks)plugins/maxim/go.mod(1 hunks)plugins/maxim/main.go(8 hunks)plugins/maxim/plugin_test.go(5 hunks)transports/bifrost-http/lib/ctx.go(1 hunks)transports/bifrost-http/main.go(1 hunks)transports/go.mod(1 hunks)ui/app/config/views/pluginsForm.tsx(9 hunks)ui/lib/types/config.ts(2 hunks)ui/lib/types/plugins.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- docs/docs.json
- ui/lib/types/plugins.ts
- ui/app/config/views/pluginsForm.tsx
- plugins/maxim/plugin_test.go
- transports/bifrost-http/lib/ctx.go
- transports/bifrost-http/main.go
- ui/lib/types/config.ts
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/maxim/main.go (3)
transports/bifrost-http/lib/config.go (1)
Config(113-136)ui/lib/types/plugins.ts (1)
Plugin(6-10)transports/bifrost-http/lib/ctx.go (1)
ContextKey(68-68)
🪛 Gitleaks (8.27.2)
docs/features/observability.mdx
[high] 124-125: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 129-130: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
plugins/maxim/README.md
[high] 64-65: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
⏰ 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 (5)
plugins/maxim/README.md (2)
16-19: Init(config) usage looks good.Switch to config-based Init is clear and consistent.
120-129: Env var names standardized — LGTM.Using MAXIM_LOG_REPO_ID consistently is good.
plugins/maxim/main.go (2)
116-135: Single‑repo selection is clear — ensure docs align (no multi‑repo claim).Code intentionally supports one effective repo (header > default > skip). Make sure PR description/docs don’t promise multi‑repo fan‑out.
64-80: Typed context keys are package-scoped — OK: transport uses maxim.ContextKey; no raw string keys found.
Confirmed: no context.WithValue calls with raw string keys; see transports/bifrost-http/lib/ctx.go and plugins/maxim/plugin_test.go.plugins/maxim/go.mod (1)
9-9: Rungo mod tidyin module roots and confirm maxim-go v0.1.10 API compatibilitymaxim-go v0.1.10 is present in plugins/maxim/go.mod:9 (direct) and transports/go.mod:80 (// indirect).
- Run: cd plugins/maxim && go mod tidy && git diff -- plugins/maxim/go.mod plugins/maxim/go.sum
- Run: cd transports && go mod tidy && git diff -- transports/go.mod transports/go.sum
- Verify plugins/maxim/main.go import alias and SDK usages still match v0.1.10; fix code or dependency if any breaking changes are found.
Pratham-Mishra04
force-pushed
the
09-13-enhancement_configuration_updates_in_maxim_plugin
branch
from
c908094 to
2156d53
Compare
Pratham-Mishra04
force-pushed
the
09-16-feat_api_key_support_added_for_bedrock_provider
branch
from
83da938 to
9853050
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
docs/features/observability.mdx (1)
141-155: Use typed context keys from the maxim package; raw strings won’t match.The plugin reads typed keys (maxim.ContextKey). Update the snippet to use maxim.*Key constants and add an import note.
ctx := context.Background() -// Set custom trace ID -ctx = context.WithValue(ctx, "trace-id", "custom-trace-123") +// Set custom trace ID +ctx = context.WithValue(ctx, maxim.TraceIDKey, "custom-trace-123") -// Set custom generation ID -ctx = context.WithValue(ctx, "generation-id", "custom-gen-456") +// Set custom generation ID +ctx = context.WithValue(ctx, maxim.GenerationIDKey, "custom-gen-456") -// Set custom session ID -ctx = context.WithValue(ctx, "session-id", "user-session-789") +// Set custom session ID +ctx = context.WithValue(ctx, maxim.SessionIDKey, "user-session-789")Add below the snippet: “If using the Go SDK directly, import github.com/maximhq/bifrost/plugins/maxim and use maxim.*Key typed constants; plain string keys won’t match.”
plugins/maxim/README.md (1)
102-116: Use typed context keys in examples; strings won’t match plugin lookups.Switch to maxim.*Key constants and mention the import.
- ctx = context.WithValue(ctx, "generation-id", "123") + ctx = context.WithValue(ctx, maxim.GenerationIDKey, "123")Add a short note: “Import github.com/maximhq/bifrost/plugins/maxim and use maxim.*Key typed constants for context keys.”
🧹 Nitpick comments (13)
docs/features/observability.mdx (3)
125-126: Replace literal token with env var in curl examples (fixes Gitleaks warnings).Use an environment variable placeholder instead of a hardcoded bearer token.
- -H "Authorization: Bearer your-api-key" \ + -H "Authorization: Bearer $BIFROST_API_KEY" \ @@ - -H "Authorization: Bearer your-api-key" \ + -H "Authorization: Bearer $BIFROST_API_KEY" \Also applies to: 129-132
110-119: Clarify single‑repo behavior to avoid confusion with multi‑header claims.Current implementation logs to one repository per request (header OR default). Add a note that repeated x-bf-maxim-log-repo-id headers are not aggregated.
3. **Skip Logging** - If neither is available + +Note: Only a single repository is used per request. If multiple x-bf-maxim-log-repo-id headers are sent, only one will be used.
178-186: Align “Supported Request Types” with current implementation.Docs list only Chat/Text. If embedding/speech/transcription support isn’t wired here, either add them with caveats or explicitly state they’re not yet logged. Otherwise this reads inconsistent with the PR summary.
plugins/maxim/README.md (2)
62-68: Use env var for bearer token in curl example.Prevents accidental token leaks and silences Gitleaks.
- curl -H "x-bf-maxim-log-repo-id: specific-repo-123" \ - -H "Authorization: Bearer your-api-key" \ + curl -H "x-bf-maxim-log-repo-id: specific-repo-123" \ + -H "Authorization: Bearer $BIFROST_API_KEY" \ -X POST http://localhost:8080/v1/chat/completions \ -d '{"model": "gpt-4", "messages": [...]}'
34-46: Document single‑repo selection explicitly.State that only one repo is used per request (header overrides default). Avoid implying multi‑repo fan‑out.
The plugin uses the following priority for selecting which log repository to use: @@ 3. **Skip logging** - If neither header nor default config is available, logging is skipped + +Note: The plugin logs to a single repository per request. Repeating x-bf-maxim-log-repo-id does not fan out to multiple repos.ui/app/config/views/pluginsForm.tsx (4)
167-198: Fix debounced revert logic for cache config (use previous snapshot).Reverting to
cacheConfigcaptured in the closure can be wrong after multiple edits. CaptureprevConfigbefore local update and use it on failure.-const debouncedUpdateCacheConfig = useCallback( +const debouncedUpdateCacheConfig = useCallback( (updates: Partial<CacheConfig>) => { - // Update local state immediately for responsive UI - const newConfig = { ...cacheConfig, ...updates }; + const prevConfig = cacheConfig; + const newConfig = { ...cacheConfig, ...updates }; setCacheConfig(newConfig); @@ - .catch((error) => { + .catch((error) => { toast.error("Failed to update cache configuration"); - // Revert on error - setCacheConfig(cacheConfig); + // Revert on error + setCacheConfig(prevConfig); });
200-231: Fix debounced revert logic for Maxim config (use previous snapshot).Same issue for Maxim; capture
prevConfigand revert to it on error.-const debouncedUpdateMaximConfig = useCallback( +const debouncedUpdateMaximConfig = useCallback( (updates: Partial<MaximConfig>) => { - // Update local state immediately for responsive UI - const newConfig = { ...maximConfig, ...updates }; + const prevConfig = maximConfig; + const newConfig = { ...maximConfig, ...updates }; setMaximConfig(newConfig); @@ - .catch((error) => { + .catch((error) => { toast.error("Failed to update Maxim configuration"); - // Revert on error - use the newConfig that was captured in closure - setMaximConfig(maximConfig); + // Revert on error + setMaximConfig(prevConfig); });
496-503: Mask the API key input and avoid generic id.Use a password field and a unique id; disable autocomplete/spellcheck.
-<Label htmlFor="api_key">Maxim API Key</Label> -<Input - id="api_key" - placeholder="your-maxim-api-key" +<Label htmlFor="maxim_api_key">Maxim API Key</Label> +<Input + id="maxim_api_key" + type="password" + autoComplete="off" + spellCheck={false} + placeholder="your-maxim-api-key" value={maximConfig.api_key} onChange={(e) => debouncedUpdateMaximConfig({ api_key: e.target.value })} />
505-512: Log Repo ID is optional in backend; drop the asterisk.The server treats log_repo_id as optional (fallback via header). Align the UI label.
-<Label htmlFor="log_repo_id">Log Repo ID*</Label> +<Label htmlFor="log_repo_id">Log Repo ID</Label>plugins/maxim/plugin_test.go (1)
199-247: Improve Init validation testability with light indirection.Current tests skip real initialization but don’t assert success for valid configs. Consider injecting a factory (e.g., var newMaxim = maxim.Init) to stub during tests, so you can assert Init returns a plugin without real credentials.
plugins/maxim/main.go (3)
412-418: Flush only the used logger; avoid flushing all on every request.Flushing all loggers per request can hurt throughput with multiple repos. Flush just the effective logger.
-// Flush all loggers -plugin.loggerMutex.RLock() -for _, logger := range plugin.loggers { - logger.Flush() -} -plugin.loggerMutex.RUnlock() +// Flush only the logger used for this request +if effectiveLogRepoID != "" { + if logger, err := plugin.getOrCreateLogger(effectiveLogRepoID); err == nil { + logger.Flush() + } +}
248-276: Cap latestMessage length to keep logs bounded.Very long prompts can bloat traces. Truncate to a reasonable limit (e.g., 2KB).
- latestMessage = *lastMsg.Content.ContentStr + latestMessage = *lastMsg.Content.ContentStr + if len(latestMessage) > 2048 { + latestMessage = latestMessage[:2048] + }Apply the same cap wherever latestMessage is assigned for text completion.
315-324: Handle JSON marshal/unmarshal errors explicitly for model params.Minor robustness: check unmarshalling error and keep params empty on failure.
jsonData, err := json.Marshal(req.Params) -if err == nil { - json.Unmarshal(jsonData, &modelParams) -} +if err == nil { + if uerr := json.Unmarshal(jsonData, &modelParams); uerr != nil { + modelParams = map[string]interface{}{} + } +} else { + modelParams = map[string]interface{}{} +}
📜 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 (12)
docs/docs.json(1 hunks)docs/features/observability.mdx(1 hunks)plugins/maxim/README.md(3 hunks)plugins/maxim/go.mod(1 hunks)plugins/maxim/main.go(8 hunks)plugins/maxim/plugin_test.go(5 hunks)transports/bifrost-http/lib/ctx.go(1 hunks)transports/bifrost-http/main.go(1 hunks)transports/go.mod(1 hunks)ui/app/config/views/pluginsForm.tsx(9 hunks)ui/lib/types/config.ts(2 hunks)ui/lib/types/plugins.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- plugins/maxim/go.mod
- ui/lib/types/config.ts
- transports/go.mod
- ui/lib/types/plugins.ts
- transports/bifrost-http/lib/ctx.go
- transports/bifrost-http/main.go
- docs/docs.json
🧰 Additional context used
🧬 Code graph analysis (3)
plugins/maxim/plugin_test.go (1)
plugins/maxim/main.go (5)
Init(37-62)Config(24-27)Plugin(104-109)LogRepoIDKey(79-79)PluginName(19-19)
ui/app/config/views/pluginsForm.tsx (2)
ui/lib/types/config.ts (3)
MaximConfig(240-243)CacheConfig(224-237)ModelProviderName(12-12)ui/lib/types/plugins.ts (1)
MAXIM_PLUGIN(4-4)
plugins/maxim/main.go (3)
transports/bifrost-http/lib/config.go (1)
Config(113-136)ui/lib/types/plugins.ts (1)
Plugin(6-10)transports/bifrost-http/lib/ctx.go (1)
ContextKey(68-68)
🪛 Gitleaks (8.27.2)
docs/features/observability.mdx
[high] 124-125: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 129-130: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
plugins/maxim/README.md
[high] 64-65: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
⏰ 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). (10)
- 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 (3)
ui/app/config/views/pluginsForm.tsx (1)
162-165: LGTM on separate debounce timers and cleanup.Using distinct refs for cache/Maxim with proper cleanup prevents races and leaks.
Also applies to: 233-241
plugins/maxim/plugin_test.go (1)
81-85: Fix env var name in test comment (consistency).Tests and code use MAXIM_LOG_REPO_ID; update the comment.
[raise_nitpick_refactor]
-// - MAXIM_LOGGER_ID: Your Maxim logger repository ID +// - MAXIM_LOG_REPO_ID: Your Maxim logger repository IDplugins/maxim/main.go (1)
116-135: Single-repo selection logic is clear and matches transports.Priority: header -> default -> skip. Good separation of concerns.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
docs/features/observability.mdx (1)
145-153: *Use typed context keys (maxim.Key) instead of raw strings.Apply:
-// Set custom trace ID -ctx = context.WithValue(ctx, "trace-id", "custom-trace-123") +// Set custom trace ID +ctx = context.WithValue(ctx, maxim.TraceIDKey, "custom-trace-123") -// Set custom generation ID -ctx = context.WithValue(ctx, "generation-id", "custom-gen-456") +// Set custom generation ID +ctx = context.WithValue(ctx, maxim.GenerationIDKey, "custom-gen-456") -// Set custom session ID -ctx = context.WithValue(ctx, "session-id", "user-session-789") +// Set custom session ID +ctx = context.WithValue(ctx, maxim.SessionIDKey, "user-session-789")Add a short note below the snippet: “If using the Go SDK directly, use maxim.*Key typed constants; plain string keys won’t match.”
ui/app/config/views/pluginsForm.tsx (1)
496-503: Mask the API key input and use a unique id.Prevents shoulder-surfing; avoids id collisions.
Apply:
- <Label htmlFor="api_key">Maxim API Key</Label> + <Label htmlFor="maxim_api_key">Maxim API Key</Label> <Input - id="api_key" - placeholder="your-maxim-api-key" + id="maxim_api_key" + type="password" + autoComplete="off" + placeholder="your-maxim-api-key" value={maximConfig.api_key} onChange={(e) => debouncedUpdateMaximConfig({ api_key: e.target.value })} />
🧹 Nitpick comments (11)
plugins/maxim/main.go (4)
52-59: Don’t hard‑fail Init on default logger prewarm; defer creation.Prewarming is nice-to-have. A transient SDK/network error here prevents the whole plugin from loading.
Apply:
// Initialize default logger if LogRepoId is provided if config.LogRepoId != "" { logger, err := mx.GetLogger(&logging.LoggerConfig{Id: config.LogRepoId}) if err != nil { - return nil, fmt.Errorf("failed to initialize default logger: %w", err) + // Defer to lazy creation in getOrCreateLogger + // Optionally log at debug level if you have a logger + // fmt.Printf("maxim: default logger prewarm failed: %v\n", err) + } else { + plugin.loggers[config.LogRepoId] = logger } - plugin.loggers[config.LogRepoId] = logger }
307-313: Trim latestMessage before logging to avoid log bloat/PII bleed-through.Large prompts can explode trace size. Bound to a sane preview length.
Apply:
- if err == nil { - trace := logger.Trace(&traceConfig) - trace.SetInput(latestMessage) - } + if err == nil { + trace := logger.Trace(&traceConfig) + trace.SetInput(safePreview(latestMessage)) + }Add helper (outside this hunk):
// keep outside of methods const maxPreviewLen = 4096 func safePreview(s string) string { if len(s) > maxPreviewLen { return s[:maxPreviewLen] + "…" } return s }
412-417: Avoid flushing all loggers on every request; move to Cleanup or gate behind a config.Per-request Flush() can be expensive and holds the RLock while flushing.
Apply:
- // Flush all loggers - plugin.loggerMutex.RLock() - for _, logger := range plugin.loggers { - logger.Flush() - } - plugin.loggerMutex.RUnlock() + // Consider periodic/async flush; Cleanup() already flushes on shutdown.
64-80: Typed context keys won’t match raw strings from other packages.If transports can’t import maxim to set typed keys, add similar fallbacks for TraceIDKey/GenerationIDKey/SessionIDKey/TagsKey (like you did for repo ID).
Would you like a follow-up patch to add safe fallbacks for these keys as well?
plugins/maxim/plugin_test.go (1)
121-126: Prefer t.Logf over log.Printf in tests.Keeps output tied to the test runner and respects verbosity.
Apply:
- if bifrostErr != nil { - log.Printf("Error in Bifrost request: %v", bifrostErr) - } - - log.Println("Bifrost request completed, check your Maxim Dashboard for the trace") + if bifrostErr != nil { + t.Logf("Error in Bifrost request: %v", bifrostErr) + } + t.Log("Bifrost request completed, check your Maxim Dashboard for the trace")plugins/maxim/README.md (1)
106-116: Use typed context keys; raw strings won’t match.Apply:
- ctx = context.WithValue(ctx, "generation-id", "123") + ctx = context.WithValue(ctx, maxim.GenerationIDKey, "123") @@ - result, err := bifrostClient.ChatCompletionRequest(schemas.OpenAI, &schemas.BifrostRequest{ + result, err := bifrostClient.ChatCompletionRequest(schemas.OpenAI, &schemas.BifrostRequest{Consider updating the preceding snippet header to import the maxim package so the constants resolve.
Do you also want this README to call out that single-repo selection is the current behavior? The PR description still mentions “multiple repositories via repeated headers.”
ui/app/config/views/pluginsForm.tsx (4)
505-512: Label indicates required, but repo id is optional.Clarify to reduce friction; keep validation consistent with backend.
Apply:
- <Label htmlFor="log_repo_id">Log Repo ID*</Label> + <Label htmlFor="log_repo_id">Log Repo ID (optional)</Label>
473-479: Prevent enabling Maxim without an API key.Guard the toggle to avoid creating a broken config.
Apply:
<Switch id="enable-maxim" size="md" checked={isMaximEnabled} - disabled={providersLoading || providers.length === 0} + disabled={providersLoading || providers.length === 0 || !maximConfig.api_key} onCheckedChange={(checked) => { handleMaximToggle(checked); }} />
166-198: Debounced cache update should revert to previous state on error.Current revert uses a potentially stale closure.
Apply:
const debouncedUpdateCacheConfig = useCallback( (updates: Partial<CacheConfig>) => { // Update local state immediately for responsive UI - const newConfig = { ...cacheConfig, ...updates }; + const prevConfig = cacheConfig; + const newConfig = { ...prevConfig, ...updates }; setCacheConfig(newConfig); @@ - // Revert on error - setCacheConfig(cacheConfig); + // Revert on error + setCacheConfig(prevConfig); }); }, 500); // 500ms debounce } }, - [cacheConfig, semanticCachePlugin?.enabled, updatePlugin], + [cacheConfig, semanticCachePlugin?.enabled, updatePlugin], );
200-231: Same revert bug in Maxim debounced update.Capture prev and revert to it on failure.
Apply:
const debouncedUpdateMaximConfig = useCallback( (updates: Partial<MaximConfig>) => { // Update local state immediately for responsive UI - const newConfig = { ...maximConfig, ...updates }; + const prevConfig = maximConfig; + const newConfig = { ...prevConfig, ...updates }; setMaximConfig(newConfig); @@ - // Revert on error - use the newConfig that was captured in closure - setMaximConfig(maximConfig); + // Revert on error + setMaximConfig(prevConfig); }); }, 500); // 500ms debounce } }, - [maximConfig, maximPlugin?.enabled, updatePlugin], + [maximConfig, maximPlugin?.enabled, updatePlugin], );docs/features/observability.mdx (1)
122-133: Avoid literal tokens in docs; use env var placeholder.Prevents leak-y copy/paste and resolves gitleaks warnings.
Apply:
# Use default repository (from config) curl -X POST http://localhost:8080/v1/chat/completions \ - -H "Authorization: Bearer your-api-key" \ + -H "Authorization: Bearer $BIFROST_API_KEY" \ -d '{"model": "gpt-4", "messages": [...]}' @@ # Override with specific repository curl -X POST http://localhost:8080/v1/chat/completions \ - -H "Authorization: Bearer your-api-key" \ + -H "Authorization: Bearer $BIFROST_API_KEY" \ -H "x-bf-maxim-log-repo-id: project-specific-repo" \ -d '{"model": "gpt-4", "messages": [...]}'
📜 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 (12)
docs/docs.json(1 hunks)docs/features/observability.mdx(1 hunks)plugins/maxim/README.md(3 hunks)plugins/maxim/go.mod(1 hunks)plugins/maxim/main.go(8 hunks)plugins/maxim/plugin_test.go(5 hunks)transports/bifrost-http/lib/ctx.go(1 hunks)transports/bifrost-http/main.go(1 hunks)transports/go.mod(1 hunks)ui/app/config/views/pluginsForm.tsx(9 hunks)ui/lib/types/config.ts(2 hunks)ui/lib/types/plugins.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- plugins/maxim/go.mod
- transports/go.mod
- ui/lib/types/config.ts
- docs/docs.json
- transports/bifrost-http/lib/ctx.go
- transports/bifrost-http/main.go
- ui/lib/types/plugins.ts
🧰 Additional context used
🧬 Code graph analysis (3)
ui/app/config/views/pluginsForm.tsx (2)
ui/lib/types/config.ts (3)
MaximConfig(240-243)CacheConfig(224-237)ModelProviderName(12-12)ui/lib/types/plugins.ts (1)
MAXIM_PLUGIN(4-4)
plugins/maxim/plugin_test.go (1)
plugins/maxim/main.go (5)
Init(37-62)Config(24-27)Plugin(104-109)LogRepoIDKey(79-79)PluginName(19-19)
plugins/maxim/main.go (3)
transports/bifrost-http/lib/config.go (1)
Config(113-136)ui/lib/types/plugins.ts (1)
Plugin(6-10)transports/bifrost-http/lib/ctx.go (1)
ContextKey(68-68)
🪛 Gitleaks (8.27.2)
docs/features/observability.mdx
[high] 124-125: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 129-130: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
plugins/maxim/README.md
[high] 64-65: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
⏰ 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). (13)
- 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 (3)
plugins/maxim/main.go (2)
137-165: Thread-safe, double-checked logger cache looks good.
116-135: Typed context key is already used — raw-string fallback not neededtransports/bifrost-http writes the header into context with maxim.ContextKey(labelName) and a string value, and tests set LogRepoIDKey directly, so the current lookup of LogRepoIDKey already matches. Optional: accept []string values (pick first) for defensiveness; do not add raw-string key fallbacks.
Locations: transports/bifrost-http/lib/ctx.go:115-117; plugins/maxim/plugin_test.go:179; plugins/maxim/main.go:79.
Likely an incorrect or invalid review comment.
plugins/maxim/plugin_test.go (1)
81-85: Fix env name in comments: MAXIM_LOG_REPO_ID (not MAXIM_LOGGER_ID).
[raise_nitpick_refactor]
Apply:-// - MAXIM_LOGGER_ID: Your Maxim logger repository ID +// - MAXIM_LOG_REPO_ID: Your Maxim logger repository ID
Pratham-Mishra04
force-pushed
the
09-13-enhancement_configuration_updates_in_maxim_plugin
branch
from
2156d53 to
91e2d09
Compare
Pratham-Mishra04
force-pushed
the
09-16-feat_api_key_support_added_for_bedrock_provider
branch
from
9853050 to
3292a13
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
ui/app/config/views/pluginsForm.tsx (4)
169-199: Fix stale reverts in debounced cache updates.On backend save failure, reverting to
cacheConfigfrom the closure can restore a newer/different state. Capture a snapshot before local update and revert to that.const debouncedUpdateCacheConfig = useCallback( (updates: Partial<CacheConfig>) => { - // Update local state immediately for responsive UI - const newConfig = { ...cacheConfig, ...updates }; + // Update local state immediately for responsive UI + const prevConfig = cacheConfig; + const newConfig = { ...cacheConfig, ...updates }; setCacheConfig(newConfig); @@ .catch((error) => { toast.error("Failed to update cache configuration"); // Revert on error - setCacheConfig(cacheConfig); + setCacheConfig(prevConfig); });
201-232: Fix stale reverts in debounced Maxim updates.Same issue as cache: revert should use the snapshot from before the optimistic update.
const debouncedUpdateMaximConfig = useCallback( (updates: Partial<MaximConfig>) => { - // Update local state immediately for responsive UI - const newConfig = { ...maximConfig, ...updates }; + // Update local state immediately for responsive UI + const prevConfig = maximConfig; + const newConfig = { ...maximConfig, ...updates }; setMaximConfig(newConfig); @@ .catch((error) => { toast.error("Failed to update Maxim configuration"); - // Revert on error - use the newConfig that was captured in closure - setMaximConfig(maximConfig); + // Revert on error + setMaximConfig(prevConfig); });
501-507: Mask the API key and use a unique input id.Prevent shoulder-surfing and improve a11y. Also consider
autoComplete="off"to avoid browser persistence.-<Label htmlFor="api_key">Maxim API Key</Label> -<Input - id="api_key" - placeholder="your-maxim-api-key" +<Label htmlFor="maxim_api_key">Maxim API Key</Label> +<Input + id="maxim_api_key" + type="password" + autoComplete="off" + placeholder="your-maxim-api-key" value={maximConfig.api_key} onChange={(e) => debouncedUpdateMaximConfig({ api_key: e.target.value })} />
479-486: Optional: reduce toast spam on rapid edits.Debounced saves can still produce frequent success toasts while typing. Consider throttling success notifications (e.g., only on blur) or deduplicating identical messages.
plugins/maxim/main.go (6)
116-136: Add compatibility fallback for header-sourced repo IDs.To tolerate transports that set raw string keys (or legacy headers), fall back to checking plain strings and slices.
func (plugin *Plugin) getEffectiveLogRepoID(ctx *context.Context) string { // Check for header log repo ID first (highest priority) if ctx != nil { - if headerRepoID, ok := (*ctx).Value(LogRepoIDKey).(string); ok && headerRepoID != "" { - return headerRepoID - } + if headerRepoID, ok := (*ctx).Value(LogRepoIDKey).(string); ok && headerRepoID != "" { + return headerRepoID + } + // Fallbacks for raw string keys + if v := (*ctx).Value("log-repo-id"); v != nil { + if s, ok := v.(string); ok && s != "" { + return s + } + if ss, ok := v.([]string); ok && len(ss) > 0 && ss[0] != "" { + return ss[0] + } + } + if v := (*ctx).Value("x-bf-maxim-log-repo-id"); v != nil { + if s, ok := v.(string); ok && s != "" { + return s + } + if ss, ok := v.([]string); ok && len(ss) > 0 && ss[0] != "" { + return ss[0] + } + } }
307-313: Bound trace input size to avoid oversized payloads.Large messages can inflate logs and costs. Truncate to a safe limit.
- trace := logger.Trace(&traceConfig) - trace.SetInput(latestMessage) + trace := logger.Trace(&traceConfig) + // Truncate excessively long inputs (e.g., 2KB) + if len(latestMessage) > 2048 { + latestMessage = latestMessage[:2048] + } + trace.SetInput(latestMessage)
412-418: Avoid holding the read lock while flushing.
Flush()may block; copy pointers under lock, then flush outside to reduce lock contention.- // Flush all loggers - plugin.loggerMutex.RLock() - for _, logger := range plugin.loggers { - logger.Flush() - } - plugin.loggerMutex.RUnlock() + // Flush all loggers without holding the lock + plugin.loggerMutex.RLock() + toFlush := make([]*logging.Logger, 0, len(plugin.loggers)) + for _, lg := range plugin.loggers { + toFlush = append(toFlush, lg) + } + plugin.loggerMutex.RUnlock() + for _, lg := range toFlush { + lg.Flush() + }
424-429: Same here: copy then flush outside the lock in Cleanup.- // Flush all loggers - plugin.loggerMutex.RLock() - for _, logger := range plugin.loggers { - logger.Flush() - } - plugin.loggerMutex.RUnlock() + // Flush all loggers + plugin.loggerMutex.RLock() + toFlush := make([]*logging.Logger, 0, len(plugin.loggers)) + for _, lg := range plugin.loggers { + toFlush = append(toFlush, lg) + } + plugin.loggerMutex.RUnlock() + for _, lg := range toFlush { + lg.Flush() + }
238-285: Coverage gap: Embedding/Speech/Transcription inputs not handled here.PR summary mentions support for these inputs; this file handles chat/text only. Confirm if other paths cover them; if not, add sanitized logging for those inputs.
I can draft a minimal, sanitized handling block for these input types if needed.
195-201: Minor: avoid duplicate logger lookups.Cache the
logger, err := plugin.getOrCreateLogger(effectiveLogRepoID)once per hook to reduce repeated calls.Also applies to: 340-345, 385-401, 404-410
plugins/maxim/plugin_test.go (1)
81-85: Update env var name in docstring.Keep docs consistent with code and README: use MAXIM_LOG_REPO_ID, not MAXIM_LOGGER_ID.
-// - MAXIM_LOGGER_ID: Your Maxim logger repository ID +// - MAXIM_LOG_REPO_ID: Your Maxim logger repository IDdocs/features/observability.mdx (1)
229-231: Remove duplicate bullet.“Cost Analytics” is listed twice.
- - **Cost Analytics**: Per-request and aggregate cost tracking - - **Cost Analytics**: Per-request and aggregate cost tracking + - **Cost Analytics**: Per-request and aggregate cost tracking
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
docs/media/ui-observability-config.pngis excluded by!**/*.pngplugins/maxim/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
docs/docs.json(1 hunks)docs/features/observability.mdx(1 hunks)plugins/maxim/README.md(0 hunks)plugins/maxim/go.mod(1 hunks)plugins/maxim/main.go(8 hunks)plugins/maxim/plugin_test.go(5 hunks)transports/bifrost-http/lib/ctx.go(1 hunks)transports/bifrost-http/main.go(1 hunks)transports/go.mod(1 hunks)ui/app/config/views/pluginsForm.tsx(9 hunks)ui/lib/types/config.ts(2 hunks)ui/lib/types/plugins.ts(1 hunks)
💤 Files with no reviewable changes (1)
- plugins/maxim/README.md
🚧 Files skipped from review as they are similar to previous changes (7)
- ui/lib/types/plugins.ts
- transports/go.mod
- docs/docs.json
- transports/bifrost-http/lib/ctx.go
- plugins/maxim/go.mod
- ui/lib/types/config.ts
- transports/bifrost-http/main.go
🧰 Additional context used
🧬 Code graph analysis (3)
plugins/maxim/plugin_test.go (1)
plugins/maxim/main.go (5)
Init(37-62)Config(24-27)Plugin(104-109)LogRepoIDKey(79-79)PluginName(19-19)
plugins/maxim/main.go (2)
transports/bifrost-http/lib/config.go (1)
Config(113-136)transports/bifrost-http/lib/ctx.go (1)
ContextKey(68-68)
ui/app/config/views/pluginsForm.tsx (2)
ui/lib/types/config.ts (2)
MaximConfig(240-243)ModelProviderName(12-12)ui/lib/types/plugins.ts (1)
MAXIM_PLUGIN(4-4)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (4)
plugins/maxim/main.go (1)
79-80: Verify context key type alignment across packages.
ContextKeyis defined here; if transports define their owntype ContextKey string, keys won’t match at runtime. Ensure transports import and usemaxim.LogRepoIDKey(and other keys) directly.Would you like a follow-up patch to add compatible fallbacks if transports currently set raw string keys?
plugins/maxim/plugin_test.go (1)
32-35: LGTM: Init(Config) path exercised.Test now targets the new
Init(Config)API; env sourcing aligns with code.docs/features/observability.mdx (2)
120-129: Port consistency check (docs vs. examples).Examples use 8080 here, while PR/testing guidance uses 8000. Standardize the port across docs to reduce confusion.
110-116: Docs look aligned with single‑repo selection and typed keys.Examples correctly show
maxim.LogRepoIDKeyand reserved header keys. Thanks for avoiding raw auth tokens in curl examples.Also applies to: 156-166, 176-186, 191-199
Pratham-Mishra04
force-pushed
the
09-13-enhancement_configuration_updates_in_maxim_plugin
branch
from
91e2d09 to
37bc39a
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)
plugins/maxim/main.go (1)
248-285: Sanitize logged message contents to avoid PII/binary leakage.messages append raw MessageContent which may include images/audio or large blobs. Log text snippets and structured metadata instead.
- for _, message := range *req.Input.ChatCompletionInput { - messages = append(messages, logging.CompletionRequest{ - Role: string(message.Role), - Content: message.Content, - }) - } + const maxText = 2048 + for _, msg := range *req.Input.ChatCompletionInput { + sanitized := map[string]any{"type": "message"} + if msg.Content.ContentStr != nil { + txt := *msg.Content.ContentStr + if len(txt) > maxText { txt = txt[:maxText] } + sanitized["text"] = txt + } + if blocks := msg.Content.ContentBlocks; blocks != nil { + // summarize blocks: count by type, do not include raw data + counts := map[string]int{} + for _, b := range *blocks { counts[b.Type]++ } + sanitized["blocks"] = counts + } + messages = append(messages, logging.CompletionRequest{ + Role: string(msg.Role), + Content: sanitized, + }) + }Additionally, truncate latestMessage to a bounded length and include length metadata.
- latestMessage = *lastMsg.Content.ContentStr + if s := *lastMsg.Content.ContentStr; len(s) > 2048 { latestMessage = s[:2048] } else { latestMessage = s }
🧹 Nitpick comments (12)
docs/features/plugins/jsonparser.mdx (1)
49-56: Missing context import in example.Code calls context.Background() but import block lacks "context".
import ( + "context" "time" "github.com/maximhq/bifrost/core" "github.com/maximhq/bifrost/core/schemas" "github.com/maximhq/bifrost/plugins/jsonparser" )docs/features/plugins/mocker.mdx (3)
33-41: Example checks the wrong error variable.initErr is assigned; err is checked → compile/runtime bug in docs.
- client, initErr := bifrost.Init(context.Background(), schemas.BifrostConfig{ + client, initErr := bifrost.Init(context.Background(), schemas.BifrostConfig{ Account: &yourAccount, Plugins: []schemas.Plugin{plugin}, }) - if err != nil { - panic(err) + if initErr != nil { + panic(initErr) }
127-132: Consistently handle errors in snippets.Consider showing initErr handling here too for consistency.
479-484: Same note: show error handling for Init in debug example.Improves copy-paste reliability.
docs/quickstart/go-sdk/tool-calling.mdx (1)
76-89: LGTM: Updated Init signature.Ensure surrounding snippet includes context import in the full example.
docs/features/mcp.mdx (2)
106-111: Missing context import in example.Example uses context.Background() without importing "context".
import ( + "context" "github.com/maximhq/bifrost/core" "github.com/maximhq/bifrost/core/schemas" )
285-301: Same: ensure "context" is imported in this longer example.import ( + "context" "fmt" "github.com/maximhq/bifrost/core" "github.com/maximhq/bifrost/core/schemas" )plugins/maxim/main.go (5)
37-61: Init should degrade gracefully if default logger creation fails.Failing plugin Init when default LogRepoId is invalid blocks header-driven logging. Prefer warning and continue.
- logger, 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 + if logger, err := mx.GetLogger(&logging.LoggerConfig{Id: config.LogRepoId}); err == nil { + plugin.loggers[config.LogRepoId] = logger + } else { + // log at warn level if you have a logger; otherwise, proceed without default + }
187-201: Behavior mismatch with PR description: single-repo selection only.Code selects one repo ID; PR text claims “multiple log repositories via repeated x-bf-maxim-log-repo-id”. Either implement fan‑out or update docs/PR.
Would you like a follow-up patch to support multiple repo IDs (fan-out) while preserving order and de-dup?
279-285: Support embedding/speech/transcription inputs as stated in PR goals.PreHook only handles chat/text. Add branches for EmbeddingInput, SpeechInput, TranscriptionInput with metadata-only logging.
I can draft the exact branches mirroring schemas with sanitized fields.
307-313: Swallowing logger errors hides misconfiguration.Consider emitting a debug/warn when getOrCreateLogger fails so operators can diagnose missing repos/keys.
Also applies to: 340-344
412-417: Flushing all loggers on every request can cause overhead.Track the repoID used in context and flush only that (or batch flush on a timer).
- // Flush all loggers - plugin.loggerMutex.RLock() - for _, logger := range plugin.loggers { logger.Flush() } - plugin.loggerMutex.RUnlock() + if v := ctx.Value(LogRepoIDKey); v != nil { + if rid, ok := v.(string); ok && rid != "" { + if logger, err := plugin.getOrCreateLogger(rid); err == nil { logger.Flush() } + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
docs/media/maxim-logs.pngis excluded by!**/*.pngdocs/media/ui-observability-config.pngis excluded by!**/*.pngplugins/maxim/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
docs/docs.json(1 hunks)docs/features/mcp.mdx(3 hunks)docs/features/observability.mdx(1 hunks)docs/features/plugins/jsonparser.mdx(3 hunks)docs/features/plugins/mocker.mdx(3 hunks)docs/quickstart/go-sdk/setting-up.mdx(1 hunks)docs/quickstart/go-sdk/tool-calling.mdx(1 hunks)plugins/maxim/README.md(0 hunks)plugins/maxim/go.mod(1 hunks)plugins/maxim/main.go(8 hunks)plugins/maxim/plugin_test.go(5 hunks)transports/bifrost-http/lib/ctx.go(1 hunks)transports/bifrost-http/main.go(1 hunks)transports/go.mod(1 hunks)ui/app/config/views/pluginsForm.tsx(9 hunks)ui/lib/types/config.ts(2 hunks)ui/lib/types/plugins.ts(1 hunks)
💤 Files with no reviewable changes (1)
- plugins/maxim/README.md
✅ Files skipped from review due to trivial changes (1)
- plugins/maxim/go.mod
🚧 Files skipped from review as they are similar to previous changes (7)
- transports/bifrost-http/lib/ctx.go
- ui/lib/types/plugins.ts
- docs/docs.json
- plugins/maxim/plugin_test.go
- transports/bifrost-http/main.go
- ui/app/config/views/pluginsForm.tsx
- docs/features/observability.mdx
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/maxim/main.go (3)
transports/bifrost-http/lib/config.go (1)
Config(113-136)ui/lib/types/plugins.ts (1)
Plugin(6-10)transports/bifrost-http/lib/ctx.go (1)
ContextKey(68-68)
⏰ 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). (26)
- 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 (5)
docs/quickstart/go-sdk/setting-up.mdx (1)
75-82: LGTM: Init(ctx, cfg) usage is correct and error is handled.plugins/maxim/main.go (2)
423-432: Cleanup is good; consider idempotency under concurrent calls.Current Flush loop is safe; no changes required.
116-135: Context key coupling: bifrost-http already writes maxim.ContextKey; verify other transports
- Found in transports/bifrost-http/lib/ctx.go (~lines 112–116): it calls context.WithValue(..., maxim.ContextKey(labelName), string(value)) when labelName == string(maxim.LogRepoIDKey), so ctx.Value(LogRepoIDKey) will be populated.
- Action: only add the proposed fallbacks (plain string keys, []string, "x-bf-maxim-log-repo-id"/"log-repo-id") if other transports set keys differently; scan remaining transports and apply the diff where needed.
ui/lib/types/config.ts (1)
216-216: enable_maxim is safe — backend doesn't expect a top-level field; UI uses plugins API
- Verified: enable_maxim only appears in ui/lib/types/config.ts (line 216); no Go structs or json tags reference it.
- Backend handles Maxim via plugin entries (transports/bifrost-http/main.go:418–436) and the plugins create/update handlers (transports/bifrost-http/handlers/plugins.go:93–129); encoding/json will ignore unknown JSON fields, so the extra UI flag won't cause 500s.
transports/go.mod (1)
80-80: Run go mod tidy and ensure github.com/maximhq/maxim-go v0.1.10 is pinned consistently
- transports/go.mod (line 80) already contains github.com/maximhq/maxim-go v0.1.10.
- go mod tidy produced changes that need committing: tests/core-chatbot (go.mod, go.sum), tests/core-providers (touched tests/core-chatbot files), and transports/go.sum — run go mod tidy in those module dirs and commit the updated go.mod/go.sum files.
- If you intended to bump maxim-go across modules, add an explicit require to the affected go.mod files and re-run go mod tidy.
TejasGhatte
force-pushed
the
09-13-enhancement_configuration_updates_in_maxim_plugin
branch
from
37bc39a to
c81c8bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/features/plugins/mocker.mdx (1)
33-39: Fix incorrect error variable check after InitYou assign Init’s error to initErr but check err, which will skip handling Init failures and can lead to nil client usage in copy‑pasted code.
Apply this diff:
- client, initErr := bifrost.Init(context.Background(), schemas.BifrostConfig{ + client, err := bifrost.Init(context.Background(), schemas.BifrostConfig{ Account: &yourAccount, Plugins: []schemas.Plugin{plugin}, }) - if err != nil { + if err != nil { panic(err) }plugins/maxim/main.go (1)
251-285: Nil‑pointer risk: req.Input is dereferenced without a nil check.
req.Input.ChatCompletionInput/TextCompletionInputare accessed even ifreq.Inputis nil, causing a panic.Apply:
- if req.Input.ChatCompletionInput != nil { + if req.Input != nil && req.Input.ChatCompletionInput != nil { ... - } else if req.Input.TextCompletionInput != nil { + } else if req.Input != nil && req.Input.TextCompletionInput != nil { ... - } + } else { + requestType = "unknown" + latestMessage = "-" + }
♻️ Duplicate comments (1)
plugins/maxim/main.go (1)
116-135: Single‑repo selection is clear; ensure PR/docs don’t claim multi‑repo fan‑out.This function enforces a single effective repo (header > default), which contradicts the PR summary claiming multi‑repo via repeated headers. If single‑repo is the intended v1 behavior, update PR text/docs accordingly.
Additionally, make lookups resilient to transports that store plain string keys (not this package’s key type). Suggested patch:
func (plugin *Plugin) getEffectiveLogRepoID(ctx *context.Context) string { // Check for header log repo ID first (highest priority) if ctx != nil { - if headerRepoID, ok := (*ctx).Value(LogRepoIDKey).(string); ok && headerRepoID != "" { - return headerRepoID - } + if headerRepoID, ok := (*ctx).Value(LogRepoIDKey).(string); ok && headerRepoID != "" { + return headerRepoID + } + // Fallbacks for transports using plain string keys or multi-value headers + for _, k := range []any{"log-repo-id", "x-bf-maxim-log-repo-id"} { + if v := (*ctx).Value(k); v != nil { + switch t := v.(type) { + case string: + if t != "" { + return t + } + case []string: + // pick the first non-empty if provided multiple times + for _, s := range t { + if s != "" { + return s + } + } + } + } + } }
🧹 Nitpick comments (4)
docs/features/plugins/mocker.mdx (2)
127-132: Use a consistent error variable name for InitFor consistency with other examples and to reduce copy‑paste mistakes, prefer err over initErr here.
- client, initErr := bifrost.Init(context.Background(), schemas.BifrostConfig{ + client, err := bifrost.Init(context.Background(), schemas.BifrostConfig{ Account: &yourAccount, Plugins: []schemas.Plugin{plugin}, Logger: bifrost.NewDefaultLogger(schemas.LogLevelInfo), })
479-484: Align error variable name and encourage checking itMirror the earlier snippet: use err for Init and (optionally) show an error check to prevent nil client usage when copied verbatim.
- client, initErr := bifrost.Init(context.Background(), schemas.BifrostConfig{ + client, err := bifrost.Init(context.Background(), schemas.BifrostConfig{ Account: &account, Plugins: []schemas.Plugin{plugin}, Logger: bifrost.NewDefaultLogger(schemas.LogLevelDebug), })Optionally:
if err != nil { log.Fatal(err) }plugins/maxim/main.go (2)
289-313: Avoid logging unbounded user text; trimlatestMessage.Limit the trace input to a small, fixed length to reduce PII/log bloat risk.
Apply:
- trace := logger.Trace(&traceConfig) - trace.SetInput(latestMessage) + trace := logger.Trace(&traceConfig) + trace.SetInput(truncate(latestMessage, 512))Add helper (anywhere in this file, e.g., after imports):
func truncate(s string, max int) string { if max <= 0 { return "" } r := []rune(s) if len(r) <= max { return s } return string(r[:max]) }
340-344: Minor: avoid duplicate logger lookups.You call
getOrCreateLoggerearlier (for trace) and again here. Cache the logger when available to shave an extra map lock/lookup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
docs/media/maxim-logs.pngis excluded by!**/*.pngdocs/media/ui-observability-config.pngis excluded by!**/*.pngplugins/maxim/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
docs/docs.json(1 hunks)docs/features/mcp.mdx(3 hunks)docs/features/observability.mdx(1 hunks)docs/features/plugins/jsonparser.mdx(3 hunks)docs/features/plugins/mocker.mdx(3 hunks)docs/quickstart/go-sdk/setting-up.mdx(1 hunks)docs/quickstart/go-sdk/tool-calling.mdx(1 hunks)plugins/maxim/README.md(0 hunks)plugins/maxim/go.mod(1 hunks)plugins/maxim/main.go(8 hunks)plugins/maxim/plugin_test.go(5 hunks)transports/bifrost-http/lib/ctx.go(1 hunks)transports/bifrost-http/main.go(1 hunks)transports/go.mod(1 hunks)ui/app/config/views/pluginsForm.tsx(9 hunks)ui/lib/types/config.ts(2 hunks)ui/lib/types/plugins.ts(1 hunks)
💤 Files with no reviewable changes (1)
- plugins/maxim/README.md
✅ Files skipped from review due to trivial changes (1)
- transports/go.mod
🚧 Files skipped from review as they are similar to previous changes (13)
- ui/lib/types/plugins.ts
- plugins/maxim/go.mod
- docs/docs.json
- docs/features/plugins/jsonparser.mdx
- plugins/maxim/plugin_test.go
- ui/lib/types/config.ts
- transports/bifrost-http/lib/ctx.go
- ui/app/config/views/pluginsForm.tsx
- docs/features/mcp.mdx
- docs/quickstart/go-sdk/tool-calling.mdx
- transports/bifrost-http/main.go
- docs/features/observability.mdx
- docs/quickstart/go-sdk/setting-up.mdx
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/maxim/main.go (3)
transports/bifrost-http/lib/config.go (1)
Config(113-136)ui/lib/types/plugins.ts (1)
Plugin(6-10)transports/bifrost-http/lib/ctx.go (1)
ContextKey(68-68)
⏰ 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). (15)
- 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 (5)
plugins/maxim/main.go (5)
18-27: Config shape and JSON tags look good.Fields are minimal, named well, and JSON tags match the UI/transport needs.
29-61: Init flow is clean and concurrency‑ready.Good: API key validation, SDK init, lazy per‑repo logger map with RWMutex, and optional default logger warm‑up.
137-165: Thread‑safe logger cache looks correct.Double‑checked RW locking and double‑check pattern; no data races apparent.
423-431: Cleanup flushes all loggers — good for shutdown.
64-80: No change required — maxim keys are used consistently; no fallback needed.transports/bifrost-http/lib/ctx.go writes maxim keys with maxim.ContextKey(...), and plugins/maxim/main.go (plus tests/docs) read maxim.LogRepoIDKey/TraceIDKey/etc.; no plain-string writes for these keys were found in code.
TejasGhatte
force-pushed
the
09-13-enhancement_configuration_updates_in_maxim_plugin
branch
from
c81c8bb to
4dbcbce
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/features/plugins/mocker.mdx (1)
33-39: Fix incorrect error variable check after InitThe snippet assigns
initErrfrombifrost.Init(...)but checkserr, which refers to the earlier plugin creation. This will miss Init errors.Apply this diff to make the check correct and keep naming consistent:
- // Initialize Bifrost with the plugin - client, initErr := bifrost.Init(context.Background(), schemas.BifrostConfig{ + // Initialize Bifrost with the plugin + client, err := bifrost.Init(context.Background(), schemas.BifrostConfig{ Account: &yourAccount, Plugins: []schemas.Plugin{plugin}, }) - if err != nil { - panic(err) + if err != nil { + panic(err) }
🧹 Nitpick comments (11)
docs/features/plugins/mocker.mdx (2)
127-132: Consistent var naming in examplesElsewhere you use
errfor Init; here it’sinitErr. Prefer a single convention across docs to reduce confusion.Suggested tweak:
-client, initErr := bifrost.Init(context.Background(), schemas.BifrostConfig{ +client, err := bifrost.Init(context.Background(), schemas.BifrostConfig{
479-484: Align debug-mode Init example with unified error varMatch the earlier suggestion for consistent
errnaming across snippets.-client, initErr := bifrost.Init(context.Background(), schemas.BifrostConfig{ +client, err := bifrost.Init(context.Background(), schemas.BifrostConfig{plugins/maxim/main.go (6)
46-49: Use ID over Id for Go identifiers (consistency).Prefer “ID” instead of “Id” in identifier names per Go conventions.
- defaultLogRepoId: config.LogRepoId, + defaultLogRepoID: config.LogRepoId,-type Plugin struct { - mx *maxim.Maxim - defaultLogRepoId string - loggers map[string]*logging.Logger - loggerMutex *sync.RWMutex -} +type Plugin struct { + mx *maxim.Maxim + defaultLogRepoID string + loggers map[string]*logging.Logger + loggerMutex *sync.RWMutex +}- if plugin.defaultLogRepoId != "" { - return plugin.defaultLogRepoId + if plugin.defaultLogRepoID != "" { + return plugin.defaultLogRepoID }Also applies to: 104-109
120-135: Accept []string in context for repeated header sources (pick first).Transports may store repeated headers as []string. Safely support both string and []string without enabling multi-repo fan‑out.
func (plugin *Plugin) getEffectiveLogRepoID(ctx *context.Context) string { // Check for header log repo ID first (highest priority) if ctx != nil { - if headerRepoID, ok := (*ctx).Value(LogRepoIDKey).(string); ok && headerRepoID != "" { - return headerRepoID - } + if headerRepoID, ok := (*ctx).Value(LogRepoIDKey).(string); ok && headerRepoID != "" { + return headerRepoID + } + if headerRepoIDs, ok := (*ctx).Value(LogRepoIDKey).([]string); ok && len(headerRepoIDs) > 0 && headerRepoIDs[0] != "" { + return headerRepoIDs[0] + } } // Fall back to default log repo ID from config - if plugin.defaultLogRepoId != "" { - return plugin.defaultLogRepoId + if plugin.defaultLogRepoID != "" { + return plugin.defaultLogRepoID } // Return empty string if neither header nor default is available return "" }
187-201: Guard against nil req/input to avoid panics.Defensive check before dereferencing req and req.Input.
// If no log repo ID available, skip logging if effectiveLogRepoID == "" { return req, nil, nil } + // Defensive guards + if req == nil || req.Input == nil { + return req, nil, nil + }Also applies to: 248-285
307-313: Truncate/sanitize trace input to bound payload size.Avoid sending arbitrarily large prompt text in SetInput; cap to a small, safe size.
- trace := logger.Trace(&traceConfig) - trace.SetInput(latestMessage) + trace := logger.Trace(&traceConfig) + trace.SetInput(truncateString(latestMessage, 2048))Add helper (outside this hunk):
// truncateString returns s limited to max runes (not bytes) with an ellipsis. func truncateString(s string, max int) string { if max <= 0 || len(s) == 0 { return s } r := []rune(s) if len(r) <= max { return s } return string(r[:max]) + "…" }
315-323: Handle JSON marshal errors explicitly (don’t silently ignore).Current code silently ignores Marshal/Unmarshal errors. At minimum, avoid populating a partially decoded map.
- jsonData, err := json.Marshal(req.Params) - if err == nil { - json.Unmarshal(jsonData, &modelParams) - } + if jsonData, err := json.Marshal(req.Params); err == nil { + _ = json.Unmarshal(jsonData, &modelParams) + } else { + // leave modelParams empty on error + }
45-61: Potential unbounded logger cache growth.loggers map grows per distinct repo ID and never evicts. Consider a size cap or TTL/LRU if high cardinality is expected.
docs/features/observability.mdx (3)
59-61: Fix wording: JSON snippet isn’t “environment variables.”Adjust caption to match the JSON example.
-For HTTP transport, configure via environment variables: +For HTTP transport, configure via JSON (or equivalent environment variables):
88-95: Clarify single‑repo behavior to avoid confusion.Add an explicit note that only one repository is selected per request.
The plugin uses repository selection with the following priority: 1. **Header/Context Repository** - Highest priority 2. **Default Repository** (from plugin config) - Fallback 3. **Skip Logging** - If neither is available + +Note: The Maxim plugin logs to a single repository per request. Repeated +`x-bf-maxim-log-repo-id` headers are not aggregated; the transport should +select a single effective repository.
219-220: Remove duplicated bullet item.“Cost Analytics” appears twice.
- - **Cost Analytics**: Per-request and aggregate cost tracking
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
docs/media/maxim-logs.pngis excluded by!**/*.pngdocs/media/ui-observability-config.pngis excluded by!**/*.pngplugins/maxim/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
docs/docs.json(1 hunks)docs/features/mcp.mdx(3 hunks)docs/features/observability.mdx(1 hunks)docs/features/plugins/jsonparser.mdx(3 hunks)docs/features/plugins/mocker.mdx(3 hunks)docs/quickstart/go-sdk/setting-up.mdx(1 hunks)docs/quickstart/go-sdk/tool-calling.mdx(1 hunks)plugins/maxim/README.md(0 hunks)plugins/maxim/go.mod(1 hunks)plugins/maxim/main.go(8 hunks)plugins/maxim/plugin_test.go(5 hunks)transports/bifrost-http/lib/ctx.go(1 hunks)transports/bifrost-http/main.go(1 hunks)transports/go.mod(1 hunks)ui/app/config/views/pluginsForm.tsx(9 hunks)ui/lib/types/config.ts(2 hunks)ui/lib/types/plugins.ts(1 hunks)
💤 Files with no reviewable changes (1)
- plugins/maxim/README.md
🚧 Files skipped from review as they are similar to previous changes (13)
- plugins/maxim/go.mod
- docs/docs.json
- transports/bifrost-http/lib/ctx.go
- transports/go.mod
- docs/quickstart/go-sdk/setting-up.mdx
- ui/lib/types/plugins.ts
- ui/lib/types/config.ts
- plugins/maxim/plugin_test.go
- ui/app/config/views/pluginsForm.tsx
- docs/features/mcp.mdx
- docs/quickstart/go-sdk/tool-calling.mdx
- transports/bifrost-http/main.go
- docs/features/plugins/jsonparser.mdx
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/maxim/main.go (3)
transports/bifrost-http/lib/config.go (1)
Config(113-136)ui/lib/types/plugins.ts (1)
Plugin(6-10)transports/bifrost-http/lib/ctx.go (1)
ContextKey(68-68)
⏰ 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). (3)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (5)
plugins/maxim/main.go (3)
412-417: LGTM: per-request flush limited to effective logger.Good improvement vs. flushing all repos on every request.
424-433: LGTM: Cleanup flushes all cached loggers.Appropriate for shutdown paths.
69-80: Unverified — confirm transports use maxim.ContextKey constants for ctx valuesAutomated search returned no matches; cannot verify that context.WithValue calls use maxim.SessionIDKey / TraceIDKey / TraceNameKey / GenerationIDKey / GenerationNameKey / TagsKey / LogRepoIDKey rather than raw string literals. Inspect transports and store code (start with plugins/maxim/main.go, lines 69–80) and replace any context.WithValue(..., "trace-id" | "session-id" | "generation-id" | "trace-name" | "generation-name" | "maxim-tags" | "log-repo-id") with the corresponding maxim.. Re-run the ripgrep from the original comment and attach the output for re-check.
docs/features/observability.mdx (2)
96-121: LGTM: Repository selection examples align with single‑repo implementation.Examples are consistent with the code’s behavior.
42-51: No change — Shutdown() is parameterless.core/bifrost.go defines
func (bifrost *Bifrost) Shutdown()at line 1546;defer client.Shutdown()in the docs is correct.
Merge activity
|
akshaydeo
changed the base branch from
09-16-feat_api_key_support_added_for_bedrock_provider
to
graphite-base/471
akshaydeo
deleted the
09-13-enhancement_configuration_updates_in_maxim_plugin
branch
## Summary
Enhance the Maxim plugin to support multiple log repositories per request and improve configuration management.
## Changes
- Renamed `NewMaximLoggerPlugin` to `Init` for consistency with other plugins
- Added support for multiple log repositories per request via headers
- Implemented thread-safe logger management with a map and mutex
- Added support for embedding, speech, and transcription inputs
- Improved error handling and logging
- Updated UI to support Maxim plugin configuration
- Added proper configuration structure with JSON tags
## Type of change
- [ ] Bug fix
- [x] Feature
- [x] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [x] UI (Next.js)
- [ ] Docs
## How to test
Test the Maxim plugin with multiple log repositories:
```sh
# Set up environment variables
export MAXIM_API_KEY=your_api_key
export MAXIM_LOGGER_ID=your_logger_id
# Run tests
go test ./plugins/maxim/...
# Test with HTTP headers
curl -X POST http://localhost:8000/v1/chat/completions \
-H "Content-Type: application/json" \
-H "x-bf-maxim-log-repo-id: repo1" \
-H "x-bf-maxim-log-repo-id: repo2" \
-d '{"model": "gpt-3.5-turbo", "messages": [{"role": "user", "content": "Hello"}]}'
```
## Breaking changes
- [x] Yes
- [ ] No
The plugin initialization function has been renamed from `NewMaximLoggerPlugin` to `Init` and now accepts a `Config` struct instead of individual parameters. Update any direct calls to this function.
## Related issues
Improves logging capabilities and configuration management for the Maxim plugin.
## Security considerations
The plugin handles API keys which should be kept secure. Configuration is now properly structured to avoid exposing sensitive information.
## Checklist
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
2 participants
Summary
Enhance the Maxim plugin to support multiple log repositories per request and improve configuration management.
Changes
NewMaximLoggerPlugintoInitfor consistency with other pluginsType of change
Affected areas
How to test
Test the Maxim plugin with multiple log repositories:
Breaking changes
The plugin initialization function has been renamed from
NewMaximLoggerPlugintoInitand now accepts aConfigstruct instead of individual parameters. Update any direct calls to this function.Related issues
Improves logging capabilities and configuration management for the Maxim plugin.
Security considerations
The plugin handles API keys which should be kept secure. Configuration is now properly structured to avoid exposing sensitive information.
Checklist