Conversation
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds support for resolving MCP client names from config files to MCP client IDs before persisting virtual-key MCP configs, plus example JSON configs, tests, and a script config entry to exercise the new workflow. Changes
Sequence DiagramsequenceDiagram
participant CF as Config File
participant Parser as Config Parser
participant Resolver as Name Resolver
participant Store as ConfigStore
participant DB as PostgreSQL
CF->>Parser: load config.json (virtual_keys + mcp_configs with mcp_client_name)
Parser->>Resolver: resolveMCPConfigClientIDs(mcp_configs, vk_id)
Resolver->>Store: query MCP clients by name
Store->>DB: SELECT id FROM config_mcp_clients WHERE name IN (...)
DB-->>Store: return matching ids
Store-->>Resolver: matched ids
Resolver->>Resolver: map names -> ids, drop unresolved (warn)
Resolver-->>Parser: return filtered mcp_configs with mcp_client_id
Parser->>Store: persist virtual key and governance_virtual_key_mcp_configs
Store->>DB: INSERT governance_virtual_key_mcp_configs (...) (FK satisfied)
DB-->>Store: success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
This was referenced Jan 29, 2026
Closed
Closed
Closed
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
akshaydeo
force-pushed
the
01-29-fixes_race_condition_while_loading_mcp_servers_from_config_file
branch
2 times, most recently
from
9f543d0 to
d46d034
Compare
akshaydeo
force-pushed
the
01-19-feat_plugin_schemas_refactor_and_added_mcp_plugins
branch
from
10fddc2 to
3be2a0e
Compare
16 tasks
![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: 2
🤖 Fix all issues with AI agents
In @.github/workflows/scripts/release-bifrost-http.sh:
- Around line 154-155: The test config named withpostgresmcpclientsinconfig
references MCP HTTP endpoints (WeatherService at localhost:8080/mcp and
CalendarService at localhost:8081/mcp) that are not provided by the compose
setup and cause Bifrost to fail on init; fix this by either adding lightweight
MCP mock servers to your docker-compose so those endpoints exist, or edit the
withpostgresmcpclientsinconfig test config to set those MCP clients' is_enabled
to false (or mark them optional/fault-tolerant) so Bifrost won’t attempt
mandatory connections to localhost:8080/mcp and localhost:8081/mcp during
integration tests.
In `@transports/bifrost-http/lib/config_test.go`:
- Around line 9927-9997: The inline JSON uses tempDir unescaped and a hardcoded
"/" path separator which breaks on Windows; change to build paths with
filepath.Join (e.g., dbPath := filepath.Join(tempDir, "config.db") and cfgPath
:= filepath.Join(tempDir, "config.json")) and inject the DB path into configJSON
using a JSON-quoted %q (e.g., fmt.Sprintf(..., dbPath, keyID)) so backslashes
are escaped; finally use os.WriteFile(cfgPath, []byte(configJSON), 0644) to
write the file. Ensure you update the other similar test block the same way.
🧹 Nitpick comments (2)
transports/bifrost-http/lib/config.go (1)
1983-2033: Cache MCP client lookups to avoid repeated DB hits.If multiple MCP configs reference the same client name, a small cache prevents duplicate
GetMCPClientByNamecalls.♻️ Suggested cache to avoid duplicate lookups
func resolveMCPConfigClientIDs( ctx context.Context, store configstore.ConfigStore, mcpConfigs []configstoreTables.TableVirtualKeyMCPConfig, virtualKeyID string, ) []configstoreTables.TableVirtualKeyMCPConfig { if store == nil || len(mcpConfigs) == 0 { return mcpConfigs } resolvedConfigs := make([]configstoreTables.TableVirtualKeyMCPConfig, 0, len(mcpConfigs)) + resolvedByName := make(map[string]*configstoreTables.TableMCPClient) for i := range mcpConfigs { mc := &mcpConfigs[i] // If MCPClientID is already set (e.g., from database or direct construction), keep it if mc.MCPClientID != 0 { resolvedConfigs = append(resolvedConfigs, *mc) continue } // If MCPClientName is set (from config.json parsing), resolve it to MCPClientID if mc.MCPClientName != "" { + if cached, ok := resolvedByName[mc.MCPClientName]; ok { + if cached != nil { + mc.MCPClientID = cached.ID + resolvedConfigs = append(resolvedConfigs, *mc) + } + continue + } mcpClient, err := store.GetMCPClientByName(ctx, mc.MCPClientName) if err != nil { logger.Warn("virtual key %s: failed to resolve MCP client '%s': %v (skipping this MCP config)", virtualKeyID, mc.MCPClientName, err) + resolvedByName[mc.MCPClientName] = nil continue } if mcpClient == nil { logger.Warn("virtual key %s: MCP client '%s' not found (skipping this MCP config)", virtualKeyID, mc.MCPClientName) + resolvedByName[mc.MCPClientName] = nil continue } + resolvedByName[mc.MCPClientName] = mcpClient mc.MCPClientID = mcpClient.ID resolvedConfigs = append(resolvedConfigs, *mc) continue }examples/configs/withpostgresmcpclientsinconfig/config.json (1)
1-139: Consider env placeholders for Postgres passwords in the example.This encourages safer copy‑paste defaults for users consuming the example config.
💡 Example tweak
- "password": "bifrost_password", + "password": "env.BIFROST_DB_PASSWORD",- "password": "bifrost_password", + "password": "env.BIFROST_DB_PASSWORD",
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@transports/bifrost-http/lib/config_test.go`:
- Around line 9925-9997: The JSON string interpolates raw tempDir which on
Windows can include backslashes that form invalid JSON escape sequences; fix by
building filesystem paths with filepath.Join (e.g., path :=
filepath.Join(tempDir, "config.db") and cfgPath := filepath.Join(tempDir,
"config.json")) and inject them into configJSON using a quoted format (use %q or
strconv.Quote) so the path is properly escaped in the JSON string, and use
cfgPath in the os.WriteFile call instead of tempDir+"/config.json"; update the
code sections that set configJSON and write the file (variables: configJSON,
tempDir, and the os.WriteFile call).
In `@transports/bifrost-http/lib/config.go`:
- Around line 1352-1354: The comment incorrectly claims the
resolveMCPConfigClientIDs call is "done outside the transaction setup" even
though it is invoked inside the ExecuteTransaction callback; update the comment
near the resolveMCPConfigClientIDs(ctx, config.ConfigStore, mcpConfigs,
virtualKey.ID) call to accurately state that the resolution runs within the
transaction context (or simply remove the "outside the transaction" phrase), and
keep mention that it only performs reads against config.ConfigStore and operates
on mcpConfigs and virtualKey.ID.
🧹 Nitpick comments (1)
.github/workflows/configs/withpostgresmcpclientsinconfig/config.json (1)
21-28: Hardcoded database credentials in workflow config.The password
bifrost_passwordis hardcoded. While this is acceptable for CI test configurations, consider using environment variable references (e.g.,"password": "env.POSTGRES_PASSWORD") for consistency with other sensitive values in this file that already use theenv.pattern (likeenv.BIFROST_ADMIN_PASSWORD).♻️ Optional: Use environment variables for database credentials
"config_store": { "enabled": true, "type": "postgres", "config": { "host": "localhost", "port": "5432", "user": "bifrost", - "password": "bifrost_password", + "password": "env.POSTGRES_PASSWORD", "db_name": "bifrost", "ssl_mode": "disable" } }, "logs_store": { "enabled": true, "type": "postgres", "config": { "host": "localhost", "port": "5432", "user": "bifrost", - "password": "bifrost_password", + "password": "env.POSTGRES_PASSWORD", "db_name": "bifrost", "ssl_mode": "disable" } },Also applies to: 33-40
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
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)
1048-1116: Config hash computed before MCPClientName resolution; mcp_client_name-only changes undetected.
GenerateVirtualKeyHashis called at line 1050 withMCPClientID = 0(from JSON unmarshaling), butresolveMCPConfigClientIDsis invoked later at lines 1084-1086 and 1114-1116. Since the hash includes onlyMCPClientIDandToolsToExecuteinVirtualKeyMCPConfigHashInput, a change tomcp_client_namealone will produce the same hash (both resolve fromMCPClientID = 0). This prevents detection and synchronization of name-only changes from config.json.Resolve MCP client names before computing the hash, or update
GenerateVirtualKeyHashto includeMCPClientNamewhenMCPClientIDis zero.
🧹 Nitpick comments (1)
examples/configs/withpostgresmcpclientsinconfig/config.json (1)
18-39: Consider env-backed Postgres passwords in examples.Even for sample configs, using
env.*for the password reduces the chance of users copying plaintext secrets into production configs.Suggested change
- "password": "bifrost_password", + "password": "env.BIFROST_POSTGRES_PASSWORD", ... - "password": "bifrost_password", + "password": "env.BIFROST_POSTGRES_PASSWORD",
akshaydeo
force-pushed
the
01-19-feat_plugin_schemas_refactor_and_added_mcp_plugins
branch
from
3be2a0e to
95e2529
Compare
akshaydeo
force-pushed
the
01-29-fixes_race_condition_while_loading_mcp_servers_from_config_file
branch
from
d46d034 to
8f6f420
Compare
Base automatically changed from
01-19-feat_plugin_schemas_refactor_and_added_mcp_plugins
to
main
January 29, 2026 13:32
Contributor
Author
Merge activity
|
akshaydeo
deleted the
01-29-fixes_race_condition_while_loading_mcp_servers_from_config_file
branch
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
1 participant
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.
Support for MCP Client Names in Virtual Key Configurations
This PR adds support for referencing MCP clients by name in virtual key configurations, making it easier to define MCP access permissions in config files.
Issue
Closes #1462
Changes
mcp_client_namein virtual key MCP configurations, allowing config files to reference MCP clients by their human-readable names instead of database IDsType of change
Affected areas
How to test
Breaking changes
Related issues
Fixes an issue where virtual keys with MCP configurations in config files would fail with foreign key constraint violations when using client names instead of IDs.
Security considerations
No security implications - this is a usability improvement that maintains the same access control model.
Checklist