feat: log database type when connecting in NewConfigStore - #3724
Conversation
|
Warning Review limit reached
More reviews will be available in 4 minutes and 21 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 Walkthrough<review_stack_artifact> </review_stack_artifact> ✨ Finishing Touches🧪 Generate unit tests (beta)
|
07e9554 to
4446e5d
Compare
00f2e17 to
b2ca296
Compare
4446e5d to
b095986
Compare
b095986 to
33dbbd7
Compare
33dbbd7 to
3f019d6
Compare
3f019d6 to
033af18
Compare
033af18 to
230ed48
Compare
NewConfigStore
Confidence Score: 5/5This is a one-line additive change that only emits a log message; it does not alter any logic, error handling, or data flow. The change adds a single log statement before an existing switch and returns no new errors. No existing paths are modified, and no data or control flow is affected. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "chore: added connecting to db log on ser..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 13
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 (5)
docs/mcp/sessions.mdx (1)
1-195: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftAdd the required Mintlify tab structure to this page.
This page is missing the required Web UI / API / config.json tabs for docs pages. Please add those sections (API/config tabs can explicitly call out unsupported flows when applicable) to align with repo docs standards.
As per coding guidelines
docs/**/*.mdx: Mintlify MDX documentation must have Web UI / API / config.json tabs; validate config.json examples against transports/config.schema.json.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/mcp/sessions.mdx` around lines 1 - 195, The page "MCP Sessions" (docs/mcp/sessions.mdx) is missing the required Mintlify tabbed sections; add a Mintlify Tab structure containing "Web UI", "API", and "config.json" tabs beneath the Overview (or after the frontmatter) and move or duplicate relevant content into each tab as appropriate: put the UI walkthrough and Actions/Statuses under "Web UI", document endpoints/responses and any unsupported flows under "API", and include a validated example config JSON under "config.json" (explicitly note unsupported options when applicable); validate the example JSON against transports/config.schema.json before committing.framework/configstore/store.go (1)
541-552:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMove the startup log into the successful backend branches.
Line 541 logs
connecting to %s databasebefore the type assertion succeeds, so invalid configs will claim a connection attempt that never happened. Log only after the SQLite/Postgres config has been validated.Suggested adjustment
- logger.Info("connecting to %s database", config.Type) switch config.Type { case ConfigStoreTypeSQLite: if sqliteConfig, ok := config.Config.(*SQLiteConfig); ok { + logger.Info("connecting to %s database", config.Type) return newSqliteConfigStore(ctx, sqliteConfig, logger) } return nil, fmt.Errorf("invalid sqlite config: %T", config.Config) case ConfigStoreTypePostgres: if postgresConfig, ok := config.Config.(*PostgresConfig); ok { + logger.Info("connecting to %s database", config.Type) return newPostgresConfigStore(ctx, postgresConfig, logger) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/configstore/store.go` around lines 541 - 552, The startup log currently runs before validating the config and may report a connection attempt even when the type assertion fails; move the logger.Info("connecting to %s database", config.Type) call inside each successful branch after the type assertion passes (i.e., inside the block where ConfigStoreTypeSQLite and config.Config is asserted to *SQLiteConfig before calling newSqliteConfigStore, and likewise inside the ConfigStoreTypePostgres branch after asserting *PostgresConfig and before calling newPostgresConfigStore) so only valid backend initializations emit the "connecting" message.core/bifrost.go (1)
3649-3677:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
ctxagainst nil inVerifyHeadersConnection(avoids panic)
core/bifrost.goforwardsctxtocore/mcp/clientmanager.go’sMCPManager.VerifyHeadersConnection, which callscontext.WithTimeout(ctx, ...)without a nil check; passing a nilctxwill panic. Add a nil fallback before delegating. (AddMCPClientalready routes throughMCPManager.AddClient, which handlesrequestCtx == nil.)💡 Proposed fix
func (bifrost *Bifrost) AddMCPClient(ctx context.Context, config *schemas.MCPClientConfig) error { + if ctx == nil { + ctx = context.Background() + } if bifrost.MCPManager == nil { // Use sync.Once to ensure thread-safe initialization bifrost.mcpInitOnce.Do(func() { @@ return bifrost.MCPManager.AddClient(ctx, config) } @@ func (bifrost *Bifrost) VerifyHeadersConnection(ctx context.Context, config *schemas.MCPClientConfig, userHeaders map[string]string) (map[string]schemas.ChatTool, map[string]string, error) { + if ctx == nil { + ctx = context.Background() + } if bifrost.MCPManager == nil { bifrost.mcpInitOnce.Do(func() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/bifrost.go` around lines 3649 - 3677, The VerifyHeadersConnection path can panic if a nil context is forwarded; before calling MCPManager.VerifyHeadersConnection (or before delegating any ctx into mcp/clientmanager.go), ensure you replace a nil ctx with a fallback context (e.g., bifrost.ctx or context.Background()) so VerifyHeadersConnection never receives nil; update AddMCPClient (and any other call sites that forward ctx into MCPManager.VerifyHeadersConnection) to check if ctx == nil and set ctx = bifrost.ctx (or context.Background()) before calling MCPManager.VerifyHeadersConnection so context.WithTimeout inside VerifyHeadersConnection is always safe.transports/bifrost-http/lib/config.go (1)
4751-4766:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCanonicalize
PerUserHeaderKeyson live add/update too.Line 1470 normalizes these keys before hashing/persistence, but the runtime paths here still keep user input verbatim. That leaves the in-memory MCP client state able to diverge from the stored/reloaded state for case-variant header names.
♻️ Suggested fix
func (c *Config) AddMCPClient(ctx context.Context, clientConfig *schemas.MCPClientConfig) error { if c.client == nil { return fmt.Errorf("bifrost client not set") } c.muMCP.Lock() defer c.muMCP.Unlock() if c.MCPConfig == nil { c.MCPConfig = &schemas.MCPConfig{} } + clientConfig.PerUserHeaderKeys = mcputils.CanonicalizeHeaderKeys(clientConfig.PerUserHeaderKeys) // Track new environment variables c.MCPConfig.ClientConfigs = append(c.MCPConfig.ClientConfigs, clientConfig) // Config with processed env vars if err := c.client.AddMCPClient(ctx, clientConfig); err != nil { c.MCPConfig.ClientConfigs = c.MCPConfig.ClientConfigs[:len(c.MCPConfig.ClientConfigs)-1] @@ // Update the in-memory configuration with only the fields that were changed // Preserve connection info (connection_type, connection_string, stdio_config) from oldConfig // as these are read-only and not sent in the update request + normalizedPerUserHeaderKeys := mcputils.CanonicalizeHeaderKeys(updatedConfig.PerUserHeaderKeys) c.MCPConfig.ClientConfigs[configIndex].Name = updatedConfig.Name c.MCPConfig.ClientConfigs[configIndex].IsCodeModeClient = updatedConfig.IsCodeModeClient c.MCPConfig.ClientConfigs[configIndex].Headers = updatedConfig.Headers c.MCPConfig.ClientConfigs[configIndex].ToolsToExecute = updatedConfig.ToolsToExecute c.MCPConfig.ClientConfigs[configIndex].ToolsToAutoExecute = updatedConfig.ToolsToAutoExecute @@ c.MCPConfig.ClientConfigs[configIndex].ToolSyncInterval = updatedConfig.ToolSyncInterval c.MCPConfig.ClientConfigs[configIndex].AllowOnAllVirtualKeys = updatedConfig.AllowOnAllVirtualKeys c.MCPConfig.ClientConfigs[configIndex].Disabled = updatedConfig.Disabled - c.MCPConfig.ClientConfigs[configIndex].PerUserHeaderKeys = updatedConfig.PerUserHeaderKeys + c.MCPConfig.ClientConfigs[configIndex].PerUserHeaderKeys = normalizedPerUserHeaderKeysAlso applies to: 4855-4866
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config.go` around lines 4751 - 4766, Before appending/activating a new or updated MCP client, canonicalize its PerUserHeaderKeys so the in-memory clientConfig matches the persisted/hashed form: in AddMCPClient (and the analogous UpdateMCPClient path) normalize clientConfig.PerUserHeaderKeys (use the existing normalize/canonicalization helper used at persistence if available, otherwise implement consistent normalization such as trimming and lowercasing each header and removing duplicates) and assign the normalized slice back onto clientConfig before appending to c.MCPConfig.ClientConfigs and before calling c.client.AddMCPClient; this ensures case-variant header names don't diverge between runtime and stored state.ui/app/workspace/mcp-registry/views/mcpClientForm.tsx (1)
282-282:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock sheet dismissal while the headers authorizer is open.
Line 282 still allows the parent sheet to close during the new per-user-headers flow because it only guards
oauthFlow. If the user presses Escape or closes the sheet backdrop at that point,headersFlow.payloadis dropped and the creation flow is aborted.Suggested fix
- <Sheet open={open} onOpenChange={(open) => !open && !oauthFlow && onClose()}> + <Sheet open={open} onOpenChange={(open) => !open && !oauthFlow && !headersFlow && onClose()}>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/app/workspace/mcp-registry/views/mcpClientForm.tsx` at line 282, The Sheet's onOpenChange currently prevents closing only when oauthFlow is active, but must also block dismissal when headersFlow.payload exists; update the onOpenChange handler in the Sheet component so it checks both oauthFlow and headersFlow.payload (or a boolean like headersFlowOpen) and only calls onClose() when neither oauthFlow nor headersFlow.payload are set, ensuring Escape/backdrop closes are ignored while the per-user headers authorizer is open.
🟡 Minor comments (8)
core/mcp/credstore/static_headers.go-28-31 (1)
28-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake Authorization header selection deterministic across case-variant duplicates.
When both
Authorizationandauthorization(or other case variants) are present, iterating a Go map here can pick either value nondeterministically. Prefer deterministic precedence (e.g., exact"Authorization"first, then a single case-insensitive fallback, or reject duplicates explicitly).Suggested fix
func (r *staticHeadersResolver) ConnectionHeaders(_ *schemas.BifrostContext, config *schemas.MCPClientConfig) (http.Header, error) { headers := http.Header{} if config == nil { return headers, nil } + if v, ok := config.Headers["Authorization"]; ok { + headers.Set("Authorization", v.GetValue()) + return headers, nil + } // Headers are case-insensitive on the wire but case-sensitive in Go maps; // match case-insensitively (consistent with utils.StaticConfigHeaders' // Authorization exclusion) to keep the security guarantee tight. for key, value := range config.Headers { if strings.EqualFold(key, "Authorization") { headers.Set("Authorization", value.GetValue()) break } } return headers, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/mcp/credstore/static_headers.go` around lines 28 - 31, The loop over config.Headers that sets the Authorization header is nondeterministic because it ranges a map; change SetAuthorization logic in static_headers.go so it first checks for an exact key "Authorization" in config.Headers and uses that value (headers.Set("Authorization", value.GetValue())), and only if that exact key is missing scan the map for a single case-insensitive match (e.g., strings.EqualFold) and use that; alternatively, if you prefer strictness, detect multiple case-variant duplicates and return an error instead of picking one. Update the code that currently iterates over config.Headers (the block that calls headers.Set("Authorization", ...)) to implement this deterministic precedence and ensure only one value is chosen.core/mcp/agent_test.go-86-90 (1)
86-90:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve
*schemas.MCPAuthRequiredErrordetails in the mock pipeline stubs.In
core/mcp/agent_test.go, bothMockClientManager.RunWithPluginPipelineandMockAutoClientManager.RunWithPluginPipelineflatten anyop(req)failure into&schemas.BifrostError{... Message: err.Error()}, which discards the concrete*schemas.MCPAuthRequiredErrorand any auth metadata. Production code preserves this viaerrors.As(..., &authRequiredErr)and stores it onBifrostError.ExtraFields.MCPAuthRequired, so adjust these mocks to propagate typed auth-required errors (and fields) for faithful auth-required branch coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/mcp/agent_test.go` around lines 86 - 90, The mock RunWithPluginPipeline stubs (MockClientManager.RunWithPluginPipeline and MockAutoClientManager.RunWithPluginPipeline) currently convert any op(req) error into a plain BifrostError with only Error.Message, losing concrete *schemas.MCPAuthRequiredError details; update both methods to inspect the returned err using errors.As to see if it is a *schemas.MCPAuthRequiredError and, when so, populate the returned *schemas.BifrostError.ExtraFields.MCPAuthRequired with that typed error (while still setting Error.Message and IsBifrostError appropriately), otherwise fall back to the existing behavior — this mirrors production code's preservation of auth-required metadata for proper test coverage.docs/plugins/writing-go-plugin.mdx-1315-1318 (1)
1315-1318:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude
Cleanupin the symbol-check command.This grep omits
Cleanup, even though the section above calls it out as one of the only required exports. As written, the debugging command can miss the exact missing-symbol case that blocks plugin loading.📝 Suggested doc fix
-go tool nm my-plugin.so | grep -E 'Init|GetName|PreLLMHook|PreMCPHook|PreMCPConnectionHook|PostMCPHook|PostMCPConnectionHook' +go tool nm my-plugin.so | grep -E 'Init|GetName|Cleanup|PreLLMHook|PostLLMHook|PreMCPHook|PostMCPHook|PreMCPConnectionHook|PostMCPConnectionHook'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plugins/writing-go-plugin.mdx` around lines 1315 - 1318, Update the symbol-check command to include the required Cleanup export so the grep will detect it; modify the pattern used in the go tool nm pipeline (currently matching Init, GetName, PreLLMHook, PreMCPHook, PreMCPConnectionHook, PostMCPHook, PostMCPConnectionHook) to also match Cleanup (e.g., add Cleanup to the alternation alongside Init and GetName) so missing Cleanup is reported when validating plugin symbols.ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx-910-918 (1)
910-918:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a
data-testidto the new Agent Mode help link.The new interactive link at Line 910 should expose a stable selector for E2E coverage, same as the Code Mode help link.
Suggested fix
<a href="https://docs.getbifrost.ai/mcp/agent-mode" target="_blank" rel="noopener noreferrer" + data-testid="mcpclient-auto-execute-help-link" aria-label="Learn more about Auto-execute and Agent Mode"As per coding guidelines: "Add data-testid to all new interactive elements in React components for E2E test compatibility".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx` around lines 910 - 918, The Agent Mode help link in mcpClientSheet.tsx (the <a> element rendering the Info icon with className "text-muted-foreground ...") lacks a stable selector for E2E tests; add a data-testid attribute (matching the pattern used by the Code Mode help link) to that anchor (e.g., data-testid="mcp-agent-mode-help-link" or the existing Code Mode test id) so the interactive element can be targeted reliably in tests.ui/app/workspace/mcp-sessions/auth/page.tsx-297-306 (1)
297-306:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't trust
submitted=truefrom the URL as success state.This skips the fetch and shows the “Headers saved” card based solely on an editable query param, so a hand-modified URL can claim the credential was stored even when the flow is still pending or invalid. Keep this as local mutation state (or another trusted navigation state) and only show success after the submit call actually returns.
Suggested fix
- const [submitted, setSubmitted] = useQueryState("submitted"); + const [submitted, setSubmitted] = useState(false); // Skip the GET once the submit has completed — the backend deletes the // flow row on success, so any refetch returns 404 and we'd render the // "expired" view on top of a freshly successful submission. const { data: detail, isLoading, isError, error, - } = useGetMCPPerUserHeadersFlowQuery(flowId, { skip: submitted === "true" }); + } = useGetMCPPerUserHeadersFlowQuery(flowId, { skip: submitted }); const [submit, { isLoading: submitting }] = useSubmitMCPPerUserHeadersFlowMutation(); - if (submitted === "true") { + if (submitted) { return ( <CenteredCard> <div className="mb-5 flex size-12 items-center justify-center rounded-full bg-emerald-500/10"> <CheckCircle2 className="size-6 text-emerald-600" /> @@ const handleSubmit = async (values: Record<string, string>) => { try { await submit({ flowId, body: { headers: values } }).unwrap(); - void setSubmitted("true"); + setSubmitted(true); } catch (err) { toast({ title: "Submission failed",Also applies to: 310-325
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/app/workspace/mcp-sessions/auth/page.tsx` around lines 297 - 306, The page trusts the editable "submitted" query param (useQueryState("submitted")) to skip the fetch and render success; instead, stop relying on the URL for authoritative success and keep submit state locally: replace the query-param-backed submitted/setSubmitted with a local React state (e.g., useState in the same component) and only set that state to "submitted" after the actual submit call resolves successfully; remove the skip: submitted === "true" condition passed to useGetMCPPerUserHeadersFlowQuery (use flowId only) so the GET always runs unless the local success state is set, and update any other uses (lines ~310-325) that read submitted to read the local state so a hand-modified URL cannot fake success.core/mcp/utils/utils.go-35-39 (1)
35-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn
""for a nil context here.This helper documents an empty-string fallback when the callback base URL is unavailable, but
ctx.Value(...)will panic if a caller passesnil. Makingnilbehave like “unavailable” keeps the contract intact and matches the nil-safe pattern already used byExtractFilteredExtras.Suggested fix
func BuildMCPCallbackBaseURL(ctx *schemas.BifrostContext) string { + if ctx == nil { + return "" + } if base, ok := ctx.Value(schemas.BifrostContextKeyMCPCallbackBaseURL).(string); ok && base != "" { return strings.TrimRight(base, "/") } return "" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/mcp/utils/utils.go` around lines 35 - 39, The BuildMCPCallbackBaseURL function should return "" when passed a nil context to avoid a panic; update BuildMCPCallbackBaseURL to check if ctx == nil and immediately return "" before calling ctx.Value(schemas.BifrostContextKeyMCPCallbackBaseURL), preserving the existing behavior of trimming trailing slashes and returning "" for missing/empty values (matching the nil-safe pattern used by ExtractFilteredExtras).ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx-62-70 (1)
62-70:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOnly skip focus restore for the Edit handoff.
This
onCloseAutoFocusruns for every close path, so Escape/outside-click/Reconnect/Delete no longer restore focus to the trigger. That leaves keyboard users without a stable focus target after the menu closes. GatepreventDefault()behind an “opening sheet” flag and only set it from the Edit action.Suggested change
function MCPClientActionsMenu({ client, hasUpdateAccess, hasDeleteAccess, isReconnecting, isPerUserAuth, onEdit, onReconnect, onDelete, }: { @@ }) { const [isOpen, setIsOpen] = useState(false); + const [skipRestoreFocus, setSkipRestoreFocus] = useState(false); return ( <DropdownMenu open={isOpen} onOpenChange={setIsOpen}> @@ <DropdownMenuContent align="end" onCloseAutoFocus={(e) => { - // Edit opens a Sheet; letting the dropdown restore focus to its - // trigger fights the Sheet's autofocus and leaves focus outside - // the dialog — which breaks ESC-to-close. Hand focus off to the - // Sheet by skipping the dropdown's auto-restore. - e.preventDefault(); + if (skipRestoreFocus) { + e.preventDefault(); + setSkipRestoreFocus(false); + } }} > @@ data-testid={`mcp-client-edit-${client.config.client_id}-menu-item`} onSelect={(e) => { e.preventDefault(); + setSkipRestoreFocus(true); onEdit(client); setIsOpen(false); }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx` around lines 62 - 70, The onCloseAutoFocus handler on DropdownMenuContent is preventing focus restore for every close path; add a boolean flag (e.g., isOpeningSheet) in the component state and set it only when the Edit action is invoked (for example, in your edit handler like handleEditClick or the Edit menu item's onSelect), pass that flag into DropdownMenuContent and change onCloseAutoFocus to call e.preventDefault() only when isOpeningSheet is true, then reset isOpeningSheet (false) when the Sheet finishes opening or when the Edit flow completes/aborts so normal close paths (Escape/outside-click/Delete/Reconnect) restore focus as before.docs/mcp/auth/per-user-headers.mdx-144-145 (1)
144-145:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign schema-edit behavior docs with current implementation.
The docs currently say removed keys also force
needs_update, but current handler logic only forces re-submission when new required keys are introduced.Suggested wording update
-If the admin later changes `per_user_header_keys` (adds, removes, or renames a required header), all existing credentials for that MCP server flip to `needs_update`. +If the admin later changes `per_user_header_keys`, credentials flip to `needs_update` when the new schema introduces additional required keys (including rename scenarios that add a new key).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/mcp/auth/per-user-headers.mdx` around lines 144 - 145, The docs text incorrectly states that removed keys in per_user_header_keys force existing credentials to flip to needs_update; update the wording to match the handler logic by stating that only when new required keys are introduced are credentials marked needs_update and end-users receive an mcp_auth_required payload and are sent to the submission form (existing values are preserved for unchanged keys and removed keys do not trigger re-submission). Ensure you reference the behavior around per_user_header_keys, needs_update, and mcp_auth_required so readers see the exact implemented flow.
🧹 Nitpick comments (2)
core/mcp/credstore/server_oauth.go (1)
1-1: ⚡ Quick winRename this file to match repository Go filename convention.
server_oauth.gouses an underscore; this repository rule requires concatenated lowercase names for non-test Go files.As per coding guidelines "No underscores in Go filenames except for _test.go suffix; concatenate words in lowercase for multi-word filenames".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/mcp/credstore/server_oauth.go` at line 1, Rename the file server_oauth.go to follow the repository Go filename convention (concatenate words, lowercase) — e.g., serveroauth.go; ensure the package declaration (package credstore) and all symbols (types/functions in this file) remain unchanged, update any external tooling or build scripts that explicitly reference server_oauth.go, and run go build/test to verify no breakages after the rename.ui/app/workspace/mcp-registry/views/mcpClientForm.tsx (1)
196-205: ⚡ Quick winSurface the required per-user header key error inline.
This new field currently fails via a destructive toast only. Please render the validation message next to the textarea so users can correct it in place, like the rest of this form.
As per coding guidelines, "Prefer inline field errors in forms rather than toast notifications".
Also applies to: 512-539
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/app/workspace/mcp-registry/views/mcpClientForm.tsx` around lines 196 - 205, The per-user header validation currently only triggers a destructive toast inside the authType === "per_user_headers" branch (see perUserHeaderKeys, toast and hasErrors) — change this to set and render an inline field error for the textarea that collects perUserHeaderKeys (match the pattern used elsewhere in this form for inline errors), e.g., add a validation state/variable (e.g., perUserHeaderKeysError) when perUserHeaderKeys.length === 0 instead of calling toast, propagate it into the textarea component's error/help props so the message "Declare at least one header name users must supply." appears next to the textarea, and remove the toast call; apply the same inline-error approach to the other occurrence referenced (lines ~512-539) so both places show field-level errors rather than toasts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/mcp/codemode/starlark/executecode.go`:
- Around line 471-493: The AcquireClientConn call and credStore.RequestHeaders
call must be moved into the plugin gate op closure so PreMCPHook short-circuits
don't incur connection/auth costs: remove the top-level conn, release, err :=
s.clientManager.AcquireClientConn(...) and reqHeaders, err :=
s.credStore.RequestHeaders(...) calls and instead invoke
s.clientManager.AcquireClientConn(nestedCtx, client) at the start of the
func(preReq *schemas.BifrostMCPRequest) closure, check and return errors from
that call, defer the returned release() inside the closure, then call
reqHeaders, err := s.credStore.RequestHeaders(nestedCtx, client.ExecutionConfig)
inside the same closure and return any error; leave RunWithPluginPipeline,
getToolExecutionTimeout, and the wire CallTool logic unchanged otherwise.
In `@core/mcp/credstore/server_oauth.go`:
- Around line 17-20: The method serverOAuthResolver.ConnectionHeaders
dereferences its config parameter without checking for nil, which can panic; add
a nil guard at the start of ConnectionHeaders to return
schemas.ErrOAuth2ConfigNotFound (or an appropriate error) if config == nil
before accessing config.OauthConfigID, keeping the existing behavior when
config.OauthConfigID is nil/empty; update any callers/tests if they relied on a
panic.
In `@core/mcp/toolmanager.go`:
- Around line 557-563: ExecuteTool currently dereferences caller-supplied
pointers without checks (e.g., executionConfig.Name and clientConn.CallTool via
executeToolInternal), which can panic; add nil-guards at the start of
ExecuteTool to validate clientConn and executionConfig (and any nested required
fields like executionConfig.Name) and return a clear error if they are nil, and
ensure executeToolInternal is invoked only after these validations; apply the
same nil checks/guards around the other code paths that call executeToolInternal
(the region referenced around lines 641-700) so no exported API path can
nil-dereference caller inputs.
In `@docs/mcp/auth/per-user-oauth.mdx`:
- Around line 35-57: The Web UI-only Tab block (Tabs/Tab title="Web UI") must be
expanded to include three tabs: Web UI, API, and config.json; add Tab entries
for "API" and "config.json" alongside the existing "Web UI" Tab, put the current
UI steps under Web UI, add an API section describing equivalent API endpoints or
a clear "Not supported" note if APIs aren't available, and include a config.json
example in the config.json Tab that is validated against
transports/config.schema.json (ensure required fields and schema types match);
mirror the same change for the other occurrence referenced around lines 138-151
so both sections follow the Mintlify Web UI / API / config.json structure.
In `@examples/plugins/mcp-only/main.go`:
- Around line 228-233: The current PreMCPConnectionHook logs the full header map
from ctx.Value(schemas.BifrostContextKeyRequestHeaders) when
pluginConfig.EnableLogging is true, which can leak secrets; change the logging
to redact header values (or only log header names) instead: retrieve the header
map in PreMCPConnectionHook, build a sanitized representation that replaces
values for sensitive keys (e.g., "Authorization", "Cookie", "Set-Cookie", any
bearer/token keys) with a fixed mask like "<redacted>" or only emit the header
names, then log that sanitized map instead of the raw allHeaders. Ensure you
reference pluginConfig.EnableLogging, PreMCPConnectionHook and
schemas.BifrostContextKeyRequestHeaders when making the change.
In `@framework/configstore/migrations.go`:
- Around line 8716-8723: GenerateMCPClientHash currently omits
PerUserHeaderKeysJSON so config drift for per_user_header_keys_json isn't
detected; update GenerateMCPClientHash to include the PerUserHeaderKeysJSON
field (serialize it deterministically the same way as Headers/StdioConfig) so
any change changes the hash, and also update mergeMCPConfig to compare
PerUserHeaderKeysJSON and call MarkMCPPerUserHeaderCredentialsNeedsUpdate when
that field differs (or, alternatively, add a dedicated reconciliation step that
flips needs_update for per-user header credentials when
PerUserHeaderKeysJSON/schema changes).
In `@framework/configstore/rdb.go`:
- Around line 5421-5449: The query in
GetMCPPerUserHeaderFlowByModeIdentityAndMCPClient can return expired/stale rows;
update the GORM query (the q variable) to restrict to only live pending flows by
adding a WHERE that the flow is in the pending state and not expired (e.g.,
"state = 'pending' AND expires_at > NOW()" or the equivalent fields you have),
then run q.First(&flow) as before; reference the
GetMCPPerUserHeaderFlowByModeIdentityAndMCPClient function, the q query builder,
and tables.TableMCPPerUserHeaderFlow when making this change.
In `@framework/configstore/tables/mcp_per_user_headers.go`:
- Around line 99-112: BeforeSave is mutating and encrypting HeadersJSON on any
save (including status-only Update calls); change it to only set default
HeadersJSON and perform encryption when the payload is actually being written
(e.g. on create/insert or when HeadersJSON/EncryptionStatus are marked as
changed). In TableMCPPerUserHeaderCredential.BeforeSave use GORM's statement
info (e.g. check tx.Statement.Operation == "INSERT" or
tx.Statement.Changed("HeadersJSON") / tx.Statement.Changed("EncryptionStatus"))
and only then apply the "{}" default, call encryptString(&c.HeadersJSON), and
set EncryptionStatusEncrypted; skip those steps for status-only updates.
In `@framework/mcp_headers/main.go`:
- Around line 80-83: The current error handling for
p.configStore.GetMCPPerUserHeaderCredentialByMode wraps every error, which can
leak configstore.ErrNotFound instead of the documented sentinel; change the
handling to check the returned err with errors.Is(err, configstore.ErrNotFound)
(or equivalent comparison) and return schemas.ErrHeadersCredentialNotFound in
that case, otherwise wrap and return the original error as currently done —
locate the call to GetMCPPerUserHeaderCredentialByMode in main.go (variables
row, err) and update the error branch accordingly.
In `@plugins/governance/main.go`:
- Around line 1725-1745: PreMCPConnectionHook currently treats any returned
virtual key from p.store.GetVirtualKey as valid and writes governance identity
into context (ctx.SetValue with
schemas.BifrostContextKeyGovernanceVirtualKeyID/...); update the hook to first
check the virtual key's active/valid status (e.g., vk.Active / vk.Revoked /
vk.IsActive or vk.State) and if the key is inactive/revoked skip writing any
governance fields and return req, nil, nil instead; ensure all ctx.SetValue
calls for virtual-key, team and customer IDs/names are guarded behind that
active check so revoked keys cannot set identity on the MCP connect path.
In `@transports/bifrost-http/handlers/mcp_per_user_headers.go`:
- Around line 195-207: The handler currently reads flows with
GetMCPPerUserHeaderFlowByID (used in flowSubmit and again at 275-285) and then
best-effort deletes them, which allows race conditions; change the flow
consumption to an atomic compare-and-consume operation in the ConfigStore (e.g.,
add/use a CompareAndConsumeMCPPerUserHeaderFlow or
UpdateMCPPerUserHeaderFlowIfStatus API) that transitions the flow from a known
pending status to consumed/deleted in one DB transaction before calling
UpsertCredential; if the compare-and-consume fails (no rows affected or status
!= pending) return an appropriate error (409 Conflict or 410 Gone) and do not
call UpsertCredential, and ensure both occurrences (the current flowSubmit path
and the code at 275-285) use this new atomic consume operation.
In `@transports/bifrost-http/handlers/mcp.go`:
- Line 1039: The persisted DB update record is built from req.TableMCPClient
before resolvePerUserHeaderKeys is applied to runtime schemasConfig, so omitted
per_user_header_keys can desync DB state; update the code path that constructs
the persisted update (the object built from req.TableMCPClient) to set
PerUserHeaderKeys = resolvePerUserHeaderKeys(existingConfig, req) (or call
resolvePerUserHeaderKeys with the same args and assign its result) so the
persisted record and runtime schemasConfig use the same resolved per-user header
keys; ensure you reference resolvePerUserHeaderKeys, PerUserHeaderKeys,
existingConfig and req.TableMCPClient in that updated assignment.
---
Outside diff comments:
In `@core/bifrost.go`:
- Around line 3649-3677: The VerifyHeadersConnection path can panic if a nil
context is forwarded; before calling MCPManager.VerifyHeadersConnection (or
before delegating any ctx into mcp/clientmanager.go), ensure you replace a nil
ctx with a fallback context (e.g., bifrost.ctx or context.Background()) so
VerifyHeadersConnection never receives nil; update AddMCPClient (and any other
call sites that forward ctx into MCPManager.VerifyHeadersConnection) to check if
ctx == nil and set ctx = bifrost.ctx (or context.Background()) before calling
MCPManager.VerifyHeadersConnection so context.WithTimeout inside
VerifyHeadersConnection is always safe.
In `@docs/mcp/sessions.mdx`:
- Around line 1-195: The page "MCP Sessions" (docs/mcp/sessions.mdx) is missing
the required Mintlify tabbed sections; add a Mintlify Tab structure containing
"Web UI", "API", and "config.json" tabs beneath the Overview (or after the
frontmatter) and move or duplicate relevant content into each tab as
appropriate: put the UI walkthrough and Actions/Statuses under "Web UI",
document endpoints/responses and any unsupported flows under "API", and include
a validated example config JSON under "config.json" (explicitly note unsupported
options when applicable); validate the example JSON against
transports/config.schema.json before committing.
In `@framework/configstore/store.go`:
- Around line 541-552: The startup log currently runs before validating the
config and may report a connection attempt even when the type assertion fails;
move the logger.Info("connecting to %s database", config.Type) call inside each
successful branch after the type assertion passes (i.e., inside the block where
ConfigStoreTypeSQLite and config.Config is asserted to *SQLiteConfig before
calling newSqliteConfigStore, and likewise inside the ConfigStoreTypePostgres
branch after asserting *PostgresConfig and before calling
newPostgresConfigStore) so only valid backend initializations emit the
"connecting" message.
In `@transports/bifrost-http/lib/config.go`:
- Around line 4751-4766: Before appending/activating a new or updated MCP
client, canonicalize its PerUserHeaderKeys so the in-memory clientConfig matches
the persisted/hashed form: in AddMCPClient (and the analogous UpdateMCPClient
path) normalize clientConfig.PerUserHeaderKeys (use the existing
normalize/canonicalization helper used at persistence if available, otherwise
implement consistent normalization such as trimming and lowercasing each header
and removing duplicates) and assign the normalized slice back onto clientConfig
before appending to c.MCPConfig.ClientConfigs and before calling
c.client.AddMCPClient; this ensures case-variant header names don't diverge
between runtime and stored state.
In `@ui/app/workspace/mcp-registry/views/mcpClientForm.tsx`:
- Line 282: The Sheet's onOpenChange currently prevents closing only when
oauthFlow is active, but must also block dismissal when headersFlow.payload
exists; update the onOpenChange handler in the Sheet component so it checks both
oauthFlow and headersFlow.payload (or a boolean like headersFlowOpen) and only
calls onClose() when neither oauthFlow nor headersFlow.payload are set, ensuring
Escape/backdrop closes are ignored while the per-user headers authorizer is
open.
---
Minor comments:
In `@core/mcp/agent_test.go`:
- Around line 86-90: The mock RunWithPluginPipeline stubs
(MockClientManager.RunWithPluginPipeline and
MockAutoClientManager.RunWithPluginPipeline) currently convert any op(req) error
into a plain BifrostError with only Error.Message, losing concrete
*schemas.MCPAuthRequiredError details; update both methods to inspect the
returned err using errors.As to see if it is a *schemas.MCPAuthRequiredError
and, when so, populate the returned
*schemas.BifrostError.ExtraFields.MCPAuthRequired with that typed error (while
still setting Error.Message and IsBifrostError appropriately), otherwise fall
back to the existing behavior — this mirrors production code's preservation of
auth-required metadata for proper test coverage.
In `@core/mcp/credstore/static_headers.go`:
- Around line 28-31: The loop over config.Headers that sets the Authorization
header is nondeterministic because it ranges a map; change SetAuthorization
logic in static_headers.go so it first checks for an exact key "Authorization"
in config.Headers and uses that value (headers.Set("Authorization",
value.GetValue())), and only if that exact key is missing scan the map for a
single case-insensitive match (e.g., strings.EqualFold) and use that;
alternatively, if you prefer strictness, detect multiple case-variant duplicates
and return an error instead of picking one. Update the code that currently
iterates over config.Headers (the block that calls headers.Set("Authorization",
...)) to implement this deterministic precedence and ensure only one value is
chosen.
In `@core/mcp/utils/utils.go`:
- Around line 35-39: The BuildMCPCallbackBaseURL function should return "" when
passed a nil context to avoid a panic; update BuildMCPCallbackBaseURL to check
if ctx == nil and immediately return "" before calling
ctx.Value(schemas.BifrostContextKeyMCPCallbackBaseURL), preserving the existing
behavior of trimming trailing slashes and returning "" for missing/empty values
(matching the nil-safe pattern used by ExtractFilteredExtras).
In `@docs/mcp/auth/per-user-headers.mdx`:
- Around line 144-145: The docs text incorrectly states that removed keys in
per_user_header_keys force existing credentials to flip to needs_update; update
the wording to match the handler logic by stating that only when new required
keys are introduced are credentials marked needs_update and end-users receive an
mcp_auth_required payload and are sent to the submission form (existing values
are preserved for unchanged keys and removed keys do not trigger re-submission).
Ensure you reference the behavior around per_user_header_keys, needs_update, and
mcp_auth_required so readers see the exact implemented flow.
In `@docs/plugins/writing-go-plugin.mdx`:
- Around line 1315-1318: Update the symbol-check command to include the required
Cleanup export so the grep will detect it; modify the pattern used in the go
tool nm pipeline (currently matching Init, GetName, PreLLMHook, PreMCPHook,
PreMCPConnectionHook, PostMCPHook, PostMCPConnectionHook) to also match Cleanup
(e.g., add Cleanup to the alternation alongside Init and GetName) so missing
Cleanup is reported when validating plugin symbols.
In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx`:
- Around line 910-918: The Agent Mode help link in mcpClientSheet.tsx (the <a>
element rendering the Info icon with className "text-muted-foreground ...")
lacks a stable selector for E2E tests; add a data-testid attribute (matching the
pattern used by the Code Mode help link) to that anchor (e.g.,
data-testid="mcp-agent-mode-help-link" or the existing Code Mode test id) so the
interactive element can be targeted reliably in tests.
In `@ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx`:
- Around line 62-70: The onCloseAutoFocus handler on DropdownMenuContent is
preventing focus restore for every close path; add a boolean flag (e.g.,
isOpeningSheet) in the component state and set it only when the Edit action is
invoked (for example, in your edit handler like handleEditClick or the Edit menu
item's onSelect), pass that flag into DropdownMenuContent and change
onCloseAutoFocus to call e.preventDefault() only when isOpeningSheet is true,
then reset isOpeningSheet (false) when the Sheet finishes opening or when the
Edit flow completes/aborts so normal close paths
(Escape/outside-click/Delete/Reconnect) restore focus as before.
In `@ui/app/workspace/mcp-sessions/auth/page.tsx`:
- Around line 297-306: The page trusts the editable "submitted" query param
(useQueryState("submitted")) to skip the fetch and render success; instead, stop
relying on the URL for authoritative success and keep submit state locally:
replace the query-param-backed submitted/setSubmitted with a local React state
(e.g., useState in the same component) and only set that state to "submitted"
after the actual submit call resolves successfully; remove the skip: submitted
=== "true" condition passed to useGetMCPPerUserHeadersFlowQuery (use flowId
only) so the GET always runs unless the local success state is set, and update
any other uses (lines ~310-325) that read submitted to read the local state so a
hand-modified URL cannot fake success.
---
Nitpick comments:
In `@core/mcp/credstore/server_oauth.go`:
- Line 1: Rename the file server_oauth.go to follow the repository Go filename
convention (concatenate words, lowercase) — e.g., serveroauth.go; ensure the
package declaration (package credstore) and all symbols (types/functions in this
file) remain unchanged, update any external tooling or build scripts that
explicitly reference server_oauth.go, and run go build/test to verify no
breakages after the rename.
In `@ui/app/workspace/mcp-registry/views/mcpClientForm.tsx`:
- Around line 196-205: The per-user header validation currently only triggers a
destructive toast inside the authType === "per_user_headers" branch (see
perUserHeaderKeys, toast and hasErrors) — change this to set and render an
inline field error for the textarea that collects perUserHeaderKeys (match the
pattern used elsewhere in this form for inline errors), e.g., add a validation
state/variable (e.g., perUserHeaderKeysError) when perUserHeaderKeys.length ===
0 instead of calling toast, propagate it into the textarea component's
error/help props so the message "Declare at least one header name users must
supply." appears next to the textarea, and remove the toast call; apply the same
inline-error approach to the other occurrence referenced (lines ~512-539) so
both places show field-level errors rather than toasts.
🪄 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: 9454e2a6-6526-47eb-ae94-6a30a68cbfa4
⛔ Files ignored due to path filters (20)
docs/media/mcp-classic-flow.pngis excluded by!**/*.pngdocs/media/mcp-codemode-cost-diff.pngis excluded by!**/*.pngdocs/media/mcp-codemode-flow.pngis excluded by!**/*.pngdocs/media/mcp-codemode-tokens-diff.pngis excluded by!**/*.pngdocs/media/ui-config-mcp-disable-auto-tool-inject.pngis excluded by!**/*.pngdocs/media/ui-mcp-auth-headers-form.pngis excluded by!**/*.pngdocs/media/ui-mcp-auth-none-form.pngis excluded by!**/*.pngdocs/media/ui-mcp-auth-oauth-popup.pngis excluded by!**/*.pngdocs/media/ui-mcp-auth-per-user-headers-verify-dialog.pngis excluded by!**/*.pngdocs/media/ui-mcp-per-user-auth-flow-lazy.svgis excluded by!**/*.svgdocs/media/ui-mcp-per-user-headers-submit.pngis excluded by!**/*.pngdocs/media/ui-mcp-per-user-headers-success.pngis excluded by!**/*.pngdocs/media/ui-mcp-per-user-headers-update.pngis excluded by!**/*.pngdocs/media/ui-mcp-per-user-oauth-consent-flow.pngis excluded by!**/*.pngdocs/media/ui-mcp-per-user-oauth-flow-lazy.svgis excluded by!**/*.svgdocs/media/ui-mcp-per-user-oauth-setup.pngis excluded by!**/*.pngdocs/media/ui-mcp-sessions-table.pngis excluded by!**/*.pngexamples/mcps/temperature/package-lock.jsonis excluded by!**/package-lock.jsonexamples/mcps/test-tools-server/package-lock.jsonis excluded by!**/package-lock.jsonexamples/plugins/mcp-only/go.sumis excluded by!**/*.sum
📒 Files selected for processing (100)
core/bifrost.gocore/internal/mcptests/agent_filtering_test.gocore/internal/mcptests/client_management_test.gocore/internal/mcptests/concurrency_advanced_test.gocore/internal/mcptests/connect_ping_listtools_test.gocore/internal/mcptests/error_handling_protocol_test.gocore/internal/mcptests/health_monitoring_test.gocore/internal/mcptests/integration_test.gocore/internal/mcptests/tool_filtering_test.gocore/mcp/agent.gocore/mcp/agent_test.gocore/mcp/clientmanager.gocore/mcp/codemode.gocore/mcp/codemode/starlark/executecode.gocore/mcp/codemode/starlark/starlark.gocore/mcp/codemode/starlark/starlark_test.gocore/mcp/credstore/credstore.gocore/mcp/credstore/none.gocore/mcp/credstore/per_user_headers.gocore/mcp/credstore/per_user_oauth.gocore/mcp/credstore/server_oauth.gocore/mcp/credstore/static_headers.gocore/mcp/credstore/utils.gocore/mcp/exec.gocore/mcp/interface.gocore/mcp/mcp.gocore/mcp/pluginpipeline.gocore/mcp/toolmanager.gocore/mcp/toolmanager_test.gocore/mcp/utils.gocore/mcp/utils/utils.gocore/schemas/bifrost.gocore/schemas/mcp.gocore/schemas/mcp_headers.godocs/cli-agents/claude-desktop.mdxdocs/cli-agents/overview.mdxdocs/cli-agents/roo-code.mdxdocs/docs.jsondocs/mcp/auth/headers.mdxdocs/mcp/auth/none.mdxdocs/mcp/auth/oauth.mdxdocs/mcp/auth/overview.mdxdocs/mcp/auth/per-user-headers.mdxdocs/mcp/auth/per-user-oauth.mdxdocs/mcp/code-mode.mdxdocs/mcp/connecting-to-servers.mdxdocs/mcp/gateway.mdxdocs/mcp/oauth.mdxdocs/mcp/overview.mdxdocs/mcp/per-user-oauth.mdxdocs/mcp/sessions.mdxdocs/openapi/openapi.jsondocs/openapi/openapi.yamldocs/openapi/paths/management/mcp.yamldocs/openapi/paths/management/oauth.yamldocs/openapi/schemas/management/mcp.yamldocs/openapi/schemas/management/oauth.yamldocs/overview.mdxdocs/plugins/writing-go-plugin.mdxexamples/plugins/mcp-only/README.mdexamples/plugins/mcp-only/go.modexamples/plugins/mcp-only/main.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/mcp.goframework/configstore/tables/mcp_per_user_headers.goframework/mcp_headers/main.goframework/mcp_headers/sweep.goframework/temptoken/scope.goplugins/governance/main.goplugins/logging/main.goplugins/logging/operations_test.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcp_per_user_headers.gotransports/bifrost-http/handlers/mcp_sessions.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/temp_token_scopes.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/lib.gotransports/bifrost-http/server/server.goui/app/_fallbacks/enterprise/components/mcp-tool-groups/mcpToolGroups.tsxui/app/workspace/mcp-registry/views/mcpClientForm.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/mcpClientsTable.tsxui/app/workspace/mcp-registry/views/mcpHeadersAuthorizer.tsxui/app/workspace/mcp-sessions/auth/page.tsxui/app/workspace/mcp-sessions/views/sessionsTable.tsxui/components/headersForm.tsxui/lib/store/apis/baseApi.tsui/lib/store/apis/index.tsui/lib/store/apis/mcpApi.tsui/lib/store/apis/mcpPerUserHeadersApi.tsui/lib/types/mcp.tsui/lib/types/mcpPerUserHeaders.tsui/lib/types/mcpSessions.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (2)
- docs/mcp/oauth.mdx
- docs/mcp/per-user-oauth.mdx
✅ Files skipped from review due to trivial changes (12)
- ui/lib/store/apis/index.ts
- ui/app/_fallbacks/enterprise/components/mcp-tool-groups/mcpToolGroups.tsx
- core/mcp/credstore/none.go
- core/mcp/credstore/utils.go
- docs/cli-agents/roo-code.mdx
- docs/mcp/auth/none.mdx
- ui/lib/store/apis/baseApi.ts
- docs/cli-agents/claude-desktop.mdx
- docs/mcp/auth/headers.mdx
- docs/mcp/overview.mdx
- docs/mcp/auth/overview.mdx
- examples/plugins/mcp-only/README.md
6110a99 to
155f865
Compare
Merge activity
|
## Summary Adds a log message when the config store initializes a database connection, making it easier to observe which database backend is being used at startup. ## Changes - A log line is emitted at the `Info` level before the database type switch, reporting the configured store type (e.g., SQLite) when a connection is being established. ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./... ``` Start the application with a configured config store and verify that a log line similar to the following appears on startup: ``` connecting to sqlite database ``` ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations The log message only exposes the database type (e.g., `sqlite`), not any connection credentials or sensitive configuration values. ## 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
## Summary This PR releases **core v1.5.14**, **framework v1.3.14**, **transports v1.5.6**, and bumps all dependent plugins to their respective `.14` patch versions. It delivers a broad set of new capabilities across MCP authentication, key rotation, OTel metrics, Bedrock/Anthropic compatibility, and UI improvements, alongside a number of targeted bug fixes and refactors. ## Changes - **Direct API Key Header** — Providers can now receive an API key passed directly via a request header (#3817) - **MCP Per-User Auth** — Introduced `MCPCredentialStore` abstraction, per-user MCP credential reconciliation, and a new per-user header auth type with lazy-auth submission flow (#3656, #3702, #3703, #3704, #3705) - **MCP TLS Configuration** — Added configurable TLS (`insecureSkipVerify`, `caCertPem`) for HTTP/SSE MCP client connections (#3779, #3783) - **MCP Sessions Management** — Filter, search, and pagination on the MCP sessions list API and table, plus a `can_reauth` identity gate (#3823, #3824, #3825) - **Key Rotation** — Keys now rotate on 401/402/403 responses; returns `502 upstream_credentials_exhausted` when all keys are permanently exhausted. Added `triggered_rotation` to `KeyAttemptRecord` and tightened `bifrost_key_rotation_events_total` semantics (#3430, #3491) - **OTel Metrics** — Added OTel spec-compatible metrics (backward compatible) with provider cache and semantic cache attributes in metrics export (#3865, #3816) - **Opus 4.8 Support** — System message handling and general compatibility for Opus 4.8 (#3868, #3878) - **Dimension Rankings** — New `GetDimensionRankings` API and dashboard tabs for team, customer, BU, and user rankings (#3766) - **Model Pricing Attributes** — `additional_attributes` field on model pricing rows with management API and UI editor (#3829) - **Prompt Cache Retention** — Added prompt cache retention parameter on responses requests (#3810) - **Tool Call Execution UI** — Inline tool-call execution, stop streaming, bulk execute/submit, and a redesigned tool-call UI (#3837, #3843) - **Sheet Navigation** — Prev/next keyboard navigation and URL state across virtual key, MCP client, and routing rule sheets (#3739, #3740, #3744, #3745) - **Bedrock Tool Name Truncation** — Truncate Bedrock function/tool names to the provider length limit - **Bedrock Guardrails** — Set guardrail config in Bedrock requests built from responses (#3862) - **Anthropic Tool Use** — Default `tool_use` input to `{}` when arguments are absent (#3880) - **Responses Streaming** — Fixed responses stream events (#3838) - **Compat Flow** — Fixed missing parameter parsing on the compat flow (#3881) - **Passthrough API Version** — Set a default API version in passthrough requests as a fallback (#3853) - **Virtual Key Updates** — Avoid overriding optional fields during virtual key update (#3855) - **User-Mode Flows** — Gate user-mode flows on caller `user_id`, skip temp token mint, and unify flow/credential kind filtering for pending flows (#3841, #3859) - **Partial Tool Calls** — Handle partial tool call execution failures and return successful results (#3849) - **URL Query Escaping** — Support escaped characters in URL query parameters (#3826) - **MCP Auth Errors** — Inline banner and retry support for MCP auth-required errors (#3856) - **Renamed Resolvers** — `staticHeadersResolver`/`serverOAuthResolver` renamed to `sharedHeadersResolver`/`sharedOAuthResolver` (#3840) - **Starlark Nested Tool Calls** — Exposed `RunWithPluginPipeline` on `ClientManager` and routed Starlark nested tool calls through the canonical plugin gate (#3794) - **Deferred-Fill OAuth Removed** — Removed deferred-fill user-mode OAuth flow support (#3839) - **Go 1.26.3** — Upgraded toolchain to Go 1.26.3 (#3782) ## Type of change - [x] Bug fix - [x] Feature - [x] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go version # should report go1.26.3 go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` - Validate MCP per-user auth by configuring a per-user header auth type and confirming credentials are stored and reconciled on virtual key and MCP client changes. - Validate key rotation by triggering a 401/402/403 from an upstream provider and confirming rotation occurs; exhaust all keys and confirm a `502 upstream_credentials_exhausted` is returned. - Validate OTel metrics output includes `provider_cache` and `semantic_cache` attributes. - Validate Bedrock requests with tool names exceeding the provider limit are truncated correctly. - Validate Opus 4.8 system message handling by sending a request with a system message to an Opus 4.8 endpoint. ## Breaking changes - [x] Yes - [ ] No The deferred-fill user-mode OAuth flow has been removed (#3839). Any integrations relying on that flow must migrate to the new per-user credential store approach. The `staticHeadersResolver` and `serverOAuthResolver` identifiers have been renamed to `sharedHeadersResolver` and `sharedOAuthResolver` respectively (#3840); any direct references must be updated. ## Related issues #3817, #3656, #3702, #3703, #3704, #3705, #3779, #3783, #3823, #3824, #3825, #3430, #3491, #3865, #3816, #3868, #3878, #3766, #3829, #3810, #3837, #3843, #3739, #3740, #3744, #3745, #3862, #3880, #3838, #3881, #3853, #3855, #3841, #3859, #3849, #3826, #3856, #3840, #3794, #3839, #3782, #3724, #3814, #3836, #3869, #3886 ## Security considerations - MCP per-user credentials are stored via the new `MCPCredentialStore` abstraction; ensure the backing store is appropriately access-controlled and that credential values are encrypted at rest. - The direct API key header feature passes provider secrets via HTTP headers; ensure TLS is enforced on all ingress paths and that headers are not logged in plaintext. - User-mode flows are now gated on `caller user_id` and temp token minting is skipped where appropriate, reducing the surface for privilege escalation. - TLS configuration for MCP HTTP/SSE connections supports `insecureSkipVerify`; this should only be enabled in controlled environments. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable

Summary
Adds a log message when the config store initializes a database connection, making it easier to observe which database backend is being used at startup.
Changes
Infolevel before the database type switch, reporting the configured store type (e.g., SQLite) when a connection is being established.Type of change
Affected areas
How to test
go test ./...Start the application with a configured config store and verify that a log line similar to the following appears on startup:
Screenshots/Recordings
N/A
Breaking changes
Related issues
N/A
Security considerations
The log message only exposes the database type (e.g.,
sqlite), not any connection credentials or sensitive configuration values.Checklist
docs/contributing/README.mdand followed the guidelines