V1.5.0

[Pratham-Mishra04]

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.

# 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

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 5 committers have signed the CLA.

✅ impoiler
✅ BearTS
❌ roroghost17
❌ akshaydeo
❌ danpiths
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[github-actions]

🧪 Test Suite Available

This PR can be tested by a repository admin.

Run tests for PR #2245

[coderabbitai]

Warning

Rate limit exceeded

@akshaydeo has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 6 minutes and 8 seconds before requesting another review.

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 6 minutes and 8 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 883fe94a-b6d5-4a39-93aa-2ababe654feb

📥 Commits

Reviewing files that changed from the base of the PR and between 34d5d79 and f42eac2.

⛔ Files ignored due to path filters (1)

• core/go.sum is excluded by !**/*.sum

📒 Files selected for processing (43)

• .github/workflows/helm-release.yml
• .github/workflows/release-pipeline.yml
• .github/workflows/scripts/run-migration-tests.sh
• .github/workflows/scripts/setup-go-workspace.sh
• .github/workflows/scripts/validate-helm-config-fields.sh
• core/bifrost.go
• core/bifrost_test.go
• core/go.mod
• core/internal/llmtests/account.go
• core/mcp/utils.go
• core/providers/anthropic/anthropic.go
• core/providers/anthropic/chat.go
• core/providers/anthropic/chat_test.go
• core/providers/anthropic/responses.go
• core/providers/anthropic/types.go
• core/providers/anthropic/utils.go
• core/providers/azure/azure.go
• core/providers/azure/azure_test.go
• core/providers/bedrock/bedrock.go
• core/providers/bedrock/bedrock_test.go
• core/providers/bedrock/invoke.go
• core/providers/bedrock/types.go
• core/providers/cohere/cohere.go
• core/providers/elevenlabs/elevenlabs.go
• core/providers/gemini/gemini.go
• core/providers/gemini/types.go
• core/providers/huggingface/huggingface.go
• core/providers/mistral/mistral.go
• core/providers/openai/chat_test.go
• core/providers/openai/openai.go
• core/providers/openai/types.go
• core/providers/perplexity/chat.go
• core/providers/replicate/replicate.go
• core/providers/utils/utils.go
• core/providers/vertex/vertex.go
• core/providers/vertex/vertex_test.go
• core/providers/vllm/vllm.go
• core/schemas/chatcompletions.go
• core/schemas/images.go
• docs/docs.json
• docs/openapi/openapi.json
• examples/plugins/hello-world/go.mod
• framework/configstore/migrations.go

📝 Walkthrough

Walkthrough

Added pooled plugin‑scoped contexts and plugin log draining; centralized model-listing via a ListModelsPipeline with WhiteList/BlackList and aliases/backfill; extended MCP/ToolsManager for per-user OAuth, header forwarding, and disable-auto-inject; many providers stopped injecting provider/request metadata into errors/ExtraFields; clarified resolve-pr-comments skill as local-only. (48 words)

Changes

Cohort / File(s)	Summary
Core engine & plugin context core/bifrost.go, core/schemas/context.go, core/schemas/trace.go, core/schemas/bifrost.go	Added pooled plugin-scoped contexts (WithPluginScope/ReleasePluginScope), plugin log store/drain APIs, trace RequestID/PluginLogs, and wired plugin-log draining/attachment into streaming/post-hook flows.
Model-listing pipeline & shared utils core/providers/utils/models.go, core/providers/*/models.go	Introduced ListModelsPipeline and moved per-provider allow/deny/alias/backfill logic to pipeline; provider list-model conversions now take schemas.WhiteList/schemas.BlackList and aliases.
Schemas & MCP types core/schemas/account.go, core/schemas/mcp.go, core/schemas/images.go, core/schemas/transcriptions.go, core/schemas/envvar.go	Added WhiteList/BlackList types and helpers; updated Key and MCP config fields (models, blacklists, disable-auto-tool-inject, allowed extra headers); added MCP context keys and plugin log entry type; EnvVar.IsDefined added.
MCP manager, ToolsManager & headers core/mcp/toolmanager.go, core/mcp/clientmanager.go, core/mcp/mcp.go, core/mcp/interface.go, core/mcp/utils/utils.go	Wired oauth2 provider into ToolsManager; added VerifyPerUserOAuthConnection and SetClientTools; exposed SetPluginPipeline; changed public MCP APIs to accept *schemas.BifrostContext; added GetHeadersForToolExecution; implemented per-user OAuth execution path and disable-auto-inject gating.
Starlark / CodeMode core/mcp/codemode/..., core/mcp/codemode/starlark/...	CodeMode tool execution now uses *schemas.BifrostContext; added canonical/compat tool-name helpers and matching; removed non-pipeline execution paths; improved Starlark dialect options and timeout handling; centralized tool execution headers.
Agent, adapters & usage accounting core/mcp/agent.go, core/mcp/agentadaptors.go, core/mcp/healthmonitor.go	Added usage accumulation (mergeUsage) and adapter extractUsage/applyUsage; health-check snapshotting; auto-tool execution captures per-user-OAuth errors and returns structured MCP-auth-required errors.
Provider conversions & error handling core/providers/... (many providers)	Removed provider/request metadata enrichment from numerous ExtraFields and error constructors; refactored many parseError signatures to drop request metadata; standardized model attribution to OriginalModelRequested/ResolvedModelUsed; Bedrock gained Stability AI and Cohere-on-Bedrock image/embedding helpers.
Image utilities & sizing core/providers/utils/images.go, core/providers/bedrock/images.go, core/providers/replicate/images.go	Added ConvertSizeToAspectRatioAndResolution; added Stability AI image generation/edit converters for Bedrock; updated Replicate image conversion to use shared converter.
Streaming, realtime & hooks core/bifrost.go, core/schemas/context.go, core/providers/utils/utils.go, core/providers/*/realtime.go	Cached streaming plugin-scoped contexts, released on reset; plugin-log draining/attachment across stream boundaries; added realtime turn hooks and tracer lifecycle integration.
Tests, fixtures & CI tooling core/internal/mcptests/*, core/internal/llmtests/*, .github/workflows/scripts/*, .github/workflows/*	Updated tests to use schemas.MCPContextKey* and WhiteList types; added noopPluginPipeline fixture; adapted tests to OriginalModelRequested/ResolvedModelUsed; added schemasync tool and CI validation scripts; bumped version and Go toolchain.
Docs, skills & templates .agents/skills/*, .claude/skills/*, AGENTS.md, .github/*	Added expect skill docs; clarified .claude/skills/resolve-pr-comments/SKILL.md to be local-only (no commits/pushes); updated docs/templates to reflect UI stack (React + Vite) and related config changes.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Client as Client
  participant Bifrost as Bifrost
  participant ToolsMgr as ToolsManager
  participant OAuth as OAuthProvider
  participant MCPClient as MCPClient
  participant Tracer as Tracer

  Note over Client,Bifrost: Incoming request with BifrostContext (may include user identity / include-filters)
  Client->>Bifrost: POST /api (ctx *schemas.BifrostContext, req)
  Bifrost->>ToolsMgr: ParseAndAddToolsToRequest(ctx, req)
  ToolsMgr->>ToolsMgr: Check disableAutoToolInject & ctx include filters
  alt per-user OAuth required
    ToolsMgr->>OAuth: VerifyPerUserOAuthConnection(ctx, config, accessToken)
    OAuth-->>ToolsMgr: discovered tools or authorize URL/session
    ToolsMgr->>MCPClient: Execute tool call with temporary client (Bearer token)
  else standard execution
    ToolsMgr->>MCPClient: Execute tool call (persistent conn with headers from GetHeadersForToolExecution)
  end
  MCPClient-->>ToolsMgr: Tool result or MCPUserOAuthRequiredError
  ToolsMgr-->>Bifrost: Attach tool result to request flow
  Bifrost->>Bifrost: DrainPluginLogs(ctx.DrainPluginLogs)
  Bifrost->>Tracer: AttachPluginLogs(traceID, logs)
  Bifrost-->>Client: Final response (with plugin logs attached in trace/extra)

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120+ minutes

Possibly related PRs

• exception fixes #2676 — Overlaps plugin-scoped contexts, transport post-hook completer, and context value snapshot/restore changes.
• feat: add realtime provider interfaces, schemas, and engine hooks #2337 — Related realtime/hooks additions and SelectKeyForProviderRequestType changes.
• feat: add model alias #2355 — Related model alias resolution and OriginalModelRequested/ResolvedModelUsed plumbing.

Suggested reviewers

• danpiths

Poem

🐰 I hopped through scoped contexts late at night,

Collected plugin logs and kept them tight.
White‑lists, blacklists, aliases all in a row,
OAuth handshakes fetched so tool‑calls can go.
Pipelines hum softly — the repo’s shining bright.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch v1.5.0

[coderabbitai]

Actionable comments posted: 20

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (9)

core/schemas/provider.go (1)

462-467: ⚠️ Potential issue | 🟠 Major

Clone RequestPathOverrides in CheckAndSetDefaults() too.

Line 462 already does the right thing for ExtraHeaders, but CustomProviderConfig.RequestPathOverrides is still shared by reference. A post-init mutation there can race with live requests and change routing unexpectedly.

♻️ Proposed fix

 	// Create a defensive copy of ExtraHeaders to prevent data races
 	if config.NetworkConfig.ExtraHeaders != nil {
 		headersCopy := make(map[string]string, len(config.NetworkConfig.ExtraHeaders))
 		maps.Copy(headersCopy, config.NetworkConfig.ExtraHeaders)
 		config.NetworkConfig.ExtraHeaders = headersCopy
 	}
+	if config.CustomProviderConfig != nil && config.CustomProviderConfig.RequestPathOverrides != nil {
+		overridesCopy := make(map[RequestType]string, len(config.CustomProviderConfig.RequestPathOverrides))
+		maps.Copy(overridesCopy, config.CustomProviderConfig.RequestPathOverrides)
+		config.CustomProviderConfig.RequestPathOverrides = overridesCopy
+	}
 }

As per coding guidelines, "Deep-copy map fields in config structs using maps.Copy() in CheckAndSetDefaults() to prevent data races between concurrent requests."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/schemas/provider.go` around lines 462 - 467, CheckAndSetDefaults()
currently defensively copies NetworkConfig.ExtraHeaders but does not copy
CustomProviderConfig.RequestPathOverrides, leaving RequestPathOverrides shared
by reference and susceptible to post-init mutation races; update
CheckAndSetDefaults() to clone RequestPathOverrides when non-nil by allocating a
new map with the same length and using maps.Copy (similar to the headersCopy
logic) and assign that new map back to
config.CustomProviderConfig.RequestPathOverrides so runtime mutations cannot
affect the original shared map.

docs/features/governance/routing.mdx (1)

84-99: ⚠️ Potential issue | 🟡 Minor

Keep this json example valid JSON.

Lines 90 and 95 add // comments inside a json code fence, so readers can't paste this into config.json as shown. Move the explanation into surrounding prose, or switch the fence to a comment-capable format if the docs renderer supports it.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/features/governance/routing.mdx` around lines 84 - 99, The JSON example
contains inline `//` comments which make it invalid JSON (see the
`allowed_models": ["*"]` entries); either remove the `//` comments from inside
the ```json``` fence so the snippet remains valid JSON, or change the code fence
to a comment-capable format supported by the renderer (for example `jsonc` or
`js`) and keep the explanatory text outside the object; update the surrounding
prose to explain that `["*"]` allows all and that validation is handled by the
Model Catalog so readers can paste the shown object into config.json without
syntax errors.

transports/bifrost-http/handlers/mcp.go (1)

199-214: ⚠️ Potential issue | 🟠 Major

Paginated MCP client responses still drop allowed_extra_headers.

addMCPClient and updateMCPClient now persist this field, but the paginated GET path rebuilds schemas.MCPClientConfig without it. As soon as the UI uses limit, offset, or search, the returned config is incomplete.

♻️ Proposed fix

 		clientConfig := &schemas.MCPClientConfig{
 			ID:                 dbClient.ClientID,
 			Name:               dbClient.Name,
 			IsCodeModeClient:   dbClient.IsCodeModeClient,
 			ConnectionType:     schemas.MCPConnectionType(dbClient.ConnectionType),
 			ConnectionString:   dbClient.ConnectionString,
 			StdioConfig:        dbClient.StdioConfig,
 			AuthType:           schemas.MCPAuthType(dbClient.AuthType),
 			OauthConfigID:      dbClient.OauthConfigID,
 			ToolsToExecute:     dbClient.ToolsToExecute,
 			ToolsToAutoExecute: dbClient.ToolsToAutoExecute,
 			Headers:            dbClient.Headers,
+			AllowedExtraHeaders: dbClient.AllowedExtraHeaders,
 			IsPingAvailable:    &isPingAvailable,
 			ToolSyncInterval:   time.Duration(dbClient.ToolSyncInterval) * time.Minute,
 			ToolPricing:        dbClient.ToolPricing,
 		}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@transports/bifrost-http/handlers/mcp.go` around lines 199 - 214, The
paginated GET builds a schemas.MCPClientConfig (in the handler that creates
clientConfig) but omits the AllowedExtraHeaders field, so configs returned for
paginated queries drop that data; update the clientConfig construction to map
dbClient.AllowedExtraHeaders into schemas.MCPClientConfig.AllowedExtraHeaders
(same place where ToolPricing, Headers, etc. are set) so paginated GET responses
include allowed_extra_headers (consistent with addMCPClient/updateMCPClient
persistence).

transports/bifrost-http/handlers/providers.go (1)

741-775: ⚠️ Potential issue | 🟠 Major

Unknown key IDs should not widen the result set to all models.

If keys= is present but every ID is stale or belongs to another provider, this branch still falls back to the full model list. That makes GET /api/models?keys=... behave like allow-all instead of “models accessible by these keys.”

♻️ Proposed fix

 	allowedModels := make(map[string]bool)
 	hasRestrictedKey := false
 	hasUnrestrictedKey := false
 	hasDenyAllKey := false
+	matchedAnyKey := false
 	for _, keyID := range keyIDs {
 		for _, key := range config.Keys {
 			if key.ID == keyID {
+				matchedAnyKey = true
 				if key.Models.IsUnrestricted() {
 					// Key allows all models (wildcard)
 					hasUnrestrictedKey = true
@@
 	// If no keys were matched or restricted, but at least one key explicitly denies all, return nothing
 	if !hasRestrictedKey && hasDenyAllKey {
 		return []string{}
 	}
-	// If no keys have model restrictions (e.g., unknown key IDs), return all models
+	// Unknown/stale key IDs should not expand access.
 	if !hasRestrictedKey {
+		if len(keyIDs) > 0 && !matchedAnyKey {
+			return []string{}
+		}
 		return models
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@transports/bifrost-http/handlers/providers.go` around lines 741 - 775, The
current logic treats unknown/stale key IDs as "no keys provided" and returns the
full model list; add tracking for whether any provided key ID was matched (e.g.,
a boolean anyMatched or matchedCount incremented inside the key.ID == keyID
branch) and use it to change the fallback: after the loop, if len(keyIDs) > 0 &&
!anyMatched return an empty slice (no models) so stale/foreign key IDs do not
widen access; also adjust the final fallback condition that currently returns
models when !hasRestrictedKey to only do so when no key IDs were supplied (e.g.,
if !hasRestrictedKey && len(keyIDs) == 0 return models) and keep the existing
hasUnrestrictedKey and hasDenyAllKey handling intact (refer to variables keyIDs,
config.Keys, hasRestrictedKey, hasUnrestrictedKey, hasDenyAllKey, allowedModels,
and models).

ui/components/ui/modelMultiselect.tsx (1)

114-165: ⚠️ Potential issue | 🟡 Minor

Add unfiltered to the loadOptions dependency array.

The callback reads unfiltered in the getModels request payload (line 148) but it's missing from the dependency array (line 164). If the prop changes, subsequent search requests will use the previous value until another dependency triggers a recreation.

Fix

-		[getModels, getBaseModels, provider, keys, shouldLoadOnEmpty, shouldUseBaseModels, allowAllOption],
+		[getModels, getBaseModels, provider, keys, shouldLoadOnEmpty, shouldUseBaseModels, allowAllOption, unfiltered],

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ui/components/ui/modelMultiselect.tsx` around lines 114 - 165, The
loadOptions callback captures the unfiltered variable but the dependency array
for useCallback omits it, so changes to unfiltered won't recreate loadOptions;
update the dependency array for the useCallback that defines loadOptions to
include unfiltered alongside getModels, getBaseModels, provider, keys,
shouldLoadOnEmpty, shouldUseBaseModels, and allowAllOption so the closure sees
the latest unfiltered value when issuing getModels requests.

framework/configstore/tables/virtualkey.go (1)

47-76: ⚠️ Potential issue | 🟠 Major

Keep the old allowed_keys config input working for at least one release.

This unmarshaller no longer reads the previous allowed_keys field, so existing config.json entries will deserialize to AllowAllKeys=false and Keys=nil after upgrade. With the new deny-by-default semantics, that silently blocks every provider key for affected virtual keys unless every config file is migrated in lockstep.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@framework/configstore/tables/virtualkey.go` around lines 47 - 76, The custom
UnmarshalJSON on TableVirtualKeyProviderConfig must preserve the old
allowed_keys field: update UnmarshalJSON (and TempProviderConfig) to also read
an AllowedKeys []string `json:"allowed_keys"` and, when temp.KeyIDs is empty,
use temp.AllowedKeys to populate pc.Keys or set pc.AllowAllKeys (treat ["*"] as
allow all) so existing configs deserialize unchanged; ensure key_ids takes
precedence over allowed_keys if both are present and keep the existing logic
that skips conversion when pc.Keys is already set.

ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx (1)

747-852: ⚠️ Potential issue | 🟡 Minor

Add stable selectors for the new wildcard key/tool controls.

These changed pickers drive the new allow-all flows, but they still don't expose data-testids the way the surrounding controls do. That makes the new behavior much harder to cover in E2E.

As per coding guidelines ui/**/*.{tsx,ts}: Add new interactive UI elements with data-testid attributes following the pattern: data-testid="<entity>-<element>-<qualifier>".

