Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 2 minutes and 6 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (50)
📝 WalkthroughWalkthroughDowngrade Go toolchain pins from 1.26.2 → 1.26.1 across CI/go.mod/Docker; add EnsureStreamFinalizerCalled and invoke it in many provider streaming goroutines; make stream forwarding ctx-aware to drain on cancel; add Anthropic/Bedrock tool validation/conversion; expand pprof allocation data and UI leak analysis. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Bifrost as Bifrost Core
participant Provider
participant Upstream as UpstreamProducer
Note over Client,Bifrost: Request carries ctx
Client->>Bifrost: Request (ctx)
Bifrost->>Provider: Start stream goroutine
Provider->>Upstream: Subscribe / request stream
Upstream->>Bifrost: send stream chunk
alt Client disconnects (before change)
Client-xBifrost: ctx.Done()
Bifrost->>Bifrost: blocking send to outputStream
Upstream->>Upstream: blocked producing
else Client disconnects (after change)
Client-xBifrost: ctx.Done()
Bifrost->>Bifrost: select -> sees ctx.Done()
Bifrost->>Upstream: drain upstream stream to unblock producer
Bifrost->>Bifrost: return early (defer EnsureStreamFinalizerCalled runs)
end
sequenceDiagram
participant StreamG as Provider Stream Goroutine
participant Utils as providerUtils
participant Finalizer as FinalizerFunc
participant Context as ctx
StreamG->>Utils: SetupStreamCancellation(ctx, resp)
StreamG->>Utils: defer EnsureStreamFinalizerCalled(ctx)
StreamG->>StreamG: read/process stream
alt Normal completion
StreamG->>StreamG: loop finishes, goroutine exits
else Error or ctx cancelled
StreamG-xContext: exit early
end
StreamG->>Finalizer: deferred EnsureStreamFinalizerCalled invokes finalizer(ctx) with panic recovery
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
|
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
This was referenced Apr 17, 2026
Contributor
Confidence Score: 5/5Safe to merge — all go.mod files and the Dockerfile are now consistently at 1.26.1, and the streaming safety improvements are correct and well-tested. All previous concerns about version inconsistency (transports/go.mod, framework/go.mod, plugin go.mods) have been resolved in this PR. The only finding is a P2 test quality issue in stream_test.go. The streaming improvements are sound: EnsureStreamFinalizerCalled is properly deferred across all providers, and the ctx.Done() drain path in CheckFirstStreamChunkForError is correctly implemented and tested. core/providers/utils/stream_test.go — minor test quality fix Important Files Changed
Reviews (14): Last reviewed commit: "move back go to 1.26.1" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
core/providers/anthropic/anthropic.go (1)
2783-2817:⚠️ Potential issue | 🟠 MajorRemove direct finalizer call on EOF to prevent double execution.
Lines 2815–2817 call the finalizer directly, but line 2783 already defers
EnsureStreamFinalizerCalled(ctx). SinceEnsureStreamFinalizerCalledlacks idempotency protection and the finalizer remains in context, this will invoke the finalizer twice: once at line 2816 and again via the defer when the goroutine exits. Remove the direct finalizer invocation at lines 2815–2817 and rely solely on the defer.Current code (lines 2810–2817)
postHookRunner(ctx, finalResp, nil) if finalizer, ok := ctx.Value(schemas.BifrostContextKeyPostHookSpanFinalizer).(func(context.Context)); ok && finalizer != nil { finalizer(ctx) } returnShould become:
postHookRunner(ctx, finalResp, nil) return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/anthropic.go` around lines 2783 - 2817, The code calls the post-hook finalizer twice because EnsureStreamFinalizerCalled(ctx) is deferred, then the goroutine explicitly retrieves and invokes the function stored under schemas.BifrostContextKeyPostHookSpanFinalizer after postHookRunner; remove the direct invocation (the block that fetches and calls finalizer(ctx)) and leave only postHookRunner(ctx, finalResp, nil) followed by return so the deferred EnsureStreamFinalizerCalled(ctx) handles finalization; references: EnsureStreamFinalizerCalled, schemas.BifrostContextKeyPostHookSpanFinalizer, postHookRunner.core/providers/mistral/mistral.go (1)
584-591:⚠️ Potential issue | 🟠 MajorMove the stream finalizer into the outer defer.
This defer currently runs before the outer
HandleStreamCancellation/HandleStreamTimeoutblock because of Go’s LIFO defer order. On canceled or timed-out streams, the finalizer can fire before the terminal stream event is processed.Suggested fix
defer func() { if ctx.Err() == context.Canceled { providerUtils.HandleStreamCancellation(ctx, postHookRunner, responseChan, providerName, request.Model, schemas.TranscriptionStreamRequest, provider.logger) } else if ctx.Err() == context.DeadlineExceeded { providerUtils.HandleStreamTimeout(ctx, postHookRunner, responseChan, providerName, request.Model, schemas.TranscriptionStreamRequest, provider.logger) } + providerUtils.EnsureStreamFinalizerCalled(ctx) close(responseChan) }() stopCancellation := providerUtils.SetupStreamCancellation(ctx, resp.BodyStream(), provider.logger) defer stopCancellation() - defer providerUtils.EnsureStreamFinalizerCalled(ctx)Also applies to: 603-605
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/mistral/mistral.go` around lines 584 - 591, The inner defer currently calls close(responseChan) before the outer stream-finalizer runs due to Go's LIFO defer order; move the channel close into the outer defer so HandleStreamCancellation / HandleStreamTimeout run before the finalizer. Specifically, in the defer that checks ctx.Err() and invokes providerUtils.HandleStreamCancellation and providerUtils.HandleStreamTimeout (using postHookRunner, providerName, request.Model, schemas.TranscriptionStreamRequest, provider.logger), ensure you do NOT close responseChan there; instead perform close(responseChan) only in the outer-most defer after those handler calls so the terminal stream event is processed prior to closing responseChan; apply the same change for the similar defer that references the same symbols.core/providers/gemini/gemini.go (1)
483-491:⚠️ Potential issue | 🟠 MajorRun
EnsureStreamFinalizerCalledafter the cancellation/timeout defer.Because defers execute LIFO, this new defer runs before the outer deferred block that calls
HandleStreamCancellation/HandleStreamTimeout. On disconnects and deadlines, that can finalize the stream span/hooks before the terminal cancellation/timeout event is emitted.Suggested fix
defer func() { if ctx.Err() == context.Canceled { providerUtils.HandleStreamCancellation(ctx, postHookRunner, responseChan, providerName, model, requestType, logger) } else if ctx.Err() == context.DeadlineExceeded { providerUtils.HandleStreamTimeout(ctx, postHookRunner, responseChan, providerName, model, requestType, logger) } + providerUtils.EnsureStreamFinalizerCalled(ctx) close(responseChan) }() stopCancellation := providerUtils.SetupStreamCancellation(ctx, resp.BodyStream(), logger) defer stopCancellation() - defer providerUtils.EnsureStreamFinalizerCalled(ctx)Also applies to: 515-517, 1021-1028, 1059-1061, 1558-1565, 1579-1581, 1877-1884, 1896-1898, 4564-4575
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/gemini/gemini.go` around lines 483 - 491, The EnsureStreamFinalizerCalled defer is currently registered after the outer defer that calls providerUtils.HandleStreamCancellation / HandleStreamTimeout, causing it to run before those handlers due to LIFO defer ordering; move the defer that calls EnsureStreamFinalizerCalled so it is registered (appears) before the surrounding defer that handles cancellation/timeout in the same goroutine (so the cancellation/timeout defer runs first and EnsureStreamFinalizerCalled runs afterwards), and apply this same reorder to the other similar sites (around lines referencing EnsureStreamFinalizerCalled and the cancellation/timeout handling at the other listed locations).
🧹 Nitpick comments (2)
core/providers/utils/stream_test.go (1)
220-223: Bound the post-cancel source send to avoid potential test hangs.Line 220 can block indefinitely in a regression scenario before the
drainDonetimeout is reached. Add a timeout around that send so the test fails fast.♻️ Proposed tweak
- src <- &schemas.BifrostStreamChunk{ - BifrostChatResponse: &schemas.BifrostChatResponse{ID: "3"}, - } + select { + case src <- &schemas.BifrostStreamChunk{ + BifrostChatResponse: &schemas.BifrostChatResponse{ID: "3"}, + }: + case <-time.After(time.Second): + t.Fatal("timed out while sending post-cancel chunk to source") + } close(src)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/utils/stream_test.go` around lines 220 - 223, The test can hang because the send to the channel variable src of &schemas.BifrostStreamChunk{BifrostChatResponse: &schemas.BifrostChatResponse{ID: "3"}} may block; wrap that send in a select that includes a timeout (e.g. case src <- chunk: and case <-time.After(100*time.Millisecond): t.Fatalf("timed out sending post-cancel chunk")) or a non-blocking default case so the test fails fast instead of waiting for drainDone; update the send before close(src) to use this select pattern.core/providers/openai/openai.go (1)
7230-7230: Remove the now-redundant direct finalizer call inPassthroughStream.This function still invokes the post-hook span finalizer directly on Lines 7262-7264. With the new deferred helper in place, that becomes a double invocation on the happy path, and the direct call bypasses
EnsureStreamFinalizerCalled's panic recovery. Let the defer own finalization here, or route the EOF path through the helper too.♻️ Suggested cleanup
- if finalizer, ok := ctx.Value(schemas.BifrostContextKeyPostHookSpanFinalizer).(func(context.Context)); ok && finalizer != nil { - finalizer(ctx) - } return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/openai/openai.go` at line 7230, The PassthroughStream function currently calls the post-hook span finalizer directly in its EOF/happy-path branch while also deferring providerUtils.EnsureStreamFinalizerCalled(ctx), causing double-finalization and bypassing the helper's panic recovery; remove the direct call to the post-hook finalizer inside PassthroughStream so the deferred EnsureStreamFinalizerCalled(ctx) is the sole finalizer (or alternatively route the EOF path through providerUtils.EnsureStreamFinalizerCalled), keeping references to PassthroughStream and providerUtils.EnsureStreamFinalizerCalled to locate where to delete the redundant direct finalizer invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/e2e-tests.yml:
- Line 35: The CI workflow pins go-version: "1.26.1" but several modules
(framework, transports, plugins/telemetry, plugins/maxim, plugins/mocker,
plugins/semanticcache, plugins/otel, plugins/jsonparser, plugins/logging,
plugins/governance, plugins/litellmcompat) declare Go 1.26.2 in their go.mod
files, causing a version mismatch; fix by making the versions consistent—either
update the workflow go-version value to "1.26.2" in
.github/workflows/e2e-tests.yml or change the go directive in each module's
go.mod to "1.26.1" (choose one approach and apply it across all listed modules
to ensure parity).
In `@core/bifrost.go`:
- Around line 5227-5236: The streaming cleanup double-release occurs because
CheckFirstStreamChunkForError's error path calls
bifrost.releasePluginPipeline(pipeline) directly while provider goroutines also
invoke postHookSpanFinalizer (protected by sync.Once), causing the pipeline to
be released twice; to fix, remove the direct call to
bifrost.releasePluginPipeline(pipeline) from the first-chunk error path and
instead call the already-defined postHookSpanFinalizer(ctx) (or otherwise route
cleanup through finalizer) so that FinalizeStreamingPostHookSpans(ctx) and
releasePluginPipeline(pipeline) always run under the same sync.Once protection.
- Around line 4692-4701: The goroutine that drains shortCircuit.Stream and sends
to outputStream still calls pipelinePostHookRunner and thus uses the
PluginPipeline after tryStreamRequest returns; since the outer function defers
bifrost.releasePluginPipeline(pipeline) the pipeline can be returned to the pool
while this goroutine is still running. Fix by extending the pipeline lifetime
until the goroutine finishes: ensure you do not call
bifrost.releasePluginPipeline(pipeline) before the goroutine exits (e.g. add a
sync mechanism such as a WaitGroup or channel that the goroutine signals before
release), or move the pipelinePostHookRunner invocation out of the goroutine so
the pipeline is not accessed after release; adjust
tryStreamRequest/outputStream/shortCircuit.Stream handling accordingly to wait
for the goroutine before releasing the pipeline.
---
Outside diff comments:
In `@core/providers/anthropic/anthropic.go`:
- Around line 2783-2817: The code calls the post-hook finalizer twice because
EnsureStreamFinalizerCalled(ctx) is deferred, then the goroutine explicitly
retrieves and invokes the function stored under
schemas.BifrostContextKeyPostHookSpanFinalizer after postHookRunner; remove the
direct invocation (the block that fetches and calls finalizer(ctx)) and leave
only postHookRunner(ctx, finalResp, nil) followed by return so the deferred
EnsureStreamFinalizerCalled(ctx) handles finalization; references:
EnsureStreamFinalizerCalled, schemas.BifrostContextKeyPostHookSpanFinalizer,
postHookRunner.
In `@core/providers/gemini/gemini.go`:
- Around line 483-491: The EnsureStreamFinalizerCalled defer is currently
registered after the outer defer that calls
providerUtils.HandleStreamCancellation / HandleStreamTimeout, causing it to run
before those handlers due to LIFO defer ordering; move the defer that calls
EnsureStreamFinalizerCalled so it is registered (appears) before the surrounding
defer that handles cancellation/timeout in the same goroutine (so the
cancellation/timeout defer runs first and EnsureStreamFinalizerCalled runs
afterwards), and apply this same reorder to the other similar sites (around
lines referencing EnsureStreamFinalizerCalled and the cancellation/timeout
handling at the other listed locations).
In `@core/providers/mistral/mistral.go`:
- Around line 584-591: The inner defer currently calls close(responseChan)
before the outer stream-finalizer runs due to Go's LIFO defer order; move the
channel close into the outer defer so HandleStreamCancellation /
HandleStreamTimeout run before the finalizer. Specifically, in the defer that
checks ctx.Err() and invokes providerUtils.HandleStreamCancellation and
providerUtils.HandleStreamTimeout (using postHookRunner, providerName,
request.Model, schemas.TranscriptionStreamRequest, provider.logger), ensure you
do NOT close responseChan there; instead perform close(responseChan) only in the
outer-most defer after those handler calls so the terminal stream event is
processed prior to closing responseChan; apply the same change for the similar
defer that references the same symbols.
---
Nitpick comments:
In `@core/providers/openai/openai.go`:
- Line 7230: The PassthroughStream function currently calls the post-hook span
finalizer directly in its EOF/happy-path branch while also deferring
providerUtils.EnsureStreamFinalizerCalled(ctx), causing double-finalization and
bypassing the helper's panic recovery; remove the direct call to the post-hook
finalizer inside PassthroughStream so the deferred
EnsureStreamFinalizerCalled(ctx) is the sole finalizer (or alternatively route
the EOF path through providerUtils.EnsureStreamFinalizerCalled), keeping
references to PassthroughStream and providerUtils.EnsureStreamFinalizerCalled to
locate where to delete the redundant direct finalizer invocation.
In `@core/providers/utils/stream_test.go`:
- Around line 220-223: The test can hang because the send to the channel
variable src of &schemas.BifrostStreamChunk{BifrostChatResponse:
&schemas.BifrostChatResponse{ID: "3"}} may block; wrap that send in a select
that includes a timeout (e.g. case src <- chunk: and case
<-time.After(100*time.Millisecond): t.Fatalf("timed out sending post-cancel
chunk")) or a non-blocking default case so the test fails fast instead of
waiting for drainDone; update the send before close(src) to use this select
pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7637244-33ad-41bf-bbcc-1ca38f5c746a
📒 Files selected for processing (27)
.github/workflows/e2e-tests.yml.github/workflows/pr-tests.yml.github/workflows/release-cli.yml.github/workflows/release-pipeline.yml.github/workflows/snyk.ymlcli/go.modcore/bifrost.gocore/go.modcore/providers/anthropic/anthropic.gocore/providers/azure/azure.gocore/providers/bedrock/bedrock.gocore/providers/cohere/cohere.gocore/providers/elevenlabs/elevenlabs.gocore/providers/gemini/gemini.gocore/providers/huggingface/huggingface.gocore/providers/mistral/mistral.gocore/providers/openai/openai.gocore/providers/replicate/replicate.gocore/providers/utils/stream.gocore/providers/utils/stream_test.gocore/providers/utils/utils.gocore/providers/vertex/vertex.gocore/providers/vllm/vllm.goexamples/plugins/hello-world/go.modtests/scripts/1millogs/go.modtransports/Dockerfiletransports/Dockerfile.local
akshaydeo
force-pushed
the
04-17-move_back_go_to_1.26.1
branch
from
cab4b6f to
75be072
Compare
akshaydeo
force-pushed
the
04-17-dependabot_fixes
branch
from
efc8305 to
2009384
Compare
akshaydeo
force-pushed
the
04-17-move_back_go_to_1.26.1
branch
2 times, most recently
from
152079d to
e917237
Compare
akshaydeo
force-pushed
the
04-17-dependabot_fixes
branch
from
2009384 to
eb6c00f
Compare
akshaydeo
force-pushed
the
04-17-move_back_go_to_1.26.1
branch
from
e917237 to
c4a2480
Compare
akshaydeo
force-pushed
the
04-17-dependabot_fixes
branch
from
eb6c00f to
306f668
Compare
akshaydeo
force-pushed
the
04-17-move_back_go_to_1.26.1
branch
from
c4a2480 to
ccb44b8
Compare
Contributor
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 (1)
core/providers/openai/openai.go (1)
533-555:⚠️ Potential issue | 🔴 CriticalRemove duplicate finalizer call in PassthroughStream.
The new
defer providerUtils.EnsureStreamFinalizerCalled(ctx)at line 7230 conflicts with the explicit finalizer call at lines 7262–7263. On the EOF happy path, the finalizer fires twice: once explicitly afterpostHookRunner, then again via defer (LIFO order executes the defer first). The explicit call is redundant and should be removed.Lines 7262–7263 should be deleted:
if finalizer, ok := ctx.Value(schemas.BifrostContextKeyPostHookSpanFinalizer).(func(context.Context)); ok && finalizer != nil { finalizer(ctx) }The other 7 streaming handlers in this file (lines 533, 1093, 1698, 2320, 2762, 3211, 4474) correctly use only the defer and have no explicit finalizer calls, so they are safe. However, Vertex, Gemini, Azure, and Anthropic providers also have similar explicit finalizer calls in their PassthroughStream implementations; ensure they receive the same fix when the defer is added to avoid double-finalization there as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/openai/openai.go` around lines 533 - 555, The PassthroughStream goroutine now defers providerUtils.EnsureStreamFinalizerCalled(ctx), so remove the explicit finalizer invocation to avoid double-finalization: delete the block that looks up ctx.Value(schemas.BifrostContextKeyPostHookSpanFinalizer) and calls the finalizer (the two lines that check the finalizer func and call finalizer(ctx)) inside the goroutine after postHookRunner; leave the defer providerUtils.EnsureStreamFinalizerCalled(ctx) in place and keep postHookRunner usage as-is. Also scan other PassthroughStream implementations (Vertex/Gemini/Azure/Anthropic) for the same explicit finalizer call and remove those redundant blocks where EnsureStreamFinalizerCalled(ctx) is already deferred.
♻️ Duplicate comments (2)
core/bifrost.go (2)
5219-5257:⚠️ Potential issue | 🔴 CriticalRoute first-chunk stream setup failures through the finalizer too.
finalizerOnceonly protects callers that go throughpostHookSpanFinalizer. WhenCheckFirstStreamChunkForError(...)turns an apparently successful stream intobifrostError, the code below still falls into the directbifrost.releasePluginPipeline(pipeline)path, so provider-side deferred finalizers can still finalize/release the same pipeline a second time.💡 Suggested fix
- // Release pipeline immediately for non-streaming requests only - // For streaming, the pipeline is released in the postHookSpanFinalizer after streaming completes - // Exception: if streaming request has an error, release immediately since finalizer won't be called - if pipeline != nil && (!IsStreamRequestType(req.RequestType) || bifrostError != nil) { - bifrost.releasePluginPipeline(pipeline) - } + // Release non-streaming pipelines immediately. + // Streaming cleanup must always go through the finalizer so span finalization + // and pool release share the same sync.Once protection. + if pipeline != nil { + if IsStreamRequestType(req.RequestType) { + if bifrostError != nil { + if finalizer, ok := req.Context.Value(schemas.BifrostContextKeyPostHookSpanFinalizer).(func(context.Context)); ok { + finalizer(req.Context) + } + } + } else { + bifrost.releasePluginPipeline(pipeline) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 5219 - 5257, When a streaming request is turned into an error by CheckFirstStreamChunkForError(...), we must route the pipeline release through postHookSpanFinalizer (protected by finalizerOnce) instead of directly calling bifrost.releasePluginPipeline(pipeline) to avoid double-release. Update the release logic around releasePluginPipeline so that: if pipeline != nil and the request is non-streaming, call bifrost.releasePluginPipeline(pipeline) as before; if pipeline != nil and the request is streaming but bifrostError != nil, call postHookSpanFinalizer(req.Context) (not releasePluginPipeline) so the finalizer handles release; if streaming and no error, do nothing here because postHookSpanFinalizer will run at stream end. Ensure you reference finalizerOnce/postHookSpanFinalizer and releasePluginPipeline in the change.
4634-4705:⚠️ Potential issue | 🔴 CriticalKeep the plugin pipeline alive until the short-circuit stream goroutine exits.
pipelinePostHookRunnerstill closes overpipeline, buttryStreamRequestreturns immediately and hits the outerdefer bifrost.releasePluginPipeline(pipeline). The new cancel/drain path can keep that goroutine alive even longer, so the same pooledPluginPipelinecan be reused concurrently while post-hooks are still mutating it.💡 Suggested fix
pipeline := bifrost.getPluginPipeline() - defer bifrost.releasePluginPipeline(pipeline) + releasePipeline := func() { + bifrost.releasePluginPipeline(pipeline) + } preReq, shortCircuit, preCount := pipeline.RunLLMPreHooks(ctx, req) if shortCircuit != nil { // Handle short-circuit with response (success case) if shortCircuit.Response != nil { @@ if shortCircuit.Stream != nil { outputStream := make(chan *schemas.BifrostStreamChunk) @@ go func() { + defer releasePipeline() defer close(outputStream) @@ }() return outputStream, nil } @@ } + defer releasePipeline()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 4634 - 4705, The code currently defers bifrost.releasePluginPipeline(pipeline) immediately after getPluginPipeline(), but pipelinePostHookRunner's goroutine may outlive that defer causing the pooled PluginPipeline to be reused while post-hooks still run; remove the outer defer and instead release the pipeline explicitly in each return path: call bifrost.releasePluginPipeline(pipeline) after handling the short-circuit response case (before returning the channel from newBifrostMessageChan) and also call bifrost.releasePluginPipeline(pipeline) inside the stream goroutine when it finishes (e.g., defer bifrost.releasePluginPipeline(pipeline) at the top of the goroutine), ensuring getPluginPipeline()/releasePluginPipeline(pipeline), pipelinePostHookRunner, and the goroutine that calls pipeline.RunPostLLMHooks are the loci of the change.
🧹 Nitpick comments (3)
core/providers/anthropic/validate_chat_tools_test.go (1)
21-117: Add aCustom-tool case to this table.
ValidateChatToolsForProvideralso has a dedicatedtool.Custom != nilkeep-path, but this suite only exercises function tools and server-tool filtering. A small custom-tool row would lock in that new branch too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/validate_chat_tools_test.go` around lines 21 - 117, Add a test-case exercising the custom-tool branch so ValidateChatToolsForProvider's tool.Custom != nil path is covered: define a small custom tool (similar to fnTool/serverTool patterns, e.g. customTool := schemas.ChatTool{Custom: &schemas.CustomTool{...}}) and add a table row in validate_chat_tools_test.go that passes that customTool for a provider (e.g. schemas.Bedrock or schemas.Anthropic) with wantKeep: 1 (and no wantDropped), so the test suite asserts the custom-tool keep-path is hit.core/providers/bedrock/convert_tool_config_test.go (1)
84-100: Clarify this test’s intent with an explicit assertion.Line 99 currently makes the test pass regardless of output shape, so it doesn’t verify behavior beyond “no panic.” Consider either asserting the expected
cfgvalue or renaming the test to reflect panic-safety only.Possible tightening (assert current behavior)
func TestConvertToolConfig_KeepsBedrockSupportedServerTools(t *testing.T) { @@ cfg := convertToolConfig("global.anthropic.claude-sonnet-4-6", params) - _ = cfg // may be nil or populated; test succeeds if no panic + no invalid shape + if cfg != nil { + t.Fatalf("expected nil ToolConfig until server-tool conversion is implemented, got %+v", cfg) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/convert_tool_config_test.go` around lines 84 - 100, The test TestConvertToolConfig_KeepsBedrockSupportedServerTools currently swallows cfg with `_ = cfg` and only asserts "no panic"; update the test to explicitly assert the expected output shape from convertToolConfig for the given schemas.ChatParameters: replace the no-op with an assertion that cfg is non-nil and that its tool entries include the converted Bedrock representation for the "bash" tool (or, if current intended behavior is only panic-safety, rename the test to reflect that); reference convertToolConfig, cfg, and TestConvertToolConfig_KeepsBedrockSupportedServerTools when making the change.core/providers/utils/stream.go (1)
45-51: Make first-chunk error draining context-aware as wellAt Line 45, the drain goroutine ignores
ctx.Done(). Since the caller waits ondrainDonein the error-first-chunk path, this can stall retry/fallback cleanup if upstream doesn’t close promptly.Proposed patch
// Drain source channel to let the provider goroutine exit cleanly done := make(chan struct{}) go func() { defer close(done) - for range stream { - } + for { + select { + case <-ctx.Done(): + return + case _, ok := <-stream: + if !ok { + return + } + } + } }() return nil, done, firstChunk.BifrostError }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/utils/stream.go` around lines 45 - 51, The drain goroutine that currently just loops "for range stream" can block indefinitely because it ignores context cancellation; update the drain routine (the anonymous goroutine that creates done := make(chan struct{}) and ranges over stream) to be context-aware by replacing the simple range with a select that listens on both the stream channel and ctx.Done() (and still defers close(done)); this ensures the goroutine returns when the context is canceled so the caller waiting on drainDone (drainDone/done) won't stall during first-chunk error handling or retries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/bedrock/utils.go`:
- Around line 969-970: After calling anthropic.ValidateChatToolsForProvider and
assigning filtered, reconcile params.ToolChoice against the filtered set: if
params.ToolChoice is non-empty and not present in the filtered tools (the same
collection used to build toolConfig.tools), clear or reset params.ToolChoice
(e.g., set to empty/default) so Bedrock won't receive a toolChoice that was
stripped; apply the same fix to the other occurrence around the 1030-1035 block
to ensure both code paths behave consistently.
- Around line 969-973: The conversion loop after calling
ValidateChatToolsForProvider drops Bedrock server tools because it only
materializes tool.Function into BedrockTool and ignores server-specific fields
(ToolConfig), so when filtered contains server tools like computer_*, bash_*,
memory_* the resulting bedrockTools array lacks the necessary ToolConfig; update
the loop that iterates over filtered (the variable named filtered and the
conversion producing BedrockTool) to preserve and map the original tool's
ToolConfig (and any server-specific metadata) into the BedrockTool structure
instead of only copying tool.Function, ensuring ToolConfig is populated whenever
the incoming tool contains server/tool config data so Bedrock retains access to
supported server tools.
In `@core/providers/replicate/replicate.go`:
- Line 1427: The defer calling providerUtils.EnsureStreamFinalizerCalled(ctx) is
registered after ResponsesStream can return an empty reader (so the finalizer is
skipped); move the defer so it runs immediately after entering ResponsesStream
(i.e., place the defer before the empty-reader return branch) to ensure
EnsureStreamFinalizerCalled is always registered regardless of the early return
path; update the code around the ResponsesStream call and the early-return
handling to register the defer before any returns.
In `@core/providers/utils/utils.go`:
- Around line 1911-1919: EnsureStreamFinalizerCalled can panic if ctx is nil
because ctx.Value(...) is executed before the defer/recover is installed; move
the panic-protection to the top and guard nil context: add an immediate defer
with recovery at the start of EnsureStreamFinalizerCalled (so any panic during
ctx access is caught) and/or return early if ctx == nil before calling
ctx.Value; keep the remainder of the logic that reads
schemas.BifrostContextKeyPostHookSpanFinalizer and invokes the finalizer
unchanged.
---
Outside diff comments:
In `@core/providers/openai/openai.go`:
- Around line 533-555: The PassthroughStream goroutine now defers
providerUtils.EnsureStreamFinalizerCalled(ctx), so remove the explicit finalizer
invocation to avoid double-finalization: delete the block that looks up
ctx.Value(schemas.BifrostContextKeyPostHookSpanFinalizer) and calls the
finalizer (the two lines that check the finalizer func and call finalizer(ctx))
inside the goroutine after postHookRunner; leave the defer
providerUtils.EnsureStreamFinalizerCalled(ctx) in place and keep postHookRunner
usage as-is. Also scan other PassthroughStream implementations
(Vertex/Gemini/Azure/Anthropic) for the same explicit finalizer call and remove
those redundant blocks where EnsureStreamFinalizerCalled(ctx) is already
deferred.
---
Duplicate comments:
In `@core/bifrost.go`:
- Around line 5219-5257: When a streaming request is turned into an error by
CheckFirstStreamChunkForError(...), we must route the pipeline release through
postHookSpanFinalizer (protected by finalizerOnce) instead of directly calling
bifrost.releasePluginPipeline(pipeline) to avoid double-release. Update the
release logic around releasePluginPipeline so that: if pipeline != nil and the
request is non-streaming, call bifrost.releasePluginPipeline(pipeline) as
before; if pipeline != nil and the request is streaming but bifrostError != nil,
call postHookSpanFinalizer(req.Context) (not releasePluginPipeline) so the
finalizer handles release; if streaming and no error, do nothing here because
postHookSpanFinalizer will run at stream end. Ensure you reference
finalizerOnce/postHookSpanFinalizer and releasePluginPipeline in the change.
- Around line 4634-4705: The code currently defers
bifrost.releasePluginPipeline(pipeline) immediately after getPluginPipeline(),
but pipelinePostHookRunner's goroutine may outlive that defer causing the pooled
PluginPipeline to be reused while post-hooks still run; remove the outer defer
and instead release the pipeline explicitly in each return path: call
bifrost.releasePluginPipeline(pipeline) after handling the short-circuit
response case (before returning the channel from newBifrostMessageChan) and also
call bifrost.releasePluginPipeline(pipeline) inside the stream goroutine when it
finishes (e.g., defer bifrost.releasePluginPipeline(pipeline) at the top of the
goroutine), ensuring getPluginPipeline()/releasePluginPipeline(pipeline),
pipelinePostHookRunner, and the goroutine that calls pipeline.RunPostLLMHooks
are the loci of the change.
---
Nitpick comments:
In `@core/providers/anthropic/validate_chat_tools_test.go`:
- Around line 21-117: Add a test-case exercising the custom-tool branch so
ValidateChatToolsForProvider's tool.Custom != nil path is covered: define a
small custom tool (similar to fnTool/serverTool patterns, e.g. customTool :=
schemas.ChatTool{Custom: &schemas.CustomTool{...}}) and add a table row in
validate_chat_tools_test.go that passes that customTool for a provider (e.g.
schemas.Bedrock or schemas.Anthropic) with wantKeep: 1 (and no wantDropped), so
the test suite asserts the custom-tool keep-path is hit.
In `@core/providers/bedrock/convert_tool_config_test.go`:
- Around line 84-100: The test
TestConvertToolConfig_KeepsBedrockSupportedServerTools currently swallows cfg
with `_ = cfg` and only asserts "no panic"; update the test to explicitly assert
the expected output shape from convertToolConfig for the given
schemas.ChatParameters: replace the no-op with an assertion that cfg is non-nil
and that its tool entries include the converted Bedrock representation for the
"bash" tool (or, if current intended behavior is only panic-safety, rename the
test to reflect that); reference convertToolConfig, cfg, and
TestConvertToolConfig_KeepsBedrockSupportedServerTools when making the change.
In `@core/providers/utils/stream.go`:
- Around line 45-51: The drain goroutine that currently just loops "for range
stream" can block indefinitely because it ignores context cancellation; update
the drain routine (the anonymous goroutine that creates done := make(chan
struct{}) and ranges over stream) to be context-aware by replacing the simple
range with a select that listens on both the stream channel and ctx.Done() (and
still defers close(done)); this ensures the goroutine returns when the context
is canceled so the caller waiting on drainDone (drainDone/done) won't stall
during first-chunk error handling or retries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9023a854-2e9f-4a85-a699-177aea1d2d98
📒 Files selected for processing (46)
.github/workflows/e2e-tests.yml.github/workflows/pr-tests.yml.github/workflows/release-cli.yml.github/workflows/release-pipeline.yml.github/workflows/snyk.ymlcli/go.modcore/bifrost.gocore/go.modcore/providers/anthropic/anthropic.gocore/providers/anthropic/chat.gocore/providers/anthropic/utils.gocore/providers/anthropic/validate_chat_tools_test.gocore/providers/azure/azure.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/convert_tool_config_test.gocore/providers/bedrock/utils.gocore/providers/cohere/cohere.gocore/providers/elevenlabs/elevenlabs.gocore/providers/gemini/gemini.gocore/providers/huggingface/huggingface.gocore/providers/mistral/mistral.gocore/providers/openai/openai.gocore/providers/replicate/replicate.gocore/providers/utils/stream.gocore/providers/utils/stream_test.gocore/providers/utils/utils.gocore/providers/vertex/vertex.gocore/providers/vllm/vllm.goexamples/plugins/hello-world/go.modframework/go.modplugins/governance/go.modplugins/jsonparser/go.modplugins/litellmcompat/go.modplugins/logging/go.modplugins/maxim/go.modplugins/mocker/go.modplugins/otel/go.modplugins/semanticcache/go.modplugins/telemetry/go.modtests/scripts/1millogs/go.modtransports/Dockerfiletransports/Dockerfile.localtransports/bifrost-http/handlers/devpprof.gotransports/go.modui/app/pprof/page.tsxui/lib/store/apis/devApi.ts
✅ Files skipped from review due to trivial changes (27)
- cli/go.mod
- core/go.mod
- plugins/logging/go.mod
- .github/workflows/snyk.yml
- framework/go.mod
- plugins/jsonparser/go.mod
- plugins/mocker/go.mod
- plugins/telemetry/go.mod
- examples/plugins/hello-world/go.mod
- transports/go.mod
- tests/scripts/1millogs/go.mod
- plugins/otel/go.mod
- plugins/semanticcache/go.mod
- .github/workflows/pr-tests.yml
- plugins/governance/go.mod
- plugins/litellmcompat/go.mod
- .github/workflows/release-pipeline.yml
- .github/workflows/e2e-tests.yml
- core/providers/elevenlabs/elevenlabs.go
- core/providers/huggingface/huggingface.go
- transports/Dockerfile.local
- plugins/maxim/go.mod
- transports/Dockerfile
- core/providers/anthropic/anthropic.go
- .github/workflows/release-cli.yml
- core/providers/utils/stream_test.go
- core/providers/azure/azure.go
🚧 Files skipped from review as they are similar to previous changes (3)
- core/providers/vllm/vllm.go
- core/providers/vertex/vertex.go
- core/providers/gemini/gemini.go
akshaydeo
force-pushed
the
04-17-move_back_go_to_1.26.1
branch
from
ccb44b8 to
b10c2ca
Compare
akshaydeo
force-pushed
the
04-17-dependabot_fixes
branch
from
306f668 to
8dba7a5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
core/providers/bedrock/utils.go (1)
1130-1134: Consider extracting validation to avoid duplicate filtering.Both
collectBedrockServerTools(line 1134) andconvertToolConfig(line 1203) callValidateChatToolsForProviderwith identical inputs when invoked fromconvertChatParameters. This duplication is harmless but could be optimized by filtering once in the caller and passing the result to both helpers.♻️ Optional: Single-pass filtering
// In convertChatParameters, filter once and pass to both: +filtered, _ := anthropic.ValidateChatToolsForProvider(bifrostReq.Params.Tools, schemas.Bedrock) + -if toolConfig := convertToolConfig(bifrostReq.Model, bifrostReq.Params); toolConfig != nil { +if toolConfig := convertToolConfigFromFiltered(bifrostReq.Model, bifrostReq.Params, filtered); toolConfig != nil { bedrockReq.ToolConfig = toolConfig } -if serverTools, betaHeaders := collectBedrockServerTools(bifrostReq.Params); len(serverTools) > 0 { +if serverTools, betaHeaders := collectBedrockServerToolsFromFiltered(filtered); len(serverTools) > 0 {Also applies to: 1202-1206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/utils.go` around lines 1130 - 1134, The two helpers collectBedrockServerTools and convertToolConfig duplicate the same call to anthropic.ValidateChatToolsForProvider; to avoid double-filtering, move the ValidateChatToolsForProvider call up into convertChatParameters (call it once), then change collectBedrockServerTools and convertToolConfig to accept the already-filtered tools (or nil) instead of re-reading params.Tools, and remove their internal ValidateChatToolsForProvider calls; update convertChatParameters to pass the filtered result into both helpers so filtering is done only once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/openai/openai.go`:
- Line 555: EnsureStreamFinalizerCalled is being invoked twice and in the wrong
order: PassthroughStream explicitly calls EnsureStreamFinalizerCalled on EOF and
a defer at the goroutine top also calls it, and the finalizer runs before outer
cleanup (cancel/timeout) across streaming goroutines; fix by making the
finalizer idempotent and ordering-safe—add a sync.Once (or equivalent atomic
guard) inside providerUtils.EnsureStreamFinalizerCalled so multiple invocations
(from PassthroughStream EOF path and the defer) are no-ops after the first, and
optionally move the defer to execute after the outer cleanup if you prefer
single-call semantics; update EnsureStreamFinalizerCalled implementation and
rerun make test-core to verify across providers (references:
EnsureStreamFinalizerCalled, PassthroughStream, providerUtils).
In `@core/providers/replicate/replicate.go`:
- Around line 612-613: Multiple stream handlers (TextCompletionStream,
ChatCompletionStream, ImageGenerationStream, ImageEditStream) call defer
providerUtils.EnsureStreamFinalizerCalled(ctx) too late so it runs before outer
cancellation/timeout defers; move each EnsureStreamFinalizerCalled(ctx) defer to
be the outermost defer in the function (immediately after entering the function,
like ResponsesStream does) so it executes last, i.e., place the defer
providerUtils.EnsureStreamFinalizerCalled(ctx) before any other defer calls
(cancellation/timeouts/response finalizers) in the named functions to preserve
correct finalization order.
In `@core/providers/vllm/vllm.go`:
- Line 548: The defer calling providerUtils.EnsureStreamFinalizerCalled(ctx) is
registered too late and therefore runs before outer defers like
stopCancellation(), so move the defer
providerUtils.EnsureStreamFinalizerCalled(ctx) to execute earlier than other
defers: register it immediately after creating the stream/context (before any
later defers such as stopCancellation() or timeout cancellation), ensuring
EnsureStreamFinalizerCalled runs last (LIFO) after the cancellation/timeout
handlers and post-error handling; reference EnsureStreamFinalizerCalled,
stopCancellation(), and any existing defer registrations in vllm.go to reorder
them accordingly.
In `@ui/app/pprof/page.tsx`:
- Around line 309-312: The row expansion key currently uses the loop index `i`
in the allocations.map callback which causes expanded state to move when rows
reorder; change the key generation (used where `const key = ...` in the
allocations.map and where rows are toggled against `expandedKeys`) to use a
stable stack signature derived from the allocation (for example a concatenation
of `alloc.function`, `alloc.line`, and a deterministic `alloc.stack`
fingerprint) instead of `i`, and update all places that read/write
`expandedKeys` to use that stable signature so expansion follows the same
allocation regardless of sort or polling reorders.
---
Nitpick comments:
In `@core/providers/bedrock/utils.go`:
- Around line 1130-1134: The two helpers collectBedrockServerTools and
convertToolConfig duplicate the same call to
anthropic.ValidateChatToolsForProvider; to avoid double-filtering, move the
ValidateChatToolsForProvider call up into convertChatParameters (call it once),
then change collectBedrockServerTools and convertToolConfig to accept the
already-filtered tools (or nil) instead of re-reading params.Tools, and remove
their internal ValidateChatToolsForProvider calls; update convertChatParameters
to pass the filtered result into both helpers so filtering is done only once.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf392e01-cab6-42ee-a914-40cab03f9770
📒 Files selected for processing (46)
.github/workflows/e2e-tests.yml.github/workflows/pr-tests.yml.github/workflows/release-cli.yml.github/workflows/release-pipeline.yml.github/workflows/snyk.ymlcli/go.modcore/bifrost.gocore/go.modcore/providers/anthropic/anthropic.gocore/providers/anthropic/chat.gocore/providers/anthropic/utils.gocore/providers/anthropic/validate_chat_tools_test.gocore/providers/azure/azure.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/convert_tool_config_test.gocore/providers/bedrock/utils.gocore/providers/cohere/cohere.gocore/providers/elevenlabs/elevenlabs.gocore/providers/gemini/gemini.gocore/providers/huggingface/huggingface.gocore/providers/mistral/mistral.gocore/providers/openai/openai.gocore/providers/replicate/replicate.gocore/providers/utils/stream.gocore/providers/utils/stream_test.gocore/providers/utils/utils.gocore/providers/vertex/vertex.gocore/providers/vllm/vllm.goexamples/plugins/hello-world/go.modframework/go.modplugins/governance/go.modplugins/jsonparser/go.modplugins/litellmcompat/go.modplugins/logging/go.modplugins/maxim/go.modplugins/mocker/go.modplugins/otel/go.modplugins/semanticcache/go.modplugins/telemetry/go.modtests/scripts/1millogs/go.modtransports/Dockerfiletransports/Dockerfile.localtransports/bifrost-http/handlers/devpprof.gotransports/go.modui/app/pprof/page.tsxui/lib/store/apis/devApi.ts
✅ Files skipped from review due to trivial changes (26)
- plugins/mocker/go.mod
- plugins/logging/go.mod
- .github/workflows/release-pipeline.yml
- examples/plugins/hello-world/go.mod
- plugins/semanticcache/go.mod
- .github/workflows/pr-tests.yml
- tests/scripts/1millogs/go.mod
- core/go.mod
- .github/workflows/e2e-tests.yml
- plugins/jsonparser/go.mod
- framework/go.mod
- plugins/governance/go.mod
- plugins/otel/go.mod
- core/providers/huggingface/huggingface.go
- plugins/telemetry/go.mod
- plugins/maxim/go.mod
- transports/Dockerfile.local
- transports/Dockerfile
- .github/workflows/snyk.yml
- cli/go.mod
- core/providers/azure/azure.go
- plugins/litellmcompat/go.mod
- core/providers/bedrock/bedrock.go
- core/providers/utils/utils.go
- core/providers/gemini/gemini.go
- transports/go.mod
🚧 Files skipped from review as they are similar to previous changes (11)
- core/providers/mistral/mistral.go
- .github/workflows/release-cli.yml
- core/providers/vertex/vertex.go
- ui/lib/store/apis/devApi.ts
- core/providers/anthropic/validate_chat_tools_test.go
- transports/bifrost-http/handlers/devpprof.go
- core/providers/utils/stream.go
- core/providers/anthropic/utils.go
- core/providers/elevenlabs/elevenlabs.go
- core/bifrost.go
- core/providers/cohere/cohere.go
akshaydeo
force-pushed
the
04-17-move_back_go_to_1.26.1
branch
from
b10c2ca to
85f165e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
transports/bifrost-http/handlers/devpprof.go (1)
301-303: Return empty arrays instead ofnullon allocation-profile errors.Line 302 and Line 307 currently return
nil, nil, which serializes asnullslices and can break clients expecting array-shaped fields (top_allocations,inuse_allocations).Proposed fix
func getAllocations() (cumulative, inuse []AllocationInfo) { var buf bytes.Buffer if err := pprof.WriteHeapProfile(&buf); err != nil { - return nil, nil + return []AllocationInfo{}, []AllocationInfo{} } p, err := profile.Parse(&buf) if err != nil { - return nil, nil + return []AllocationInfo{}, []AllocationInfo{} }Also applies to: 305-308
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/devpprof.go` around lines 301 - 303, The handler currently returns nil slices on pprof allocation/profile errors (the branches after pprof.WriteHeapProfile(&buf) and the similar block later), resulting in JSON nulls for top_allocations/inuse_allocations; change those return statements to return empty slices instead (e.g. make([]AllocationType, 0) or []AllocationType{} for the top_allocations and inuse_allocations return values) while still returning the error (i.e., return make(...,0), make(...,0), err). Update both occurrences around the pprof.WriteHeapProfile(&buf) error handling so callers always receive array-shaped fields even on error.core/providers/bedrock/utils.go (1)
88-110: Consider caching the filtered tool set to avoid duplicate validation.
ValidateChatToolsForProvideris called twice for the sameparams.Tools:
- Inside
convertToolConfig(line 89 → line 1203)- Inside
collectBedrockServerTools(line 102 → line 1134)This is a minor efficiency concern since both functions independently filter the same input. The current design favors encapsulation (each function is self-contained), which is reasonable.
💡 Optional: Single validation pass
+ // Filter tools once and pass to both consumers + filteredTools, _ := anthropic.ValidateChatToolsForProvider(bifrostReq.Params.Tools, schemas.Bedrock) + // Convert tool config (function/custom tools → Converse toolConfig.tools). - if toolConfig := convertToolConfig(bifrostReq.Model, bifrostReq.Params); toolConfig != nil { + if toolConfig := convertToolConfigFromFiltered(bifrostReq.Model, bifrostReq.Params, filteredTools); toolConfig != nil { bedrockReq.ToolConfig = toolConfig } - if serverTools, betaHeaders := collectBedrockServerTools(bifrostReq.Params); len(serverTools) > 0 { + if serverTools, betaHeaders := collectBedrockServerToolsFromFiltered(filteredTools); len(serverTools) > 0 {This would require refactoring both helper functions to accept pre-filtered tools.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/utils.go` around lines 88 - 110, The code currently validates/filtering tools twice via ValidateChatToolsForProvider inside convertToolConfig and collectBedrockServerTools; to avoid duplicate work, perform a single validation pass before those calls (call ValidateChatToolsForProvider once on bifrostReq.Params.Tools), store the filtered result (e.g., filteredTools) and update convertToolConfig and collectBedrockServerTools to accept the pre-filtered tool list (or add optional parameters) so they use filteredTools instead of re-calling ValidateChatToolsForProvider; update calls in this file to pass the cached filteredTools into convertToolConfig(bifrostReq.Model, filteredTools) and collectBedrockServerTools(filteredTools) while preserving existing behavior when helpers are called elsewhere by keeping the original code path as a fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/bedrock/utils.go`:
- Around line 1140-1143: The silent skip on marshal failure hides configuration
problems; update the error branch after providerUtils.MarshalSorted(tool) so it
emits observability (e.g., call the provider-local logger to warn with the tool
identifier and err and/or increment a provider-local metric) instead of just
continuing; reference providerUtils.MarshalSorted(tool) and the local variable
tool and ensure you use the provider-local logging/metrics pattern (not core
package imports) when adding the warning/metric increment.
In `@core/providers/openai/openai.go`:
- Around line 3041-3043: The large-payload early-return branches call
handleOpenAILargePayloadPassthrough(...) and return parsed image responses
without applying the Model fallback, so ensure each early-return path sets
response.Model = request.Model before returning; specifically add this
assignment in the passthrough/early-return branches inside
HandleOpenAIImageEditRequest and HandleOpenAIImageVariationRequest (and any
other handler that uses handleOpenAILargePayloadPassthrough(...)) so the Model
fallback mirrors the normal decode path's behavior.
---
Nitpick comments:
In `@core/providers/bedrock/utils.go`:
- Around line 88-110: The code currently validates/filtering tools twice via
ValidateChatToolsForProvider inside convertToolConfig and
collectBedrockServerTools; to avoid duplicate work, perform a single validation
pass before those calls (call ValidateChatToolsForProvider once on
bifrostReq.Params.Tools), store the filtered result (e.g., filteredTools) and
update convertToolConfig and collectBedrockServerTools to accept the
pre-filtered tool list (or add optional parameters) so they use filteredTools
instead of re-calling ValidateChatToolsForProvider; update calls in this file to
pass the cached filteredTools into convertToolConfig(bifrostReq.Model,
filteredTools) and collectBedrockServerTools(filteredTools) while preserving
existing behavior when helpers are called elsewhere by keeping the original code
path as a fallback.
In `@transports/bifrost-http/handlers/devpprof.go`:
- Around line 301-303: The handler currently returns nil slices on pprof
allocation/profile errors (the branches after pprof.WriteHeapProfile(&buf) and
the similar block later), resulting in JSON nulls for
top_allocations/inuse_allocations; change those return statements to return
empty slices instead (e.g. make([]AllocationType, 0) or []AllocationType{} for
the top_allocations and inuse_allocations return values) while still returning
the error (i.e., return make(...,0), make(...,0), err). Update both occurrences
around the pprof.WriteHeapProfile(&buf) error handling so callers always receive
array-shaped fields even on error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc304fee-2325-470b-9855-10996847f7ea
📒 Files selected for processing (47)
.github/workflows/e2e-tests.yml.github/workflows/pr-tests.yml.github/workflows/release-cli.yml.github/workflows/release-pipeline.yml.github/workflows/snyk.ymlcli/go.modcore/bifrost.gocore/go.modcore/providers/anthropic/anthropic.gocore/providers/anthropic/chat.gocore/providers/anthropic/utils.gocore/providers/anthropic/validate_chat_tools_test.gocore/providers/azure/azure.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/convert_tool_config_test.gocore/providers/bedrock/utils.gocore/providers/cohere/cohere.gocore/providers/elevenlabs/elevenlabs.gocore/providers/gemini/gemini.gocore/providers/huggingface/huggingface.gocore/providers/mistral/mistral.gocore/providers/openai/openai.gocore/providers/replicate/replicate.gocore/providers/utils/stream.gocore/providers/utils/stream_test.gocore/providers/utils/utils.gocore/providers/vertex/vertex.gocore/providers/vllm/vllm.goexamples/plugins/hello-world/go.modframework/go.modplugins/governance/go.modplugins/jsonparser/go.modplugins/litellmcompat/go.modplugins/logging/go.modplugins/maxim/go.modplugins/mocker/go.modplugins/otel/go.modplugins/semanticcache/go.modplugins/telemetry/go.modtests/scripts/1millogs/go.modtransports/Dockerfiletransports/Dockerfile.localtransports/bifrost-http/handlers/devpprof.gotransports/changelog.mdtransports/go.modui/app/pprof/page.tsxui/lib/store/apis/devApi.ts
✅ Files skipped from review due to trivial changes (26)
- plugins/maxim/go.mod
- core/go.mod
- plugins/logging/go.mod
- .github/workflows/e2e-tests.yml
- cli/go.mod
- tests/scripts/1millogs/go.mod
- .github/workflows/pr-tests.yml
- plugins/telemetry/go.mod
- plugins/litellmcompat/go.mod
- plugins/jsonparser/go.mod
- plugins/governance/go.mod
- plugins/otel/go.mod
- plugins/mocker/go.mod
- examples/plugins/hello-world/go.mod
- .github/workflows/release-pipeline.yml
- .github/workflows/snyk.yml
- framework/go.mod
- core/providers/cohere/cohere.go
- plugins/semanticcache/go.mod
- core/providers/huggingface/huggingface.go
- core/providers/gemini/gemini.go
- transports/Dockerfile.local
- transports/Dockerfile
- transports/changelog.md
- core/providers/bedrock/bedrock.go
- ui/app/pprof/page.tsx
🚧 Files skipped from review as they are similar to previous changes (13)
- .github/workflows/release-cli.yml
- core/providers/vllm/vllm.go
- transports/go.mod
- core/providers/vertex/vertex.go
- core/providers/azure/azure.go
- core/providers/anthropic/chat.go
- core/providers/utils/stream_test.go
- core/providers/anthropic/anthropic.go
- core/providers/utils/utils.go
- core/providers/replicate/replicate.go
- core/providers/utils/stream.go
- core/providers/anthropic/validate_chat_tools_test.go
- core/bifrost.go
akshaydeo
force-pushed
the
04-17-move_back_go_to_1.26.1
branch
from
85f165e to
ff1bb3d
Compare
Contributor
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)
core/providers/bedrock/utils.go (1)
1202-1248:⚠️ Potential issue | 🟠 Major
tool.Customis validated as supported but never reaches Bedrock.This loop only materializes
tool.Function.anthropic.ValidateChatToolsForProvider(...)keepstool.Custom, and the comment above says function/custom tools should flow into ConversetoolConfig.tools, but any custom-only request currently returnsniland silently loses tool access.Either convert
tool.Customhere as well, or stop treating custom tools as supported in the validation layer so the behavior is consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/utils.go` around lines 1202 - 1248, The loop only handles tool.Function and drops tool.Custom even though anthropic.ValidateChatToolsForProvider(...) preserves custom tools; update the conversion in core/providers/bedrock/utils.go to also materialize tool.Custom into a BedrockTool (similar to the tool.Function branch) so custom tools are forwarded to Bedrock/Converse instead of being lost: detect when tool.Custom != nil, build a BedrockTool with a ToolSpec (or appropriate fields) using tool.Custom's name/description and parameters (serialize parameters via providerUtils.MarshalSorted or fallback to {"type":"object","properties":{}}), and append it to bedrockTools; ensure you also preserve CacheControl behavior (the same check with schemas.IsNovaModel(model)) for custom tools as you do for function tools.core/providers/openai/openai.go (1)
7219-7264:⚠️ Potential issue | 🟠 MajorCall the EOF finalizer through
EnsureStreamFinalizerCalled.Lines 7262-7264 invoke the raw context finalizer directly instead of the helper. That bypasses the helper’s safety wrapper, so a panic in the registered finalizer would escape this goroutine and can take down the process. Keep the early-EOF finalization, but route it through
providerUtils.EnsureStreamFinalizerCalled(ctx)instead.🛠️ Proposed fix
- if finalizer, ok := ctx.Value(schemas.BifrostContextKeyPostHookSpanFinalizer).(func(context.Context)); ok && finalizer != nil { - finalizer(ctx) - } + providerUtils.EnsureStreamFinalizerCalled(ctx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/openai/openai.go` around lines 7219 - 7264, The code currently invokes the raw finalizer from ctx.Value(schemas.BifrostContextKeyPostHookSpanFinalizer) directly after postHookRunner, which bypasses the safety wrapper and can panic the goroutine; replace that direct invocation with a call to providerUtils.EnsureStreamFinalizerCalled(ctx) (and remove the manual finalizer retrieval/invocation) so the registered post-hook span finalizer runs through the helper's panic-safe wrapper while preserving the early-EOF finalization behavior alongside postHookRunner and finalResp in the streaming loop.
♻️ Duplicate comments (4)
core/providers/elevenlabs/elevenlabs.go (1)
427-430:⚠️ Potential issue | 🟠 MajorRegister the stream finalizer defer first in the goroutine.
At Line 429,
EnsureStreamFinalizerCalledis deferred afterdefer stopCancellation()(Line 428), so it executes earlier under LIFO. That can finalize the stream span before cancellation/timeout teardown finishes.Suggested fix
go func() { + defer providerUtils.EnsureStreamFinalizerCalled(ctx) defer func() { if ctx.Err() == context.Canceled { providerUtils.HandleStreamCancellation(ctx, postHookRunner, responseChan, providerName, request.Model, schemas.SpeechStreamRequest, provider.logger) @@ stopCancellation := providerUtils.SetupStreamCancellation(ctx, resp.BodyStream(), provider.logger) defer stopCancellation() - defer providerUtils.EnsureStreamFinalizerCalled(ctx)#!/bin/bash set -euo pipefail # Verify defer registration order in Elevenlabs SpeechStream goroutine rg -n -C6 'go func\(\)|EnsureStreamFinalizerCalled|SetupStreamCancellation|defer stopCancellation' core/providers/elevenlabs/elevenlabs.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/elevenlabs/elevenlabs.go` around lines 427 - 430, The stream finalizer is deferred after the cancellation stop in the Elevenlabs SpeechStream goroutine, causing EnsureStreamFinalizerCalled to run before stopCancellation; swap the defer registrations so EnsureStreamFinalizerCalled(ctx) is deferred first and then defer stopCancellation() (the result of providerUtils.SetupStreamCancellation(ctx, resp.BodyStream(), provider.logger)) to ensure the finalizer runs after cancellation/timeout teardown.core/providers/mistral/mistral.go (1)
603-606:⚠️ Potential issue | 🟠 MajorMove the finalizer defer above the other defers in this goroutine.
At Line 605, the finalizer is currently scheduled after
defer stopCancellation()(Line 604), so it runs first on exit. It should run last, after cancellation/timeout cleanup.Suggested fix
go func() { + defer providerUtils.EnsureStreamFinalizerCalled(ctx) defer func() { if ctx.Err() == context.Canceled { providerUtils.HandleStreamCancellation(ctx, postHookRunner, responseChan, providerName, request.Model, schemas.TranscriptionStreamRequest, provider.logger) @@ stopCancellation := providerUtils.SetupStreamCancellation(ctx, resp.BodyStream(), provider.logger) defer stopCancellation() - defer providerUtils.EnsureStreamFinalizerCalled(ctx)#!/bin/bash set -euo pipefail # Verify defer registration order in Mistral TranscriptionStream goroutine rg -n -C6 'go func\(\)|EnsureStreamFinalizerCalled|SetupStreamCancellation|defer stopCancellation' core/providers/mistral/mistral.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/mistral/mistral.go` around lines 603 - 606, The EnsureStreamFinalizerCalled defer is registered after stopCancellation so it currently runs first; move the call to providerUtils.EnsureStreamFinalizerCalled(ctx) so it is deferred before calling providerUtils.SetupStreamCancellation(ctx, resp.BodyStream(), provider.logger) (i.e., defer providerUtils.EnsureStreamFinalizerCalled(ctx) should appear above defer stopCancellation()) so the finalizer executes last after cancellation/timeout cleanup in that goroutine.core/bifrost.go (2)
4634-4635:⚠️ Potential issue | 🔴 CriticalKeep the plugin pipeline alive until the short-circuit stream goroutine exits.
This goroutine still calls
pipelinePostHookRunneraftertryStreamRequestreturns, so the defer on Line 4635 putspipelineback into the pool while the goroutine is still using it. The new cancel/drain branch makes that lifetime even longer.💡 Suggested fix
pipeline := bifrost.getPluginPipeline() - defer bifrost.releasePluginPipeline(pipeline) + releaseWithStream := false + defer func() { + if !releaseWithStream { + bifrost.releasePluginPipeline(pipeline) + } + }() preReq, shortCircuit, preCount := pipeline.RunLLMPreHooks(ctx, req) if shortCircuit != nil { @@ if shortCircuit.Stream != nil { + releaseWithStream = true outputStream := make(chan *schemas.BifrostStreamChunk) @@ go func() { + defer bifrost.releasePluginPipeline(pipeline) defer close(outputStream) for streamMsg := range shortCircuit.Stream {Also applies to: 4651-4701
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 4634 - 4635, The pipeline retrieved via bifrost.getPluginPipeline() is being returned too early by the defer bifrost.releasePluginPipeline(pipeline) while a spawned short-circuit stream goroutine still uses it (it calls pipelinePostHookRunner after tryStreamRequest returns); remove the premature defer and instead ensure the pipeline is released only after the goroutine finishes (e.g., move releasePluginPipeline(pipeline) into the goroutine exit path or synchronize with a WaitGroup/channel and call release after pipelinePostHookRunner completes), updating both occurrences around the short-circuit stream code (the block that calls tryStreamRequest and pipelinePostHookRunner) so the pipeline remains valid for the goroutine's full lifetime.
5227-5238:⚠️ Potential issue | 🔴 CriticalRoute all streaming cleanup through
postHookSpanFinalizer.
sync.Onceonly helps callers that go throughpostHookSpanFinalizer. The directreleasePluginPipelinebelow still bypasses it on streaming errors, so first-chunk SSE errors can double-put the same pooled pipeline when the provider goroutine later invokes the stored finalizer.💡 Suggested fix
- // Release pipeline immediately for non-streaming requests only - // For streaming, the pipeline is released in the postHookSpanFinalizer after streaming completes - // Exception: if streaming request has an error, release immediately since finalizer won't be called - if pipeline != nil && (!IsStreamRequestType(req.RequestType) || bifrostError != nil) { - bifrost.releasePluginPipeline(pipeline) + // For streaming requests, always route cleanup through the stored finalizer so + // release/finalize stays under the same sync.Once across all exit paths. + if pipeline != nil && bifrostError != nil { + if finalizer, ok := req.Context.Value(schemas.BifrostContextKeyPostHookSpanFinalizer).(func(context.Context)); ok { + finalizer(req.Context) + } else { + bifrost.releasePluginPipeline(pipeline) + } }Also applies to: 5252-5257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 5227 - 5238, The pipeline release bypasses the sync.Once-protected postHookSpanFinalizer: replace direct calls to bifrost.releasePluginPipeline(pipeline) (including the other occurrence around the 5252-5257 region) with a call to the stored finalizer (invoke postHookSpanFinalizer with the appropriate ctx) so that pipeline.FinalizeStreamingPostHookSpans and releasePluginPipeline are always executed under finalizerOnce.Do; ensure req.Context.SetValue remains as-is (so callers can fetch the same postHookSpanFinalizer) and remove any duplicate direct release calls to prevent double-put into the pool.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/bedrock/utils.go`:
- Around line 93-109: collectBedrockServerTools currently only adds "tools" and
"anthropic_beta" to bedrockReq.AdditionalModelRequestFields which lets server
tools appear in the request but drops the caller's pinned tool via the existing
reconciliation that checks tool_choice against bedrockTools; update
collectBedrockServerTools (or the block that writes
AdditionalModelRequestFields) to also compute and set the Anthropic-native
"tool_choice" entry for any kept/pinned server tool (same naming/shape Anthropic
expects) so the forced-tool contract is preserved, ensure the reconciliation
logic that reads tool_choice will find the server-tool name, and add a
regression test that sends a request with a pinned server tool (e.g., computer_*
or bash_*) verifying the outgoing bedrock request contains both tools and
tool_choice and that the server-tool is honored.
---
Outside diff comments:
In `@core/providers/bedrock/utils.go`:
- Around line 1202-1248: The loop only handles tool.Function and drops
tool.Custom even though anthropic.ValidateChatToolsForProvider(...) preserves
custom tools; update the conversion in core/providers/bedrock/utils.go to also
materialize tool.Custom into a BedrockTool (similar to the tool.Function branch)
so custom tools are forwarded to Bedrock/Converse instead of being lost: detect
when tool.Custom != nil, build a BedrockTool with a ToolSpec (or appropriate
fields) using tool.Custom's name/description and parameters (serialize
parameters via providerUtils.MarshalSorted or fallback to
{"type":"object","properties":{}}), and append it to bedrockTools; ensure you
also preserve CacheControl behavior (the same check with
schemas.IsNovaModel(model)) for custom tools as you do for function tools.
In `@core/providers/openai/openai.go`:
- Around line 7219-7264: The code currently invokes the raw finalizer from
ctx.Value(schemas.BifrostContextKeyPostHookSpanFinalizer) directly after
postHookRunner, which bypasses the safety wrapper and can panic the goroutine;
replace that direct invocation with a call to
providerUtils.EnsureStreamFinalizerCalled(ctx) (and remove the manual finalizer
retrieval/invocation) so the registered post-hook span finalizer runs through
the helper's panic-safe wrapper while preserving the early-EOF finalization
behavior alongside postHookRunner and finalResp in the streaming loop.
---
Duplicate comments:
In `@core/bifrost.go`:
- Around line 4634-4635: The pipeline retrieved via bifrost.getPluginPipeline()
is being returned too early by the defer bifrost.releasePluginPipeline(pipeline)
while a spawned short-circuit stream goroutine still uses it (it calls
pipelinePostHookRunner after tryStreamRequest returns); remove the premature
defer and instead ensure the pipeline is released only after the goroutine
finishes (e.g., move releasePluginPipeline(pipeline) into the goroutine exit
path or synchronize with a WaitGroup/channel and call release after
pipelinePostHookRunner completes), updating both occurrences around the
short-circuit stream code (the block that calls tryStreamRequest and
pipelinePostHookRunner) so the pipeline remains valid for the goroutine's full
lifetime.
- Around line 5227-5238: The pipeline release bypasses the sync.Once-protected
postHookSpanFinalizer: replace direct calls to
bifrost.releasePluginPipeline(pipeline) (including the other occurrence around
the 5252-5257 region) with a call to the stored finalizer (invoke
postHookSpanFinalizer with the appropriate ctx) so that
pipeline.FinalizeStreamingPostHookSpans and releasePluginPipeline are always
executed under finalizerOnce.Do; ensure req.Context.SetValue remains as-is (so
callers can fetch the same postHookSpanFinalizer) and remove any duplicate
direct release calls to prevent double-put into the pool.
In `@core/providers/elevenlabs/elevenlabs.go`:
- Around line 427-430: The stream finalizer is deferred after the cancellation
stop in the Elevenlabs SpeechStream goroutine, causing
EnsureStreamFinalizerCalled to run before stopCancellation; swap the defer
registrations so EnsureStreamFinalizerCalled(ctx) is deferred first and then
defer stopCancellation() (the result of
providerUtils.SetupStreamCancellation(ctx, resp.BodyStream(), provider.logger))
to ensure the finalizer runs after cancellation/timeout teardown.
In `@core/providers/mistral/mistral.go`:
- Around line 603-606: The EnsureStreamFinalizerCalled defer is registered after
stopCancellation so it currently runs first; move the call to
providerUtils.EnsureStreamFinalizerCalled(ctx) so it is deferred before calling
providerUtils.SetupStreamCancellation(ctx, resp.BodyStream(), provider.logger)
(i.e., defer providerUtils.EnsureStreamFinalizerCalled(ctx) should appear above
defer stopCancellation()) so the finalizer executes last after
cancellation/timeout cleanup in that goroutine.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bfe39345-2cb8-40c1-99ee-b865aab39f6f
📒 Files selected for processing (49)
.github/workflows/e2e-tests.yml.github/workflows/pr-tests.yml.github/workflows/release-cli.yml.github/workflows/release-pipeline.yml.github/workflows/snyk.ymlcli/go.modcore/bifrost.gocore/go.modcore/providers/anthropic/anthropic.gocore/providers/anthropic/chat.gocore/providers/anthropic/utils.gocore/providers/anthropic/validate_chat_tools_test.gocore/providers/azure/azure.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/convert_tool_config_test.gocore/providers/bedrock/utils.gocore/providers/cohere/cohere.gocore/providers/elevenlabs/elevenlabs.gocore/providers/gemini/gemini.gocore/providers/huggingface/huggingface.gocore/providers/mistral/mistral.gocore/providers/openai/openai.gocore/providers/replicate/replicate.gocore/providers/utils/stream.gocore/providers/utils/stream_test.gocore/providers/utils/utils.gocore/providers/vertex/vertex.gocore/providers/vllm/vllm.gocore/schemas/images.gocore/schemas/videos.goexamples/plugins/hello-world/go.modframework/go.modplugins/governance/go.modplugins/jsonparser/go.modplugins/litellmcompat/go.modplugins/logging/go.modplugins/maxim/go.modplugins/mocker/go.modplugins/otel/go.modplugins/semanticcache/go.modplugins/telemetry/go.modtests/scripts/1millogs/go.modtransports/Dockerfiletransports/Dockerfile.localtransports/bifrost-http/handlers/devpprof.gotransports/changelog.mdtransports/go.modui/app/pprof/page.tsxui/lib/store/apis/devApi.ts
✅ Files skipped from review due to trivial changes (23)
- examples/plugins/hello-world/go.mod
- .github/workflows/e2e-tests.yml
- tests/scripts/1millogs/go.mod
- .github/workflows/release-cli.yml
- core/go.mod
- framework/go.mod
- plugins/logging/go.mod
- cli/go.mod
- plugins/litellmcompat/go.mod
- plugins/jsonparser/go.mod
- plugins/telemetry/go.mod
- plugins/governance/go.mod
- plugins/mocker/go.mod
- .github/workflows/snyk.yml
- plugins/otel/go.mod
- .github/workflows/release-pipeline.yml
- plugins/semanticcache/go.mod
- plugins/maxim/go.mod
- core/providers/azure/azure.go
- transports/Dockerfile.local
- transports/Dockerfile
- core/providers/anthropic/validate_chat_tools_test.go
- transports/changelog.md
🚧 Files skipped from review as they are similar to previous changes (14)
- .github/workflows/pr-tests.yml
- transports/go.mod
- ui/lib/store/apis/devApi.ts
- core/providers/cohere/cohere.go
- core/providers/vertex/vertex.go
- core/providers/replicate/replicate.go
- core/providers/bedrock/bedrock.go
- core/providers/huggingface/huggingface.go
- core/providers/utils/stream_test.go
- core/providers/gemini/gemini.go
- core/providers/anthropic/anthropic.go
- core/providers/utils/utils.go
- transports/bifrost-http/handlers/devpprof.go
- ui/app/pprof/page.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
core/bifrost.go (2)
4634-4705:⚠️ Potential issue | 🔴 CriticalKeep the pooled
PluginPipelinealive until the short-circuit stream goroutine exits.Line 4635 still defers
bifrost.releasePluginPipeline(pipeline), but this goroutine keeps callingpipeline.RunPostLLMHooksthroughpipelinePostHookRunneraftertryStreamRequestreturns. That returns the same pooled pipeline to another request while it is still in use here.Suggested fix
pipeline := bifrost.getPluginPipeline() - defer bifrost.releasePluginPipeline(pipeline) + releaseInCaller := true + defer func() { + if releaseInCaller { + bifrost.releasePluginPipeline(pipeline) + } + }() @@ if shortCircuit.Stream != nil { outputStream := make(chan *schemas.BifrostStreamChunk) @@ go func() { + defer bifrost.releasePluginPipeline(pipeline) defer close(outputStream) @@ }() + releaseInCaller = false return outputStream, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 4634 - 4705, The deferred release of the pooled PluginPipeline (obtained via bifrost.getPluginPipeline and released via bifrost.releasePluginPipeline) happens before a goroutine spawned for shortCircuit.Stream finishes and the goroutine calls pipeline.RunPostLLMHooks via pipelinePostHookRunner; keep the pipeline alive until that goroutine exits by removing the early defer and instead releasing the pipeline after the goroutine completes (e.g., capture the pipeline locally and call bifrost.releasePluginPipeline(pipeline) from a defer inside the goroutine or use a sync.WaitGroup/done channel to release the pipeline once the stream loop finishes), ensuring pipelinePostHookRunner still references the valid pipeline.
5215-5257:⚠️ Potential issue | 🔴 CriticalRoute streaming setup errors through the finalizer instead of releasing the pipeline directly.
With the stack-wide
EnsureStreamFinalizerCalled(ctx)safety net, a stream can still invokeBifrostContextKeyPostHookSpanFinalizerafterCheckFirstStreamChunkForErrorturns setup intobifrostError. The directbifrost.releasePluginPipeline(pipeline)below bypassesfinalizerOnce, so the same pipeline can be put back twice.Suggested fix
- if pipeline != nil && (!IsStreamRequestType(req.RequestType) || bifrostError != nil) { - bifrost.releasePluginPipeline(pipeline) - } + if pipeline != nil && bifrostError != nil { + if finalizer, ok := req.Context.Value(schemas.BifrostContextKeyPostHookSpanFinalizer).(func(context.Context)); ok && finalizer != nil { + finalizer(req.Context) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 5215 - 5257, The pipeline is being released directly on streaming setup errors which can bypass finalizerOnce and double-release the pipeline; instead, when IsStreamRequestType(req.RequestType) is true and bifrostError != nil, invoke the stored finalizer (postHookSpanFinalizer) so the release goes through finalizerOnce. Modify the release logic that currently calls bifrost.releasePluginPipeline(pipeline) to: if pipeline != nil then if !IsStreamRequestType(req.RequestType) || bifrostError == nil { bifrost.releasePluginPipeline(pipeline) } else { retrieve and call the finalizer (postHookSpanFinalizer) from the request context using schemas.BifrostContextKeyPostHookSpanFinalizer or call the local postHookSpanFinalizer variable so the pipeline is released via finalizerOnce.Finalizer-related symbols: postHookSpanFinalizer, finalizerOnce, schemas.BifrostContextKeyPostHookSpanFinalizer; pipeline lifecycle symbols: getPluginPipeline, releasePluginPipeline.core/providers/mistral/mistral.go (1)
605-605:⚠️ Potential issue | 🟠 MajorMove finalizer defer to the top of the goroutine.
At Line 605,
EnsureStreamFinalizerCalledis registered after other defers, so it executes too early. It should be the first defer in the goroutine to run last.Proposed fix
go func() { + defer providerUtils.EnsureStreamFinalizerCalled(ctx) defer func() { if ctx.Err() == context.Canceled { providerUtils.HandleStreamCancellation(ctx, postHookRunner, responseChan, providerName, request.Model, schemas.TranscriptionStreamRequest, provider.logger) @@ stopCancellation := providerUtils.SetupStreamCancellation(ctx, resp.BodyStream(), provider.logger) defer stopCancellation() - defer providerUtils.EnsureStreamFinalizerCalled(ctx) sseReader := providerUtils.GetSSEEventReader(ctx, reader)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/mistral/mistral.go` at line 605, The defer providerUtils.EnsureStreamFinalizerCalled(ctx) is registered too late inside the goroutine so it runs early; move that defer to be the very first defer call immediately after entering the goroutine (before any other defer statements or cleanup logic) so EnsureStreamFinalizerCalled executes last when the goroutine returns; locate the goroutine that currently registers EnsureStreamFinalizerCalled and place that defer at the top of the goroutine body (keeping the same ctx) to ensure proper finalization.
🧹 Nitpick comments (1)
core/providers/bedrock/convert_tool_config_test.go (1)
51-77: Add a nil-params regression test forconvertToolConfig.Given the wrapper currently accepts
*schemas.ChatParameters, a direct nil-input test would lock in the expected non-panicking behavior once the guard is added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/convert_tool_config_test.go` around lines 51 - 77, Add a regression test that ensures convertToolConfig handles a nil *schemas.ChatParameters without panicking and returns nil; create a test (e.g., TestConvertToolConfig_NilParamsDoesNotPanic) that calls convertToolConfig("global.anthropic.claude-sonnet-4-6", nil) and asserts the result is nil (and that the call does not panic), so the nil-guard in convertToolConfig is exercised and locked in.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/bedrock/utils.go`:
- Around line 1321-1327: The function convertToolConfig currently dereferences
params before checking for nil; add a nil guard at the top of convertToolConfig
(e.g., if params == nil { return nil }) so callers can't panic, then keep the
existing logic (check len(params.Tools) and call
anthropic.ValidateChatToolsForProvider and convertToolConfigFromFiltered).
Ensure you reference convertToolConfig, convertToolConfigFromFiltered, and
anthropic.ValidateChatToolsForProvider when updating the function.
In `@transports/bifrost-http/handlers/devpprof.go`:
- Around line 29-30: The topAllocationsCount truncation is causing leak
candidates to be lost because top_allocations and inuse_allocations are
independently truncated; update the logic that builds the pprof response so leak
detection is performed on the full, un-truncated allocation lists before
applying the topAllocationsCount limit (or alternatively ensure each emitted
live stack includes its cumulative bytes so leak detection can operate
correctly). Locate the code that slices/truncates the allocation arrays
(references around topAllocationsCount and the allocation-building routines near
lines ~414-423) and move the leak-candidate computation to run against the
complete arrays, then apply the truncation only to the payload sent to the UI;
ensure cumulative bytes are preserved for any live stack included in the emitted
top list.
---
Duplicate comments:
In `@core/bifrost.go`:
- Around line 4634-4705: The deferred release of the pooled PluginPipeline
(obtained via bifrost.getPluginPipeline and released via
bifrost.releasePluginPipeline) happens before a goroutine spawned for
shortCircuit.Stream finishes and the goroutine calls pipeline.RunPostLLMHooks
via pipelinePostHookRunner; keep the pipeline alive until that goroutine exits
by removing the early defer and instead releasing the pipeline after the
goroutine completes (e.g., capture the pipeline locally and call
bifrost.releasePluginPipeline(pipeline) from a defer inside the goroutine or use
a sync.WaitGroup/done channel to release the pipeline once the stream loop
finishes), ensuring pipelinePostHookRunner still references the valid pipeline.
- Around line 5215-5257: The pipeline is being released directly on streaming
setup errors which can bypass finalizerOnce and double-release the pipeline;
instead, when IsStreamRequestType(req.RequestType) is true and bifrostError !=
nil, invoke the stored finalizer (postHookSpanFinalizer) so the release goes
through finalizerOnce. Modify the release logic that currently calls
bifrost.releasePluginPipeline(pipeline) to: if pipeline != nil then if
!IsStreamRequestType(req.RequestType) || bifrostError == nil {
bifrost.releasePluginPipeline(pipeline) } else { retrieve and call the finalizer
(postHookSpanFinalizer) from the request context using
schemas.BifrostContextKeyPostHookSpanFinalizer or call the local
postHookSpanFinalizer variable so the pipeline is released via
finalizerOnce.Finalizer-related symbols: postHookSpanFinalizer, finalizerOnce,
schemas.BifrostContextKeyPostHookSpanFinalizer; pipeline lifecycle symbols:
getPluginPipeline, releasePluginPipeline.
In `@core/providers/mistral/mistral.go`:
- Line 605: The defer providerUtils.EnsureStreamFinalizerCalled(ctx) is
registered too late inside the goroutine so it runs early; move that defer to be
the very first defer call immediately after entering the goroutine (before any
other defer statements or cleanup logic) so EnsureStreamFinalizerCalled executes
last when the goroutine returns; locate the goroutine that currently registers
EnsureStreamFinalizerCalled and place that defer at the top of the goroutine
body (keeping the same ctx) to ensure proper finalization.
---
Nitpick comments:
In `@core/providers/bedrock/convert_tool_config_test.go`:
- Around line 51-77: Add a regression test that ensures convertToolConfig
handles a nil *schemas.ChatParameters without panicking and returns nil; create
a test (e.g., TestConvertToolConfig_NilParamsDoesNotPanic) that calls
convertToolConfig("global.anthropic.claude-sonnet-4-6", nil) and asserts the
result is nil (and that the call does not panic), so the nil-guard in
convertToolConfig is exercised and locked in.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1bea4526-c0fa-4657-bb22-b8835043bcde
📒 Files selected for processing (49)
.github/workflows/e2e-tests.yml.github/workflows/pr-tests.yml.github/workflows/release-cli.yml.github/workflows/release-pipeline.yml.github/workflows/snyk.ymlcli/go.modcore/bifrost.gocore/go.modcore/providers/anthropic/anthropic.gocore/providers/anthropic/chat.gocore/providers/anthropic/utils.gocore/providers/anthropic/validate_chat_tools_test.gocore/providers/azure/azure.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/convert_tool_config_test.gocore/providers/bedrock/utils.gocore/providers/cohere/cohere.gocore/providers/elevenlabs/elevenlabs.gocore/providers/gemini/gemini.gocore/providers/huggingface/huggingface.gocore/providers/mistral/mistral.gocore/providers/openai/openai.gocore/providers/replicate/replicate.gocore/providers/utils/stream.gocore/providers/utils/stream_test.gocore/providers/utils/utils.gocore/providers/vertex/vertex.gocore/providers/vllm/vllm.gocore/schemas/images.gocore/schemas/videos.goexamples/plugins/hello-world/go.modframework/go.modplugins/governance/go.modplugins/jsonparser/go.modplugins/litellmcompat/go.modplugins/logging/go.modplugins/maxim/go.modplugins/mocker/go.modplugins/otel/go.modplugins/semanticcache/go.modplugins/telemetry/go.modtests/scripts/1millogs/go.modtransports/Dockerfiletransports/Dockerfile.localtransports/bifrost-http/handlers/devpprof.gotransports/changelog.mdtransports/go.modui/app/pprof/page.tsxui/lib/store/apis/devApi.ts
✅ Files skipped from review due to trivial changes (25)
- cli/go.mod
- plugins/telemetry/go.mod
- core/go.mod
- plugins/logging/go.mod
- .github/workflows/e2e-tests.yml
- tests/scripts/1millogs/go.mod
- .github/workflows/release-cli.yml
- transports/go.mod
- .github/workflows/snyk.yml
- plugins/jsonparser/go.mod
- framework/go.mod
- plugins/maxim/go.mod
- plugins/litellmcompat/go.mod
- plugins/semanticcache/go.mod
- .github/workflows/pr-tests.yml
- examples/plugins/hello-world/go.mod
- plugins/mocker/go.mod
- .github/workflows/release-pipeline.yml
- plugins/governance/go.mod
- core/providers/azure/azure.go
- transports/Dockerfile
- transports/Dockerfile.local
- plugins/otel/go.mod
- core/providers/anthropic/validate_chat_tools_test.go
- core/providers/anthropic/utils.go
🚧 Files skipped from review as they are similar to previous changes (9)
- ui/lib/store/apis/devApi.ts
- core/providers/vertex/vertex.go
- core/providers/replicate/replicate.go
- core/schemas/images.go
- core/providers/gemini/gemini.go
- core/providers/bedrock/bedrock.go
- core/providers/utils/stream.go
- core/providers/utils/stream_test.go
- transports/changelog.md
akshaydeo
force-pushed
the
04-17-dependabot_fixes
branch
from
8dba7a5 to
01425fe
Compare
akshaydeo
force-pushed
the
04-17-move_back_go_to_1.26.1
branch
from
2cb5b2a to
8acabfb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/providers/mistral/mistral.go (1)
601-606:⚠️ Potential issue | 🟠 MajorMove
EnsureStreamFinalizerCalledto the first defer in this goroutine.At Line 605, it’s registered too late. With Go’s LIFO defer order, it executes before
stopCancellation()and other cleanup defers, which can finalize the stream lifecycle too early.Suggested fix
go func() { + defer providerUtils.EnsureStreamFinalizerCalled(ctx) defer func() { if ctx.Err() == context.Canceled { providerUtils.HandleStreamCancellation(ctx, postHookRunner, responseChan, providerName, request.Model, schemas.TranscriptionStreamRequest, provider.logger) @@ stopCancellation := providerUtils.SetupStreamCancellation(ctx, resp.BodyStream(), provider.logger) defer stopCancellation() - defer providerUtils.EnsureStreamFinalizerCalled(ctx) sseReader := providerUtils.GetSSEEventReader(ctx, reader)#!/bin/bash # Verify defer registration order in the Mistral transcription streaming goroutine. rg -n -C8 'go func\(\) \{|defer providerUtils\.EnsureStreamFinalizerCalled\(ctx\)|defer stopCancellation\(\)|HandleStreamCancellation|HandleStreamTimeout' core/providers/mistral/mistral.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/mistral/mistral.go` around lines 601 - 606, The EnsureStreamFinalizerCalled defer is registered after stopCancellation in the goroutine, causing it to run too early due to Go's LIFO defer order; move the call to providerUtils.EnsureStreamFinalizerCalled(ctx) so its defer is the first defer inside the goroutine (before calling providerUtils.SetupStreamCancellation and deferring stopCancellation())—i.e., locate the goroutine that uses resp.BodyStream() and ctx, call defer providerUtils.EnsureStreamFinalizerCalled(ctx) first, then call stopCancellation := providerUtils.SetupStreamCancellation(ctx, resp.BodyStream(), provider.logger) and defer stopCancellation().
🧹 Nitpick comments (2)
core/providers/anthropic/validate_chat_tools_test.go (1)
29-35: Consider adding an explicit custom-tool survivor case.You already lock in function-tool behavior; adding one
tool.Custom != nilcase would fully cover the “function/custom always keep” contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/validate_chat_tools_test.go` around lines 29 - 35, Add a test case that mirrors the existing function-tool survivor case but uses a ChatTool whose Custom field is non-nil to assert custom tools always survive; e.g. add a case named like "custom tools always survive on any provider" in validate_chat_tools_test.go that uses provider schemas.Bedrock, input []schemas.ChatTool{customTool, customTool} (or construct a tool with Custom != nil), and expect wantKeep: 2 so the contract "function/custom always keep" is fully covered alongside fnTool.ui/app/pprof/page.tsx (1)
690-694: Don't evict leak history after a single missing snapshot.Because
inuse_allocationsis already top-N filtered, a stack can disappear for one poll without actually being freed. Deleting its history immediately resets the growth signal for borderline leaks and makes the detector flap around the cutoff. A short miss TTL would make this much steadier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/pprof/page.tsx` around lines 690 - 694, The current loop that immediately deletes entries from map when a key is not in seen causes transient eviction; instead add a short miss-TTL by tracking misses or lastSeen for each key (e.g., introduce missCounts or lastSeen map alongside map) and only delete from map when misses exceed a configured threshold or lastSeen is older than TTL; update the eviction loop that iterates [...map.keys()] to increment/reset the miss counter for keys not in seen (reset for keys present) and only call map.delete(key) when the counter/age passes the threshold, and ensure you initialize and maintain the new miss tracking in the same scope as map and seen so it persists across polls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/pprof/page.tsx`:
- Around line 161-170: detectLeaks() currently drops live allocations when their
cumulative counterpart is absent (if (!cum) continue), which causes real leaks
to be missed because the backend truncates cumulative and in-use lists
independently in flattenAndTopN; either correlate cumulative+inuse before top-N
truncation in transports/bifrost-http/handlers/devpprof.go (move the matching
logic into flattenAndTopN so it returns paired rows) or change the backend
response to include cumulative bytes/retention on each live row so detectLeaks
(the loop using makeStackKey, cumMap, inuseHistory, isMonotonicGrowing,
classifyLeakSeverity) can compute retention without skipping entries; update the
code paths accordingly so detectLeaks no longer drops live entries when cum is
missing.
---
Duplicate comments:
In `@core/providers/mistral/mistral.go`:
- Around line 601-606: The EnsureStreamFinalizerCalled defer is registered after
stopCancellation in the goroutine, causing it to run too early due to Go's LIFO
defer order; move the call to providerUtils.EnsureStreamFinalizerCalled(ctx) so
its defer is the first defer inside the goroutine (before calling
providerUtils.SetupStreamCancellation and deferring stopCancellation())—i.e.,
locate the goroutine that uses resp.BodyStream() and ctx, call defer
providerUtils.EnsureStreamFinalizerCalled(ctx) first, then call stopCancellation
:= providerUtils.SetupStreamCancellation(ctx, resp.BodyStream(),
provider.logger) and defer stopCancellation().
---
Nitpick comments:
In `@core/providers/anthropic/validate_chat_tools_test.go`:
- Around line 29-35: Add a test case that mirrors the existing function-tool
survivor case but uses a ChatTool whose Custom field is non-nil to assert custom
tools always survive; e.g. add a case named like "custom tools always survive on
any provider" in validate_chat_tools_test.go that uses provider schemas.Bedrock,
input []schemas.ChatTool{customTool, customTool} (or construct a tool with
Custom != nil), and expect wantKeep: 2 so the contract "function/custom always
keep" is fully covered alongside fnTool.
In `@ui/app/pprof/page.tsx`:
- Around line 690-694: The current loop that immediately deletes entries from
map when a key is not in seen causes transient eviction; instead add a short
miss-TTL by tracking misses or lastSeen for each key (e.g., introduce missCounts
or lastSeen map alongside map) and only delete from map when misses exceed a
configured threshold or lastSeen is older than TTL; update the eviction loop
that iterates [...map.keys()] to increment/reset the miss counter for keys not
in seen (reset for keys present) and only call map.delete(key) when the
counter/age passes the threshold, and ensure you initialize and maintain the new
miss tracking in the same scope as map and seen so it persists across polls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ca5fe75-18a2-4bfb-bd10-e7f0af6c932c
📒 Files selected for processing (49)
.github/workflows/e2e-tests.yml.github/workflows/pr-tests.yml.github/workflows/release-cli.yml.github/workflows/release-pipeline.yml.github/workflows/snyk.ymlcli/go.modcore/bifrost.gocore/go.modcore/providers/anthropic/anthropic.gocore/providers/anthropic/chat.gocore/providers/anthropic/utils.gocore/providers/anthropic/validate_chat_tools_test.gocore/providers/azure/azure.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/convert_tool_config_test.gocore/providers/bedrock/utils.gocore/providers/cohere/cohere.gocore/providers/elevenlabs/elevenlabs.gocore/providers/gemini/gemini.gocore/providers/huggingface/huggingface.gocore/providers/mistral/mistral.gocore/providers/openai/openai.gocore/providers/replicate/replicate.gocore/providers/utils/stream.gocore/providers/utils/stream_test.gocore/providers/utils/utils.gocore/providers/vertex/vertex.gocore/providers/vllm/vllm.gocore/schemas/images.gocore/schemas/videos.goexamples/plugins/hello-world/go.modframework/go.modplugins/governance/go.modplugins/jsonparser/go.modplugins/litellmcompat/go.modplugins/logging/go.modplugins/maxim/go.modplugins/mocker/go.modplugins/otel/go.modplugins/semanticcache/go.modplugins/telemetry/go.modtests/scripts/1millogs/go.modtransports/Dockerfiletransports/Dockerfile.localtransports/bifrost-http/handlers/devpprof.gotransports/changelog.mdtransports/go.modui/app/pprof/page.tsxui/lib/store/apis/devApi.ts
✅ Files skipped from review due to trivial changes (25)
- examples/plugins/hello-world/go.mod
- .github/workflows/snyk.yml
- core/go.mod
- tests/scripts/1millogs/go.mod
- .github/workflows/release-cli.yml
- plugins/telemetry/go.mod
- plugins/mocker/go.mod
- cli/go.mod
- framework/go.mod
- plugins/governance/go.mod
- plugins/logging/go.mod
- plugins/jsonparser/go.mod
- transports/go.mod
- .github/workflows/e2e-tests.yml
- .github/workflows/release-pipeline.yml
- transports/Dockerfile
- plugins/maxim/go.mod
- plugins/semanticcache/go.mod
- plugins/litellmcompat/go.mod
- plugins/otel/go.mod
- transports/Dockerfile.local
- core/providers/gemini/gemini.go
- core/providers/anthropic/chat.go
- transports/changelog.md
- core/providers/bedrock/convert_tool_config_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- core/providers/vertex/vertex.go
- core/providers/cohere/cohere.go
- core/schemas/images.go
- core/providers/elevenlabs/elevenlabs.go
- core/providers/huggingface/huggingface.go
- core/providers/replicate/replicate.go
- core/providers/azure/azure.go
- core/providers/utils/stream_test.go
- core/providers/utils/stream.go
- core/providers/utils/utils.go
- transports/bifrost-http/handlers/devpprof.go
- core/bifrost.go
- core/providers/bedrock/utils.go
akshaydeo
force-pushed
the
04-17-move_back_go_to_1.26.1
branch
from
8acabfb to
9d2ce38
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
core/providers/bedrock/convert_tool_config_test.go (2)
119-120: Prefer structural JSON assertions over substring checks.On Line 119, Line 145, Line 239, Line 290, and Line 397,
strings.Containson raw JSON is somewhat brittle. Unmarshal into a typed/map shape and assert fields directly to make tests resilient to serialization changes.♻️ Proposed refactor pattern
+func assertJSONFieldEquals[T comparable](t *testing.T, raw json.RawMessage, key string, want T) { + t.Helper() + var obj map[string]any + if err := json.Unmarshal(raw, &obj); err != nil { + t.Fatalf("failed to unmarshal json: %v", err) + } + got, ok := obj[key] + if !ok { + t.Fatalf("missing key %q in %s", key, string(raw)) + } + gotTyped, ok := got.(T) + if !ok || gotTyped != want { + t.Fatalf("unexpected value for %q: got=%v want=%v", key, got, want) + } +}- got := string(tools[0]) - if !strings.Contains(got, `"type":"bash_20250124"`) || !strings.Contains(got, `"name":"bash"`) { - t.Errorf("expected native Anthropic bash shape, got %s", got) - } + assertJSONFieldEquals[string](t, tools[0], "type", "bash_20250124") + assertJSONFieldEquals[string](t, tools[0], "name", "bash")Based on learnings: In
core/providers/bedrockunit tests, use structural/type-field assertions to avoid brittleness from dynamic string/field matching.Also applies to: 145-147, 239-241, 290-292, 397-399
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/convert_tool_config_test.go` around lines 119 - 120, Replace brittle substring assertions on the JSON output stored in variable got with structural JSON assertions: unmarshal got (using encoding/json) into a map[string]interface{} or a small struct and assert the concrete fields (e.g., check that parsed["type"] == "bash_20250124" and parsed["name"] == "bash") instead of using strings.Contains; do the same replacement for the other occurrences noted (the checks around lines 145, 239, 290, 397) so tests in convert_tool_config_test.go validate fields directly rather than matching raw serialized substrings.
17-99: Consider table-driven consolidation for similar server-tool scenarios.Several tests repeat near-identical setup/assertion scaffolding (tool construction, call, len checks). A table-driven helper would reduce duplication and make adding new Bedrock tool versions easier.
Also applies to: 101-190, 214-344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/convert_tool_config_test.go` around lines 17 - 99, Many tests in this file (e.g., TestConvertToolConfig_DropsServerToolsOnBedrock, TestConvertToolConfig_ReturnsNilWhenAllDropped, TestConvertToolConfig_KeepsBedrockSupportedServerTools and others around 101-190, 214-344) repeat identical setup/assertion scaffolding; replace the duplicated cases with a table-driven test that iterates test cases (fields: name, tools []schemas.ChatTool, expectedNil bool, expectedCount int, expectedToolName string) and calls convertToolConfig for each, and factor shared asserts into a small helper (e.g., runConvertToolConfigCase or assertToolConfig) to reduce duplication and make adding new Bedrock tool versions trivial. Ensure each original test’s intent is preserved by mapping its expectations into table rows and removing the old individual tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/bedrock/utils.go`:
- Around line 112-121: When adding tool_choice to
bedrockReq.AdditionalModelRequestFields after
collectBedrockServerToolsFromFiltered and buildBedrockServerToolChoice, skip
setting tool_choice if bifrostReq.Params contains a response_format that forces
the synthetic structured-output tool (e.g., values beginning with "bf_so_");
i.e., check bifrostReq.Params["response_format"] (or equivalent) and only call
Set("tool_choice", choice) when no forced synthetic format is present. Update
the logic around buildBedrockServerToolChoice(...) so it returns/uses the choice
only when response_format does not pin a bf_so_* tool, and add a regression test
that constructs a request with response_format set to a bf_so_* value plus a
pinned/required server tool to assert no conflicting tool_choice is sent.
In `@ui/app/pprof/page.tsx`:
- Around line 672-701: The effect that rolls in-use samples currently dedupes by
comparing lastInuseRef.current === inuse, which fails when RTK Query reuses the
same array reference; change the dedupe to use data.timestamp instead: store the
last seen timestamp (e.g., lastInuseRef.currentTimestamp) and return early if
that equals data.timestamp or if data?.inuse_allocations is missing, then update
lastInuseRef.currentTimestamp = data.timestamp before mutating
inuseHistoryRef.current and bumping historyVersion so the 60s window and
detectLeaks(input...) (called via useMemo with historyVersion) reflect every new
poll.
---
Nitpick comments:
In `@core/providers/bedrock/convert_tool_config_test.go`:
- Around line 119-120: Replace brittle substring assertions on the JSON output
stored in variable got with structural JSON assertions: unmarshal got (using
encoding/json) into a map[string]interface{} or a small struct and assert the
concrete fields (e.g., check that parsed["type"] == "bash_20250124" and
parsed["name"] == "bash") instead of using strings.Contains; do the same
replacement for the other occurrences noted (the checks around lines 145, 239,
290, 397) so tests in convert_tool_config_test.go validate fields directly
rather than matching raw serialized substrings.
- Around line 17-99: Many tests in this file (e.g.,
TestConvertToolConfig_DropsServerToolsOnBedrock,
TestConvertToolConfig_ReturnsNilWhenAllDropped,
TestConvertToolConfig_KeepsBedrockSupportedServerTools and others around
101-190, 214-344) repeat identical setup/assertion scaffolding; replace the
duplicated cases with a table-driven test that iterates test cases (fields:
name, tools []schemas.ChatTool, expectedNil bool, expectedCount int,
expectedToolName string) and calls convertToolConfig for each, and factor shared
asserts into a small helper (e.g., runConvertToolConfigCase or assertToolConfig)
to reduce duplication and make adding new Bedrock tool versions trivial. Ensure
each original test’s intent is preserved by mapping its expectations into table
rows and removing the old individual tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f3a6fb7-e11d-4687-8277-427797bfe8ba
📒 Files selected for processing (49)
.github/workflows/e2e-tests.yml.github/workflows/pr-tests.yml.github/workflows/release-cli.yml.github/workflows/release-pipeline.yml.github/workflows/snyk.ymlcli/go.modcore/bifrost.gocore/go.modcore/providers/anthropic/anthropic.gocore/providers/anthropic/chat.gocore/providers/anthropic/utils.gocore/providers/anthropic/validate_chat_tools_test.gocore/providers/azure/azure.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/convert_tool_config_test.gocore/providers/bedrock/utils.gocore/providers/cohere/cohere.gocore/providers/elevenlabs/elevenlabs.gocore/providers/gemini/gemini.gocore/providers/huggingface/huggingface.gocore/providers/mistral/mistral.gocore/providers/openai/openai.gocore/providers/replicate/replicate.gocore/providers/utils/stream.gocore/providers/utils/stream_test.gocore/providers/utils/utils.gocore/providers/vertex/vertex.gocore/providers/vllm/vllm.gocore/schemas/images.gocore/schemas/videos.goexamples/plugins/hello-world/go.modframework/go.modplugins/governance/go.modplugins/jsonparser/go.modplugins/litellmcompat/go.modplugins/logging/go.modplugins/maxim/go.modplugins/mocker/go.modplugins/otel/go.modplugins/semanticcache/go.modplugins/telemetry/go.modtests/scripts/1millogs/go.modtransports/Dockerfiletransports/Dockerfile.localtransports/bifrost-http/handlers/devpprof.gotransports/changelog.mdtransports/go.modui/app/pprof/page.tsxui/lib/store/apis/devApi.ts
✅ Files skipped from review due to trivial changes (27)
- .github/workflows/pr-tests.yml
- cli/go.mod
- core/go.mod
- .github/workflows/e2e-tests.yml
- plugins/mocker/go.mod
- plugins/maxim/go.mod
- plugins/governance/go.mod
- framework/go.mod
- examples/plugins/hello-world/go.mod
- plugins/logging/go.mod
- .github/workflows/snyk.yml
- plugins/telemetry/go.mod
- core/providers/mistral/mistral.go
- plugins/otel/go.mod
- transports/go.mod
- plugins/jsonparser/go.mod
- .github/workflows/release-pipeline.yml
- plugins/semanticcache/go.mod
- transports/Dockerfile
- core/providers/azure/azure.go
- core/providers/replicate/replicate.go
- core/providers/gemini/gemini.go
- transports/Dockerfile.local
- tests/scripts/1millogs/go.mod
- plugins/litellmcompat/go.mod
- transports/changelog.md
- core/providers/anthropic/utils.go
🚧 Files skipped from review as they are similar to previous changes (9)
- core/schemas/videos.go
- .github/workflows/release-cli.yml
- core/providers/elevenlabs/elevenlabs.go
- core/providers/anthropic/chat.go
- core/providers/bedrock/bedrock.go
- core/providers/utils/stream_test.go
- core/schemas/images.go
- core/bifrost.go
- transports/bifrost-http/handlers/devpprof.go
akshaydeo
force-pushed
the
04-17-dependabot_fixes
branch
from
01425fe to
f54cfce
Compare
akshaydeo
force-pushed
the
04-17-move_back_go_to_1.26.1
branch
2 times, most recently
from
86283f8 to
a3146cc
Compare
akshaydeo
force-pushed
the
04-17-dependabot_fixes
branch
from
f54cfce to
3e611df
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/anthropic/chat.go (1)
401-425:⚠️ Potential issue | 🟠 MajorReconcile
tool_choiceafter filtering tools.Tool filtering at line 409 can drop unsupported server tools, but
ToolChoiceis derived from the original request at lines 428-458 without validating against the filtered set. If a user specifiestool_choicewith a function name that gets filtered out, the resultinganthropicReqbecomes internally inconsistent—it references a tool inToolChoice.Namethat doesn't exist inTools—causing an upstream provider error instead of graceful degradation.Track the names of kept tools during filtering, then clear or downgrade
ToolChoiceif the selected tool was filtered out or if all tools were filtered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/chat.go` around lines 401 - 425, The tool-filtering block that calls ValidateChatToolsForProvider and builds anthropicReq.Tools via convertFunctionToolToAnthropic/convertServerToolToAnthropic can remove the tool referenced by bifrostReq.Params.ToolChoice.Name, leaving anthropicReq.ToolChoice inconsistent; after building the kept tools list, collect their names into a set and then reconcile anthropicReq.ToolChoice: if ToolChoice is nil do nothing, if ToolChoice.Name is not in the kept set or if no tools survived, clear/downgrade anthropicReq.ToolChoice (set to nil or to a safe default) so anthropicReq does not reference a non-existent tool. Ensure this reconciliation runs immediately after the code that appends to anthropicReq.Tools.
♻️ Duplicate comments (1)
core/providers/bedrock/utils.go (1)
120-121:⚠️ Potential issue | 🟠 MajorAvoid emitting two conflicting
tool_choicedirectives whenresponse_formatforcesbf_so_*.At Line 121 you may tunnel
additionalModelRequestFields.tool_choice, but at Line 271 structured output always sets typedToolConfig.ToolChoice. Inresponse_format + pinned/required server-toolrequests this produces competing tool-choice directives. Gate the tunneled path whenresponseFormatTool != nil, and add a regression for that combination.🩹 Minimal fix
- if choice, ok := buildBedrockServerToolChoice(bifrostReq.Params, filteredTools); ok { - bedrockReq.AdditionalModelRequestFields.Set("tool_choice", choice) - } + if responseFormatTool == nil { + if choice, ok := buildBedrockServerToolChoice(bifrostReq.Params, filteredTools); ok { + bedrockReq.AdditionalModelRequestFields.Set("tool_choice", choice) + } + }Also applies to: 264-275
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/utils.go` around lines 120 - 121, The code currently tunnels a tool choice into bedrockReq.AdditionalModelRequestFields via buildBedrockServerToolChoice even when a structured response format already sets ToolConfig.ToolChoice (causing conflicting directives); modify the path that calls buildBedrockServerToolChoice and bedrockReq.AdditionalModelRequestFields.Set("tool_choice", ...) to only run when responseFormatTool == nil (i.e., no typed response-format tool is present), and add a regression test that constructs a request with response_format that pins/forces a bf_so_* tool plus a server-tool in params to ensure no duplicate/conflicting tool_choice is emitted.
🧹 Nitpick comments (1)
core/providers/bedrock/convert_tool_config_test.go (1)
118-121: Prefer structural JSON assertions over substring checks.Using
strings.Containson raw JSON can miss shape/type regressions. Decode and assert fields structurally for stronger unit-test guarantees.♻️ Suggested refactor pattern
+func mustUnmarshalRawMessage(t *testing.T, raw json.RawMessage) map[string]any { + t.Helper() + var m map[string]any + if err := json.Unmarshal(raw, &m); err != nil { + t.Fatalf("failed to unmarshal json: %v", err) + } + return m +} ... - got := string(tools[0]) - if !strings.Contains(got, `"type":"bash_20250124"`) || !strings.Contains(got, `"name":"bash"`) { - t.Errorf("expected native Anthropic bash shape, got %s", got) - } + got := mustUnmarshalRawMessage(t, tools[0]) + if got["type"] != "bash_20250124" || got["name"] != "bash" { + t.Errorf("expected native Anthropic bash shape, got %+v", got) + }Based on learnings: In core/providers/bedrock tests, unit tests should perform structural comparisons and type/field checks to avoid brittleness from dynamic fields.
Also applies to: 145-147, 239-241, 290-292, 397-399
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/convert_tool_config_test.go` around lines 118 - 121, Replace brittle substring assertions on the JSON blob with a structural JSON check: unmarshal tools[0] (the byte/[]byte used to build got) into a map[string]interface{} or a small struct and assert the "type" and "name" fields exactly equal "bash_20250124" and "bash" (instead of using strings.Contains on got). Update the same pattern at the other locations mentioned (lines ~145-147, 239-241, 290-292, 397-399) to unmarshal and assert specific fields to ensure robust structural tests; use the variables tools and got to locate the test payloads and replace strings.Contains-based checks with explicit field assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/utils/stream.go`:
- Around line 62-71: Replace the blocking "for chunk := range stream" range with
an explicit receive/select loop so ctx.Done() is observed even if no new chunks
arrive: inside the loop use "select { case chunk, ok := <-stream: ... case
<-ctx.Done(): ... }" to handle both incoming chunks and cancellation; when
ctx.Done() fires, spawn or run the same drain logic over the original "stream"
(for range stream { } or for { if _, ok := <-stream; !ok { break } }) to unblock
the provider's send, close/return as appropriate, and ensure sends to "wrapped"
happen only when "ok" is true and on the non-cancel path.
---
Outside diff comments:
In `@core/providers/anthropic/chat.go`:
- Around line 401-425: The tool-filtering block that calls
ValidateChatToolsForProvider and builds anthropicReq.Tools via
convertFunctionToolToAnthropic/convertServerToolToAnthropic can remove the tool
referenced by bifrostReq.Params.ToolChoice.Name, leaving anthropicReq.ToolChoice
inconsistent; after building the kept tools list, collect their names into a set
and then reconcile anthropicReq.ToolChoice: if ToolChoice is nil do nothing, if
ToolChoice.Name is not in the kept set or if no tools survived, clear/downgrade
anthropicReq.ToolChoice (set to nil or to a safe default) so anthropicReq does
not reference a non-existent tool. Ensure this reconciliation runs immediately
after the code that appends to anthropicReq.Tools.
---
Duplicate comments:
In `@core/providers/bedrock/utils.go`:
- Around line 120-121: The code currently tunnels a tool choice into
bedrockReq.AdditionalModelRequestFields via buildBedrockServerToolChoice even
when a structured response format already sets ToolConfig.ToolChoice (causing
conflicting directives); modify the path that calls buildBedrockServerToolChoice
and bedrockReq.AdditionalModelRequestFields.Set("tool_choice", ...) to only run
when responseFormatTool == nil (i.e., no typed response-format tool is present),
and add a regression test that constructs a request with response_format that
pins/forces a bf_so_* tool plus a server-tool in params to ensure no
duplicate/conflicting tool_choice is emitted.
---
Nitpick comments:
In `@core/providers/bedrock/convert_tool_config_test.go`:
- Around line 118-121: Replace brittle substring assertions on the JSON blob
with a structural JSON check: unmarshal tools[0] (the byte/[]byte used to build
got) into a map[string]interface{} or a small struct and assert the "type" and
"name" fields exactly equal "bash_20250124" and "bash" (instead of using
strings.Contains on got). Update the same pattern at the other locations
mentioned (lines ~145-147, 239-241, 290-292, 397-399) to unmarshal and assert
specific fields to ensure robust structural tests; use the variables tools and
got to locate the test payloads and replace strings.Contains-based checks with
explicit field assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8bbcad7a-6e92-4b25-bc4e-1d782e211d17
📒 Files selected for processing (49)
.github/workflows/e2e-tests.yml.github/workflows/pr-tests.yml.github/workflows/release-cli.yml.github/workflows/release-pipeline.yml.github/workflows/snyk.ymlcli/go.modcore/bifrost.gocore/go.modcore/providers/anthropic/anthropic.gocore/providers/anthropic/chat.gocore/providers/anthropic/utils.gocore/providers/anthropic/validate_chat_tools_test.gocore/providers/azure/azure.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/convert_tool_config_test.gocore/providers/bedrock/utils.gocore/providers/cohere/cohere.gocore/providers/elevenlabs/elevenlabs.gocore/providers/gemini/gemini.gocore/providers/huggingface/huggingface.gocore/providers/mistral/mistral.gocore/providers/openai/openai.gocore/providers/replicate/replicate.gocore/providers/utils/stream.gocore/providers/utils/stream_test.gocore/providers/utils/utils.gocore/providers/vertex/vertex.gocore/providers/vllm/vllm.gocore/schemas/images.gocore/schemas/videos.goexamples/plugins/hello-world/go.modframework/go.modplugins/governance/go.modplugins/jsonparser/go.modplugins/litellmcompat/go.modplugins/logging/go.modplugins/maxim/go.modplugins/mocker/go.modplugins/otel/go.modplugins/semanticcache/go.modplugins/telemetry/go.modtests/scripts/1millogs/go.modtransports/Dockerfiletransports/Dockerfile.localtransports/bifrost-http/handlers/devpprof.gotransports/changelog.mdtransports/go.modui/app/pprof/page.tsxui/lib/store/apis/devApi.ts
✅ Files skipped from review due to trivial changes (24)
- tests/scripts/1millogs/go.mod
- .github/workflows/e2e-tests.yml
- .github/workflows/pr-tests.yml
- cli/go.mod
- core/go.mod
- plugins/otel/go.mod
- examples/plugins/hello-world/go.mod
- plugins/logging/go.mod
- plugins/governance/go.mod
- plugins/semanticcache/go.mod
- framework/go.mod
- transports/go.mod
- transports/Dockerfile
- plugins/maxim/go.mod
- plugins/litellmcompat/go.mod
- .github/workflows/release-cli.yml
- .github/workflows/snyk.yml
- transports/Dockerfile.local
- core/providers/vertex/vertex.go
- plugins/jsonparser/go.mod
- .github/workflows/release-pipeline.yml
- plugins/telemetry/go.mod
- plugins/mocker/go.mod
- ui/app/pprof/page.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- core/providers/vllm/vllm.go
- core/providers/anthropic/anthropic.go
- core/providers/utils/stream_test.go
- core/providers/replicate/replicate.go
- core/providers/azure/azure.go
- core/providers/gemini/gemini.go
- transports/changelog.md
- core/providers/anthropic/utils.go
- core/bifrost.go
- transports/bifrost-http/handlers/devpprof.go
akshaydeo
force-pushed
the
04-17-move_back_go_to_1.26.1
branch
from
a3146cc to
bddb70e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
core/providers/utils/stream.go (1)
62-71:⚠️ Potential issue | 🟠 MajorCancellation is still not observed while blocked on source receive.
At Line 62,
for chunk := range streamblocks until a chunk/close arrives, soctx.Done()cannot interrupt an idle/stalled source. This contradicts the intended cancellation behavior and can leak the forwarding goroutine.Proposed fix
- for chunk := range stream { - select { - case wrapped <- chunk: - case <-ctx.Done(): - // Consumer abandoned the wrapped channel. Drain the source so the - // provider's blocked send unblocks and its goroutine can exit. - for range stream { - } - return - } - } + for { + select { + case <-ctx.Done(): + // Consumer abandoned the wrapped channel. Drain the source so the + // provider's blocked send unblocks and its goroutine can exit. + for range stream { + } + return + case chunk, ok := <-stream: + if !ok { + return + } + select { + case wrapped <- chunk: + case <-ctx.Done(): + for range stream { + } + return + } + } + }#!/bin/bash # Verify whether forwarding loop is cancellation-aware while waiting for source input. # Expected after fix: no plain `for chunk := range stream` in this forwarding path. rg -n -A8 -B3 'for chunk := range stream|case <-ctx.Done\(\)' core/providers/utils/stream.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/utils/stream.go` around lines 62 - 71, The forwarding goroutine uses "for chunk := range stream" which blocks on the source receive and therefore cannot observe ctx.Done(), risking a leak; replace the range receive with a select that listens on both ctx.Done() and the source channel (stream) so cancellation can interrupt a stalled source, ensure you still drain the source when the consumer abandons wrapped, and preserve the existing behavior of sending chunks into wrapped; update the forwarding loop around variables stream, wrapped, ctx to use a select case for receiving from stream (e.g., case chunk, ok := <-stream) and a case <-ctx.Done() to return/cleanup.
🧹 Nitpick comments (2)
core/providers/bedrock/convert_tool_config_test.go (1)
120-121: Prefer structural JSON assertions over substring matching.These checks can pass even when payload shape is wrong (or fail on harmless formatting/serialization shifts). Decode to typed/map structures and assert fields directly for stronger guarantees.
Based on learnings: In `core/providers/bedrock` tests, unit tests should prefer structural comparisons/type-field checks to avoid brittleness from string/field matching.🧪 Suggested pattern (apply across similar assertions)
- got := string(tools[0]) - if !strings.Contains(got, `"type":"bash_20250124"`) || !strings.Contains(got, `"name":"bash"`) { - t.Errorf("expected native Anthropic bash shape, got %s", got) - } + var got map[string]any + if err := json.Unmarshal(tools[0], &got); err != nil { + t.Fatalf("failed to decode server tool payload: %v", err) + } + if got["type"] != "bash_20250124" || got["name"] != "bash" { + t.Errorf("unexpected server tool payload: %+v", got) + }Also applies to: 146-147, 240-242, 291-293, 398-400
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/convert_tool_config_test.go` around lines 120 - 121, Replace brittle substring checks on the JSON string variable got with structural JSON assertions: unmarshal got into a map[string]interface{} (or the exact struct if available) and assert that the keys "type" equals "bash_20250124" and "name" equals "bash" (and similarly update the other occurrences mentioned). Locate the test(s) using the got variable in convert_tool_config_test.go and replace the strings.Contains assertions with direct checks against the unmarshaled object's fields, returning a helpful t.Errorf if the fields are missing or have unexpected values.core/providers/elevenlabs/elevenlabs.go (1)
407-430: MoveEnsureStreamFinalizerCalledto the first defer in this goroutine.At Line 429, this defer currently executes before the cancellation/timeout-close defer chain (LIFO). In this stack, the helper is being added as the first defer so it runs last, after stream shutdown handling and channel close.
♻️ Suggested adjustment
go func() { + defer providerUtils.EnsureStreamFinalizerCalled(ctx) defer func() { if ctx.Err() == context.Canceled { providerUtils.HandleStreamCancellation(ctx, postHookRunner, responseChan, providerName, request.Model, schemas.SpeechStreamRequest, provider.logger) } else if ctx.Err() == context.DeadlineExceeded { providerUtils.HandleStreamTimeout(ctx, postHookRunner, responseChan, providerName, request.Model, schemas.SpeechStreamRequest, provider.logger) } close(responseChan) }() defer providerUtils.ReleaseStreamingResponse(resp) @@ stopCancellation := providerUtils.SetupStreamCancellation(ctx, resp.BodyStream(), provider.logger) defer stopCancellation() - defer providerUtils.EnsureStreamFinalizerCalled(ctx)As per coding guidelines: "
**: 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."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/elevenlabs/elevenlabs.go` around lines 407 - 430, The EnsureStreamFinalizerCalled defer should be the first defer inside the goroutine so it is registered before the other cleanup defers; move the call to providerUtils.EnsureStreamFinalizerCalled(ctx) to the top of the goroutine's defer block (i.e., make "defer providerUtils.EnsureStreamFinalizerCalled(ctx)" the first defer statement) so it is executed last after the cancellation/timeout handlers, ReleaseStreamingResponse, gzip release, idle timeout stop, cancellation stop and channel close; update the goroutine containing providerUtils.ReleaseStreamingResponse, DecompressStreamBody, NewIdleTimeoutReader, SetupStreamCancellation and close(responseChan) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/changelog.md`:
- Around line 4-14: Changelog is missing the Go downgrade and explicit
stream-cancellation notes described in the PR: add a short bullet under the
release notes (near the existing “Provider Queue Shutdown Panic” and “Responses
Streaming Errors” entries) stating the Go toolchain was downgraded from 1.26.2
to 1.26.1 (reflecting go.mod), and add a separate bullet titled “Stream
cancellation safety” (or similar) describing guarded channel sends, finalizer
protection, and context cancellation support to properly drain upstream
channels; reference those exact phrases so it matches the PR summary.
In `@ui/app/pprof/page.tsx`:
- Around line 118-126: Change the leak history from a plain number[] and
fixed-sample-count window to time-stamped samples and a wall-clock eviction
policy: replace the samples: number[] shape with samples: { bytes: number; ts:
number }[] (or similar) and stop using LEAK_MAX_SAMPLES to determine the window;
instead keep samples whose ts >= now - WINDOW_MS (e.g., 60_000) whenever you
push a new sample or on refetch(), compute growth and "Last …s" from the
earliest and latest ts in samples, and adjust the growth detection logic
(isGrowing, growthBytes, severity) to use these time-stamped values and a
minimum-duration threshold (instead of LEAK_MIN_GROWTH_SAMPLES) to avoid false
positives on rapid manual refreshes. Ensure any code referencing
LEAK_MAX_SAMPLES or LEAK_MIN_GROWTH_SAMPLES is updated to use the new WINDOW_MS
and minimum-duration checks.
- Around line 693-697: The current loop that deletes entries from map when a key
is not in seen drops history on a single miss; instead, track consecutive misses
per site and only prune after a grace period (e.g., one full time window or N
consecutive misses). Add a miss counter or last-seen timestamp alongside map
entries (use a new missCounts Map or extend the stored value), increment the
counter when a key is absent in seen, reset it when present, and change the
deletion condition in the for (const key of [...map.keys()]) loop to only
map.delete(key) when the counter exceeds the configured threshold (or last-seen
is older than one window); reference map and seen to locate where to update
logic.
---
Duplicate comments:
In `@core/providers/utils/stream.go`:
- Around line 62-71: The forwarding goroutine uses "for chunk := range stream"
which blocks on the source receive and therefore cannot observe ctx.Done(),
risking a leak; replace the range receive with a select that listens on both
ctx.Done() and the source channel (stream) so cancellation can interrupt a
stalled source, ensure you still drain the source when the consumer abandons
wrapped, and preserve the existing behavior of sending chunks into wrapped;
update the forwarding loop around variables stream, wrapped, ctx to use a select
case for receiving from stream (e.g., case chunk, ok := <-stream) and a case
<-ctx.Done() to return/cleanup.
---
Nitpick comments:
In `@core/providers/bedrock/convert_tool_config_test.go`:
- Around line 120-121: Replace brittle substring checks on the JSON string
variable got with structural JSON assertions: unmarshal got into a
map[string]interface{} (or the exact struct if available) and assert that the
keys "type" equals "bash_20250124" and "name" equals "bash" (and similarly
update the other occurrences mentioned). Locate the test(s) using the got
variable in convert_tool_config_test.go and replace the strings.Contains
assertions with direct checks against the unmarshaled object's fields, returning
a helpful t.Errorf if the fields are missing or have unexpected values.
In `@core/providers/elevenlabs/elevenlabs.go`:
- Around line 407-430: The EnsureStreamFinalizerCalled defer should be the first
defer inside the goroutine so it is registered before the other cleanup defers;
move the call to providerUtils.EnsureStreamFinalizerCalled(ctx) to the top of
the goroutine's defer block (i.e., make "defer
providerUtils.EnsureStreamFinalizerCalled(ctx)" the first defer statement) so it
is executed last after the cancellation/timeout handlers,
ReleaseStreamingResponse, gzip release, idle timeout stop, cancellation stop and
channel close; update the goroutine containing
providerUtils.ReleaseStreamingResponse, DecompressStreamBody,
NewIdleTimeoutReader, SetupStreamCancellation and close(responseChan)
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92102e6f-f71c-43b4-b9a8-e80a2c3b0853
📒 Files selected for processing (50)
.github/workflows/e2e-tests.yml.github/workflows/pr-tests.yml.github/workflows/release-cli.yml.github/workflows/release-pipeline.yml.github/workflows/snyk.ymlREADME.mdcli/go.modcore/bifrost.gocore/go.modcore/providers/anthropic/anthropic.gocore/providers/anthropic/chat.gocore/providers/anthropic/utils.gocore/providers/anthropic/validate_chat_tools_test.gocore/providers/azure/azure.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/convert_tool_config_test.gocore/providers/bedrock/utils.gocore/providers/cohere/cohere.gocore/providers/elevenlabs/elevenlabs.gocore/providers/gemini/gemini.gocore/providers/huggingface/huggingface.gocore/providers/mistral/mistral.gocore/providers/openai/openai.gocore/providers/replicate/replicate.gocore/providers/utils/stream.gocore/providers/utils/stream_test.gocore/providers/utils/utils.gocore/providers/vertex/vertex.gocore/providers/vllm/vllm.gocore/schemas/images.gocore/schemas/videos.goexamples/plugins/hello-world/go.modframework/go.modplugins/governance/go.modplugins/jsonparser/go.modplugins/litellmcompat/go.modplugins/logging/go.modplugins/maxim/go.modplugins/mocker/go.modplugins/otel/go.modplugins/semanticcache/go.modplugins/telemetry/go.modtests/scripts/1millogs/go.modtransports/Dockerfiletransports/Dockerfile.localtransports/bifrost-http/handlers/devpprof.gotransports/changelog.mdtransports/go.modui/app/pprof/page.tsxui/lib/store/apis/devApi.ts
💤 Files with no reviewable changes (1)
- README.md
✅ Files skipped from review due to trivial changes (25)
- cli/go.mod
- examples/plugins/hello-world/go.mod
- framework/go.mod
- plugins/logging/go.mod
- core/go.mod
- .github/workflows/snyk.yml
- plugins/jsonparser/go.mod
- tests/scripts/1millogs/go.mod
- plugins/governance/go.mod
- .github/workflows/release-pipeline.yml
- plugins/litellmcompat/go.mod
- .github/workflows/pr-tests.yml
- .github/workflows/release-cli.yml
- plugins/mocker/go.mod
- plugins/maxim/go.mod
- core/providers/azure/azure.go
- plugins/otel/go.mod
- core/providers/vertex/vertex.go
- core/providers/mistral/mistral.go
- plugins/telemetry/go.mod
- .github/workflows/e2e-tests.yml
- transports/go.mod
- transports/Dockerfile
- transports/Dockerfile.local
- core/providers/bedrock/bedrock.go
🚧 Files skipped from review as they are similar to previous changes (12)
- plugins/semanticcache/go.mod
- core/providers/vllm/vllm.go
- core/providers/huggingface/huggingface.go
- ui/lib/store/apis/devApi.ts
- core/providers/replicate/replicate.go
- core/providers/anthropic/anthropic.go
- core/schemas/images.go
- core/providers/anthropic/validate_chat_tools_test.go
- core/providers/anthropic/utils.go
- core/bifrost.go
- transports/bifrost-http/handlers/devpprof.go
- core/providers/bedrock/utils.go
akshaydeo
force-pushed
the
04-17-move_back_go_to_1.26.1
branch
from
bddb70e to
74df3b6
Compare
akshaydeo
force-pushed
the
04-17-dependabot_fixes
branch
from
3e611df to
d1b6c72
Compare
Contributor
Author
|
@greptile |
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
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)
core/providers/gemini/gemini.go (1)
4563-4609:⚠️ Potential issue | 🟠 MajorUse the centralized
EnsureStreamFinalizerCalledhelper for finalization inPassthroughStream.The EOF path at lines 4607–4609 directly invokes the post-hook span finalizer via
ctx.Value(), creating a second finalization path alongside the deferredEnsureStreamFinalizerCalled(ctx)at line 4564. All other streaming methods (ChatStream, ResponsesStream, SpeechStream, TranscriptionStream) rely exclusively on the deferred helper and do not bypass it. Remove the direct finalizer invocation and let the deferred helper handle all finalization paths consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/gemini/gemini.go` around lines 4563 - 4609, In PassthroughStream's EOF handling, remove the direct post-hook span finalizer invocation that fetches and calls ctx.Value(schemas.BifrostContextKeyPostHookSpanFinalizer) so finalization isn't executed twice; keep the postHookRunner(ctx, finalResp, nil) call and rely on the already-deferred providerUtils.EnsureStreamFinalizerCalled(ctx) to run the span finalizer for all stream termination paths, updating any comments if needed to note that finalization is centralized.core/providers/bedrock/utils.go (1)
1346-1428:⚠️ Potential issue | 🟠 MajorCustom tools are still dropped on the Bedrock conversion path.
This loop only materializes
tool.Function.tool.Customalready gets excluded fromcollectBedrockServerToolsFromFiltered, andbuildBedrockServerToolChoicetreats custom tools as if the typed path can carry them, so a request with only custom tools now ends up withToolConfig == niland no fallbacktool_choice. Please either serializetool.Customhere as well, or stop counting custom tools as supported in the other new helpers so they fail/strip consistently instead of disappearing silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/utils.go` around lines 1346 - 1428, The conversion loop only handles tool.Function, causing tool.Custom entries to be dropped; update the loop in the Bedrock conversion (the code that builds bedrockTools and later reconciles against params.ToolChoice) to also handle tool.Custom by creating a corresponding BedrockTool/BedrockToolSpec and BedrockToolInputSchema (serialize the custom tool's parameter/schema into json.RawMessage and use its description/name similar to how tool.Function is handled), and ensure the Name used for reconciliation matches the custom tool name so buildBedrockServerToolChoice and collectBedrockServerToolsFromFiltered remain consistent; alternatively, if custom tools should not be supported, change collectBedrockServerToolsFromFiltered and buildBedrockServerToolChoice to exclude/count them consistently so they are stripped upstream rather than silently disappearing.
♻️ Duplicate comments (6)
transports/changelog.md (1)
4-14:⚠️ Potential issue | 🟡 MinorGo version downgrade and stream cancellation safety still missing from changelog.
The past review correctly identified that the changelog is missing:
Go toolchain downgrade: The PR downgrades Go from 1.26.2 to 1.26.1 across go.mod files, workflows, and Dockerfiles, but this isn't documented anywhere in the changelog.
Stream cancellation safety improvements: The PR summary mentions "stream cancellation safety with guarded channel sends and finalizer protection" and "context cancellation support to properly drain upstream channels," but these aren't explicitly called out. While "Provider Queue Shutdown Panic" (line 10) and "Responses Streaming Errors" (line 20) touch on streaming, they don't capture the specific finalizer (
EnsureStreamFinalizerCalled) and context-aware draining improvements.Add a bullet documenting the Go version change and another explicitly describing the stream cancellation safety enhancements to match the PR description.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/changelog.md` around lines 4 - 14, Add two explicit bullets to the changelog: one noting the Go toolchain downgrade from 1.26.2 to 1.26.1 (applies to go.mod files, CI workflows, and Dockerfiles), and one describing the stream cancellation safety improvements (mention guarded channel sends, context-aware draining of upstream channels, and the EnsureStreamFinalizerCalled finalizer to guarantee cleanup); update the "Fixed" or appropriate section near the existing "Provider Queue Shutdown Panic" and "Responses Streaming Errors" entries so reviewers can see the Go downgrade and the specific finalizer/context-drain changes.core/providers/utils/stream.go (1)
62-71:⚠️ Potential issue | 🟠 MajorCancellation can still hang while blocked on source receive.
At Line 62,
for chunk := range streamcan still block indefinitely;ctx.Done()is only checked after a chunk is received. If upstream stalls, cancellation won’t trigger drain/exit promptly.Suggested fix
- for chunk := range stream { - select { - case wrapped <- chunk: - case <-ctx.Done(): - // Consumer abandoned the wrapped channel. Drain the source so the - // provider's blocked send unblocks and its goroutine can exit. - for range stream { - } - return - } - } + for { + select { + case <-ctx.Done(): + // Consumer abandoned the wrapped channel. Drain the source so the + // provider's blocked send unblocks and its goroutine can exit. + for range stream { + } + return + case chunk, ok := <-stream: + if !ok { + return + } + select { + case wrapped <- chunk: + case <-ctx.Done(): + for range stream { + } + return + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/utils/stream.go` around lines 62 - 71, The loop reading from the source channel `stream` can block and delay cancellation because it does a range read without checking `ctx.Done()`; change the loop to a select-based receive so cancellation is observed while waiting for the next item. Replace `for chunk := range stream` with a `for` that does `select { case <-ctx.Done(): /* stop/return */; case chunk, ok := <-stream: if !ok { return } ... }` and keep the existing send-to-`wrapped` logic (also using a select to handle `ctx.Done()` when sending). Ensure that when the consumer abandons `wrapped` you still drain `stream` to unblock the provider, and use the `ok` flag from `chunk, ok := <-stream` to detect closed source. This references the variables `ctx`, `stream`, and `wrapped` in core/providers/utils/stream.go.core/providers/elevenlabs/elevenlabs.go (1)
407-430:⚠️ Potential issue | 🟠 MajorMove
EnsureStreamFinalizerCalledto the first defer in this goroutine.At Line 429 it is registered too late. With Go’s LIFO defer order, it runs before
stopCancellation()and before the cancellation/timeout defer block, which can finalize stream post-hooks prematurely.🔧 Proposed fix
go func() { + defer providerUtils.EnsureStreamFinalizerCalled(ctx) defer func() { if ctx.Err() == context.Canceled { providerUtils.HandleStreamCancellation(ctx, postHookRunner, responseChan, providerName, request.Model, schemas.SpeechStreamRequest, provider.logger) } else if ctx.Err() == context.DeadlineExceeded { providerUtils.HandleStreamTimeout(ctx, postHookRunner, responseChan, providerName, request.Model, schemas.SpeechStreamRequest, provider.logger) @@ stopCancellation := providerUtils.SetupStreamCancellation(ctx, resp.BodyStream(), provider.logger) defer stopCancellation() - defer providerUtils.EnsureStreamFinalizerCalled(ctx)#!/bin/bash # Verify defer ordering around SpeechStream goroutine finalization. rg -n -C6 --type go --iglob '*elevenlabs.go' 'go func\(\)|defer providerUtils\.EnsureStreamFinalizerCalled|defer stopCancellation|HandleStreamCancellation|HandleStreamTimeout'Expected result: in this goroutine,
defer providerUtils.EnsureStreamFinalizerCalled(ctx)should appear immediately aftergo func() {so it runs last.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/elevenlabs/elevenlabs.go` around lines 407 - 430, The defer for providerUtils.EnsureStreamFinalizerCalled(ctx) is registered too late inside the goroutine so it runs before stopCancellation and the cancellation/timeout handlers; move defer providerUtils.EnsureStreamFinalizerCalled(ctx) to be the first defer immediately after the start of the goroutine (i.e., directly after go func() {) so its call is executed last; keep all other defers—ReleaseStreamingResponse(resp), releaseGzip(), stopIdleTimeout(), stopCancellation(), and the cancellation/timeout closure that calls HandleStreamCancellation/HandleStreamTimeout—unchanged in their current relative order.core/providers/anthropic/chat.go (1)
412-469:⚠️ Potential issue | 🟠 MajorReconcile named
tool_choicewith the final tool list.
ValidateChatToolsForProvidercan strip tools here, butanthropicReq.ToolChoiceis still derived later from the original request. If the caller forced one of the dropped tools, the request goes upstream withtool_choice.namepointing at a tool that is no longer present inanthropicReq.Tools, which turns the silent-strip behavior into a 4xx. Please clear or downgrade the named choice when its target is absent from the final tool list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/chat.go` around lines 412 - 469, The tool-choice name from bifrostReq.Params.ToolChoice can reference a tool removed by ValidateChatToolsForProvider, causing anthropicReq.ToolChoice.Name to point to a missing entry; after building anthropicReq.Tools (and before assigning anthropicReq.ToolChoice), check if toolChoice.Name (from bifrostReq.Params.ToolChoice.ChatToolChoiceStruct.Function.Name) exists in the final anthropicReq.Tools slice (compare by tool name/identifier), and if it does not, either clear toolChoice.Name and set toolChoice.Type to a safe fallback ("none" or "auto") or downgrade the choice to a non-named type; update the logic around ValidateChatToolsForProvider, anthropicReq.Tools and the block that constructs anthropicReq.ToolChoice to perform this validation and mutation.core/bifrost.go (2)
5227-5236:⚠️ Potential issue | 🔴 CriticalRoute stream-error cleanup through this finalizer too.
This
sync.Onceonly protects callers that invokepostHookSpanFinalizer. The first-chunk SSE error path still reaches Line 5255 and callsbifrost.releasePluginPipeline(pipeline)directly, so the provider goroutine can finalize/release once and this path can release the same pipeline again.💡 Suggested direction
- if pipeline != nil && (!IsStreamRequestType(req.RequestType) || bifrostError != nil) { - bifrost.releasePluginPipeline(pipeline) - } + if pipeline != nil { + if IsStreamRequestType(req.RequestType) { + if bifrostError != nil { + if finalizer, ok := req.Context.Value(schemas.BifrostContextKeyPostHookSpanFinalizer).(func(context.Context)); ok && finalizer != nil { + finalizer(req.Context) + } + } + } else { + bifrost.releasePluginPipeline(pipeline) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 5227 - 5236, The first-chunk SSE error path directly calls bifrost.releasePluginPipeline(pipeline) and bypasses the sync.Once-protected postHookSpanFinalizer, allowing double-release; modify the error path to call postHookSpanFinalizer(ctx) instead of calling releasePluginPipeline directly (so finalizerOnce.Do runs FinalizeStreamingPostHookSpans and releasePluginPipeline exactly once), and remove or replace any other direct calls to bifrost.releasePluginPipeline(pipeline) in that error flow to ensure the finalizer is always used and the context is passed through.
4692-4701:⚠️ Potential issue | 🔴 CriticalKeep the pre-hook pipeline alive until the short-circuit stream goroutine exits.
The guarded send is fine, but
pipelinePostHookRunnerstill closes overpipelinewhile Line 4635 returns that samePluginPipelineto the pool as soon astryStreamRequestreturnsoutputStream. That lets another request reuse/reset the pipeline while this goroutine is still running post-hooks.💡 Suggested direction
pipeline := bifrost.getPluginPipeline() - defer bifrost.releasePluginPipeline(pipeline) + releaseInGoroutine := false + defer func() { + if !releaseInGoroutine { + bifrost.releasePluginPipeline(pipeline) + } + }() preReq, shortCircuit, preCount := pipeline.RunLLMPreHooks(ctx, req) if shortCircuit != nil { ... if shortCircuit.Stream != nil { outputStream := make(chan *schemas.BifrostStreamChunk) + releaseInGoroutine = true go func() { + defer bifrost.releasePluginPipeline(pipeline) defer close(outputStream) for streamMsg := range shortCircuit.Stream { ... } }() return outputStream, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 4692 - 4701, The post-hook goroutine is closing over the PluginPipeline while tryStreamRequest returns the pipeline to the pool, allowing reuse before post-hooks finish; to fix, ensure the pipeline is held until pipelinePostHookRunner completes by delaying return-to-pool or explicit release: modify pipelinePostHookRunner (and the guarded send goroutine that reads shortCircuit.Stream/outputStream) to take ownership of the PluginPipeline and call the pipeline release/reset after the post-hook processing and stream draining completes (or use a sync.WaitGroup or channel to signal completion before tryStreamRequest returns outputStream), referencing pipelinePostHookRunner, tryStreamRequest, PluginPipeline, outputStream, and shortCircuit.Stream to find where to move the release.
🧹 Nitpick comments (3)
ui/app/pprof/page.tsx (1)
491-495: "Last …s" label assumes consistent 10-second polling.The calculation
c.samples.length * 10will be inaccurate if manualrefetch()calls occur, showing "Last 60s" when the actual window might be much shorter. This ties to the broader wall-clock tracking issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/pprof/page.tsx` around lines 491 - 495, The label "Last {c.samples.length * 10}s" assumes 10s polling and is inaccurate when refetch() shortens the window; update the UI to compute the real wall-clock window from sample timestamps instead of assuming 10s per sample: when c.samples contains timestamped entries, compute seconds = Math.round((lastSample.timestamp - firstSample.timestamp) / 1000) and display "Last {seconds}s"; if samples are not timestamped, fall back to a neutral label like "Last {c.samples.length} samples" to avoid showing a misleading seconds value. Use the existing c.samples array (and the sample value formatter formatBytes for values) and ensure the timestamp access (e.g., sample.timestamp) is null-safe before computing the duration.core/providers/mistral/mistral.go (1)
583-606: MoveEnsureStreamFinalizerCalledto the top of defer registration.Line 605 registers the defer last, so it executes first (Go LIFO). This causes finalization before cancellation/timeout cleanup and channel close. Register it first in the goroutine to execute last, consistent with all streaming goroutines in Anthropic provider.
Proposed change
go func() { + defer providerUtils.EnsureStreamFinalizerCalled(ctx) defer func() { if ctx.Err() == context.Canceled { providerUtils.HandleStreamCancellation(ctx, postHookRunner, responseChan, providerName, request.Model, schemas.TranscriptionStreamRequest, provider.logger) } else if ctx.Err() == context.DeadlineExceeded { providerUtils.HandleStreamTimeout(ctx, postHookRunner, responseChan, providerName, request.Model, schemas.TranscriptionStreamRequest, provider.logger) } close(responseChan) }() defer providerUtils.ReleaseStreamingResponse(resp) // Decompress gzip-encoded streams transparently (no-op for non-gzip) reader, releaseGzip := providerUtils.DecompressStreamBody(resp) defer releaseGzip() // Wrap reader with idle timeout to detect stalled streams. reader, stopIdleTimeout := providerUtils.NewIdleTimeoutReader(reader, resp.BodyStream(), providerUtils.GetStreamIdleTimeout(ctx)) defer stopIdleTimeout() // Setup cancellation handler to close the raw network stream on ctx cancellation, // which immediately unblocks any in-progress read (including reads blocked inside a gzip decompression layer). stopCancellation := providerUtils.SetupStreamCancellation(ctx, resp.BodyStream(), provider.logger) defer stopCancellation() - defer providerUtils.EnsureStreamFinalizerCalled(ctx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/mistral/mistral.go` around lines 583 - 606, Defer registration order is wrong: move the call to providerUtils.EnsureStreamFinalizerCalled(ctx) so it is deferred first in the goroutine (i.e., placed before all other defer statements) so it runs last; specifically, in the goroutine that handles resp you should register defer providerUtils.EnsureStreamFinalizerCalled(ctx) before defers for providerUtils.ReleaseStreamingResponse(resp), providerUtils.DecompressStreamBody/ releaseGzip, providerUtils.NewIdleTimeoutReader/ stopIdleTimeout, providerUtils.SetupStreamCancellation/ stopCancellation and the anonymous defer that handles ctx cancellation/timeouts and closes responseChan, ensuring finalizer executes after all cancellation/timeout cleanup and channel close.core/providers/anthropic/validate_chat_tools_test.go (1)
21-117: Add an explicitCustomtool case to lock the documented contract.The table currently validates function/server paths well, but it doesn’t cover the
tool.Custom != nilbranch that this test description also claims as universal-keep behavior.Suggested test addition
{ name: "function tools always survive on any provider", provider: schemas.Bedrock, input: []schemas.ChatTool{fnTool, fnTool}, wantKeep: 2, }, + { + name: "custom tools always survive on any provider", + provider: schemas.Vertex, + input: []schemas.ChatTool{ + { + Type: schemas.ChatToolTypeCustom, + Custom: &schemas.ChatToolCustom{Name: "custom_tool"}, + }, + }, + wantKeep: 1, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/validate_chat_tools_test.go` around lines 21 - 117, The test suite for ChatTool filtering omits a case exercising the tool.Custom != nil branch (the contract that custom tools always survive); add a new case in the cases slice (alongside fnTool and serverTool usages) that sets input to a schemas.ChatTool whose Custom field is non-nil and assert wantKeep reflects it is preserved (e.g., with provider schemas.Bedrock and wantKeep incremented), and include wantDropped/assertNotes as appropriate to document the expected universal-keep behavior for custom tools.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/Dockerfile.local`:
- Line 21: The FROM image digest is incorrect: update the Dockerfile FROM
reference for golang:1.26.1-alpine3.23 (the token starting with
"golang:1.26.1-alpine3.23@sha256:...") to use the correct digest
sha256:d337ecb3075f0ec76d81652b3fa52af47c3eba6c8ba9f93b835752df7ce62946 so the
base image pin is reproducible; replace the existing sha256 value in that FROM
line with the provided correct digest.
---
Outside diff comments:
In `@core/providers/bedrock/utils.go`:
- Around line 1346-1428: The conversion loop only handles tool.Function, causing
tool.Custom entries to be dropped; update the loop in the Bedrock conversion
(the code that builds bedrockTools and later reconciles against
params.ToolChoice) to also handle tool.Custom by creating a corresponding
BedrockTool/BedrockToolSpec and BedrockToolInputSchema (serialize the custom
tool's parameter/schema into json.RawMessage and use its description/name
similar to how tool.Function is handled), and ensure the Name used for
reconciliation matches the custom tool name so buildBedrockServerToolChoice and
collectBedrockServerToolsFromFiltered remain consistent; alternatively, if
custom tools should not be supported, change
collectBedrockServerToolsFromFiltered and buildBedrockServerToolChoice to
exclude/count them consistently so they are stripped upstream rather than
silently disappearing.
In `@core/providers/gemini/gemini.go`:
- Around line 4563-4609: In PassthroughStream's EOF handling, remove the direct
post-hook span finalizer invocation that fetches and calls
ctx.Value(schemas.BifrostContextKeyPostHookSpanFinalizer) so finalization isn't
executed twice; keep the postHookRunner(ctx, finalResp, nil) call and rely on
the already-deferred providerUtils.EnsureStreamFinalizerCalled(ctx) to run the
span finalizer for all stream termination paths, updating any comments if needed
to note that finalization is centralized.
---
Duplicate comments:
In `@core/bifrost.go`:
- Around line 5227-5236: The first-chunk SSE error path directly calls
bifrost.releasePluginPipeline(pipeline) and bypasses the sync.Once-protected
postHookSpanFinalizer, allowing double-release; modify the error path to call
postHookSpanFinalizer(ctx) instead of calling releasePluginPipeline directly (so
finalizerOnce.Do runs FinalizeStreamingPostHookSpans and releasePluginPipeline
exactly once), and remove or replace any other direct calls to
bifrost.releasePluginPipeline(pipeline) in that error flow to ensure the
finalizer is always used and the context is passed through.
- Around line 4692-4701: The post-hook goroutine is closing over the
PluginPipeline while tryStreamRequest returns the pipeline to the pool, allowing
reuse before post-hooks finish; to fix, ensure the pipeline is held until
pipelinePostHookRunner completes by delaying return-to-pool or explicit release:
modify pipelinePostHookRunner (and the guarded send goroutine that reads
shortCircuit.Stream/outputStream) to take ownership of the PluginPipeline and
call the pipeline release/reset after the post-hook processing and stream
draining completes (or use a sync.WaitGroup or channel to signal completion
before tryStreamRequest returns outputStream), referencing
pipelinePostHookRunner, tryStreamRequest, PluginPipeline, outputStream, and
shortCircuit.Stream to find where to move the release.
In `@core/providers/anthropic/chat.go`:
- Around line 412-469: The tool-choice name from bifrostReq.Params.ToolChoice
can reference a tool removed by ValidateChatToolsForProvider, causing
anthropicReq.ToolChoice.Name to point to a missing entry; after building
anthropicReq.Tools (and before assigning anthropicReq.ToolChoice), check if
toolChoice.Name (from
bifrostReq.Params.ToolChoice.ChatToolChoiceStruct.Function.Name) exists in the
final anthropicReq.Tools slice (compare by tool name/identifier), and if it does
not, either clear toolChoice.Name and set toolChoice.Type to a safe fallback
("none" or "auto") or downgrade the choice to a non-named type; update the logic
around ValidateChatToolsForProvider, anthropicReq.Tools and the block that
constructs anthropicReq.ToolChoice to perform this validation and mutation.
In `@core/providers/elevenlabs/elevenlabs.go`:
- Around line 407-430: The defer for
providerUtils.EnsureStreamFinalizerCalled(ctx) is registered too late inside the
goroutine so it runs before stopCancellation and the cancellation/timeout
handlers; move defer providerUtils.EnsureStreamFinalizerCalled(ctx) to be the
first defer immediately after the start of the goroutine (i.e., directly after
go func() {) so its call is executed last; keep all other
defers—ReleaseStreamingResponse(resp), releaseGzip(), stopIdleTimeout(),
stopCancellation(), and the cancellation/timeout closure that calls
HandleStreamCancellation/HandleStreamTimeout—unchanged in their current relative
order.
In `@core/providers/utils/stream.go`:
- Around line 62-71: The loop reading from the source channel `stream` can block
and delay cancellation because it does a range read without checking
`ctx.Done()`; change the loop to a select-based receive so cancellation is
observed while waiting for the next item. Replace `for chunk := range stream`
with a `for` that does `select { case <-ctx.Done(): /* stop/return */; case
chunk, ok := <-stream: if !ok { return } ... }` and keep the existing
send-to-`wrapped` logic (also using a select to handle `ctx.Done()` when
sending). Ensure that when the consumer abandons `wrapped` you still drain
`stream` to unblock the provider, and use the `ok` flag from `chunk, ok :=
<-stream` to detect closed source. This references the variables `ctx`,
`stream`, and `wrapped` in core/providers/utils/stream.go.
In `@transports/changelog.md`:
- Around line 4-14: Add two explicit bullets to the changelog: one noting the Go
toolchain downgrade from 1.26.2 to 1.26.1 (applies to go.mod files, CI
workflows, and Dockerfiles), and one describing the stream cancellation safety
improvements (mention guarded channel sends, context-aware draining of upstream
channels, and the EnsureStreamFinalizerCalled finalizer to guarantee cleanup);
update the "Fixed" or appropriate section near the existing "Provider Queue
Shutdown Panic" and "Responses Streaming Errors" entries so reviewers can see
the Go downgrade and the specific finalizer/context-drain changes.
---
Nitpick comments:
In `@core/providers/anthropic/validate_chat_tools_test.go`:
- Around line 21-117: The test suite for ChatTool filtering omits a case
exercising the tool.Custom != nil branch (the contract that custom tools always
survive); add a new case in the cases slice (alongside fnTool and serverTool
usages) that sets input to a schemas.ChatTool whose Custom field is non-nil and
assert wantKeep reflects it is preserved (e.g., with provider schemas.Bedrock
and wantKeep incremented), and include wantDropped/assertNotes as appropriate to
document the expected universal-keep behavior for custom tools.
In `@core/providers/mistral/mistral.go`:
- Around line 583-606: Defer registration order is wrong: move the call to
providerUtils.EnsureStreamFinalizerCalled(ctx) so it is deferred first in the
goroutine (i.e., placed before all other defer statements) so it runs last;
specifically, in the goroutine that handles resp you should register defer
providerUtils.EnsureStreamFinalizerCalled(ctx) before defers for
providerUtils.ReleaseStreamingResponse(resp),
providerUtils.DecompressStreamBody/ releaseGzip,
providerUtils.NewIdleTimeoutReader/ stopIdleTimeout,
providerUtils.SetupStreamCancellation/ stopCancellation and the anonymous defer
that handles ctx cancellation/timeouts and closes responseChan, ensuring
finalizer executes after all cancellation/timeout cleanup and channel close.
In `@ui/app/pprof/page.tsx`:
- Around line 491-495: The label "Last {c.samples.length * 10}s" assumes 10s
polling and is inaccurate when refetch() shortens the window; update the UI to
compute the real wall-clock window from sample timestamps instead of assuming
10s per sample: when c.samples contains timestamped entries, compute seconds =
Math.round((lastSample.timestamp - firstSample.timestamp) / 1000) and display
"Last {seconds}s"; if samples are not timestamped, fall back to a neutral label
like "Last {c.samples.length} samples" to avoid showing a misleading seconds
value. Use the existing c.samples array (and the sample value formatter
formatBytes for values) and ensure the timestamp access (e.g., sample.timestamp)
is null-safe before computing the duration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c272e3d-a842-411e-a92a-ea5d089c37d6
📒 Files selected for processing (50)
.github/workflows/e2e-tests.yml.github/workflows/pr-tests.yml.github/workflows/release-cli.yml.github/workflows/release-pipeline.yml.github/workflows/snyk.ymlREADME.mdcli/go.modcore/bifrost.gocore/go.modcore/providers/anthropic/anthropic.gocore/providers/anthropic/chat.gocore/providers/anthropic/utils.gocore/providers/anthropic/validate_chat_tools_test.gocore/providers/azure/azure.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/convert_tool_config_test.gocore/providers/bedrock/utils.gocore/providers/cohere/cohere.gocore/providers/elevenlabs/elevenlabs.gocore/providers/gemini/gemini.gocore/providers/huggingface/huggingface.gocore/providers/mistral/mistral.gocore/providers/openai/openai.gocore/providers/replicate/replicate.gocore/providers/utils/stream.gocore/providers/utils/stream_test.gocore/providers/utils/utils.gocore/providers/vertex/vertex.gocore/providers/vllm/vllm.gocore/schemas/images.gocore/schemas/videos.goexamples/plugins/hello-world/go.modframework/go.modplugins/governance/go.modplugins/jsonparser/go.modplugins/litellmcompat/go.modplugins/logging/go.modplugins/maxim/go.modplugins/mocker/go.modplugins/otel/go.modplugins/semanticcache/go.modplugins/telemetry/go.modtests/scripts/1millogs/go.modtransports/Dockerfiletransports/Dockerfile.localtransports/bifrost-http/handlers/devpprof.gotransports/changelog.mdtransports/go.modui/app/pprof/page.tsxui/lib/store/apis/devApi.ts
💤 Files with no reviewable changes (1)
- README.md
Contributor
Author
Merge activity
|
akshaydeo
changed the base branch from
04-17-dependabot_fixes
to
graphite-base/2792
akshaydeo
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
akshaydeo
force-pushed
the
04-17-move_back_go_to_1.26.1
branch
from
74df3b6 to
086578f
Compare
18 tasks
dwjwlxs
added a commit
to dwjwlxs/bifrost
that referenced
this pull request
Apr 20, 2026
* fix: delete fallbacks from anthropic req (#2754)
## Summary
Remove the `fallbacks` field from Anthropic provider request bodies to ensure compatibility with the Anthropic API specification.
## Changes
- Added logic to delete the `fallbacks` field from JSON request bodies in the Anthropic provider's `getRequestBodyForResponses` function
- Implemented proper error handling for the field deletion operation with appropriate Bifrost error wrapping
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Test Anthropic provider requests to ensure the `fallbacks` field is properly removed and requests succeed:
```sh
# Core/Transports
go version
go test ./...
# Test specific Anthropic provider functionality
go test ./core/providers/anthropic/...
```
Verify that requests to the Anthropic API no longer include the `fallbacks` field and complete successfully.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No security implications - this change only removes an unsupported field from API requests.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* fix: preserve context values in async requests (#2703)
## Summary
Refactors async job execution to pass the full BifrostContext instead of just the virtual key value, enabling proper context preservation for background operations including virtual keys, tracing headers, and other request metadata.
## Changes
- Modified `AsyncJobExecutor.SubmitJob()` to accept `*schemas.BifrostContext` instead of `*string` for virtual key
- Updated `executeJob()` to restore all original request context values in the background goroutine
- Added `getVirtualKeyFromContext()` helper function to extract virtual key from BifrostContext
- Updated all async handler methods to pass BifrostContext directly to `SubmitJob()`
- Removed redundant virtual key extraction logic from HTTP handlers
## Type of change
- [x] Refactor
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
## How to test
Verify async job execution preserves request context properly:
```sh
# Core/Transports
go version
go test ./...
# Test async endpoints with virtual keys and tracing headers
curl -X POST http://localhost:8080/v1/async/chat/completions \
-H "Authorization: Bearer vk_test_key" \
-H "X-Trace-Id: test-trace-123" \
-d '{"model": "gpt-3.5-turbo", "messages": [{"role": "user", "content": "Hello"}]}'
# Verify job execution maintains context
curl http://localhost:8080/v1/async/jobs/{job_id}
```
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
Improves security by ensuring proper context isolation and virtual key handling in async operations.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* [fix]: Gemini provider - handle content block tool outputs in Responses API path (#2692)
When function_call_output messages arrive via the Anthropic Responses API
format, their output is an array of content blocks (ResponsesFunctionToolCallOutputBlocks),
not a plain string (ResponsesToolCallOutputStr). The Gemini provider's
convertResponsesMessagesToGeminiContents only checked the string case,
silently dropping all tool result content and sending empty {} responses
to Gemini. This caused the model to loop endlessly retrying tool calls
it never saw results for.
Other providers (Bedrock, OpenAI, Cohere) already handle both output
formats. This aligns the Gemini provider with them.
Affected packages:
- core/providers/gemini/responses.go - Add ResponsesFunctionToolCallOutputBlocks handling
- core/providers/gemini/gemini_test.go - Add test for content block outputs
Co-authored-by: tom <tom@asteroid.ai>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Akshay Deo <akshay@akshaydeo.com>
* fix: gemini thinking level and finish reason round-trip preservation (#2697)
## Summary
Fixes two critical regressions in the Gemini provider's GenAI integration: preserves `thinkingLevel` parameters during round-trip conversions and ensures `MAX_TOKENS` finish reasons survive Bifrost transformations.
## Changes
- **Fixed thinking level preservation**: Modified `convertGenerationConfigToResponsesParameters()` to only set effort from `thinkingLevel` without deriving a `thinkingBudget`, preventing unwanted behavior changes in Gemini 3.x models
- **Enhanced finish reason handling**: Added bidirectional conversion between Gemini and Bifrost finish reasons, prioritizing `StopReason` over `IncompleteDetails` to preserve `MAX_TOKENS` finish reasons
- **Expanded finish reason support**: Added new Gemini finish reason constants for image generation, tool calls, and malformed responses
- **Improved response conversion**: Updated response conversion logic to properly handle error finish reasons and set appropriate status/error fields
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Validate the thinking level and finish reason preservation:
```sh
# Run Gemini provider tests
go test ./core/providers/gemini/... -v
# Specifically test the regression fixes
go test ./core/providers/gemini/... -run "TestGenAIThinkingLevel_RoundTripPreservesLevelNotBudget|TestGenAIFinishReasonMaxTokens_PersistsThroughBifrostRoundTrip" -v
```
Test with actual Gemini API calls using thinking levels and verify that:
- `thinkingLevel` parameters are preserved without generating unwanted `thinkingBudget` values
- Responses with `MAX_TOKENS` finish reason maintain that status through the conversion pipeline
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
Addresses regressions in GenAI path where thinking configuration and finish reasons were being incorrectly transformed during Bifrost conversions.
## Security considerations
No security implications - this change only affects internal data structure conversions and doesn't modify authentication, secrets handling, or data exposure.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* fix: remove cc user agent guard from streaming in anthropic (#2706)
## Summary
Fixes WebSearch tool argument handling for all clients by removing the Claude Code user agent restriction. Previously, only Claude Code clients received proper WebSearch query arguments in the streaming response, while other clients lost the query data due to skipped argument deltas.
## Changes
- Removed the `IsClaudeCodeRequest(ctx)` check that was restricting WebSearch argument sanitization and synthetic delta generation to only Claude Code clients
- WebSearch tool arguments are now sanitized and synthetic `input_json_delta` events are generated for all clients during `output_item.done` events
- Added comprehensive test coverage for the WebSearch tool flow including argument delta skipping, synthetic delta generation, and full end-to-end streaming scenarios
- Enhanced code comments to clarify the WebSearch tool handling logic
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Validate the WebSearch tool behavior with the new test suite:
```sh
# Run the new WebSearch tests
go test ./core/providers/anthropic -run TestWebSearch -v
# Run all provider tests to ensure no regressions
go test ./core/providers/anthropic/...
# Full test suite
go test ./...
```
Test with different user agents to verify WebSearch queries are properly streamed to all clients, not just Claude Code.
## Screenshots/Recordings
N/A - This is a backend streaming API fix.
## Breaking changes
- [ ] Yes
- [x] No
This change expands functionality to previously broken clients without affecting existing working behavior.
## Related issues
Fixes WebSearch tool argument streaming for non-Claude Code clients.
## Security considerations
The change maintains existing argument sanitization for WebSearch tools while expanding it to all clients, preserving the same security posture.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* remove unnecessary marshalling of payload (#2770)
## Summary
Optimized JSON parsing in the Anthropic integration by replacing full JSON unmarshaling with targeted field extraction using gjson for retrieving the "type" field from streaming responses.
## Changes
- Replaced `sonic.Unmarshal()` with `gjson.Get()` to extract only the "type" field from Anthropic stream events
- Eliminated the need to unmarshal the entire JSON response into an `AnthropicStreamEvent` struct
- Improved performance by avoiding unnecessary JSON parsing overhead
## Type of change
- [x] Refactor
- [ ] Bug fix
- [ ] Feature
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Test streaming responses from the Anthropic integration to ensure the type field is correctly extracted:
```sh
# Core/Transports
go version
go test ./...
# Test specifically the Anthropic integration
go test ./transports/bifrost-http/integrations/
```
## Screenshots/Recordings
N/A
## Breaking changes
- [x] No
- [ ] Yes
## Related issues
N/A
## Security considerations
No security implications - this is a performance optimization that maintains the same functionality.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* feat: claude opus 4.7 compatibility (#2773)
## Summary
Adds support for Claude Opus 4.7 model with specific parameter handling and reasoning configuration changes. Opus 4.7 rejects temperature, top_p, and top_k parameters and only supports adaptive thinking mode without budget tokens.
## Changes
- Added `IsOpus47()` function to detect Claude Opus 4.7 models
- Modified parameter handling to skip temperature, top_p, and top_k for Opus 4.7 models
- Updated reasoning configuration to use adaptive thinking only for Opus 4.7 (no budget_tokens)
- Added support for `display` parameter in thinking configuration to control output visibility
- Extended adaptive thinking support to include Sonnet 4.6 models
- Added task budget support with new beta header `task-budgets-2026-03-13`
- Updated effort mapping to handle Opus 4.7's "xhigh" effort level
- Added comprehensive test coverage for Opus 4.7 specific behaviors
- Fixed OpenAI responses to filter out Anthropic-specific summary:"none" parameter
## Type of change
- [x] Feature
- [x] Bug fix
## Affected areas
- [x] Core (Go)
- [x] Providers/Integrations
## How to test
Validate the changes with the following tests:
```sh
# Core/Transports
go version
go test ./core/providers/anthropic/...
# Specific test cases for Opus 4.7
go test -run TestToAnthropicChatRequest_Opus47 ./core/providers/anthropic/
go test -run TestSupportsAdaptiveThinking ./core/providers/anthropic/
go test -run TestAddMissingBetaHeadersToContext_TaskBudgets ./core/providers/anthropic/
```
Test with Claude Opus 4.7 model requests to ensure:
- Temperature, top_p, top_k parameters are stripped
- Reasoning uses adaptive thinking without budget_tokens
- Task budget beta headers are properly added
## Breaking changes
- [ ] Yes
- [x] No
The changes maintain backward compatibility while adding new model support.
## Security considerations
No security implications. Changes only affect parameter handling and model-specific configurations for Anthropic's Claude models.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* docs: restructure helm guide into comprehensive multi-page reference (#2776)
* docs: restructure helm guide into comprehensive multi-page reference (#2771)
## Summary
Restructures the Helm deployment documentation into a comprehensive multi-page guide with dedicated sections for each configuration area. The main Helm page now provides quickstart instructions for both OSS and Enterprise deployments, while detailed configuration is split into focused sub-pages.
## Changes
- **Restructured main Helm page**: Condensed from 740+ lines to 103 lines with clear quickstart tabs for OSS vs Enterprise
- **Added 8 new dedicated configuration pages**:
- `values.mdx` - Complete values reference with examples and common patterns
- `client.mdx` - Client configuration (pool size, logging, CORS, auth, compat shims)
- `providers.mdx` - Provider setup for all 23+ supported LLM providers with cloud-native auth
- `storage.mdx` - Storage backends (SQLite, PostgreSQL, object storage, vector stores)
- `plugins.mdx` - Plugin configuration (telemetry, logging, semantic cache, OTel, Datadog)
- `governance.mdx` - Governance setup (budgets, rate limits, virtual keys, routing rules)
- `cluster.mdx` - Multi-replica HA with gossip-based peer discovery
- `troubleshooting.mdx` - Common issues and diagnostic commands
- **Updated chart version**: Bumped from 1.5.0 to 2.1.0
- **Enhanced navigation**: Added nested Helm section in docs.json with proper icons and organization
## Type of change
- [x] Documentation
## Affected areas
- [x] Docs
## How to test
Navigate through the new Helm documentation structure:
1. Visit the main Helm page for quickstart instructions
2. Follow the quickstart for either OSS or Enterprise deployment
3. Use the sub-pages for detailed configuration of specific areas
4. Verify all internal links work correctly
5. Test the troubleshooting commands on a real deployment
The documentation now provides both quick-start paths and comprehensive reference material for production deployments.
## Screenshots/Recordings
N/A - Documentation changes only
## Breaking changes
- [ ] Yes
- [x] No
This is purely a documentation restructure with no functional changes to the Helm chart itself.
## Related issues
Improves Helm documentation organization and usability for both new users and production deployments.
## Security considerations
The new documentation emphasizes security best practices:
- Kubernetes Secrets for all sensitive values
- Cloud-native authentication (IRSA, Workload Identity, Managed Identity)
- Proper RBAC setup for cluster mode
- Compliance considerations (HIPAA, PCI) for content logging
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* docs: update guardrails docs
* v1.4.23 cut (#2778)
## Summary
Release version 1.4.20-1.4.23 with bug fixes for provider integrations, streaming error handling, and migration test improvements. This release addresses critical issues in Gemini, Bedrock, and Anthropic providers while adding support for Claude Opus 4.7.
## Changes
- **Provider Fixes**: Fixed Gemini tool outputs handling, Bedrock streaming events, and image content preservation in tool results
- **Streaming Improvements**: Added proper error capture for Responses streaming API to prevent silent failures
- **Migration Tests**: Added support for v1.4.22 governance model pricing flex tier columns in both PostgreSQL and SQLite migration tests
- **Anthropic Enhancements**: Removed fallback fields from outgoing requests and added Claude Opus 4.7 compatibility
- **Framework Fixes**: Improved async context propagation and custom provider model validation
- **Plugin Updates**: Enhanced OTEL metrics and configuration defaults
## Type of change
- [x] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [x] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [x] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Validate the migration test changes and provider fixes:
```sh
# Test migration scripts
./.github/workflows/scripts/run-migration-tests.sh
# Core/Transports
go version
go test ./...
# Test provider integrations
go test ./transports/...
go test ./plugins/...
```
Test the new governance model pricing columns are properly handled in migration scenarios.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
Addresses multiple provider integration issues and streaming API error handling improvements.
## Security considerations
No security implications - changes are focused on bug fixes and migration test improvements.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* validator fix (#2780)
## Summary
Enhanced GitHub Actions security by transitioning from audit-only to strict network egress control using step-security/harden-runner. This change blocks all outbound network traffic by default and explicitly allows only required endpoints for each workflow.
## Changes
- Changed `egress-policy` from `audit` to `block` across all GitHub Actions workflows
- Added comprehensive `allowed-endpoints` lists for each job, specifying only the necessary external services
- Updated step names from "Harden the runner (Audit all outbound calls)" to "Harden Runner" for consistency
- Fixed schema validation script to use correct JSON paths for concurrency and SCIM configuration validation
- Reformatted JSON schema file for improved readability (whitespace and formatting changes only)
## Type of change
- [x] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [x] Plugins
- [x] UI (Next.js)
- [x] Docs
## How to test
Verify that all GitHub Actions workflows continue to function properly with the new network restrictions:
```sh
# Trigger workflows by pushing to a branch or creating a PR
git push origin feature-branch
# Monitor workflow runs in GitHub Actions tab to ensure:
# - All jobs complete successfully
# - No network connectivity errors occur
# - All required external services remain accessible
```
Key endpoints that should remain accessible include:
- GitHub API and release assets
- Package registries (npm, PyPI, Go modules)
- Docker registries
- Cloud storage services
- External APIs used by tests and integrations
## Screenshots/Recordings
N/A - Infrastructure/CI changes only
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
This change significantly improves security posture by:
- Preventing unauthorized outbound network connections from CI runners
- Creating an explicit allowlist of required external services
- Reducing attack surface for supply chain attacks
- Providing better visibility into network dependencies
The transition from audit to block mode ensures that any new network dependencies must be explicitly approved and documented.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* fix: token usage for vllm --skip-pipeline (#2784)
## Summary
Fixed token usage attribution for vLLM by treating empty-string content the same as nil content in streaming responses. vLLM sends `delta.content=""` (instead of `delta: null`) in finish_reason chunks, which was being forwarded and causing the synthesis chunk to lose its finish_reason, breaking usage attribution in logs and UI.
## Changes
- Modified streaming content handling to check for both nil and empty string content before processing chunks
- This prevents empty content deltas from being forwarded, ensuring finish_reason is preserved for proper token usage tracking
- Removed extraneous whitespace and formatting inconsistencies throughout the OpenAI provider code
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Test with vLLM provider to ensure token usage is properly attributed:
```sh
# Core/Transports
go version
go test ./...
# Test streaming chat completion with vLLM
# Verify that finish_reason is preserved in final chunks
# Check that token usage appears correctly in logs/UI
```
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
Fixes token usage tracking issues with vLLM provider.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* [fix]: OpenAI provider - flatten array-form tool_result output for Responses API (#2781) --skip-pipeline
When Anthropic tool_result blocks arrive with array-form content (the
standard shape for multi-turn tool exchanges), the OpenAI provider's
MarshalJSON emitted the output as a JSON array on the wire. The OpenAI
Responses API defines function_call_output.output as a string — strict
upstreams (Ollama Cloud, openai-go typed models) reject the array form
with HTTP 400.
Fix: before marshaling, collapse text-only
ResponsesFunctionToolCallOutputBlocks into a newline-joined string.
Non-text blocks (images, files) are left as-is. The schema type is
unchanged; the transformation lives in the OpenAI provider's outbound
marshaler only.
Closes #2779
Affected packages:
- core/providers/openai/types.go - Flatten text-only output blocks to string
- core/providers/openai/responses_marshal_test.go - Three regression tests
- core/changelog.md - Changelog entry
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: prevent send on closed channel panic in provider queue shutdown --skip-pipeline (#2725)
## Summary
Fixes a race condition in provider queue shutdown that caused "send on closed channel" panics in production. The issue occurred when producers passed the `isClosing()` check but then attempted to send to a queue that was closed before they reached the select statement.
## Changes
- **Removed queue channel closure**: Queue channels are never closed to prevent "send on closed channel" panics
- **Updated worker exit mechanism**: Workers now exit via the `done` channel signal instead of waiting for queue closure
- **Enhanced shutdown handling**: Workers drain remaining buffered requests and send shutdown errors when `done` is signaled
- **Added producer re-routing**: Stale producers can transparently re-route to new queues during `UpdateProvider`
- **Improved error handling**: Added rollback logic for failed provider updates with proper cleanup
- **Enhanced transfer logic**: Buffered requests are transferred before signaling shutdown to ensure they reach new workers
- **Added comprehensive tests**: Race condition demonstration and validation of the fix across multiple scenarios
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Run the new race condition test to verify the fix:
```sh
go test -run TestProviderQueue_SendOnClosedChannel_Race ./core -v
```
Run the comprehensive provider lifecycle tests:
```sh
go test -run TestProviderQueue ./core -v
go test -run TestUpdateProvider ./core -v
go test -run TestRemoveProvider ./core -v
```
Run the full test suite to ensure no regressions:
```sh
go test ./...
```
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
Fixes production panics related to concurrent provider queue operations during shutdown/updates.
## Security considerations
None - this is an internal concurrency fix that doesn't affect external interfaces or data handling.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: preserve MCP tool annotations in bidirectional conversion --skip-pipeline (#2746)
## Summary
Adds support for preserving MCP tool annotations when converting between MCP tools and Bifrost schemas. This enables MCP servers to provide behavioral hints (read-only, destructive, idempotent, open-world) that help agents make better reasoning decisions about tool usage.
## Changes
- Added `MCPToolAnnotations` struct to capture MCP spec hints including title, read-only, destructive, idempotent, and open-world indicators
- Modified `convertMCPToolToBifrostSchema` to preserve MCP tool annotations when converting from MCP tools to Bifrost chat tools
- Updated `ChatToolFunction` to include optional annotations field
- Enhanced MCP server sync logic to map Bifrost annotations back to MCP tool annotations for bidirectional compatibility
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Test with an MCP server that provides tool annotations to verify they are preserved through the conversion process:
```sh
# Core/Transports
go version
go test ./...
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
Verify that MCP tools with annotations maintain their behavioral hints when converted to Bifrost schemas and back.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No security implications - this change only preserves metadata hints that help with tool behavior classification.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* fix: add support for Anthropic structured output and response format (#1972)
* fix: add support for Anthropic structured output and response format conversion
* fix: refactor output configuration setting in ToBedrockResponsesRequest
* run go fmt on responses.go
* fix: streamline response format conversion for Anthropic models
* fix: enhance merging of additional model request fields and output configuration
* fix: remove koanf/maps dependency and replace its usage with internal merge function
* preserve order in output_config
* update type casting
* add non-anthropic test-case
* check for output_config first
* diversify anthropic output formats
* move bifrost ctx update
* guard tested field
* guard format.jsonschema
* test fixes --skip-pipeline (#2782)
## Summary
Updates test configurations to align with current API specifications and replaces deprecated utility function usage.
## Changes
- Replaced `schemas.Ptr("test")` with `new("test")` in Anthropic chat test for string pointer creation
- Updated MCP client configuration tests to use `sse` connection type instead of `websocket` with simplified `connection_string` field
- Modified HTTP MCP client config to use `connection_string` instead of nested `http_config` object
- Changed OpenTelemetry plugin tests to use `genai_extension` trace type instead of `otel`
## Type of change
- [x] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
## How to test
Validate that all tests pass with the updated configurations:
```sh
# Core/Transports
go version
go test ./...
```
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No security implications - these are test configuration updates only.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* anthropic container changes --skip-pipeline (#2783)
## Summary
Briefly explain the purpose of this PR and the problem it solves.
## Changes
- What was changed and why
- Any notable design decisions or trade-offs
## Type of change
- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Describe the steps to validate this change. Include commands and expected outcomes.
```sh
# Core/Transports
go version
go test ./...
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
If adding new configs or environment variables, document them here.
## Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
## Breaking changes
- [ ] Yes
- [ ] No
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
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* core schema changes --skip-pipeline (#2787)
## Summary
Promotes Anthropic-native parameters to the neutral ChatParameters layer, enabling direct access to advanced Anthropic features like containers, MCP servers, task budgets, and enhanced tool configurations without requiring ExtraParams.
## Changes
- Added neutral fields to `ChatParameters` for Anthropic-specific features: `TopK`, `Speed`, `InferenceGeo`, `MCPServers`, `Container`, `CacheControl`, `TaskBudget`, and `ContextManagement`
- Enhanced `ChatTool` with Anthropic tool flags: `DeferLoading`, `AllowedCallers`, `InputExamples`, and `EagerInputStreaming`
- Added `Display` field to `ChatReasoning` for Anthropic adaptive thinking control
- Implemented `StripUnsupportedAnthropicFields` function to remove unsupported features based on provider capabilities
- Updated parameter mapping logic to prefer neutral fields over ExtraParams with fallback support
- Added comprehensive JSON marshaling/unmarshaling for union types like `ChatContainer`
The design maintains backward compatibility by falling back to ExtraParams when neutral fields are not set, while providing type-safe access to advanced Anthropic features.
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Validate the new parameter handling and provider feature gating:
```sh
# Core/Transports
go version
go test ./...
# Test Anthropic provider parameter mapping
go test ./core/providers/anthropic/...
# Verify schema validation
go test ./core/schemas/...
```
Test with requests containing the new neutral fields to ensure proper mapping to Anthropic API format and appropriate stripping for unsupported providers.
## Screenshots/Recordings
N/A - Backend API changes only.
## Breaking changes
- [ ] Yes
- [x] No
This change is fully backward compatible. Existing ExtraParams usage continues to work, while new neutral fields provide enhanced type safety.
## Related issues
N/A
## Security considerations
The new MCP server configuration includes authorization tokens. Ensure proper handling of sensitive credentials in the `ChatMCPServer.AuthorizationToken` field.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* dependabot fixes --skip-pipeline (#2788)
## Summary
This PR adds the Hono web framework as a direct dependency to all MCP server examples and updates various dependencies across the project to their latest versions.
## Changes
- Added `hono@^4.12.14` as a direct dependency to all MCP server examples (edge-case-server, error-test-server, parallel-test-server, temperature, test-tools-server)
- Upgraded Hono from version 4.11.4 to 4.12.14 and changed it from a peer dependency to a direct dependency
- Updated Python dependencies including authlib (1.6.6 → 1.6.11), langchain-core (1.2.28 → 1.2.31), langchain-openai (1.1.4 → 1.1.14), langchain-text-splitters (1.1.0 → 1.1.2), langsmith (0.5.0 → 0.7.32), openai (2.13.0 → 2.32.0), and python-multipart (0.0.20 → 0.0.26)
- Updated TypeScript dependencies including langsmith (0.5.18 → 0.5.19) and added it as a direct dependency
- Added `github.com/tidwall/gjson v1.18.0` as a direct dependency in Go transports module
- Updated UI dependencies including dompurify (3.3.3 → 3.4.0) and follow-redirects (1.15.11 → 1.16.0) via package overrides
## Type of change
- [x] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [x] UI (Next.js)
## How to test
Validate that all dependencies are properly installed and examples still function:
```sh
# Test MCP server examples
cd examples/mcps/temperature
npm install
npm run build
# Test Go transports
cd transports
go mod tidy
go test ./...
# Test Python integrations
cd tests/integrations/python
uv sync
uv run python -m pytest
# Test TypeScript integrations
cd tests/integrations/typescript
npm install
npm test
# Test UI
cd ui
pnpm install
pnpm test
pnpm build
```
## Screenshots/Recordings
N/A - dependency updates only
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
The dependency updates include security patches, particularly for dompurify and follow-redirects which are explicitly overridden in the UI package.json for security reasons.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* move back go to 1.26.1 (#2792)
## Summary
Downgrade Go version from 1.26.2 to 1.26.1 across all GitHub Actions workflows, Go modules, and Docker images to address compatibility issues.
## Changes
- Downgraded Go version from 1.26.2 to 1.26.1 in all GitHub Actions workflows (e2e-tests, pr-tests, release-cli, release-pipeline, snyk)
- Updated go.mod files for core, CLI, examples, and test modules to use Go 1.26.1
- Updated Docker base images in transports/Dockerfile and transports/Dockerfile.local to use golang:1.26.1-alpine3.23
- Added stream cancellation safety improvements with guarded channel sends and finalizer protection to prevent goroutine leaks when clients disconnect
- Enhanced stream error checking with context cancellation support to properly drain upstream channels
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [x] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Validate the Go version downgrade and streaming improvements:
```sh
# Verify Go version
go version
# Core/Transports
go test ./...
# Test streaming endpoints with client disconnection scenarios
# to verify proper cleanup and no goroutine leaks
```
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
The streaming improvements enhance resource cleanup and prevent potential goroutine leaks when clients disconnect unexpectedly, improving overall system stability.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* temp gotoolchain auto (#2809)
* temp hack for tests (#2810)
## Summary
The Go workspace setup script was not specifying a `go` directive or toolchain version, which caused `GOTOOLCHAIN=auto` to select a Go version lower than what `core@v1.4.19` requires. This adds an explicit `go 1.26.2` and `toolchain go1.26.2` directive to the workspace so the correct toolchain is used automatically.
## Changes
- Added `go work edit -go=1.26.2 -toolchain=go1.26.2` to `setup-go-workspace.sh` so that `GOTOOLCHAIN=auto` selects Go >= 1.26.2, satisfying the minimum version required by the published `core@v1.4.19` module referenced in `transports/go.mod`.
## Type of change
- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [x] Chore/CI
## Affected areas
- [ ] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
```sh
# Verify the workspace is initialized with the correct Go version
bash .github/workflows/scripts/setup-go-workspace.sh
grep -E "^go |^toolchain" go.work
# Expected output:
# go 1.26.2
# toolchain go1.26.2
go test ./...
```
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
## Security considerations
None.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* temp block docker build (#2811)
## Summary
Temporarily disables the `test-docker-image-amd64` and `test-docker-image-arm64` CI jobs in the release pipeline by commenting them out.
## Changes
- Both Docker image test jobs (`test-docker-image-amd64` and `test-docker-image-arm64`) have been commented out rather than removed, preserving the full job definitions for easy re-enablement later.
## Type of change
- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [x] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
No functional code changes. Verify the release pipeline runs without executing the Docker image test jobs.
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No security implications.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* removed docker build steps (#2812)
## Summary
The `test-docker-image-amd64` and `test-docker-image-arm64` CI jobs have been removed from the release pipeline. These jobs were already commented out and non-functional, and all references to them as dependencies and gate conditions in downstream release jobs have been cleaned up.
## Changes
- Deleted the commented-out `test-docker-image-amd64` and `test-docker-image-arm64` job definitions from the release pipeline.
- Removed `test-docker-image-amd64` and `test-docker-image-arm64` from the `needs` arrays of `core-release`, `framework-release`, `plugins-release`, `bifrost-http-release`, the Docker build/push jobs, the manifest job, and the final notification job.
- Removed the corresponding result checks for those two jobs from all `if` conditions in the affected release jobs.
## Type of change
- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [x] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Trigger the release pipeline and confirm that all release jobs proceed without waiting on or referencing the removed Docker image test jobs.
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
## Security considerations
None.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* moves tests to 1.26.2 and 1.26.1 (#2813)
## Summary
Bumps the Go version used across all release pipeline jobs from `1.26.1` to `1.26.2` to keep the CI environment on the latest patch release.
## Changes
- Updated Go version from `1.26.1` to `1.26.2` in all `setup-go` steps within the release pipeline workflow.
## Type of change
- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [x] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
The release pipeline will use the updated Go version on the next run. No additional manual steps are required beyond verifying the CI pipeline passes.
```sh
go version
go test ./...
```
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
Patch releases often include security and bug fixes. Staying on the latest patch version reduces exposure to known vulnerabilities in the Go toolchain.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* ocr test fixes (#2814)
## Summary
Adds an operation-allowed check for OCR requests before they are dispatched to a provider, and fixes the Mistral provider to return its custom provider name when one is configured.
## Changes
- Added a `CheckOperationAllowed` guard for `OCRRequest` in `handleProviderRequest`, consistent with how other request types are gated. If the operation is not permitted, a `BifrostError` is returned with the provider key, request type, and requested model populated.
- Updated `MistralProvider.GetProviderKey()` to use `providerUtils.GetProviderName` so that custom provider configurations are respected, rather than always returning the hardcoded `schemas.Mistral` value.
## Type of change
- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
```sh
go version
go test ./...
```
- Configure a custom provider wrapping Mistral and verify that `GetProviderKey()` returns the custom provider name rather than `mistral`.
- Attempt an OCR request against a provider where the operation is not allowed and confirm a `BifrostError` is returned with the correct `Provider`, `RequestType`, and `ModelRequested` fields set.
- Attempt an OCR request against a provider where the operation is allowed and confirm the request proceeds normally.
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
## Security considerations
None.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* revert to old schema (#2815)
## Summary
This PR simplifies and consolidates the `config.schema.json` by removing several features, collapsing provider-specific schema variants, and restructuring key configuration definitions to reduce complexity and align with updated runtime semantics.
## Changes
- Removed the top-level `version` field that controlled allow-list semantics for empty arrays
- Removed the `compat` plugin configuration block (including `convert_text_to_chat`, `convert_chat_to_responses`, `should_drop_params`, `should_convert_params`)
- Replaced `compat` with a simpler `enable_litellm_fallbacks` boolean for Groq text completion fallbacks
- Removed `mcp_disable_auto_tool_inject` and `routing_chain_max_depth` from server config
- Collapsed `provider_with_ollama_config`, `provider_with_sgl_config`, and `provider_with_replicate_config` into the generic `provider` definition; removed their corresponding key types (`ollama_key`, `sgl_key`, `replicate_key`) and `network_config_without_base_url`
- Removed providers `nebius`, `xai`, and `runway` from the providers block
- Moved `calendar_aligned` from `virtual_key` to the `budget` object; removed `virtual_key_id` and `provider_config_id` from budget in favor of a standalone `budget_id` reference on virtual keys
- Removed `chain_rule` from routing rules and relaxed the `scope_id` conditional requirement
- Simplified `virtual_key_provider_config` to inline key definitions with full provider-specific key configs (Azure, Vertex, Bedrock, VLLM), replacing the separate `key_ids` and `keys` split
- Removed `mcp_client_name` and `allow_on_all_virtual_keys` from MCP configs; removed `allowed_extra_headers` and `disable_auto_tool_inject` from MCP client config
- Added `websocket` as a supported MCP connection type with a dedicated `websocket_config` block; removed `inprocess` connection type
- Removed `per_user_oauth` as an MCP auth type and dropped the conditional `oauth_config_id` requirement
- Renamed `concurrency_and_buffer_size` to `concurrency_config`; renamed `retry_backoff_initial`/`retry_backoff_max` to `retry_backoff_initial_ms`/`retry_backoff_max_ms`; removed `enforce_http2` and `openai_config` from network config
- Moved `pricing_overrides` from the top-level config into individual provider definitions
- Simplified `provider_pricing_override` schema, removing scoped fields (`scope_kind`, `virtual_key_id`, `provider_id`, `provider_key_id`) and replacing `pattern` with `model_pattern`; added `regex` as a valid `match_type`; expanded supported `request_types`
- Renamed `scim_config` to `saml_config` in the top-level schema
- Removed `apiToken` from Okta config and made `clientSecret` optional; updated required fields to only `issuerUrl` and `clientId`
- Removed `object_storage` and `retention_days` from the logs store config
- Removed `id` and `description` fields from provider config entries in the `provider_configs` array
- Removed `websocket_responses` and `realtime` from `custom_provider_config` allowed requests; removed the enum constraint on `base_provider_type`
- Removed `disable_auto_tool_inject` from `mcp_client_config` VFS settings
- Added `deployments` mapping to `azure_key_config` and `vertex_key_config`
- Updated `otel` plugin `trace_type` to only accept `"otel"` (removed `genai_extension`, `vercel`, `open_inference`)
- Removed `prompts` from the built-in plugin name list
- Removed `builtin` as a valid plugin `placement` value
- Changed `cluster_config` discovery `dial_timeout` from a Go duration string to an integer (nanoseconds)
- Reformatted many inline `required` arrays to multi-line style for readability
## Type of change
- [ ] Bug fix
- [ ] Feature
- [x] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [x] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Validate existing configs against the updated schema to confirm they parse correctly. Verify that configs using removed fields (`version`, `compat`, `mcp_disable_auto_tool_inject`, `chain_rule`, etc.) are rejected by the schema validator.
```sh
go test ./...
```
Confirm that provider configs for Ollama, SGL, and Replicate continue to work using the generic `provider` definition. Confirm MCP clients using `websocket` connection type validate correctly with a `websocket_config` block.
## Breaking changes
- [x] Yes
- [ ] No
The following fields have been removed and configs using them will fail schema validation:
- `version` (top-level)
- `compat` block under server config
- `mcp_disable_auto_tool_inject` and `routing_chain_max_depth` under server config
- `chain_rule` on routing rules
- `calendar_aligned` on virtual keys (now on budgets)
- `virtual_key_id` / `provider_config_id` on budgets
- `apiToken` on Okta config (now optional `clientSecret` only)
- `object_storage` and `retention_days` on logs store
- `id`, `description` on provider config entries
- `allow_on_all_virtual_keys`, `allowed_extra_headers`, `disable_auto_tool_inject` on MCP client config
- `inprocess` MCP connection type and `per_user_oauth` auth type
- `enforce_http2` and `openai_config` from network config
- `builtin` plugin placement value; `prompts` built-in plugin name
- `nebius`, `xai`, `runway` provider entries
Migrate by removing or replacing these fields according to the updated schema definitions.
## Related issues
## Security considerations
Removal of `per_user_oauth` as an MCP auth type should be reviewed to ensure no active integrations depend on it. The relaxed `scope_id` requirement on routing rules should be validated to confirm it does not inadvertently broaden access scope.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* reduced release pipeline for this cut for go downgrade (#2816)
## Summary
This PR removes all test jobs from the release pipeline and decouples them from the release gate conditions, allowing releases to proceed without waiting for (often flaky) provider API test results. It also significantly expands and restructures `config.schema.json` to reflect new features, provider support, and breaking semantic changes introduced in v1.5.0.
## Changes
- **Release pipeline**: Removed `test-core`, `approve-flaky-test-core`, `test-framework`, `test-plugins`, `test-bifrost-http`, `test-migrations`, and `test-e2e-ui` jobs entirely from `release-pipeline.yml`. All release jobs (`core-release`, `framework-release`, `plugins-release`, `bifrost-http-prep`, `docker-build-amd64`, `docker-build-arm64`, `push-mintlify-changelog`) now depend only on change detection and upstream release jobs, not on test outcomes.
- **Schema: deny-by-default semantics (v1.5.0)**: Empty arrays in `provider_configs`, `mcp_configs`, `allowed_models`, `key_ids`, and `tools_to_execute` now mean "deny all" rather than "allow all". Use `["*"]` to allow all. A top-level `version` field (enum `1` or `2`, default `2`) controls which semantic applies, with `1` restoring v1.4.x behavior.
- **Schema: new providers**: Added `nebius`, `xai`, and `runway` as first-class provider entries.
- **Schema: provider key restructuring**: Replaced the inline key object definition in `virtual_key_provider_config` with a flat `key_ids` string array. Introduced dedicated key types `ollama_key`, `sgl_key`, and `replicate_key` with their own `_key_config` blocks. Removed `deployments` from `azure_key_config` and `vertex_key_config` (replaced by `aliases` on `base_key`). Added `aliases` to `base_key` for model-to-deployment/inference-profile mappings.
- **Schema: provider variants**: `ollama` and `sgl` now reference `provider_with_ollama_config` and `provider_with_sgl_config` respectively, which use `network_config_without_base_url` (URL is per-key). `replicate` references `provider_with_replicate_config`. Added `openai_config` def with `disable_store` for the Responses API. Renamed `concurrency_config` to `concurrency_and_buffer_size`.
- **Schema: network config**: Split `network_config` into `network_config` (with `base_url`) and `network_config_without_base_url`. Added `enforce_http2`, `stream_idle_timeout_in_seconds`, `max_conns_per_host`, `beta_header_overrides`, and `ca_cert_pem` fields. Renamed `retry_backoff_initial_ms`/`retry_backoff_max_ms` to `retry_backoff_initial`/`retry_backoff_max`.
- **Schema: MCP changes**: Removed `websocket` connection type; added `inprocess`. Added `per_user_oauth` auth type. Added `mcp_client_name` for config-file resolution. Added `allowed_extra_headers` and `allow_on_all_virtual_keys` to `mcp_client_config`. Added `disable_auto_tool_inject` to MCP plugin config. Added global `mcp_disable_auto_tool_inject` and `routing_chain_max_depth` to server config.
- **Schema: routing rules**: Added `chain_rule` boolean to `routing_rule`. Made `scope_id` required (non-null string) when `scope` is `team`, `customer`, or `virtual_key`.
- **Schema: budgets**: Moved `calendar_aligned` from the budget object to the virtual key level. Replaced `budget_id` on virtual key with `virtual_key_id`/`provider_config_id` on the budget object itself. Removed `budget_id` from `virtual_key_provider_config`.
- **Schema: logs store**: Added `object_storage` (S3/GCS) and `retention_days` to the logs store config.
- **Schema: pricing overrides**: Moved `pricing_overrides` from per-provider to a top-level array with scoped `provider_pricing_override` objects supporting `scope_kind`, `virtual_key_id`, `provider_id`, `provider_key_id`, `match_type`, `pattern`, `request_types`, and `pricing_patch`.
- **Schema: compat plugin**: Replaced `enable_litellm_fallbacks` with a structured `compat` object supporting `convert_text_to_chat`, `convert_chat_to_responses`, `should_drop_params`, and `should_convert_params`.
- **Schema: OTEL plugin**: Expanded `trace_type` enum to `genai_extension`, `vercel`, `open_inference` (was only `otel`).
- **Schema: SCIM**: Renamed `saml_config` to `scim_config`. Added `apiToken` to `okta_config` and made `clientSecret` and `apiToken` required. Changed cluster `dial_timeout` from integer (nanoseconds) to Go duration string.
- **Schema: misc**: Added `prompts` and `builtin` to plugin name/placement enums. Added `provider_configs` fields `id`, `description`, `network_config`, `proxy_config`, `custom_provider_config`, `concurrency_and_buffer_size`, and `openai_config`. Added `scim_config` top-level ref. Normalized multi-item `required` arrays to single-line format throughout.
## Type of change
- [ ] Bug fix
- [x] Feature
- [x] Refactor
- [ ] Documentation
- [x] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [x] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
```sh
# Validate schema against existing configs
npx ajv validate -s transports/config.schema.json -d your-config.json
# Verify release pipeline runs without test gate
# Push a tagged commit and confirm release jobs trigger directly after detect-changes
```
If upgrading from v1.4.x, set `"version": 1` in your config to preserve allow-all semantics for empty arrays, or migrate empty arrays to `["*"]` and adopt v2 deny-by-default semantics.
## Breaking changes
- [x] Yes
- [ ] No
**Empty arrays in `allowed_models`, `key_ids`, `tools_to_execute`, `provider_configs`, and `mcp_configs` now deny all access by default (v2 semantics).** To allow all, use `["*"]`. To restore v1.4.x behavior, set `"version": 1` at the top level of your config.
`enable_litellm_fallbacks` has been removed; replace with the `compat` object. `saml_config` has been renamed to `scim_config`. `budget_id` has been removed from virtual keys and `virtual_key_provider_config`. `calendar_aligned` has moved from the budget object to the virtual key. `deployments` has been removed from `azure_key_config` and `vertex_key_config`; use `aliases` on the key instead. `retry_backoff_initial_ms`/`retry_backoff_max_ms` renamed to `retry_backoff_initial`/`retry_backoff_max`. The `websocket` MCP connection type has been removed; use `http` or `sse`. Okta SCIM config now requires `clientSecret` and `apiToken`.
## Related issues
N/A
## Security considerations
The `insecure_skip_verify` and `ca_cert_pem` fields on `network_config` expose TLS bypass options; these should only be used in controlled environments. The `per_user_oauth` auth type for MCP introduces per-user credential flows that require careful OAuth config management. Removal of test gates from the release pipeline means regressions from flaky provider APIs will no longer block releases, but also means real failures could ship if not caught by other means.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* force verstion back to go 1.26.1 (#2817)
## Summary
Bumps `core` to v1.4.21 and updates `transports` to depend on `core` v1.4.20, while removing a now-unnecessary workspace Go directive workaround that was previously required to satisfy the toolchain constraint introduced by `core` v1.4.19.
## Changes
- Incremented `core` version from `1.4.20` to `1.4.21`
- Updated `transports/go.mod` to reference `core` v1.4.20 (previously v1.4.19)
- Removed the `go work edit -go=1.26.2 -toolchain=go1.26.2` workaround from the workspace setup script, which was only needed to satisfy the toolchain requirement imposed by the published `core` v1.4.19
## Type of change
- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [x] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
```sh
go work sync
go test ./...
```
Verify the workspace initializes without the explicit Go/toolchain directive and that all modules resolve correctly.
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
## Security considerations
None.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* revert everything to go1.26.1 (#2818)
## Summary
Bumps the core version to `1.4.22` and rolls back dependency versions across the framework, plugins, and transports to align with a prior stable set of releases. This resolves a version inconsistency introduced by forward-referencing newer module versions that were not yet intended to be consumed by downstream packages.
## Changes
- Incremented `core/version` from `1.4.21` to `1.4.22`
- Downgraded `bifrost/core` from `v1.4.19` → `v1.4.17` across `framework`, `governance`, `jsonparser`, `litellmcompat`, `logging`, `maxim`, `mocker`, `otel`, `semanticcache`, and `telemetry` plugins
- Downgraded `bifrost/framework` from `v1.2.38` → `v1.2.36` (or `v1.2.35` for `semanticcache`) across all dependent plugins
- Downgraded `bifrost/core` in `transports` from `v1.4.20` → `v1.4.19`
- Downgraded all plugin versions referenced in `transports` (governance, litellmcompat, logging, maxim, otel, semanticcache, telemetry) to their corresponding prior releases
- Downgraded `go.opentelemetry.io/otel/sdk` and `go.opentelemetry.io/otel/sdk/metric` from `v1.43.0` → `v1.40.0` in affected plugins
- Bumped Go toolchain version in `transports/go.mod` from `1.26.1` to `1.26.2`
## Type of change
- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [x] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
```sh
go test ./...
```
Verify that all modules resolve correctly with the pinned dependency versions and that no import errors occur during build.
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
None. These are internal module version adjustments with no changes to auth, secrets, or data handling.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* bumped up hello-world dep (#2819)
## Summary
Pins the `bifrost/core` dependency in the example plugin modules to a consistent released version (`v1.4.17`), removing a local `replace` directive that was pointing to the local `core` module path.
## Changes
- Replaced the local `replace` directive in `hello-world-wasm-go/go.mod` with a direct reference to `github.com/maximhq/bifrost/core v1.4.17`
- Downgraded `hello-world/go.mod` from `v1.4.19` to `v1.4.17` to align both example plugins on the same released version
## Type of change
- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [x] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
```sh
cd examples/plugins/hello-world-wasm-go
go mod tidy
go build ./...
cd examples/plugins/hello-world
go mod tidy
go build ./...
```
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
## Security considerations
No security implications. This change only affects dependency resolution for example plugin modules.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* framework: bump core to v1.4.22 --skip-pipeline
* plugins/governance: bump core to v1.4.22 and framework to v1.2.39 --skip-pipeline
* plugins/jsonparser: bump core to v1.4.22 and framework to v1.2.39 --skip-pipeline
* plugins/litellmcompat: bump core to v1.4.22 and framework to v1.2.39 --skip-pipeline
* plugins/logging: bump core to v1.4.22 and framework to v1.2.39 --skip-pipeline
* plugins/maxim: bump core to v1.4.22 and framework to v1.2.39 --skip-pipeline
* plugins/mocker: bump core to v1.4.22 and framework to v1.2.39 --skip-pipeline
* plugins/otel: bump core to v1.4.22 and framework to v1.2.39 --skip-pipeline
* plugins/semanticcache: bump core to v1.4.22 and framework to v1.2.39 --skip-pipeline
* plugins/telemetry: bump core to v1.4.22 and framework to v1.2.39 --skip-pipeline
* enforce go 1.26.1 (#2820)
* transports: update dependencies --skip-pipeline
* Adds changelog for v1.4.23 --skip-pipeline
* V1.5.0 (#2245)
* refactor: standardize empty array conventions for VK Provider & MCP Configs, and makes Provider Config weight optional for routing (#1932)
## Summary
Changes Virtual Key provider and MCP configurations from "allow-all by default" to "deny-by-default" security model. Virtual Keys now require explicit provider and MCP client configurations to allow access, improving security posture.
## Changes
- **Provider Configs**: Empty `provider_configs` now blocks all providers instead of allowing all
- **MCP Configs**: Empty `mcp_configs` now blocks all MCP tools instead of allowing all
- **Weight Field**: Changed provider `weight` from required `float64` to optional `*float64` - null weight excludes provider from weighted routin…
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Downgrade Go version from 1.26.2 to 1.26.1 across all GitHub Actions workflows, Go modules, and Docker images to address compatibility issues.
Changes
Type of change
Affected areas
How to test
Validate the Go version downgrade and streaming improvements:
Screenshots/Recordings
N/A
Breaking changes
Related issues
N/A
Security considerations
The streaming improvements enhance resource cleanup and prevent potential goroutine leaks when clients disconnect unexpectedly, improving overall system stability.
Checklist
docs/contributing/README.mdand followed the guidelines