feat: plugin schemas segregation for mcp plugins#1299
feat: plugin schemas segregation for mcp plugins#1299Pratham-Mishra04 wants to merge 2 commits intomainfrom
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis PR separates LLM and MCP plugin domains, introduces domain-specific hook names and per-domain plugin pipelines, unifies MCP tool execution around BifrostMCPRequest/BifrostMCPResponse, updates plugin loader/implementations, moves MCP inference to a dedicated handler, and removes the interactive chatbot test harness. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant Handler as MCPInferenceHandler
participant Bifrost as Bifrost Core
participant Pipeline as PluginPipeline
participant ToolMgr as ToolsManager
participant MCP as MCP Manager
Client->>Handler: POST /v1/mcp/tool/execute (chat/responses)
Handler->>Handler: Parse & validate request
Handler->>Bifrost: ConvertToBifrostContext()
Handler->>Bifrost: ExecuteChatMCPTool / ExecuteResponsesMCPTool
Bifrost->>Bifrost: getMCPRequest() (from pool)
Bifrost->>Pipeline: getPluginPipeline()
Pipeline-->>Bifrost: PluginPipeline
Bifrost->>Pipeline: RunMCPPreHooks(ctx, request)
alt Short-circuit
Pipeline-->>Bifrost: short-circuit response
else Normal flow
Bifrost->>ToolMgr: ExecuteTool(ctx, mcpRequest)
ToolMgr->>MCP: ExecuteToolCall()
MCP-->>ToolMgr: BifrostMCPResponse
ToolMgr-->>Bifrost: BifrostMCPResponse
Bifrost->>Pipeline: RunMCPPostHooks(ctx, response)
Pipeline-->>Bifrost: Final response
end
Bifrost->>Bifrost: releaseMCPRequest()
Bifrost->>Pipeline: releasePluginPipeline()
Bifrost-->>Handler: result
Handler->>Client: JSON response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (16)
transports/bifrost-http/lib/config.go (1)
2268-2366: UpdateGetLoadedPlugins()call site to useGetLoadedLLMPlugins()for API consistency. TheAddLoadedPlugin/RemoveLoadedPlugin2-arg APIs are fully migrated across all call sites. However, there's one straggler attransports/bifrost-http/server/server.go:716that uses the legacyGetLoadedPlugins()method. While it still works (delegated for backward compatibility), it should be updated toGetLoadedLLMPlugins()to align with the new plugin type-keyed API design.docs/features/plugins/mocker.mdx (1)
33-39: Fix doc snippet error handling (initErrvserr) while updating toLLMPlugins.Good update to the new config surface (
LLMPlugins). However, in the first snippet you assigninitErrbut checkerr(Line 33-39), which would confuse readers.Proposed doc fix
- client, initErr := bifrost.Init(context.Background(), schemas.BifrostConfig{ + client, err := bifrost.Init(context.Background(), schemas.BifrostConfig{ Account: &yourAccount, LLMPlugins: []schemas.LLMPlugin{plugin}, }) - if err != nil { - panic(err) - } + if err != nil { panic(err) }Also applies to: 165-170, 561-566
transports/bifrost-http/handlers/middlewares.go (1)
52-90: Stop the middleware chain if a plugin illegally changes method/path.Right now, if
applyHTTPRequestToCtxdetects method/path mismatch it writes an error and returns (Line 123-128), butTransportInterceptorMiddlewarestill proceeds to set user values and callnext(ctx)(Line 83-89). That can lead to handlers running after an error response was already written.Proposed fix
- applyHTTPRequestToCtx(ctx, req) + if ok := applyHTTPRequestToCtx(ctx, req); !ok { + return + } // Adding user values for key, value := range bifrostCtx.GetUserValues() { ctx.SetUserValue(key, value) } next(ctx)-func applyHTTPRequestToCtx(ctx *fasthttp.RequestCtx, req *schemas.HTTPRequest) { +func applyHTTPRequestToCtx(ctx *fasthttp.RequestCtx, req *schemas.HTTPRequest) bool { // If path/method is different, throw error if req.Method != string(ctx.Method()) || req.Path != string(ctx.Path()) { logger.Error("request method/path mismatch: %s %s != %s %s", req.Method, req.Path, string(ctx.Method()), string(ctx.Path())) SendError(ctx, fasthttp.StatusConflict, "request method/path was modified by a plugin, this is not allowed") - return + return false } // Apply headers for key, value := range req.Headers { ctx.Request.Header.Set(key, value) } // Apply query params for key, value := range req.Query { ctx.Request.URI().QueryArgs().Set(key, value) } // Apply body if set if req.Body != nil { ctx.Request.SetBody(req.Body) } + return true }plugins/maxim/plugin_test.go (1)
86-127: MakeTestMaximLoggerPluginskip (not fail) when required env vars are missing.As written, CI will fail unless
MAXIM_API_KEY(and realisticallyOPENAI_API_KEY) are always present. Prefert.Skipfor env-gated integration tests.Proposed change
func TestMaximLoggerPlugin(t *testing.T) { + if os.Getenv("MAXIM_API_KEY") == "" || os.Getenv("OPENAI_API_KEY") == "" { + t.Skip("skipping integration test: MAXIM_API_KEY and/or OPENAI_API_KEY not set") + } ctx := context.Background() // Initialize the Maxim plugin plugin, err := getPlugin() if err != nil { t.Fatalf("Error setting up the plugin: %v", err) }docs/features/governance/virtual-keys.mdx (1)
491-505: Clarify “mandatory virtual keys” vs “mandatoryx-bf-vk”.Line 493/Line 543 implies enforcement requires
x-bf-vk, but later you document VK-in-Authorization/x-api-keyflows (auth disabled). Consider rewording to “must include a virtual key in one of the supported VK headers” (and explicitly call out that when auth is enabled,Authorizationis reserved for auth so VK must bex-bf-vk).Proposed doc tweak
-All governance-enabled requests must include the virtual key header: +All governance-enabled requests must include a virtual key (in a supported virtual-key header): ... -When the governance header is enforced, the request will be rejected if the `x-bf-vk` header is not present. +When enforced, the request will be rejected if no virtual key is present in the request headers. +If authentication is enabled, `Authorization` is used for auth, so provide the virtual key via `x-bf-vk`.Also applies to: 543-543
docs/plugins/writing-go-plugin.mdx (1)
124-178: v1.3.x tab still documents v1.4+ hook names/types.If v1.3.x actually used
PreHook/PostHookandPluginShortCircuit, this tab should not showPreLLMHook/PostLLMHook+LLMPluginShortCircuit. Otherwise users on v1.3 will implement the wrong exported symbols and fail to load.Suggested doc correction (v1.3.x)
-func PreLLMHook(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) (*schemas.BifrostRequest, *schemas.LLMPluginShortCircuit, error) { +func PreHook(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) (*schemas.BifrostRequest, *schemas.PluginShortCircuit, error) { ... -func PostLLMHook(ctx *schemas.BifrostContext, resp *schemas.BifrostResponse, bifrostErr *schemas.BifrostError) (*schemas.BifrostResponse, *schemas.BifrostError, error) { +func PostHook(ctx *schemas.BifrostContext, resp *schemas.BifrostResponse, bifrostErr *schemas.BifrostError) (*schemas.BifrostResponse, *schemas.BifrostError, error) {plugins/semanticcache/main.go (1)
343-355: Update PreLLMHook doc comment (return type changed).The comment still describes returning a cached
*schemas.BifrostResponse, but the function now returns*schemas.LLMPluginShortCircuit. Please update the “Returns” section to match the new contract.framework/plugins/soplugin_test.go (1)
141-158: Rename test case label to match PostLLMHook.
"PostHook_WithError"now exercisesPostLLMHook; worth renaming for grep-ability and consistency.plugins/mocker/main.go (2)
765-887: Potential nil deref in Responses mode when overriding model.In
generateSuccessShortCircuit, if the request isschemas.ResponsesRequest,mockResponse.ChatResponseis nil; the block:if content.Model != nil { mockResponse.ChatResponse.Model = *content.Model }will panic.
Proposed fix
- // Override model if specified - if content.Model != nil { - mockResponse.ChatResponse.Model = *content.Model - } + // Override model if specified + if content.Model != nil { + if mockResponse.ChatResponse != nil { + mockResponse.ChatResponse.Model = *content.Model + } + // Optional: also reflect it for ResponsesResponse if your schema supports it + // (otherwise omit / keep ModelRequested only) + }
992-1041: DefaultBehaviorSuccess returns chat-shaped response even for ResponsesRequest.
PreLLMHookexplicitly supportsschemas.ResponsesRequest, buthandleDefaultBehavior’s success branch always constructsChatResponse. That’s likely an API-shape mismatch for callers using the Responses API.Suggested direction
case DefaultBehaviorSuccess: finishReason := "stop" - return req, &schemas.LLMPluginShortCircuit{ - Response: &schemas.BifrostResponse{ - ChatResponse: &schemas.BifrostChatResponse{ ... }, - }, - }, nil + if req.RequestType == schemas.ResponsesRequest { + return req, &schemas.LLMPluginShortCircuit{ + Response: &schemas.BifrostResponse{ + ResponsesResponse: &schemas.BifrostResponsesResponse{ + CreatedAt: int(time.Now().Unix()), + Output: []schemas.ResponsesMessage{{ + Role: bifrost.Ptr(schemas.ResponsesInputMessageRoleAssistant), + Content: &schemas.ResponsesMessageContent{ContentStr: bifrost.Ptr("Mock plugin default response")}, + Type: bifrost.Ptr(schemas.ResponsesMessageTypeMessage), + }}, + }, + }, + }, nil + } + return req, &schemas.LLMPluginShortCircuit{ + Response: &schemas.BifrostResponse{ + ChatResponse: &schemas.BifrostChatResponse{ ... }, + }, + }, nilcore/mcp/codemodeexecutecode.go (1)
372-376: Pass timeoutCtx (with proper BifrostContext type) to callMCPTool to align timeout budgets.The issue is real and critical. Line 372 creates
timeoutCtxusingcontext.WithTimeout(), which returns a standardcontext.Context, not a*schemas.BifrostContext. When line 520 passes the originalctxtocallMCPTool, the function type-asserts it to*schemas.BifrostContext; if the assertion succeeds and the context has no deadline,callMCPToolcreates a freshnestedCtxfromcontext.Background(), then a fresh timeout context later. This independent timeout allows tool execution to continue after the VM'stimeoutCtxhas expired, creating a resource-leak and reliability risk (in-flight MCP calls after user request timeout).The suggested fix is correct: use
NewBifrostContextWithTimeoutat line 372 to create a timeout context that remains a*BifrostContext, then pass it (not the originalctx) tocallMCPTool. This ensures the type-assertion succeeds, the deadline properly cascades throughnestedCtx, and timeout handling stays unified.Suggested direction
- timeoutCtx, cancel := context.WithTimeout(ctx, toolExecutionTimeout) + // Preserve BifrostContext type for proper type-assertion in callMCPTool + timeoutCtx, cancel := schemas.NewBifrostContextWithTimeout(ctx, toolExecutionTimeout) defer cancel() ... - result, err := m.callMCPTool(ctx, clientNameFinal, toolNameFinal, argsMap, appendLog) + result, err := m.callMCPTool(timeoutCtx, clientNameFinal, toolNameFinal, argsMap, appendLog)framework/plugins/soplugin.go (1)
9-24: Guard against nil function pointers (panic risk) and fix misleading comments.
GetName,PreLLMHook,PostLLMHook,Cleanupall call function fields without nil checks; unlikeHTTPTransportIntercept, these will panic if the symbol wasn’t loaded. Also the “not used” comments don’t match behavior.Proposed defensive guards
func (dp *DynamicLLMPlugin) GetName() string { - return dp.getName() + if dp.getName == nil { + return "" + } + return dp.getName() } func (dp *DynamicLLMPlugin) PreLLMHook(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) (*schemas.BifrostRequest, *schemas.LLMPluginShortCircuit, error) { - return dp.preHook(ctx, req) + if dp.preHook == nil { + return req, nil, nil + } + return dp.preHook(ctx, req) } func (dp *DynamicLLMPlugin) PostLLMHook(ctx *schemas.BifrostContext, resp *schemas.BifrostResponse, bifrostErr *schemas.BifrostError) (*schemas.BifrostResponse, *schemas.BifrostError, error) { - return dp.postHook(ctx, resp, bifrostErr) + if dp.postHook == nil { + return resp, bifrostErr, nil + } + return dp.postHook(ctx, resp, bifrostErr) } func (dp *DynamicLLMPlugin) Cleanup() error { - return dp.cleanup() + if dp.cleanup == nil { + return nil + } + return dp.cleanup() }Also applies to: 27-52
plugins/semanticcache/search.go (1)
161-208: TTL expiry deletion spawns goroutines on read-path (potential load amplifier).
If many expired entries are hit, this will create many short-lived goroutines and background deletes; consider a bounded worker / queue, or doing best-effort synchronous delete with a small timeout when QPS is low.core/mcp/toolmanager.go (1)
398-482: Be tolerant of empty/absentArgumentsfor tool calls.
json.Unmarshal([]byte(toolCall.Function.Arguments), ...)will fail on empty string; many tool-call producers use""for “no args”. Consider treating""as{}.transports/bifrost-http/server/server.go (1)
1039-1126: Avoid panics onisDisabled.(bool)in RemovePlugin.
ctx.Value("isDisabled")may be nil or non-bool depending on caller; use anoktype assertion.Proposed fix
// Update plugin status isDisabled := ctx.Value("isDisabled") - if isDisabled != nil && isDisabled.(bool) { + if v, ok := isDisabled.(bool); ok && v { s.UpdatePluginStatus(name, schemas.PluginStatusDisabled, []string{fmt.Sprintf("plugin %s is disabled", name)}) } else { // Plugin is being deleted - remove the status entry completely s.DeletePluginStatus(name) }core/bifrost.go (1)
2736-2826: Post-hook “runFrom” must not depend on current global plugin length (hot-reload correctness).
Usinglen(*bifrost.llmPlugins.Load())can under/over-run post-hooks if plugins are added/removed concurrently; preferpreCount(fromRunLLMPreHooks) orlen(pipeline.llmPlugins)for the pipeline you’re executing with.Proposed fix (non-stream + error recovery + streaming runner)
- preReq, shortCircuit, preCount := pipeline.RunLLMPreHooks(ctx, req) + preReq, shortCircuit, preCount := pipeline.RunLLMPreHooks(ctx, req) ... - pluginCount := len(*bifrost.llmPlugins.Load()) select { case result = <-msg.Response: - resp, bifrostErr := pipeline.RunPostHooks(msg.Context, result, nil, pluginCount) + resp, bifrostErr := pipeline.RunPostHooks(msg.Context, result, nil, preCount) ... case bifrostErrVal := <-msg.Err: bifrostErrPtr := &bifrostErrVal - resp, bifrostErrPtr = pipeline.RunPostHooks(msg.Context, nil, bifrostErrPtr, pluginCount) + resp, bifrostErrPtr = pipeline.RunPostHooks(msg.Context, nil, bifrostErrPtr, preCount) ... }- recoveredResp, recoveredErr := pipeline.RunPostHooks(ctx, nil, &bifrostErrVal, len(*bifrost.llmPlugins.Load())) + recoveredResp, recoveredErr := pipeline.RunPostHooks(ctx, nil, &bifrostErrVal, preCount)postHookRunner = func(ctx *schemas.BifrostContext, result *schemas.BifrostResponse, err *schemas.BifrostError) (*schemas.BifrostResponse, *schemas.BifrostError) { - resp, bifrostErr := pipeline.RunPostHooks(ctx, result, err, len(*bifrost.llmPlugins.Load())) + resp, bifrostErr := pipeline.RunPostHooks(ctx, result, err, len(pipeline.llmPlugins)) if bifrostErr != nil { return nil, bifrostErr } return resp, nil }Also applies to: 3003-3006, 3255-3265
🤖 Fix all issues with AI agents
In @core/mcp/agent.go:
- Around line 293-309: The goroutine calling executeToolFunc can return a
non-nil mcpResponse with a nil ChatMessage and currently sends nothing to
channelToolResults, which causes the collector to miss a result; inside the
anonymous func (around executeToolFunc, mcpResponse, toolErr handling) add an
explicit send to channelToolResults when mcpResponse is non-nil but
mcpResponse.ChatMessage is nil—use createToolResultMessage(toolCall, "", nil) or
a similar empty/result placeholder so every toolCall always sends exactly one
result; ensure wg.Done() and error logging behavior remain unchanged.
In @core/mcp/codemodeexecutecode.go:
- Around line 882-907: Pre-hook modifications to toolCall
(preReq.ChatAssistantMessageToolCall -> toolCall) are ignored because
callRequest.Params.Name is set from originalToolName computed before hooks;
update the code to derive the tool name from toolCall.Function.Name after
applying pre-hooks (falling back to originalToolName if empty) and use that
value when constructing mcp.CallToolRequest.Params.Name (refer to toolCall,
toolCall.Function.Name, originalToolName, and callRequest).
- Around line 795-825: The nested context is rooted at context.Background() so
parent cancellation/cancelation propagation is lost; replace
schemas.NewBifrostContext(context.Background(), deadline) with
schemas.NewBifrostContext(bifrostCtx, deadline) (and do the same in the other
occurrence around the 909-915 region) and ensure you call
nestedCtx.BlockRestrictedWrites() (or the existing BlockRestrictedWrites
mechanism) immediately after creation to prevent inheriting plugin-reserved keys
while still preserving parent cancellation and deadlines; keep the subsequent
SetValue copies as-is for tracer/trace IDs and request IDs.
In @core/mcp/toolmanager.go:
- Around line 332-396: Guard against nil panics by validating the incoming
request and nested fields before using them: at the top of ExecuteTool check
request != nil and return an error if nil; when handling
MCPRequestTypeResponsesToolCall ensure request.ResponsesToolMessage != nil
before calling ToChatAssistantMessageToolCall and handle a nil conversion
result; after obtaining toolCall validate toolCall != nil and toolCall.Function
!= nil before checking Function.Name (and still check Function.Name != nil);
likewise, when returning Responses format validate chatResult != nil and that
chatResult.ToResponsesToolMessage() != nil and return a clear error if
conversion fails; keep the existing request.RequestType switch but perform these
nil checks early to avoid panics.
In @core/utils.go:
- Around line 418-421: IsCodemodeTool is implemented but unused; replace the
duplicated direct comparisons against mcp.ToolTypeListToolFiles,
mcp.ToolTypeReadToolFile, and mcp.ToolTypeExecuteToolCode wherever they appear
(the MCP agent and tool manager code paths that currently use direct string
equality checks) by calling IsCodemodeTool(toolName), or if you prefer to keep
the code inline, remove the unused IsCodemodeTool function to avoid dead code;
ensure all places that previously compared those three constants now delegate to
IsCodemodeTool to consolidate logic, or delete the function if consolidation is
not applied.
In @docs/features/governance/virtual-keys.mdx:
- Around line 589-592: The UI path is inconsistent: replace "Settings →
Security" with "Config → Security" in the step that references toggling "Disable
Auth on Inference" (and update the nearby block that shows "Go to Settings →
Security" to "Go to Config → Security"), and scan the other occurrences around
the earlier block (referenced in the comment as lines 510-513) to make the same
replacement so both mentions use "Config → Security" consistently.
- Around line 565-570: Replace the real-looking bearer token in the "Using
Authorization header (OpenAI style)" example—change the header value currently
shown as "Authorization: Bearer sk-bf-your-key" to a safe placeholder such as
"Authorization: Bearer <VIRTUAL_KEY>" or "Authorization: Bearer
<PROVIDER_API_KEY>"; keep any actual provider prefixes like "sk-bf-..." only in
explanatory prose, not inside header examples or copyable snippets (update the
snippet in the Using Authorization header (OpenAI style) section accordingly).
In @plugins/maxim/main.go:
- Around line 58-60: Doc comment for Init contains a typo: change the malformed
type name `schemas.LLMPlugingin` to the correct `schemas.LLMPlugin` in the
function comment above Init so the documented return signature matches the
actual return type; update the comment line that lists the return values to
reference `schemas.LLMPlugin` and keep the rest of the comment unchanged.
In @transports/bifrost-http/lib/config.go:
- Around line 2200-2238: GetLoadedLLMPlugins, GetLoadedMCPPlugins,
GetLoadedPlugins and GetLoadedHTTPTransportPlugins return shared snapshot slices
that callers must not mutate; add a short doc comment on each getter stating "Do
not modify the returned slice; it is a shared snapshot and must be treated
read-only" and, if you prefer safety over allocations, optionally change the
return to return a defensive copy (e.g., slices.Clone) before returning to avoid
callers corrupting the shared atomic snapshot.
In @transports/bifrost-http/server/server.go:
- Around line 825-900: SyncLoadedPlugin currently unlocks PluginsMutex before
calling Client.ReloadConfig and passes s.LLMPlugins/s.MCPPlugins which can be
mutated concurrently; under PluginsMutex lock make shallow copies of
s.LLMPlugins and s.MCPPlugins and pass those copies to ReloadConfig to avoid
races, and additionally address the broken plugin reload in Bifrost.ReloadConfig
(in core/bifrost.go) which currently ignores plugin fields — either implement
handling of LLMPlugins/MCPPlugins there to update the runtime plugin lists or
use a different Client API that is intended for hot-reloading plugins; ensure
UpdatePluginStatus/error paths remain unchanged and keep Config.AddLoadedPlugin
usage for persistent config updates.
🧹 Nitpick comments (5)
transports/bifrost-http/lib/config.go (2)
236-240: Consider initializing all plugin pointers consistently (or document nil-as-empty). Right nowLoadConfigexplicitly setsLLMPluginsbut leavesMCPPlugins/HTTPTransportPluginsas zero-values; functionally fine, but it’s easy for future readers to assume they’re always initialized the same way.Also applies to: 308-313
2240-2266: Approach is sound; HTTPTransport is confirmed as a capability-only feature. All plugins in the codebase that implementHTTPTransportPluginare registered as eitherLLMPluginorMCPPlugin(their primary type). There is no registration mechanism for HTTPTransport-only plugins—plugins must be added viaAddLoadedPlugin()withPluginTypeLLMorPluginTypeMCP. The caching strategy is correct for this architecture.Consider adding a clarifying comment to
rebuildHTTPTransportPlugins()stating that "HTTPTransport is a capability feature, not a primary plugin type; all HTTP transport-capable plugins are registered via LLM or MCP plugin sets" to prevent future confusion about the filtering logic.core/schemas/mcp.go (1)
26-33: Consider using a concrete type instead ofinterface{}for type safety.The
PluginPipelineProviderandReleasePluginPipelinefields useinterface{}for the pipeline type, which eliminates compile-time type checking. If the pipeline type is known (e.g.,*PluginPipelineor similar), consider using the concrete type to prevent runtime type assertion failures.If decoupling is required, consider defining a minimal interface that the pipeline must satisfy.
Example refactor with typed interface
+// PluginPipeline represents a pipeline for executing plugin hooks (define in appropriate location) +type PluginPipeline interface { + RunMCPPreHooks(ctx *BifrostContext, req *BifrostMCPRequest) (*BifrostMCPRequest, error) + RunMCPPostHooks(ctx *BifrostContext, resp *BifrostMCPResponse) (*BifrostMCPResponse, error) +} type MCPConfig struct { // ... existing fields ... - PluginPipelineProvider func() interface{} `json:"-"` + PluginPipelineProvider func() PluginPipeline `json:"-"` - ReleasePluginPipeline func(pipeline interface{}) `json:"-"` + ReleasePluginPipeline func(pipeline PluginPipeline) `json:"-"` }core/mcp/mcp.go (1)
83-101: Plugin pipeline type assertion silently returns nil on failure.At line 90-92, when the type assertion
pipeline.(PluginPipeline)fails, the function silently returnsnil. This could mask configuration issues where a non-compliant pipeline provider is passed.Consider logging a warning when the type assertion fails to aid debugging:
🔧 Suggested improvement
pluginPipelineProvider = func() PluginPipeline { if pipeline := config.PluginPipelineProvider(); pipeline != nil { if pp, ok := pipeline.(PluginPipeline); ok { return pp + } else { + logger.Warn(fmt.Sprintf("%s Plugin pipeline provider returned incompatible type", MCPLogPrefix)) } } return nil }transports/bifrost-http/server/server.go (1)
442-484: SimplifyFindPluginByName/FindMCPPluginByName(reflection looks redundant).
GivenTis already constrained toschemas.LLMPlugin/schemas.MCPPlugin, the firstplugin.(T)should be sufficient; the reflect branch adds complexity and runtime cost. If you keep it, please verify thereflect.TypeOf((*T)(nil)).Elem()pattern is safe for all intendedT(interface and concrete).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/media/ui-disable-auth-on-inference.pngis excluded by!**/*.pngdocs/media/ui-enforce-virtual-keys.pngis excluded by!**/*.png
📒 Files selected for processing (68)
core/bifrost.gocore/chatbot_test.gocore/internal/testutil/setup.gocore/mcp/agent.gocore/mcp/codemodeexecutecode.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/schemas/bifrost.gocore/schemas/mcp.gocore/schemas/plugin.gocore/schemas/plugin_native.gocore/schemas/plugin_wasm.gocore/schemas/trace.gocore/utils.godocs/architecture/core/plugins.mdxdocs/architecture/framework/streaming.mdxdocs/changelogs/v1.3.12.mdxdocs/features/governance/virtual-keys.mdxdocs/features/mcp/tool-execution.mdxdocs/features/observability/default.mdxdocs/features/observability/maxim.mdxdocs/features/observability/otel.mdxdocs/features/plugins/jsonparser.mdxdocs/features/plugins/mocker.mdxdocs/features/semantic-caching.mdxdocs/plugins/getting-started.mdxdocs/plugins/migration-guide.mdxdocs/plugins/writing-go-plugin.mdxdocs/quickstart/go-sdk/context-keys.mdxexamples/plugins/hello-world-wasm-go/README.mdexamples/plugins/hello-world-wasm-go/types.goexamples/plugins/hello-world-wasm-rust/README.mdexamples/plugins/hello-world-wasm-rust/src/lib.rsexamples/plugins/hello-world-wasm-rust/src/types.rsexamples/plugins/hello-world-wasm-typescript/README.mdexamples/plugins/hello-world/main.goframework/plugins/loader.goframework/plugins/main.goframework/plugins/soloader.goframework/plugins/soplugin.goframework/plugins/soplugin_test.goplugins/governance/main.goplugins/governance/utils.goplugins/jsonparser/main.goplugins/jsonparser/plugin_test.goplugins/logging/main.goplugins/maxim/main.goplugins/maxim/plugin_test.goplugins/mocker/benchmark_test.goplugins/mocker/main.goplugins/mocker/plugin_test.goplugins/otel/main.goplugins/semanticcache/main.goplugins/semanticcache/plugin_core_test.goplugins/semanticcache/plugin_integration_test.goplugins/semanticcache/search.goplugins/semanticcache/test_utils.goplugins/telemetry/main.gotransports/bifrost-http/handlers/cache.gotransports/bifrost-http/handlers/devpprof.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/lib.gotransports/bifrost-http/server/server.gotransports/changelog.mdui/app/workspace/config/views/securityView.tsx
💤 Files with no reviewable changes (3)
- core/internal/testutil/setup.go
- transports/bifrost-http/handlers/mcp.go
- core/chatbot_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
docs/quickstart/go-sdk/context-keys.mdxtransports/bifrost-http/handlers/devpprof.goframework/plugins/loader.godocs/architecture/framework/streaming.mdxdocs/plugins/getting-started.mdxexamples/plugins/hello-world-wasm-rust/src/types.rsdocs/plugins/migration-guide.mdxplugins/semanticcache/plugin_core_test.goexamples/plugins/hello-world-wasm-go/types.goexamples/plugins/hello-world-wasm-rust/README.mdexamples/plugins/hello-world-wasm-typescript/README.mdcore/utils.goexamples/plugins/hello-world-wasm-go/README.mdcore/schemas/mcp.godocs/features/semantic-caching.mdxplugins/mocker/benchmark_test.goframework/plugins/main.godocs/features/observability/default.mdxdocs/features/governance/virtual-keys.mdxplugins/semanticcache/test_utils.godocs/architecture/core/plugins.mdxplugins/mocker/plugin_test.gotransports/bifrost-http/handlers/cache.goplugins/maxim/main.gocore/schemas/trace.gocore/schemas/plugin_native.godocs/features/observability/maxim.mdxplugins/otel/main.goexamples/plugins/hello-world/main.gocore/mcp/mcp.gotransports/changelog.mddocs/features/plugins/jsonparser.mdxtransports/bifrost-http/handlers/mcpinference.goui/app/workspace/config/views/securityView.tsxplugins/telemetry/main.godocs/plugins/writing-go-plugin.mdxframework/plugins/soplugin_test.gocore/schemas/plugin_wasm.goplugins/semanticcache/main.goplugins/mocker/main.gocore/schemas/bifrost.goplugins/logging/main.gocore/schemas/plugin.godocs/features/observability/otel.mdxplugins/semanticcache/plugin_integration_test.goplugins/governance/utils.goplugins/governance/main.gocore/mcp/agent.godocs/features/mcp/tool-execution.mdxtransports/bifrost-http/lib/config.goplugins/maxim/plugin_test.goexamples/plugins/hello-world-wasm-rust/src/lib.rsplugins/semanticcache/search.godocs/features/plugins/mocker.mdxdocs/changelogs/v1.3.12.mdxtransports/bifrost-http/lib/lib.goframework/plugins/soloader.gocore/mcp/codemodeexecutecode.gocore/mcp/toolmanager.goplugins/jsonparser/plugin_test.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/middlewares.goplugins/jsonparser/main.goframework/plugins/soplugin.gocore/bifrost.go
🧠 Learnings (6)
📚 Learning: 2025-12-30T05:37:48.365Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1180
File: docs/features/mcp/connecting-to-servers.mdx:452-458
Timestamp: 2025-12-30T05:37:48.365Z
Learning: When reviewing documentation PRs in a Graphite-managed stack, first check related or previous PRs in the stack for feature implementations before flagging documentation as incorrect or unsupported. Documentation MDX files often reference features implemented in earlier stack PRs; verify that the documented behavior exists in earlier changes and that the docs accurately reflect the implemented state before requesting edits.
Applied to files:
docs/quickstart/go-sdk/context-keys.mdxdocs/architecture/framework/streaming.mdxdocs/plugins/getting-started.mdxdocs/plugins/migration-guide.mdxdocs/features/semantic-caching.mdxdocs/features/observability/default.mdxdocs/features/governance/virtual-keys.mdxdocs/architecture/core/plugins.mdxdocs/features/observability/maxim.mdxdocs/features/plugins/jsonparser.mdxdocs/plugins/writing-go-plugin.mdxdocs/features/observability/otel.mdxdocs/features/mcp/tool-execution.mdxdocs/features/plugins/mocker.mdxdocs/changelogs/v1.3.12.mdx
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
transports/bifrost-http/handlers/devpprof.goframework/plugins/loader.goplugins/semanticcache/plugin_core_test.goexamples/plugins/hello-world-wasm-go/types.gocore/utils.gocore/schemas/mcp.goplugins/mocker/benchmark_test.goframework/plugins/main.goplugins/semanticcache/test_utils.goplugins/mocker/plugin_test.gotransports/bifrost-http/handlers/cache.goplugins/maxim/main.gocore/schemas/trace.gocore/schemas/plugin_native.goplugins/otel/main.goexamples/plugins/hello-world/main.gocore/mcp/mcp.gotransports/bifrost-http/handlers/mcpinference.goplugins/telemetry/main.goframework/plugins/soplugin_test.gocore/schemas/plugin_wasm.goplugins/semanticcache/main.goplugins/mocker/main.gocore/schemas/bifrost.goplugins/logging/main.gocore/schemas/plugin.goplugins/semanticcache/plugin_integration_test.goplugins/governance/utils.goplugins/governance/main.gocore/mcp/agent.gotransports/bifrost-http/lib/config.goplugins/maxim/plugin_test.goplugins/semanticcache/search.gotransports/bifrost-http/lib/lib.goframework/plugins/soloader.gocore/mcp/codemodeexecutecode.gocore/mcp/toolmanager.goplugins/jsonparser/plugin_test.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/middlewares.goplugins/jsonparser/main.goframework/plugins/soplugin.gocore/bifrost.go
📚 Learning: 2025-12-29T11:54:55.836Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1153
File: framework/configstore/rdb.go:2221-2246
Timestamp: 2025-12-29T11:54:55.836Z
Learning: In Go reviews, do not flag range-over-int patterns like for i := range n as compile-time errors, assuming Go 1.22+ semantics. Only flag actual range-capable values (slices, arrays, maps, channels, strings) and other compile-time issues. This applies to all Go files across the repository.
Applied to files:
transports/bifrost-http/handlers/devpprof.goframework/plugins/loader.goplugins/semanticcache/plugin_core_test.goexamples/plugins/hello-world-wasm-go/types.gocore/utils.gocore/schemas/mcp.goplugins/mocker/benchmark_test.goframework/plugins/main.goplugins/semanticcache/test_utils.goplugins/mocker/plugin_test.gotransports/bifrost-http/handlers/cache.goplugins/maxim/main.gocore/schemas/trace.gocore/schemas/plugin_native.goplugins/otel/main.goexamples/plugins/hello-world/main.gocore/mcp/mcp.gotransports/bifrost-http/handlers/mcpinference.goplugins/telemetry/main.goframework/plugins/soplugin_test.gocore/schemas/plugin_wasm.goplugins/semanticcache/main.goplugins/mocker/main.gocore/schemas/bifrost.goplugins/logging/main.gocore/schemas/plugin.goplugins/semanticcache/plugin_integration_test.goplugins/governance/utils.goplugins/governance/main.gocore/mcp/agent.gotransports/bifrost-http/lib/config.goplugins/maxim/plugin_test.goplugins/semanticcache/search.gotransports/bifrost-http/lib/lib.goframework/plugins/soloader.gocore/mcp/codemodeexecutecode.gocore/mcp/toolmanager.goplugins/jsonparser/plugin_test.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/middlewares.goplugins/jsonparser/main.goframework/plugins/soplugin.gocore/bifrost.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/handlers/devpprof.gotransports/bifrost-http/handlers/cache.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/lib.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/middlewares.go
📚 Learning: 2025-12-29T09:14:16.633Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 888
File: transports/bifrost-http/handlers/middlewares.go:246-256
Timestamp: 2025-12-29T09:14:16.633Z
Learning: In the bifrost HTTP transport, fasthttp.RequestCtx is the primary context carrier and should be passed directly to functions that expect a context.Context. Do not convert to context.Context unless explicitly required. Ensure tracer implementations and related components are designed to accept fasthttp.RequestCtx directly, and document this architectural decision for maintainers.
Applied to files:
transports/bifrost-http/handlers/devpprof.gotransports/bifrost-http/handlers/cache.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/lib.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/middlewares.go
📚 Learning: 2025-12-22T10:50:40.990Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1154
File: plugins/governance/store.go:1165-1186
Timestamp: 2025-12-22T10:50:40.990Z
Learning: In the Bifrost governance plugin, budgets and rate limits have 1:1 relationships with their parent entities (virtual keys, teams, customers). Do not assume sharing; ensure cascade deletion logic only deletes budgets/rate limits when there are no shared references. Enforce invariants in code and add tests to verify no cross-entity sharing and that cascade deletes only remove the specific child of the parent. If a counterexample arises, adjust data model or add guards.
Applied to files:
plugins/governance/utils.goplugins/governance/main.go
🧬 Code graph analysis (30)
framework/plugins/loader.go (1)
core/schemas/plugin.go (1)
LLMPlugin(135-140)
examples/plugins/hello-world-wasm-rust/src/types.rs (2)
core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
examples/plugins/hello-world-wasm-go/types.go (2)
core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
core/utils.go (1)
core/mcp/toolmanager.go (3)
ToolTypeListToolFiles(51-51)ToolTypeReadToolFile(52-52)ToolTypeExecuteToolCode(53-53)
plugins/mocker/benchmark_test.go (3)
core/schemas/bifrost.go (3)
BifrostRequest(177-198)RequestType(88-88)ChatCompletionRequest(94-94)transports/bifrost-http/handlers/inference.go (1)
ChatRequest(181-185)examples/plugins/hello-world/main.go (1)
PreLLMHook(28-34)
plugins/semanticcache/test_utils.go (2)
core/schemas/plugin.go (1)
LLMPlugin(135-140)core/bifrost.go (1)
Bifrost(55-78)
plugins/mocker/plugin_test.go (2)
core/schemas/account.go (1)
Account(81-97)core/schemas/plugin.go (1)
LLMPlugin(135-140)
transports/bifrost-http/handlers/cache.go (1)
core/schemas/plugin.go (1)
LLMPlugin(135-140)
plugins/maxim/main.go (3)
examples/plugins/hello-world/main.go (2)
PreLLMHook(28-34)PostLLMHook(36-43)core/schemas/plugin.go (1)
LLMPlugin(135-140)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
core/schemas/plugin_native.go (1)
core/schemas/bifrost.go (4)
BifrostResponse(368-389)BifrostStream(492-499)BifrostError(527-536)BifrostMCPResponse(442-446)
plugins/otel/main.go (2)
core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
core/mcp/mcp.go (4)
core/bifrost.go (1)
PluginPipeline(81-97)core/mcp/toolmanager.go (2)
PluginPipeline(45-48)NewToolsManager(69-106)core/mcp.go (1)
MCPManager(49-56)core/schemas/bifrost.go (2)
BifrostMCPRequest(358-363)BifrostMCPResponse(442-446)
plugins/telemetry/main.go (4)
core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)core/utils.go (1)
GetResponseFields(243-250)
framework/plugins/soplugin_test.go (4)
framework/plugins/main.go (2)
LoadLLMPlugins(39-55)Config(16-18)core/schemas/plugin.go (1)
HTTPTransportPlugin(129-133)examples/plugins/hello-world/main.go (3)
HTTPTransportIntercept(18-26)PreLLMHook(28-34)PostLLMHook(36-43)framework/plugins/soplugin.go (1)
DynamicLLMPlugin(10-24)
plugins/semanticcache/main.go (4)
core/schemas/plugin.go (1)
LLMPlugin(135-140)core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/bifrost.go (3)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(527-536)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)
core/schemas/bifrost.go (3)
core/schemas/plugin.go (2)
LLMPlugin(135-140)MCPPlugin(142-147)core/schemas/chatcompletions.go (2)
ChatAssistantMessageToolCall(756-761)ChatMessage(511-520)core/schemas/responses.go (2)
ResponsesToolMessage(471-491)ResponsesMessage(319-332)
plugins/logging/main.go (4)
core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/bifrost.go (2)
BifrostResponse(368-389)BifrostError(527-536)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
plugins/governance/utils.go (3)
core/schemas/plugin.go (1)
HTTPRequest(30-36)core/utils.go (1)
Ptr(57-59)plugins/governance/main.go (1)
VirtualKeyPrefix(29-29)
plugins/governance/main.go (4)
core/schemas/bifrost.go (4)
BifrostResponse(368-389)BifrostError(527-536)BifrostContextKeyVirtualKey(122-122)BifrostContextKeyRequestID(124-124)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)core/utils.go (1)
GetStringFromContext(306-313)
core/mcp/agent.go (2)
core/schemas/bifrost.go (4)
BifrostMCPRequest(358-363)BifrostMCPResponse(442-446)RequestType(88-88)MCPRequestTypeChatToolCall(350-350)core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(756-761)
transports/bifrost-http/lib/config.go (2)
core/schemas/plugin.go (7)
LLMPlugin(135-140)MCPPlugin(142-147)HTTPTransportPlugin(129-133)BasePlugin(119-127)PluginType(81-81)PluginTypeLLM(84-84)PluginTypeMCP(85-85)framework/plugins/loader.go (1)
PluginLoader(6-8)
plugins/maxim/plugin_test.go (2)
core/schemas/plugin.go (1)
LLMPlugin(135-140)core/schemas/account.go (1)
Account(81-97)
plugins/semanticcache/search.go (3)
core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)framework/vectorstore/store.go (1)
SearchResult(46-50)
transports/bifrost-http/lib/lib.go (1)
core/schemas/plugin.go (1)
LLMPlugin(135-140)
framework/plugins/soloader.go (5)
core/schemas/plugin.go (1)
LLMPlugin(135-140)framework/plugins/soplugin.go (1)
DynamicLLMPlugin(10-24)core/schemas/bifrost.go (3)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(527-536)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
plugins/jsonparser/plugin_test.go (2)
core/schemas/account.go (1)
Account(81-97)core/schemas/plugin.go (1)
LLMPlugin(135-140)
transports/bifrost-http/server/server.go (3)
core/schemas/plugin.go (3)
LLMPlugin(135-140)MCPPlugin(142-147)BasePlugin(119-127)framework/plugins/loader.go (1)
PluginLoader(6-8)core/schemas/bifrost.go (1)
BifrostConfig(18-28)
transports/bifrost-http/handlers/middlewares.go (1)
core/schemas/plugin.go (2)
BasePlugin(119-127)ObservabilityPlugin(173-187)
plugins/jsonparser/main.go (2)
core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
framework/plugins/soplugin.go (3)
core/schemas/bifrost.go (2)
BifrostResponse(368-389)BifrostError(527-536)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
🪛 Gitleaks (8.30.0)
docs/features/governance/virtual-keys.mdx
[high] 566-567: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
Pratham-Mishra04
force-pushed
the
01-07-feat_plugin_schemas_segregation_for_mcp_plugins
branch
from
70c18ef to
0b88c60
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
core/bifrost.go (5)
122-216: Init should clone plugin slices before storing pointers.
bifrost.llmPlugins.Store(&config.LLMPlugins)/bifrost.mcpPlugins.Store(&config.MCPPlugins)keeps a pointer to a slice header derived from caller input; if the caller mutates the slice elements later, you can race with request processing.ReloadConfigalready clones, soInitshould too.Proposed fix
@@ bifrost.tracer.Store(&tracerWrapper{tracer: tracer}) - bifrost.llmPlugins.Store(&config.LLMPlugins) - bifrost.mcpPlugins.Store(&config.MCPPlugins) + llmPluginsCopy := slices.Clone(config.LLMPlugins) + mcpPluginsCopy := slices.Clone(config.MCPPlugins) + bifrost.llmPlugins.Store(&llmPluginsCopy) + bifrost.mcpPlugins.Store(&mcpPluginsCopy) @@ for range config.InitialPoolSize { @@ - bifrost.mcpRequestPool.Put(&schemas.BifrostMCPRequest{}) + bifrost.mcpRequestPool.Put(&schemas.BifrostMCPRequest{}) }
2717-2842: Post-hook “runFrom” should usepreCount(not current plugin length).
IntryRequest, you computepreCountfromRunLLMPreHooks, but later passpluginCount := len(*bifrost.llmPlugins.Load())intoRunPostHooks. If plugins hot-reload mid-flight, this can skip posthooks or run an unexpected subset.Proposed fix
@@ - preReq, shortCircuit, preCount := pipeline.RunLLMPreHooks(ctx, req) + preReq, shortCircuit, preCount := pipeline.RunLLMPreHooks(ctx, req) @@ - pluginCount := len(*bifrost.llmPlugins.Load()) select { case result = <-msg.Response: - resp, bifrostErr := pipeline.RunPostHooks(msg.Context, result, nil, pluginCount) + resp, bifrostErr := pipeline.RunPostHooks(msg.Context, result, nil, preCount) @@ case bifrostErrVal := <-msg.Err: bifrostErrPtr := &bifrostErrVal - resp, bifrostErrPtr = pipeline.RunPostHooks(msg.Context, nil, bifrostErrPtr, pluginCount) + resp, bifrostErrPtr = pipeline.RunPostHooks(msg.Context, nil, bifrostErrPtr, preCount)
2844-3030: Stream error recovery path should also usepreCount.
tryStreamRequestcallsRunLLMPreHooks(...)=preCountbut on error useslen(*bifrost.llmPlugins.Load()). Same hot-reload mismatch as non-streaming.Proposed fix
@@ - preReq, shortCircuit, preCount := pipeline.RunLLMPreHooks(ctx, req) + preReq, shortCircuit, preCount := pipeline.RunLLMPreHooks(ctx, req) @@ - recoveredResp, recoveredErr := pipeline.RunPostHooks(ctx, nil, &bifrostErrVal, len(*bifrost.llmPlugins.Load())) + recoveredResp, recoveredErr := pipeline.RunPostHooks(ctx, nil, &bifrostErrVal, preCount)
3273-3287: At minimum, don’t use global plugin length inside the streaming postHookRunner.
RunPostHooks(..., len(*bifrost.llmPlugins.Load()))can drift from the pipeline snapshot even withinrequestWorker. Uselen(pipeline.llmPlugins)(or arunFromvalue captured once) so the runner is internally consistent.Proposed fix
@@ postHookRunner = func(ctx *schemas.BifrostContext, result *schemas.BifrostResponse, err *schemas.BifrostError) (*schemas.BifrostResponse, *schemas.BifrostError) { - resp, bifrostErr := pipeline.RunPostHooks(ctx, result, err, len(*bifrost.llmPlugins.Load())) + resp, bifrostErr := pipeline.RunPostHooks(ctx, result, err, len(pipeline.llmPlugins)) if bifrostErr != nil { return nil, bifrostErr } return resp, nil }
3269-3308: Streaming prehooks and per-chunk posthooks execute on mismatched pipeline snapshots.
tryStreamRequestrunsRunLLMPreHookswith a pipeline snapshot and capturespreCount, but when the request is enqueued torequestWorker, streaming posthooks execute via a different pipeline snapshot obtained inrequestWorker. Additionally,requestWorkerpasseslen(*bifrost.llmPlugins.Load())instead of the originalpreCount, so posthooks may run against a different set of plugins if a plugin reload occurs between prehook and posthook execution. This violates the invariant that posthooks should reverse/correspond to the plugins that executed prehooks and can produce inconsistent spans/behavior.
🤖 Fix all issues with AI agents
In @core/bifrost.go:
- Around line 1474-1550: Add defensive input checks and ctx defaulting: in
ExecuteResponsesMCPTool validate that the incoming toolCall
(*schemas.ResponsesToolMessage) is not nil and return a BifrostError similar to
ExecuteChatMCPTool when it is nil; also ensure both ExecuteResponsesMCPTool and
ExecuteChatMCPTool set ctx = bifrost.defaultContext() (or call the same
ctx-normalization used by other public APIs) when the passed ctx is nil before
calling handleMCPToolExecution so handleMCPToolExecution never receives a nil
context. Use the same error construction pattern and RequestType values already
used in these functions to keep consistency.
In @transports/bifrost-http/lib/config.go:
- Around line 2263-2285: Add validation to reject plugins with empty names
before attempting to add them: in AddLoadedPlugin, call plugin.GetName() and if
it is an empty string return an error like "plugin name is empty" (do this prior
to the type switch so it applies to all plugin types). Ensure you do not call
addLoadedLLMPlugin or addLoadedMCPPlugin when the name is empty and preserve
existing interface checks and cache updates performed by those methods.
In @transports/bifrost-http/server/server.go:
- Around line 933-957: The reloadObservabilityPlugins function currently only
updates the tracing middleware when observabilityPlugins is non-empty, leaving
stale plugins when all are removed; change reloadObservabilityPlugins so that
after collecting plugins from s.LLMPlugins and s.MCPPlugins (while holding
s.PluginsMutex.RLock/RUnlock) it always calls
s.tracingMiddleware.SetObservabilityPlugins(observabilityPlugins) even when
observabilityPlugins is empty (i.e., pass an empty slice to clear middleware
state) so the tracing middleware is updated on removals as well.
- Around line 826-905: SyncLoadedPlugin currently only adds or replaces entries
in s.LLMPlugins / s.MCPPlugins but never removes stale entries when the new
plugin instance no longer implements an interface (llmPlugin == nil or mcpPlugin
== nil); update SyncLoadedPlugin so under the existing s.PluginsMutex lock you
remove any entry from s.LLMPlugins whose GetName() matches plugin.GetName() when
llmPlugin == nil (and likewise remove matches from s.MCPPlugins when mcpPlugin
== nil), then continue to append/replace when non-nil as before, produce the
slices clones (llmPluginsCopy / mcpPluginsCopy) after these add/remove
modifications, and proceed with Config.AddLoadedPlugin and Client.ReloadConfig
using those updated copies to avoid leaving stale interface entries.
🧹 Nitpick comments (8)
plugins/otel/main.go (1)
237-238: Consider adding a compile-time check forLLMPlugininterface.The plugin implements
PreLLMHookandPostLLMHook, so it should also satisfy theLLMPlugininterface. Adding a compile-time check would catch interface drift early.♻️ Suggested addition
// Compile-time check that OtelPlugin implements ObservabilityPlugin var _ schemas.ObservabilityPlugin = (*OtelPlugin)(nil) + +// Compile-time check that OtelPlugin implements LLMPlugin +var _ schemas.LLMPlugin = (*OtelPlugin)(nil)plugins/mocker/benchmark_test.go (1)
12-67: LGTM! Benchmark correctly callsPreLLMHook.The method invocation and comment are properly updated.
Optional: Consider renaming benchmark functions for consistency.
The benchmark function names still reference
PreHook(e.g.,BenchmarkMockerPlugin_PreHook_SimpleRule) while the code callsPreLLMHook. Renaming toBenchmarkMockerPlugin_PreLLMHook_*would improve consistency.core/mcp/agent.go (1)
293-314: Consider logging when executeToolFunc returns (nil, nil).The fallback path at lines 310-312 handles the case where both
mcpResponseandtoolErrare nil by sending an empty result. This case likely indicates a bug inexecuteToolFunc(violating the contract of returning either a response or an error). Consider adding a warning log to aid debugging:} else { // Fallback: send empty result when both mcpResponse and toolErr are nil logger.Warn(fmt.Sprintf("Tool execution for %s returned (nil, nil) - this may indicate a bug in executeToolFunc", func() string { if toolCall.Function.Name != nil { return *toolCall.Function.Name } return "unknown" }())) channelToolResults <- createToolResultMessage(toolCall, "", nil) }transports/bifrost-http/lib/config.go (1)
312-312: Consider initializing all plugin atomic pointers for consistency.Only
LLMPluginsis explicitly initialized here, whileMCPPluginsandHTTPTransportPluginsrely on zero values. While functionally correct (atomic.Pointer's zero value is safe), initializing all three would improve code clarity and consistency.Suggested initialization for consistency
config := &Config{ configPath: configFilePath, EnvKeys: make(map[string][]configstore.EnvKeyInfo), Providers: make(map[schemas.ModelProvider]configstore.ProviderConfig), LLMPlugins: atomic.Pointer[[]schemas.LLMPlugin]{}, + MCPPlugins: atomic.Pointer[[]schemas.MCPPlugin]{}, + HTTPTransportPlugins: atomic.Pointer[[]schemas.HTTPTransportPlugin]{}, }core/bifrost.go (1)
3924-3937: getPluginPipeline should guard nil loads (or document invariant).
pipeline.llmPlugins = *bifrost.llmPlugins.Load()will panic if the pointer is ever unset; if “always stored in Init” is the invariant, consider a small guard + empty slice fallback to make this harder to misuse in tests/alternate constructors.transports/bifrost-http/server/server.go (2)
311-440: LoadPlugins: consider surfacing “plugin implements neither LLM nor MCP” as error/status.
Right nowaddPluginToArrayssilently drops such a plugin from both arrays while still marking it active. That’s surprising for operators.
442-484: FindPluginByName/FindMCPPluginByName: precompute reflect.Type once + guard nil entries.
You can movezeroType := reflect.TypeOf((*T)(nil)).Elem()outside the loop; also consider skipping nil plugin entries to avoid panics onplugin.GetName().core/mcp/toolmanager.go (1)
416-500: Tool argument parsing: consider treating emptyArgumentsas{}.
IftoolCall.Function.Argumentscan be empty,json.Unmarshal([]byte(""), &arguments)errors; a small guard would make execution more forgiving.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/media/ui-disable-auth-on-inference.pngis excluded by!**/*.pngdocs/media/ui-enforce-virtual-keys.pngis excluded by!**/*.png
📒 Files selected for processing (60)
core/bifrost.gocore/mcp/agent.gocore/mcp/codemodeexecutecode.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/schemas/bifrost.gocore/schemas/mcp.gocore/schemas/plugin.gocore/schemas/plugin_native.gocore/schemas/trace.gocore/utils.godocs/architecture/core/plugins.mdxdocs/architecture/framework/streaming.mdxdocs/changelogs/v1.3.12.mdxdocs/features/governance/virtual-keys.mdxdocs/features/mcp/tool-execution.mdxdocs/features/observability/default.mdxdocs/features/observability/maxim.mdxdocs/features/observability/otel.mdxdocs/features/plugins/jsonparser.mdxdocs/features/plugins/mocker.mdxdocs/features/semantic-caching.mdxdocs/plugins/getting-started.mdxdocs/plugins/migration-guide.mdxdocs/plugins/writing-go-plugin.mdxdocs/quickstart/go-sdk/context-keys.mdxexamples/plugins/hello-world-wasm-go/README.mdexamples/plugins/hello-world/main.goframework/plugins/loader.goframework/plugins/main.goframework/plugins/soloader.goframework/plugins/soplugin.goframework/plugins/soplugin_test.goplugins/governance/main.goplugins/governance/utils.goplugins/jsonparser/main.goplugins/jsonparser/plugin_test.goplugins/logging/main.goplugins/maxim/main.goplugins/maxim/plugin_test.goplugins/mocker/benchmark_test.goplugins/mocker/main.goplugins/mocker/plugin_test.goplugins/otel/main.goplugins/semanticcache/main.goplugins/semanticcache/plugin_core_test.goplugins/semanticcache/plugin_integration_test.goplugins/semanticcache/search.goplugins/semanticcache/test_utils.goplugins/telemetry/main.gotransports/bifrost-http/handlers/cache.gotransports/bifrost-http/handlers/devpprof.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/lib.gotransports/bifrost-http/server/server.gotransports/changelog.mdui/app/workspace/config/views/securityView.tsx
💤 Files with no reviewable changes (1)
- transports/bifrost-http/handlers/mcp.go
🚧 Files skipped from review as they are similar to previous changes (21)
- plugins/semanticcache/plugin_core_test.go
- transports/bifrost-http/handlers/devpprof.go
- plugins/jsonparser/plugin_test.go
- core/schemas/trace.go
- core/utils.go
- transports/bifrost-http/handlers/cache.go
- docs/plugins/migration-guide.mdx
- ui/app/workspace/config/views/securityView.tsx
- transports/changelog.md
- docs/features/semantic-caching.mdx
- docs/features/observability/default.mdx
- docs/architecture/framework/streaming.mdx
- docs/features/plugins/mocker.mdx
- plugins/semanticcache/plugin_integration_test.go
- docs/features/observability/maxim.mdx
- docs/plugins/getting-started.mdx
- plugins/governance/main.go
- framework/plugins/main.go
- docs/features/observability/otel.mdx
- plugins/governance/utils.go
- docs/quickstart/go-sdk/context-keys.mdx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
core/schemas/mcp.goplugins/semanticcache/test_utils.gocore/mcp/mcp.goframework/plugins/loader.godocs/changelogs/v1.3.12.mdxtransports/bifrost-http/handlers/middlewares.gocore/schemas/plugin_native.goplugins/mocker/plugin_test.godocs/features/governance/virtual-keys.mdxplugins/semanticcache/search.goplugins/mocker/benchmark_test.gotransports/bifrost-http/handlers/mcpinference.goplugins/maxim/main.gocore/mcp/agent.goplugins/maxim/plugin_test.goframework/plugins/soplugin.goplugins/otel/main.gotransports/bifrost-http/lib/lib.gocore/mcp/codemodeexecutecode.gocore/schemas/bifrost.godocs/features/mcp/tool-execution.mdxplugins/logging/main.godocs/features/plugins/jsonparser.mdxplugins/mocker/main.goframework/plugins/soplugin_test.godocs/plugins/writing-go-plugin.mdxdocs/architecture/core/plugins.mdxframework/plugins/soloader.goexamples/plugins/hello-world-wasm-go/README.mdcore/schemas/plugin.goplugins/semanticcache/main.gocore/bifrost.goplugins/telemetry/main.goplugins/jsonparser/main.gotransports/bifrost-http/server/server.goexamples/plugins/hello-world/main.gocore/mcp/toolmanager.gotransports/bifrost-http/lib/config.go
🧠 Learnings (6)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/schemas/mcp.goplugins/semanticcache/test_utils.gocore/mcp/mcp.goframework/plugins/loader.gotransports/bifrost-http/handlers/middlewares.gocore/schemas/plugin_native.goplugins/mocker/plugin_test.goplugins/semanticcache/search.goplugins/mocker/benchmark_test.gotransports/bifrost-http/handlers/mcpinference.goplugins/maxim/main.gocore/mcp/agent.goplugins/maxim/plugin_test.goframework/plugins/soplugin.goplugins/otel/main.gotransports/bifrost-http/lib/lib.gocore/mcp/codemodeexecutecode.gocore/schemas/bifrost.goplugins/logging/main.goplugins/mocker/main.goframework/plugins/soplugin_test.goframework/plugins/soloader.gocore/schemas/plugin.goplugins/semanticcache/main.gocore/bifrost.goplugins/telemetry/main.goplugins/jsonparser/main.gotransports/bifrost-http/server/server.goexamples/plugins/hello-world/main.gocore/mcp/toolmanager.gotransports/bifrost-http/lib/config.go
📚 Learning: 2025-12-29T11:54:55.836Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1153
File: framework/configstore/rdb.go:2221-2246
Timestamp: 2025-12-29T11:54:55.836Z
Learning: In Go reviews, do not flag range-over-int patterns like for i := range n as compile-time errors, assuming Go 1.22+ semantics. Only flag actual range-capable values (slices, arrays, maps, channels, strings) and other compile-time issues. This applies to all Go files across the repository.
Applied to files:
core/schemas/mcp.goplugins/semanticcache/test_utils.gocore/mcp/mcp.goframework/plugins/loader.gotransports/bifrost-http/handlers/middlewares.gocore/schemas/plugin_native.goplugins/mocker/plugin_test.goplugins/semanticcache/search.goplugins/mocker/benchmark_test.gotransports/bifrost-http/handlers/mcpinference.goplugins/maxim/main.gocore/mcp/agent.goplugins/maxim/plugin_test.goframework/plugins/soplugin.goplugins/otel/main.gotransports/bifrost-http/lib/lib.gocore/mcp/codemodeexecutecode.gocore/schemas/bifrost.goplugins/logging/main.goplugins/mocker/main.goframework/plugins/soplugin_test.goframework/plugins/soloader.gocore/schemas/plugin.goplugins/semanticcache/main.gocore/bifrost.goplugins/telemetry/main.goplugins/jsonparser/main.gotransports/bifrost-http/server/server.goexamples/plugins/hello-world/main.gocore/mcp/toolmanager.gotransports/bifrost-http/lib/config.go
📚 Learning: 2025-12-30T05:37:48.365Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1180
File: docs/features/mcp/connecting-to-servers.mdx:452-458
Timestamp: 2025-12-30T05:37:48.365Z
Learning: When reviewing documentation PRs in a Graphite-managed stack, first check related or previous PRs in the stack for feature implementations before flagging documentation as incorrect or unsupported. Documentation MDX files often reference features implemented in earlier stack PRs; verify that the documented behavior exists in earlier changes and that the docs accurately reflect the implemented state before requesting edits.
Applied to files:
docs/changelogs/v1.3.12.mdxdocs/features/governance/virtual-keys.mdxdocs/features/mcp/tool-execution.mdxdocs/features/plugins/jsonparser.mdxdocs/plugins/writing-go-plugin.mdxdocs/architecture/core/plugins.mdx
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/lib/lib.gotransports/bifrost-http/server/server.gotransports/bifrost-http/lib/config.go
📚 Learning: 2025-12-29T09:14:16.633Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 888
File: transports/bifrost-http/handlers/middlewares.go:246-256
Timestamp: 2025-12-29T09:14:16.633Z
Learning: In the bifrost HTTP transport, fasthttp.RequestCtx is the primary context carrier and should be passed directly to functions that expect a context.Context. Do not convert to context.Context unless explicitly required. Ensure tracer implementations and related components are designed to accept fasthttp.RequestCtx directly, and document this architectural decision for maintainers.
Applied to files:
transports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/lib/lib.gotransports/bifrost-http/server/server.gotransports/bifrost-http/lib/config.go
📚 Learning: 2026-01-10T08:07:42.582Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1299
File: core/mcp/codemodeexecutecode.go:882-907
Timestamp: 2026-01-10T08:07:42.582Z
Learning: In the MCP tool execution flow (core/mcp/codemodeexecutecode.go callMCPTool function), pre-hook modifications to tool names should NOT be applied. The originalToolName computed before pre-hooks must be used in the final MCP CallToolRequest. Only argument modifications from pre-hooks should be honored. This is a security/consistency decision to prevent plugins from redirecting tool execution.
Applied to files:
core/mcp/codemodeexecutecode.go
🧬 Code graph analysis (23)
plugins/semanticcache/test_utils.go (2)
core/schemas/plugin.go (1)
LLMPlugin(135-140)core/bifrost.go (1)
Bifrost(55-78)
core/mcp/mcp.go (4)
core/mcp/toolmanager.go (2)
PluginPipeline(45-48)NewToolsManager(69-106)core/mcp.go (1)
MCPManager(49-56)core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/bifrost.go (2)
BifrostMCPRequest(358-363)BifrostMCPResponse(442-446)
framework/plugins/loader.go (1)
core/schemas/plugin.go (1)
LLMPlugin(135-140)
transports/bifrost-http/handlers/middlewares.go (1)
core/schemas/plugin.go (2)
BasePlugin(119-127)ObservabilityPlugin(173-187)
core/schemas/plugin_native.go (1)
core/schemas/bifrost.go (4)
BifrostResponse(368-389)BifrostStream(492-499)BifrostError(527-536)BifrostMCPResponse(442-446)
plugins/mocker/plugin_test.go (2)
core/schemas/account.go (1)
Account(81-97)core/schemas/plugin.go (1)
LLMPlugin(135-140)
plugins/semanticcache/search.go (3)
core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)framework/vectorstore/store.go (1)
SearchResult(46-50)
plugins/mocker/benchmark_test.go (3)
core/schemas/bifrost.go (2)
BifrostRequest(177-198)RequestType(88-88)transports/bifrost-http/handlers/inference.go (1)
ChatRequest(181-185)examples/plugins/hello-world/main.go (1)
PreLLMHook(28-34)
transports/bifrost-http/handlers/mcpinference.go (4)
transports/bifrost-http/lib/middleware.go (1)
ChainMiddlewares(11-23)transports/bifrost-http/handlers/utils.go (2)
SendError(35-44)SendBifrostError(47-62)core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(756-761)transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(81-409)
plugins/maxim/plugin_test.go (2)
core/schemas/plugin.go (1)
LLMPlugin(135-140)core/schemas/account.go (1)
Account(81-97)
plugins/otel/main.go (3)
core/schemas/bifrost.go (2)
BifrostResponse(368-389)BifrostError(527-536)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
transports/bifrost-http/lib/lib.go (1)
core/schemas/plugin.go (1)
LLMPlugin(135-140)
core/mcp/codemodeexecutecode.go (6)
core/schemas/context.go (2)
BifrostContext(32-43)NewBifrostContext(48-71)core/schemas/bifrost.go (2)
BifrostContextKeyRequestID(124-124)BifrostContextKeyParentMCPRequestID(141-141)core/schemas/chatcompletions.go (2)
ChatAssistantMessageToolCall(756-761)ChatAssistantMessageToolCallFunction(764-767)core/utils.go (1)
Ptr(57-59)core/schemas/utils.go (1)
Ptr(14-16)core/mcp/toolmanager.go (1)
ToolsManager(22-41)
core/schemas/bifrost.go (3)
core/schemas/plugin.go (2)
LLMPlugin(135-140)MCPPlugin(142-147)core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(756-761)core/schemas/responses.go (2)
ResponsesToolMessage(471-491)ResponsesMessage(319-332)
plugins/logging/main.go (4)
core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/bifrost.go (3)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(527-536)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
plugins/mocker/main.go (2)
core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
framework/plugins/soloader.go (4)
core/schemas/plugin.go (1)
LLMPlugin(135-140)framework/plugins/soplugin.go (1)
DynamicLLMPlugin(10-24)core/schemas/bifrost.go (3)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(527-536)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
plugins/semanticcache/main.go (3)
examples/plugins/hello-world/main.go (2)
PreLLMHook(28-34)PostLLMHook(36-43)core/schemas/plugin.go (1)
LLMPlugin(135-140)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)
core/bifrost.go (4)
core/schemas/plugin.go (3)
LLMPlugin(135-140)MCPPlugin(142-147)BasePlugin(119-127)core/schemas/bifrost.go (7)
KeySelector(13-13)BifrostMCPRequest(358-363)BifrostConfig(18-28)BifrostError(527-536)RequestType(88-88)BifrostMCPResponse(442-446)BifrostRequest(177-198)core/schemas/mcp.go (2)
MCPConfig(16-34)MCPClientConfig(56-78)core/mcp/toolmanager.go (1)
PluginPipeline(45-48)
plugins/telemetry/main.go (5)
core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/bifrost.go (3)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(527-536)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)core/utils.go (1)
GetResponseFields(243-250)
plugins/jsonparser/main.go (5)
examples/plugins/hello-world/main.go (2)
PreLLMHook(28-34)PostLLMHook(36-43)core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/bifrost.go (3)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(527-536)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
transports/bifrost-http/server/server.go (3)
core/schemas/plugin.go (3)
LLMPlugin(135-140)MCPPlugin(142-147)BasePlugin(119-127)framework/plugins/main.go (4)
Config(16-18)LoadLLMPlugins(39-55)AsLLMPlugin(22-27)AsMCPPlugin(31-36)framework/plugins/loader.go (1)
PluginLoader(6-8)
transports/bifrost-http/lib/config.go (2)
core/schemas/plugin.go (7)
LLMPlugin(135-140)MCPPlugin(142-147)HTTPTransportPlugin(129-133)BasePlugin(119-127)PluginType(81-81)PluginTypeLLM(84-84)PluginTypeMCP(85-85)framework/plugins/loader.go (1)
PluginLoader(6-8)
🔇 Additional comments (86)
examples/plugins/hello-world-wasm-go/README.md (2)
97-122: LGTM! Documentation correctly reflects the hook renaming.The documentation updates for PreLLMHook and PostLLMHook are consistent with the broader plugin architecture refactoring. The JSON schemas accurately describe the new short-circuit structure and hook signatures.
170-170: LGTM! Feature parity statement updated.The feature list correctly references the renamed PreLLMHook and PostLLMHook hooks, maintaining documentation consistency with the implementation changes.
docs/architecture/core/plugins.mdx (3)
151-227: LGTM! Sequence diagrams and flows correctly updated.All sequence diagrams (normal flow, short-circuit, streaming, error recovery) have been consistently updated to reference PreLLMHook and PostLLMHook. The architectural semantics (symmetric execution, reverse order, short-circuit behavior) remain intact with only terminology changes.
264-264: LGTM! PostLLMHook guarantee correctly stated.The guarantee that all executed PreLLMHooks receive corresponding PostLLMHook calls is accurately preserved with the updated terminology.
278-349: LGTM! Complex flows updated consistently.The short-circuit error flow with AllowFallbacks and the complex plugin decision flow graph both correctly reference the renamed hooks and LLMPluginShortCircuit type throughout.
transports/bifrost-http/lib/lib.go (1)
20-20: LGTM! Interface updated to reflect LLM plugin domain.The LoadGovernancePlugin return type change from
schemas.Plugintoschemas.LLMPlugincorrectly aligns the governance plugin loading with the new domain-specific plugin architecture. This is consistent with the broader refactoring that separates LLM and MCP plugin concerns.Note: This is a breaking change for any implementations of the
EnterpriseOverridesinterface outside this codebase.examples/plugins/hello-world/main.go (2)
22-34: LGTM! PreLLMHook implementation correctly updated.The PreHook function has been correctly renamed to PreLLMHook with the updated signature returning
*schemas.LLMPluginShortCircuit. The implementation behavior is preserved (pass-through with context value storage), and console output is updated for consistency.
36-43: LGTM! PostLLMHook implementation correctly updated.The PostHook function has been correctly renamed to PostLLMHook with the matching signature. Context value retrieval and pass-through behavior are preserved, with updated console output for debugging consistency.
plugins/telemetry/main.go (3)
28-28: LGTM! Plugin interface declaration updated.The comment correctly reflects that PrometheusPlugin now implements the
schemas.LLMPlugininterface, aligning with the plugin domain segregation.
289-294: LGTM! PreLLMHook correctly implements the new interface.The PreLLMHook function signature has been correctly updated to return
*schemas.LLMPluginShortCircuit. The start time recording logic is preserved, ensuring duration calculations remain accurate in PostLLMHook.
296-307: LGTM! PostLLMHook correctly implements the new interface.The PostLLMHook function signature and implementation are correctly updated:
- Parameters match the LLMPlugin interface requirements
- Start time retrieval logic preserved
- Error message on Line 305 correctly references "PostLLMHook" for consistency
The metrics collection logic remains intact and will continue to function correctly with the new interface.
plugins/semanticcache/search.go (3)
16-16: LGTM: Function signatures updated correctly.The return type changes from
*schemas.PluginShortCircuitto*schemas.LLMPluginShortCircuitare consistent across all cache search and response-building functions, correctly aligning with the per-domain plugin architecture introduced in this PR.Also applies to: 86-86, 162-162, 241-241, 288-288
99-99: LGTM: Comment correctly reflects hook rename.The comment update from "PostHook" to "PostLLMHook" is accurate and aligns with the systematic hook renaming across the codebase.
282-284: LGTM: Return statements correctly construct LLMPluginShortCircuit.Both single and streaming response builders correctly return
&schemas.LLMPluginShortCircuitwith the appropriate Response or Stream field, matching the new type definition.Also applies to: 366-368
docs/changelogs/v1.3.12.mdx (1)
54-54: LGTM: Changelog accurately reflects hook rename.The update from "PostHook" to "PostLLMHook" in the jsonparser v1.3.12 changelog entry correctly documents the hook naming change introduced in this PR.
docs/features/mcp/tool-execution.mdx (1)
21-32: LGTM: Authentication documentation is clear and well-structured.The new Authentication section effectively clarifies that the MCP tool execution endpoint shares authentication behavior with inference endpoints. The table format clearly shows the two configuration scenarios, and the links to related documentation provide helpful additional context.
Based on learnings, as this is part of a Graphite stack with related PRs, the documented authentication behavior is likely implemented in the stack.
plugins/mocker/plugin_test.go (1)
66-67: LGTM: Test configurations updated correctly for LLM plugin domain.All six test functions consistently update the
BifrostConfiginitialization fromPlugins: []schemas.Plugin{plugin}toLLMPlugins: []schemas.LLMPlugin{plugin}, correctly reflecting the new per-domain plugin architecture where mocker operates as an LLM plugin.Also applies to: 109-111, 185-187, 264-266, 327-329, 397-399
framework/plugins/loader.go (1)
7-7: LGTM: Interface method renamed and return type updated correctly.The rename from
LoadDynamicPlugintoLoadDynamicLLMPluginand the return type change fromschemas.Plugintoschemas.LLMPlugincorrectly reflect the per-domain plugin architecture. The method name now clearly indicates it loads LLM-specific plugins, improving API clarity.core/schemas/mcp.go (1)
25-33: LGTM! Well-documented plugin pipeline lifecycle fields.The use of
interface{}is appropriate here to avoid circular dependencies between packages. The acquire/release pattern aligns with the existing pool-based design incore/bifrost.go.core/schemas/plugin_native.go (1)
14-27: LGTM! Clean separation of LLM and MCP short-circuit types.The domain-specific split is well-structured:
LLMPluginShortCircuitretains theStreamfield for streaming LLM responses.MCPPluginShortCircuitomitsStreamsince MCP tool execution is synchronous.Both types appropriately share
BifrostErrorfor consistent error handling across domains.plugins/maxim/plugin_test.go (2)
24-26: LGTM! Return type correctly updated toLLMPlugin.The function signature and documentation are consistent with the per-domain plugin architecture.
97-101: LGTM! Bifrost initialization correctly usesLLMPlugins.The test properly migrates from the legacy
Pluginsfield to the domain-specificLLMPluginscollection.plugins/semanticcache/test_utils.go (2)
300-316: LGTM! Test utilities correctly migrated toLLMPlugin.The parameter type and
BifrostConfiginitialization are properly updated to use the domain-specificLLMPluginscollection.
325-331: LGTM!TestSetupstruct correctly usesLLMPlugintype.plugins/otel/main.go (1)
189-199: LGTM! Hook methods correctly renamed and typed.The
PreLLMHookandPostLLMHookimplementations are appropriately no-ops since the OTEL plugin operates via theObservabilityPlugin.Injectmethod for trace forwarding.plugins/mocker/benchmark_test.go (4)
69-124: LGTM! Regex rule benchmark correctly updated.
126-203: LGTM! Multiple rules benchmark correctly updated.
205-261: LGTM! No-match benchmark correctly updated.
263-316: LGTM! Template benchmark correctly updated.transports/bifrost-http/handlers/middlewares.go (2)
52-59: LGTM! Correctly uses domain-specific plugin accessor.Using
GetLoadedHTTPTransportPlugins()instead of the genericGetLoadedPlugins()properly aligns with the per-domain plugin architecture, ensuring only HTTP transport-capable plugins are processed.
442-457: LGTM! Parameter type correctly updated toBasePlugin.The change from
[]schemas.Pluginto[]schemas.BasePluginis appropriate sinceObservabilityPluginembedsBasePlugin. The type assertion at line 451 correctly filters for plugins implementingObservabilityPlugin.docs/plugins/writing-go-plugin.mdx (1)
124-178: Verify v1.3.x documentation accuracy.Both the v1.4.x+ and v1.3.x tabs show the same hook names (
PreLLMHook/PostLLMHook) and types (LLMPluginShortCircuit). If this PR introduces breaking changes to the plugin API in v1.4.0, the v1.3.x tab should document the previous API surface (e.g.,PreHook/PostHookandPluginShortCircuit).Confirm whether:
- The v1.3.x tab should show the old naming, or
- These changes were backported to v1.3.x
core/mcp/codemodeexecutecode.go (5)
520-522: Approve: Correct context propagation for plugin hooks.Passing the original
ctx(BifrostContext) instead oftimeoutCtxensures proper parent-child request ID tracking and plugin hook execution. ThecallMCPToolfunction handles timeout internally via its owncontext.WithTimeouton line 895.
882-891: Correct: Original tool name preserved for MCP call.The implementation correctly uses
originalToolName(computed before pre-hooks at line 771) in theCallToolRequest, ensuring that plugin pre-hooks cannot redirect tool execution. Only argument modifications from pre-hooks are honored, which aligns with the security/consistency requirement. Based on learnings.
795-808: Well-documented context isolation.The comment clearly explains why
NewBifrostContextis required instead ofWithValue- to prevent the parent's request ID from being overwritten. This prevents subtle bugs where the parent executeToolCode's response would overwrite the last nested tool's log entry.
969-1019: LGTM: Clean fallback implementation.The
callMCPToolDirectfunction provides a symmetric fallback path when the plugin pipeline is unavailable, maintaining consistent behavior for direct MCP tool calls without hooks.
1021-1037: LGTM: Appropriate JSON parsing with fallback.The helper correctly handles nil cases and provides sensible fallback behavior when the content is not valid JSON.
core/mcp/mcp.go (2)
83-101: Approve: Plugin pipeline wiring is correct.The conversion wrappers properly adapt the config-level plugin pipeline functions to the internal
PluginPipelineinterface. The nil check on line 89 and type assertion on line 90 ensure safe handling when the pipeline doesn't implement the expected interface.
132-147: Clean API consolidation.The unified
ExecuteToolCallmethod simplifies the public API by accepting aBifrostMCPRequestthat can contain either Chat or Responses format tool calls, eliminating the need for separate entry points.transports/bifrost-http/handlers/mcpinference.go (3)
15-31: LGTM: Handler structure follows established patterns.The handler follows the same structure as other handlers in the transport layer, with proper middleware chaining and clean separation.
62-68: Proper context lifecycle management.The deferred
cancel()call ensures proper cleanup of the cancellable context even if the handler returns early due to errors. The nil check forbifrostCtxhandles the edge case where context conversion fails.
70-78: Verify client method existence.The handler calls
h.client.ExecuteChatMCPToolandh.client.ExecuteResponsesMCPTool, but based on the changes incore/mcp/mcp.go, the unified API isExecuteToolCall. Verify these methods exist on the Bifrost client or if they should be updated to use the unified API.#!/bin/bash # Check what MCP execution methods exist on the Bifrost client ast-grep --pattern 'func ($_ *Bifrost) Execute$_MCPTool($$$) ($$$) { $$$ }' # Also check for ExecuteToolCall rg -n "func.*ExecuteChatMCPTool|func.*ExecuteResponsesMCPTool|func.*ExecuteToolCall" --type goframework/plugins/soplugin_test.go (3)
31-42: LGTM: Test config updated to use LLMPlugins.The test configuration correctly uses the renamed
LLMPluginsfield andDynamicLLMPluginConfigtype, consistent with the domain-specific plugin refactoring.
73-81: HTTPTransportPlugin interface check added.Good addition of the type assertion to
schemas.HTTPTransportPluginbefore callingHTTPTransportIntercept, making the test more explicit about the expected interface.
83-108: Hook method tests correctly renamed.The test methods and assertions are updated to use
PreLLMHookandPostLLMHook, with the correct return type including*schemas.LLMPluginShortCircuit.plugins/mocker/main.go (3)
486-552: LGTM: PreLLMHook correctly renamed and typed.The method signature and all return paths correctly use
*schemas.LLMPluginShortCircuit. The internal logic remains unchanged, which is appropriate since only the type nomenclature changed.
554-557: LGTM: PostLLMHook signature updated.The PostLLMHook method signature is correctly updated to match the new LLM-specific hook interface.
993-1041: handleDefaultBehavior returns correct type.The helper function correctly returns
*schemas.LLMPluginShortCircuitfor all code paths (error, success, and passthrough).plugins/maxim/main.go (4)
35-60: LGTM: Plugin interface updated to LLMPlugin.The plugin correctly implements
schemas.LLMPluginand theInitfunction return type is updated accordingly. The comment on line 35 is also updated to reference the correct interface.
214-235: PreLLMHook documentation and signature correct.The method documentation is thorough and the signature correctly returns
*schemas.LLMPluginShortCircuit.
438-440: Log message updated for consistency.Good attention to detail - the warning message now references
PreLLMHookinstead of the oldPreHookname.
454-474: PostLLMHook documentation and signature correct.The method documentation is updated and the signature correctly matches the
LLMPlugininterface.docs/features/governance/virtual-keys.mdx (1)
545-620: Documentation clearly explains authentication and virtual keys model.The new "Authentication and Virtual Keys" section effectively describes the two-layer model with a helpful header mapping table and practical examples for both auth-enabled and auth-disabled scenarios. All referenced images are present and correctly linked.
framework/plugins/soloader.go (1)
14-94: LGTM! Loader refactored for LLM plugin domain.The renaming from
LoadDynamicPlugintoLoadDynamicLLMPluginand the corresponding type updates (PreHook → PreLLMHook, PostHook → PostLLMHook, PluginShortCircuit → LLMPluginShortCircuit) are consistent with the plugin schema segregation objectives. The loader correctly instantiatesDynamicLLMPluginand looks up the renamed hook symbols.core/schemas/bifrost.go (5)
20-21: LGTM! Plugin collections separated by domain.Splitting
BifrostConfig.PluginsintoLLMPlugins []LLMPluginandMCPPlugins []MCPPlugincleanly separates concerns for LLM inference hooks versus MCP tool execution hooks, aligning with the PR objectives.
141-141: LGTM! Context key added for nested MCP tool calls.The new context key
BifrostContextKeyParentMCPRequestIDsupports tracking parent request IDs for nested tool calls (e.g., from executeCode), which is essential for distributed tracing and debugging.
347-363: LGTM! MCP request type introduced.The new
BifrostMCPRequeststruct withMCPRequestTypediscriminator and embedded pointers forChatAssistantMessageToolCallandResponsesToolMessageprovides a unified request shape for MCP tool execution across both Chat and Responses APIs. The comment correctly notes mutual exclusivity.
438-467: LGTM! MCP response types added.
BifrostMCPResponseandBifrostMCPResponseExtraFieldsprovide structured response types for MCP tool execution, including observability fields (ToolName, Latency, RawRequest, RawResponse) that align with the existingBifrostResponseExtraFieldspattern.
523-523: LGTM! Documentation updated for renamed hooks.Comment correctly references
PreLLMHookinstead of the oldPreHook, maintaining consistency with the hook renaming across the codebase.core/mcp/agent.go (1)
23-23: LGTM! Function signature updated for unified MCP request/response flow.The
executeToolFuncsignature change from accepting individual tool calls to accepting*schemas.BifrostMCPRequestand returning*schemas.BifrostMCPResponseunifies the MCP tool execution interface across Chat and Responses APIs, aligning with the PR's schema segregation objectives.Also applies to: 36-36, 76-76, 89-89, 128-128, 139-139
core/schemas/plugin.go (4)
80-86: LGTM! Plugin type constants added.The new
PluginTypewithPluginTypeLLMandPluginTypeMCPconstants provides clear domain categorization for plugins, supporting the schema segregation objectives.
119-147: LGTM! Interface hierarchy refactored for domain separation.The refactoring from a single
Plugininterface to a hierarchy (BasePlugin,HTTPTransportPlugin,LLMPlugin,MCPPlugin) cleanly separates concerns:
BasePlugin: Core plugin lifecycle (GetName, Cleanup)HTTPTransportPlugin: HTTP interception capabilityLLMPlugin: LLM inference hooks (PreLLMHook, PostLLMHook)MCPPlugin: MCP tool execution hooks (PreMCPHook, PostMCPHook)This design follows the Interface Segregation Principle and aligns with the PR's objectives for plugin schema segregation.
97-111: LGTM! Documentation updated for renamed hooks.Comments correctly reference
PreLLMHook,PostLLMHook, andLLMPluginShortCircuit, maintaining consistency with the hook renaming and new short-circuit types across the codebase.
174-174: LGTM! ObservabilityPlugin updated to embed BasePlugin.Correctly updated to embed
BasePlugininstead of the oldPlugininterface, maintaining consistency with the refactored interface hierarchy.framework/plugins/soplugin.go (1)
10-52: LGTM! Dynamic plugin refactored for LLM domain.The renaming from
DynamicPlugintoDynamicLLMPluginand the corresponding updates to method names (PreHook → PreLLMHook, PostHook → PostLLMHook) and short-circuit types (PluginShortCircuit → LLMPluginShortCircuit) are consistent with the plugin schema segregation objectives. The implementation now correctly implements theLLMPlugininterface defined incore/schemas/plugin.go.transports/bifrost-http/lib/config.go (6)
236-239: LGTM: Well-designed typed plugin segregation.The atomic pointer fields (
LLMPlugins,MCPPlugins,HTTPTransportPlugins) provide type-safe, lock-free access for different plugin domains. This design enables:
- Compile-time type checking for plugin types
- Zero-lock reads on hot paths (critical for performance)
- Clear separation of concerns for LLM vs. MCP plugin pipelines
The atomic.Pointer pattern correctly supports concurrent access with minimal overhead.
2200-2233: LGTM: Safe lock-free plugin accessors with defensive copying.All three
GetLoaded*methods correctly:
- Use atomic loads for consistent snapshots
- Handle nil pointers gracefully
- Return clones via
slices.Clone()to prevent external modificationsMinor suggestion: The comment "Do not modify the returned slice; it is a shared snapshot" might be misleading since
slices.Clone()returns a new slice, not a shared reference. Consider clarifying that the clone protects internal state rather than sharing it.
2235-2261: Verify acceptability of brief inconsistency window for HTTP transport plugin cache.The
rebuildHTTPTransportPluginsmethod correctly scans both LLM and MCP plugins forHTTPTransportPluginimplementations. However, there's a brief window of eventual inconsistency:
addLoadedLLMPlugin(line 2325) performs CAS to updateLLMPluginsrebuildHTTPTransportPlugins(line 2326) is called to rebuild cache- During step 2,
GetLoadedHTTPTransportPluginsmay return stale data if called concurrentlyImpact: The inconsistency window is very small (microseconds), and the cache is eventually consistent. For the expected usage pattern, this is likely acceptable.
Performance note: Rebuilding the cache on every plugin add/remove is O(n) where n = total plugin count. The comments mention "typically 5-10 plugins," so this should be fine.
Consider confirming:
- Is the eventual consistency acceptable for HTTP transport plugin discovery?
- Are there scenarios with >50 plugins where rebuild cost might matter?
2301-2361: LGTM: CAS loops correctly implement lock-free plugin addition with replace semantics.Both
addLoadedLLMPluginandaddLoadedMCPPlugincorrectly:
- Use CAS loops with retry for lock-free updates
- Handle nil initialization case (lines 2305-2312, 2336-2343)
- Implement replace-if-exists semantics: if a plugin with the same name exists, it's removed before adding the new one (lines 2317-2322, 2348-2353)
- Rebuild HTTP transport cache after successful update
Concurrency note: If two goroutines concurrently add different plugins with the same name, the last CAS to succeed wins. This is typical last-write-wins semantics for lock-free data structures and is likely acceptable.
2363-2421: LGTM: CAS loops correctly implement lock-free plugin removal.Both
removeLoadedLLMPluginandremoveLoadedMCPPlugincorrectly:
- Use CAS loops with retry for lock-free updates
- Handle nil pointer case with clear error (lines 2368, 2398)
- Create new slice without the removed plugin (lines 2372-2380, 2402-2410)
- Return error if plugin not found (lines 2383, 2413)
- Rebuild HTTP transport cache after successful removal (lines 2387, 2417)
Concurrency note: If two goroutines try to remove the same plugin concurrently, one will succeed and the other will receive a "plugin not found" error. This is acceptable behavior for concurrent removals.
2423-2449: LGTM: Plugin existence check correctly searches both plugin types.The
IsPluginLoadedmethod correctly:
- Checks both LLM and MCP plugin lists (lines 2429-2436, 2439-2446)
- Uses atomic loads for consistent snapshots
- Returns early on first match (efficient)
Performance assumption: The comment (line 2426) states "For small plugin counts, this is faster than maintaining a separate map." This is valid for the expected 5-10 plugins mentioned elsewhere in the codebase. Linear search is O(n+m) but has lower overhead than map lookups for small n and m.
Note: If plugin counts grow significantly (>50 plugins), consider profiling to determine if a map-based lookup would be beneficial.
core/bifrost.go (8)
55-78: Good separation of LLM vs MCP plugin domains in core state / pipeline.
The split intollmPlugins/mcpPluginsand addingmcpRequestPoollooks coherent with the new MCP hook surface.Also applies to: 81-97
223-239: MCPConfig wiring for plugin pipelines is solid; ensure context defaults are consistent.
ThePluginPipelineProvider/ReleasePluginPipelineplumbing looks right for nested tool execution. Consider also mirroring the common “nil ctx → bifrost.ctx” pattern in MCP entry points to avoid surprising nil derefs inside hooks.Also applies to: 2071-2088
289-310: ReloadConfig: atomic swap + cloning is correct.
Copying plugin slices before storing avoids external mutation issues and matches intended concurrency semantics.
1552-1737: Plugin lifecycle CAS loops look OK; consider status/error semantics.
The CAS-based replace/remove with cleanup-after-swap is reasonable. Small note: returningnilwhen not found is fine, but if external callers expect “not found” errors, confirm this behavior is intended across the stack.
3512-3615: MCP tool execution path is clean; ensure ctx/request-id/tracer expectations are met.
The prehook → execute → posthook flow is straightforward. Please confirm whether MCP tool execution should also ensure a request ID exists in context (like LLM paths do) for plugin correlation.
3620-3830: PluginPipeline hook implementations look consistent across LLM and MCP.
TherunFromclamping and reverse-order posthooks are good defensive measures; MCP hook parity looks correct.
4038-4055: MCP request pooling/reset is correct.
Resetting pointer fields avoids cross-request leakage.
4351-4367: Shutdown cleanup correctly covers both plugin domains.
Nice to see LLM + MCP plugins cleaned up independently.transports/bifrost-http/server/server.go (4)
85-118: Server-level split intoLLMPluginsandMCPPluginsis a good move.
This aligns the transport with the core’s domain separation.
1044-1131: RemovePlugin flow is consistent across core/config/server lists.
Nice job removing per-interface from core + config and then from in-memory slices. After the observability clearing fix above, this should behave well.
1133-1143: NewMCPInferenceHandlerroute registration: please verify middleware parity + streaming semantics.
Since this is meant to fix auth consistency, confirmhandlers.NewMCPInferenceHandleruses the same auth/trace middleware behavior as the regular inference handler and doesn’t regress SSE event framing.
219-309: Dynamic plugins are limited to LLM-only implementation; confirm whether MCP-only dynamic plugins are required.
LoadDynamicLLMPluginreturnsschemas.LLMPlugin, andDynamicLLMPluginimplements onlyPreLLMHookandPostLLMHook(no MCP hook fields). A dynamic plugin cannot implement justMCPPluginbecause the struct lacks the required interface methods. If MCP-only dynamic plugins are expected, both the loader andDynamicLLMPluginstruct would need refactoring to support separate MCP plugin paths.core/mcp/toolmanager.go (3)
22-106: PluginPipeline hook-in is a good decoupling; ensure nil-safe usage in code-mode paths.
pluginPipelineProvider/releasePluginPipelineare optional—please confirm any usage (e.g., executeCode nesting) guards nil before calling.
332-414: UnifiedExecuteTool(request)looks correct (validation + format conversion + latency).
The request-type switch, nil checks, and conversion back to Responses format are clean and should reduce duplicated logic.
515-562: Agent execution now correctly routes through the unified tool executor.
This simplifies the agent loop surface nicely.
Pratham-Mishra04
force-pushed
the
01-07-feat_plugin_schemas_segregation_for_mcp_plugins
branch
from
0b88c60 to
91a9ac3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
examples/plugins/hello-world-wasm-go/README.md (1)
97-122: PostLLMHook docs: correct both input and output field names to match the actual schema.The documentation shows
errin the input andbifrost_errorin the output, but the actual schema useserrorfor both. Plugin authors will fail to parse inputs/outputs if they follow this README.**PostLLMHook Input:** - `ctx`: Context JSON - `resp`: Bifrost response JSON -- `err`: Bifrost error JSON (or null) +- `error`: Bifrost error JSON (or null) **PostLLMHook Output:** ```json { "response": { ... }, - "bifrost_error": null, + "error": null, "error": "" }</blockquote></details> <details> <summary>plugins/mocker/main.go (1)</summary><blockquote> `861-864`: **Potential nil pointer dereference when overriding model.** At lines 862-864, `mockResponse.ChatResponse.Model` is accessed without checking if `ChatResponse` is nil. When `req.RequestType == schemas.ResponsesRequest`, `mockResponse.ChatResponse` is nil, but the model override code still attempts to access it. <details> <summary>🐛 Proposed fix</summary> ```diff // Override model if specified - if content.Model != nil { + if content.Model != nil && mockResponse.ChatResponse != nil { mockResponse.ChatResponse.Model = *content.Model }
plugins/semanticcache/search.go (2)
86-159: Fix potential nil deref on semantic search Score logging (panic risk).
Line 155 logs*result.Scorewithout a nil check;vectorstore.SearchResult.Scoreis a*float64and can be nil.Proposed fix
- plugin.logger.Debug(fmt.Sprintf("%s Found semantic match with ID: %s, Score: %f", PluginLoggerPrefix, result.ID, *result.Score)) + score := 0.0 + if result.Score != nil { + score = *result.Score + } + plugin.logger.Debug(fmt.Sprintf("%s Found semantic match with ID: %s, Score: %f", PluginLoggerPrefix, result.ID, score))
288-369: Avoid mutatingctxfrom the streaming goroutine (likely data race / brittle semantics).
buildStreamingResponseFromResultcallsctx.SetValue(...)both before returning and again inside the goroutine (Lines 298-310, 330). Ifschemas.BifrostContextisn’t explicitly safe for concurrent writes, this is a race; even if it is, it couples stream consumption with side effects on shared context.Suggested direction: set cache-hit markers once before spawning the goroutine, and avoid any further ctx mutations from the producer; communicate “stream end” via the final stream chunk payload (or a dedicated field) instead of ctx.
transports/bifrost-http/server/server.go (2)
1057-1144: HardenisDisabledcontext value casting to avoid panics.
RemovePlugindoesisDisabled != nil && isDisabled.(bool)which will panic if the value is present but not a bool.Proposed fix
- isDisabled := ctx.Value("isDisabled") - if isDisabled != nil && isDisabled.(bool) { + isDisabled := ctx.Value("isDisabled") + if v, ok := isDisabled.(bool); ok && v { s.UpdatePluginStatus(name, schemas.PluginStatusDisabled, []string{fmt.Sprintf("plugin %s is disabled", name)}) } else { s.DeletePluginStatus(name) }
219-309: Dynamic plugin loading explicitly supports only LLM plugins; MCP plugins cannot be dynamically loaded.In the
path != nilbranch,LoadPlugincallsdynamicPlugins.LoadLLMPlugins(...)and thedynamicPlugins.Configstruct only definesLLMPluginsfield—there is noMCPPluginsfield orLoadMCPPluginsfunction in the codebase. If MCP dynamic plugin support is intended, this capability gap should be addressed; otherwise, add an explicit comment clarifying that MCP plugins are builtin-only.
core/bifrost.go (1)
2753-2877: Post-hooks should usepreCount(or pipeline snapshot), not the current atomic plugin length.
IntryRequest, you computepluginCount := len(*bifrost.llmPlugins.Load())and pass that intopipeline.RunPostHooks(...). If plugins are hot-reloaded between pre and post, you can skip post-hooks (or run the wrong subset). You already havepreCountfromRunLLMPreHooks—that’s the correct “runFrom”.Proposed fix
- preReq, shortCircuit, preCount := pipeline.RunLLMPreHooks(ctx, req) + preReq, shortCircuit, preCount := pipeline.RunLLMPreHooks(ctx, req) ... - pluginCount := len(*bifrost.llmPlugins.Load()) select { case result = <-msg.Response: - resp, bifrostErr := pipeline.RunPostHooks(msg.Context, result, nil, pluginCount) + resp, bifrostErr := pipeline.RunPostHooks(msg.Context, result, nil, preCount) ... case bifrostErrVal := <-msg.Err: bifrostErrPtr := &bifrostErrVal - resp, bifrostErrPtr = pipeline.RunPostHooks(msg.Context, nil, bifrostErrPtr, pluginCount) + resp, bifrostErrPtr = pipeline.RunPostHooks(msg.Context, nil, bifrostErrPtr, preCount) ... }
🤖 Fix all issues with AI agents
In @core/bifrost.go:
- Around line 3548-3651: The short-circuit error branch in
handleMCPToolExecution discards any modifications from RunMCPPostHooks by
returning shortCircuit.Error directly; update that branch to capture the
post-hook results (e.g., finalResp, finalErr := pipeline.RunMCPPostHooks(ctx,
nil, shortCircuit.Error, preCount)) and then return finalErr if non-nil, else
return finalResp if non-nil, and only fall back to returning shortCircuit.Error
if both post-hook outputs are nil, ensuring post-hook transformations or
recovery are respected.
In @core/mcp/codemodeexecutecode.go:
- Around line 882-907: The code currently derives toolNameToCall from
toolCall.Function.Name (after pre-hooks) and passes it into the CallToolRequest,
which allows pre-hooks to modify the tool name; instead, always use the
originalToolName (computed earlier) when building the
mcp.CallToolRequest.Params.Name so pre-hook changes are ignored. Locate the
block creating toolNameToCall and the CallToolRequest (symbols: toolNameToCall,
originalToolName, toolCall.Function.Name, CallToolRequest, client.Conn.CallTool)
and remove/ignore the assignment from toolCall.Function.Name, setting
Params.Name to originalToolName before calling client.Conn.CallTool.
In @framework/plugins/loader.go:
- Line 7: The method name LoadDynamicLLMPlugin is misleading because it loads
any plugin type; rename the interface method to LoadDynamicPlugin and update its
signature (and all implementations and callers) to return a generic plugin
result (e.g., (any, error) or a common plugin interface such as schemas.Plugin
if available) instead of schemas.LLMPlugin so the loader can return LLM or MCP
plugin instances without post-load confusion; update all references
(implementations, call sites, tests, and docs) from LoadDynamicLLMPlugin to
LoadDynamicPlugin and adjust expected return handling accordingly.
🧹 Nitpick comments (5)
core/utils.go (1)
418-421: Prefer aswitchfor readability + easier extension.
[additionally: consider whether callers should pass normalized names (case, whitespace) or if this helper should normalize.]Proposed refactor
func IsCodemodeTool(toolName string) bool { - return toolName == mcp.ToolTypeListToolFiles || toolName == mcp.ToolTypeReadToolFile || toolName == mcp.ToolTypeExecuteToolCode + switch toolName { + case mcp.ToolTypeListToolFiles, mcp.ToolTypeReadToolFile, mcp.ToolTypeExecuteToolCode: + return true + default: + return false + } }plugins/governance/utils.go (1)
26-31: Consider clarifying the case handling for Authorization header parsing.The code uses
strings.ToLower(authHeader)to check for the "bearer " prefix (lowercase) but then slicesauthHeader[7:]directly. While this works correctly (both "Bearer " and "bearer " are 7 characters), the mixed case handling could be clearer.♻️ Optional refactor for consistency
authHeader := req.Headers["authorization"] if authHeader != "" { - if strings.HasPrefix(strings.ToLower(authHeader), "bearer ") { - authHeaderValue := strings.TrimSpace(authHeader[7:]) // Remove "Bearer " prefix + // Extract Bearer token (case-insensitive "Bearer " prefix) + const bearerPrefix = "bearer " + if strings.HasPrefix(strings.ToLower(authHeader), bearerPrefix) { + authHeaderValue := strings.TrimSpace(authHeader[len(bearerPrefix):]) if authHeaderValue != "" && strings.HasPrefix(strings.ToLower(authHeaderValue), VirtualKeyPrefix) { virtualKeyValue = authHeaderValue } } }This makes it explicit that the prefix length is derived from the comparison string, improving maintainability.
core/mcp/agent.go (1)
293-314: Consolidate redundant nil-handling branches.Lines 307-312 contain two branches that produce identical behavior: when
mcpResponse.ChatMessageis nil and whenmcpResponseis nil, both callcreateToolResultMessage(toolCall, "", nil). Consider consolidating:- } else if mcpResponse != nil && mcpResponse.ChatMessage == nil { - // Send empty result when mcpResponse is non-nil but ChatMessage is nil - channelToolResults <- createToolResultMessage(toolCall, "", nil) - } else { - // Fallback: send empty result when both mcpResponse and toolErr are nil + } else { + // Fallback: send empty result when mcpResponse is nil or ChatMessage is missing channelToolResults <- createToolResultMessage(toolCall, "", nil) }Also, the closing brace on line 314 is oddly formatted with the goroutine call—ensure consistent formatting.
core/mcp/mcp.go (1)
83-101: Silent type assertion failure may mask configuration errors.At lines 89-94, if
config.PluginPipelineProvider()returns a non-nil value that fails thePluginPipelinetype assertion, the function silently returnsnil. This could mask misconfiguration issues.Consider logging a warning when the type assertion fails to aid debugging:
if config.PluginPipelineProvider != nil && config.ReleasePluginPipeline != nil { pluginPipelineProvider = func() PluginPipeline { if pipeline := config.PluginPipelineProvider(); pipeline != nil { if pp, ok := pipeline.(PluginPipeline); ok { return pp + } else { + logger.Warn(MCPLogPrefix + " PluginPipelineProvider returned non-nil value that does not implement PluginPipeline interface") } } return nil }core/bifrost.go (1)
3959-3973: (Optional) Clear plugin slice references when returning pipelines to the pool.
resetPluginPipeline()doesn’t nilp.llmPlugins/p.mcpPlugins. This can keep old slice headers alive longer than needed (usually not a big deal since plugins are long-lived), but it’s cheap to clean.Proposed tweak
func (p *PluginPipeline) resetPluginPipeline() { p.executedPreHooks = 0 + p.llmPlugins = nil + p.mcpPlugins = nil p.preHookErrors = p.preHookErrors[:0] p.postHookErrors = p.postHookErrors[:0] ... }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/media/ui-disable-auth-on-inference.pngis excluded by!**/*.pngdocs/media/ui-enforce-virtual-keys.pngis excluded by!**/*.png
📒 Files selected for processing (61)
core/bifrost.gocore/mcp/agent.gocore/mcp/codemodeexecutecode.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/schemas/bifrost.gocore/schemas/mcp.gocore/schemas/plugin.gocore/schemas/plugin_native.gocore/schemas/trace.gocore/utils.godocs/architecture/core/plugins.mdxdocs/architecture/framework/streaming.mdxdocs/changelogs/v1.3.12.mdxdocs/features/governance/virtual-keys.mdxdocs/features/mcp/tool-execution.mdxdocs/features/observability/default.mdxdocs/features/observability/maxim.mdxdocs/features/observability/otel.mdxdocs/features/plugins/jsonparser.mdxdocs/features/plugins/mocker.mdxdocs/features/semantic-caching.mdxdocs/plugins/getting-started.mdxdocs/plugins/migration-guide.mdxdocs/plugins/writing-go-plugin.mdxdocs/quickstart/go-sdk/context-keys.mdxexamples/plugins/hello-world-wasm-go/README.mdexamples/plugins/hello-world/main.goframework/plugins/loader.goframework/plugins/main.goframework/plugins/soloader.goframework/plugins/soplugin.goframework/plugins/soplugin_test.goplugins/governance/main.goplugins/governance/utils.goplugins/jsonparser/main.goplugins/jsonparser/plugin_test.goplugins/logging/main.goplugins/maxim/main.goplugins/maxim/plugin_test.goplugins/mocker/benchmark_test.goplugins/mocker/main.goplugins/mocker/plugin_test.goplugins/otel/main.goplugins/semanticcache/main.goplugins/semanticcache/plugin_core_test.goplugins/semanticcache/plugin_integration_test.goplugins/semanticcache/search.goplugins/semanticcache/test_utils.goplugins/telemetry/main.gotransports/bifrost-http/handlers/cache.gotransports/bifrost-http/handlers/devpprof.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/lib.gotransports/bifrost-http/server/server.gotransports/changelog.mdui/app/workspace/config/views/securityView.tsx
💤 Files with no reviewable changes (1)
- transports/bifrost-http/handlers/mcp.go
✅ Files skipped from review due to trivial changes (1)
- transports/changelog.md
🚧 Files skipped from review as they are similar to previous changes (16)
- plugins/jsonparser/plugin_test.go
- docs/plugins/migration-guide.mdx
- plugins/mocker/benchmark_test.go
- plugins/semanticcache/test_utils.go
- transports/bifrost-http/handlers/devpprof.go
- plugins/semanticcache/plugin_integration_test.go
- plugins/semanticcache/plugin_core_test.go
- docs/quickstart/go-sdk/context-keys.mdx
- docs/plugins/getting-started.mdx
- docs/architecture/framework/streaming.mdx
- ui/app/workspace/config/views/securityView.tsx
- docs/features/observability/otel.mdx
- core/schemas/mcp.go
- plugins/jsonparser/main.go
- examples/plugins/hello-world/main.go
- docs/changelogs/v1.3.12.mdx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
plugins/governance/utils.godocs/features/mcp/tool-execution.mdxplugins/maxim/plugin_test.godocs/features/semantic-caching.mdxtransports/bifrost-http/handlers/cache.goplugins/otel/main.goplugins/mocker/plugin_test.godocs/features/observability/default.mdxplugins/telemetry/main.gocore/mcp/agent.gotransports/bifrost-http/handlers/mcpserver.gocore/schemas/plugin_native.goframework/plugins/main.goplugins/mocker/main.goframework/plugins/loader.goframework/plugins/soplugin_test.gocore/schemas/plugin.godocs/features/observability/maxim.mdxcore/utils.godocs/features/plugins/jsonparser.mdxdocs/plugins/writing-go-plugin.mdxcore/mcp/codemodeexecutecode.godocs/features/governance/virtual-keys.mdxplugins/maxim/main.gotransports/bifrost-http/lib/lib.gocore/mcp/mcp.gocore/schemas/bifrost.gotransports/bifrost-http/lib/config.goplugins/semanticcache/search.goexamples/plugins/hello-world-wasm-go/README.mdplugins/semanticcache/main.goframework/plugins/soplugin.gotransports/bifrost-http/handlers/mcpinference.gocore/mcp/toolmanager.goplugins/governance/main.goframework/plugins/soloader.godocs/features/plugins/mocker.mdxplugins/logging/main.gotransports/bifrost-http/server/server.gocore/bifrost.gocore/schemas/trace.gotransports/bifrost-http/handlers/middlewares.godocs/architecture/core/plugins.mdx
🧠 Learnings (7)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
plugins/governance/utils.goplugins/maxim/plugin_test.gotransports/bifrost-http/handlers/cache.goplugins/otel/main.goplugins/mocker/plugin_test.goplugins/telemetry/main.gocore/mcp/agent.gotransports/bifrost-http/handlers/mcpserver.gocore/schemas/plugin_native.goframework/plugins/main.goplugins/mocker/main.goframework/plugins/loader.goframework/plugins/soplugin_test.gocore/schemas/plugin.gocore/utils.gocore/mcp/codemodeexecutecode.goplugins/maxim/main.gotransports/bifrost-http/lib/lib.gocore/mcp/mcp.gocore/schemas/bifrost.gotransports/bifrost-http/lib/config.goplugins/semanticcache/search.goplugins/semanticcache/main.goframework/plugins/soplugin.gotransports/bifrost-http/handlers/mcpinference.gocore/mcp/toolmanager.goplugins/governance/main.goframework/plugins/soloader.goplugins/logging/main.gotransports/bifrost-http/server/server.gocore/bifrost.gocore/schemas/trace.gotransports/bifrost-http/handlers/middlewares.go
📚 Learning: 2025-12-29T11:54:55.836Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1153
File: framework/configstore/rdb.go:2221-2246
Timestamp: 2025-12-29T11:54:55.836Z
Learning: In Go reviews, do not flag range-over-int patterns like for i := range n as compile-time errors, assuming Go 1.22+ semantics. Only flag actual range-capable values (slices, arrays, maps, channels, strings) and other compile-time issues. This applies to all Go files across the repository.
Applied to files:
plugins/governance/utils.goplugins/maxim/plugin_test.gotransports/bifrost-http/handlers/cache.goplugins/otel/main.goplugins/mocker/plugin_test.goplugins/telemetry/main.gocore/mcp/agent.gotransports/bifrost-http/handlers/mcpserver.gocore/schemas/plugin_native.goframework/plugins/main.goplugins/mocker/main.goframework/plugins/loader.goframework/plugins/soplugin_test.gocore/schemas/plugin.gocore/utils.gocore/mcp/codemodeexecutecode.goplugins/maxim/main.gotransports/bifrost-http/lib/lib.gocore/mcp/mcp.gocore/schemas/bifrost.gotransports/bifrost-http/lib/config.goplugins/semanticcache/search.goplugins/semanticcache/main.goframework/plugins/soplugin.gotransports/bifrost-http/handlers/mcpinference.gocore/mcp/toolmanager.goplugins/governance/main.goframework/plugins/soloader.goplugins/logging/main.gotransports/bifrost-http/server/server.gocore/bifrost.gocore/schemas/trace.gotransports/bifrost-http/handlers/middlewares.go
📚 Learning: 2025-12-22T10:50:40.990Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1154
File: plugins/governance/store.go:1165-1186
Timestamp: 2025-12-22T10:50:40.990Z
Learning: In the Bifrost governance plugin, budgets and rate limits have 1:1 relationships with their parent entities (virtual keys, teams, customers). Do not assume sharing; ensure cascade deletion logic only deletes budgets/rate limits when there are no shared references. Enforce invariants in code and add tests to verify no cross-entity sharing and that cascade deletes only remove the specific child of the parent. If a counterexample arises, adjust data model or add guards.
Applied to files:
plugins/governance/utils.goplugins/governance/main.go
📚 Learning: 2025-12-30T05:37:48.365Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1180
File: docs/features/mcp/connecting-to-servers.mdx:452-458
Timestamp: 2025-12-30T05:37:48.365Z
Learning: When reviewing documentation PRs in a Graphite-managed stack, first check related or previous PRs in the stack for feature implementations before flagging documentation as incorrect or unsupported. Documentation MDX files often reference features implemented in earlier stack PRs; verify that the documented behavior exists in earlier changes and that the docs accurately reflect the implemented state before requesting edits.
Applied to files:
docs/features/mcp/tool-execution.mdxdocs/features/semantic-caching.mdxdocs/features/observability/default.mdxdocs/features/observability/maxim.mdxdocs/features/plugins/jsonparser.mdxdocs/plugins/writing-go-plugin.mdxdocs/features/governance/virtual-keys.mdxdocs/features/plugins/mocker.mdxdocs/architecture/core/plugins.mdx
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/handlers/cache.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/lib/lib.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/middlewares.go
📚 Learning: 2025-12-29T09:14:16.633Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 888
File: transports/bifrost-http/handlers/middlewares.go:246-256
Timestamp: 2025-12-29T09:14:16.633Z
Learning: In the bifrost HTTP transport, fasthttp.RequestCtx is the primary context carrier and should be passed directly to functions that expect a context.Context. Do not convert to context.Context unless explicitly required. Ensure tracer implementations and related components are designed to accept fasthttp.RequestCtx directly, and document this architectural decision for maintainers.
Applied to files:
transports/bifrost-http/handlers/cache.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/lib/lib.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/middlewares.go
📚 Learning: 2026-01-10T08:07:42.582Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1299
File: core/mcp/codemodeexecutecode.go:882-907
Timestamp: 2026-01-10T08:07:42.582Z
Learning: In the MCP tool execution flow (core/mcp/codemodeexecutecode.go callMCPTool function), pre-hook modifications to tool names should NOT be applied. The originalToolName computed before pre-hooks must be used in the final MCP CallToolRequest. Only argument modifications from pre-hooks should be honored. This is a security/consistency decision to prevent plugins from redirecting tool execution.
Applied to files:
core/mcp/codemodeexecutecode.go
🧬 Code graph analysis (27)
plugins/governance/utils.go (3)
core/schemas/plugin.go (1)
HTTPRequest(30-36)core/utils.go (1)
Ptr(57-59)plugins/governance/main.go (1)
VirtualKeyPrefix(29-29)
plugins/maxim/plugin_test.go (2)
core/schemas/plugin.go (1)
LLMPlugin(135-140)core/schemas/account.go (1)
Account(81-97)
transports/bifrost-http/handlers/cache.go (1)
core/schemas/plugin.go (1)
LLMPlugin(135-140)
plugins/otel/main.go (5)
examples/plugins/hello-world/main.go (2)
PreLLMHook(28-34)PostLLMHook(36-43)core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/bifrost.go (3)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(527-536)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
plugins/mocker/plugin_test.go (2)
core/schemas/account.go (1)
Account(81-97)core/schemas/plugin.go (1)
LLMPlugin(135-140)
core/mcp/agent.go (3)
core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/bifrost.go (4)
BifrostMCPRequest(358-363)BifrostMCPResponse(442-446)RequestType(88-88)MCPRequestTypeChatToolCall(350-350)core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(756-761)
transports/bifrost-http/handlers/mcpserver.go (2)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(756-761)core/schemas/bifrost.go (1)
BifrostError(527-536)
core/schemas/plugin_native.go (1)
core/schemas/bifrost.go (4)
BifrostResponse(368-389)BifrostStream(492-499)BifrostError(527-536)BifrostMCPResponse(442-446)
framework/plugins/main.go (2)
core/schemas/plugin.go (3)
BasePlugin(119-127)LLMPlugin(135-140)MCPPlugin(142-147)framework/plugins/loader.go (1)
PluginLoader(6-8)
plugins/mocker/main.go (5)
examples/plugins/hello-world/main.go (2)
PreLLMHook(28-34)PostLLMHook(36-43)core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/bifrost.go (3)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(527-536)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
framework/plugins/loader.go (1)
core/schemas/plugin.go (1)
LLMPlugin(135-140)
framework/plugins/soplugin_test.go (5)
framework/plugins/main.go (3)
DynamicLLMPluginConfig(8-13)LoadLLMPlugins(39-55)Config(16-18)core/schemas/plugin.go (1)
HTTPTransportPlugin(129-133)examples/plugins/hello-world/main.go (3)
HTTPTransportIntercept(18-26)PreLLMHook(28-34)PostLLMHook(36-43)core/schemas/context.go (1)
NewBifrostContextWithTimeout(83-86)framework/plugins/soplugin.go (1)
DynamicLLMPlugin(10-24)
core/schemas/plugin.go (3)
examples/plugins/hello-world/main.go (5)
GetName(14-16)Cleanup(45-48)HTTPTransportIntercept(18-26)PreLLMHook(28-34)PostLLMHook(36-43)core/schemas/plugin_native.go (2)
LLMPluginShortCircuit(16-20)MCPPluginShortCircuit(24-27)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
core/utils.go (1)
core/mcp/toolmanager.go (3)
ToolTypeListToolFiles(51-51)ToolTypeReadToolFile(52-52)ToolTypeExecuteToolCode(53-53)
transports/bifrost-http/lib/lib.go (1)
core/schemas/plugin.go (1)
LLMPlugin(135-140)
core/mcp/mcp.go (4)
core/bifrost.go (1)
PluginPipeline(81-97)core/mcp/toolmanager.go (2)
PluginPipeline(45-48)NewToolsManager(69-106)core/mcp.go (1)
MCPManager(49-56)core/schemas/bifrost.go (2)
BifrostMCPRequest(358-363)BifrostMCPResponse(442-446)
core/schemas/bifrost.go (3)
core/schemas/plugin.go (2)
LLMPlugin(135-140)MCPPlugin(142-147)core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(756-761)core/schemas/responses.go (2)
ResponsesToolMessage(471-491)ResponsesMessage(319-332)
transports/bifrost-http/lib/config.go (2)
core/schemas/plugin.go (7)
LLMPlugin(135-140)MCPPlugin(142-147)HTTPTransportPlugin(129-133)BasePlugin(119-127)PluginType(81-81)PluginTypeLLM(84-84)PluginTypeMCP(85-85)framework/plugins/loader.go (1)
PluginLoader(6-8)
plugins/semanticcache/search.go (3)
core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)framework/vectorstore/store.go (1)
SearchResult(46-50)
framework/plugins/soplugin.go (2)
core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
transports/bifrost-http/handlers/mcpinference.go (3)
transports/bifrost-http/lib/middleware.go (1)
ChainMiddlewares(11-23)transports/bifrost-http/handlers/utils.go (2)
SendError(35-44)SendBifrostError(47-62)transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(81-409)
plugins/governance/main.go (7)
examples/plugins/hello-world/main.go (2)
PreLLMHook(28-34)PostLLMHook(36-43)core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/bifrost.go (3)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(527-536)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)ui/lib/types/logs.ts (1)
BifrostError(234-240)core/utils.go (1)
GetStringFromContext(306-313)
framework/plugins/soloader.go (4)
core/schemas/plugin.go (1)
LLMPlugin(135-140)framework/plugins/soplugin.go (1)
DynamicLLMPlugin(10-24)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
plugins/logging/main.go (3)
core/schemas/bifrost.go (2)
BifrostResponse(368-389)BifrostError(527-536)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
transports/bifrost-http/server/server.go (5)
core/schemas/plugin.go (3)
LLMPlugin(135-140)MCPPlugin(142-147)BasePlugin(119-127)framework/plugins/main.go (4)
Config(16-18)LoadLLMPlugins(39-55)AsLLMPlugin(22-27)AsMCPPlugin(31-36)transports/bifrost-http/lib/config.go (1)
Config(212-246)core/schemas/bifrost.go (1)
BifrostConfig(18-28)core/schemas/mcp.go (1)
MCPConfig(16-34)
core/bifrost.go (2)
core/schemas/plugin.go (3)
LLMPlugin(135-140)MCPPlugin(142-147)BasePlugin(119-127)core/mcp/toolmanager.go (1)
PluginPipeline(45-48)
transports/bifrost-http/handlers/middlewares.go (1)
core/schemas/plugin.go (2)
BasePlugin(119-127)ObservabilityPlugin(173-187)
🔇 Additional comments (65)
core/schemas/trace.go (1)
124-125: Comment-only rename looks consistent with PreLLMHook/PostLLMHook.
No functional impact; aligns with the repo-wide hook rename mentioned in the stack context.docs/plugins/writing-go-plugin.mdx (2)
100-114: Terminology/type rename is applied consistently across the page (PreLLMHook/PostLLMHook + LLMPluginShortCircuit).
Matches the stated direction of the stack (LLM vs MCP plugin separation + hook rename).Also applies to: 155-169, 259-305, 481-489, 497-510, 518-530, 532-539, 443-459, 789-797, 834-839
124-179: The v1.3.x tab uses the correct hook names (PreLLMHook/PostLLMHook) for that version. Both the v1.3.x and v1.4.x+ tabs use PreLLMHook/PostLLMHook with LLMPluginShortCircuit, which is accurate. However, there is a real discrepancy: the v1.3.x tab showsTransportInterceptorwith an outdated signature (url, headers, body parameters), while v1.4.x+ correctly showsHTTPTransportInterceptwith the current signature (HTTPRequest/HTTPResponse types). The v1.3.x tab should be updated to reflect the correct HTTP transport hook interface for that version.Likely an incorrect or invalid review comment.
core/utils.go (1)
3-16: No import cycle risk detected. Thecore/mcppackage only importscore/schemas, not back into thecorepackage orcore/utils. Adding thecore/mcpimport tocore/utils.gois safe.Likely an incorrect or invalid review comment.
docs/features/semantic-caching.mdx (1)
136-140: Go SDK example is accurate—no config schema drift.The
LLMPluginsfield in the code snippet is correct for SDK initialization. The config.json schema maintains a unified"plugins"field by design; at runtime, theLoadPlugins()function distributes plugins intoLLMPluginsandMCPPluginsarrays based on which interfaces each plugin implements (a plugin can implement both). No documentation inconsistency exists, so the doc as written is sound across the stack.docs/features/mcp/tool-execution.mdx (1)
21-32: Documentation aligns with PR objectives.The Authentication section correctly documents that the
/v1/mcp/tool/executeendpoint shares authentication behavior with other inference endpoints, which aligns with the PR's stated goal of "separate mcp inference handler for auth consistency." The table clearly describes the two authentication modes based on thedisable_auth_on_inferenceconfiguration flag.docs/features/observability/maxim.mdx (1)
45-45: Documentation correctly reflects the API change.The update from
Plugins: []schemas.Plugin{maximPlugin}toLLMPlugins: []schemas.LLMPlugin{maximPlugin}aligns with the plugin schema segregation introduced in this PR. The Maxim plugin is correctly categorized as an LLM plugin for observability purposes.transports/bifrost-http/handlers/middlewares.go (2)
55-55: Improved specificity for HTTP transport plugins.The change from
GetLoadedPlugins()toGetLoadedHTTPTransportPlugins()correctly narrows the scope to only HTTP transport plugins, which is semantically appropriate for this middleware that runs HTTP transport interceptors. This aligns with the plugin domain segregation introduced in the PR.
444-456: Correct adaptation to BasePlugin hierarchy.The signature change from
[]schemas.Pluginto[]schemas.BasePlugincorrectly reflects the new plugin base interface hierarchy. The type assertionplugin.(schemas.ObservabilityPlugin)at line 451 remains valid sinceObservabilityPluginembedsBasePlugin, maintaining the filtering logic while aligning with the plugin domain segregation.plugins/mocker/plugin_test.go (1)
65-69: LGTM! Consistent test updates for new plugin API.All test initializations correctly updated from
Plugins: []schemas.PlugintoLLMPlugins: []schemas.LLMPlugin, aligning with the plugin schema segregation changes.Also applies to: 108-112, 184-188, 263-267, 326-330, 396-400
transports/bifrost-http/handlers/cache.go (1)
15-15: LGTM! Constructor signature updated to match new plugin API.The signature change from
schemas.Plugintoschemas.LLMPluginaligns with the plugin schema segregation. The runtime type assertion to*semanticcache.Pluginremains necessary and correct.docs/architecture/core/plugins.mdx (1)
151-161: LGTM! Comprehensive documentation update for new hook naming.All references to
PreHook/PostHookhave been consistently updated toPreLLMHook/PostLLMHookacross sequence diagrams, flowcharts, and narrative text. The documentation accurately reflects the plugin schema segregation changes.Also applies to: 181-190, 206-226, 264-264, 278-292, 306-320, 336-350
core/schemas/plugin_native.go (2)
14-20: LGTM! Clean rename for LLM plugin short-circuit type.The rename from
PluginShortCircuittoLLMPluginShortCircuitprovides clear domain specificity while preserving all existing fields and behavior.
22-27: MCP streaming is supported through a different architectural pattern—no changes needed.MCP tool execution does support streaming, but it's handled at the message content level through
ResponsesStreamResponseTypeenum values (e.g.,MCP_CALL_ARGUMENTS_DELTA,MCP_CALL_ARGUMENTS_DONE), not at the response channel level like LLM responses. The absence of aStreamfield inMCPPluginShortCircuitis intentional: MCP tool responses are always discrete, singleBifrostMCPResponseobjects, while LLM responses stream continuously via channel. This architectural difference is by design and requires no changes.plugins/maxim/plugin_test.go (2)
24-26: LGTM! Helper function updated to return LLMPlugin type.The
getPlugin()return type correctly changed fromschemas.Plugintoschemas.LLMPlugin, with corresponding comment update.
97-100: LGTM! Test configuration updated for new plugin API.The Bifrost initialization correctly updated from
PluginstoLLMPluginsfield, consistent with the schema segregation changes.transports/bifrost-http/handlers/mcpserver.go (1)
26-30: Interface signature change looks correct.The
ExecuteChatMCPToolmethod now takes a pointer toChatAssistantMessageToolCall, aligning with the unified MCP request/response flow introduced in this PR. The call site at line 242 correctly passes a pointer to the locally-constructedtoolCall.plugins/semanticcache/main.go (3)
261-263: Init return type correctly updated toschemas.LLMPlugin.The function signature now returns
schemas.LLMPlugininstead of the formerschemas.Plugin, aligning with the per-domain plugin architecture.
343-355:PreLLMHooksignature and return type correctly updated.The hook is renamed from
PreHooktoPreLLMHookwith the return type now including*schemas.LLMPluginShortCircuit. The implementation logic remains unchanged.
424-447:PostLLMHooksignature correctly updated.The hook is renamed from
PostHooktoPostLLMHook. The signature(*schemas.BifrostResponse, *schemas.BifrostError, error)matches theLLMPlugininterface definition shown incore/schemas/plugin.go.plugins/otel/main.go (1)
189-199: Hook renames toPreLLMHookandPostLLMHookare correct.Both hooks remain no-ops as documented, with tracing handled via the
Injectmethod. The return type forPreLLMHookcorrectly uses*schemas.LLMPluginShortCircuit.docs/features/plugins/mocker.mdx (3)
33-36: Documentation correctly updated to useLLMPlugins.The example code now uses
LLMPlugins: []schemas.LLMPlugin{plugin}instead of the formerPlugins: []schemas.Plugin{plugin}, matching the updatedBifrostConfigAPI.
165-169: Second example consistently updated.
561-565: Debug mode example consistently updated.docs/features/observability/default.mdx (2)
46-48: Workflow hook names correctly updated toPreLLMHookandPostLLMHook.The documentation accurately reflects the renamed hooks in the observability workflow description.
184-187: Go SDK example correctly updated to useLLMPlugins.The example now uses
LLMPlugins: []schemas.LLMPlugin{loggingPlugin}, matching the updatedBifrostConfigAPI.docs/features/plugins/jsonparser.mdx (3)
49-54: AllRequests example correctly updated to useLLMPlugins.The example now uses
LLMPlugins: []schemas.LLMPlugin{}and the comment at line 61 correctly referencesPostLLMHook.
87-92: PerRequest example consistently updated.
204-207: Real-life streaming example consistently updated.transports/bifrost-http/lib/lib.go (1)
18-22:EnterpriseOverridesinterface method signature verified.The
LoadGovernancePluginmethod returnsschemas.LLMPlugin, which is semantically appropriate for governance plugins. However, the original comment's claim about a breaking change fromschemas.Plugintoschemas.LLMPlugincannot be verified—schemas.Plugindoes not exist in the codebase. Additionally, no implementations ofEnterpriseOverridesare present in the bifrost repository; this interface appears to be an extension point for external enterprise implementations.docs/features/governance/virtual-keys.mdx (1)
545-620: LGTM! Well-documented authentication layer.The new "Authentication and Virtual Keys" section clearly documents the independent two-layer model (authentication vs. virtual keys) with comprehensive examples for both
disable_auth_on_inference: trueandfalsescenarios. The configuration examples across UI, API, and config.json are consistent and thorough.plugins/logging/main.go (3)
96-156: LGTM! Efficient pool and cleanup design.The addition of sync.Pool for LogMessage and UpdateLogData with 1000 prewarmed entries is a good performance optimization for high-throughput logging. The cleanup ticker (1-minute intervals for 30-minute old logs) provides appropriate background maintenance without excessive overhead.
199-328: LGTM! Correct PreLLMHook implementation.The method rename from PreHook to PreLLMHook aligns with the LLMPlugin interface refactor. Pool usage pattern is correct (acquire, defer release, pass to goroutine), and error handling appropriately logs issues without failing the request flow.
330-624: LGTM! Correct PostLLMHook implementation.The PostLLMHook method properly implements the LLMPlugin interface with correct pool management for both LogMessage and UpdateLogData. Stream accumulator cleanup is appropriately delegated to the tracer (as noted in the comment), avoiding duplicate cleanup logic.
core/mcp/codemodeexecutecode.go (2)
773-843: LGTM! Robust plugin pipeline integration.The plugin pipeline setup properly handles BifrostContext type checking, creates child contexts for nested calls with parent-child tracking, and gracefully falls back to direct execution when the pipeline is unavailable. The comment at lines 796-799 correctly explains why
NewBifrostContext()is necessary to avoid overwriting the parent's request ID.
976-1044: LGTM! Clean fallback and helper implementations.The
callMCPToolDirectfunction provides symmetric functionality for non-Bifrost contexts, andextractResultFromChatMessagehandles JSON parsing with appropriate fallback to string representation.framework/plugins/main.go (3)
8-18: LGTM! Clean domain-specific configuration.The rename from
DynamicPluginConfigtoDynamicLLMPluginConfigand the addition of theConfigfield provide clear domain separation and flexible per-plugin configuration. The JSON tag update tollm_pluginsmaintains API clarity.
20-36: LGTM! Useful type-safe interface adapters.The
AsLLMPluginandAsMCPPluginhelper functions provide clean, type-safe interface adaptation with appropriate nil returns when the plugin doesn't implement the target interface.
38-55: LGTM! Consistent LLM plugin loading.The
LoadLLMPluginsfunction correctly loads LLM-domain plugins using the updated configuration structure and loader method. The logic preserves error handling and disabled plugin filtering.transports/bifrost-http/handlers/mcpinference.go (2)
33-46: LGTM! Clean format-based routing.The
executeTooldispatcher correctly handles case-insensitive format parameters with sensible defaults (empty → chat) and appropriate error responses for invalid values.
48-79: LGTM! Robust request handling with proper cleanup.Both
executeChatMCPToolandexecuteResponsesMCPToolimplement consistent patterns:
- Input validation for required fields
- Context conversion with deferred cleanup (preventing context leaks)
- Appropriate error handling and response formatting
The security model correctly delegates authentication to middlewares and leverages the Bifrost client's plugin pipeline for tool execution.
Also applies to: 81-112
core/mcp/mcp.go (1)
132-147: LGTM!The unified
ExecuteToolCallmethod cleanly replaces the previous per-format entry points (ExecuteChatTool,ExecuteResponsesTool) with a single MCP-based flow. The documentation accurately describes the request/response types and delegation toToolsManager.ExecuteTool.plugins/mocker/main.go (1)
486-488: LGTM!The hook renames from
PreHook/PostHooktoPreLLMHook/PostLLMHookare consistent with the broader plugin schema segregation. Return types correctly use*schemas.LLMPluginShortCircuit.Also applies to: 554-557
plugins/governance/main.go (3)
44-45: LGTM!The
BaseGovernancePlugininterface correctly updates hook signatures toPreLLMHook/PostLLMHookwith*schemas.LLMPluginShortCircuitreturn types, aligning with the broader plugin schema segregation.
437-455: LGTM!The
PreLLMHookimplementation correctly:
- Uses
bifrost.GetStringFromContextfor consistent context value extraction- Returns
*schemas.LLMPluginShortCircuitfor short-circuit responses- Maintains proper governance decision flow with appropriate HTTP status codes (401, 402, 403, 429)
539-546: LGTM!The
PostLLMHookimplementation correctly usesbifrost.GetStringFromContextfor extracting virtual key and request ID, maintaining consistency with the updated context utility pattern.framework/plugins/soplugin_test.go (3)
31-42: LGTM!Test configuration correctly updated from
PluginstoLLMPluginsand fromLoadPluginstoLoadLLMPlugins, maintaining consistency with the renamed plugin loading infrastructure.
73-75: LGTM!The type assertion to
schemas.HTTPTransportPluginis a good practice for explicitly verifying the plugin implements the expected interface before callingHTTPTransportIntercept.
83-108: LGTM!Test correctly renamed to
PreLLMHookwith updated method call and assertion messages reflecting the new terminology.plugins/maxim/main.go (3)
35-60: LGTM!The Plugin struct documentation and
Initfunction correctly reflect theschemas.LLMPlugininterface implementation. Return type updated fromschemas.Plugintoschemas.LLMPlugin.
214-235: LGTM!The
PreLLMHookmethod documentation is comprehensive and accurately describes the trace/generation tracking behavior. Return type correctly uses*schemas.LLMPluginShortCircuit.
454-474: LGTM!The
PostLLMHookmethod documentation accurately describes the trace completion behavior. Return signature matches theLLMPlugininterface requirements.framework/plugins/soloader.go (2)
14-16: LGTM!The loader function correctly renamed from
LoadDynamicPlugintoLoadDynamicLLMPluginwith return typeschemas.LLMPlugin. InstantiatesDynamicLLMPlugininstead ofDynamicPlugin.
68-83: LGTM!Symbol lookups correctly updated:
PreHook→PreLLMHookwith*schemas.LLMPluginShortCircuitin the type assertionPostHook→PostLLMHookwith updated error messageError messages accurately describe the expected function signatures.
core/schemas/plugin.go (1)
80-147: LGTM! Clean plugin architecture refactoring.The refactoring from a single
Plugininterface to domain-specific interfaces (LLMPlugin,MCPPlugin) with a sharedBasePluginbase is well-designed. The composition-based approach allows plugins to implement multiple interfaces (e.g., bothLLMPluginandHTTPTransportPlugin), which provides flexibility for cross-cutting concerns.The documentation clearly explains the lifecycle and error handling semantics for each plugin type.
core/schemas/bifrost.go (2)
20-21: LGTM! Improved type safety with domain-specific plugin collections.The split from a single
Pluginsfield to separateLLMPluginsandMCPPluginsfields provides better type safety and clarity about which plugins are invoked in which contexts.
347-467: LGTM! MCP types follow established patterns.The new MCP request/response types follow the same design patterns as the existing
BifrostRequest/BifrostResponsetypes:
- Request type enum for routing
- Embedded pointers for variant types
- ExtraFields for metadata
The design is consistent and well-structured.
framework/plugins/soplugin.go (1)
10-52: LGTM! Consistent renaming to LLM-specific plugin.The renaming from
DynamicPlugintoDynamicLLMPluginand the hook method renames (PreHook→PreLLMHook,PostHook→PostLLMHook) are consistent throughout the file. The updated types align with the new plugin architecture.transports/bifrost-http/lib/config.go (3)
2200-2233: LGTM! Lock-free plugin accessors with proper isolation.The lock-free plugin accessors using atomic pointers and
slices.Cloneprovide good performance characteristics for hot paths. The use ofslices.Cloneensures that callers cannot modify the slice structure, although the plugin instances themselves remain shared (which is acceptable since plugins should be immutable after loading).The clear documentation about read-only usage is helpful.
2235-2261: LGTM! HTTPTransportPlugin cache correctly scans all plugin types.The
rebuildHTTPTransportPluginsmethod correctly scans both LLM and MCP plugin collections forHTTPTransportPluginimplementations. This ensures that any plugin implementing theHTTPTransportPlugininterface will be discovered and cached, regardless of its primary type.The cache is rebuilt atomically after each plugin add/remove operation.
2263-2421: LGTM! Plugin add/remove operations are thread-safe and maintain cache consistency.The CAS-based add/remove operations for plugins are correctly implemented:
- Atomic updates prevent race conditions
- Plugin replacement correctly removes old instances with the same name
rebuildHTTPTransportPluginsis called after successful updates to maintain cache consistency- Error handling provides clear feedback for type mismatches and missing plugins
The implementation ensures that the
HTTPTransportPluginscache remains synchronized with the LLM/MCP plugin collections.plugins/telemetry/main.go (1)
28-28: LGTM! Telemetry plugin updated to LLMPlugin interface.The telemetry plugin has been cleanly updated to implement the
LLMPlugininterface:
- Documentation updated to reflect
LLMPlugininterface (line 28)- Hooks renamed to
PreLLMHook/PostLLMHookwith updated signatures- Warning messages updated to reference new hook names
- No logic changes, only interface alignment
The changes are consistent and maintain the plugin's functionality while adapting to the new plugin architecture.
Also applies to: 289-294, 296-470
core/mcp/toolmanager.go (2)
56-106: NewPluginPipelineplumbing looks fine; ensure nil-safe usage at call sites.
NewToolsManagerstorespluginPipelineProvider/releasePluginPipelinebut (in this snippet) they aren’t used yet—please ensure all usage sites guard nil and always release on all paths.
332-414: API compatibility verified for v0.43.2.All concerns are addressed:
mcp-go v0.43.2has stable definitions forCallToolRequest,CallToolParams, andMethodToolsCall; the conversion methodsToChatAssistantMessageToolCall()andToResponsesToolMessage()are properly implemented; and the unifiedExecuteToolpath correctly handles both Chat and Responses formats throughout the codebase.core/bifrost.go (1)
3770-3865: No action required. ThePostMCPHookinterface signature incore/schemas/plugin.go:146correctly returns(*BifrostMCPResponse, *BifrostError, error), which matches the assignment inRunMCPPostHooksatcore/bifrost.go:3841. There is no compile break or semantic mismatch.Likely an incorrect or invalid review comment.
Pratham-Mishra04
force-pushed
the
01-07-feat_plugin_schemas_segregation_for_mcp_plugins
branch
from
91a9ac3 to
97d1b5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/features/governance/virtual-keys.mdx (1)
491-543: Avoid ambiguity between “governance enabled” vs “virtual keys enforced”, and call out the legacy field name.Two doc tweaks to reduce confusion:
- Reword “All governance-enabled requests must include…” to something like “When enforcement is enabled, requests must include…”.
- Since the UI says “Enforce Virtual Keys” but the config/API field is still
enforce_governance_header, add a short note that the field name is legacy.docs/features/plugins/jsonparser.mdx (1)
49-62: Fix missingcontextimport and addbifrostimport alias to make snippets copy-pasteable.The first snippet (lines 49–62) is missing the
contextimport required forcontext.Background(). Additionally, all three code examples (lines 49–62, 87–93, and 203–208) callbifrost.Init()but import the package as"github.com/maximhq/bifrost/core"without an import alias, which will not compile.Import fix for all three snippets
import ( + "context" "time" - "github.com/maximhq/bifrost/core" + bifrost "github.com/maximhq/bifrost/core" "github.com/maximhq/bifrost/core/schemas" "github.com/maximhq/bifrost/plugins/jsonparser" )examples/plugins/hello-world-wasm-go/README.md (1)
97-122: PostLLMHook Output JSON schema is incorrect in documentation.The README shows
"bifrost_error": nullas an output key, but the actual WASM ABI struct (PostHookOutput) uses"error"(not"bifrost_error"). Additionally, the example is incomplete—it omits"context","has_error", and"hook_error"fields that are part of the actual output.Update the PostLLMHook Output example to match the actual struct:
{ "context": { ... }, "response": { ... }, "error": null, "has_error": false, "hook_error": "" }Also clarify that the exported functions are
pre_hookandpost_hook; these are the WASM export names that implement the PreLLMHook/PostLLMHook stages.plugins/mocker/main.go (1)
861-864: Potential nil pointer dereference when overriding model.When
content.Modelis set butmockResponse.ChatResponseis nil (which happens forResponsesRequest), this will panic.🐛 Proposed fix
// Override model if specified - if content.Model != nil { + if content.Model != nil && mockResponse.ChatResponse != nil { mockResponse.ChatResponse.Model = *content.Model }
🤖 Fix all issues with AI agents
In @examples/plugins/hello-world-wasm-go/README.md:
- Line 170: The phrase "Full feature parity" is misleading; update the README
line that currently reads "Full feature parity: HTTP transport intercept,
PreLLMHook, and PostLLMHook all supported" to a clearer wording such as "Feature
parity for HTTP intercept + LLM hooks: HTTP transport intercept, PreLLMHook, and
PostLLMHook supported" (or similar) so it limits the claim to these specific
features and avoids implying complete parity with native plugins.
In @framework/plugins/soplugin.go:
- Around line 39-47: The comments for DynamicLLMPlugin.PreLLMHook and
DynamicLLMPlugin.PostLLMHook are misleading (they say "is not used for dynamic
plugins") but the methods delegate to dp.preHook and dp.postHook; update those
comments to accurately describe that these hooks are implemented for dynamic
plugins and delegate to the internal preHook/postHook handlers (mentioning
DynamicLLMPlugin, PreLLMHook, PostLLMHook, preHook, postHook) so the comment
reflects actual behavior.
🧹 Nitpick comments (8)
transports/bifrost-http/lib/config.go (2)
313-313: Optional refactor: Remove unnecessary zero-value initialization.The explicit initialization of
LLMPluginsto its zero value is redundant sinceatomic.Pointerfields automatically receive their zero values. The other atomic.Pointer fields (BasePlugins,MCPPlugins,HTTPTransportPlugins) are not explicitly initialized, creating a minor style inconsistency.♻️ Optional cleanup
config := &Config{ configPath: configFilePath, EnvKeys: make(map[string][]configstore.EnvKeyInfo), Providers: make(map[schemas.ModelProvider]configstore.ProviderConfig), - LLMPlugins: atomic.Pointer[[]schemas.LLMPlugin]{}, }
2301-2333: Consider documenting whypluginTypeparameter is ignored.Both
AddLoadedPluginandRemoveLoadedPluginaccept aschemas.PluginTypeparameter that is explicitly ignored (using_discard). The plugin type is correctly determined by interface implementation (LLMPlugin, MCPPlugin, etc.) rather than the parameter value. However, this design choice could confuse callers who might expect the type to be validated or used.Consider adding a doc comment explaining this behavior, for example:
// AddLoadedPlugin adds a plugin to the loaded plugins list. // The pluginType parameter is ignored; plugin type is determined by which // interfaces the plugin implements (LLMPlugin, MCPPlugin, HTTPTransportPlugin). // A single plugin may appear in multiple typed views if it implements multiple interfaces. func (c *Config) AddLoadedPlugin(plugin schemas.BasePlugin, _ schemas.PluginType) error {Also applies to: 2335-2365
core/mcp/agent.go (1)
293-314: Consider logging the fallback case.The error handling covers multiple scenarios well. However, the fallback case at lines 310-312 (when both
mcpResponseandtoolErrare nil) silently sends an empty result. This could mask upstream issues where the tool execution returned neither a response nor an error.💡 Suggested enhancement
} else if mcpResponse != nil && mcpResponse.ChatMessage == nil { // Send empty result when mcpResponse is non-nil but ChatMessage is nil channelToolResults <- createToolResultMessage(toolCall, "", nil) } else { // Fallback: send empty result when both mcpResponse and toolErr are nil + logger.Warn(fmt.Sprintf("Tool execution returned neither response nor error for tool: %s", func() string { + if toolCall.Function.Name != nil { + return *toolCall.Function.Name + } + return "unknown" + }())) channelToolResults <- createToolResultMessage(toolCall, "", nil) }transports/bifrost-http/handlers/mcpinference.go (1)
1-13: Consider using sonic for JSON unmarshaling for consistency.Other files in this codebase use
github.com/bytedance/sonicfor JSON operations (e.g.,core/mcp/agent.go). Usingencoding/jsonhere creates an inconsistency. While both work correctly, using sonic would align with the codebase conventions and provide better performance.💡 Suggested change
import ( - "encoding/json" "fmt" "strings" "github.com/fasthttp/router" + "github.com/bytedance/sonic" bifrost "github.com/maximhq/bifrost/core"Then update the unmarshal calls:
- if err := json.Unmarshal(ctx.PostBody(), &req); err != nil { + if err := sonic.Unmarshal(ctx.PostBody(), &req); err != nil {core/mcp/codemodeexecutecode.go (2)
857-863: Potential nil dereference when accessing error message.When
shortCircuit.Erroris non-nil butshortCircuit.Error.Erroris nil, the code returns a generic error. However, line 860 accessesshortCircuit.Error.Error.Messagewithout checking ifError.Messageitself could be problematic (empty string).Consider adding a more descriptive error message
if shortCircuit.Error != nil { pipeline.RunMCPPostHooks(nestedCtx, nil, shortCircuit.Error, preCount) if shortCircuit.Error.Error != nil { - return nil, fmt.Errorf("%s", shortCircuit.Error.Error.Message) + msg := shortCircuit.Error.Error.Message + if msg == "" { + msg = "plugin short-circuit error (no message)" + } + return nil, fmt.Errorf("%s", msg) } return nil, fmt.Errorf("plugin short-circuit error") }
965-967: Guard against unexpected nil response from post-hooks.When
finalErris nil butfinalResporfinalResp.ChatMessageis nil, the error message could be more informative about which post-hook caused the issue.core/bifrost.go (2)
290-310: Consider using CompareAndSwap for ReloadConfig plugin updates.The current implementation uses
Store()directly after creating plugin copies. IfReloadConfigis called concurrently by multiple goroutines, there's a potential race condition where updates could be lost (last write wins). TheremoveLLMPlugin/removeMCPPluginmethods use a CAS retry loop to handle this safely.Consider adopting the same CAS pattern here for consistency and thread safety:
♻️ Proposed fix using CAS pattern
func (bifrost *Bifrost) ReloadConfig(config schemas.BifrostConfig) error { bifrost.dropExcessRequests.Store(config.DropExcessRequests) // Update LLM plugins atomically if config.LLMPlugins != nil { - llmPluginsCopy := make([]schemas.LLMPlugin, len(config.LLMPlugins)) - copy(llmPluginsCopy, config.LLMPlugins) - bifrost.llmPlugins.Store(&llmPluginsCopy) + for { + llmPluginsCopy := make([]schemas.LLMPlugin, len(config.LLMPlugins)) + copy(llmPluginsCopy, config.LLMPlugins) + if bifrost.llmPlugins.CompareAndSwap(bifrost.llmPlugins.Load(), &llmPluginsCopy) { + break + } + } } // Update MCP plugins atomically if config.MCPPlugins != nil { - mcpPluginsCopy := make([]schemas.MCPPlugin, len(config.MCPPlugins)) - copy(mcpPluginsCopy, config.MCPPlugins) - bifrost.mcpPlugins.Store(&mcpPluginsCopy) + for { + mcpPluginsCopy := make([]schemas.MCPPlugin, len(config.MCPPlugins)) + copy(mcpPluginsCopy, config.MCPPlugins) + if bifrost.mcpPlugins.CompareAndSwap(bifrost.mcpPlugins.Load(), &mcpPluginsCopy) { + break + } + } } return nil }Alternatively, if
ReloadConfigis guaranteed to be called from a single control path, add a comment documenting this assumption.
2859-2859: Consider defensive nil check for atomic pointer dereference.The code dereferences
bifrost.llmPlugins.Load()without checking for nil. While the initialization inInit()ensures this is typically non-nil, defensive programming would add a nil check to prevent potential panics during edge cases (e.g., shutdown races, initialization failures).This pattern appears in multiple locations (lines 2859, 3056, 3311, 3969).
💡 Suggested defensive pattern
-pluginCount := len(*bifrost.llmPlugins.Load()) +llmPlugins := bifrost.llmPlugins.Load() +pluginCount := 0 +if llmPlugins != nil { + pluginCount = len(*llmPlugins) +}Apply similar checks at lines 3056, 3311, and 3969 (in
getPluginPipeline).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/media/ui-disable-auth-on-inference.pngis excluded by!**/*.pngdocs/media/ui-enforce-virtual-keys.pngis excluded by!**/*.png
📒 Files selected for processing (61)
core/bifrost.gocore/mcp/agent.gocore/mcp/codemodeexecutecode.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/schemas/bifrost.gocore/schemas/mcp.gocore/schemas/plugin.gocore/schemas/plugin_native.gocore/schemas/trace.gocore/utils.godocs/architecture/core/plugins.mdxdocs/architecture/framework/streaming.mdxdocs/changelogs/v1.3.12.mdxdocs/features/governance/virtual-keys.mdxdocs/features/mcp/tool-execution.mdxdocs/features/observability/default.mdxdocs/features/observability/maxim.mdxdocs/features/observability/otel.mdxdocs/features/plugins/jsonparser.mdxdocs/features/plugins/mocker.mdxdocs/features/semantic-caching.mdxdocs/plugins/getting-started.mdxdocs/plugins/migration-guide.mdxdocs/plugins/writing-go-plugin.mdxdocs/quickstart/go-sdk/context-keys.mdxexamples/plugins/hello-world-wasm-go/README.mdexamples/plugins/hello-world/main.goframework/plugins/loader.goframework/plugins/main.goframework/plugins/soloader.goframework/plugins/soplugin.goframework/plugins/soplugin_test.goplugins/governance/main.goplugins/governance/utils.goplugins/jsonparser/main.goplugins/jsonparser/plugin_test.goplugins/logging/main.goplugins/maxim/main.goplugins/maxim/plugin_test.goplugins/mocker/benchmark_test.goplugins/mocker/main.goplugins/mocker/plugin_test.goplugins/otel/main.goplugins/semanticcache/main.goplugins/semanticcache/plugin_core_test.goplugins/semanticcache/plugin_integration_test.goplugins/semanticcache/search.goplugins/semanticcache/test_utils.goplugins/telemetry/main.gotransports/bifrost-http/handlers/cache.gotransports/bifrost-http/handlers/devpprof.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/lib.gotransports/bifrost-http/server/server.gotransports/changelog.mdui/app/workspace/config/views/securityView.tsx
💤 Files with no reviewable changes (1)
- transports/bifrost-http/handlers/mcp.go
✅ Files skipped from review due to trivial changes (1)
- transports/changelog.md
🚧 Files skipped from review as they are similar to previous changes (12)
- core/utils.go
- framework/plugins/loader.go
- core/schemas/trace.go
- core/schemas/plugin_native.go
- plugins/semanticcache/test_utils.go
- plugins/jsonparser/plugin_test.go
- docs/quickstart/go-sdk/context-keys.mdx
- docs/features/observability/otel.mdx
- plugins/mocker/benchmark_test.go
- docs/features/mcp/tool-execution.mdx
- docs/features/plugins/mocker.mdx
- transports/bifrost-http/handlers/cache.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
docs/architecture/core/plugins.mdxplugins/semanticcache/plugin_core_test.godocs/plugins/getting-started.mdxplugins/logging/main.goui/app/workspace/config/views/securityView.tsxtransports/bifrost-http/lib/lib.godocs/features/observability/default.mdxcore/mcp/agent.goplugins/mocker/plugin_test.goexamples/plugins/hello-world-wasm-go/README.mddocs/architecture/framework/streaming.mdxcore/schemas/mcp.goplugins/jsonparser/main.gotransports/bifrost-http/handlers/middlewares.goplugins/semanticcache/plugin_integration_test.goplugins/otel/main.godocs/features/governance/virtual-keys.mdxplugins/telemetry/main.gocore/mcp/mcp.goplugins/mocker/main.godocs/plugins/writing-go-plugin.mdxexamples/plugins/hello-world/main.gocore/mcp/codemodeexecutecode.goplugins/governance/main.goplugins/maxim/main.goframework/plugins/soloader.gocore/schemas/bifrost.godocs/changelogs/v1.3.12.mdxdocs/features/semantic-caching.mdxcore/schemas/plugin.goplugins/maxim/plugin_test.goplugins/governance/utils.godocs/features/plugins/jsonparser.mdxtransports/bifrost-http/handlers/devpprof.gocore/mcp/toolmanager.goplugins/semanticcache/search.gotransports/bifrost-http/lib/config.goframework/plugins/soplugin.godocs/features/observability/maxim.mdxtransports/bifrost-http/handlers/mcpserver.goframework/plugins/soplugin_test.gotransports/bifrost-http/server/server.godocs/plugins/migration-guide.mdxcore/bifrost.goframework/plugins/main.gotransports/bifrost-http/handlers/mcpinference.goplugins/semanticcache/main.go
🧠 Learnings (7)
📚 Learning: 2025-12-30T05:37:48.365Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1180
File: docs/features/mcp/connecting-to-servers.mdx:452-458
Timestamp: 2025-12-30T05:37:48.365Z
Learning: When reviewing documentation PRs in a Graphite-managed stack, first check related or previous PRs in the stack for feature implementations before flagging documentation as incorrect or unsupported. Documentation MDX files often reference features implemented in earlier stack PRs; verify that the documented behavior exists in earlier changes and that the docs accurately reflect the implemented state before requesting edits.
Applied to files:
docs/architecture/core/plugins.mdxdocs/plugins/getting-started.mdxdocs/features/observability/default.mdxdocs/architecture/framework/streaming.mdxdocs/features/governance/virtual-keys.mdxdocs/plugins/writing-go-plugin.mdxdocs/changelogs/v1.3.12.mdxdocs/features/semantic-caching.mdxdocs/features/plugins/jsonparser.mdxdocs/features/observability/maxim.mdxdocs/plugins/migration-guide.mdx
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
plugins/semanticcache/plugin_core_test.goplugins/logging/main.gotransports/bifrost-http/lib/lib.gocore/mcp/agent.goplugins/mocker/plugin_test.gocore/schemas/mcp.goplugins/jsonparser/main.gotransports/bifrost-http/handlers/middlewares.goplugins/semanticcache/plugin_integration_test.goplugins/otel/main.goplugins/telemetry/main.gocore/mcp/mcp.goplugins/mocker/main.goexamples/plugins/hello-world/main.gocore/mcp/codemodeexecutecode.goplugins/governance/main.goplugins/maxim/main.goframework/plugins/soloader.gocore/schemas/bifrost.gocore/schemas/plugin.goplugins/maxim/plugin_test.goplugins/governance/utils.gotransports/bifrost-http/handlers/devpprof.gocore/mcp/toolmanager.goplugins/semanticcache/search.gotransports/bifrost-http/lib/config.goframework/plugins/soplugin.gotransports/bifrost-http/handlers/mcpserver.goframework/plugins/soplugin_test.gotransports/bifrost-http/server/server.gocore/bifrost.goframework/plugins/main.gotransports/bifrost-http/handlers/mcpinference.goplugins/semanticcache/main.go
📚 Learning: 2025-12-29T11:54:55.836Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1153
File: framework/configstore/rdb.go:2221-2246
Timestamp: 2025-12-29T11:54:55.836Z
Learning: In Go reviews, do not flag range-over-int patterns like for i := range n as compile-time errors, assuming Go 1.22+ semantics. Only flag actual range-capable values (slices, arrays, maps, channels, strings) and other compile-time issues. This applies to all Go files across the repository.
Applied to files:
plugins/semanticcache/plugin_core_test.goplugins/logging/main.gotransports/bifrost-http/lib/lib.gocore/mcp/agent.goplugins/mocker/plugin_test.gocore/schemas/mcp.goplugins/jsonparser/main.gotransports/bifrost-http/handlers/middlewares.goplugins/semanticcache/plugin_integration_test.goplugins/otel/main.goplugins/telemetry/main.gocore/mcp/mcp.goplugins/mocker/main.goexamples/plugins/hello-world/main.gocore/mcp/codemodeexecutecode.goplugins/governance/main.goplugins/maxim/main.goframework/plugins/soloader.gocore/schemas/bifrost.gocore/schemas/plugin.goplugins/maxim/plugin_test.goplugins/governance/utils.gotransports/bifrost-http/handlers/devpprof.gocore/mcp/toolmanager.goplugins/semanticcache/search.gotransports/bifrost-http/lib/config.goframework/plugins/soplugin.gotransports/bifrost-http/handlers/mcpserver.goframework/plugins/soplugin_test.gotransports/bifrost-http/server/server.gocore/bifrost.goframework/plugins/main.gotransports/bifrost-http/handlers/mcpinference.goplugins/semanticcache/main.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/lib/lib.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/devpprof.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/mcpinference.go
📚 Learning: 2025-12-29T09:14:16.633Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 888
File: transports/bifrost-http/handlers/middlewares.go:246-256
Timestamp: 2025-12-29T09:14:16.633Z
Learning: In the bifrost HTTP transport, fasthttp.RequestCtx is the primary context carrier and should be passed directly to functions that expect a context.Context. Do not convert to context.Context unless explicitly required. Ensure tracer implementations and related components are designed to accept fasthttp.RequestCtx directly, and document this architectural decision for maintainers.
Applied to files:
transports/bifrost-http/lib/lib.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/devpprof.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/mcpinference.go
📚 Learning: 2026-01-10T08:07:42.582Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1299
File: core/mcp/codemodeexecutecode.go:882-907
Timestamp: 2026-01-10T08:07:42.582Z
Learning: In the MCP tool execution flow (core/mcp/codemodeexecutecode.go callMCPTool function), pre-hook modifications to tool names should NOT be applied. The originalToolName computed before pre-hooks must be used in the final MCP CallToolRequest. Only argument modifications from pre-hooks should be honored. This is a security/consistency decision to prevent plugins from redirecting tool execution.
Applied to files:
core/mcp/codemodeexecutecode.go
📚 Learning: 2025-12-22T10:50:40.990Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1154
File: plugins/governance/store.go:1165-1186
Timestamp: 2025-12-22T10:50:40.990Z
Learning: In the Bifrost governance plugin, budgets and rate limits have 1:1 relationships with their parent entities (virtual keys, teams, customers). Do not assume sharing; ensure cascade deletion logic only deletes budgets/rate limits when there are no shared references. Enforce invariants in code and add tests to verify no cross-entity sharing and that cascade deletes only remove the specific child of the parent. If a counterexample arises, adjust data model or add guards.
Applied to files:
plugins/governance/main.goplugins/governance/utils.go
🧬 Code graph analysis (27)
plugins/logging/main.go (5)
examples/plugins/hello-world/main.go (2)
PreLLMHook(28-34)PostLLMHook(36-43)core/schemas/bifrost.go (3)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(527-536)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)ui/lib/types/logs.ts (1)
BifrostError(234-240)
transports/bifrost-http/lib/lib.go (1)
core/schemas/plugin.go (1)
LLMPlugin(135-140)
core/mcp/agent.go (2)
core/schemas/bifrost.go (4)
BifrostMCPRequest(358-363)BifrostMCPResponse(442-446)RequestType(88-88)MCPRequestTypeChatToolCall(350-350)core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(756-761)
plugins/mocker/plugin_test.go (2)
core/schemas/account.go (1)
Account(81-97)core/schemas/plugin.go (1)
LLMPlugin(135-140)
plugins/jsonparser/main.go (2)
core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
transports/bifrost-http/handlers/middlewares.go (1)
core/schemas/plugin.go (2)
BasePlugin(119-127)ObservabilityPlugin(173-187)
plugins/semanticcache/plugin_integration_test.go (1)
examples/plugins/hello-world/main.go (2)
PreLLMHook(28-34)PostLLMHook(36-43)
plugins/otel/main.go (2)
core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
plugins/telemetry/main.go (4)
core/schemas/bifrost.go (2)
BifrostResponse(368-389)BifrostError(527-536)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)core/utils.go (1)
GetResponseFields(243-250)
core/mcp/mcp.go (5)
core/bifrost.go (1)
PluginPipeline(81-97)core/mcp/toolmanager.go (2)
PluginPipeline(45-48)NewToolsManager(69-106)transports/bifrost-http/handlers/mcp.go (1)
MCPManager(20-24)core/mcp.go (1)
MCPManager(49-56)core/schemas/bifrost.go (2)
BifrostMCPRequest(358-363)BifrostMCPResponse(442-446)
plugins/mocker/main.go (4)
examples/plugins/hello-world/main.go (2)
PreLLMHook(28-34)PostLLMHook(36-43)core/schemas/bifrost.go (3)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(527-536)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
examples/plugins/hello-world/main.go (4)
core/schemas/bifrost.go (4)
BifrostContextKey(118-118)BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(527-536)core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
plugins/governance/main.go (4)
core/schemas/bifrost.go (4)
BifrostResponse(368-389)BifrostError(527-536)BifrostContextKeyVirtualKey(122-122)BifrostContextKeyRequestID(124-124)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)core/utils.go (1)
GetStringFromContext(306-313)
plugins/maxim/main.go (2)
core/schemas/plugin.go (1)
LLMPlugin(135-140)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
framework/plugins/soloader.go (4)
core/schemas/plugin.go (1)
LLMPlugin(135-140)framework/plugins/soplugin.go (1)
DynamicLLMPlugin(10-24)core/schemas/bifrost.go (3)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(527-536)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
core/schemas/bifrost.go (3)
core/schemas/plugin.go (2)
LLMPlugin(135-140)MCPPlugin(142-147)core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(756-761)core/schemas/responses.go (2)
ResponsesToolMessage(471-491)ResponsesMessage(319-332)
core/schemas/plugin.go (2)
core/schemas/plugin_native.go (2)
LLMPluginShortCircuit(16-20)MCPPluginShortCircuit(24-27)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
plugins/maxim/plugin_test.go (2)
core/schemas/plugin.go (1)
LLMPlugin(135-140)core/schemas/account.go (1)
Account(81-97)
plugins/governance/utils.go (3)
core/schemas/plugin.go (1)
HTTPRequest(30-36)core/utils.go (1)
Ptr(57-59)plugins/governance/main.go (1)
VirtualKeyPrefix(29-29)
core/mcp/toolmanager.go (2)
core/schemas/bifrost.go (7)
BifrostMCPRequest(358-363)BifrostMCPResponse(442-446)BifrostError(527-536)RequestType(88-88)MCPRequestTypeChatToolCall(350-350)MCPRequestTypeResponsesToolCall(351-351)BifrostMCPResponseExtraFields(462-467)core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(756-761)
plugins/semanticcache/search.go (3)
core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)framework/vectorstore/store.go (1)
SearchResult(46-50)
transports/bifrost-http/lib/config.go (2)
core/schemas/plugin.go (4)
BasePlugin(119-127)LLMPlugin(135-140)MCPPlugin(142-147)HTTPTransportPlugin(129-133)framework/plugins/loader.go (1)
PluginLoader(6-8)
framework/plugins/soplugin.go (5)
core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/plugin.go (2)
HTTPRequest(30-36)HTTPResponse(40-44)core/schemas/bifrost.go (2)
BifrostResponse(368-389)BifrostError(527-536)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
transports/bifrost-http/handlers/mcpserver.go (2)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(756-761)core/schemas/bifrost.go (1)
BifrostError(527-536)
framework/plugins/soplugin_test.go (4)
framework/plugins/main.go (3)
DynamicLLMPluginConfig(8-13)LoadLLMPlugins(39-55)Config(16-18)core/schemas/plugin.go (1)
HTTPTransportPlugin(129-133)examples/plugins/hello-world/main.go (3)
HTTPTransportIntercept(18-26)PreLLMHook(28-34)PostLLMHook(36-43)framework/plugins/soplugin.go (1)
DynamicLLMPlugin(10-24)
core/bifrost.go (6)
core/schemas/plugin.go (6)
LLMPlugin(135-140)MCPPlugin(142-147)PluginType(81-81)PluginTypeLLM(84-84)PluginTypeMCP(85-85)BasePlugin(119-127)core/schemas/provider.go (1)
Provider(317-366)core/schemas/bifrost.go (8)
KeySelector(13-13)BifrostMCPRequest(358-363)BifrostConfig(18-28)BifrostError(527-536)RequestType(88-88)ResponsesRequest(96-96)BifrostMCPResponse(442-446)BifrostRequest(177-198)core/schemas/tracer.go (1)
Tracer(37-115)core/mcp/toolmanager.go (1)
PluginPipeline(45-48)core/schemas/context.go (1)
BifrostContext(32-43)
plugins/semanticcache/main.go (4)
examples/plugins/hello-world/main.go (3)
Init(9-12)PreLLMHook(28-34)PostLLMHook(36-43)core/schemas/plugin.go (1)
LLMPlugin(135-140)core/schemas/bifrost.go (3)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(527-536)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)
Pratham-Mishra04
force-pushed
the
01-07-feat_plugin_schemas_segregation_for_mcp_plugins
branch
from
97d1b5e to
cc747af
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)
plugins/mocker/main.go (2)
861-864: Potential nil pointer dereference when overriding model for ResponsesRequest.When
req.RequestType == schemas.ResponsesRequest,mockResponse.ChatResponseis nil (onlyResponsesResponseis set at line 836). AccessingmockResponse.ChatResponse.Modelwill panic.🐛 Proposed fix
// Override model if specified if content.Model != nil { - mockResponse.ChatResponse.Model = *content.Model + if mockResponse.ChatResponse != nil { + mockResponse.ChatResponse.Model = *content.Model + } + if mockResponse.ResponsesResponse != nil { + mockResponse.ResponsesResponse.Model = content.Model + } }
993-1041: handleDefaultBehavior only produces ChatResponse for success case.When
DefaultBehaviorSuccessis configured, the default response is always aChatResponse. However,PreLLMHookhandles bothChatCompletionRequestandResponsesRequesttypes (line 499). AResponsesRequestreceiving aChatResponsemay cause issues downstream.Consider generating the appropriate response type based on
req.RequestType, similar togenerateSuccessShortCircuit.
🤖 Fix all issues with AI agents
In @plugins/governance/utils.go:
- Line 24: The code reads the Authorization header using the wrong case; update
the lookup authHeader := req.Headers["Authorization"] to use the lowercase key
per schemas.HTTPRequest (Headers map[string]string uses lowercase keys) so it
becomes authHeader := req.Headers["authorization"]; keep using the same
authHeader variable and req.Headers map.
🧹 Nitpick comments (18)
transports/bifrost-http/lib/config.go (1)
309-314: Initialization only sets upLLMPluginsatomic pointer.The
LoadConfigfunction initializesLLMPluginsbut notBasePlugins,MCPPlugins, orHTTPTransportPlugins. Whileatomic.Pointerhas a zero value ofnil(which is handled correctly by the getter methods), consider initializing all plugin pointers for consistency and clarity.🔧 Optional: Initialize all plugin atomic pointers
config := &Config{ configPath: configFilePath, EnvKeys: make(map[string][]configstore.EnvKeyInfo), Providers: make(map[schemas.ModelProvider]configstore.ProviderConfig), - LLMPlugins: atomic.Pointer[[]schemas.LLMPlugin]{}, + BasePlugins: atomic.Pointer[[]schemas.BasePlugin]{}, + LLMPlugins: atomic.Pointer[[]schemas.LLMPlugin]{}, + MCPPlugins: atomic.Pointer[[]schemas.MCPPlugin]{}, + HTTPTransportPlugins: atomic.Pointer[[]schemas.HTTPTransportPlugin]{}, }core/schemas/mcp.go (1)
25-33: Consider defining a typed interface for plugin pipeline.The
interface{}type forPluginPipelineProviderandReleasePluginPipelineworks but loses type safety. Callers must cast to the concrete pipeline type, though all current usage sites properly validate the cast.If import cycles permit, consider defining a minimal interface in the schemas package that the pipeline can implement:
// PluginPipeline represents a pipeline for running plugin hooks type PluginPipeline interface { // Define minimal methods needed by MCP tool execution }This is a low-priority improvement since the current approach avoids import cycles, the
json:"-"tags correctly prevent serialization, and all call sites safely handle the type assertions and nil checks.transports/bifrost-http/handlers/mcpinference.go (1)
15-18: Unused fieldstorein MCPInferenceHandler.The
storefield is stored in the struct but not used in any of the handler methods. This may be intentional for future use, but consider removing it if unnecessary to avoid confusion.core/mcp/codemodeexecutecode.go (1)
965-970: Consider handling nil ChatMessage more gracefully.When
finalRespis non-nil butfinalResp.ChatMessageis nil, returning a generic error loses information. TheResponsesMessagefield might be populated instead (forMCPRequestTypeResponsesToolCall).♻️ Suggested improvement
if finalResp == nil || finalResp.ChatMessage == nil { - return nil, fmt.Errorf("plugin post-hooks returned invalid response") + // Check if ResponsesMessage is populated instead + if finalResp != nil && finalResp.ResponsesMessage != nil { + // Handle ResponsesMessage case if needed in the future + return nil, fmt.Errorf("ResponsesMessage response type not yet supported in code mode") + } + return nil, fmt.Errorf("plugin post-hooks returned nil response") }framework/plugins/soplugin_test.go (3)
141-142: Minor: Subtest naming inconsistency.The comment on line 141 says "Test PostLLMHook with error" but the subtest on line 142 is named
"PostHook_WithError". Consider updating the subtest name to"PostLLMHook_WithError"for consistency with the new naming convention.Suggested fix
// Test PostLLMHook with error - t.Run("PostHook_WithError", func(t *testing.T) { + t.Run("PostLLMHook_WithError", func(t *testing.T) {
454-455: Minor: Benchmark function naming inconsistency.The function name
BenchmarkDynamicPlugin_PreHookdoesn't match the newPreLLMHooknaming convention. Consider renaming toBenchmarkDynamicPlugin_PreLLMHookfor consistency.Suggested fix
-// BenchmarkDynamicPlugin_PreHook benchmarks the PreLLMHook method -func BenchmarkDynamicPlugin_PreHook(b *testing.B) { +// BenchmarkDynamicPlugin_PreLLMHook benchmarks the PreLLMHook method +func BenchmarkDynamicPlugin_PreLLMHook(b *testing.B) {
480-481: Minor: Benchmark function naming inconsistency.Similar to above,
BenchmarkDynamicPlugin_PostHookshould beBenchmarkDynamicPlugin_PostLLMHookfor consistency.Suggested fix
-// BenchmarkDynamicPlugin_PostHook benchmarks the PostLLMHook method -func BenchmarkDynamicPlugin_PostHook(b *testing.B) { +// BenchmarkDynamicPlugin_PostLLMHook benchmarks the PostLLMHook method +func BenchmarkDynamicPlugin_PostLLMHook(b *testing.B) {framework/plugins/soplugin.go (4)
9-10: Stale comment - references old type name.The comment on line 9 still says "DynamicPlugin is the interface" but the type is now
DynamicLLMPlugin. Update the comment to reflect the new name.Suggested fix
-// DynamicPlugin is the interface for a dynamic plugin +// DynamicLLMPlugin is the struct for a dynamic LLM plugin loaded from a shared object type DynamicLLMPlugin struct {
39-42: Misleading comment - PreLLMHook IS used for dynamic plugins.The comment says "PreLLMHook is not used for dynamic plugins" but the method clearly delegates to
dp.preHook(ctx, req). This comment appears to be a copy-paste artifact. Update it to accurately describe the method's purpose.Suggested fix
-// PreLLMHook is not used for dynamic plugins +// PreLLMHook executes the pre-LLM hook for this dynamic plugin func (dp *DynamicLLMPlugin) PreLLMHook(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) (*schemas.BifrostRequest, *schemas.LLMPluginShortCircuit, error) {
44-47: Misleading comment - PostLLMHook IS used for dynamic plugins.Same issue as
PreLLMHook. The comment is inaccurate.Suggested fix
-// PostLLMHook is not used for dynamic plugins +// PostLLMHook executes the post-LLM hook for this dynamic plugin func (dp *DynamicLLMPlugin) PostLLMHook(ctx *schemas.BifrostContext, resp *schemas.BifrostResponse, bifrostErr *schemas.BifrostError) (*schemas.BifrostResponse, *schemas.BifrostError, error) {
49-51: Misleading comment - Cleanup IS used for dynamic plugins.The comment says "Cleanup is not used for dynamic plugins" but the method delegates to
dp.cleanup().Suggested fix
-// Cleanup is not used for dynamic plugins +// Cleanup performs cleanup operations for this dynamic plugin func (dp *DynamicLLMPlugin) Cleanup() error {transports/bifrost-http/server/server.go (5)
448-457: Redundant reflection check after failed type assertion.The reflection check at lines 453-456 is redundant. If the type assertion
plugin.(T)at line 449 fails, the subsequentplugin.(T)at line 456 will also fail (or panic if T is a concrete type). TheImplementscheck verifies the same condition that the type assertion already tests.Consider removing the redundant reflection block:
♻️ Suggested simplification
if plugin.GetName() == name { - // Try type assertion if p, ok := plugin.(T); ok { return p, nil } - // If T is an interface type and plugin implements it, return directly - zeroType := reflect.TypeOf((*T)(nil)).Elem() - pluginType := reflect.TypeOf(plugin) - if zeroType.Kind() == reflect.Interface && pluginType.Implements(zeroType) { - return plugin.(T), nil - } return zero, fmt.Errorf("plugin %q found but type mismatch", name) }
470-479: Same redundant reflection pattern asFindPluginByName.This function has the same redundant reflection check as
FindPluginByName. The same simplification applies here.
888-900: Partial failure handling could leave inconsistent state.If
AddLoadedPluginsucceeds for LLM (line 890) but fails for MCP (line 896), the plugin remains in the LLM list but the function returns an error. This could leave the system in an inconsistent state where the plugin is partially registered.Consider rolling back the LLM addition if MCP registration fails, or documenting this behavior as intentional for plugins that implement only one interface.
1451-1463: Consider reusingreloadObservabilityPluginslogic.The observability plugin aggregation logic here duplicates the logic in
reloadObservabilityPlugins(lines 954-965). Consider extracting a helper function to collect observability plugins and reusing it in both places to reduce duplication.
226-242: Dynamic plugin loading only supports LLM plugins.The
LoadPluginfunction usesdynamicPlugins.LoadLLMPluginsfor dynamic plugins (line 226), which means dynamic plugins that only implementMCPPlugin(withoutLLMPlugin) cannot be loaded through this path. The codebase provides noLoadDynamicMCPPluginor equivalent MCP-only loading mechanism.If MCP-only dynamic plugins are a valid use case, consider extending the plugin loader to support
LoadDynamicMCPPlugin.core/bifrost.go (2)
2859-2859: Plugin count fetched outside pipeline context may be stale.At lines 2859, 3056, and 3311, the code fetches
len(*bifrost.llmPlugins.Load())to pass toRunPostHooks. However, the plugin list could have changed between when the pipeline was created (which captured the plugin list snapshot) and when this count is fetched.Consider using the pipeline's captured
llmPluginsslice length instead for consistency:// Instead of: pluginCount := len(*bifrost.llmPlugins.Load()) // Use pipeline's captured count or pass the pipeline itselfThis is a minor concern since the count is only used for bounds checking and the current implementation is defensive about out-of-bounds access.
Also applies to: 3056-3056, 3311-3311
3969-3970: Potential nil pointer dereference if plugin pointers are nil.At lines 3969-3970,
bifrost.llmPlugins.Load()andbifrost.mcpPlugins.Load()are dereferenced directly. If either returnsnil(which shouldn't happen after proper initialization), this would panic.Consider adding defensive nil checks:
♻️ Suggested defensive check
func (bifrost *Bifrost) getPluginPipeline() *PluginPipeline { pipeline := bifrost.pluginPipelinePool.Get().(*PluginPipeline) - pipeline.llmPlugins = *bifrost.llmPlugins.Load() - pipeline.mcpPlugins = *bifrost.mcpPlugins.Load() + if llmPtr := bifrost.llmPlugins.Load(); llmPtr != nil { + pipeline.llmPlugins = *llmPtr + } + if mcpPtr := bifrost.mcpPlugins.Load(); mcpPtr != nil { + pipeline.mcpPlugins = *mcpPtr + } pipeline.logger = bifrost.logger pipeline.tracer = bifrost.getTracer() return pipeline }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/media/ui-disable-auth-on-inference.pngis excluded by!**/*.pngdocs/media/ui-enforce-virtual-keys.pngis excluded by!**/*.png
📒 Files selected for processing (69)
core/bifrost.gocore/chatbot_test.gocore/internal/testutil/setup.gocore/mcp/agent.gocore/mcp/codemodeexecutecode.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/schemas/bifrost.gocore/schemas/mcp.gocore/schemas/plugin.gocore/schemas/plugin_native.gocore/schemas/plugin_wasm.gocore/schemas/trace.gocore/utils.godocs/architecture/core/plugins.mdxdocs/architecture/framework/streaming.mdxdocs/changelogs/v1.3.12.mdxdocs/features/governance/virtual-keys.mdxdocs/features/mcp/tool-execution.mdxdocs/features/observability/default.mdxdocs/features/observability/maxim.mdxdocs/features/observability/otel.mdxdocs/features/plugins/jsonparser.mdxdocs/features/plugins/mocker.mdxdocs/features/semantic-caching.mdxdocs/plugins/getting-started.mdxdocs/plugins/migration-guide.mdxdocs/plugins/writing-go-plugin.mdxdocs/quickstart/go-sdk/context-keys.mdxexamples/plugins/hello-world-wasm-go/README.mdexamples/plugins/hello-world-wasm-go/types.goexamples/plugins/hello-world-wasm-rust/README.mdexamples/plugins/hello-world-wasm-rust/src/lib.rsexamples/plugins/hello-world-wasm-rust/src/types.rsexamples/plugins/hello-world-wasm-typescript/README.mdexamples/plugins/hello-world/main.goframework/plugins/loader.goframework/plugins/main.goframework/plugins/soloader.goframework/plugins/soplugin.goframework/plugins/soplugin_test.goplugins/governance/main.goplugins/governance/utils.goplugins/jsonparser/main.goplugins/jsonparser/plugin_test.goplugins/logging/main.goplugins/maxim/main.goplugins/maxim/plugin_test.goplugins/mocker/benchmark_test.goplugins/mocker/main.goplugins/mocker/plugin_test.goplugins/otel/main.goplugins/semanticcache/main.goplugins/semanticcache/plugin_core_test.goplugins/semanticcache/plugin_integration_test.goplugins/semanticcache/search.goplugins/semanticcache/test_utils.goplugins/telemetry/main.gotransports/bifrost-http/handlers/cache.gotransports/bifrost-http/handlers/devpprof.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/lib.gotransports/bifrost-http/server/server.gotransports/changelog.mdui/app/workspace/config/views/securityView.tsx
💤 Files with no reviewable changes (3)
- transports/bifrost-http/handlers/mcp.go
- core/internal/testutil/setup.go
- core/chatbot_test.go
🚧 Files skipped from review as they are similar to previous changes (19)
- transports/bifrost-http/handlers/devpprof.go
- transports/changelog.md
- examples/plugins/hello-world-wasm-rust/src/types.rs
- plugins/semanticcache/test_utils.go
- docs/plugins/migration-guide.mdx
- framework/plugins/loader.go
- plugins/semanticcache/plugin_core_test.go
- docs/changelogs/v1.3.12.mdx
- plugins/mocker/benchmark_test.go
- plugins/jsonparser/main.go
- core/utils.go
- docs/features/mcp/tool-execution.mdx
- docs/plugins/getting-started.mdx
- transports/bifrost-http/handlers/cache.go
- transports/bifrost-http/lib/lib.go
- docs/features/observability/otel.mdx
- core/schemas/trace.go
- transports/bifrost-http/handlers/mcpserver.go
- core/schemas/plugin_wasm.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
transports/bifrost-http/handlers/middlewares.goui/app/workspace/config/views/securityView.tsxplugins/jsonparser/plugin_test.goplugins/semanticcache/plugin_integration_test.godocs/features/observability/default.mdxplugins/governance/utils.godocs/quickstart/go-sdk/context-keys.mdxexamples/plugins/hello-world-wasm-rust/README.mdexamples/plugins/hello-world-wasm-go/types.godocs/features/observability/maxim.mdxdocs/features/semantic-caching.mdxcore/mcp/agent.goplugins/telemetry/main.goplugins/mocker/main.godocs/features/plugins/mocker.mdxcore/schemas/mcp.gocore/mcp/codemodeexecutecode.goframework/plugins/soplugin_test.godocs/architecture/framework/streaming.mdxtransports/bifrost-http/server/server.goplugins/maxim/main.goplugins/otel/main.goexamples/plugins/hello-world-wasm-rust/src/lib.rsplugins/logging/main.godocs/architecture/core/plugins.mdxexamples/plugins/hello-world-wasm-go/README.mdplugins/maxim/plugin_test.gocore/schemas/plugin.goplugins/governance/main.godocs/features/plugins/jsonparser.mdxdocs/features/governance/virtual-keys.mdxexamples/plugins/hello-world/main.goplugins/semanticcache/main.gotransports/bifrost-http/handlers/mcpinference.goframework/plugins/soloader.gocore/mcp/toolmanager.goexamples/plugins/hello-world-wasm-typescript/README.mdcore/schemas/plugin_native.gocore/mcp/mcp.goframework/plugins/main.gocore/bifrost.goplugins/semanticcache/search.goplugins/mocker/plugin_test.godocs/plugins/writing-go-plugin.mdxcore/schemas/bifrost.goframework/plugins/soplugin.gotransports/bifrost-http/lib/config.go
🧠 Learnings (8)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
transports/bifrost-http/handlers/middlewares.goplugins/jsonparser/plugin_test.goplugins/semanticcache/plugin_integration_test.goplugins/governance/utils.goexamples/plugins/hello-world-wasm-go/types.gocore/mcp/agent.goplugins/telemetry/main.goplugins/mocker/main.gocore/schemas/mcp.gocore/mcp/codemodeexecutecode.goframework/plugins/soplugin_test.gotransports/bifrost-http/server/server.goplugins/maxim/main.goplugins/otel/main.goplugins/logging/main.goplugins/maxim/plugin_test.gocore/schemas/plugin.goplugins/governance/main.goexamples/plugins/hello-world/main.goplugins/semanticcache/main.gotransports/bifrost-http/handlers/mcpinference.goframework/plugins/soloader.gocore/mcp/toolmanager.gocore/schemas/plugin_native.gocore/mcp/mcp.goframework/plugins/main.gocore/bifrost.goplugins/semanticcache/search.goplugins/mocker/plugin_test.gocore/schemas/bifrost.goframework/plugins/soplugin.gotransports/bifrost-http/lib/config.go
📚 Learning: 2025-12-29T11:54:55.836Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1153
File: framework/configstore/rdb.go:2221-2246
Timestamp: 2025-12-29T11:54:55.836Z
Learning: In Go reviews, do not flag range-over-int patterns like for i := range n as compile-time errors, assuming Go 1.22+ semantics. Only flag actual range-capable values (slices, arrays, maps, channels, strings) and other compile-time issues. This applies to all Go files across the repository.
Applied to files:
transports/bifrost-http/handlers/middlewares.goplugins/jsonparser/plugin_test.goplugins/semanticcache/plugin_integration_test.goplugins/governance/utils.goexamples/plugins/hello-world-wasm-go/types.gocore/mcp/agent.goplugins/telemetry/main.goplugins/mocker/main.gocore/schemas/mcp.gocore/mcp/codemodeexecutecode.goframework/plugins/soplugin_test.gotransports/bifrost-http/server/server.goplugins/maxim/main.goplugins/otel/main.goplugins/logging/main.goplugins/maxim/plugin_test.gocore/schemas/plugin.goplugins/governance/main.goexamples/plugins/hello-world/main.goplugins/semanticcache/main.gotransports/bifrost-http/handlers/mcpinference.goframework/plugins/soloader.gocore/mcp/toolmanager.gocore/schemas/plugin_native.gocore/mcp/mcp.goframework/plugins/main.gocore/bifrost.goplugins/semanticcache/search.goplugins/mocker/plugin_test.gocore/schemas/bifrost.goframework/plugins/soplugin.gotransports/bifrost-http/lib/config.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/lib/config.go
📚 Learning: 2025-12-29T09:14:16.633Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 888
File: transports/bifrost-http/handlers/middlewares.go:246-256
Timestamp: 2025-12-29T09:14:16.633Z
Learning: In the bifrost HTTP transport, fasthttp.RequestCtx is the primary context carrier and should be passed directly to functions that expect a context.Context. Do not convert to context.Context unless explicitly required. Ensure tracer implementations and related components are designed to accept fasthttp.RequestCtx directly, and document this architectural decision for maintainers.
Applied to files:
transports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/lib/config.go
📚 Learning: 2025-12-30T05:37:48.365Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1180
File: docs/features/mcp/connecting-to-servers.mdx:452-458
Timestamp: 2025-12-30T05:37:48.365Z
Learning: When reviewing documentation PRs in a Graphite-managed stack, first check related or previous PRs in the stack for feature implementations before flagging documentation as incorrect or unsupported. Documentation MDX files often reference features implemented in earlier stack PRs; verify that the documented behavior exists in earlier changes and that the docs accurately reflect the implemented state before requesting edits.
Applied to files:
docs/features/observability/default.mdxdocs/quickstart/go-sdk/context-keys.mdxdocs/features/observability/maxim.mdxdocs/features/semantic-caching.mdxdocs/features/plugins/mocker.mdxdocs/architecture/framework/streaming.mdxdocs/architecture/core/plugins.mdxdocs/features/plugins/jsonparser.mdxdocs/features/governance/virtual-keys.mdxdocs/plugins/writing-go-plugin.mdx
📚 Learning: 2025-12-22T10:50:40.990Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1154
File: plugins/governance/store.go:1165-1186
Timestamp: 2025-12-22T10:50:40.990Z
Learning: In the Bifrost governance plugin, budgets and rate limits have 1:1 relationships with their parent entities (virtual keys, teams, customers). Do not assume sharing; ensure cascade deletion logic only deletes budgets/rate limits when there are no shared references. Enforce invariants in code and add tests to verify no cross-entity sharing and that cascade deletes only remove the specific child of the parent. If a counterexample arises, adjust data model or add guards.
Applied to files:
plugins/governance/utils.goplugins/governance/main.go
📚 Learning: 2026-01-10T08:07:45.952Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1299
File: core/mcp/codemodeexecutecode.go:882-907
Timestamp: 2026-01-10T08:07:45.952Z
Learning: In the MCP tool execution flow (core/mcp/codemodeexecutecode.go callMCPTool function), pre-hook modifications to tool names should NOT be applied. The originalToolName computed before pre-hooks must be used in the final MCP CallToolRequest. Only argument modifications from pre-hooks should be honored. This is a security/consistency decision to prevent plugins from redirecting tool execution.
Applied to files:
core/mcp/codemodeexecutecode.go
📚 Learning: 2026-01-12T05:34:25.834Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1300
File: plugins/logging/main.go:675-769
Timestamp: 2026-01-12T05:34:25.834Z
Learning: In plugins/logging/main.go, PreMCPHook, tool names are sourced from ChatAssistantMessageToolCall.Function.Name or ResponsesToolMessage.Name and upstream validation guarantees they are non-empty. Therefore, remove any empty-tool-name guard before inserting into the database. Ensure downstream logic relies on non-empty strings and add a clarifying comment about the upstream guarantee. This guidance is specific to this file’s flow; apply similar reasoning only where upstream validation is equally guaranteed.
Applied to files:
plugins/logging/main.go
🧬 Code graph analysis (26)
transports/bifrost-http/handlers/middlewares.go (1)
core/schemas/plugin.go (2)
BasePlugin(119-127)ObservabilityPlugin(173-187)
plugins/jsonparser/plugin_test.go (2)
core/schemas/account.go (1)
Account(81-97)core/schemas/plugin.go (1)
LLMPlugin(135-140)
plugins/semanticcache/plugin_integration_test.go (3)
plugins/maxim/main.go (1)
Plugin(44-50)plugins/semanticcache/main.go (1)
Plugin(137-144)examples/plugins/hello-world/main.go (2)
PreLLMHook(28-34)PostLLMHook(36-43)
plugins/governance/utils.go (3)
core/schemas/plugin.go (1)
HTTPRequest(30-36)core/utils.go (1)
Ptr(57-59)plugins/governance/main.go (1)
VirtualKeyPrefix(29-29)
examples/plugins/hello-world-wasm-go/types.go (2)
core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
core/mcp/agent.go (2)
core/schemas/bifrost.go (4)
BifrostMCPRequest(358-363)BifrostMCPResponse(442-446)RequestType(88-88)MCPRequestTypeChatToolCall(350-350)core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(756-761)
plugins/telemetry/main.go (4)
core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/bifrost.go (3)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(528-537)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)core/utils.go (1)
GetResponseFields(243-250)
plugins/mocker/main.go (2)
core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
core/mcp/codemodeexecutecode.go (6)
core/schemas/context.go (2)
BifrostContext(32-43)NewBifrostContext(48-71)core/schemas/bifrost.go (7)
BifrostContextKeyRequestID(124-124)BifrostContextKeyParentMCPRequestID(141-141)BifrostMCPRequest(358-363)RequestType(88-88)MCPRequestTypeChatToolCall(350-350)BifrostMCPResponse(442-446)BifrostMCPResponseExtraFields(462-467)core/schemas/chatcompletions.go (2)
ChatAssistantMessageToolCall(756-761)ChatAssistantMessageToolCallFunction(764-767)core/utils.go (1)
Ptr(57-59)core/schemas/utils.go (1)
Ptr(14-16)core/mcp/toolmanager.go (1)
ToolsManager(22-41)
framework/plugins/soplugin_test.go (5)
framework/plugins/main.go (3)
DynamicLLMPluginConfig(8-13)LoadLLMPlugins(39-55)Config(16-18)core/schemas/plugin.go (1)
HTTPTransportPlugin(129-133)examples/plugins/hello-world/main.go (3)
HTTPTransportIntercept(18-26)PreLLMHook(28-34)PostLLMHook(36-43)core/schemas/context.go (1)
NewBifrostContextWithTimeout(83-86)framework/plugins/soplugin.go (1)
DynamicLLMPlugin(10-24)
transports/bifrost-http/server/server.go (3)
core/schemas/plugin.go (3)
LLMPlugin(135-140)MCPPlugin(142-147)BasePlugin(119-127)framework/plugins/main.go (4)
Config(16-18)LoadLLMPlugins(39-55)AsLLMPlugin(22-27)AsMCPPlugin(31-36)framework/plugins/loader.go (1)
PluginLoader(6-8)
plugins/maxim/main.go (3)
core/schemas/plugin.go (1)
LLMPlugin(135-140)core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
plugins/otel/main.go (5)
examples/plugins/hello-world/main.go (2)
PreLLMHook(28-34)PostLLMHook(36-43)core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/bifrost.go (3)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(528-537)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
plugins/logging/main.go (4)
core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/bifrost.go (2)
BifrostResponse(368-389)BifrostError(528-537)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
plugins/maxim/plugin_test.go (1)
core/schemas/plugin.go (1)
LLMPlugin(135-140)
core/schemas/plugin.go (4)
core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/bifrost.go (5)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(528-537)BifrostMCPRequest(358-363)BifrostMCPResponse(442-446)core/schemas/plugin_native.go (2)
LLMPluginShortCircuit(16-20)MCPPluginShortCircuit(24-27)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
plugins/governance/main.go (4)
core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/bifrost.go (3)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(528-537)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)core/utils.go (1)
GetStringFromContext(306-313)
examples/plugins/hello-world/main.go (4)
core/schemas/bifrost.go (3)
BifrostContextKey(118-118)BifrostResponse(368-389)BifrostError(528-537)core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
framework/plugins/soloader.go (3)
core/schemas/plugin.go (1)
LLMPlugin(135-140)framework/plugins/soplugin.go (1)
DynamicLLMPlugin(10-24)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)
core/mcp/toolmanager.go (4)
core/bifrost.go (1)
PluginPipeline(81-97)core/schemas/bifrost.go (6)
BifrostMCPRequest(358-363)BifrostMCPResponse(442-446)RequestType(88-88)MCPRequestTypeChatToolCall(350-350)MCPRequestTypeResponsesToolCall(351-351)BifrostMCPResponseExtraFields(462-467)core/schemas/mcp.go (1)
MCPToolManagerConfig(36-40)core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(756-761)
core/schemas/plugin_native.go (1)
core/schemas/bifrost.go (2)
BifrostError(528-537)BifrostMCPResponse(442-446)
framework/plugins/main.go (2)
core/schemas/plugin.go (3)
BasePlugin(119-127)LLMPlugin(135-140)MCPPlugin(142-147)framework/plugins/loader.go (1)
PluginLoader(6-8)
plugins/mocker/plugin_test.go (2)
core/schemas/account.go (1)
Account(81-97)core/schemas/plugin.go (1)
LLMPlugin(135-140)
core/schemas/bifrost.go (3)
core/schemas/plugin.go (2)
LLMPlugin(135-140)MCPPlugin(142-147)core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(756-761)core/schemas/responses.go (2)
ResponsesToolMessage(471-491)ResponsesMessage(319-332)
framework/plugins/soplugin.go (4)
core/schemas/plugin.go (2)
HTTPRequest(30-36)HTTPResponse(40-44)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)examples/plugins/hello-world/main.go (5)
GetName(14-16)HTTPTransportIntercept(18-26)PreLLMHook(28-34)PostLLMHook(36-43)Cleanup(45-48)
transports/bifrost-http/lib/config.go (2)
core/schemas/plugin.go (4)
BasePlugin(119-127)LLMPlugin(135-140)MCPPlugin(142-147)HTTPTransportPlugin(129-133)framework/plugins/loader.go (1)
PluginLoader(6-8)
| if vkHeader != "" { | ||
| return bifrost.Ptr(vkHeader) | ||
| } | ||
| authHeader := req.Headers["Authorization"] |
There was a problem hiding this comment.
Header key casing inconsistency.
Per schemas.HTTPRequest definition, header keys are lowercase (see Headers map[string]string comment: "keys are lowercase"). The lookup should use "authorization" instead of "Authorization".
Suggested fix
- authHeader := req.Headers["Authorization"]
+ authHeader := req.Headers["authorization"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| authHeader := req.Headers["Authorization"] | |
| authHeader := req.Headers["authorization"] |
🤖 Prompt for AI Agents
In @plugins/governance/utils.go at line 24, The code reads the Authorization
header using the wrong case; update the lookup authHeader :=
req.Headers["Authorization"] to use the lowercase key per schemas.HTTPRequest
(Headers map[string]string uses lowercase keys) so it becomes authHeader :=
req.Headers["authorization"]; keep using the same authHeader variable and
req.Headers map.
cc747af to
fa24e05
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
plugins/mocker/main.go (2)
861-864: Nil pointer dereference when overriding model for Responses requests.When
req.RequestType == schemas.ResponsesRequest,mockResponse.ChatResponseisnil(line 835-858 showsResponsesResponseis populated instead). Line 863 will panic with a nil pointer dereference.🐛 Proposed fix
// Override model if specified if content.Model != nil { - mockResponse.ChatResponse.Model = *content.Model + if mockResponse.ChatResponse != nil { + mockResponse.ChatResponse.Model = *content.Model + } + // Note: ResponsesResponse doesn't have a Model field to override }
993-1041:handleDefaultBehavioronly generates ChatResponse, not ResponsesResponse.When
DefaultBehaviorSuccessis used and the original request was aResponsesRequest, the short-circuit returns aChatResponsestructure (lines 1008-1036). This may cause issues downstream if the caller expects aResponsesResponseformat.Consider mirroring the logic in
generateSuccessShortCircuitto handle both request types.framework/plugins/soplugin_test.go (2)
24-28: Skip these plugin-build tests on Windows (Go plugins unsupported) and optionally when toolchain prerequisites are missing.
Right now Windows will try-buildmode=pluginandmake clean, which is expected to fail.Proposed fix
func TestDynamicPluginLifecycle(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Go plugins (-buildmode=plugin) are not supported on Windows") + } // Build the hello-world plugin first pluginPath := buildHelloWorldPlugin(t) defer cleanupHelloWorldPlugin(t)func buildHelloWorldPlugin(t *testing.T) string { t.Helper() + + if runtime.GOOS == "windows" { + t.Skip("Go plugins (-buildmode=plugin) are not supported on Windows") + }func buildHelloWorldPluginForBenchmark(b *testing.B) string { b.Helper() + + if runtime.GOOS == "windows" { + b.Skip("Go plugins (-buildmode=plugin) are not supported on Windows") + }Also applies to: 342-376, 519-553
455-503: Benchmarks: avoid a fixed 10s context deadline across the whole benchmark loop.
With longer-benchtimeor slower CI, the context can expire mid-run and contaminate measurements.Proposed fix
func BenchmarkDynamicPlugin_PreHook(b *testing.B) { @@ - ctx := context.Background() + ctx := context.Background() @@ - pluginCtx, cancel := schemas.NewBifrostContextWithTimeout(ctx, 10*time.Second) + pluginCtx, cancel := schemas.NewBifrostContextWithTimeout(ctx, time.Hour) defer cancel() for i := 0; i < b.N; i++ { _, _, _ = plugin.PreLLMHook(pluginCtx, req) } }func BenchmarkDynamicPlugin_PostHook(b *testing.B) { @@ - pluginCtx, cancel := schemas.NewBifrostContextWithTimeout(ctx, 10*time.Second) + pluginCtx, cancel := schemas.NewBifrostContextWithTimeout(ctx, time.Hour) defer cancel() b.ResetTimer() for i := 0; i < b.N; i++ { _, _, _ = plugin.PostLLMHook(pluginCtx, resp, nil) } }transports/bifrost-http/server/server.go (1)
219-242: Add support for loading MCP-only dynamic plugins.Line 226 uses
LoadLLMPluginsto load all dynamic plugins, but the framework only supports loading LLM plugins. TheLoadDynamicLLMPluginmethod (soloader.go) requires LLM-specific symbols likePreLLMHookandPostLLMHook. A plugin that only implementsMCPPlugin(withPreMCPHookandPostMCPHook) cannot be loaded and will fail during symbol lookup. Add a complementaryLoadDynamicMCPPluginmethod toPluginLoaderand aLoadMCPPluginsfunction to handle MCP-only plugins.
🤖 Fix all issues with AI agents
In @core/schemas/bifrost.go:
- Around line 20-21: The test initialization uses the removed Plugins field on
BifrostConfig; update the call that constructs schemas.BifrostConfig in
tests/core-mcp/setup.go to remove the Plugins entry and, if plugin lists are
required, set the new LLMPlugins and/or MCPPlugins slices instead; locate the
bifrost.Init call and replace the Plugins: nil line with either no field at all
or with LLMPlugins: nil and/or MCPPlugins: nil as appropriate to match the new
BifrostConfig struct.
In @docs/plugins/getting-started.mdx:
- Around line 60-67: The v1.3.x docs incorrectly list the new hook names; update
the Tab titled "v1.3.x" to use the legacy hook names by replacing `PreLLMHook()`
with `PreHook()` and `PostLLMHook()` with `PostHook()` (leave other entries like
`Init(config any) error`, `GetName() string`, `TransportInterceptor()`, and
`Cleanup() error` unchanged) so the v1.3.x section reflects the old naming
convention.
- Around line 95-102: The documentation uses inconsistent terminology: replace
the occurrences of "PreHooks" with the correct "PreLLMHooks" in both tab blocks
(the line stating "PostLLMHook - Executes in reverse order of PreHooks" under
v1.4.x and the identical line under v1.3.x) so the docs consistently use the
updated hook name PreLLMHooks.
In @docs/plugins/migration-guide.mdx:
- Around line 11-13: The note is misleading about hook names; update the <Note>
text to explicitly state that while hook behavior/signature is unchanged, the
hook names were renamed (e.g., PreHook → PreLLMHook and PostHook → PostLLMHook),
and keep that Init, GetName, and Cleanup behaviors/signatures remain unchanged;
modify the sentence to clarify “behavior remains unchanged but names have been
updated from PreHook/PostHook to PreLLMHook/PostLLMHook” so readers aren’t
confused.
In @plugins/semanticcache/search.go:
- Around line 16-17: The semanticcache package is being compiled into WASM where
schemas.LLMPluginShortCircuit lacks a Stream field; update the package so
streaming-only code is excluded from WASM or made conditional: either add a
build tag (e.g., //go:build !tinygo && !wasm) to files containing streaming
logic (such as the file with buildStreamingResponseFromResult) so they aren’t
built for WASM, or refactor buildStreamingResponseFromResult to detect at
compile time whether schemas.LLMPluginShortCircuit supports Stream (use separate
non-WASM file that returns &schemas.LLMPluginShortCircuit{Stream: streamChan}
and keep WASM-safe file that returns a compatible struct). Ensure you reference
and modify buildStreamingResponseFromResult and the files that construct
schemas.LLMPluginShortCircuit so WASM builds no longer reference the Stream
field.
In @transports/bifrost-http/lib/config.go:
- Around line 2236-2299: The current rebuildPluginCaches calls
rebuildLLMPlugins, rebuildMCPPlugins, and rebuildHTTPTransportPlugins which each
call BasePlugins.Load() separately, risking mixed snapshots; change
rebuildPluginCaches to take a single snapshot by calling base :=
c.BasePlugins.Load() once and then pass that snapshot into new helper variants
(e.g., rebuildLLMPluginsFrom(base), rebuildMCPPluginsFrom(base),
rebuildHTTPTransportPluginsFrom(base)) or modify the existing rebuild* methods
to accept the snapshot as an argument and use it to build each cache so all
three caches are rebuilt from the same BasePlugins snapshot.
🧹 Nitpick comments (10)
plugins/mocker/benchmark_test.go (1)
12-67: Migration toPreLLMHooklooks correct.The method call updates from
PreHooktoPreLLMHookalign with the broader plugin domain segregation (LLM vs MCP) across the codebase.Optional: Consider renaming the benchmark function names and their doc comments to match the new terminology (e.g.,
BenchmarkMockerPlugin_PreLLMHook_SimpleRule) for consistency. Currently, function names referencePreHookwhile the actual method being tested isPreLLMHook.examples/plugins/hello-world-wasm-typescript/README.md (1)
1-3: Clarify this is an LLM plugin example.Given the plugin domain segregation introduced in this PR (LLM vs MCP), consider updating the title and description to clarify this is an LLM plugin example. This helps readers understand the scope and applicability of this example.
📝 Suggested clarification
-# Bifrost WASM Plugin (TypeScript/AssemblyScript) +# Bifrost LLM WASM Plugin (TypeScript/AssemblyScript) -A comprehensive example of a Bifrost plugin written in TypeScript and compiled to WebAssembly using AssemblyScript. This plugin demonstrates proper structure definitions, JSON parsing, context handling, and request/response modification patterns. +A comprehensive example of a Bifrost LLM plugin written in TypeScript and compiled to WebAssembly using AssemblyScript. This plugin demonstrates proper structure definitions, JSON parsing, context handling, and request/response modification patterns for LLM inference workflows.transports/bifrost-http/handlers/mcpinference.go (1)
48-79: Consider extracting common validation and context conversion logic.Both
executeChatMCPToolandexecuteResponsesMCPToolshare a similar pattern: JSON unmarshal → validate required field → convert context → execute → respond. The duplication is minor but could be reduced with a helper for context conversion and error handling.Also applies to: 81-112
core/mcp/mcp.go (1)
83-101: Silent failure on plugin pipeline type assertion may hide configuration issues.Lines 89-94: If
config.PluginPipelineProvider()returns a non-nil value that doesn't implementPluginPipeline, the wrapper silently returnsnil. This could lead to MCP requests being processed without plugin hooks, with no indication of the misconfiguration.Consider adding a warning log when the type assertion fails:
🔧 Suggested improvement
if config.PluginPipelineProvider != nil && config.ReleasePluginPipeline != nil { pluginPipelineProvider = func() PluginPipeline { if pipeline := config.PluginPipelineProvider(); pipeline != nil { if pp, ok := pipeline.(PluginPipeline); ok { return pp } + logger.Warn(fmt.Sprintf("%s plugin pipeline provider returned incompatible type", MCPLogPrefix)) } return nil }plugins/logging/main.go (1)
1-639: Consider implementing MCP hooks for tool execution logging if MCP tool execution events should be tracked alongside LLM requests.The PR introduces MCP plugin separation with
PreMCPHook/PostMCPHookinterfaces (defined incore/schemas/plugin.go), whileLoggerPlugincurrently only implementsLLMPluginhooks. If comprehensive logging of MCP tool executions is needed, consider addingPreMCPHookandPostMCPHookimplementations toLoggerPluginto maintain consistent logging coverage across both LLM and MCP operations.This may be intentionally deferred to a follow-up PR.
framework/plugins/soplugin.go (1)
39-47: Misleading comments: methods ARE used for dynamic plugins.The comments on lines 39 and 44 state "is not used for dynamic plugins", but these methods delegate to
dp.preHookanddp.postHookrespectively, which ARE the core hook implementations for dynamic plugins. The comments appear to be copy-paste artifacts.📝 Suggested comment fix
-// PreLLMHook is not used for dynamic plugins +// PreLLMHook executes the pre-hook loaded from the dynamic plugin func (dp *DynamicLLMPlugin) PreLLMHook(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) (*schemas.BifrostRequest, *schemas.LLMPluginShortCircuit, error) { return dp.preHook(ctx, req) } -// PostLLMHook is not used for dynamic plugins +// PostLLMHook executes the post-hook loaded from the dynamic plugin func (dp *DynamicLLMPlugin) PostLLMHook(ctx *schemas.BifrostContext, resp *schemas.BifrostResponse, bifrostErr *schemas.BifrostError) (*schemas.BifrostResponse, *schemas.BifrostError, error) { return dp.postHook(ctx, resp, bifrostErr) }framework/plugins/soplugin_test.go (1)
300-339: Consider capturing goroutine errors instead of callingassert.*from goroutines.
It’ll make failures cleaner and avoid interleaved logs.plugins/semanticcache/search.go (1)
31-34: Minor: typo in comment (“Store has and metadata…” → “hash”).transports/bifrost-http/server/server.go (1)
442-484: Consider extracting common plugin-finding logic to reduce duplication.
FindPluginByNameandFindMCPPluginByNamehave nearly identical implementations. The reflect-based interface compatibility check (lines 453-457 and 475-479) is duplicated.♻️ Suggestion: Extract shared logic into a generic helper
A single generic function with appropriate type constraints could reduce this duplication:
// findPluginByNameGeneric is a helper that implements the common lookup logic func findPluginByNameGeneric[T schemas.BasePlugin](plugins []T, name string) (T, error) { var zero T for _, plugin := range plugins { if plugin.GetName() == name { return plugin, nil } } return zero, fmt.Errorf("plugin %q not found", name) }The current implementation works but the reflect-based fallback (lines 453-457, 475-479) appears unnecessary since the type assertion on line 449/471 should succeed for properly typed interface implementations.
core/bifrost.go (1)
3966-3974: Consider defensive nil checks for atomic pointer dereferences.Lines 3969-3970 directly dereference the atomic pointers. While
InitandReloadConfigalways store non-nil slices, a defensive check would prevent panics if this invariant is ever violated.♻️ Defensive nil handling
func (bifrost *Bifrost) getPluginPipeline() *PluginPipeline { pipeline := bifrost.pluginPipelinePool.Get().(*PluginPipeline) - pipeline.llmPlugins = *bifrost.llmPlugins.Load() - pipeline.mcpPlugins = *bifrost.mcpPlugins.Load() + if llm := bifrost.llmPlugins.Load(); llm != nil { + pipeline.llmPlugins = *llm + } + if mcp := bifrost.mcpPlugins.Load(); mcp != nil { + pipeline.mcpPlugins = *mcp + } pipeline.logger = bifrost.logger pipeline.tracer = bifrost.getTracer() return pipeline }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/media/ui-disable-auth-on-inference.pngis excluded by!**/*.pngdocs/media/ui-enforce-virtual-keys.pngis excluded by!**/*.png
📒 Files selected for processing (69)
core/bifrost.gocore/chatbot_test.gocore/internal/testutil/setup.gocore/mcp/agent.gocore/mcp/codemodeexecutecode.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/schemas/bifrost.gocore/schemas/mcp.gocore/schemas/plugin.gocore/schemas/plugin_native.gocore/schemas/plugin_wasm.gocore/schemas/trace.gocore/utils.godocs/architecture/core/plugins.mdxdocs/architecture/framework/streaming.mdxdocs/changelogs/v1.3.12.mdxdocs/features/governance/virtual-keys.mdxdocs/features/mcp/tool-execution.mdxdocs/features/observability/default.mdxdocs/features/observability/maxim.mdxdocs/features/observability/otel.mdxdocs/features/plugins/jsonparser.mdxdocs/features/plugins/mocker.mdxdocs/features/semantic-caching.mdxdocs/plugins/getting-started.mdxdocs/plugins/migration-guide.mdxdocs/plugins/writing-go-plugin.mdxdocs/quickstart/go-sdk/context-keys.mdxexamples/plugins/hello-world-wasm-go/README.mdexamples/plugins/hello-world-wasm-go/types.goexamples/plugins/hello-world-wasm-rust/README.mdexamples/plugins/hello-world-wasm-rust/src/lib.rsexamples/plugins/hello-world-wasm-rust/src/types.rsexamples/plugins/hello-world-wasm-typescript/README.mdexamples/plugins/hello-world/main.goframework/plugins/loader.goframework/plugins/main.goframework/plugins/soloader.goframework/plugins/soplugin.goframework/plugins/soplugin_test.goplugins/governance/main.goplugins/governance/utils.goplugins/jsonparser/main.goplugins/jsonparser/plugin_test.goplugins/logging/main.goplugins/maxim/main.goplugins/maxim/plugin_test.goplugins/mocker/benchmark_test.goplugins/mocker/main.goplugins/mocker/plugin_test.goplugins/otel/main.goplugins/semanticcache/main.goplugins/semanticcache/plugin_core_test.goplugins/semanticcache/plugin_integration_test.goplugins/semanticcache/search.goplugins/semanticcache/test_utils.goplugins/telemetry/main.gotransports/bifrost-http/handlers/cache.gotransports/bifrost-http/handlers/devpprof.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/lib.gotransports/bifrost-http/server/server.gotransports/changelog.mdui/app/workspace/config/views/securityView.tsx
💤 Files with no reviewable changes (3)
- transports/bifrost-http/handlers/mcp.go
- core/internal/testutil/setup.go
- core/chatbot_test.go
✅ Files skipped from review due to trivial changes (1)
- transports/changelog.md
🚧 Files skipped from review as they are similar to previous changes (24)
- core/schemas/plugin_wasm.go
- core/utils.go
- plugins/mocker/plugin_test.go
- plugins/governance/utils.go
- examples/plugins/hello-world-wasm-rust/src/types.rs
- ui/app/workspace/config/views/securityView.tsx
- docs/architecture/core/plugins.mdx
- transports/bifrost-http/handlers/devpprof.go
- core/schemas/mcp.go
- examples/plugins/hello-world-wasm-go/README.md
- plugins/semanticcache/plugin_core_test.go
- docs/changelogs/v1.3.12.mdx
- docs/features/observability/otel.mdx
- docs/features/mcp/tool-execution.mdx
- docs/quickstart/go-sdk/context-keys.mdx
- transports/bifrost-http/handlers/middlewares.go
- docs/features/observability/maxim.mdx
- plugins/semanticcache/plugin_integration_test.go
- docs/features/plugins/mocker.mdx
- examples/plugins/hello-world-wasm-go/types.go
- examples/plugins/hello-world/main.go
- examples/plugins/hello-world-wasm-rust/src/lib.rs
- core/schemas/trace.go
- examples/plugins/hello-world-wasm-rust/README.md
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
docs/plugins/getting-started.mdxtransports/bifrost-http/lib/lib.goplugins/maxim/plugin_test.godocs/features/semantic-caching.mdxplugins/jsonparser/plugin_test.gotransports/bifrost-http/handlers/mcpserver.godocs/features/plugins/jsonparser.mdxplugins/mocker/benchmark_test.goplugins/semanticcache/test_utils.godocs/architecture/framework/streaming.mdxtransports/bifrost-http/lib/config.gocore/schemas/plugin_native.goframework/plugins/main.goplugins/jsonparser/main.goframework/plugins/loader.godocs/features/observability/default.mdxdocs/features/governance/virtual-keys.mdxtransports/bifrost-http/handlers/cache.gocore/mcp/mcp.goframework/plugins/soplugin_test.gocore/mcp/agent.goplugins/otel/main.gocore/mcp/codemodeexecutecode.gotransports/bifrost-http/handlers/mcpinference.goplugins/telemetry/main.gocore/schemas/plugin.goexamples/plugins/hello-world-wasm-typescript/README.mdplugins/semanticcache/main.goplugins/semanticcache/search.goplugins/governance/main.goframework/plugins/soloader.gocore/schemas/bifrost.godocs/plugins/migration-guide.mdxtransports/bifrost-http/server/server.goplugins/logging/main.goplugins/maxim/main.goframework/plugins/soplugin.gocore/mcp/toolmanager.godocs/plugins/writing-go-plugin.mdxplugins/mocker/main.gocore/bifrost.go
🧠 Learnings (9)
📚 Learning: 2025-12-30T05:37:48.365Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1180
File: docs/features/mcp/connecting-to-servers.mdx:452-458
Timestamp: 2025-12-30T05:37:48.365Z
Learning: When reviewing documentation PRs in a Graphite-managed stack, first check related or previous PRs in the stack for feature implementations before flagging documentation as incorrect or unsupported. Documentation MDX files often reference features implemented in earlier stack PRs; verify that the documented behavior exists in earlier changes and that the docs accurately reflect the implemented state before requesting edits.
Applied to files:
docs/plugins/getting-started.mdxdocs/features/semantic-caching.mdxdocs/features/plugins/jsonparser.mdxdocs/architecture/framework/streaming.mdxdocs/features/observability/default.mdxdocs/features/governance/virtual-keys.mdxdocs/plugins/migration-guide.mdxdocs/plugins/writing-go-plugin.mdx
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
transports/bifrost-http/lib/lib.goplugins/maxim/plugin_test.goplugins/jsonparser/plugin_test.gotransports/bifrost-http/handlers/mcpserver.goplugins/mocker/benchmark_test.goplugins/semanticcache/test_utils.gotransports/bifrost-http/lib/config.gocore/schemas/plugin_native.goframework/plugins/main.goplugins/jsonparser/main.goframework/plugins/loader.gotransports/bifrost-http/handlers/cache.gocore/mcp/mcp.goframework/plugins/soplugin_test.gocore/mcp/agent.goplugins/otel/main.gocore/mcp/codemodeexecutecode.gotransports/bifrost-http/handlers/mcpinference.goplugins/telemetry/main.gocore/schemas/plugin.goplugins/semanticcache/main.goplugins/semanticcache/search.goplugins/governance/main.goframework/plugins/soloader.gocore/schemas/bifrost.gotransports/bifrost-http/server/server.goplugins/logging/main.goplugins/maxim/main.goframework/plugins/soplugin.gocore/mcp/toolmanager.goplugins/mocker/main.gocore/bifrost.go
📚 Learning: 2025-12-29T11:54:55.836Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1153
File: framework/configstore/rdb.go:2221-2246
Timestamp: 2025-12-29T11:54:55.836Z
Learning: In Go reviews, do not flag range-over-int patterns like for i := range n as compile-time errors, assuming Go 1.22+ semantics. Only flag actual range-capable values (slices, arrays, maps, channels, strings) and other compile-time issues. This applies to all Go files across the repository.
Applied to files:
transports/bifrost-http/lib/lib.goplugins/maxim/plugin_test.goplugins/jsonparser/plugin_test.gotransports/bifrost-http/handlers/mcpserver.goplugins/mocker/benchmark_test.goplugins/semanticcache/test_utils.gotransports/bifrost-http/lib/config.gocore/schemas/plugin_native.goframework/plugins/main.goplugins/jsonparser/main.goframework/plugins/loader.gotransports/bifrost-http/handlers/cache.gocore/mcp/mcp.goframework/plugins/soplugin_test.gocore/mcp/agent.goplugins/otel/main.gocore/mcp/codemodeexecutecode.gotransports/bifrost-http/handlers/mcpinference.goplugins/telemetry/main.gocore/schemas/plugin.goplugins/semanticcache/main.goplugins/semanticcache/search.goplugins/governance/main.goframework/plugins/soloader.gocore/schemas/bifrost.gotransports/bifrost-http/server/server.goplugins/logging/main.goplugins/maxim/main.goframework/plugins/soplugin.gocore/mcp/toolmanager.goplugins/mocker/main.gocore/bifrost.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/lib/lib.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/handlers/cache.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/server/server.go
📚 Learning: 2025-12-29T09:14:16.633Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 888
File: transports/bifrost-http/handlers/middlewares.go:246-256
Timestamp: 2025-12-29T09:14:16.633Z
Learning: In the bifrost HTTP transport, fasthttp.RequestCtx is the primary context carrier and should be passed directly to functions that expect a context.Context. Do not convert to context.Context unless explicitly required. Ensure tracer implementations and related components are designed to accept fasthttp.RequestCtx directly, and document this architectural decision for maintainers.
Applied to files:
transports/bifrost-http/lib/lib.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/handlers/cache.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/server/server.go
📚 Learning: 2026-01-12T08:52:47.116Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1302
File: core/mcp/toolmanager.go:378-390
Timestamp: 2026-01-12T08:52:47.116Z
Learning: In MCP tool system under core/mcp/, tool names are sanitized by replacing '-' with '_' during discovery and this sanitized form is used throughout the system, including CallToolParams.Name and ExtraFields.ToolName. Do not reverse-sanitize or maintain mappings back to original names; rely on sanitized names in all MCP server calls and UI representations.
Applied to files:
core/mcp/mcp.gocore/mcp/agent.gocore/mcp/codemodeexecutecode.gocore/mcp/toolmanager.go
📚 Learning: 2026-01-10T08:07:45.952Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1299
File: core/mcp/codemodeexecutecode.go:882-907
Timestamp: 2026-01-10T08:07:45.952Z
Learning: In the MCP tool execution flow (core/mcp/codemodeexecutecode.go callMCPTool function), pre-hook modifications to tool names should NOT be applied. The originalToolName computed before pre-hooks must be used in the final MCP CallToolRequest. Only argument modifications from pre-hooks should be honored. This is a security/consistency decision to prevent plugins from redirecting tool execution.
Applied to files:
core/mcp/codemodeexecutecode.go
📚 Learning: 2025-12-22T10:50:40.990Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1154
File: plugins/governance/store.go:1165-1186
Timestamp: 2025-12-22T10:50:40.990Z
Learning: In the Bifrost governance plugin, budgets and rate limits have 1:1 relationships with their parent entities (virtual keys, teams, customers). Do not assume sharing; ensure cascade deletion logic only deletes budgets/rate limits when there are no shared references. Enforce invariants in code and add tests to verify no cross-entity sharing and that cascade deletes only remove the specific child of the parent. If a counterexample arises, adjust data model or add guards.
Applied to files:
plugins/governance/main.go
📚 Learning: 2026-01-12T05:34:25.834Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1300
File: plugins/logging/main.go:675-769
Timestamp: 2026-01-12T05:34:25.834Z
Learning: In plugins/logging/main.go, PreMCPHook, tool names are sourced from ChatAssistantMessageToolCall.Function.Name or ResponsesToolMessage.Name and upstream validation guarantees they are non-empty. Therefore, remove any empty-tool-name guard before inserting into the database. Ensure downstream logic relies on non-empty strings and add a clarifying comment about the upstream guarantee. This guidance is specific to this file’s flow; apply similar reasoning only where upstream validation is equally guaranteed.
Applied to files:
plugins/logging/main.go
🧬 Code graph analysis (29)
transports/bifrost-http/lib/lib.go (1)
core/schemas/plugin.go (1)
LLMPlugin(135-140)
plugins/maxim/plugin_test.go (2)
core/schemas/plugin.go (1)
LLMPlugin(135-140)core/schemas/account.go (1)
Account(81-97)
plugins/jsonparser/plugin_test.go (2)
core/schemas/account.go (1)
Account(81-97)core/schemas/plugin.go (1)
LLMPlugin(135-140)
transports/bifrost-http/handlers/mcpserver.go (2)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(756-761)core/schemas/bifrost.go (1)
BifrostError(528-537)
plugins/mocker/benchmark_test.go (4)
core/schemas/bifrost.go (3)
BifrostRequest(177-198)RequestType(88-88)ChatCompletionRequest(94-94)ui/lib/types/config.ts (1)
RequestType(144-167)transports/bifrost-http/handlers/inference.go (1)
ChatRequest(181-185)examples/plugins/hello-world/main.go (1)
PreLLMHook(28-34)
plugins/semanticcache/test_utils.go (2)
core/schemas/plugin.go (1)
LLMPlugin(135-140)core/bifrost.go (1)
Bifrost(55-78)
transports/bifrost-http/lib/config.go (3)
core/schemas/plugin.go (5)
BasePlugin(119-127)LLMPlugin(135-140)MCPPlugin(142-147)HTTPTransportPlugin(129-133)PluginType(81-81)framework/plugins/loader.go (1)
PluginLoader(6-8)framework/plugins/main.go (1)
Config(16-18)
core/schemas/plugin_native.go (2)
plugins/mocker/main.go (1)
Response(97-103)core/schemas/bifrost.go (4)
BifrostResponse(368-389)BifrostStream(493-500)BifrostError(528-537)BifrostMCPResponse(442-446)
framework/plugins/main.go (2)
core/schemas/plugin.go (3)
BasePlugin(119-127)LLMPlugin(135-140)MCPPlugin(142-147)framework/plugins/loader.go (1)
PluginLoader(6-8)
plugins/jsonparser/main.go (4)
examples/plugins/hello-world/main.go (2)
PreLLMHook(28-34)PostLLMHook(36-43)core/schemas/bifrost.go (3)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(528-537)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
framework/plugins/loader.go (1)
core/schemas/plugin.go (1)
LLMPlugin(135-140)
transports/bifrost-http/handlers/cache.go (1)
core/schemas/plugin.go (1)
LLMPlugin(135-140)
core/mcp/mcp.go (4)
core/bifrost.go (1)
PluginPipeline(81-97)core/mcp/toolmanager.go (2)
PluginPipeline(45-48)NewToolsManager(69-106)core/mcp.go (1)
MCPManager(49-56)core/schemas/bifrost.go (2)
BifrostMCPRequest(358-363)BifrostMCPResponse(442-446)
core/mcp/agent.go (2)
core/schemas/bifrost.go (4)
BifrostMCPRequest(358-363)BifrostMCPResponse(442-446)RequestType(88-88)MCPRequestTypeChatToolCall(350-350)core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(756-761)
plugins/otel/main.go (5)
examples/plugins/hello-world/main.go (2)
PreLLMHook(28-34)PostLLMHook(36-43)core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/bifrost.go (3)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(528-537)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
core/mcp/codemodeexecutecode.go (4)
core/schemas/context.go (2)
BifrostContext(32-43)NewBifrostContext(48-71)core/schemas/bifrost.go (6)
BifrostContextKeyRequestID(124-124)BifrostContextKeyParentMCPRequestID(141-141)RequestType(88-88)MCPRequestTypeChatToolCall(350-350)BifrostMCPResponse(442-446)BifrostMCPResponseExtraFields(462-467)core/schemas/chatcompletions.go (2)
ChatAssistantMessageToolCall(756-761)ChatAssistantMessageToolCallFunction(764-767)core/mcp/toolmanager.go (1)
ToolsManager(22-41)
transports/bifrost-http/handlers/mcpinference.go (2)
transports/bifrost-http/handlers/utils.go (2)
SendError(35-44)SendBifrostError(47-62)transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(81-409)
core/schemas/plugin.go (4)
core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/bifrost.go (4)
BifrostResponse(368-389)BifrostError(528-537)BifrostMCPRequest(358-363)BifrostMCPResponse(442-446)core/schemas/plugin_native.go (2)
LLMPluginShortCircuit(16-20)MCPPluginShortCircuit(24-27)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
plugins/semanticcache/main.go (3)
core/schemas/plugin.go (1)
LLMPlugin(135-140)core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)
plugins/semanticcache/search.go (3)
core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)framework/vectorstore/store.go (1)
SearchResult(46-50)
plugins/governance/main.go (5)
examples/plugins/hello-world/main.go (2)
PreLLMHook(28-34)PostLLMHook(36-43)core/schemas/bifrost.go (5)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(528-537)BifrostContextKeyVirtualKey(122-122)BifrostContextKeyRequestID(124-124)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)core/utils.go (1)
GetStringFromContext(306-313)
framework/plugins/soloader.go (5)
core/schemas/plugin.go (1)
LLMPlugin(135-140)framework/plugins/soplugin.go (1)
DynamicLLMPlugin(10-24)core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/bifrost.go (3)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(528-537)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
core/schemas/bifrost.go (3)
core/schemas/plugin.go (2)
LLMPlugin(135-140)MCPPlugin(142-147)core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(756-761)core/schemas/responses.go (1)
ResponsesToolMessage(471-491)
plugins/logging/main.go (5)
examples/plugins/hello-world/main.go (2)
PreLLMHook(28-34)PostLLMHook(36-43)core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/bifrost.go (3)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(528-537)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
plugins/maxim/main.go (4)
examples/plugins/hello-world/main.go (2)
PreLLMHook(28-34)PostLLMHook(36-43)core/schemas/plugin.go (1)
LLMPlugin(135-140)core/schemas/bifrost.go (3)
BifrostRequest(177-198)BifrostResponse(368-389)BifrostError(528-537)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)
framework/plugins/soplugin.go (4)
core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/plugin.go (2)
HTTPRequest(30-36)HTTPResponse(40-44)core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
core/mcp/toolmanager.go (4)
core/schemas/context.go (1)
BifrostContext(32-43)core/schemas/bifrost.go (6)
BifrostMCPRequest(358-363)BifrostMCPResponse(442-446)RequestType(88-88)MCPRequestTypeChatToolCall(350-350)MCPRequestTypeResponsesToolCall(351-351)BifrostMCPResponseExtraFields(462-467)core/schemas/mcp.go (1)
MCPToolManagerConfig(36-40)core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(756-761)
plugins/mocker/main.go (2)
core/schemas/plugin_native.go (1)
LLMPluginShortCircuit(16-20)core/schemas/plugin_wasm.go (1)
LLMPluginShortCircuit(7-10)
core/bifrost.go (6)
core/schemas/plugin.go (6)
LLMPlugin(135-140)MCPPlugin(142-147)PluginType(81-81)PluginTypeLLM(84-84)PluginTypeMCP(85-85)BasePlugin(119-127)core/schemas/bifrost.go (5)
BifrostMCPRequest(358-363)BifrostError(528-537)RequestType(88-88)BifrostMCPResponse(442-446)BifrostRequest(177-198)framework/tracing/tracer.go (1)
Tracer(17-20)core/schemas/mcp.go (2)
MCPConfig(16-34)MCPClientConfig(56-79)core/mcp/toolmanager.go (1)
PluginPipeline(45-48)core/schemas/context.go (1)
BifrostContext(32-43)
| <Tab title="v1.3.x"> | ||
| - `Init(config any) error` - Initialize the plugin with configuration | ||
| - `GetName() string` - Return the plugin name | ||
| - `PreHook()` - Intercept requests before they reach providers | ||
| - `PostHook()` - Process responses after provider calls | ||
| - `PreLLMHook()` - Intercept requests before they reach providers | ||
| - `PostLLMHook()` - Process responses after provider calls | ||
| - `TransportInterceptor()` - Modify raw HTTP headers/body (HTTP transport only) | ||
| - `Cleanup() error` - Clean up resources on shutdown | ||
| </Tab> |
There was a problem hiding this comment.
Inconsistent hook naming in v1.3.x tab.
The v1.3.x tab shows PreLLMHook() and PostLLMHook(), but v1.3.x uses the old naming convention (PreHook/PostHook). Only v1.4.x+ should reference the new PreLLMHook/PostLLMHook names.
Proposed fix
<Tab title="v1.3.x">
- `Init(config any) error` - Initialize the plugin with configuration
- `GetName() string` - Return the plugin name
- - `PreLLMHook()` - Intercept requests before they reach providers
- - `PostLLMHook()` - Process responses after provider calls
+ - `PreHook()` - Intercept requests before they reach providers
+ - `PostHook()` - Process responses after provider calls
- `TransportInterceptor()` - Modify raw HTTP headers/body (HTTP transport only)
- `Cleanup() error` - Clean up resources on shutdown
</Tab>🤖 Prompt for AI Agents
In @docs/plugins/getting-started.mdx around lines 60 - 67, The v1.3.x docs
incorrectly list the new hook names; update the Tab titled "v1.3.x" to use the
legacy hook names by replacing `PreLLMHook()` with `PreHook()` and
`PostLLMHook()` with `PostHook()` (leave other entries like `Init(config any)
error`, `GetName() string`, `TransportInterceptor()`, and `Cleanup() error`
unchanged) so the v1.3.x section reflects the old naming convention.
| func (plugin *Plugin) performDirectSearch(ctx *schemas.BifrostContext, req *schemas.BifrostRequest, cacheKey string) (*schemas.LLMPluginShortCircuit, error) { | ||
| // Generate hash for the request |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if semanticcache package exists and its build tags
echo "=== Checking semanticcache package ==="
find . -type f -path "*/semanticcache/*.go" | head -20
echo -e "\n=== Checking for build tags in semanticcache ==="
rg -n "go:build|//\s*\+build" plugins/semanticcache -S
echo -e "\n=== Checking LLMPluginShortCircuit struct definitions ==="
rg -n "type LLMPluginShortCircuit struct" -A 10
echo -e "\n=== Checking for Stream field usage ==="
rg -n "Stream\s+chan" --type go
echo -e "\n=== Checking WASM build configuration ==="
rg -n "wasm" --type go | head -20Repository: maximhq/bifrost
Length of output: 3567
🏁 Script executed:
#!/bin/bash
# Verify actual usage of LLMPluginShortCircuit.Stream in semanticcache/search.go
echo "=== Checking search.go for Stream field usage ==="
sed -n '16,30p' plugins/semanticcache/search.go
echo -e "\n=== Checking lines 86-103 in search.go ==="
sed -n '86,103p' plugins/semanticcache/search.go
echo -e "\n=== Checking lines 240-369 in search.go ==="
sed -n '240,369p' plugins/semanticcache/search.go
echo -e "\n=== Checking if semanticcache imports plugin_native or plugin_wasm ==="
rg "plugin_native|plugin_wasm" plugins/semanticcache/
echo -e "\n=== Checking semanticcache imports and actual Stream usage ==="
grep -n "Stream:" plugins/semanticcache/search.goRepository: maximhq/bifrost
Length of output: 6682
🏁 Script executed:
#!/bin/bash
# Check the PR context and whether semanticcache should be in WASM builds
echo "=== Checking main.go in semanticcache ==="
head -30 plugins/semanticcache/main.go
echo -e "\n=== Check if there's any wasm-specific configuration or comments ==="
rg -i "wasm" plugins/semanticcache/ --type go
echo -e "\n=== Check if other plugins have build tags ==="
fd "main.go" plugins/ --exec grep -l "go:build" {} \;
echo -e "\n=== Verify the exact error that would occur in WASM build ==="
echo "In WASM builds:"
echo "- plugin_wasm.go defines LLMPluginShortCircuit WITHOUT Stream field"
echo "- semanticcache/search.go line 367 tries to use Stream: streamChan"
echo "- semanticcache has no build tags, so it's included in WASM builds"
echo "- This causes compile error: unknown field 'Stream' in struct literal"Repository: maximhq/bifrost
Length of output: 1758
Add build tags to semanticcache package or conditionally compile streaming response logic.
The WASM build of LLMPluginShortCircuit (plugin_wasm.go) does not have a Stream field, but buildStreamingResponseFromResult() at line 367 attempts to return &schemas.LLMPluginShortCircuit{Stream: streamChan}. Since semanticcache has no build tags, it will be compiled for both native and WASM builds, causing a compilation failure in WASM targets.
Either add //go:build !tinygo && !wasm to semanticcache files to exclude WASM builds, or refactor streaming response handling to work with both struct definitions.
🤖 Prompt for AI Agents
In @plugins/semanticcache/search.go around lines 16 - 17, The semanticcache
package is being compiled into WASM where schemas.LLMPluginShortCircuit lacks a
Stream field; update the package so streaming-only code is excluded from WASM or
made conditional: either add a build tag (e.g., //go:build !tinygo && !wasm) to
files containing streaming logic (such as the file with
buildStreamingResponseFromResult) so they aren’t built for WASM, or refactor
buildStreamingResponseFromResult to detect at compile time whether
schemas.LLMPluginShortCircuit supports Stream (use separate non-WASM file that
returns &schemas.LLMPluginShortCircuit{Stream: streamChan} and keep WASM-safe
file that returns a compatible struct). Ensure you reference and modify
buildStreamingResponseFromResult and the files that construct
schemas.LLMPluginShortCircuit so WASM builds no longer reference the Stream
field.
| // rebuildHTTPTransportPlugins rebuilds the cached list of HTTP transport plugins. | ||
| // This should be called after any plugin add/reload/remove operation. | ||
| // It filters BasePlugins for HTTPTransportPlugin implementations. | ||
| func (c *Config) rebuildHTTPTransportPlugins() { | ||
| var httpTransportPlugins []schemas.HTTPTransportPlugin | ||
|
|
||
| // Filter BasePlugins for HTTPTransportPlugin implementations | ||
| if basePlugins := c.BasePlugins.Load(); basePlugins != nil { | ||
| for _, p := range *basePlugins { | ||
| if httpPlugin, ok := p.(schemas.HTTPTransportPlugin); ok { | ||
| httpTransportPlugins = append(httpTransportPlugins, httpPlugin) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Store the rebuilt list | ||
| c.HTTPTransportPlugins.Store(&httpTransportPlugins) | ||
| } | ||
|
|
||
| // rebuildLLMPlugins rebuilds the cached list of LLM plugins. | ||
| // This should be called after any plugin add/reload/remove operation. | ||
| // It filters BasePlugins for LLMPlugin implementations. | ||
| func (c *Config) rebuildLLMPlugins() { | ||
| var llmPlugins []schemas.LLMPlugin | ||
|
|
||
| // Filter BasePlugins for LLMPlugin implementations | ||
| if basePlugins := c.BasePlugins.Load(); basePlugins != nil { | ||
| for _, p := range *basePlugins { | ||
| if llmPlugin, ok := p.(schemas.LLMPlugin); ok { | ||
| llmPlugins = append(llmPlugins, llmPlugin) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Store the rebuilt list | ||
| c.LLMPlugins.Store(&llmPlugins) | ||
| } | ||
|
|
||
| // rebuildMCPPlugins rebuilds the cached list of MCP plugins. | ||
| // This should be called after any plugin add/reload/remove operation. | ||
| // It filters BasePlugins for MCPPlugin implementations. | ||
| func (c *Config) rebuildMCPPlugins() { | ||
| var mcpPlugins []schemas.MCPPlugin | ||
|
|
||
| // Filter BasePlugins for MCPPlugin implementations | ||
| if basePlugins := c.BasePlugins.Load(); basePlugins != nil { | ||
| for _, p := range *basePlugins { | ||
| if mcpPlugin, ok := p.(schemas.MCPPlugin); ok { | ||
| mcpPlugins = append(mcpPlugins, mcpPlugin) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Store the rebuilt list | ||
| c.MCPPlugins.Store(&mcpPlugins) | ||
| } | ||
|
|
||
| // rebuildPluginCaches rebuilds all plugin caches from BasePlugins. | ||
| // This should be called after any plugin add/reload/remove operation. | ||
| func (c *Config) rebuildPluginCaches() { | ||
| c.rebuildLLMPlugins() | ||
| c.rebuildMCPPlugins() | ||
| c.rebuildHTTPTransportPlugins() | ||
| } |
There was a problem hiding this comment.
Rebuild caches from a single BasePlugins snapshot to avoid mixed views under concurrent updates.
Right now rebuildPluginCaches() can compute LLM/MCP/HTTP caches from different BasePlugins versions.
Proposed fix
func (c *Config) rebuildPluginCaches() {
- c.rebuildLLMPlugins()
- c.rebuildMCPPlugins()
- c.rebuildHTTPTransportPlugins()
+ var llmPlugins []schemas.LLMPlugin
+ var mcpPlugins []schemas.MCPPlugin
+ var httpTransportPlugins []schemas.HTTPTransportPlugin
+
+ if basePlugins := c.BasePlugins.Load(); basePlugins != nil {
+ for _, p := range *basePlugins {
+ if llmPlugin, ok := p.(schemas.LLMPlugin); ok {
+ llmPlugins = append(llmPlugins, llmPlugin)
+ }
+ if mcpPlugin, ok := p.(schemas.MCPPlugin); ok {
+ mcpPlugins = append(mcpPlugins, mcpPlugin)
+ }
+ if httpPlugin, ok := p.(schemas.HTTPTransportPlugin); ok {
+ httpTransportPlugins = append(httpTransportPlugins, httpPlugin)
+ }
+ }
+ }
+
+ c.LLMPlugins.Store(&llmPlugins)
+ c.MCPPlugins.Store(&mcpPlugins)
+ c.HTTPTransportPlugins.Store(&httpTransportPlugins)
}🤖 Prompt for AI Agents
In @transports/bifrost-http/lib/config.go around lines 2236 - 2299, The current
rebuildPluginCaches calls rebuildLLMPlugins, rebuildMCPPlugins, and
rebuildHTTPTransportPlugins which each call BasePlugins.Load() separately,
risking mixed snapshots; change rebuildPluginCaches to take a single snapshot by
calling base := c.BasePlugins.Load() once and then pass that snapshot into new
helper variants (e.g., rebuildLLMPluginsFrom(base), rebuildMCPPluginsFrom(base),
rebuildHTTPTransportPluginsFrom(base)) or modify the existing rebuild* methods
to accept the snapshot as an argument and use it to build each cache so all
three caches are rebuilt from the same BasePlugins snapshot.
fa24e05 to
5ced4fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
plugins/mocker/main.go (1)
866-869: Potential nil pointer dereference when RequestType is ResponsesRequest.When
req.RequestType == schemas.ResponsesRequest, onlymockResponse.ResponsesResponseis populated (lines 841-864), leavingmockResponse.ChatResponseas nil. However, line 868 unconditionally accessesmockResponse.ChatResponse.Modelwhencontent.Model != nil, which will panic.🐛 Proposed fix
// Override model if specified if content.Model != nil { - mockResponse.ChatResponse.Model = *content.Model + if mockResponse.ChatResponse != nil { + mockResponse.ChatResponse.Model = *content.Model + } else if mockResponse.ResponsesResponse != nil { + mockResponse.ResponsesResponse.Model = content.Model + } }core/mcp/codemodeexecutecode.go (1)
728-971: Guard against nil tool name after PreMCPHook updates.If a pre-hook returns a tool call without
Function.Name, the later dereference inExtraFields.ToolNamecan panic. Also, tool-name changes should be ignored per policy—enforce the original name back onto the tool call before it’s reused.🛠️ Suggested fix
// If pre-hooks modified the request, extract updated tool name and args if preReq != nil && preReq.ChatAssistantMessageToolCall != nil { toolCall = *preReq.ChatAssistantMessageToolCall + // Enforce original tool name (pre-hook name changes are ignored) + toolCall.Function.Name = schemas.Ptr(toolName) if toolCall.Function.Arguments != "" { // Re-parse arguments if they were modified if err := sonic.Unmarshal([]byte(toolCall.Function.Arguments), &args); err != nil { logger.Warn(fmt.Sprintf("%s Failed to parse modified tool arguments, using original: %v", CodeModeLogPrefix, err)) } } }Based on learnings, tool name changes should be ignored.
plugins/semanticcache/main.go (1)
348-360: Update PreLLMHook return doc to LLMPluginShortCircuit.The return description still references
*schemas.BifrostResponse, which no longer matches the signature.✏️ Doc fix
-// - *schemas.BifrostResponse: Cached response if found, nil otherwise +// - *schemas.LLMPluginShortCircuit: Short-circuit with cached response if found, nil otherwiseframework/plugins/soplugin_test.go (1)
108-112: Fix compile-time call to HTTPTransportPostHook.Line 109:
pluginisschemas.LLMPlugin, which doesn’t expose HTTPTransportPostHook. Use a type assertion like the pre-hook test to avoid a compile error.🛠️ Proposed fix
- err := plugin.HTTPTransportPostHook(pluginCtx, req, resp) + httpTransportPlugin, ok := plugin.(schemas.HTTPTransportPlugin) + require.True(t, ok, "Plugin should be a HTTPTransportPlugin") + err := httpTransportPlugin.HTTPTransportPostHook(pluginCtx, req, resp)core/bifrost.go (1)
2964-2977: Use preCount to preserve pre/post hook symmetry.Lines 2964 and 3165 use the current LLM plugin count, which can skip PostLLMHook calls if plugins are reloaded mid-flight. Use the
preCountcaptured from RunLLMPreHooks to guarantee symmetry for every request.🐛 Proposed fix
- pluginCount := len(*bifrost.llmPlugins.Load()) + pluginCount := preCount @@ - resp, bifrostErr := pipeline.RunPostHooks(msg.Context, result, nil, pluginCount) + resp, bifrostErr := pipeline.RunPostHooks(msg.Context, result, nil, preCount) @@ - resp, bifrostErrPtr = pipeline.RunPostHooks(msg.Context, nil, bifrostErrPtr, pluginCount) + resp, bifrostErrPtr = pipeline.RunPostHooks(msg.Context, nil, bifrostErrPtr, preCount) @@ - recoveredResp, recoveredErr := pipeline.RunPostHooks(ctx, nil, &bifrostErrVal, len(*bifrost.llmPlugins.Load())) + recoveredResp, recoveredErr := pipeline.RunPostHooks(ctx, nil, &bifrostErrVal, preCount)Also applies to: 3165-3165
🤖 Fix all issues with AI agents
In `@core/schemas/plugin_wasm.go`:
- Around line 5-7: Update the doc comment for the LLMPluginShortCircuit type to
remove the implication that streaming short-circuits are supported in WASM;
specifically, edit the comment that currently lists "a response, a stream, or an
error" so it only mentions response (success short-circuit) and error (error
short-circuit) and clearly states that streams are not supported in the WASM
build for LLMPluginShortCircuit.
In `@framework/plugins/loader.go`:
- Line 7: The build error is caused by referencing an undefined type
schemas.LLMPlugin in the signature of LoadDynamicLLMPlugin; verify that the
LLMPlugin type is declared and exported (type LLMPlugin interface{...}) in
core/schemas/plugin.go and that this PR or its base stack includes that file,
then fix loader.go by importing the correct package path (core/schemas) and
using the exported type name (schemas.LLMPlugin) or update the signature to the
actual package/type if the type was moved/renamed; ensure the import block in
framework/plugins/loader.go includes the core/schemas package and rerun CI.
In `@plugins/governance/utils.go`:
- Around line 18-22: The function parseVirtualKeyFromHTTPRequest dereferences
req without checking for nil; add a defensive nil guard at the start of
parseVirtualKeyFromHTTPRequest that returns nil if req is nil, then proceed to
call req.CaseInsensitiveHeaderLookup("x-bf-vk") and return bifrost.Ptr(vkHeader)
only when the header is non-empty; this prevents a panic when callers may pass a
nil *schemas.HTTPRequest.
- Around line 26-43: The prefix validation uses case-insensitive checks
(strings.ToLower(...)) but the function returns the raw header values, causing
potential lookup mismatches against the in-memory store; normalize returned
virtual keys to a canonical form (lowercase) before returning so lookups succeed
(e.g., when extracting authHeaderValue, xAPIKey, xGoogleAPIKey return
strings.ToLower(value) or equivalent); keep VirtualKeyPrefix and
GenerateVirtualKey() semantics intact while ensuring CaseInsensitiveHeaderLookup
results are normalized to lowercase before calling bifrost.Ptr.
♻️ Duplicate comments (3)
plugins/semanticcache/search.go (1)
367-369: WASM build still referencesStreamonLLMPluginShortCircuit.Line 367 constructs
LLMPluginShortCircuit{Stream: ...}but the WASM/TinyGo definition omitsStream, so this still breaks WASM builds. This is the same issue raised earlier—please address via build tags or split implementations.docs/plugins/getting-started.mdx (1)
61-68: v1.3.x tab still uses incorrect hook names.The v1.3.x tab at lines 65-66 shows
PreLLMHook()andPostLLMHook(), but v1.3.x uses the legacy naming convention (PreHook/PostHook). This contradicts lines 101-103 in the same file which correctly showPreHook/PostHookfor v1.3.x.Proposed fix
<Tab title="v1.3.x"> - `Init(config any) error` - Initialize the plugin with configuration - `GetName() string` - Return the plugin name - `TransportInterceptor()` - Modify raw HTTP headers/body (HTTP transport only) - - `PreLLMHook()` - Intercept requests before they reach providers - - `PostLLMHook()` - Process responses after provider calls + - `PreHook()` - Intercept requests before they reach providers + - `PostHook()` - Process responses after provider calls - `Cleanup() error` - Clean up resources on shutdown </Tab>framework/plugins/soplugin.go (1)
42-45: Misleading comment - PreLLMHook IS used for dynamic plugins.The comment states "is not used for dynamic plugins" but the method body clearly delegates to
dp.preHook(ctx, req). This appears to be an incomplete fix from the previous review - the PostLLMHook comment was corrected but PreLLMHook was not.Fix the misleading comment
-// PreLLMHook is not used for dynamic plugins +// PreLLMHook executes the pre-LLM hook for this dynamic plugin func (dp *DynamicLLMPlugin) PreLLMHook(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) (*schemas.BifrostRequest, *schemas.LLMPluginShortCircuit, error) { return dp.preHook(ctx, req) }
🧹 Nitpick comments (2)
core/schemas/plugin.go (1)
265-285: Clarify ObservabilityPlugin’s LLM hook expectations.Line 265: the comment says these plugins “continue to work as regular plugins via PreLLMHook/PostLLMHook,” but ObservabilityPlugin no longer embeds LLMPlugin. Consider explicitly stating that implementations should also satisfy LLMPlugin (or embed it) to avoid confusion.
framework/plugins/soplugin_test.go (1)
625-628: Prefer bifrost.Ptr in helper.Line 627: the repo convention is to use
bifrost.Ptr()instead of&for pointer creation. Based on learnings, please switch the helper.♻️ Proposed fix
-func stringPtr(s string) *string { - return &s -} +func stringPtr(s string) *string { + return bifrost.Ptr(s) +}import ( "context" "os" @@ - "github.com/maximhq/bifrost/core/schemas" + bifrost "github.com/maximhq/bifrost/core" + "github.com/maximhq/bifrost/core/schemas"
| // LLMPluginShortCircuit represents a plugin's decision to short-circuit the normal flow. | ||
| // It can contain either a response (success short-circuit), a stream (streaming short-circuit), or an error (error short-circuit). | ||
| // Streams are not supported in WASM plugins. |
There was a problem hiding this comment.
Docstring overstates streaming support in WASM build.
Line 6 suggests stream short-circuit is possible, but the WASM type has no Stream field and explicitly says streams aren’t supported. Tighten the comment to avoid implying a supported field.
📝 Proposed doc fix
-// It can contain either a response (success short-circuit), a stream (streaming short-circuit), or an error (error short-circuit).
-// Streams are not supported in WASM plugins.
+// It can contain either a response (success short-circuit) or an error (error short-circuit).
+// Streaming short-circuit is only available in native builds (see plugin_native.go).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // LLMPluginShortCircuit represents a plugin's decision to short-circuit the normal flow. | |
| // It can contain either a response (success short-circuit), a stream (streaming short-circuit), or an error (error short-circuit). | |
| // Streams are not supported in WASM plugins. | |
| // LLMPluginShortCircuit represents a plugin's decision to short-circuit the normal flow. | |
| // It can contain either a response (success short-circuit) or an error (error short-circuit). | |
| // Streaming short-circuit is only available in native builds (see plugin_native.go). |
🤖 Prompt for AI Agents
In `@core/schemas/plugin_wasm.go` around lines 5 - 7, Update the doc comment for
the LLMPluginShortCircuit type to remove the implication that streaming
short-circuits are supported in WASM; specifically, edit the comment that
currently lists "a response, a stream, or an error" so it only mentions response
(success short-circuit) and error (error short-circuit) and clearly states that
streams are not supported in the WASM build for LLMPluginShortCircuit.
| if strings.HasPrefix(strings.ToLower(authHeader), "bearer ") { | ||
| authHeaderValue := strings.TrimSpace(authHeader[7:]) // Remove "Bearer " prefix | ||
| if authHeaderValue != "" && strings.HasPrefix(strings.ToLower(authHeaderValue), VirtualKeyPrefix) { | ||
| virtualKeyValue = authHeaderValue | ||
| } | ||
| } | ||
| } | ||
| return "" | ||
| if virtualKeyValue != "" { | ||
| return bifrost.Ptr(virtualKeyValue) | ||
| } | ||
| xAPIKey := req.CaseInsensitiveHeaderLookup("x-api-key") | ||
| if xAPIKey != "" && strings.HasPrefix(strings.ToLower(xAPIKey), VirtualKeyPrefix) { | ||
| return bifrost.Ptr(xAPIKey) | ||
| } | ||
| // Checking x-goog-api-key header | ||
| xGoogleAPIKey := req.CaseInsensitiveHeaderLookup("x-goog-api-key") | ||
| if xGoogleAPIKey != "" && strings.HasPrefix(strings.ToLower(xGoogleAPIKey), VirtualKeyPrefix) { | ||
| return bifrost.Ptr(xGoogleAPIKey) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the full file to understand the function and context
cat -n plugins/governance/utils.go | head -80Repository: maximhq/bifrost
Length of output: 2665
🏁 Script executed:
# Search for VirtualKeyPrefix to understand what it is and how it's used
rg "VirtualKeyPrefix" -B 2 -A 2Repository: maximhq/bifrost
Length of output: 5306
🏁 Script executed:
# Look for where this function is called and how the returned value is used
rg "virtualKeyValue|xAPIKey|xGoogleAPIKey" -B 2 -A 2 --type goRepository: maximhq/bifrost
Length of output: 4585
🏁 Script executed:
# Search for GetVirtualKey implementation to understand if it's case-sensitive
rg "GetVirtualKey" -B 3 -A 5 --type goRepository: maximhq/bifrost
Length of output: 45403
🏁 Script executed:
# Check how virtual keys are stored in the store
rg "store.*VirtualKey|VirtualKey.*store" -B 2 -A 2 --type goRepository: maximhq/bifrost
Length of output: 50371
🏁 Script executed:
# Look for the store implementation and key comparison logic
fd -e go -path "*/store*" | head -20Repository: maximhq/bifrost
Length of output: 227
🏁 Script executed:
# Check how virtual keys are loaded and stored in the in-memory store
rg "virtualKeys\." -B 2 -A 2 --type go | head -100Repository: maximhq/bifrost
Length of output: 5500
🏁 Script executed:
# Look at the LocalGovernanceStore initialization to see how keys are loaded
rg "NewLocalGovernanceStore|LoadGovernanceData" -B 5 -A 10 --type go | head -150Repository: maximhq/bifrost
Length of output: 10641
Consider normalizing virtual key casing for consistency between validation and lookup.
The code uses case-insensitive prefix checks (strings.ToLower(...)) but performs case-sensitive key lookups in the in-memory store via exact match. If a header contains mixed-case input like SK-BF-xxx, it will pass validation but fail the subsequent store lookup. While generated virtual keys are always lowercase (from GenerateVirtualKey()), manually created keys with non-standard casing could cause silent lookup failures. For consistency, either normalize returned values to lowercase before lookup or document that virtual key values must be lowercase.
🤖 Prompt for AI Agents
In `@plugins/governance/utils.go` around lines 26 - 43, The prefix validation uses
case-insensitive checks (strings.ToLower(...)) but the function returns the raw
header values, causing potential lookup mismatches against the in-memory store;
normalize returned virtual keys to a canonical form (lowercase) before returning
so lookups succeed (e.g., when extracting authHeaderValue, xAPIKey,
xGoogleAPIKey return strings.ToLower(value) or equivalent); keep
VirtualKeyPrefix and GenerateVirtualKey() semantics intact while ensuring
CaseInsensitiveHeaderLookup results are normalized to lowercase before calling
bifrost.Ptr.
Pratham-Mishra04
force-pushed
the
01-07-feat_plugin_schemas_segregation_for_mcp_plugins
branch
from
5ced4fe to
39fae13
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
plugins/maxim/main.go (1)
491-551: Avoid ctx access inside the async logging goroutine.
bifrost.GetTracerFromContext(ctx)is still called after the goroutine boundary, which can race if the sameBifrostContextis reused/mutated elsewhere. Capture tracer info before spawning the goroutine and reuse it inside.🔧 Suggested fix
isFinalChunk := bifrost.IsFinalChunk(ctx) + tracer, bifrostTraceID, tracerErr := bifrost.GetTracerFromContext(ctx) + hasTracer := tracerErr == nil && tracer != nil && bifrostTraceID != "" go func() { requestType, _, model := bifrost.GetResponseFields(result, bifrostErr) var streamResponse *streaming.ProcessedStreamResponse if bifrost.IsStreamRequestType(requestType) { - // Use central tracer's accumulator - tracer, bifrostTraceID, err := bifrost.GetTracerFromContext(ctx) - if err == nil && tracer != nil && bifrostTraceID != "" { + // Use central tracer's accumulator + if hasTracer { accResult := tracer.ProcessStreamingChunk(bifrostTraceID, isFinalChunk, result, bifrostErr) if accResult != nil { streamResponse = convertAccResultToProcessedStreamResponse(accResult) } } @@ if bifrost.IsStreamRequestType(requestType) { // Cleanup via central tracer - tracer, bifrostTraceID, err := bifrost.GetTracerFromContext(ctx) - if err == nil && tracer != nil && bifrostTraceID != "" { + if hasTracer { tracer.CleanupStreamAccumulator(bifrostTraceID) } } } else if result != nil { @@ if streamResponse != nil && isFinalChunk { // Cleanup via central tracer - tracer, bifrostTraceID, err := bifrost.GetTracerFromContext(ctx) - if err == nil && tracer != nil && bifrostTraceID != "" { + if hasTracer { tracer.CleanupStreamAccumulator(bifrostTraceID) } } }plugins/jsonparser/main.go (1)
125-134: Guardresultbefore callingGetExtraFields().
result.GetExtraFields()is called before the nil check, so a nilresultwill panic. Move the nil guard ahead of the extra fields access.🔧 Proposed fix
- extraFields := result.GetExtraFields() - - // Check if plugin should run based on usage type - if !p.shouldRun(ctx, extraFields.RequestType) { - return result, err, nil - } - - // If no chat response, return as is - if result == nil || result.ChatResponse == nil { + // If no response, return as is + if result == nil || result.ChatResponse == nil { return result, err, nil } + + extraFields := result.GetExtraFields() + + // Check if plugin should run based on usage type + if !p.shouldRun(ctx, extraFields.RequestType) { + return result, err, nil + }plugins/mocker/main.go (1)
866-869: Guard model override to avoid nil dereference.When the request is a Responses API call,
mockResponse.ChatResponseis nil; settingmockResponse.ChatResponse.Modelwill panic ifcontent.Modelis set.🔧 Proposed fix
- if content.Model != nil { - mockResponse.ChatResponse.Model = *content.Model - } + if content.Model != nil && mockResponse.ChatResponse != nil { + mockResponse.ChatResponse.Model = *content.Model + }
🤖 Fix all issues with AI agents
In `@core/mcp/codemodeexecutecode.go`:
- Around line 866-945: The response metadata can reflect a pre-hook mutated tool
name because ExtraFields.ToolName uses *toolCall.Function.Name; ensure the
reported tool name matches what was actually executed by using the execution
variable (toolNameToCall or originalToolName) instead of the possibly modified
toolCall.Function.Name. Update the construction of mcpResp (and any ExtraFields
or log/metadata fields that reference the tool name) to set ToolName =
toolNameToCall and keep createToolResponseMessage using toolCall for
arguments/response content if needed, so execution and metadata remain
consistent.
In `@examples/plugins/hello-world/main.go`:
- Around line 38-43: The PreLLMHook function currently returns
*schemas.PluginShortCircuit which mismatches the loader's expected type; change
its return type to *schemas.LLMPluginShortCircuit so LoadDynamicLLMPlugin's
type-assertion succeeds, update any variable names/return values accordingly in
PreLLMHook, and ensure the function signature exactly matches the expected
signature used by LoadDynamicLLMPlugin (ctx *schemas.BifrostContext, req
*schemas.BifrostRequest) (*schemas.BifrostRequest,
*schemas.LLMPluginShortCircuit, error).
In `@transports/bifrost-http/handlers/mcpinference.go`:
- Around line 62-68: ConvertToBifrostContext may return a nil bifrostCtx (and no
cancel), so calling defer cancel() immediately can panic; update the handlers to
check bifrostCtx for nil first, call cancel() if non-nil before returning on
error, and only defer cancel() after confirming bifrostCtx (and cancel) are
valid. Specifically, in the code using ConvertToBifrostContext, move the
nil-check for bifrostCtx ahead of defer, call cancel() conditionally when
returning (if cancel != nil), and then place defer cancel() after the successful
nil-check; apply the same change in both handler locations that use
ConvertToBifrostContext (references: ConvertToBifrostContext, bifrostCtx,
cancel, SendError).
♻️ Duplicate comments (8)
examples/plugins/hello-world-wasm-go/README.md (1)
170-170: Soften “Full feature parity” wording.This still reads as complete parity with native plugins. Consider narrowing the claim to these specific hooks/features.
docs/plugins/getting-started.mdx (1)
61-68: Inconsistent hook naming in v1.3.x tab.The v1.3.x tab shows
PreLLMHook()andPostLLMHook()but v1.3.x uses the legacy naming convention (PreHook/PostHook). Only v1.4.x+ should reference the new hook names.Proposed fix
<Tab title="v1.3.x"> - `Init(config any) error` - Initialize the plugin with configuration - `GetName() string` - Return the plugin name - `TransportInterceptor()` - Modify raw HTTP headers/body (HTTP transport only) - - `PreLLMHook()` - Intercept requests before they reach providers - - `PostLLMHook()` - Process responses after provider calls + - `PreHook()` - Intercept requests before they reach providers + - `PostHook()` - Process responses after provider calls - `Cleanup() error` - Clean up resources on shutdown </Tab>plugins/governance/utils.go (1)
11-45: Add a nil guard before dereferencingreq(Line 20).This was already raised earlier; still applies to prevent a panic if callers pass nil.
plugins/semanticcache/search.go (1)
367-369: WASM build incompatibility withStreamfield.The
LLMPluginShortCircuitstruct incore/schemas/plugin_wasm.go(lines 7-10) does not include aStreamfield, but this code assigns toStream: streamChan. Sincesemanticcachehas no build tags, it will be compiled for WASM targets, causing a compilation failure.This issue was already flagged in a previous review. Either add build tags (
//go:build !tinygo && !wasm) to exclude this file from WASM builds, or refactor streaming response handling to conditionally compile.core/schemas/plugin_wasm.go (1)
5-11: Docstring still mentions streaming short-circuit which isn't supported in WASM.The comment on line 6 mentions "a stream (streaming short-circuit)" but the WASM type has no
Streamfield and line 7 explicitly states streams aren't supported. This inconsistency was previously flagged.📝 Proposed fix
-// LLMPluginShortCircuit represents a plugin's decision to short-circuit the normal flow. -// It can contain either a response (success short-circuit), a stream (streaming short-circuit), or an error (error short-circuit). -// Streams are not supported in WASM plugins. +// LLMPluginShortCircuit represents a plugin's decision to short-circuit the normal flow. +// It can contain either a response (success short-circuit) or an error (error short-circuit). +// Streaming short-circuit is only available in native builds (see plugin_native.go). type LLMPluginShortCircuit struct {framework/plugins/soplugin.go (1)
42-45: Misleading comment - hook IS used for dynamic plugins.The comment on line 42 states "is not used for dynamic plugins", but the method body clearly delegates to
dp.preHook. This appears to be a copy-paste artifact that wasn't fully addressed.📝 Proposed fix
-// PreLLMHook is not used for dynamic plugins +// PreLLMHook executes the pre-LLM hook for this dynamic plugin func (dp *DynamicLLMPlugin) PreLLMHook(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) (*schemas.BifrostRequest, *schemas.LLMPluginShortCircuit, error) { return dp.preHook(ctx, req) }transports/bifrost-http/lib/config.go (2)
2115-2121: Rebuild caches still susceptible to mixed snapshots under concurrent updates.
rebuildPluginCaches()calls three separate rebuild methods, each callingc.BasePlugins.Load()independently. Under concurrent plugin updates, each cache could be derived from a differentBasePluginssnapshot.Proposed single-snapshot fix
func (c *Config) rebuildPluginCaches() { - c.rebuildLLMPlugins() - c.rebuildMCPPlugins() - c.rebuildHTTPTransportPlugins() + basePlugins := c.BasePlugins.Load() + + var llmPlugins []schemas.LLMPlugin + var mcpPlugins []schemas.MCPPlugin + var httpTransportPlugins []schemas.HTTPTransportPlugin + + if basePlugins != nil { + for _, p := range *basePlugins { + if llmPlugin, ok := p.(schemas.LLMPlugin); ok { + llmPlugins = append(llmPlugins, llmPlugin) + } + if mcpPlugin, ok := p.(schemas.MCPPlugin); ok { + mcpPlugins = append(mcpPlugins, mcpPlugin) + } + if httpPlugin, ok := p.(schemas.HTTPTransportPlugin); ok { + httpTransportPlugins = append(httpTransportPlugins, httpPlugin) + } + } + } + + c.LLMPlugins.Store(&llmPlugins) + c.MCPPlugins.Store(&mcpPlugins) + c.HTTPTransportPlugins.Store(&httpTransportPlugins) }
2123-2155: Add validation for empty plugin names before adding.
AddLoadedPlugindoesn't validate thatplugin.GetName()returns a non-empty string. Empty names could cause issues with plugin lookup and removal.Proposed fix
func (c *Config) AddLoadedPlugin(plugin schemas.BasePlugin, _ schemas.PluginType) error { + if plugin.GetName() == "" { + return fmt.Errorf("plugin name cannot be empty") + } for { oldPlugins := c.BasePlugins.Load()
🧹 Nitpick comments (5)
docs/plugins/getting-started.mdx (1)
86-87: Hook execution step uses version-specific terminology without version context.Line 86 references
PreLLMHook()andPostLLMHook()but this section (Plugin Lifecycle) appears outside the version tabs. Consider either wrapping this in version tabs or using a generic term like "pre/post hooks" with a note about version-specific names.Proposed fix
1. **Load** - Bifrost loads the `.so` file using Go's `plugin.Open()` 2. **Initialize** - Calls `Init()` with configuration from `config.json` -3. **Hook Execution** - Calls `PreLLMHook()` and `PostLLMHook()` for each request +3. **Hook Execution** - Calls pre and post hooks for each request (v1.4.x+: `PreLLMHook()`/`PostLLMHook()`, v1.3.x: `PreHook()`/`PostHook()`) 4. **Cleanup** - Calls `Cleanup()` when Bifrost shuts downplugins/maxim/main.go (1)
399-428: Preferbifrost.Ptrfor string pointers.Use the repo-wide pointer helper for consistency when setting
SessionIdandName.Based on learnings, please prefer `bifrost.Ptr(...)` for pointer creation.♻️ Proposed change
if sessionID != "" { - traceConfig.SessionId = &sessionID + traceConfig.SessionId = bifrost.Ptr(sessionID) } @@ if generationName != "" { - generationConfig.Name = &generationName + generationConfig.Name = bifrost.Ptr(generationName) }plugins/mocker/main.go (2)
491-497: Remove redundantEnabledcheck inside plugin hooks.Disabled plugins aren’t added to the chain, so this branch is unnecessary and adds dead logic.
Based on learnings, rely on the loader to exclude disabled plugins.♻️ Proposed cleanup
- // Skip processing if plugin is disabled - if !p.config.Enabled { - return req, nil, nil - }
801-808: Preferbifrost.Ptr(...)for pointer literals.This matches the repo’s pointer-creation convention. Based on learnings, use
bifrost.Ptr()instead of&value.♻️ Proposed cleanup
- var finishReason *string - if content.FinishReason != nil { - finishReason = content.FinishReason - } else { - // Use a static string to avoid allocation - static := "stop" - finishReason = &static - } + finishReason := content.FinishReason + if finishReason == nil { + finishReason = bifrost.Ptr("stop") + }- finishReason := "stop" return req, &schemas.LLMPluginShortCircuit{ Response: &schemas.BifrostResponse{ ChatResponse: &schemas.BifrostChatResponse{ @@ - FinishReason: &finishReason, + FinishReason: bifrost.Ptr("stop"), }, }, }, }, nilAlso applies to: 1011-1033
core/bifrost.go (1)
2964-2964: Potential inconsistency: plugin count should use pipeline snapshot.The
pluginCountis obtained from a fresh atomic load, butpipeline.llmPlugins(set at pipeline creation) is whatRunPostHooksiterates over. If plugins are modified between pipeline creation and this point:
- Plugins removed →
pluginCountis smaller, some post-hooks may be skipped- Plugins added →
pluginCountis larger, gets clamped (safe but semantically inconsistent)Consider using the pipeline's snapshot for consistency:
Suggested fix
- pluginCount := len(*bifrost.llmPlugins.Load()) + pluginCount := len(pipeline.llmPlugins)The same pattern appears at lines 3165 and 3420 and should be updated similarly.
| // If pre-hooks modified the request, extract updated tool name and args | ||
| if preReq != nil && preReq.ChatAssistantMessageToolCall != nil { | ||
| toolCall = *preReq.ChatAssistantMessageToolCall | ||
| if toolCall.Function.Arguments != "" { | ||
| // Re-parse arguments if they were modified | ||
| if err := sonic.Unmarshal([]byte(toolCall.Function.Arguments), &args); err != nil { | ||
| logger.Warn(fmt.Sprintf("%s Failed to parse modified tool arguments, using original: %v", CodeModeLogPrefix, err)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // ==================== EXECUTE TOOL ==================== | ||
|
|
||
| // Capture start time for latency calculation | ||
| startTime := time.Now() | ||
|
|
||
| // Derive tool name from originalToolName (ignore pre-hook modifications to tool name) | ||
| // Pre-hooks should not modify which tool gets called, only arguments | ||
| toolNameToCall := originalToolName | ||
|
|
||
| // Call the tool via MCP client | ||
| callRequest := mcp.CallToolRequest{ | ||
| Request: mcp.Request{ | ||
| Method: string(mcp.MethodToolsCall), | ||
| }, | ||
| Params: mcp.CallToolParams{ | ||
| Name: toolNameToCall, | ||
| Arguments: args, | ||
| }, | ||
| } | ||
|
|
||
| // Create timeout context | ||
| toolExecutionTimeout := m.toolExecutionTimeout.Load().(time.Duration) | ||
| toolCtx, cancel := context.WithTimeout(nestedCtx, toolExecutionTimeout) | ||
| defer cancel() | ||
|
|
||
| toolResponse, callErr := client.Conn.CallTool(toolCtx, callRequest) | ||
|
|
||
| // Calculate latency | ||
| latency := time.Since(startTime).Milliseconds() | ||
|
|
||
| // ==================== PREPARE RESPONSE FOR POST-HOOKS ==================== | ||
|
|
||
| var mcpResp *schemas.BifrostMCPResponse | ||
| var bifrostErr *schemas.BifrostError | ||
|
|
||
| if callErr != nil { | ||
| logger.Debug(fmt.Sprintf("%s Tool call failed: %s.%s - %v", CodeModeLogPrefix, clientName, toolName, callErr)) | ||
| appendLog(fmt.Sprintf("[TOOL] %s.%s error: %v", clientName, toolName, callErr)) | ||
| bifrostErr = &schemas.BifrostError{ | ||
| IsBifrostError: false, | ||
| Error: &schemas.ErrorField{ | ||
| Message: fmt.Sprintf("tool call failed for %s.%s: %v", clientName, toolName, callErr), | ||
| }, | ||
| } | ||
| } else { | ||
| // Extract result | ||
| rawResult := extractTextFromMCPResponse(toolResponse, toolName) | ||
|
|
||
| // Check if this is an error result (from NewToolResultError) | ||
| // Error results start with "Error: " prefix | ||
| if after, ok := strings.CutPrefix(rawResult, "Error: "); ok { | ||
| errorMsg := after | ||
| logger.Debug(fmt.Sprintf("%s Tool returned error result: %s.%s - %s", CodeModeLogPrefix, clientName, toolName, errorMsg)) | ||
| appendLog(fmt.Sprintf("[TOOL] %s.%s error result: %s", clientName, toolName, errorMsg)) | ||
| bifrostErr = &schemas.BifrostError{ | ||
| IsBifrostError: false, | ||
| Error: &schemas.ErrorField{ | ||
| Message: errorMsg, | ||
| }, | ||
| } | ||
| } else { | ||
| // Success case - create response | ||
| mcpResp = &schemas.BifrostMCPResponse{ | ||
| ChatMessage: createToolResponseMessage(toolCall, rawResult), | ||
| ExtraFields: schemas.BifrostMCPResponseExtraFields{ | ||
| ToolName: *toolCall.Function.Name, | ||
| Latency: latency, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Reset tool name after pre-hooks to keep metadata consistent.
You ignore pre-hook tool-name mutations for execution (good), but toolCall.Function.Name is still used in response metadata (ExtraFields.ToolName). If a plugin mutates the name, logs/metadata may not match the executed tool.
🛠️ Suggested fix
if preReq != nil && preReq.ChatAssistantMessageToolCall != nil {
toolCall = *preReq.ChatAssistantMessageToolCall
+ // Keep metadata aligned with the executed tool (ignore pre-hook name mutations)
+ toolCall.Function.Name = schemas.Ptr(toolName)
if toolCall.Function.Arguments != "" {
// Re-parse arguments if they were modified
if err := sonic.Unmarshal([]byte(toolCall.Function.Arguments), &args); err != nil {
logger.Warn(fmt.Sprintf("%s Failed to parse modified tool arguments, using original: %v", CodeModeLogPrefix, err))
}
}
}🤖 Prompt for AI Agents
In `@core/mcp/codemodeexecutecode.go` around lines 866 - 945, The response
metadata can reflect a pre-hook mutated tool name because ExtraFields.ToolName
uses *toolCall.Function.Name; ensure the reported tool name matches what was
actually executed by using the execution variable (toolNameToCall or
originalToolName) instead of the possibly modified toolCall.Function.Name.
Update the construction of mcpResp (and any ExtraFields or log/metadata fields
that reference the tool name) to set ToolName = toolNameToCall and keep
createToolResponseMessage using toolCall for arguments/response content if
needed, so execution and metadata remain consistent.
| func PreLLMHook(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) (*schemas.BifrostRequest, *schemas.PluginShortCircuit, error) { | ||
| value1 := ctx.Value(schemas.BifrostContextKey("hello-world-plugin-transport-pre-hook")) | ||
| fmt.Println("value1:", value1) | ||
| ctx.SetValue(schemas.BifrostContextKey("hello-world-plugin-pre-hook"), "pre-hook-value") | ||
| fmt.Println("PreHook called") | ||
| fmt.Println("PreLLMHook called") | ||
| return req, nil, nil |
There was a problem hiding this comment.
Fix PreLLMHook short-circuit type to match loader expectations.
LoadDynamicLLMPlugin type-asserts PreLLMHook to return *schemas.LLMPluginShortCircuit. Using *schemas.PluginShortCircuit will fail the lookup and prevent the plugin from loading.
🐛 Suggested fix
-func PreLLMHook(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) (*schemas.BifrostRequest, *schemas.PluginShortCircuit, error) {
+func PreLLMHook(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) (*schemas.BifrostRequest, *schemas.LLMPluginShortCircuit, error) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func PreLLMHook(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) (*schemas.BifrostRequest, *schemas.PluginShortCircuit, error) { | |
| value1 := ctx.Value(schemas.BifrostContextKey("hello-world-plugin-transport-pre-hook")) | |
| fmt.Println("value1:", value1) | |
| ctx.SetValue(schemas.BifrostContextKey("hello-world-plugin-pre-hook"), "pre-hook-value") | |
| fmt.Println("PreHook called") | |
| fmt.Println("PreLLMHook called") | |
| return req, nil, nil | |
| func PreLLMHook(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) (*schemas.BifrostRequest, *schemas.LLMPluginShortCircuit, error) { | |
| value1 := ctx.Value(schemas.BifrostContextKey("hello-world-plugin-transport-pre-hook")) | |
| fmt.Println("value1:", value1) | |
| ctx.SetValue(schemas.BifrostContextKey("hello-world-plugin-pre-hook"), "pre-hook-value") | |
| fmt.Println("PreLLMHook called") | |
| return req, nil, nil |
🤖 Prompt for AI Agents
In `@examples/plugins/hello-world/main.go` around lines 38 - 43, The PreLLMHook
function currently returns *schemas.PluginShortCircuit which mismatches the
loader's expected type; change its return type to *schemas.LLMPluginShortCircuit
so LoadDynamicLLMPlugin's type-assertion succeeds, update any variable
names/return values accordingly in PreLLMHook, and ensure the function signature
exactly matches the expected signature used by LoadDynamicLLMPlugin (ctx
*schemas.BifrostContext, req *schemas.BifrostRequest) (*schemas.BifrostRequest,
*schemas.LLMPluginShortCircuit, error).
| // Convert context | ||
| bifrostCtx, cancel := lib.ConvertToBifrostContext(ctx, false, h.store.GetHeaderFilterConfig()) | ||
| defer cancel() // Ensure cleanup on function exit | ||
| if bifrostCtx == nil { | ||
| SendError(ctx, fasthttp.StatusInternalServerError, "Failed to convert context") | ||
| return | ||
| } |
There was a problem hiding this comment.
Avoid defer cancel() before confirming bifrostCtx (and cancel) is valid.
ConvertToBifrostContext can return a nil context (and no cancel). Deferring cancel() before the nil check risks a panic.
🔧 Proposed fix (apply in both handlers)
- bifrostCtx, cancel := lib.ConvertToBifrostContext(ctx, false, h.store.GetHeaderFilterConfig())
- defer cancel() // Ensure cleanup on function exit
- if bifrostCtx == nil {
+ bifrostCtx, cancel := lib.ConvertToBifrostContext(ctx, false, h.store.GetHeaderFilterConfig())
+ if bifrostCtx == nil {
+ if cancel != nil {
+ cancel()
+ }
SendError(ctx, fasthttp.StatusInternalServerError, "Failed to convert context")
return
}
+ defer cancel() // Ensure cleanup on function exitAlso applies to: 95-101
🤖 Prompt for AI Agents
In `@transports/bifrost-http/handlers/mcpinference.go` around lines 62 - 68,
ConvertToBifrostContext may return a nil bifrostCtx (and no cancel), so calling
defer cancel() immediately can panic; update the handlers to check bifrostCtx
for nil first, call cancel() if non-nil before returning on error, and only
defer cancel() after confirming bifrostCtx (and cancel) are valid. Specifically,
in the code using ConvertToBifrostContext, move the nil-check for bifrostCtx
ahead of defer, call cancel() conditionally when returning (if cancel != nil),
and then place defer cancel() after the successful nil-check; apply the same
change in both handler locations that use ConvertToBifrostContext (references:
ConvertToBifrostContext, bifrostCtx, cancel, SendError).
1 participant
Summary
Briefly explain the purpose of this PR and the problem it solves.
Changes
Type of change
Affected areas
How to test
Describe the steps to validate this change. Include commands and expected outcomes.
If adding new configs or environment variables, document them here.
Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Link related issues and discussions. Example: Closes #123
Security considerations
Note any security implications (auth, secrets, PII, sandboxing, etc.).
Checklist
docs/contributing/README.mdand followed the guidelines