Also applies to: 1058-1098

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` around lines 747 -
852, Add stable data-testid attributes to the new wildcard key/tool pickers so
E2E can target them: in the AsyncMultiSelect instance used for "Allowed Keys"
(component AsyncMultiSelect) add data-testid="virtualkey-allowedkeys-select";
inside its custom multiValue renderer (multiValueProps) add
data-testid="virtualkey-allowedkeys-chip" to the outer div and
data-testid="virtualkey-allowedkeys-chip-remove" to the X remove button; inside
the custom option renderer (optionProps) add
data-testid="virtualkey-allowedkeys-option" to the Option wrapper or its label
span; and repeat the same pattern for the other AsyncMultiSelect instance later
in the file that implements the wildcard behavior so both pickers follow the
data-testid pattern.

core/bifrost.go (1)

3416-3420: ⚠️ Potential issue | 🟠 Major

These exported MCP API changes are source-incompatible.

GetAvailableMCPTools now requires *schemas.BifrostContext, and UpdateToolManagerConfig adds a positional bool. Both changes compile-break existing callers; the first also stops following the usual nil-context fallback used by the rest of Bifrost. Prefer keeping the current exported signatures and converting/wrapping internally, or add new opt-in methods instead.

Also applies to: 3543-3555

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/bifrost.go` around lines 3416 - 3420, The change to exported APIs is
source-incompatible: restore the previous public signatures for
GetAvailableMCPTools and UpdateToolManagerConfig and perform any new-argument
conversion internally; specifically, revert GetAvailableMCPTools to accept the
original parameter type (or no context) and inside that method convert or
construct a *schemas.BifrostContext before calling
bifrost.MCPManager.GetAvailableTools so you preserve the existing nil-context
fallback behavior used across Bifrost, and for UpdateToolManagerConfig remove
the new positional bool from the exported signature (or add a new opt-in method
like UpdateToolManagerConfigWithOptions) and handle the boolean internally or
via the new opt-in function so existing callers continue to compile. Ensure
references to GetAvailableMCPTools, UpdateToolManagerConfig, and
Bifrost.MCPManager are updated accordingly.

framework/configstore/tables/key.go (1)

479-483: ⚠️ Potential issue | 🟠 Major

Reset Models when ModelsJSON is empty to avoid stale runtime state.

Line 479 currently skips assignment when ModelsJSON == "", but Models is a non-persisted field (gorm:"-"). If the same struct instance is reused, the previous row’s Models can leak into the current row.

💡 Proposed fix

