Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (23)
✅ Files skipped from review due to trivial changes (7)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a persisted per-client ChangesMCP Client Disable/Enable Lifecycle
Sequence Diagram(s)sequenceDiagram
participant User as User (UI)
participant Handler as HTTP Handler
participant Config as Config Layer
participant DBStore as Config Store
participant DB as Database
participant Bifrost as Bifrost Core
participant Manager as MCP Manager
participant Monitor as Health Monitor
User->>Handler: PUT /mcp/clients/:id {disabled: true}
Handler->>Config: DisableMCPClient(id)
Config->>DBStore: GetMCPClientConfigByID(id)
DBStore->>DB: SELECT client
DB-->>DBStore: client row
DBStore-->>Config: config
Config->>DBStore: UpdateMCPClientConfig(id, disabled=true)
DBStore->>DB: UPDATE disabled
DB-->>DBStore: success
Config->>Bifrost: DisableMCPClient(id)
Bifrost->>Manager: DisableClient(id)
Manager->>Monitor: Stop health monitoring
Manager->>Manager: Cancel SSE / Close transport / Set state=disabled
Manager-->>Bifrost: success
Bifrost-->>Config: success
Config-->>Handler: success
Handler->>Handler: SyncAllMCPServers
Handler-->>User: 200 OK {disabled:true, state:"disabled"}
sequenceDiagram
participant User as User (UI)
participant Handler as HTTP Handler
participant Config as Config Layer
participant DBStore as Config Store
participant DB as Database
participant Bifrost as Bifrost Core
participant Manager as MCP Manager
participant Remote as Remote MCP Server
participant Monitor as Health Monitor
User->>Handler: PUT /mcp/clients/:id {disabled: false}
Handler->>Config: EnableMCPClient(id)
Config->>DBStore: GetMCPClientConfigByID(id)
DBStore->>DB: SELECT client
DB-->>DBStore: client row
DBStore-->>Config: config
Config->>DBStore: UpdateMCPClientConfig(id, disabled=false)
DBStore->>DB: UPDATE disabled
DB-->>DBStore: success
Config->>Bifrost: EnableMCPClient(id)
Bifrost->>Manager: EnableClient(id)
Manager->>Manager: Clear ExecutionConfig.Disabled
alt Per-User OAuth
Manager->>Manager: Set state based on ToolMap (connected|pending_tools)
else Other Auth
Manager->>Remote: connectToMCPClient
Remote-->>Manager: success or error
alt success
Manager->>Manager: Set state = connected/pending_tools
else failure
Manager->>Manager: Set state = disconnected
Manager->>Monitor: Start health monitoring
end
end
Manager-->>Bifrost: success
Bifrost-->>Config: success
Config-->>Handler: success
Handler->>Handler: SyncAllMCPServers
Handler-->>User: 200 OK {disabled:false, state:"connected"|"pending_tools"}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Review rate limit: 3/5 reviews remaining, refill in 22 minutes and 29 seconds. Comment |
This was referenced Apr 27, 2026
feat: add opt-in per-request overrides for content logging and raw request/response visibility
#3066
Merged
Collaborator
Author
18 tasks
|
|
Contributor
Confidence Score: 3/5Not safe to merge — new P1 gap in GetClientForTool and several prior P1s are still open. Multiple P1 findings: the new GetClientForTool disabled-state gap allows disabled per-user-OAuth tools to execute; the for-range copy bug in DisableMCPClient/EnableMCPClient leaves in-memory ClientConfigs permanently stale; the ExecutionConfig.Disabled rollback gap silently re-enables a client after a failed enable. Score is pulled to 3 (below the P1 ceiling of 4) because there are three independent P1s affecting the core disable/enable path. core/mcp/utils.go (GetClientForTool missing disabled guard), transports/bifrost-http/lib/config.go (for-range copy in DisableMCPClient/EnableMCPClient), core/mcp/clientmanager.go (EnableClient error path and ExecutionConfig rollback) Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Pratham-Mishra04
force-pushed
the
04-27-docs_default_provider_selection_doc_updates
branch
from
920eba0 to
62a5d4c
Compare
Pratham-Mishra04
force-pushed
the
04-27-feat_adds_option_to_disable_mcp_clients
branch
from
05848bb to
8190181
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx (1)
274-274: Consider guarding reconnect onconfig.disabledas well.This makes the button resilient to transient state lag between config persistence and runtime state refresh.
Suggested tweak
- disabled={reconnectingClients.includes(c.config.client_id) || !hasUpdateMCPClientAccess || c.state === "disabled"} + disabled={ + reconnectingClients.includes(c.config.client_id) || + !hasUpdateMCPClientAccess || + c.state === "disabled" || + c.config.disabled + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx` at line 274, Update the disabled predicate for the reconnect control to also guard on the client's persisted config disabled flag (use c.config.disabled or c.config?.disabled) so the button is disabled when the config is disabled in addition to when reconnectingClients.includes(c.config.client_id), !hasUpdateMCPClientAccess, or c.state === "disabled"; modify the disabled expression in mcpClientsTable.tsx where reconnectingClients, hasUpdateMCPClientAccess and c.state are checked to include this new config check.ui/lib/constants/config.ts (1)
98-104: KeepMCP_STATUS_COLORSkeyed byMCPConnectionState.
Record<string, string>drops exhaustiveness here, so future MCP state additions/removals can silently miss a badge mapping. Using the shared union preserves compile-time coverage for the exact states this PR extends.♻️ Suggested tightening
import { BaseProvider, ConcurrencyAndBufferSize, NetworkConfig } from "@/lib/types/config"; +import type { MCPConnectionState } from "@/lib/types/mcp"; import { ProviderName } from "./logs"; @@ -export const MCP_STATUS_COLORS: Record<string, string> = { +export const MCP_STATUS_COLORS: Record<MCPConnectionState, string> = { connected: "bg-green-100 text-green-800", error: "bg-red-100 text-red-800", disconnected: "bg-gray-100 text-gray-800", pending_tools: "bg-yellow-100 text-yellow-800", disabled: "bg-orange-100 text-orange-800", };As per coding guidelines,
ui/lib/types/*.ts: Define reusable TypeScript types in shared locations (ui/lib/types/ for frontend).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/lib/constants/config.ts` around lines 98 - 104, MCP_STATUS_COLORS is typed as Record<string,string>, losing exhaustiveness for MCPConnectionState; change its type to Record<MCPConnectionState, string> (or Partial<Record<MCPConnectionState, string>> if some states intentionally lack mappings) and import the MCPConnectionState union from the shared frontend types (ui/lib/types/*), then update the declaration of MCP_STATUS_COLORS to use that union so the compiler enforces coverage when states are added/removed.
🤖 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/healthmonitor.go`:
- Around line 159-164: The code reads clientState.State without holding
chm.manager.mu, causing a data race; under the manager lock client state is
mutated in updateClientState(), so capture the disabled flag while inside the
manager RLock (or copy clientState into a local variable) then release the lock
and perform the check and call chm.Stop(); follow the same pattern used in
attemptReconnect() (lines around attemptReconnect) to check for
schemas.MCPConnectionStateDisabled while still holding chm.manager.mu.RLock()
before returning.
In `@docs/mcp/connecting-to-servers.mdx`:
- Around line 924-959: The "Disabling and Re-enabling Clients" section is
inconsistent with the updated UI and state model: update the Web UI copy to
reference the new "Disable Client" control in the client's edit sheet (instead
of the inline Enabled toggle in the server catalog table) and mention that the
control shows an in-flight spinner and updates state; also add "disabled" to the
earlier Connection States table on this page so the state model includes it;
keep the existing notes about persistence across restarts and that `disabled` is
a runtime API state that cannot be set in config.json, but ensure the text
references the edit-sheet control name ("Disable Client") and the Connection
States table entry "disabled" for consistency.
In `@framework/configstore/migrations.go`:
- Around line 7052-7057: The migration adds the "disabled" column to
tables.TableMCPClient but doesn't backfill existing NULLs; update the migration
so that after the mg.AddColumn(&tables.TableMCPClient{}, "disabled") call you
run an explicit backfill (e.g., execute an UPDATE on the underlying table to set
disabled = false for rows where disabled IS NULL) to match the pattern used by
is_code_mode_client and is_ping_available, and ensure this happens before
returning nil so existing rows are consistent with the new boolean column.
In `@framework/configstore/rdb.go`:
- Line 1511: Update UpdateMCPClientConfig so it does not overwrite a DB-managed
Disabled flag when reconciliation is driven by the config file: detect when the
incoming row is being synced from the config file (ConfigHash present/being
synced) and read the current Disabled value from the DB row, then use that DB
Disabled value instead of clientConfigCopy.Disabled when building the map
written to the table (the entry currently writing "disabled":
clientConfigCopy.Disabled in rdb.go). Keep references: UpdateMCPClientConfig,
mcpClientConfigToTable, clientConfigCopy.Disabled and ConfigHash to locate the
logic to change.
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 813-828: The handler updates the persisted disabled flag before
calling h.mcpManager.DisableMCPClient/EnableMCPClient, so if those lifecycle
calls fail the DB stays changed; capture the prior disabled value (e.g.
oldDisabled) before applying the update, and if DisableMCPClient or
EnableMCPClient returns an error call a rollback to restore the persisted flag
(use an existing updater on h.mcpManager such as a Set/Update disabled method or
add one) and log any rollback error; ensure you still SendError to the caller
and include context in both the lifecycle error and any rollback failure so
operator-visible state remains consistent with runtime.
- Around line 795-798: The request currently uses a plain bool req.Disabled
which defaults to false when omitted, causing accidental re-enables and uses
existingConfig (a live pointer) to compute disabledToggled; change the request
type to a presence-aware field (e.g. Disabled *bool) or otherwise detect whether
the field was supplied, then compute the effective Disabled value as: if
req.Disabled != nil use *req.Disabled else use oldDBConfig.Disabled; compute
disabledToggled by comparing oldDBConfig.Disabled (the stable pre-update value
you already loaded) to the new effective Disabled; update the struct
initialization and any logic that referenced req.Disabled and disabledToggled
(look for usages around existingConfig, oldDBConfig, Disabled, and
disabledToggled in this handler) to use the presence-aware value and oldDBConfig
for comparisons.
In `@transports/bifrost-http/lib/config.go`:
- Around line 4612-4632: The code currently flips DB and in-memory state before
calling the runtime toggle (c.client.DisableMCPClient / EnableMCPClient), which
leaves config marked changed if the runtime call fails; update DisableMCPClient
and EnableMCPClient so that either (A) you call
c.client.DisableMCPClient(id)/EnableMCPClient(id) first and only
persist/in-memory-update on success, or (B) if you must persist first, perform
the inverse rollback on both the persistent store
(c.ConfigStore.UpdateMCPClientConfig) and the in-memory MCPConfig (protected by
c.muMCP) when c.client.DisableMCPClient/EnableMCPClient returns an error; ensure
you reference and update dbClient.Disabled, cc.Disabled in the MCPConfig loop,
and return the original error after rollback.
- Line 1254: The config value `Disabled` is being treated as persistent but must
be runtime-only; ensure file-loaded values do not carry `Disabled` into
persisted/merged state by clearing it during load/parse. Specifically, in
ConfigData.UnmarshalJSON and loadMCPConfig (which accept
schemas.MCPClientConfig), reset clientConfig.Disabled = false (or omit setting
Disabled) before merging/persisting and ensure the place that builds the table
entry (where Disabled: clientConfig.Disabled is currently set) uses the
cleared/explicit false value so `"disabled": true` from config.json cannot
survive boot.
In `@transports/bifrost-http/server/server.go`:
- Around line 239-241: The current branches call
s.MCPServerHandler.SyncAllMCPServers(ctx) and only log failures (logger.Warn)
which causes the API to return success while MCP sync failed; change both
occurrences (the SyncAllMCPServers calls in the disable/enable client branches)
to propagate the error instead of suppressing it — i.e., if SyncAllMCPServers
returns an error, return that error (or wrap it with context) from the
surrounding handler/function so the HTTP response reflects the failure rather
than just logging it. Ensure you update both places where SyncAllMCPServers is
invoked in this file.
In `@ui/lib/types/schemas.ts`:
- Line 1020: The schema exposes "disabled: z.boolean().optional()," but the
optimistic cache merge skips data.disabled, so update the optimistic cache/merge
logic (where the mutation onSuccess or cache updater runs) to copy
response.data.disabled into the cached object — e.g. set cachedItem.disabled =
response.data.disabled ?? cachedItem.disabled (or include response.data.disabled
when spreading) so the Enabled badge/toggle reflects the new value immediately;
ensure you reference response.data.disabled in that cache merge/update path.
---
Nitpick comments:
In `@ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx`:
- Line 274: Update the disabled predicate for the reconnect control to also
guard on the client's persisted config disabled flag (use c.config.disabled or
c.config?.disabled) so the button is disabled when the config is disabled in
addition to when reconnectingClients.includes(c.config.client_id),
!hasUpdateMCPClientAccess, or c.state === "disabled"; modify the disabled
expression in mcpClientsTable.tsx where reconnectingClients,
hasUpdateMCPClientAccess and c.state are checked to include this new config
check.
In `@ui/lib/constants/config.ts`:
- Around line 98-104: MCP_STATUS_COLORS is typed as Record<string,string>,
losing exhaustiveness for MCPConnectionState; change its type to
Record<MCPConnectionState, string> (or Partial<Record<MCPConnectionState,
string>> if some states intentionally lack mappings) and import the
MCPConnectionState union from the shared frontend types (ui/lib/types/*), then
update the declaration of MCP_STATUS_COLORS to use that union so the compiler
enforces coverage when states are added/removed.
🪄 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: 2df57b70-dab9-422f-aab2-b5abfdc2de30
📒 Files selected for processing (22)
core/bifrost.gocore/changelog.mdcore/mcp/clientmanager.gocore/mcp/healthmonitor.gocore/mcp/interface.gocore/mcp/utils.gocore/schemas/mcp.godocs/mcp/connecting-to-servers.mdxdocs/openapi/paths/management/mcp.yamldocs/openapi/schemas/management/mcp.yamlframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/mcp.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/changelog.mdui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/mcpClientsTable.tsxui/lib/constants/config.tsui/lib/types/mcp.tsui/lib/types/schemas.ts
Pratham-Mishra04
changed the base branch from
04-27-docs_default_provider_selection_doc_updates
to
graphite-base/3076
This was referenced Apr 27, 2026
fix: allow self-looping chain rules to continue evaluating subsequent rules instead of halting
#3082
Merged
Pratham-Mishra04
changed the title
BearTS
force-pushed
the
04-27-feat_adds_option_to_disable_mcp_clients
branch
from
8190181 to
3dd5f07
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
framework/configstore/rdb.go (1)
1523-1535:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve DB
disabledduring config-hash reconciliation to avoid accidental re-enable.When reconciliation is config-file driven (
ConfigHash != ""), writingclientConfigCopy.Disabledcan unintentionally clear a runtime-disabled client if config input doesn’t carry that state.💡 Suggested fix
- updates := map[string]interface{}{ + disabledToPersist := clientConfigCopy.Disabled + // During config-file reconciliation, preserve runtime managed disabled state. + if clientConfigCopy.ConfigHash != "" { + disabledToPersist = existingClient.Disabled + } + + updates := map[string]interface{}{ "name": clientConfigCopy.Name, "is_code_mode_client": clientConfigCopy.IsCodeModeClient, "tools_to_execute_json": string(toolsToExecuteJSON), "tools_to_auto_execute_json": string(toolsToAutoExecuteJSON), "headers_json": headersJSONStr, "allowed_extra_headers_json": string(allowedExtraHeadersJSON), "tool_pricing_json": string(toolPricingJSON), "tool_sync_interval": clientConfigCopy.ToolSyncInterval, "allow_on_all_virtual_keys": clientConfigCopy.AllowOnAllVirtualKeys, - "disabled": clientConfigCopy.Disabled, + "disabled": disabledToPersist, "updated_at": time.Now(), }Also applies to: 1541-1561
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/rdb.go` around lines 1523 - 1535, When reconciling a config-driven client (i.e., when clientConfigCopy.ConfigHash != ""), do not overwrite the DB "disabled" flag with clientConfigCopy.Disabled; instead query the existing DB record's disabled value and use that for the updates map so runtime-disable is preserved. Locate the code building the updates map (references: clientConfigCopy, updates map, "disabled" key) and when ConfigHash != "" replace the value source for "disabled" with the existing record's Disabled field (fetched prior to building updates); apply the same change in the other reconciliation branch referenced around the later updates block (lines noted in the review).
🤖 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/clientmanager.go`:
- Around line 368-369: The code mutates the shared ExecutionConfig in place
(clientState.ExecutionConfig.Disabled = true) which breaks the snapshot
immutability that UpdateClient relies on; instead create a new ExecutionConfig
copy, set Disabled=true on that new struct, and assign the pointer back to
clientState.ExecutionConfig so readers keep a stable snapshot. Do the same
replacement for the other occurrence noted around the same block (the lines
analogous to 395-396) and ensure clientState.State is still set to
schemas.MCPConnectionStateDisabled after swapping in the new config.
- Around line 112-125: The disabled-client branch for config.Disabled always
seeds empty ToolMap/ToolNameMapping and returns, wiping persisted per-user OAuth
tools; modify the branch in Register/Bootstrap where m.clientMap[config.ID] is
set so that if config.ConnectionType == "per_user_oauth" and
config.DiscoveredTools / config.DiscoveredToolNameMapping are present, populate
MCPClientState.ToolMap from config.DiscoveredTools and
MCPClientState.ToolNameMapping from config.DiscoveredToolNameMapping (instead of
make(map...)), preserving ConnectionInfo and other fields, then unlock and
return; reference m.clientMap, schemas.MCPClientState,
ConnectionType/per_user_oauth, DiscoveredTools, and DiscoveredToolNameMapping to
locate and update the initialization logic.
- Around line 383-397: EnableClient currently unlocks before starting
connectToMCPClient which allows two concurrent callers to both pass the disabled
check and race; fix by making the enable path single-flight: while holding m.mu
check clientState.State and then mark the client in m.reconnectingClients (or
set a reconnecting flag) to reserve the slot, and only then release m.mu and
call connectToMCPClient; ensure you remove/unset the m.reconnectingClients entry
on error/after successful connection so subsequent enables behave correctly;
update references to EnableClient, clientState, m.mu, m.reconnectingClients and
connectToMCPClient accordingly.
---
Duplicate comments:
In `@framework/configstore/rdb.go`:
- Around line 1523-1535: When reconciling a config-driven client (i.e., when
clientConfigCopy.ConfigHash != ""), do not overwrite the DB "disabled" flag with
clientConfigCopy.Disabled; instead query the existing DB record's disabled value
and use that for the updates map so runtime-disable is preserved. Locate the
code building the updates map (references: clientConfigCopy, updates map,
"disabled" key) and when ConfigHash != "" replace the value source for
"disabled" with the existing record's Disabled field (fetched prior to building
updates); apply the same change in the other reconciliation branch referenced
around the later updates block (lines noted in the review).
🪄 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: 73f93649-057b-4d24-ac03-60774e4f0142
📒 Files selected for processing (12)
core/bifrost.gocore/mcp/clientmanager.gocore/mcp/healthmonitor.gocore/mcp/interface.gocore/mcp/utils.gocore/schemas/mcp.godocs/mcp/connecting-to-servers.mdxdocs/openapi/paths/management/mcp.yamldocs/openapi/schemas/management/mcp.yamlframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/mcp.go
✅ Files skipped from review due to trivial changes (6)
- docs/openapi/paths/management/mcp.yaml
- framework/configstore/tables/mcp.go
- core/mcp/healthmonitor.go
- core/mcp/utils.go
- docs/mcp/connecting-to-servers.mdx
- framework/configstore/migrations.go
🚧 Files skipped from review as they are similar to previous changes (3)
- core/mcp/interface.go
- docs/openapi/schemas/management/mcp.yaml
- core/bifrost.go
BearTS
force-pushed
the
04-27-feat_adds_option_to_disable_mcp_clients
branch
from
3dd5f07 to
560f655
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (5)
core/mcp/clientmanager.go (3)
368-369:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAvoid in-place mutation of
ExecutionConfigsnapshotsLine 369 and Line 395 mutate
clientState.ExecutionConfig.Disableddirectly. That breaks the immutable-snapshot pattern used elsewhere in this file and can race with concurrent readers.Proposed fix
- clientState.State = schemas.MCPConnectionStateDisabled - clientState.ExecutionConfig.Disabled = true + clientState.State = schemas.MCPConnectionStateDisabled + updatedConfig := *clientState.ExecutionConfig + updatedConfig.Disabled = true + clientState.ExecutionConfig = &updatedConfig- clientState.ExecutionConfig.Disabled = false - configCopy := clientState.ExecutionConfig + updatedConfig := *clientState.ExecutionConfig + updatedConfig.Disabled = false + clientState.ExecutionConfig = &updatedConfig + configCopy := clientState.ExecutionConfigAlso applies to: 395-396
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/mcp/clientmanager.go` around lines 368 - 369, The code mutates clientState.ExecutionConfig in place (clientState.ExecutionConfig.Disabled = true), breaking the immutable-snapshot pattern and risking races; instead, create a shallow copy of the ExecutionConfig, set Disabled on that copy, and then assign the copy back to clientState.ExecutionConfig wherever this mutation occurs (the places around clientState.State = schemas.MCPConnectionStateDisabled and the other location referenced near lines 395-396). Locate the clientState variable usage and replace direct mutation with: copy := clientState.ExecutionConfig; modify copy.Disabled; clientState.ExecutionConfig = copy so readers always see an immutable snapshot.
383-397:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMake
EnableClientsingle-flight before unlockingBetween Line 390 and Line 397, concurrent enable requests can both pass the disabled-state check and race into connection setup because this path does not reserve
m.reconnectingClientsbefore unlock.Proposed fix
if clientState.State != schemas.MCPConnectionStateDisabled { m.mu.Unlock() return fmt.Errorf("client %s is not disabled (current state: %s)", clientState.ExecutionConfig.Name, clientState.State) } + if _, inProgress := m.reconnectingClients.LoadOrStore(id, true); inProgress { + m.mu.Unlock() + return fmt.Errorf("enable already in progress for this client") + } updatedConfig := *clientState.ExecutionConfig updatedConfig.Disabled = false clientState.ExecutionConfig = &updatedConfig configCopy := clientState.ExecutionConfig m.mu.Unlock() + defer m.reconnectingClients.Delete(id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/mcp/clientmanager.go` around lines 383 - 397, EnableClient has a race where two goroutines can both observe a disabled client and proceed; while still holding m.mu (in function EnableClient) reserve a single-flight slot in m.reconnectingClients (e.g., check if id is already reconciling and if not mark it) immediately after verifying clientState.State == MCPConnectionStateDisabled and before m.mu.Unlock(), returning an error if already reconciling; then release the lock and proceed with connection setup, and ensure you remove the id from m.reconnectingClients on both success and error paths so subsequent enables can proceed.
112-125:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore persisted per-user OAuth tools in disabled bootstrap path
Line 118 and Line 119 always initialize empty maps, so disabled
per_user_oauthclients lose persisted discovered tools on restart. This regresses re-enable behavior back to re-discovery.Proposed fix
// Disabled clients get a dormant placeholder — no connection, no workers. if config.Disabled { + toolMap := make(map[string]schemas.ChatTool) + toolNameMapping := make(map[string]string) + if config.AuthType == schemas.MCPAuthTypePerUserOauth { + maps.Copy(toolMap, config.DiscoveredTools) + toolNameMapping = maps.Clone(config.DiscoveredToolNameMapping) + } + m.clientMap[config.ID] = &schemas.MCPClientState{ Name: config.Name, ExecutionConfig: config, State: schemas.MCPConnectionStateDisabled, - ToolMap: make(map[string]schemas.ChatTool), - ToolNameMapping: make(map[string]string), + ToolMap: toolMap, + ToolNameMapping: toolNameMapping, ConnectionInfo: &schemas.MCPClientConnectionInfo{Type: config.ConnectionType}, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/mcp/clientmanager.go` around lines 112 - 125, Disabled-client initialization currently overwrites any persisted per-user OAuth tool maps by always setting empty ToolMap and ToolNameMapping; change the disabled bootstrap path to preserve existing maps by checking m.clientMap[config.ID] and, if present, reuse or merge its ToolMap and ToolNameMapping into the new schemas.MCPClientState instead of creating empty maps (use the existing entry's ToolMap/ToolNameMapping when config.ConnectionType indicates per-user OAuth or when those maps are non-empty); update the block that builds MCPClientState (referencing m.clientMap, config.Disabled, schemas.MCPClientState, ToolMap, ToolNameMapping, ConnectionInfo) so persisted discovered tools survive restart.transports/bifrost-http/handlers/mcp.go (2)
815-830:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollback persisted disabled flag when enable/disable lifecycle step fails
If
DisableMCPClient/EnableMCPClientfails after the DB update, this handler returns an error but leaves the persisted flag changed, creating partial-success drift between operator intent and runtime state.Proposed fix
if disabledToggled { if req.Disabled { if err := h.mcpManager.DisableMCPClient(ctx, id); err != nil { + _ = h.store.ConfigStore.UpdateMCPClientConfig(ctx, id, oldDBConfig) logger.Error(fmt.Sprintf("Failed to disable MCP client: %v", err)) SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("updated mcp config but failed to disable MCP client: %v", err)) return } } else { if err := h.mcpManager.EnableMCPClient(ctx, id); err != nil { + _ = h.store.ConfigStore.UpdateMCPClientConfig(ctx, id, oldDBConfig) logger.Error(fmt.Sprintf("Failed to enable MCP client: %v", err)) SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("updated mcp config but failed to enable MCP client: %v", err)) return } } }🤖 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 815 - 830, When DisableMCPClient/EnableMCPClient fails after you already updated the persisted disabled flag, roll back that DB change to avoid drift: detect failure inside the disabledToggled block (use disabledToggled, req.Disabled and id), attempt to revert the persisted flag via the same persistence path or manager (e.g., call the inverse update on h.mcpManager or DB to set Disabled back to !req.Disabled), log both the lifecycle error and any rollback error via logger.Error, and return the original SendError only after attempting (and logging) the rollback so the persisted state stays consistent with runtime on failure.
797-800:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake disabled updates presence-aware to avoid accidental re-enable
Line 797 and Line 800 treat
req.Disabledas always supplied. Because it is a plain bool, omitted field =>false, so unrelated edits can unintentionally re-enable a disabled client.Proposed fix
type MCPClientUpdateRequest struct { configstoreTables.TableMCPClient + Disabled *bool `json:"disabled,omitempty"` VKConfigs *[]MCPVKConfigRequest `json:"vk_configs,omitempty"` ToolsToExecute *schemas.WhiteList `json:"tools_to_execute,omitempty"` ToolsToAutoExecute *schemas.WhiteList `json:"tools_to_auto_execute,omitempty"` }+ nextDisabled := oldDBConfig.Disabled + if req.Disabled != nil { + nextDisabled = *req.Disabled + } + req.TableMCPClient.Disabled = nextDisabled + schemasConfig := &schemas.MCPClientConfig{ ... - Disabled: req.Disabled, + Disabled: nextDisabled, } - disabledToggled := existingConfig.Disabled != req.Disabled + disabledToggled := oldDBConfig.Disabled != nextDisabled🤖 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 797 - 800, The handler currently treats req.Disabled as always supplied (plain bool), which can accidentally re-enable clients; make disabled updates presence-aware by changing the request field to a pointer or adding an explicit presence flag (e.g., Disabled *bool or DisabledPresent bool), then when constructing the new config use: if req.Disabled is nil/not present then set Disabled = existingConfig.Disabled else set Disabled = *req.Disabled; compute disabledToggled only when the field is present (e.g., disabledToggled := req.Disabled != nil && existingConfig.Disabled != *req.Disabled). Update references to req.Disabled, the Disabled assignment, and disabledToggled accordingly.
🧹 Nitpick comments (1)
ui/lib/constants/config.ts (1)
98-104: ⚡ Quick winKeep the status-color map keyed to the MCP state union.
Record<string, string>drops exhaustiveness here, so a future MCP state can be added without the compiler forcing a matching color entry. Tying this map toMCPConnectionStatekeeps the new statuses aligned going forward.♻️ Suggested change
+import { MCPConnectionState } from "@/lib/types/mcp"; -export const MCP_STATUS_COLORS: Record<string, string> = { +export const MCP_STATUS_COLORS = { connected: "bg-green-100 text-green-800", error: "bg-red-100 text-red-800", disconnected: "bg-gray-100 text-gray-800", pending_tools: "bg-yellow-100 text-yellow-800", disabled: "bg-orange-100 text-orange-800", -}; +} satisfies Record<MCPConnectionState, string>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/lib/constants/config.ts` around lines 98 - 104, The MCP_STATUS_COLORS map is typed as Record<string, string>, losing exhaustiveness; change its type to be keyed by the MCPConnectionState union (e.g., Record<MCPConnectionState, string> or use the `satisfies` operator) and import/ reference the MCPConnectionState type so the compiler requires an entry for every state; update the MCP_STATUS_COLORS declaration (symbol: MCP_STATUS_COLORS) accordingly and add any missing status keys to satisfy the union.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/mcp/clientmanager.go`:
- Around line 368-369: The code mutates clientState.ExecutionConfig in place
(clientState.ExecutionConfig.Disabled = true), breaking the immutable-snapshot
pattern and risking races; instead, create a shallow copy of the
ExecutionConfig, set Disabled on that copy, and then assign the copy back to
clientState.ExecutionConfig wherever this mutation occurs (the places around
clientState.State = schemas.MCPConnectionStateDisabled and the other location
referenced near lines 395-396). Locate the clientState variable usage and
replace direct mutation with: copy := clientState.ExecutionConfig; modify
copy.Disabled; clientState.ExecutionConfig = copy so readers always see an
immutable snapshot.
- Around line 383-397: EnableClient has a race where two goroutines can both
observe a disabled client and proceed; while still holding m.mu (in function
EnableClient) reserve a single-flight slot in m.reconnectingClients (e.g., check
if id is already reconciling and if not mark it) immediately after verifying
clientState.State == MCPConnectionStateDisabled and before m.mu.Unlock(),
returning an error if already reconciling; then release the lock and proceed
with connection setup, and ensure you remove the id from m.reconnectingClients
on both success and error paths so subsequent enables can proceed.
- Around line 112-125: Disabled-client initialization currently overwrites any
persisted per-user OAuth tool maps by always setting empty ToolMap and
ToolNameMapping; change the disabled bootstrap path to preserve existing maps by
checking m.clientMap[config.ID] and, if present, reuse or merge its ToolMap and
ToolNameMapping into the new schemas.MCPClientState instead of creating empty
maps (use the existing entry's ToolMap/ToolNameMapping when
config.ConnectionType indicates per-user OAuth or when those maps are
non-empty); update the block that builds MCPClientState (referencing
m.clientMap, config.Disabled, schemas.MCPClientState, ToolMap, ToolNameMapping,
ConnectionInfo) so persisted discovered tools survive restart.
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 815-830: When DisableMCPClient/EnableMCPClient fails after you
already updated the persisted disabled flag, roll back that DB change to avoid
drift: detect failure inside the disabledToggled block (use disabledToggled,
req.Disabled and id), attempt to revert the persisted flag via the same
persistence path or manager (e.g., call the inverse update on h.mcpManager or DB
to set Disabled back to !req.Disabled), log both the lifecycle error and any
rollback error via logger.Error, and return the original SendError only after
attempting (and logging) the rollback so the persisted state stays consistent
with runtime on failure.
- Around line 797-800: The handler currently treats req.Disabled as always
supplied (plain bool), which can accidentally re-enable clients; make disabled
updates presence-aware by changing the request field to a pointer or adding an
explicit presence flag (e.g., Disabled *bool or DisabledPresent bool), then when
constructing the new config use: if req.Disabled is nil/not present then set
Disabled = existingConfig.Disabled else set Disabled = *req.Disabled; compute
disabledToggled only when the field is present (e.g., disabledToggled :=
req.Disabled != nil && existingConfig.Disabled != *req.Disabled). Update
references to req.Disabled, the Disabled assignment, and disabledToggled
accordingly.
---
Nitpick comments:
In `@ui/lib/constants/config.ts`:
- Around line 98-104: The MCP_STATUS_COLORS map is typed as Record<string,
string>, losing exhaustiveness; change its type to be keyed by the
MCPConnectionState union (e.g., Record<MCPConnectionState, string> or use the
`satisfies` operator) and import/ reference the MCPConnectionState type so the
compiler requires an entry for every state; update the MCP_STATUS_COLORS
declaration (symbol: MCP_STATUS_COLORS) accordingly and add any missing status
keys to satisfy the union.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a0dd062-bc91-45b4-9baa-0096ca8d2f86
📒 Files selected for processing (21)
core/bifrost.gocore/mcp/clientmanager.gocore/mcp/healthmonitor.gocore/mcp/interface.gocore/mcp/utils.gocore/schemas/mcp.godocs/mcp/connecting-to-servers.mdxdocs/openapi/paths/management/mcp.yamldocs/openapi/schemas/management/mcp.yamlframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/mcp.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/mcpClientsTable.tsxui/lib/constants/config.tsui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (6)
- docs/openapi/paths/management/mcp.yaml
- core/mcp/healthmonitor.go
- core/mcp/utils.go
- docs/openapi/schemas/management/mcp.yaml
- core/mcp/interface.go
- framework/configstore/tables/mcp.go
🚧 Files skipped from review as they are similar to previous changes (9)
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- core/schemas/mcp.go
- core/bifrost.go
- docs/mcp/connecting-to-servers.mdx
- ui/lib/types/schemas.ts
- ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx
- transports/bifrost-http/lib/config.go
- framework/configstore/migrations.go
- transports/bifrost-http/server/server.go
BearTS
force-pushed
the
04-27-feat_adds_option_to_disable_mcp_clients
branch
from
560f655 to
13e251a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/lib/config.go (1)
4539-4609:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRollback the full MCP client update when the toggle step fails.
transports/bifrost-http/handlers/mcp.go:769-824writes the DB before callingUpdateMCPClient. Ifc.client.DisableMCPClient/EnableMCPClientfails here, this function only restoresDisabled, butc.client.UpdateMCPClientand the in-memory field updates have already applied the rest ofupdatedConfig. The handler then restores the old DB row, leaving DB and runtime out of sync. Capture a full snapshot ofoldConfigand restore both memory and the runtime client on this failure path, or delay applying the non-toggle update until the toggle succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/config.go` around lines 4539 - 4609, The code updates the in-memory MCPConfig and calls c.client.UpdateMCPClient before toggling runtime state, but on DisableMCPClient/EnableMCPClient failure only the Disabled flag is rolled back, leaving runtime/DB inconsistent; to fix, take a full snapshot of oldConfig (the entire c.MCPConfig.ClientConfigs[configIndex]) before calling c.client.UpdateMCPClient, then when oldDisabled != updatedConfig.Disabled and clientRegistered perform the toggle (call DisableMCPClient/EnableMCPClient) first or, if you must keep the current order, on toggle failure restore the entire in-memory client entry (c.MCPConfig.ClientConfigs[configIndex] = oldConfig) and call the inverse runtime operation to return the client to its previous registered state (use c.client.EnableMCPClient / DisableMCPClient as appropriate), ensuring both memory and runtime are reverted if the toggle fails.
♻️ Duplicate comments (1)
framework/configstore/migrations.go (1)
7262-7270:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBackfill should run even when
disabledcolumn already exists.On Line 7266, the NULL→false backfill is gated by
!HasColumn. If a DB already hasdisabledwith NULL rows, this migration skips normalization and leaves invalid state for a non-pointer bool field.Suggested patch
Migrate: func(tx *gorm.DB) error { tx = tx.WithContext(ctx) mg := tx.Migrator() if !mg.HasColumn(&tables.TableMCPClient{}, "disabled") { if err := mg.AddColumn(&tables.TableMCPClient{}, "disabled"); err != nil { return fmt.Errorf("failed to add disabled column: %w", err) } - // Initialize existing rows with false (default value) - if err := tx.Exec("UPDATE config_mcp_clients SET disabled = false WHERE disabled IS NULL").Error; err != nil { - return fmt.Errorf("failed to initialize disabled column: %w", err) - } + } + // Normalize legacy/drifted rows with NULL disabled values + if err := tx.Exec("UPDATE config_mcp_clients SET disabled = false WHERE disabled IS NULL").Error; err != nil { + return fmt.Errorf("failed to initialize disabled column: %w", err) } return nil },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 7262 - 7270, The backfill that sets NULL→false for the disabled column is currently only executed when mg.AddColumn runs; change the logic so the UPDATE runs whenever the disabled column exists (whether newly added or pre-existing). Concretely, keep the AddColumn call guarded by if !mg.HasColumn(&tables.TableMCPClient{}, "disabled") { mg.AddColumn(...) }, but move or add a subsequent check (e.g., if mg.HasColumn(&tables.TableMCPClient{}, "disabled") { if err := tx.Exec("UPDATE config_mcp_clients SET disabled = false WHERE disabled IS NULL").Error; err != nil { return fmt.Errorf("failed to initialize disabled column: %w", err) } }) so the NULL normalization always runs when the column is present; reference mg.HasColumn, mg.AddColumn, tx.Exec and the config_mcp_clients table in the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/lib/store/apis/mcpApi.ts`:
- Line 79: The optimistic cache patch currently only updates
draft.clients[index].config.disabled; extend the same
onQueryStarted/updateQueryData optimistic update to also mutate
draft.clients[index].state to match the new disabled value (e.g., mark the
client as disconnected when disabled=true and connected when disabled=false, and
clear or adjust any connection/tool-count fields that indicate an active
connection) so the UI shows consistent Enabled/State immediately after toggling.
---
Outside diff comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 4539-4609: The code updates the in-memory MCPConfig and calls
c.client.UpdateMCPClient before toggling runtime state, but on
DisableMCPClient/EnableMCPClient failure only the Disabled flag is rolled back,
leaving runtime/DB inconsistent; to fix, take a full snapshot of oldConfig (the
entire c.MCPConfig.ClientConfigs[configIndex]) before calling
c.client.UpdateMCPClient, then when oldDisabled != updatedConfig.Disabled and
clientRegistered perform the toggle (call DisableMCPClient/EnableMCPClient)
first or, if you must keep the current order, on toggle failure restore the
entire in-memory client entry (c.MCPConfig.ClientConfigs[configIndex] =
oldConfig) and call the inverse runtime operation to return the client to its
previous registered state (use c.client.EnableMCPClient / DisableMCPClient as
appropriate), ensuring both memory and runtime are reverted if the toggle fails.
---
Duplicate comments:
In `@framework/configstore/migrations.go`:
- Around line 7262-7270: The backfill that sets NULL→false for the disabled
column is currently only executed when mg.AddColumn runs; change the logic so
the UPDATE runs whenever the disabled column exists (whether newly added or
pre-existing). Concretely, keep the AddColumn call guarded by if
!mg.HasColumn(&tables.TableMCPClient{}, "disabled") { mg.AddColumn(...) }, but
move or add a subsequent check (e.g., if mg.HasColumn(&tables.TableMCPClient{},
"disabled") { if err := tx.Exec("UPDATE config_mcp_clients SET disabled = false
WHERE disabled IS NULL").Error; err != nil { return fmt.Errorf("failed to
initialize disabled column: %w", err) } }) so the NULL normalization always runs
when the column is present; reference mg.HasColumn, mg.AddColumn, tx.Exec and
the config_mcp_clients table in the fix.
🪄 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: a90ca85e-112c-4782-9b2a-c6d38e957be2
📒 Files selected for processing (22)
core/bifrost.gocore/mcp/clientmanager.gocore/mcp/healthmonitor.gocore/mcp/interface.gocore/mcp/utils.gocore/schemas/mcp.godocs/mcp/connecting-to-servers.mdxdocs/openapi/paths/management/mcp.yamldocs/openapi/schemas/management/mcp.yamlframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/mcp.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/mcpClientsTable.tsxui/lib/constants/config.tsui/lib/store/apis/mcpApi.tsui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (9)
- ui/lib/constants/config.ts
- docs/openapi/paths/management/mcp.yaml
- core/schemas/mcp.go
- ui/lib/types/schemas.ts
- framework/configstore/tables/mcp.go
- docs/openapi/schemas/management/mcp.yaml
- framework/configstore/clientconfig.go
- transports/bifrost-http/server/server.go
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- core/mcp/utils.go
- docs/mcp/connecting-to-servers.mdx
- core/mcp/healthmonitor.go
- framework/configstore/rdb.go
- core/bifrost.go
- core/mcp/clientmanager.go
- transports/bifrost-http/handlers/mcp.go
BearTS
force-pushed
the
04-27-feat_adds_option_to_disable_mcp_clients
branch
from
13e251a to
511bd47
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/clientmanager.go`:
- Around line 345-383: In DisableClient, special-case the internal local host
key (BifrostMCPClientKey) so the manager cannot disable its own in-process
client; locate the DisableClient method and add an early check for id ==
BifrostMCPClientKey (the key created by setupLocalHost) and return an error or
no-op instead of proceeding, ensuring you do not clear the ToolMap, call
CancelFunc/Conn.Close, or set ExecutionConfig.Disabled for that client; this
preserves the local placeholder config so EnableClient can rebuild tools later.
- Around line 437-453: The success path after connectToMCPClient can race with
DisableClient and re-enable a disabled entry; after a successful connect,
acquire m.mu, look up m.clientMap[id] and verify cs.State is not
schemas.MCPConnectionStateDisabled (or whatever disabled enum you use) before
setting cs.State to Connected and starting health monitoring/syncing
(NewClientHealthMonitor / healthMonitorManager.StartMonitoring). If the state is
Disabled, cleanly close/rollback the newly established client and return without
restarting monitors. Alternatively, wire a cancellable context from
DisableClient into connectToMCPClient and abort the completion if
canceled—ensure the check uses the clientMap entry (id) and the State field to
decide finalization.
In `@transports/bifrost-http/lib/config.go`:
- Around line 4658-4659: The code dereferences c.ConfigStore in the MCP toggle
APIs (calls like c.ConfigStore.GetMCPClientByID and the call around lines
4692-4693) without guarding for nil; add a nil-check on c.ConfigStore at the
start of each affected handler (e.g., before using GetMCPClientByID and the
other ConfigStore call) and return a handled error (HTTP 503 or a clear internal
error) when ConfigStore is nil instead of proceeding, so the code never panics
when the config store is disabled/unavailable.
🪄 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: 2702334e-b895-499f-af7d-4c5a96a6c13e
📒 Files selected for processing (22)
core/bifrost.gocore/mcp/clientmanager.gocore/mcp/healthmonitor.gocore/mcp/interface.gocore/mcp/utils.gocore/schemas/mcp.godocs/mcp/connecting-to-servers.mdxdocs/openapi/paths/management/mcp.yamldocs/openapi/schemas/management/mcp.yamlframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/mcp.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/mcpClientsTable.tsxui/lib/constants/config.tsui/lib/store/apis/mcpApi.tsui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (9)
- core/mcp/interface.go
- ui/lib/types/schemas.ts
- docs/openapi/paths/management/mcp.yaml
- ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx
- framework/configstore/tables/mcp.go
- framework/configstore/migrations.go
- transports/bifrost-http/handlers/mcp.go
- core/bifrost.go
- docs/mcp/connecting-to-servers.mdx
🚧 Files skipped from review as they are similar to previous changes (9)
- ui/lib/store/apis/mcpApi.ts
- core/schemas/mcp.go
- framework/configstore/clientconfig.go
- ui/lib/constants/config.ts
- core/mcp/healthmonitor.go
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- framework/configstore/rdb.go
- docs/openapi/schemas/management/mcp.yaml
- core/mcp/utils.go
BearTS
force-pushed
the
04-27-feat_adds_option_to_disable_mcp_clients
branch
from
511bd47 to
441ae61
Compare
BearTS
changed the title
disabled field to MCP clients for toggling connection and workers without removal
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/clientmanager.go`:
- Around line 364-387: DisableClient() can race with async writers
(healthmonitor.updateClientState and the SSE OnConnectionLost callback) which
still set Disconnected/Error after StopMonitoring; make those writers no-op when
the client is disabled by checking clientState.ExecutionConfig.Disabled or
clientState.State == schemas.MCPConnectionStateDisabled at their start. Update
core/mcp/healthmonitor.go:updateClientState() and the SSE OnConnectionLost
handler in clientmanager.go to acquire the same clientState mutex (or add one to
the clientState struct if missing), return immediately if
ExecutionConfig.Disabled is true or State == MCPConnectionStateDisabled, and
ensure DisableClient() sets ExecutionConfig.Disabled and State to
MCPConnectionStateDisabled under that same mutex before calling
StopMonitoring/Close so no race can flip state back.
In `@framework/configstore/clientconfig.go`:
- Line 1243: GenerateMCPClientHash currently includes the DB-managed runtime
field m.Disabled in the computed hash which causes reconciles to overwrite the
DB toggle; remove any use of m.Disabled from GenerateMCPClientHash so the
file-derived hash only reflects config.json fields, and ensure
UpdateMCPClientConfig logic continues to compare the file hash to the stored
hash (without Disabled) so toggling m.Disabled in the DB is not reverted by an
unchanged file version.
🪄 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: f889c4fb-8c12-495c-896a-7b81a105e45d
📒 Files selected for processing (22)
core/bifrost.gocore/mcp/clientmanager.gocore/mcp/healthmonitor.gocore/mcp/interface.gocore/mcp/utils.gocore/schemas/mcp.godocs/mcp/connecting-to-servers.mdxdocs/openapi/paths/management/mcp.yamldocs/openapi/schemas/management/mcp.yamlframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/mcp.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/mcpClientsTable.tsxui/lib/constants/config.tsui/lib/store/apis/mcpApi.tsui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (7)
- core/mcp/interface.go
- docs/openapi/paths/management/mcp.yaml
- core/mcp/utils.go
- ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx
- docs/openapi/schemas/management/mcp.yaml
- framework/configstore/tables/mcp.go
- docs/mcp/connecting-to-servers.mdx
🚧 Files skipped from review as they are similar to previous changes (9)
- ui/lib/types/schemas.ts
- ui/lib/store/apis/mcpApi.ts
- ui/lib/constants/config.ts
- framework/configstore/rdb.go
- core/bifrost.go
- framework/configstore/migrations.go
- transports/bifrost-http/handlers/mcp.go
- core/mcp/healthmonitor.go
- transports/bifrost-http/lib/config.go
BearTS
force-pushed
the
04-27-feat_adds_option_to_disable_mcp_clients
branch
from
441ae61 to
55c08f9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/configstore/rdb.go`:
- Line 1534: The update map writes the wrong value for the "disabled" key (it
currently assigns clientConfigCopy.ConfigHash); change it to use the boolean
flag clientConfigCopy.Disabled (or the appropriate boolean field on
clientConfigCopy) so the "disabled" entry stores the correct boolean state
instead of the ConfigHash string; locate the map construction around the use of
clientConfigCopy and update that key assignment to use clientConfigCopy.Disabled
(or convert the field to a bool if necessary) so MCP client updates persist
correct state.
🪄 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: 1a0a783c-eaf8-4284-8600-05d4a30bfdaa
📒 Files selected for processing (23)
core/bifrost.gocore/mcp/clientmanager.gocore/mcp/healthmonitor.gocore/mcp/interface.gocore/mcp/utils.gocore/schemas/mcp.godocs/mcp/connecting-to-servers.mdxdocs/openapi/paths/management/mcp.yamldocs/openapi/schemas/management/mcp.yamlframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/mcp.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/mcpClientsTable.tsxui/lib/constants/config.tsui/lib/store/apis/mcpApi.tsui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (8)
- docs/mcp/connecting-to-servers.mdx
- docs/openapi/paths/management/mcp.yaml
- ui/lib/constants/config.ts
- framework/configstore/tables/mcp.go
- core/schemas/mcp.go
- core/bifrost.go
- core/mcp/utils.go
- docs/openapi/schemas/management/mcp.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- transports/bifrost-http/handlers/mcp.go
- framework/configstore/migrations.go
- ui/lib/store/apis/mcpApi.ts
- ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx
- core/mcp/clientmanager.go
- transports/bifrost-http/lib/config.go
BearTS
force-pushed
the
04-27-feat_adds_option_to_disable_mcp_clients
branch
from
55c08f9 to
9541a9e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/mcp/connecting-to-servers.mdx`:
- Line 933: The docs contain a broken image reference named
placeholder-mcp-disable-toggle.png; locate the image markup in the
connecting-to-servers MDX and either replace placeholder-mcp-disable-toggle.png
with the correct existing asset path or remove the image markup entirely until
the asset is added, then re-run the docs link/validation to confirm the
broken-links check passes.
🪄 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: 7286b6a9-a21f-4410-b334-57300b21abb3
📒 Files selected for processing (23)
core/bifrost.gocore/mcp/clientmanager.gocore/mcp/healthmonitor.gocore/mcp/interface.gocore/mcp/utils.gocore/schemas/mcp.godocs/mcp/connecting-to-servers.mdxdocs/openapi/paths/management/mcp.yamldocs/openapi/schemas/management/mcp.yamlframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/mcp.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/mcpClientsTable.tsxui/lib/constants/config.tsui/lib/store/apis/mcpApi.tsui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (6)
- docs/openapi/paths/management/mcp.yaml
- ui/lib/constants/config.ts
- core/schemas/mcp.go
- core/mcp/healthmonitor.go
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- transports/bifrost-http/lib/config.go
🚧 Files skipped from review as they are similar to previous changes (8)
- ui/lib/types/mcp.ts
- core/mcp/interface.go
- ui/lib/store/apis/mcpApi.ts
- framework/configstore/migrations.go
- transports/bifrost-http/handlers/mcp.go
- ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx
- core/mcp/clientmanager.go
- framework/configstore/rdb.go
BearTS
force-pushed
the
04-27-feat_adds_option_to_disable_mcp_clients
branch
from
9541a9e to
42ab84b
Compare
BearTS
force-pushed
the
04-27-feat_adds_option_to_disable_mcp_clients
branch
from
42ab84b to
9f6de2d
Compare
18 tasks
akshaydeo
approved these changes
May 5, 2026
Contributor
Merge activity
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the ability to temporarily disable an MCP client without removing it. When disabled, the client's connection, health monitor, and tool syncer are shut down, and its tools become invisible to inference requests. The client entry is preserved in a
disabledstate and can be re-enabled at any time, automatically reconnecting and restarting all workers.Changes
MCPConnectionStateDisabledconnection state and aDisabledboolean field toMCPClientConfigand the database table (config_mcp_clients)DisableClientandEnableClientonMCPManager: disable tears down the connection, cancels the context, stops the health monitor and tool syncer, and clears the tool map (preserving it for per-user OAuth clients); enable reconnects and restarts workers, falling back todisconnected+ health monitor recovery if the initial connection failsDisableMCPClient/EnableMCPClientto theBifrostcore,MCPManagerInterface, HTTP handler, server callbacks, andConfiglayer, with DB persistence on each toggleupdateMCPClientHTTP handler detects adisabledfield change and calls disable/enable after applying all other config changes, so name/tool/header updates are always applied firstGetToolPerClientskips disabled clients so their tools are never surfaced to inferenceadd_mcp_client_disabled_column) to backfill the new columnconfig.jsonon boot are registered as dormant placeholders with no connection or workersconnectToMCPClientdetects if a client was disabled during the async connection setup and rolls back the newly established connection rather than overwriting the disabled stateMCPConnectionStateandMCP_STATUS_COLORSto includepending_toolsanddisabled, and wireddisabledthrough the update request schema and form statePUTexamples, and a note thatconfig.jsondoes not support thedisabledfield at load time; OpenAPI schema updated to includepending_toolsanddisabledinMCPConnectionStateanddisabledinMCPClientUpdateRequestandMCPClientConfigType of change
Affected areas
How to test
PUT /api/mcp/client/{id}with{"disabled": true}and confirm the response showsstate: disabled.PUT /api/mcp/client/{id}with{"disabled": false}and confirm the client reconnects and tools are available again.Breaking changes
Related issues
Security considerations
No new auth surfaces introduced. The
disabledfield is a runtime operational flag with no credential or PII exposure.Checklist
docs/contributing/README.mdand followed the guidelines