-	if k.ModelsJSON != "" {
-		if err := json.Unmarshal([]byte(k.ModelsJSON), &k.Models); err != nil {
-			return err
-		}
-	}
+	if k.ModelsJSON != "" {
+		if err := json.Unmarshal([]byte(k.ModelsJSON), &k.Models); err != nil {
+			return err
+		}
+	} else {
+		k.Models = nil
+	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@framework/configstore/tables/key.go` around lines 479 - 483, When
deserializing into the struct, ensure k.Models is cleared when k.ModelsJSON is
empty to avoid leaking previous in-memory state; inside the block that currently
only unmarshals when k.ModelsJSON != "" (referencing k.ModelsJSON and k.Models)
set k.Models = nil or an empty slice in the else case (or before the
conditional) so that reused instances don't keep stale Models data.

🟡 Minor comments (13)

ui/app/workspace/logs/sheets/logDetailsSheet.tsx-173-187 (1)

173-187: ⚠️ Potential issue | 🟡 Minor

Add missing test selectors on modified dropdown controls.

Line 173 (Button trigger) and Line 184 (DropdownMenuItem for delete) are interactive controls in this changed block but don’t expose data-testid, which makes E2E targeting brittle in this sheet.

Suggested patch

-										<Button variant="ghost" size="icon">
+										<Button variant="ghost" size="icon" data-testid="log-details-menu-button">
 											<MoreVertical className="h-3 w-3" />
 										</Button>
...
-											<DropdownMenuItem variant="destructive">
+											<DropdownMenuItem variant="destructive" data-testid="log-details-delete-log-button">
 												<Trash2 className="h-4 w-4" />
 												Delete log
 											</DropdownMenuItem>

As per coding guidelines ui/**/*.{tsx,ts}: “Add new interactive UI elements with data-testid attributes following the pattern: data-testid="<entity>-<element>-<qualifier>".”

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ui/app/workspace/logs/sheets/logDetailsSheet.tsx` around lines 173 - 187, The
dropdown trigger Button (component using MoreVertical) and the destructive
DropdownMenuItem wrapped by AlertDialogTrigger (the "Delete log" item) lack
data-testid attributes; add data-testid attributes to both following the project
pattern (e.g., data-testid="logdetails-dropdown-trigger" for the Button and
data-testid="logdetails-delete-button" for the delete DropdownMenuItem) so E2E
tests can reliably target them—ensure you modify the Button element and the
DropdownMenuItem (the one that triggers the AlertDialog for deletion) and keep
existing handlers like copyRequestBody(displayLog) unchanged.

docs/mcp/filtering.mdx-230-230 (1)

230-230: ⚠️ Potential issue | 🟡 Minor

Document auto-generated MCP include header as conditional.

Line 230 currently reads as unconditional override behavior. With the new DisableAutoToolInject wiring, automatic x-bf-mcp-include-tools injection/override should be documented as default behavior, not guaranteed behavior.

✏️ Suggested wording

-When a Virtual Key has no MCP configurations, **no MCP tools are available** (deny-by-default). You must explicitly add MCP client configurations to allow tools. When a Virtual Key has MCP configurations, it generates the `x-bf-mcp-include-tools` header automatically, overriding any manually sent header.
+When a Virtual Key has no MCP configurations, **no MCP tools are available** (deny-by-default). You must explicitly add MCP client configurations to allow tools. By default, when a Virtual Key has MCP configurations, Bifrost generates the `x-bf-mcp-include-tools` header automatically, overriding any manually sent header. If `mcp_disable_auto_tool_inject` is enabled, this automatic injection/override does not occur.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/mcp/filtering.mdx` at line 230, Update the sentence that claims the
`x-bf-mcp-include-tools` header is always generated/overrides manual headers to
state it is the default behavior unless auto-injection is disabled; mention the
new DisableAutoToolInject wiring (or flag) as the mechanism to disable automatic
injection, e.g., change the unconditional phrasing to: "By default a Virtual Key
with MCP configurations generates the `x-bf-mcp-include-tools` header,
overriding any manually sent header, unless auto-injection is disabled via
DisableAutoToolInject." Ensure the doc text around "no MCP tools are available
(deny-by-default)" and the sentence referencing `x-bf-mcp-include-tools`
reflects this conditional/default behavior.

docs/providers/request-options.mdx-32-32 (1)

32-32: ⚠️ Potential issue | 🟡 Minor

Document bare * support and use the exported Go context key constant.

The table and prose now describe only clientName-toolName and clientName-* formats, but the codebase treats bare ["*"] as the global allow-all sentinel (documented in changelog.md and used in filtering semantics). Additionally, the Go SDK example uses schemas.BifrostContextKey("mcp-include-tools") while the codebase defines and uses the typed constant schemas.MCPContextKeyIncludeTools for all MCP context operations, and all other context keys in the docs use this typed constant pattern.

📝 Proposed doc fix

-| `mcp-include-tools` | `x-bf-mcp-include-tools` | `[]string` | Filter MCP tools (`clientName-toolName` format, comma-separated) |
+| `mcp-include-tools` | `x-bf-mcp-include-tools` | `[]string` | Filter MCP tools (`*`, `clientName-*`, or `clientName-toolName`, comma-separated) |

-Filter MCP tools to include only the specified ones. Values must use the `clientName-toolName` format (e.g. `gmail-send_email`). Use `clientName-*` to include all tools from a client.
+Filter MCP tools to include only the specified ones. Values can be `*` to include all tools, `clientName-*` to include all tools from one client, or `clientName-toolName` (for example `gmail-send_email`) to include a specific tool.

-ctx = context.WithValue(ctx, schemas.BifrostContextKey("mcp-include-tools"), []string{"gmail-send_email", "filesystem-read_file"})
+ctx = context.WithValue(ctx, schemas.MCPContextKeyIncludeTools, []string{"gmail-send_email", "filesystem-read_file"})

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/providers/request-options.mdx` at line 32, Update the docs for the
`mcp-include-tools` (`x-bf-mcp-include-tools`) option to mention that a bare
["*"] is a global allow-all sentinel (in addition to `clientName-*` and
`clientName-toolName`), and update the Go SDK example to use the exported typed
context key `schemas.MCPContextKeyIncludeTools` instead of
`schemas.BifrostContextKey("mcp-include-tools")` so it matches the codebase’s
context-key usage and filtering semantics.

transports/bifrost-http/handlers/inference.go-749-765 (1)

749-765: ⚠️ Potential issue | 🟡 Minor

Avoid returning pricing: {} when no price field is set.

This now allocates &schemas.Pricing{} up front, so models with a catalog entry but no concrete costs start serializing an empty pricing object instead of omitting the field. That changes the wire shape and can look like pricing is available when it isn't.

💡 Suggested fix

-				pricing := &schemas.Pricing{}
+				var pricing *schemas.Pricing
+				ensurePricing := func() *schemas.Pricing {
+					if pricing == nil {
+						pricing = &schemas.Pricing{}
+					}
+					return pricing
+				}
 				if pricingEntry.InputCostPerToken != nil {
-					pricing.Prompt = bifrost.Ptr(fmt.Sprintf("%.10f", *pricingEntry.InputCostPerToken))
+					ensurePricing().Prompt = bifrost.Ptr(fmt.Sprintf("%.10f", *pricingEntry.InputCostPerToken))
 				}
 				if pricingEntry.OutputCostPerToken != nil {
-					pricing.Completion = bifrost.Ptr(fmt.Sprintf("%.10f", *pricingEntry.OutputCostPerToken))
+					ensurePricing().Completion = bifrost.Ptr(fmt.Sprintf("%.10f", *pricingEntry.OutputCostPerToken))
 				}
 				if pricingEntry.InputCostPerImage != nil {
-					pricing.Image = bifrost.Ptr(fmt.Sprintf("%.10f", *pricingEntry.InputCostPerImage))
+					ensurePricing().Image = bifrost.Ptr(fmt.Sprintf("%.10f", *pricingEntry.InputCostPerImage))
 				}
 				if pricingEntry.CacheReadInputTokenCost != nil {
-					pricing.InputCacheRead = bifrost.Ptr(fmt.Sprintf("%.10f", *pricingEntry.CacheReadInputTokenCost))
+					ensurePricing().InputCacheRead = bifrost.Ptr(fmt.Sprintf("%.10f", *pricingEntry.CacheReadInputTokenCost))
 				}
 				if pricingEntry.CacheCreationInputTokenCost != nil {
-					pricing.InputCacheWrite = bifrost.Ptr(fmt.Sprintf("%.10f", *pricingEntry.CacheCreationInputTokenCost))
+					ensurePricing().InputCacheWrite = bifrost.Ptr(fmt.Sprintf("%.10f", *pricingEntry.CacheCreationInputTokenCost))
 				}
-				resp.Data[i].Pricing = pricing
+				if pricing != nil {
+					resp.Data[i].Pricing = pricing
+				}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@transports/bifrost-http/handlers/inference.go` around lines 749 - 765, Do not
pre-allocate pricing; only set resp.Data[i].Pricing when at least one cost
exists: inspect pricingEntry fields (InputCostPerToken, OutputCostPerToken,
InputCostPerImage, CacheReadInputTokenCost, CacheCreationInputTokenCost) and
build a schemas.Pricing value only if any of those are non-nil, then assign it
to resp.Data[i].Pricing; otherwise leave resp.Data[i].Pricing nil so the pricing
field is omitted from the serialized output.

ui/app/workspace/config/views/mcpView.tsx-45-53 (1)

45-53: ⚠️ Potential issue | 🟡 Minor

Normalize defaulted MCP fields before computing hasChanges.

On configs that omit mcp_disable_auto_tool_inject, this compares undefined on the left to false on the right, so the page loads dirty and enables Save without any user edit. The same default-coalescing issue applies to the nearby defaulted MCP fields in this block.

🛠️ Suggested fix

 		return (
 			localConfig.mcp_agent_depth !== config.mcp_agent_depth ||
 			localConfig.mcp_tool_execution_timeout !== config.mcp_tool_execution_timeout ||
-			localConfig.mcp_code_mode_binding_level !== (config.mcp_code_mode_binding_level || "server") ||
-			localConfig.mcp_tool_sync_interval !== (config.mcp_tool_sync_interval ?? 10) ||
-			localConfig.mcp_disable_auto_tool_inject !== (config.mcp_disable_auto_tool_inject ?? false)
+			(localConfig.mcp_code_mode_binding_level || "server") !== (config.mcp_code_mode_binding_level || "server") ||
+			(localConfig.mcp_tool_sync_interval ?? 10) !== (config.mcp_tool_sync_interval ?? 10) ||
+			(localConfig.mcp_disable_auto_tool_inject ?? false) !== (config.mcp_disable_auto_tool_inject ?? false)
 		);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ui/app/workspace/config/views/mcpView.tsx` around lines 45 - 53, The
hasChanges computation compares localConfig fields to config fields without
normalizing defaults, causing false positives (e.g.,
localConfig.mcp_disable_auto_tool_inject === false vs
config.mcp_disable_auto_tool_inject === undefined). Update the useMemo for
hasChanges to normalize/coalesce the config-side values to the same defaults you
expect on localConfig (apply (config.mcp_code_mode_binding_level || "server"),
(config.mcp_tool_sync_interval ?? 10), (config.mcp_disable_auto_tool_inject ??
false), etc.) before comparing; ensure you reference the existing hasChanges
useMemo and the localConfig/config mcp_* fields so both sides use identical
defaulting logic.

transports/changelog.md-5-5 (1)

5-5: ⚠️ Potential issue | 🟡 Minor

Minor: Use hyphen for compound adjective.

"request level" should be hyphenated to "request-level" when used as a compound adjective modifying "extra headers".

📝 Suggested fix

-- feat: add support for request level extra headers in MCP tool execution.
+- feat: add support for request-level extra headers in MCP tool execution.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@transports/changelog.md` at line 5, Update the changelog entry text to use
the hyphenated compound adjective: replace "request level extra headers" with
"request-level extra headers" so the line reads "feat: add support for
request-level extra headers in MCP tool execution."

framework/configstore/tables/pricingoverride.go-24-25 (1)

24-25: ⚠️ Potential issue | 🟡 Minor

Add GORM auto-timestamp tags for CreatedAt and UpdatedAt.

Other GORM models in this codebase use gorm:"autoCreateTime" and gorm:"autoUpdateTime" to let GORM automatically populate these fields. Without these tags, callers must manually set timestamps.

🛠️ Proposed fix

-	CreatedAt        time.Time `gorm:"index;not null" json:"created_at"`
-	UpdatedAt        time.Time `gorm:"index;not null" json:"updated_at"`
+	CreatedAt        time.Time `gorm:"index;not null;autoCreateTime" json:"created_at"`
+	UpdatedAt        time.Time `gorm:"index;not null;autoUpdateTime" json:"updated_at"`

Based on learnings: "In this codebase using GORM, models with CreatedAt and UpdatedAt fields of type time.Time tagged with gorm:"autoCreateTime" and gorm:"autoUpdateTime" are populated automatically by GORM on insert/update."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@framework/configstore/tables/pricingoverride.go` around lines 24 - 25, The
CreatedAt and UpdatedAt fields on the PricingOverride model are missing GORM
auto-timestamp tags so GORM won't auto-populate them; update the struct tags for
CreatedAt and UpdatedAt in pricingoverride.go to include gorm:"autoCreateTime"
for CreatedAt and gorm:"autoUpdateTime" for UpdatedAt (preserve existing
index/not null and json tags) so GORM will set timestamps automatically on
insert/update.

ui/components/ui/modelMultiselect.tsx-116-119 (1)

116-119: ⚠️ Potential issue | 🟡 Minor

Include the wildcard option when the user searches for "*".

The backing value for “Allow All Models” is "*", but this matcher only prepends it for empty input or text matching "allow all models". With the paired backend wildcard handling, typing * now returns every concrete model while hiding the actual allow-all option.

♻️ Proposed fix

-			const prefix: ModelOption[] = allowAllOption && (!query || "allow all models".includes(query.toLowerCase()))
+			const normalizedQuery = query.trim().toLowerCase();
+			const prefix: ModelOption[] = allowAllOption && (
+				!normalizedQuery ||
+				normalizedQuery === "*" ||
+				"allow all models".includes(normalizedQuery)
+			)
 				? [ALL_MODELS_OPTION]
 				: [];

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ui/components/ui/modelMultiselect.tsx` around lines 116 - 119, The prefix
logic for prepending ALL_MODELS_OPTION doesn't consider the wildcard character,
so when query === "*" the allow-all option is omitted; update the condition in
the block that computes prefix (referencing allowAllOption, prefix,
ALL_MODELS_OPTION, and query) to also treat a query of "*" (and trimmed "*" with
surrounding whitespace) as a match — i.e., if allowAllOption is true and (query
is empty OR query lowercased includes "allow all models" OR query trimmed equals
"*"), then include ALL_MODELS_OPTION in prefix.

core/mcp/toolmanager.go-197-203 (1)

197-203: ⚠️ Potential issue | 🟡 Minor

Record tools only when they are actually returned.

BifrostContextKeyMCPAddedTools is appended before the IsCodeModeClient gate, so code-mode client tool names are recorded even though they are not added to availableTools. The actual code-mode tools appended later are never recorded, so downstream auditing/plugin logic sees the wrong tool set.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/mcp/toolmanager.go` around lines 197 - 203, The code currently calls
schemas.AppendToContextList(ctx, schemas.BifrostContextKeyMCPAddedTools,
tool.Function.Name) before checking client.ExecutionConfig.IsCodeModeClient, so
code-mode tool names get recorded even though they aren’t added to
availableTools and actual code-mode tools added later aren't recorded; fix by
moving the AppendToContextList call to be executed only when the tool is
actually appended to availableTools (i.e., place it inside the same branch that
appends to availableTools), and ensure any separate code-mode branch that later
appends tools also calls schemas.AppendToContextList for those tools; reference
symbols: seenToolNames, schemas.AppendToContextList,
BifrostContextKeyMCPAddedTools, client.ExecutionConfig.IsCodeModeClient,
availableTools.

docs/openapi/schemas/management/governance.yaml-1160-1207 (1)

1160-1207: ⚠️ Potential issue | 🟡 Minor

Encode the scope-specific ID requirements in the schema.

These requests describe virtual_key_id, provider_id, and provider_key_id as required for certain scope_kind values, but the OpenAPI shape leaves all three optional. Unlike RoutingRule above, generated clients can't validate those combinations, so they can build requests the server will reject. Add oneOf variants per scope so the contract is enforceable.

Also applies to: 1208-1251

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/openapi/schemas/management/governance.yaml` around lines 1160 - 1207,
The CreatePricingOverrideRequest schema currently lists virtual_key_id,
provider_id, and provider_key_id as optional but they must be required
conditionally by scope_kind; update CreatePricingOverrideRequest to include a
oneOf array of schema variants for each scope_kind value: one variant for
"global" (no extra IDs), "provider" (requires provider_id), "provider_key"
(requires provider_key_id), "virtual_key" (requires virtual_key_id),
"virtual_key_provider" (requires virtual_key_id and provider_id), and
"virtual_key_provider_key" (requires virtual_key_id and provider_key_id); ensure
each variant also includes the existing required base fields (name, scope_kind,
match_type, pattern, request_types) and preserves the existing properties like
patch and request_types so generated clients can validate the correct ID
combinations against scope_kind.

ui/lib/types/governance.ts-440-440 (1)

440-440: ⚠️ Potential issue | 🟡 Minor

Type inconsistency between request_types definitions.

PricingOverride.request_types (line 440) and UpdatePricingOverrideRequest.request_types (line 467) are typed as string[], but CreatePricingOverrideRequest.request_types (line 455) uses RequestType[]. For consistency and type safety, consider aligning all three to use RequestType[].

💡 Suggested fix

 export interface PricingOverride {
 	id: string;
 	name: string;
 	scope_kind: PricingOverrideScopeKind;
 	virtual_key_id?: string;
 	provider_id?: string;
 	provider_key_id?: string;
 	match_type: PricingOverrideMatchType;
 	pattern: string;
-	request_types?: string[];
+	request_types?: RequestType[];
 	pricing_patch: string;
 	config_hash?: string;
 	created_at: string;
 	updated_at: string;
 }

 export interface UpdatePricingOverrideRequest {
 	name?: string;
 	scope_kind?: PricingOverrideScopeKind;
 	virtual_key_id?: string;
 	provider_id?: string;
 	provider_key_id?: string;
 	match_type?: PricingOverrideMatchType;
 	pattern?: string;
-	request_types?: string[];
+	request_types?: RequestType[];
 	patch?: PricingOverridePatch;
 }

Also applies to: 467-467

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ui/lib/types/governance.ts` at line 440, PricingOverride.request_types and
UpdatePricingOverrideRequest.request_types are typed as string[] while
CreatePricingOverrideRequest.request_types uses RequestType[]; change those two
to RequestType[] to ensure consistency and type safety, update any import or
exported type references to include RequestType where needed, and run TypeScript
type checks to fix any resulting type errors referring to the properties on
PricingOverride, CreatePricingOverrideRequest, and UpdatePricingOverrideRequest.

ui/lib/types/schemas.ts-869-879 (1)

869-879: ⚠️ Potential issue | 🟡 Minor

Trim and reject blank allowed_extra_headers entries.

[" "] and [" * "] currently pass the element schema, and the wildcard refine only checks for the exact "*". That leaves this allowlist depending on downstream normalization and can persist unusable values.

Suggested validation tweak

 	allowed_extra_headers: z
-		.array(z.string())
+		.array(z.string().trim().min(1, "Header name cannot be empty"))
 		.optional()
 		.refine(
 			(headers) => {
 				if (!headers || headers.length === 0) return true;
 				const hasWildcard = headers.includes("*");
 				return !hasWildcard || headers.length === 1;
 			},
 			{ message: "Wildcard '*' cannot be combined with specific header names" },
 		),

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ui/lib/types/schemas.ts` around lines 869 - 879, The allowed_extra_headers
Zod schema currently accepts strings with surrounding whitespace and blank-only
values and only checks for an exact "*" in its refine; update the schema for
allowed_extra_headers to trim each element and reject empties by applying a
string transform/validation (e.g., transform to .trim() and then .refine(s =>
s.length > 0)), and change the array-level refine to detect wildcard after
trimming (treat "*" only when the trimmed element equals "*" and reject arrays
where a trimmed "*" coexists with other entries). Locate and modify the
allowed_extra_headers schema definition and its element/array refinements to
enforce trimming and blank-string rejection and to perform the wildcard check on
trimmed values.

framework/configstore/rdb.go-1320-1320 (1)

1320-1320: ⚠️ Potential issue | 🟡 Minor

Use a stable tie-breaker for pricing-override ordering.

These list queries sort only by created_at. When multiple overrides share the same timestamp, rows can reshuffle between pages and produce nondeterministic list results. The rest of this file already uses created_at ASC, id ASC for stable pagination.

Suggested fix

-	if err := q.Order("created_at ASC").Find(&overrides).Error; err != nil {
+	if err := q.Order("created_at ASC, id ASC").Find(&overrides).Error; err != nil {
 		return nil, s.parseGormError(err)
 	}

 	if err := baseQuery.
-		Order("created_at ASC").
+		Order("created_at ASC, id ASC").
 		Offset(offset).
 		Limit(limit).
 		Find(&overrides).Error; err != nil {

Also applies to: 1365-1369

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@framework/configstore/rdb.go` at line 1320, The list query orders overrides
only by created_at, which can yield unstable pagination when timestamps tie;
update the ordering in the queries that call q.Order("created_at ASC") (and the
other similar q.Order usages around the same area, e.g., the call near the block
at lines ~1365-1369) to include the id as a deterministic tie-breaker by
changing the order clause to "created_at ASC, id ASC" so results remain stable
across pages; ensure you update every occurrence in this file where overrides
are ordered by created_at alone.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 833294d1-d927-4103-a422-a0c0a4c477e3

📥 Commits

Reviewing files that changed from the base of the PR and between a3b439d and 11f1f5f.

⛔ Files ignored due to path filters (2)

• docs/media/ui-custom-pricing-form.png is excluded by !**/*.png
• docs/media/ui-custom-pricing-table.png is excluded by !**/*.png

📒 Files selected for processing (148)

• .claude/skills/resolve-pr-comments/SKILL.md
• core/bifrost.go
• core/changelog.md
• core/internal/mcptests/agent_test_helpers.go
• core/internal/mcptests/codemode_stdio_test.go
• core/internal/mcptests/concurrency_advanced_test.go
• core/internal/mcptests/fixtures.go
• core/mcp/agent.go
• core/mcp/clientmanager.go
• core/mcp/codemode.go
• core/mcp/codemode/starlark/executecode.go
• core/mcp/codemode/starlark/starlark.go
• core/mcp/interface.go
• core/mcp/mcp.go
• core/mcp/toolmanager.go
• core/mcp/utils.go
• core/mcp/utils/utils.go
• core/providers/anthropic/models.go
• core/providers/azure/models.go
• core/providers/bedrock/bedrock.go
• core/providers/bedrock/images.go
• core/providers/bedrock/models.go
• core/providers/bedrock/types.go
• core/providers/cohere/models.go
• core/providers/elevenlabs/models.go
• core/providers/gemini/models.go
• core/providers/huggingface/models.go
• core/providers/mistral/mistral.go
• core/providers/mistral/models.go
• core/providers/openai/models.go
• core/providers/openrouter/openrouter.go
• core/providers/replicate/models.go
• core/providers/utils/utils.go
• core/providers/vertex/models.go
• core/providers/vertex/utils.go
• core/providers/vertex/vertex.go
• core/schemas/account.go
• core/schemas/bifrost.go
• core/schemas/images.go
• core/schemas/mcp.go
• core/schemas/provider.go
• core/schemas/tracer.go
• core/schemas/transcriptions.go
• core/utils.go
• docs/architecture/framework/model-catalog.mdx
• docs/docs.json
• docs/features/governance/mcp-tools.mdx
• docs/features/governance/routing.mdx
• docs/mcp/filtering.mdx
• docs/openapi/openapi.json
• docs/openapi/openapi.yaml
• docs/openapi/paths/management/governance.yaml
• docs/openapi/schemas/management/governance.yaml
• docs/providers/custom-pricing.mdx
• docs/providers/provider-routing.mdx
• docs/providers/request-options.mdx
• docs/providers/supported-providers/bedrock.mdx
• examples/configs/partial/config.json
• examples/configs/withconfigstore/config.json
• examples/configs/withlogstore/config.json
• examples/configs/withpostgresmcpclientsinconfig/config.json
• examples/configs/withpricingoverridesnostore/config.json
• examples/configs/withpricingoverridessqlite/config.json
• examples/configs/withprompushgateway/config.json
• examples/configs/withvirtualkeys/config.json
• examples/mcps/auth-demo-server/main.go
• framework/changelog.md
• framework/configstore/clientconfig.go
• framework/configstore/migrations.go
• framework/configstore/rdb.go
• framework/configstore/store.go
• framework/configstore/tables/clientconfig.go
• framework/configstore/tables/key.go
• framework/configstore/tables/mcp.go
• framework/configstore/tables/modelpricing.go
• framework/configstore/tables/pricingoverride.go
• framework/configstore/tables/provider.go
• framework/configstore/tables/virtualkey.go
• framework/logstore/tables.go
• framework/modelcatalog/main.go
• framework/modelcatalog/main_test.go
• framework/modelcatalog/overrides.go
• framework/modelcatalog/overrides_test.go
• framework/modelcatalog/pricing.go
• framework/modelcatalog/pricing_test.go
• framework/modelcatalog/utils.go
• framework/streaming/audio.go
• framework/streaming/chat.go
• framework/streaming/images.go
• framework/streaming/responses.go
• framework/streaming/transcription.go
• framework/tracing/tracer.go
• helm-charts/bifrost/values.schema.json
• plugins/governance/changelog.md
• plugins/governance/main.go
• plugins/governance/resolver.go
• plugins/governance/resolver_test.go
• plugins/governance/utils.go
• plugins/logging/main.go
• plugins/logging/operations.go
• plugins/telemetry/main.go
• transports/bifrost-http/handlers/asyncinference.go
• transports/bifrost-http/handlers/config.go
• transports/bifrost-http/handlers/governance.go
• transports/bifrost-http/handlers/inference.go
• transports/bifrost-http/handlers/mcp.go
• transports/bifrost-http/handlers/mcpinference.go
• transports/bifrost-http/handlers/mcpserver.go
• transports/bifrost-http/handlers/pricing_override_test.go
• transports/bifrost-http/handlers/providers.go
• transports/bifrost-http/handlers/utils.go
• transports/bifrost-http/integrations/bedrock_test.go
• transports/bifrost-http/integrations/openai.go
• transports/bifrost-http/integrations/router.go
• transports/bifrost-http/lib/config.go
• transports/bifrost-http/lib/config_test.go
• transports/bifrost-http/lib/ctx.go
• transports/bifrost-http/lib/ctx_test.go
• transports/bifrost-http/server/plugins.go
• transports/bifrost-http/server/server.go
• transports/changelog.md
• transports/config.schema.json
• transports/schema_test/config_schema_test.go
• ui/app/workspace/config/views/mcpView.tsx
• ui/app/workspace/custom-pricing/overrides/page.tsx
• ui/app/workspace/custom-pricing/overrides/pricingFieldSelector.tsx
• ui/app/workspace/custom-pricing/overrides/pricingOverrideSheet.tsx
• ui/app/workspace/custom-pricing/overrides/pricingOverridesEmptyState.tsx
• ui/app/workspace/custom-pricing/overrides/scopedPricingOverridesView.tsx
• ui/app/workspace/logs/sheets/logDetailsSheet.tsx
• ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
• ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx
• ui/app/workspace/providers/fragments/index.ts
• ui/app/workspace/providers/fragments/pricingOverridesFormFragment.tsx
• ui/app/workspace/providers/views/providerKeyForm.tsx
• ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx
• ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
• ui/app/workspace/virtual-keys/views/virtualKeysTable.tsx
• ui/components/sidebar.tsx
• ui/components/ui/asyncMultiselect.tsx
• ui/components/ui/modelMultiselect.tsx
• ui/components/ui/numberAndSelect.tsx
• ui/lib/store/apis/baseApi.ts
• ui/lib/store/apis/governanceApi.ts
• ui/lib/types/config.ts
• ui/lib/types/governance.ts
• ui/lib/types/mcp.ts
• ui/lib/types/schemas.ts

💤 Files with no reviewable changes (2)

• ui/app/workspace/providers/fragments/pricingOverridesFormFragment.tsx
• ui/app/workspace/providers/fragments/index.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don't describe this as a sparse patch.

Line 180 suggests merge semantics, but TestUpdatePricingOverride_ReplacesFullBody in this PR verifies that patch is replaced wholesale: omitted fields are cleared. Please call that out here and show the complete desired patch object, otherwise users will accidentally wipe existing override fields.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/providers/custom-pricing.mdx` around lines 180 - 189, Remove the "sparse
patch" wording and explicitly document that the API's "patch" object is applied
wholesale (omitted fields are cleared) by aligning the docs with the test
TestUpdatePricingOverride_ReplacesFullBody; update the narrative around the curl
example in docs/providers/custom-pricing.mdx to state replacement semantics and
replace the partial JSON shown with a full desired "patch" object example so
users supply all fields they want to keep rather than accidentally wiping
omitted fields.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Do not log authentication secrets in plaintext.

The startup logs and printed config currently expose live X-API-Key and X-Tool-Token values, which can leak credentials via log aggregation and support dumps. Please redact or replace with placeholders.

🔐 Proposed fix

-	log.Printf("  Connection-level:   X-API-Key: %s  (middleware rejects all requests without it)", connectionAPIKey)
-	log.Printf("  Tool-execution:     X-Tool-Token: %s  (only secret_data checks this, validated inside the handler)", toolExecToken)
+	log.Printf("  Connection-level:   X-API-Key configured (middleware rejects requests without it)")
+	log.Printf("  Tool-execution:     X-Tool-Token configured (validated inside secret_data handler)")
@@
-    "X-API-Key":    "%s",
-    "X-Tool-Token": "%s"
+    "X-API-Key":    "<set-via-env-or-secret-manager>",
+    "X-Tool-Token": "<set-via-env-or-secret-manager>"
@@
-`, addr, connectionAPIKey, toolExecToken)
+`, addr)

Also applies to: 123-124, 128-128

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@examples/mcps/auth-demo-server/main.go` around lines 110 - 114, The startup
logs print sensitive values connectionAPIKey and toolExecToken directly via
log.Printf; change those log statements (the ones referencing "Connection-level:
X-API-Key" and "Tool-execution: X-Tool-Token" and the other related log.Printf
calls) to avoid plaintext secrets by replacing the raw variables with a redacted
placeholder or masked form (e.g., "[REDACTED]" or show only last N chars) before
logging; implement or call a simple maskSecret(value string) helper and use it
in those log.Printf calls so no full secret is emitted.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Hash RequestTypes before the row is persisted.

TablePricingOverride.RequestTypesJSON is json:"-", so config-origin overrides reach this helper with RequestTypes populated and RequestTypesJSON still empty. In that path, request_types changes don't affect the hash and reconciliation can miss the update.

Suggested fix

 func GeneratePricingOverrideHash(p tables.TablePricingOverride) (string, error) {
 	hash := sha256.New()
 	hash.Write([]byte(p.ID))
 	hash.Write([]byte(p.Name))
 	hash.Write([]byte(p.ScopeKind))
 	hash.Write([]byte(derefStr(p.VirtualKeyID)))
 	hash.Write([]byte(derefStr(p.ProviderID)))
 	hash.Write([]byte(derefStr(p.ProviderKeyID)))
 	hash.Write([]byte(p.MatchType))
 	hash.Write([]byte(p.Pattern))
-	hash.Write([]byte(p.RequestTypesJSON))
+	if p.RequestTypesJSON != "" {
+		hash.Write([]byte(p.RequestTypesJSON))
+	} else if len(p.RequestTypes) > 0 {
+		requestTypes := append([]schemas.RequestType(nil), p.RequestTypes...)
+		sort.Slice(requestTypes, func(i, j int) bool { return requestTypes[i] < requestTypes[j] })
+		data, err := sonic.Marshal(requestTypes)
+		if err != nil {
+			return "", err
+		}
+		hash.Write(data)
+	}
 	hash.Write([]byte(p.PricingPatchJSON))
 	return hex.EncodeToString(hash.Sum(nil)), nil
 }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	// GeneratePricingOverrideHash generates a SHA256 hash for a pricing override.
	// Skips: CreatedAt, UpdatedAt, ConfigHash (dynamic/meta fields).
	func GeneratePricingOverrideHash(p tables.TablePricingOverride) (string, error) {
	hash := sha256.New()
	hash.Write([]byte(p.ID))
	hash.Write([]byte(p.Name))
	hash.Write([]byte(p.ScopeKind))
	hash.Write([]byte(derefStr(p.VirtualKeyID)))
	hash.Write([]byte(derefStr(p.ProviderID)))
	hash.Write([]byte(derefStr(p.ProviderKeyID)))
	hash.Write([]byte(p.MatchType))
	hash.Write([]byte(p.Pattern))
	hash.Write([]byte(p.RequestTypesJSON))
	hash.Write([]byte(p.PricingPatchJSON))
	return hex.EncodeToString(hash.Sum(nil)), nil
	}
	// GeneratePricingOverrideHash generates a SHA256 hash for a pricing override.
	// Skips: CreatedAt, UpdatedAt, ConfigHash (dynamic/meta fields).
	func GeneratePricingOverrideHash(p tables.TablePricingOverride) (string, error) {
	hash := sha256.New()
	hash.Write([]byte(p.ID))
	hash.Write([]byte(p.Name))
	hash.Write([]byte(p.ScopeKind))
	hash.Write([]byte(derefStr(p.VirtualKeyID)))
	hash.Write([]byte(derefStr(p.ProviderID)))
	hash.Write([]byte(derefStr(p.ProviderKeyID)))
	hash.Write([]byte(p.MatchType))
	hash.Write([]byte(p.Pattern))
	if p.RequestTypesJSON != "" {
	hash.Write([]byte(p.RequestTypesJSON))
	} else if len(p.RequestTypes) > 0 {
	requestTypes := append([]schemas.RequestType(nil), p.RequestTypes...)
	sort.Slice(requestTypes, func(i, j int) bool { return requestTypes[i] < requestTypes[j] })
	data, err := sonic.Marshal(requestTypes)
	if err != nil {
	return "", err
	}
	hash.Write(data)
	}
	hash.Write([]byte(p.PricingPatchJSON))
	return hex.EncodeToString(hash.Sum(nil)), nil
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@framework/configstore/clientconfig.go` around lines 970 - 985,
GeneratePricingOverrideHash currently uses RequestTypesJSON which may be empty
for config-origin overrides; serialize and include the in-memory RequestTypes
field when RequestTypesJSON is empty so changes to RequestTypes affect the hash.
In GeneratePricingOverrideHash (and any callers), detect if p.RequestTypesJSON
is empty, marshal p.RequestTypes (using json.Marshal or a canonical JSON
serializer) and Write that byte slice into the hash (falling back to
RequestTypesJSON if non-empty), and propagate any marshal error out of
GeneratePricingOverrideHash instead of ignoring it.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Backfill legacy provider pricing overrides before switching to the new table.

This migration only creates/reconciles governance_pricing_overrides. The same PR stack removes provider-level pricing_overrides_json storage elsewhere, so any existing overrides stay stranded in config_providers and disappear from effective pricing after upgrade. Add a one-time copy into tables.TablePricingOverride before the old storage stops being read.

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 `@framework/configstore/migrations.go` around lines 4057 - 4072, The migration
migrationReconcilePricingOverridesTable currently only creates/automigrates
tables.TablePricingOverride but does not backfill existing provider-level
overrides stored in config_providers.pricing_overrides_json; add a one-time copy
step inside the Migrate function (after creating the new table or before
returning) that queries config_providers for non-null pricing_overrides_json,
unmarshals each JSON, and inserts corresponding rows into
tables.TablePricingOverride (avoiding dupes by checking provider_id or using
Upsert semantics on the new table) so legacy overrides are migrated before the
old field is removed.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don't return 200 after the in-memory delete fails.

At this point the DB row is already gone, so swallowing DeletePricingOverride errors leaves the active pricing catalog stale until the next reload/restart. Mirror the updateVirtualKey failure path here and fail the request instead of acknowledging success.

🔧 Proposed fix

 if err := h.governanceManager.DeletePricingOverride(ctx, id); err != nil {
-	logger.Warn("failed to delete pricing override from memory: %v", err)
+	logger.Error("failed to delete pricing override from memory: %v", err)
+	SendError(ctx, fasthttp.StatusInternalServerError, "Pricing override deleted in database but failed to update in-memory pricing state")
+	return
 }
 SendJSON(ctx, map[string]interface{}{
 	"message": "Pricing override deleted successfully",
 })

Based on learnings: if the database update succeeds but the in-memory GovernanceManager reload fails, respond with HTTP 500 rather than signaling success because the system relies on in-memory state for internal operations.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if err := h.governanceManager.DeletePricingOverride(ctx, id); err != nil {
	logger.Warn("failed to delete pricing override from memory: %v", err)
	}
	SendJSON(ctx, map[string]interface{}{
	"message": "Pricing override deleted successfully",
	})
	if err := h.governanceManager.DeletePricingOverride(ctx, id); err != nil {
	logger.Error("failed to delete pricing override from memory: %v", err)
	SendError(ctx, fasthttp.StatusInternalServerError, "Pricing override deleted in database but failed to update in-memory pricing state")
	return
	}
	SendJSON(ctx, map[string]interface{}{
	"message": "Pricing override deleted successfully",
	})

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@transports/bifrost-http/handlers/governance.go` around lines 3599 - 3604, The
current handler calls h.governanceManager.DeletePricingOverride and logs a
warning on error but still returns success via SendJSON; instead, mirror the
updateVirtualKey failure path: if h.governanceManager.DeletePricingOverride(ctx,
id) returns an error, log the error and return a 500 error response (using the
same error handling/response helper used by updateVirtualKey) and do not call
SendJSON to report success; only send the "Pricing override deleted
successfully" JSON when DeletePricingOverride succeeds.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Don't overwrite the request-level tool filter here.

This shadows any MCPContextKeyIncludeTools already present on the request with the broader virtual-key toolFilter. A caller can then skip tools/list and invoke a tool that the per-request filter meant to exclude. Intersect the existing filter with toolFilter instead of replacing it.

As per coding guidelines: core/mcp/**/*.go: Tool access follows 4-level filtering hierarchy: Global → Client-level → Tool-level → Per-request. All four levels must agree for tool availability.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@transports/bifrost-http/handlers/mcpserver.go` around lines 235 - 238, The
code currently overwrites any existing per-request tool filter when setting ctx
with context.WithValue(ctx, schemas.MCPContextKeyIncludeTools, toolFilter);
instead, read the existing value from ctx (using
schemas.MCPContextKeyIncludeTools), compute the intersection between that
existing filter and the broader toolFilter, and store the intersected result
back into the context; ensure the logic handles nil/empty cases (if existing is
nil use toolFilter, if toolFilter is nil keep existing) and keep the symbol
names ctx, toolFilter, schemas.MCPContextKeyIncludeTools, and context.WithValue
to locate and update the code path.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if backend CreateMCPClient handler accepts allowed_extra_headers
rg -n "allowed_extra_headers" --type go -C 3 transports/bifrost-http/handlers/mcp.go

Repository: maximhq/bifrost

Length of output: 817

---

🏁 Script executed:

cat -n ui/lib/types/mcp.ts | head -100

Repository: maximhq/bifrost

Length of output: 3846

---

🏁 Script executed:

#!/bin/bash
# Map the handlers to identify Create vs Update
rg -n "func.*CreateMCPClient\|func.*UpdateMCPClient" --type go transports/bifrost-http/handlers/mcp.go

Repository: maximhq/bifrost

Length of output: 41

---

🏁 Script executed:

sed -n '310,325p' transports/bifrost-http/handlers/mcp.go
sed -n '510,525p' transports/bifrost-http/handlers/mcp.go

Repository: maximhq/bifrost

Length of output: 1276

---

🏁 Script executed:

rg -n "func.*\(Create\|Update\).*MCP" --type go transports/bifrost-http/handlers/mcp.go | head -20

Repository: maximhq/bifrost

Length of output: 41

---

🏁 Script executed:

rg -n "HandleCreateMCPClient|HandleUpdateMCPClient|POST.*mcp|PUT.*mcp" --type go transports/bifrost-http/handlers/mcp.go | head -20

Repository: maximhq/bifrost

Length of output: 830

---

🏁 Script executed:

rg -B 5 "if err := validateAllowedExtraHeaders" --type go transports/bifrost-http/handlers/mcp.go | head -50

Repository: maximhq/bifrost

Length of output: 569

---

🏁 Script executed:

rg -n "^func.*addMCPClient|^func.*updateMCPClient" --type go transports/bifrost-http/handlers/mcp.go

Repository: maximhq/bifrost

Length of output: 193

---

🏁 Script executed:

sed -n '282,330p' transports/bifrost-http/handlers/mcp.go

Repository: maximhq/bifrost

Length of output: 2064

---

Add allowed_extra_headers field to CreateMCPClientRequest to match backend behavior.

The allowed_extra_headers field is present in UpdateMCPClientRequest (line 94) but missing from CreateMCPClientRequest (lines 52-64). The Go backend handler for creating MCP clients (addMCPClient) validates this field on the request, indicating it should be supported during client creation. For consistency with the backend API and the MCPClientConfig interface, this field should be added to CreateMCPClientRequest.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ui/lib/types/mcp.ts` around lines 94 - 95, CreateMCPClientRequest is missing
the allowed_extra_headers field present on UpdateMCPClientRequest and expected
by the backend handler addMCPClient and MCPClientConfig; add an optional
allowed_extra_headers?: string[] property to the CreateMCPClientRequest type so
create requests accept the same allowlist semantics (["*"] to allow all) and
match the backend validation behavior.

[coderabbitai]

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 (1)

core/mcp/codemode/starlark/getdocs.go (1)

143-174: ⚠️ Potential issue | 🟡 Minor

Update the generated Starlark guidance for top-level control flow.

This header still says top-level for/if/while must be wrapped in a function, but executeCode now enables syntax.FileOptions{TopLevelControl: true, While: true} and the new dialect tests rely on that. Please rewrite this block here and the matching "not within a function" hint path so the docs match the runtime.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/mcp/codemode/starlark/getdocs.go` around lines 143 - 174, The generated
header incorrectly instructs that top-level for/if/while must be wrapped in a
function; update the text in getdocs.go where the string builder (sb) writes the
STARLARK DIFFERENCE FROM PYTHON block and the matching "not within a function"
hint to reflect that executeCode now supports top-level control flow via
syntax.FileOptions{TopLevelControl: true, While: true}; replace the guidance to
state top-level control flow is supported (with any dialect caveats) and adjust
examples/usage to show top-level loops/ifs/while are allowed directly, ensuring
the change is applied in the same area that uses isToolLevel, clientName and
tools to build the header so documentation matches runtime behavior.

🧹 Nitpick comments (3)

transports/changelog.md (1)

1-1: Consider splitting long entry for better readability.

This entry covers three distinct aspects (wildcard support, empty array semantics, and handler optimization). Splitting into separate bullets would improve scanability.

♻️ Optional refactor for readability

-- feat: VK provider config key_ids now supports ["*"] wildcard to allow all keys; empty key_ids denies all; handler resolves wildcard to AllowAllKeys flag without DB key lookups
+- feat: VK provider config key_ids now supports ["*"] wildcard to allow all keys
+- feat: empty key_ids in VK provider config now denies all keys by default
+- refactor: handler resolves ["*"] wildcard to AllowAllKeys flag to skip DB key lookups

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@transports/changelog.md` at line 1, The changelog entry is too dense—split
the single line into three separate bullets to improve readability: one bullet
stating that the VK provider config key_ids now accepts the ["*"] wildcard, a
second bullet explaining that an empty key_ids array now denies all keys, and a
third bullet describing that the handler resolves the wildcard to an
AllowAllKeys flag to avoid DB key lookups; reference "key_ids", '["*"]',
"AllowAllKeys", and "handler" in each appropriate bullet so the intent and
impacted components are clear.

core/mcp/agent.go (1)

366-478: Consider using bifrost.Ptr() for pointer creation.

The &sum pattern works correctly, but per repository conventions, bifrost.Ptr() is preferred for creating pointers.

♻️ Optional refactor using bifrost.Ptr()

-			sum := bct + act
-			merged.CompletionTokensDetails.CitationTokens = &sum
+			merged.CompletionTokensDetails.CitationTokens = bifrost.Ptr(bct + act)

Apply similarly to NumSearchQueries and ImageTokens pointer assignments.

Based on learnings: "In the maximhq/bifrost repository, prefer using bifrost.Ptr() to create pointers instead of the address operator (&) even when & would be valid syntactically."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/mcp/agent.go` around lines 366 - 478, In mergeUsage, replace the local
idiom of creating pointer values via &sum for
CompletionTokensDetails.CitationTokens, CompletionTokensDetails.NumSearchQueries
and CompletionTokensDetails.ImageTokens with the repository-conventional
bifrost.Ptr() helper: compute the integer sum as you already do, then assign
merged.CompletionTokensDetails.CitationTokens = bifrost.Ptr(sum) (and similarly
for NumSearchQueries and ImageTokens) so pointers are created via bifrost.Ptr()
rather than the address-of operator; locate these assignments inside the
mergeUsage function where bd/ad nullable fields are aggregated.

core/mcp/codemode/starlark/starlark_test.go (1)

599-616: Share the dialect options with production code.

starlarkOpts() duplicates the syntax.FileOptions literal from executeCode, so a future option change can leave the tests exercising a different dialect than production. Extract the builder into package code and reuse it here.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/mcp/codemode/starlark/starlark_test.go` around lines 599 - 616,
starlarkOpts() in the test duplicates the syntax.FileOptions literal used by
executeCode; extract a shared builder (e.g., NewStarlarkFileOptions or
BuildFileOptions) into the package that returns *syntax.FileOptions configured
for production, replace the literal in executeCode to call that builder, and
update the test to call that new exported function (remove starlarkOpts and have
execStarlark use the shared builder) so tests and production share the same
dialect configuration.

🤖 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/mcp/agent.go`:
- Around line 332-338: The early-return when depth == 1 &&
len(allExecutedToolResults) == 0 returns currentResponse without applying
accumulatedUsage; call adapter.applyUsage(currentResponse, accumulatedUsage)
before that return so usage is always applied (mirroring the later path that
applies usage then calls adapter.createResponseWithExecutedTools), keeping the
rest of the logic and returned value (currentResponse) otherwise unchanged.

In `@core/mcp/codemode/starlark/utils.go`:
- Around line 366-398: The helpers getCanonicalToolName,
getCompatibilityToolAlias and matchesToolReference currently rely on
stripClientPrefix which only strips the "<client>-" prefix, but discovered MCP
tool names are sanitized to use "_" (e.g., "github_SEARCH_REPOS" →
"github_search_repos"); update stripClientPrefix (or callers) to also remove the
sanitized "<client>_" prefix (case-insensitively) before further
canonicalization so getCanonicalToolName and getCompatibilityToolAlias yield
"search_repos" and "search_repos" variants; update matchesToolReference to
compare against those normalized forms and add a regression test adjacent to the
canonicalization tests covering the "client_tool" sanitized prefix case (and
ensure callMCPTool usage of stripClientPrefix benefits from the same change).

In `@transports/changelog.md`:
- Line 5: Update the changelog entry text by replacing the unhyphenated phrase
"request level extra headers" with the hyphenated compound adjective
"request-level extra headers" so the line reads "feat: add support for
request-level extra headers in MCP tool execution."; ensure only that wording is
changed (preserve the rest of the entry).

---

Outside diff comments:
In `@core/mcp/codemode/starlark/getdocs.go`:
- Around line 143-174: The generated header incorrectly instructs that top-level
for/if/while must be wrapped in a function; update the text in getdocs.go where
the string builder (sb) writes the STARLARK DIFFERENCE FROM PYTHON block and the
matching "not within a function" hint to reflect that executeCode now supports
top-level control flow via syntax.FileOptions{TopLevelControl: true, While:
true}; replace the guidance to state top-level control flow is supported (with
any dialect caveats) and adjust examples/usage to show top-level loops/ifs/while
are allowed directly, ensuring the change is applied in the same area that uses
isToolLevel, clientName and tools to build the header so documentation matches
runtime behavior.

---

Nitpick comments:
In `@core/mcp/agent.go`:
- Around line 366-478: In mergeUsage, replace the local idiom of creating
pointer values via &sum for CompletionTokensDetails.CitationTokens,
CompletionTokensDetails.NumSearchQueries and CompletionTokensDetails.ImageTokens
with the repository-conventional bifrost.Ptr() helper: compute the integer sum
as you already do, then assign merged.CompletionTokensDetails.CitationTokens =
bifrost.Ptr(sum) (and similarly for NumSearchQueries and ImageTokens) so
pointers are created via bifrost.Ptr() rather than the address-of operator;
locate these assignments inside the mergeUsage function where bd/ad nullable
fields are aggregated.

In `@core/mcp/codemode/starlark/starlark_test.go`:
- Around line 599-616: starlarkOpts() in the test duplicates the
syntax.FileOptions literal used by executeCode; extract a shared builder (e.g.,
NewStarlarkFileOptions or BuildFileOptions) into the package that returns
*syntax.FileOptions configured for production, replace the literal in
executeCode to call that builder, and update the test to call that new exported
function (remove starlarkOpts and have execStarlark use the shared builder) so
tests and production share the same dialect configuration.

In `@transports/changelog.md`:
- Line 1: The changelog entry is too dense—split the single line into three
separate bullets to improve readability: one bullet stating that the VK provider
config key_ids now accepts the ["*"] wildcard, a second bullet explaining that
an empty key_ids array now denies all keys, and a third bullet describing that
the handler resolves the wildcard to an AllowAllKeys flag to avoid DB key
lookups; reference "key_ids", '["*"]', "AllowAllKeys", and "handler" in each
appropriate bullet so the intent and impacted components are clear.

🪄 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: a19ea721-6c58-4997-94b4-bad1c0daec22

📥 Commits

Reviewing files that changed from the base of the PR and between 11f1f5f and dce2cbb.

📒 Files selected for processing (10)

• core/changelog.md
• core/mcp/agent.go
• core/mcp/agentadaptors.go
• core/mcp/codemode/starlark/executecode.go
• core/mcp/codemode/starlark/getdocs.go
• core/mcp/codemode/starlark/listfiles.go
• core/mcp/codemode/starlark/readfile.go
• core/mcp/codemode/starlark/starlark_test.go
• core/mcp/codemode/starlark/utils.go
• transports/changelog.md

✅ Files skipped from review due to trivial changes (1)

• core/changelog.md

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Missing usage accumulation on early return path (line 333).

When depth == 1 && len(allExecutedToolResults) == 0, the function returns currentResponse without applying accumulatedUsage. This path returns the initial response unchanged, but if usage should be consistent, consider whether this is intentional.

💡 Potential fix if usage should be applied on all paths

 		// Return as is if its the first iteration
 		if depth == 1 && len(allExecutedToolResults) == 0 {
+			adapter.applyUsage(currentResponse, accumulatedUsage)
 			return currentResponse, nil
 		}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if depth == 1 && len(allExecutedToolResults) == 0 {
	return currentResponse, nil
	}
	// Apply accumulated usage before building the final response
	adapter.applyUsage(currentResponse, accumulatedUsage)
	// Create response with all executed tool results from all iterations, and non-auto-executable tool calls
	return adapter.createResponseWithExecutedTools(currentResponse, allExecutedToolResults, allExecutedToolCalls, nonAutoExecutableTools), nil
	if depth == 1 && len(allExecutedToolResults) == 0 {
	adapter.applyUsage(currentResponse, accumulatedUsage)
	return currentResponse, nil
	}
	// Apply accumulated usage before building the final response
	adapter.applyUsage(currentResponse, accumulatedUsage)
	// Create response with all executed tool results from all iterations, and non-auto-executable tool calls
	return adapter.createResponseWithExecutedTools(currentResponse, allExecutedToolResults, allExecutedToolCalls, nonAutoExecutableTools), nil

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/mcp/agent.go` around lines 332 - 338, The early-return when depth == 1
&& len(allExecutedToolResults) == 0 returns currentResponse without applying
accumulatedUsage; call adapter.applyUsage(currentResponse, accumulatedUsage)
before that return so usage is always applied (mirroring the later path that
applies usage then calls adapter.createResponseWithExecutedTools), keeping the
rest of the logic and returned value (currentResponse) otherwise unchanged.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Strip sanitized client prefixes before canonicalizing tool names.

These helpers only remove <client>-. In this repo the discovered MCP tool names are sanitized to _, so prefixed names like github_SEARCH_REPOS keep the server prefix and then surface as github_search_repos in list/read/docs/bindings instead of search_repos. The same assumption also leaks into callMCPTool, which reuses stripClientPrefix. Please normalize against the sanitized prefix too, and add a regression case for the client_tool form next to the new canonicalization tests.

Possible fix

 func stripClientPrefix(prefixedToolName, clientName string) string {
-	prefix := clientName + "-"
-	if strings.HasPrefix(prefixedToolName, prefix) {
-		return strings.TrimPrefix(prefixedToolName, prefix)
-	}
+	prefixes := []string{
+		clientName + "-",
+		clientName + "_",
+		parseToolName(clientName) + "_",
+	}
+	for _, prefix := range prefixes {
+		if prefix != "" && strings.HasPrefix(prefixedToolName, prefix) {
+			return strings.TrimPrefix(prefixedToolName, prefix)
+		}
+	}
 	// If prefix doesn't match, return as-is (shouldn't happen, but be safe)
 	return prefixedToolName
 }

Based on learnings: In MCP tool system under core/mcp/, tool names are sanitized by replacing '-' with '_' during discovery and this sanitized form is used throughout the system, including CallToolParams.Name and ExtraFields.ToolName.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/mcp/codemode/starlark/utils.go` around lines 366 - 398, The helpers
getCanonicalToolName, getCompatibilityToolAlias and matchesToolReference
currently rely on stripClientPrefix which only strips the "<client>-" prefix,
but discovered MCP tool names are sanitized to use "_" (e.g.,
"github_SEARCH_REPOS" → "github_search_repos"); update stripClientPrefix (or
callers) to also remove the sanitized "<client>_" prefix (case-insensitively)
before further canonicalization so getCanonicalToolName and
getCompatibilityToolAlias yield "search_repos" and "search_repos" variants;
update matchesToolReference to compare against those normalized forms and add a
regression test adjacent to the canonicalization tests covering the
"client_tool" sanitized prefix case (and ensure callMCPTool usage of
stripClientPrefix benefits from the same change).

[coderabbitai]

Actionable comments posted: 5

♻️ Duplicate comments (2)

core/schemas/tracer.go (1)

41-42: ⚠️ Potential issue | 🟠 Major

Keep schemas.Tracer backward-compatible.

Tracer is exported, so these three changes are all source-breaking for external tracer implementations and mocks: CreateTrace(parentID, requestID ...string), PopulateLLMResponseAttributes(*BifrostContext, ...), and the new required AttachPluginLogs(...) method. Please keep the existing interface stable and move request-aware/plugin-log behavior into opt-in extension interfaces or adapters.

Also applies to: 72-72, 115-117

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/schemas/tracer.go` around lines 41 - 42, The Tracer interface change is
breaking; revert the exported interface schemas.Tracer to its original shape by
restoring the previous CreateTrace(parentID string, requestID ...string)
signature, removing any new required methods (e.g. AttachPluginLogs) and
restoring PopulateLLMResponseAttributes to its prior optional form; instead
implement new functionality as separate opt-in interfaces or adapters (e.g.
define a RequestAwareTracer with PopulateLLMResponseAttributes and an
AttachPluginLogsTracer with AttachPluginLogs) so existing external
implementations and mocks remain source-compatible while callers can type-assert
to the new extension interfaces when request-aware or plugin-log behavior is
needed.

plugins/logging/main.go (1)

803-804: ⚠️ Potential issue | 🟠 Major

Use the resolved provider for pricing scope lookup.

entry.Provider is still request-derived here and can be stale after routing or fallback. That can apply provider-scoped overrides against the wrong provider and log the wrong cost. Use the final provider from result.GetExtraFields().Provider (or error extra fields when only an error is available) when building pricingScopes.

💡 Suggested fix

-		pricingScopes := modelcatalog.PricingLookupScopesFromContext(ctx, string(entry.Provider))
+		providerForPricing := entry.Provider
+		if result != nil && result.GetExtraFields().Provider != "" {
+			providerForPricing = string(result.GetExtraFields().Provider)
+		} else if bifrostErr != nil && bifrostErr.ExtraFields.Provider != "" {
+			providerForPricing = string(bifrostErr.ExtraFields.Provider)
+		}
+		pricingScopes := modelcatalog.PricingLookupScopesFromContext(ctx, providerForPricing)
 		if cost := p.pricingManager.CalculateCost(result, pricingScopes); cost > 0 {
 			entry.Cost = &cost
 		}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@plugins/logging/main.go` around lines 803 - 804, The code currently builds
pricingScopes using the request-derived entry.Provider which may be stale;
update the pricing scope lookup to use the resolved provider from the final
result (use result.GetExtraFields().Provider, or if result represents an error
use result.GetErrorExtraFields().Provider or equivalent) when calling
modelcatalog.PricingLookupScopesFromContext so
p.pricingManager.CalculateCost(result, pricingScopes) applies provider-scoped
overrides and logs cost for the actual provider that served the request.

🧹 Nitpick comments (1)

core/schemas/trace.go (1)

71-74: Consider dropping oversized PluginLogs buffers before pooling.

Reset() truncates the slice but keeps its backing capacity. Because Trace instances are pooled in framework/tracing/store.go, one noisy request can pin a large PluginLogs buffer in the pool indefinitely. Nilling the slice — or only doing so above a capacity threshold — would avoid that retention.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/schemas/trace.go` around lines 71 - 74, The Reset() method on Trace
currently zeroes elements and truncates PluginLogs but leaves its backing array,
which can retain large buffers in the Trace pool; update Trace.Reset() to drop
oversized PluginLogs buffers before pooling by setting t.PluginLogs = nil (or
conditionally nil-ing when cap(t.PluginLogs) > <threshold>) instead of only
slicing to [:0], ensuring PluginLogs is released to the GC and preventing a
single noisy request from pinning large memory in 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/schemas/context.go`:
- Around line 401-417: WithPluginScope currently assigns the root's done channel
into the scoped BifrostContext so the scoped context keeps its own doneOnce and
err while sharing the root done; this allows a plugin to call pluginCtx.Cancel()
and close the root done via the scoped doneOnce, corrupting root cancellation
and Err() state. Change WithPluginScope (and the similar block around the
423-432 region) so the scoped context does not copy the root done/doneOnce/err
but instead sets scoped.valueDelegate = bc and leaves scoped.done
nil/uninitialized; ensure Cancel(), Done(), and Err() implementations on
BifrostContext delegate cancellation/Done/Err calls to valueDelegate when
valueDelegate is non-nil so scoped contexts forward cancellation to the root
without owning the shared done state.

In `@framework/logstore/tables.go`:
- Around line 130-131: The batch-size accounting misses the new PluginLogs field
so estimateLogEntrySize() undercounts entries and can exceed maxBatchBytes;
update estimateLogEntrySize() in plugins/logging/writer.go to include the size
of the PluginLogs string (same way RoutingEngineLogs is counted) when computing
per-entry bytes, ensuring PluginLogs contributes to the batch byte total and
prevents batches from overshooting the 100 MB ceiling.

In `@plugins/logging/main.go`:
- Around line 809-813: The code always allocates entry.SelectedKey even when
none was resolved, causing empty-object serialization; change the logic in the
"Pre-apply denormalized fields" section so that you only set entry.SelectedKey =
&schemas.Key{...} when a key was actually resolved (e.g., when
entry.SelectedKeyID and/or entry.SelectedKeyName is non-empty or otherwise
indicates a resolved key). If neither identifier is present, leave
entry.SelectedKey as nil so the payload omits selected_key.

In `@transports/bifrost-http/handlers/middlewares.go`:
- Around line 341-350: The current code incorrectly reuses HTTPTransportPostHook
as an end-of-stream hook for streaming responses; instead add a separate
best-effort streaming completer and context key so post-hooks and stream-end
hooks remain distinct. Create a new context key (e.g.
BifrostContextKeyTransportStreamEndHookCompleter) and a new runner function
(e.g. runTransportStreamEndHooks) that invokes a new plugin method or calls
plugins in a best-effort way that ignores/does not short-circuit on errors; when
deferred streaming is detected (the check using
schemas.BifrostContextKeyDeferTraceCompletion), set that new completer into
context rather than reusing BifrostContextKeyTransportPostHookCompleter, and
leave runTransportPostHooks(ctx, plugins, bifrostCtx) behavior unchanged for
non-streaming responses so HTTPTransportPostHook continues to run immediately
and retain its error-short-circuit semantics.

In `@transports/bifrost-http/server/server.go`:
- Around line 515-537: The two concurrent calls to s.Client.ListModelsRequest
are sharing the mutable bfCtx which can cause request-scoped state to be
overwritten; create a fresh BifrostContext for each goroutine (e.g., clone or
construct a new context from the existing bfCtx) and pass that per-goroutine
context into ListModelsRequest (for both the filtered and the Unfiltered=true
calls) so each request has its own independent context.

---

Duplicate comments:
In `@core/schemas/tracer.go`:
- Around line 41-42: The Tracer interface change is breaking; revert the
exported interface schemas.Tracer to its original shape by restoring the
previous CreateTrace(parentID string, requestID ...string) signature, removing
any new required methods (e.g. AttachPluginLogs) and restoring
PopulateLLMResponseAttributes to its prior optional form; instead implement new
functionality as separate opt-in interfaces or adapters (e.g. define a
RequestAwareTracer with PopulateLLMResponseAttributes and an
AttachPluginLogsTracer with AttachPluginLogs) so existing external
implementations and mocks remain source-compatible while callers can type-assert
to the new extension interfaces when request-aware or plugin-log behavior is
needed.

In `@plugins/logging/main.go`:
- Around line 803-804: The code currently builds pricingScopes using the
request-derived entry.Provider which may be stale; update the pricing scope
lookup to use the resolved provider from the final result (use
result.GetExtraFields().Provider, or if result represents an error use
result.GetErrorExtraFields().Provider or equivalent) when calling
modelcatalog.PricingLookupScopesFromContext so
p.pricingManager.CalculateCost(result, pricingScopes) applies provider-scoped
overrides and logs cost for the actual provider that served the request.

---

Nitpick comments:
In `@core/schemas/trace.go`:
- Around line 71-74: The Reset() method on Trace currently zeroes elements and
truncates PluginLogs but leaves its backing array, which can retain large
buffers in the Trace pool; update Trace.Reset() to drop oversized PluginLogs
buffers before pooling by setting t.PluginLogs = nil (or conditionally nil-ing
when cap(t.PluginLogs) > <threshold>) instead of only slicing to [:0], ensuring
PluginLogs is released to the GC and preventing a single noisy request from
pinning large memory in the pool.

🪄 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: f0e50c41-5ffe-4409-a9ce-c499db1b8860

📥 Commits

Reviewing files that changed from the base of the PR and between dce2cbb and 0e06156.

📒 Files selected for processing (22)

• core/bifrost.go
• core/schemas/bifrost.go
• core/schemas/context.go
• core/schemas/context_test.go
• core/schemas/trace.go
• core/schemas/tracer.go
• examples/plugins/hello-world/main.go
• framework/logstore/migrations.go
• framework/logstore/rdb.go
• framework/logstore/store.go
• framework/logstore/tables.go
• framework/tracing/store.go
• framework/tracing/tracer.go
• plugins/logging/main.go
• plugins/logging/operations_test.go
• plugins/logging/writer.go
• transports/bifrost-http/handlers/middlewares.go
• transports/bifrost-http/lib/config.go
• transports/bifrost-http/server/server.go
• ui/app/workspace/logs/sheets/logDetailsSheet.tsx
• ui/app/workspace/logs/views/pluginLogsView.tsx
• ui/lib/types/logs.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• core/schemas/bifrost.go

👮 Files not reviewed due to content moderation or server errors (1)

• core/bifrost.go

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Scoped plugin contexts can corrupt the root cancellation state.

WithPluginScope reuses the root done channel, but the scoped wrapper keeps its own doneOnce and err. If a plugin calls pluginCtx.Cancel(), the scope closes the root channel through its own doneOnce; later the root can panic when it tries to cancel, and its Err() state is never updated correctly. Scoped contexts need to delegate cancellation to valueDelegate instead of owning shared done.

Also applies to: 423-432

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/schemas/context.go` around lines 401 - 417, WithPluginScope currently
assigns the root's done channel into the scoped BifrostContext so the scoped
context keeps its own doneOnce and err while sharing the root done; this allows
a plugin to call pluginCtx.Cancel() and close the root done via the scoped
doneOnce, corrupting root cancellation and Err() state. Change WithPluginScope
(and the similar block around the 423-432 region) so the scoped context does not
copy the root done/doneOnce/err but instead sets scoped.valueDelegate = bc and
leaves scoped.done nil/uninitialized; ensure Cancel(), Done(), and Err()
implementations on BifrostContext delegate cancellation/Done/Err calls to
valueDelegate when valueDelegate is non-nil so scoped contexts forward
cancellation to the root without owning the shared done state.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Update batch-size accounting for PluginLogs.

plugins/logging/writer.go:estimateLogEntrySize() is what enforces the 100 MB maxBatchBytes ceiling, and it currently counts RoutingEngineLogs but not this new PluginLogs text field. Once plugin logs are populated, batches can overshoot the intended cap by a lot, which defeats the safeguard against memory and latency spikes.

💡 Cross-file fix

diff --git a/plugins/logging/writer.go b/plugins/logging/writer.go
@@
 		len(log.PassthroughRequestBody) +
 		len(log.PassthroughResponseBody) +
 		len(log.ContentSummary) +
 		len(log.CacheDebug) +
-		len(log.RoutingEngineLogs)
+		len(log.RoutingEngineLogs) +
+		len(log.PluginLogs)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@framework/logstore/tables.go` around lines 130 - 131, The batch-size
accounting misses the new PluginLogs field so estimateLogEntrySize() undercounts
entries and can exceed maxBatchBytes; update estimateLogEntrySize() in
plugins/logging/writer.go to include the size of the PluginLogs string (same way
RoutingEngineLogs is counted) when computing per-entry bytes, ensuring
PluginLogs contributes to the batch byte total and prevents batches from
overshooting the 100 MB ceiling.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Only populate SelectedKey when one was actually resolved.

This pointer is now always non-nil, so payloads that previously omitted selected_key will start serializing {"id":"","name":""}. That changes the websocket payload shape and can make the UI treat “no key selected” as a real selection.

💡 Suggested fix

-	entry.SelectedKey = &schemas.Key{
-		ID:   entry.SelectedKeyID,
-		Name: entry.SelectedKeyName,
-	}
+	if entry.SelectedKeyID != "" || entry.SelectedKeyName != "" {
+		entry.SelectedKey = &schemas.Key{
+			ID:   entry.SelectedKeyID,
+			Name: entry.SelectedKeyName,
+		}
+	}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	// Pre-apply denormalized fields for WebSocket callback enrichment
	entry.SelectedKey = &schemas.Key{
	ID: entry.SelectedKeyID,
	Name: entry.SelectedKeyName,
	}
	// Pre-apply denormalized fields for WebSocket callback enrichment
	if entry.SelectedKeyID != "" || entry.SelectedKeyName != "" {
	entry.SelectedKey = &schemas.Key{
	ID: entry.SelectedKeyID,
	Name: entry.SelectedKeyName,
	}
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@plugins/logging/main.go` around lines 809 - 813, The code always allocates
entry.SelectedKey even when none was resolved, causing empty-object
serialization; change the logic in the "Pre-apply denormalized fields" section
so that you only set entry.SelectedKey = &schemas.Key{...} when a key was
actually resolved (e.g., when entry.SelectedKeyID and/or entry.SelectedKeyName
is non-empty or otherwise indicates a resolved key). If neither identifier is
present, leave entry.SelectedKey as nil so the payload omits selected_key.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don’t repurpose HTTPTransportPostHook as a streaming end-of-stream hook.

The plugin contract currently says HTTPTransportPostHook is not called for streaming responses and that a returned error short-circuits the response. Deferring it until after the stream ends breaks both guarantees at once: existing transport plugins start receiving unexpected streaming post-hooks, and once bytes are already written there is no way to honor a returned error. This needs a separate best-effort end-of-stream hook, not reuse of HTTPTransportPostHook.

Also applies to: 355-393

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@transports/bifrost-http/handlers/middlewares.go` around lines 341 - 350, The
current code incorrectly reuses HTTPTransportPostHook as an end-of-stream hook
for streaming responses; instead add a separate best-effort streaming completer
and context key so post-hooks and stream-end hooks remain distinct. Create a new
context key (e.g. BifrostContextKeyTransportStreamEndHookCompleter) and a new
runner function (e.g. runTransportStreamEndHooks) that invokes a new plugin
method or calls plugins in a best-effort way that ignores/does not short-circuit
on errors; when deferred streaming is detected (the check using
schemas.BifrostContextKeyDeferTraceCompletion), set that new completer into
context rather than reusing BifrostContextKeyTransportPostHookCompleter, and
leave runTransportPostHooks(ctx, plugins, bifrostCtx) behavior unchanged for
non-streaming responses so HTTPTransportPostHook continues to run immediately
and retain its error-short-circuit semantics.