feat: add tool-level binding option for MCP code mode #1160
Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis PR introduces Code Mode Binding Level to control how MCP tools are exposed in virtual file systems. The feature propagates through schemas, configuration storage, the tool manager, VFS file handling logic, HTTP API endpoints, and UI components, allowing tools to be organized at either server-level or tool-level granularity. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as MCP Gateway UI
participant HTTP as HTTP Handler
participant Bifrost as Bifrost Core
participant ToolMgr as Tool Manager
participant VFS as VFS Tools
User->>UI: Select binding level (server/tool)
UI->>HTTP: POST /config with mcp_code_mode_binding_level
HTTP->>HTTP: Validate binding level ∈ {server, tool}
HTTP->>Bifrost: UpdateToolManagerConfig(..., codeModeBindingLevel)
Bifrost->>ToolMgr: UpdateConfig with CodeModeBindingLevel
ToolMgr->>ToolMgr: Store atomically, log update
ToolMgr->>VFS: GetCodeModeBindingLevel() on next tool invocation
Note over VFS: VFS tools now organize files<br/>based on binding level:<br/>- server: /servers/X.d.ts<br/>- tool: /servers/X/tools/Y.d.ts
UI->>User: Config updated, VFS layout changed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
ui/lib/types/config.ts (1)
328-328: Tighten CoreConfig type to a literal union instead of plain stringBackend and UI both treat this as
"server"or"tool"only; usingstringmisses easy type safety and autocomplete.Consider introducing a dedicated union type and using it here:
Proposed type-level refinement
+export type MCPCodeModeBindingLevel = "server" | "tool"; + // Core Bifrost configuration types export interface CoreConfig { @@ - mcp_tool_execution_timeout: number; - mcp_code_mode_binding_level?: string; + mcp_tool_execution_timeout: number; + mcp_code_mode_binding_level?: MCPCodeModeBindingLevel; }transports/bifrost-http/lib/config.go (1)
217-232: Consider normalizing empty MCPCodeModeBindingLevel to the “server” defaultThe default client config sets
MCPCodeModeBindingLevelto"server", and the merge-from-file path copies a non-empty value from the file when the DB field is empty. There’s still a narrow case where both DB and file have this field empty, leaving the in-memory value as""instead of the intended default.For extra robustness (and to decouple behavior from migration guarantees), you could normalize after merging:
Suggested normalization after merge
func loadClientConfigFromFile(ctx context.Context, config *Config, configData *ConfigData) { @@ if config.ClientConfig.MCPToolExecutionTimeout == 0 && configData.Client.MCPToolExecutionTimeout != 0 { config.ClientConfig.MCPToolExecutionTimeout = configData.Client.MCPToolExecutionTimeout } - if config.ClientConfig.MCPCodeModeBindingLevel == "" && configData.Client.MCPCodeModeBindingLevel != "" { - config.ClientConfig.MCPCodeModeBindingLevel = configData.Client.MCPCodeModeBindingLevel - } + if config.ClientConfig.MCPCodeModeBindingLevel == "" && configData.Client.MCPCodeModeBindingLevel != "" { + config.ClientConfig.MCPCodeModeBindingLevel = configData.Client.MCPCodeModeBindingLevel + } + // Ensure a sane default if still empty after merging + if config.ClientConfig.MCPCodeModeBindingLevel == "" { + config.ClientConfig.MCPCodeModeBindingLevel = string(schemas.CodeModeBindingLevelServer) + }(You could also centralize this invariant anywhere you finalize
ClientConfig.)Also applies to: 508-510
ui/components/sidebar.tsx (1)
372-377: Clarify UX for the two “MCP Gateway” entries and consider de‑duplicating the iconYou now have:
- A top-level item:
title: "MCP Gateway", url: "/workspace/mcp-gateway".- A Config sub-item:
title: "MCP Gateway", url: "/workspace/config/mcp-gateway".Both use
hasMCPGatewayAccessand similar descriptions (“MCP configuration/gateway configuration”). If these represent different surfaces (e.g., runtime status vs. config), consider differentiating titles/descriptions, or dropping one if they’re effectively the same destination; otherwise it may be confusing in the sidebar.Separately, this file defines a local
MCPIconSVG whileui/components/ui/icons.tsxalso exportsMCPIconwith the same path data. Importing and reusing the shared icon instead of duplicating it here would reduce maintenance overhead.Also applies to: 489-495
framework/configstore/rdb.go (1)
30-55: MCPCodeModeBindingLevel wiring looks correct; optional explicit default in fallback branchPersisting
MCPCodeModeBindingLevelthroughUpdateClientConfig,GetClientConfig, and intoMCPToolManagerConfig.CodeModeBindingLevelinGetMCPConfigis consistent and keeps the new knob fully round-tripped.In the rare
ErrRecordNotFoundbranch inGetMCPConfig, you currently omitCodeModeBindingLevelfrom the defaultMCPToolManagerConfig. This is functionally fine becauseNewToolsManageralready normalizes an empty value toCodeModeBindingLevelServer, but if you want the config object itself to be self-describing, you could also set the field there for clarity.Also applies to: 180-205, 706-773
core/bifrost.go (1)
1887-1900: Good propagation of binding level; consider typing parameter asschemas.CodeModeBindingLevelThe new
codeModeBindingLevelparameter is correctly threaded intoMCPToolManagerConfigand will be applied atomically via the MCP manager.Since this is a public method that may be called by Go consumers directly (bypassing HTTP validation), consider changing the third argument’s type to
schemas.CodeModeBindingLevelto enforce the"server"/"tool"enum at compile time and avoid accidental invalid strings.core/mcp/codemode_listfiles.go (2)
13-39: Dynamic description is correct but heavily duplicated across branchesConditioning the tool description on
CodeModeBindingLevelis nice and keeps the behavior self‑documenting. The two branches share a lot of identical copy, though, which will be tedious to keep in sync.Consider factoring the shared paragraphs into a common base string and only varying the sentences that differ between server‑ and tool‑level modes.
77-118: File listing and VFS tree construction look correct; minor UX and style tweaks possibleThe binding‑level split in
handleListToolFilesand thebuildVFSTree/renderTreeNodehelpers produce the expected hierarchies for both:
servers/<server>.d.ts(server level),servers/<server>/<tool>.d.ts(tool level),with stable lexicographic ordering and sensible indentation.
Two small improvements you might consider:
Empty
filesedge case
IfcodeModeServerCount > 0but, for whatever reason, no tool files are derived (e.g., a misconfigured client exposing zero tools),buildVFSTreereturns"", and the user gets an empty response body. Checkinglen(files) == 0inhandleListToolFilesand returning a short explanatory message would make this failure mode clearer.Use
sort.Stringsinstead of manual bubble sort
InrenderTreeNode, replacing the custom nested loops withsort.Strings(keys)(and importingsort) would be more idiomatic and simpler to read.Also applies to: 144-229
core/mcp/toolmanager.go (1)
467-484: Config updates and accessor forCodeModeBindingLevelbehave correctly; logging is slightly optimistic
UpdateConfigcorrectly treats non‑zero timeout/depth and non‑emptyCodeModeBindingLevelas updates, andGetCodeModeBindingLevelsafely falls back toCodeModeBindingLevelServerwhen the atomic value hasn’t been initialized.One minor nuance: the log line in
UpdateConfigprints the fields from the passedconfigstruct, which may deliberately omit some values (e.g., 0 timeout used to mean “no change”), so the message can under‑report the actual effective configuration. If you care about absolute accuracy here, consider logging the values read back from the atomics instead.Functionally, though, the update and read paths are correct.
Also applies to: 486-494
transports/bifrost-http/server/server.go (1)
735-740: Consider adding validation for the codeModeBindingLevel parameter.The
codeModeBindingLevelparameter is accepted as a plain string without validation. According to the PR description, valid values are"server"and"tool". Consider adding validation to reject invalid values early, or use a typed constant fromcore/schemasto ensure type safety.🔎 Suggested validation approach
func (s *BifrostHTTPServer) UpdateMCPToolManagerConfig(ctx context.Context, maxAgentDepth int, toolExecutionTimeoutInSeconds int, codeModeBindingLevel string) error { if s.Config == nil { return fmt.Errorf("config not found") } + // Validate binding level + if codeModeBindingLevel != string(schemas.CodeModeBindingLevelServer) && codeModeBindingLevel != string(schemas.CodeModeBindingLevelTool) { + return fmt.Errorf("invalid codeModeBindingLevel: %s (must be 'server' or 'tool')", codeModeBindingLevel) + } return s.Client.UpdateToolManagerConfig(maxAgentDepth, toolExecutionTimeoutInSeconds, codeModeBindingLevel) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
core/bifrost.gocore/mcp.gocore/mcp/codemode_listfiles.gocore/mcp/codemode_readfile.gocore/mcp/toolmanager.gocore/schemas/mcp.goframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goframework/configstore/tables/mcp.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/go.modui/app/workspace/config/logging/page.tsxui/app/workspace/config/mcp-gateway/page.tsxui/app/workspace/config/views/clientSettingsView.tsxui/app/workspace/config/views/governanceView.tsxui/app/workspace/config/views/loggingView.tsxui/app/workspace/config/views/mcpView.tsxui/app/workspace/config/views/observabilityView.tsxui/app/workspace/config/views/performanceTuningView.tsxui/app/workspace/config/views/securityView.tsxui/components/sidebar.tsxui/lib/types/config.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (1)
- framework/configstore/tables/mcp.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/lib/types/config.tsui/app/workspace/config/views/loggingView.tsxui/lib/types/schemas.tsframework/configstore/clientconfig.gocore/bifrost.goui/app/workspace/config/views/securityView.tsxcore/schemas/mcp.gocore/mcp/codemode_listfiles.goui/app/workspace/config/views/observabilityView.tsxcore/mcp.goframework/configstore/tables/clientconfig.gotransports/bifrost-http/server/server.gocore/mcp/codemode_readfile.goframework/configstore/migrations.goui/app/workspace/config/mcp-gateway/page.tsxtransports/bifrost-http/handlers/config.goui/app/workspace/config/views/performanceTuningView.tsxui/app/workspace/config/views/mcpView.tsxui/components/sidebar.tsxui/app/workspace/config/logging/page.tsxtransports/bifrost-http/lib/config.gotransports/go.modui/app/workspace/config/views/governanceView.tsxframework/configstore/rdb.goui/app/workspace/config/views/clientSettingsView.tsxcore/mcp/toolmanager.go
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
framework/configstore/clientconfig.gocore/bifrost.gocore/schemas/mcp.gocore/mcp/codemode_listfiles.gocore/mcp.goframework/configstore/tables/clientconfig.gotransports/bifrost-http/server/server.gocore/mcp/codemode_readfile.goframework/configstore/migrations.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.goframework/configstore/rdb.gocore/mcp/toolmanager.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/server/server.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.go
🧬 Code graph analysis (11)
core/bifrost.go (1)
core/schemas/mcp.go (2)
MCPToolManagerConfig(25-29)CodeModeBindingLevel(37-37)
core/mcp/codemode_listfiles.go (4)
core/mcp/toolmanager.go (2)
ToolsManager(22-34)ToolTypeListToolFiles(37-37)core/schemas/chatcompletions.go (2)
ChatTool(252-257)ChatToolFunction(260-265)core/schemas/mcp.go (1)
CodeModeBindingLevelServer(40-40)core/schemas/utils.go (1)
Ptr(16-18)
core/mcp/codemode_readfile.go (5)
core/mcp/toolmanager.go (1)
ToolsManager(22-34)core/schemas/chatcompletions.go (2)
ChatTool(252-257)OrderedMap(277-277)core/schemas/mcp.go (1)
CodeModeBindingLevelServer(40-40)core/schemas/utils.go (1)
Ptr(16-18)ui/lib/types/logs.ts (1)
Function(152-157)
framework/configstore/migrations.go (2)
framework/migrator/migrator.go (3)
New(131-149)DefaultOptions(100-106)Migration(62-69)framework/configstore/tables/clientconfig.go (2)
TableClientConfig(11-41)TableClientConfig(44-44)
transports/bifrost-http/handlers/config.go (2)
framework/configstore/clientconfig.go (1)
ClientConfig(37-54)transports/bifrost-http/handlers/utils.go (1)
SendError(35-44)
ui/app/workspace/config/views/mcpView.tsx (2)
ui/postcss.config.mjs (1)
config(1-3)ui/components/ui/select.tsx (5)
Select(151-151)SelectTrigger(159-159)SelectValue(160-160)SelectContent(152-152)SelectItem(154-154)
ui/components/sidebar.tsx (1)
ui/components/ui/icons.tsx (1)
MCPIcon(1758-1773)
ui/app/workspace/config/logging/page.tsx (1)
ui/app/workspace/config/views/loggingView.tsx (1)
LoggingView(31-174)
transports/bifrost-http/lib/config.go (2)
core/schemas/mcp.go (1)
CodeModeBindingLevelServer(40-40)framework/configstore/clientconfig.go (1)
ClientConfig(37-54)
framework/configstore/rdb.go (1)
core/schemas/mcp.go (1)
CodeModeBindingLevel(37-37)
core/mcp/toolmanager.go (2)
core/schemas/mcp.go (2)
CodeModeBindingLevel(37-37)CodeModeBindingLevelServer(40-40)core/mcp.go (1)
MCPLogPrefix(31-31)
🔇 Additional comments (26)
transports/go.mod (1)
11-11: Ensure dependency aligns with MCP ecosystem direction.The dependency
github.com/mark3labs/mcp-go v0.41.1is a valid, released version and appears stable. However, note that both local and remote GitHub MCP Server have been fully migrated from mark3labs/mcp-go to the official Go SDK for the Model Context Protocol. The official SDK has reached v1.0 with a formalized compatibility guarantee, while mark3labs/mcp-go remains under active development with some advanced capabilities still in progress.Consider evaluating whether the official
github.com/modelcontextprotocol/go-sdkwould be more appropriate for long-term maintainability and alignment with the broader MCP ecosystem.core/mcp.go (1)
1140-1140: Code is type-safe as written—no type assertion removal occurred.
config.InProcessServeris already statically typed as*server.MCPServerin the schema definition, not asinterface{}. Theclient.NewInProcessClientfunction signature requiresfunc NewInProcessClient(server *server.MCPServer) (*Client, error), and the code at line 1140 passes the properly typed value directly. The nil check at lines 1135–1137 provides appropriate validation. No type assertion was removed because none existed in this code path.Likely an incorrect or invalid review comment.
core/schemas/mcp.go (1)
26-28: CodeModeBindingLevel wiring into MCPToolManagerConfig looks consistentThe enum-style type plus the new field on
MCPToolManagerConfigare well-scoped and match the intended"server"/"tool"JSON surface; no issues from this file’s side.Also applies to: 36-42
ui/app/workspace/config/views/loggingView.tsx (1)
28-28: Default MCP code-mode binding level aligns with backend defaultsSetting
mcp_code_mode_binding_levelto"server"indefaultConfigis consistent with the GoDefaultClientConfigand avoidsundefinedin local state.ui/app/workspace/config/views/observabilityView.tsx (1)
29-29: Observability default now correctly carries MCP binding levelIncluding
mcp_code_mode_binding_level: "server"indefaultConfigkeeps this view’s local CoreConfig shape aligned with the backend and other config views.ui/app/workspace/config/logging/page.tsx (1)
1-10: LoggingPage formatting changes are no-op functionallyThe added semicolons and JSX reformatting don’t alter behavior;
LoggingPagestill just rendersLoggingViewinside the same container.ui/app/workspace/config/views/securityView.tsx (1)
36-36: Security view CoreConfig default now includes MCP binding levelAdding
mcp_code_mode_binding_level: "server"keeps this view’s defaultCoreConfigconsistent with other config views and the Go defaults.ui/app/workspace/config/views/clientSettingsView.tsx (1)
26-26: LGTM! Default value is consistent.The new
mcp_code_mode_binding_levelfield with default"server"aligns with the backend defaults and Zod schema validation.ui/lib/types/schemas.ts (1)
465-465: LGTM! Schema validation is appropriate.The
z.enum(["server", "tool"])validation properly constrains the binding level values, and the default of"server"is consistent with the backend implementation.framework/configstore/clientconfig.go (2)
52-52: LGTM! Field definition is correct.The
MCPCodeModeBindingLevelfield is properly defined with an appropriate JSON tag and clear documentation of allowed values.
116-120: LGTM! Hash generation is consistent.The hash logic properly includes
MCPCodeModeBindingLevelwith a sensible default of"server"when empty, following the same pattern as other fields.framework/configstore/migrations.go (2)
101-103: LGTM! Migration is properly sequenced.The migration call is correctly placed in the sequence with appropriate error handling.
1320-1349: LGTM! Migration implementation is correct.The migration follows the established pattern with proper:
- Column existence check before adding
- Rollback support
- Error handling and formatting
The column will inherit the default value
"server"from the gorm struct tags defined inTableClientConfig.ui/app/workspace/config/mcp-gateway/page.tsx (1)
1-11: LGTM! Clean page component.This is a straightforward Next.js app router page component that properly wraps the
MCPGatewayViewwith appropriate layout classes.framework/configstore/tables/clientconfig.go (1)
26-26: LGTM! Database field is properly defined.The
MCPCodeModeBindingLevelfield has:
- Correct gorm default value (
"server")- Consistent JSON tag naming (
mcp_code_mode_binding_level)- Clear documentation explaining its purpose
ui/app/workspace/config/views/governanceView.tsx (1)
26-26: LGTM! Default value is consistent.The
mcp_code_mode_binding_levelfield is correctly added to the default config with the standard"server"value. This view doesn't provide UI controls for this field, which is appropriate since it's managed in the MCP Gateway settings.ui/app/workspace/config/views/performanceTuningView.tsx (1)
28-28: LGTM! Default value is consistent.The
mcp_code_mode_binding_levelfield is appropriately added with the default"server"value. This performance-focused view doesn't manage this setting, which is correct—it's handled in the MCP Gateway configuration.framework/configstore/rdb.go (1)
650-676: Bedrock key env processing remains safeThe Bedrock env-variable processing block still defensively copies
BedrockKeyConfig, conditionally rewrites fields, and then assigns the copy back throughbedrockConfig. No correctness issues here; this is a clean, side‑effect‑free pattern.core/mcp/toolmanager.go (1)
22-35: Thread-safe storage and defaulting forCodeModeBindingLevellook solidAdding
codeModeBindingLevel atomic.Valuealongside the existing atomic config fields and initializing it inNewToolsManager(with a default toCodeModeBindingLevelServerwhen unset) gives you a safe, race‑free way to read this setting from code‑mode helpers.The constructor’s normalization of
MaxAgentDepth,ToolExecutionTimeout, andCodeModeBindingLevelis consistent and avoids surprise zero values.Also applies to: 53-82
core/mcp/codemode_readfile.go (4)
19-74: LGTM! Well-structured dynamic description generation.The dynamic description generation based on
CodeModeBindingLevelis well-implemented. The descriptions clearly communicate the differences between server-level and tool-level binding, including proper file path formats and usage instructions.
100-170: Excellent handling of ambiguity and tool-level filtering.The implementation properly handles:
- Case-insensitive matching with ambiguity detection (
matchCount > 1)- Tool-level filtering with appropriate error messages when tools are not found
- Clear error messages listing available options
The logic correctly differentiates between server-level and tool-level reads based on the parsed file path.
266-281: Clean and focused helper function.The
parseVFSFilePathfunction is well-designed:
- Clear separation of concerns
- Proper handling of both server-level and tool-level paths
- Good documentation with examples
218-248: Robust line range validation with clear error messages.The line range slicing implementation correctly:
- Validates 1-based line numbers (not 0-based)
- Checks bounds against
totalLines- Ensures
startLine <= endLine- Provides detailed error messages with context
ui/app/workspace/config/views/mcpView.tsx (3)
27-27: LGTM! Consistent default values and proper state initialization.The
mcp_code_mode_binding_levelfield is properly initialized to"server"across:
defaultConfiglocalValuesinitial state- Synchronization from
configwith appropriate fallbackAlso applies to: 40-44, 53-53
83-88: Good defensive validation in the handler.The
handleCodeModeBindingLevelChangehandler includes validation to ensure only"server"or"tool"values are accepted, even though theSelectcomponent already constrains the options. This defensive approach is appropriate for robustness.
164-215: Excellent UX with visual examples!The Code Mode Binding Level UI section provides:
- Clear labeling and description
- A Select component with intuitive option names
- Dynamic visual examples showing the VFS structure for each binding level
- Realistic file/folder names in the examples
This significantly improves user understanding of the feature's impact.
Pratham-Mishra04
force-pushed
the
12-23-feat_added_binding_level_toggle_in_mcp_codemode
branch
from
8a83112 to
412e427
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
core/bifrost.go (1)
1887-1899: Prefer a typed CodeModeBindingLevel over a raw string parameter here.You already have
schemas.CodeModeBindingLevel, butUpdateToolManagerConfigaccepts a plainstringand then casts it, which makes it easy to pass invalid values through this boundary. Consider changing the signature to takeschemas.CodeModeBindingLevel(and converting/validating earlier), so only well-typed values reach the tool manager.Possible signature tweak
-func (bifrost *Bifrost) UpdateToolManagerConfig(maxAgentDepth int, toolExecutionTimeoutInSeconds int, codeModeBindingLevel string) error { +func (bifrost *Bifrost) UpdateToolManagerConfig( + maxAgentDepth int, + toolExecutionTimeoutInSeconds int, + codeModeBindingLevel schemas.CodeModeBindingLevel, +) error { @@ - CodeModeBindingLevel: schemas.CodeModeBindingLevel(codeModeBindingLevel), + CodeModeBindingLevel: codeModeBindingLevel,ui/app/workspace/config/mcp-gateway/page.tsx (1)
1-11: MCP gateway page wrapper looks good; consider limiting the client boundary.The page correctly wraps
MCPGatewayViewand wires the route. If../views/mcpViewalready declares"use client", you could drop"use client"from this page and let it stay a server component to reduce the client-side surface; otherwise the current setup is fine and safe.core/mcp/codemode_listfiles.go (1)
144-176: Consider usingslices.Sortinstead of bubble sort for consistency.While bubble sort is acceptable for small lists, Go 1.21+ provides
slices.Sortwhich is more idiomatic and efficient. This would also align with theslicespackage already used elsewhere in the codebase (e.g.,transports/bifrost-http/handlers/config.gousesslices.Equal).🔎 Proposed refactor using slices.Sort
import ( "context" "fmt" + "slices" "strings" "github.com/maximhq/bifrost/core/schemas" )Then in
renderTreeNode:- // Simple bubble sort for small lists (good enough for this use case) - for i := 0; i < len(keys); i++ { - for j := i + 1; j < len(keys); j++ { - if keys[j] < keys[i] { - keys[i], keys[j] = keys[j], keys[i] - } - } - } + slices.Sort(keys)core/mcp/codemode_readfile.go (1)
266-281: Consider handling paths with more than two segments.The current implementation assumes paths have at most two segments after removing the prefix. If a malformed path like
servers/foo/bar/baz.d.tsis provided, it will only capturefooandbar, silently ignoringbaz. Consider adding validation or handling this case explicitly.🔎 Proposed enhancement for edge case handling
func parseVFSFilePath(fileName string) (serverName, toolName string, isToolLevel bool) { // Remove .d.ts extension basePath := strings.TrimSuffix(fileName, ".d.ts") // Remove "servers/" prefix if present basePath = strings.TrimPrefix(basePath, "servers/") // Check for path separator parts := strings.Split(basePath, "/") + if len(parts) > 2 { + // Handle nested paths by joining extra segments into toolName + // e.g., "server/tool/subtool" -> serverName="server", toolName="tool/subtool" + return parts[0], strings.Join(parts[1:], "/"), true + } if len(parts) == 2 { // Tool-level: "serverName/toolName" return parts[0], parts[1], true } // Server-level: "serverName" return basePath, "", false }ui/app/workspace/config/views/mcpView.tsx (1)
174-182: Consider addingaria-labelfor improved accessibility.The
Selectcomponent could benefit from an explicitaria-labelto improve screen reader support.🔎 Proposed accessibility enhancement
- <Select value={localValues.mcp_code_mode_binding_level} onValueChange={handleCodeModeBindingLevelChange}> + <Select value={localValues.mcp_code_mode_binding_level} onValueChange={handleCodeModeBindingLevelChange} aria-label="Code Mode Binding Level"> <SelectTrigger id="mcp-binding-level" className="w-56"> <SelectValue placeholder="Select binding level" /> </SelectTrigger>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
core/bifrost.gocore/mcp.gocore/mcp/codemode_listfiles.gocore/mcp/codemode_readfile.gocore/mcp/toolmanager.gocore/schemas/mcp.goframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goframework/configstore/tables/mcp.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/go.modui/app/workspace/config/logging/page.tsxui/app/workspace/config/mcp-gateway/page.tsxui/app/workspace/config/views/clientSettingsView.tsxui/app/workspace/config/views/governanceView.tsxui/app/workspace/config/views/loggingView.tsxui/app/workspace/config/views/mcpView.tsxui/app/workspace/config/views/observabilityView.tsxui/app/workspace/config/views/performanceTuningView.tsxui/app/workspace/config/views/securityView.tsxui/components/sidebar.tsxui/lib/types/config.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (1)
- framework/configstore/tables/mcp.go
🚧 Files skipped from review as they are similar to previous changes (13)
- ui/lib/types/config.ts
- transports/bifrost-http/lib/config.go
- ui/app/workspace/config/views/performanceTuningView.tsx
- core/schemas/mcp.go
- ui/lib/types/schemas.ts
- ui/app/workspace/config/logging/page.tsx
- core/mcp/toolmanager.go
- ui/components/sidebar.tsx
- transports/bifrost-http/server/server.go
- transports/go.mod
- framework/configstore/rdb.go
- ui/app/workspace/config/views/loggingView.tsx
- ui/app/workspace/config/views/governanceView.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
core/mcp/codemode_listfiles.goui/app/workspace/config/views/securityView.tsxui/app/workspace/config/views/observabilityView.tsxcore/bifrost.goui/app/workspace/config/views/clientSettingsView.tsxui/app/workspace/config/views/mcpView.tsxframework/configstore/migrations.goframework/configstore/clientconfig.gotransports/bifrost-http/handlers/config.gocore/mcp.gocore/mcp/codemode_readfile.goframework/configstore/tables/clientconfig.goui/app/workspace/config/mcp-gateway/page.tsx
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/mcp/codemode_listfiles.gocore/bifrost.goframework/configstore/migrations.goframework/configstore/clientconfig.gotransports/bifrost-http/handlers/config.gocore/mcp.gocore/mcp/codemode_readfile.goframework/configstore/tables/clientconfig.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/handlers/config.go
🧬 Code graph analysis (5)
core/mcp/codemode_listfiles.go (4)
core/mcp/toolmanager.go (1)
ToolsManager(22-34)core/schemas/chatcompletions.go (2)
ChatTool(252-257)ChatToolFunction(260-265)core/schemas/mcp.go (1)
CodeModeBindingLevelServer(40-40)core/schemas/utils.go (1)
Ptr(16-18)
core/bifrost.go (1)
core/schemas/mcp.go (2)
MCPToolManagerConfig(25-29)CodeModeBindingLevel(37-37)
ui/app/workspace/config/views/mcpView.tsx (1)
ui/components/ui/select.tsx (5)
Select(151-151)SelectTrigger(159-159)SelectValue(160-160)SelectContent(152-152)SelectItem(154-154)
framework/configstore/migrations.go (2)
framework/migrator/migrator.go (3)
New(131-149)DefaultOptions(100-106)Migration(62-69)framework/configstore/tables/clientconfig.go (2)
TableClientConfig(11-41)TableClientConfig(44-44)
transports/bifrost-http/handlers/config.go (3)
framework/configstore/clientconfig.go (1)
ClientConfig(37-54)core/schemas/mcp.go (2)
CodeModeBindingLevelServer(40-40)CodeModeBindingLevelTool(41-41)transports/bifrost-http/handlers/utils.go (1)
SendError(35-44)
🔇 Additional comments (18)
framework/configstore/clientconfig.go (1)
52-54: MCP code mode binding level field and hashing look consistent.Adding
MCPCodeModeBindingLeveltoClientConfigand treating the empty value as"server"in the hash keeps DB and config.json in sync with the same effective default while still tracking non-default values. This is a clean extension of the existing hashing scheme.Also applies to: 116-120
ui/app/workspace/config/views/observabilityView.tsx (1)
29-30: Default config correctly initializes mcp_code_mode_binding_level.Including
mcp_code_mode_binding_level: "server"indefaultConfigkeeps this view’s CoreConfig shape consistent with the shared type and other config screens; no additional changes are needed here.framework/configstore/migrations.go (1)
101-103: New MCP code mode binding level migration is consistent and safe.Wiring
migrationAddMCPCodeModeBindingLevelColumnintotriggerMigrationsand implementing it with aHasColumnguard plus symmetric rollback matches the existing migration style and should be safe for both fresh and upgraded installs.Also applies to: 1320-1349
framework/configstore/tables/clientconfig.go (1)
25-27: ClientConfig DB schema aligns with the new MCP code mode binding level.Adding
MCPCodeModeBindingLevelwithgorm:"default:server"and the matching JSON tag keeps the persisted schema in sync with the in-memoryClientConfigand the intended default behavior.ui/app/workspace/config/views/securityView.tsx (1)
36-37: Security view defaultConfig updated correctly for mcp_code_mode_binding_level.Including
mcp_code_mode_binding_level: "server"in this view’sdefaultConfigkeeps it consistent with the CoreConfig type and other config views, without affecting the existing hasChanges/save logic.ui/app/workspace/config/views/clientSettingsView.tsx (1)
26-27: Client settings defaults and layout remain consistent after extension.Adding
mcp_code_mode_binding_level: "server"todefaultConfigkeeps this view in step with the extended CoreConfig, and the wrapper’s class reordering is a no-op stylistically; no further changes are required here.Also applies to: 76-77
core/mcp/codemode_listfiles.go (2)
17-53: LGTM! Dynamic description generation is well-implemented.The description correctly differentiates between server-level and tool-level binding modes, providing clear guidance to LLMs on how to navigate the VFS structure.
68-117: LGTM! File list generation correctly handles both binding levels.The logic properly iterates through code mode clients and builds the appropriate file paths based on the configured binding level.
transports/bifrost-http/handlers/config.go (3)
32-32: LGTM! Interface updated to accept the new binding level parameter.The
UpdateMCPToolManagerConfigsignature correctly includes the newcodeModeBindingLevel stringparameter.
233-239: LGTM! Validation correctly uses schema constants.The validation properly checks against
schemas.CodeModeBindingLevelServerandschemas.CodeModeBindingLevelToolconstants, avoiding magic strings as suggested in the past review.
253-264: LGTM! Change detection and update logic handles empty string correctly.The logic at lines 253-256 only triggers updates when the payload is non-empty and differs from the current value, and lines 285-288 prevent clearing the stored value when an empty string is sent. This addresses the drift concern from the previous review.
core/mcp/codemode_readfile.go (3)
19-74: LGTM! Tool definition correctly adapts descriptions based on binding level.The dynamic descriptions clearly explain the file format and usage for both server-level (
servers/<serverName>.d.ts) and tool-level (servers/<serverName>/<toolName>.d.ts) paths.
140-169: LGTM! Tool-level filtering with helpful error messages.When a tool is not found, the error message enumerates all available tools in the server, guiding users to the correct file path.
294-312: LGTM! Type definition headers correctly reflect the scope.The generated TypeScript definitions clearly indicate whether they cover a single tool or all tools in a server, helping LLMs understand the scope of the definitions.
ui/app/workspace/config/views/mcpView.tsx (3)
27-28: LGTM! Default configuration correctly includes the new binding level.The default value of
"server"aligns with the backend defaults and provides a sensible out-of-the-box experience.
83-88: LGTM! Handler validates binding level values.The handler correctly validates that only
"server"or"tool"values are propagated tolocalConfig, preventing invalid values from being saved.
164-215: LGTM! Excellent UX with the visual VFS structure example.The live visual example effectively demonstrates the difference between server-level and tool-level binding modes, making it easy for users to understand the impact of their selection.
core/mcp.go (1)
1134-1151: No changes needed—code is type-safe by design.The field
config.InProcessServeris defined incore/schemas/mcp.goas*server.MCPServer, which matches exactly whatclient.NewInProcessClientexpects (per mcp-go v0.41.1 signature). Go's static type system enforces this at compile time; no explicit type assertion was ever present or needed. The nil check at lines 1135–1137 provides appropriate validation for runtime behavior.Likely an incorrect or invalid review comment.
Pratham-Mishra04
force-pushed
the
12-23-feat_added_binding_level_toggle_in_mcp_codemode
branch
from
412e427 to
f26d081
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
ui/components/sidebar.tsx (1)
69-86: Import MCPIcon instead of redefining it.The
MCPIconcomponent is already exported fromui/components/ui/icons.tsx(as shown in the relevant code snippets). Redefining it here creates code duplication and violates the DRY principle.🔎 Proposed fix to import MCPIcon
Remove the inline definition and import it instead:
+import { MCPIcon } from "@/components/ui/icons"; import { ArrowUpRight, BookUser, @@ -66,22 +67,6 @@ import { ThemeToggle } from "./themeToggle"; import { Badge } from "./ui/badge"; import { PromoCardStack } from "./ui/promoCardStack"; - -// Custom MCP Icon Component -const MCPIcon = ({ className }: { className?: string }) => ( - <svg - className={className} - fill="currentColor" - fillRule="evenodd" - height="1em" - style={{ flex: "none", lineHeight: 1 }} - viewBox="0 0 24 24" - width="1em" - xmlns="http://www.w3.org/2000/svg" - aria-label="MCP clients icon" - > - <title>MCP clients icon</title> - <path d="M15.688 2.343a2.588 2.588 0 00-3.61 0l-9.626 9.44a.863.863 0 01-1.203 0 .823.823 0 010-1.18l9.626-9.44a4.313 4.313 0 016.016 0 4.116 4.116 0 011.204 3.54 4.3 4.3 0 013.609 1.18l.05.05a4.115 4.115 0 010 5.9l-8.706 8.537a.274.274 0 000 .393l1.788 1.754a.823.823 0 010 1.18.863.863 0 01-1.203 0l-1.788-1.753a1.92 1.92 0 010-2.754l8.706-8.538a2.47 2.47 0 000-3.54l-.05-.049a2.588 2.588 0 00-3.607-.003l-7.172 7.034-.002.002-.098.097a.863.863 0 01-1.204 0 .823.823 0 010-1.18l7.273-7.133a2.47 2.47 0 00-.003-3.537z" /> - <path d="M14.485 4.703a.823.823 0 000-1.18.863.863 0 00-1.204 0l-7.119 6.982a4.115 4.115 0 000 5.9 4.314 4.314 0 006.016 0l7.12-6.982a.823.823 0 000-1.18.863.863 0 00-1.204 0l-7.119 6.982a2.588 2.588 0 01-3.61 0 2.47 2.47 0 010-3.54l7.12-6.982z" /> - </svg> -);core/mcp/codemode_listfiles.go (1)
186-193: Consider using sort.Strings for better performance.The bubble sort implementation works for small lists but could be replaced with Go's standard
sort.Strings(keys)for better performance and readability, especially as the number of servers/tools grows.🔎 Proposed refactor
+import ( + "sort" +) func renderTreeNode(node *treeNode, indent string, lines *[]string, isRoot bool) { // Get sorted keys for consistent output var keys []string for key := range node.children { keys = append(keys, key) } - // Simple bubble sort for small lists (good enough for this use case) - for i := 0; i < len(keys); i++ { - for j := i + 1; j < len(keys); j++ { - if keys[j] < keys[i] { - keys[i], keys[j] = keys[j], keys[i] - } - } - } + // Sort keys for consistent output + sort.Strings(keys)ui/app/workspace/config/views/mcpView.tsx (1)
90-114: Consider adding binding level validation in save handler.While
handleCodeModeBindingLevelChangealready validates the value, adding explicit validation inhandleSavewould provide an extra safety layer against edge cases wherelocalConfig.mcp_code_mode_binding_levelmight contain an invalid value.🔎 Optional validation enhancement
const handleSave = useCallback(async () => { try { const agentDepth = Number.parseInt(localValues.mcp_agent_depth); const toolTimeout = Number.parseInt(localValues.mcp_tool_execution_timeout); if (isNaN(agentDepth) || agentDepth <= 0) { toast.error("Max agent depth must be a positive number."); return; } if (isNaN(toolTimeout) || toolTimeout <= 0) { toast.error("Tool execution timeout must be a positive number."); return; } + + const bindingLevel = localConfig.mcp_code_mode_binding_level; + if (bindingLevel !== "server" && bindingLevel !== "tool") { + toast.error("Invalid binding level. Please select either Server-Level or Tool-Level."); + return; + } if (!bifrostConfig) { toast.error("Configuration not loaded. Please refresh and try again."); return; } await updateCoreConfig({ ...bifrostConfig, client_config: localConfig }).unwrap(); toast.success("MCP settings updated successfully."); } catch (error) { toast.error(getErrorMessage(error)); } }, [bifrostConfig, localConfig, localValues, updateCoreConfig]);core/mcp/codemode_readfile.go (1)
266-281: Consider handling edge cases in path parsing.The
parseVFSFilePathfunction handles the common cases well, but a few edge cases could cause unexpected behavior:
- Paths with more than one
/after removing prefix (e.g.,"servers/calc/tool/extra.d.ts") would silently ignore parts beyond the second.- Empty parts from double slashes (e.g.,
"servers//tool.d.ts") would produce empty serverName.Since these are malformed inputs that shouldn't occur with proper
listToolFilesoutput, this is a minor concern. Consider adding a comment noting the expected input format assumption.🔎 Optional: Add input validation or comment
func parseVFSFilePath(fileName string) (serverName, toolName string, isToolLevel bool) { + // Expects fileName from listToolFiles in format: + // - Server-level: "servers/<serverName>.d.ts" or "<serverName>.d.ts" + // - Tool-level: "servers/<serverName>/<toolName>.d.ts" or "<serverName>/<toolName>.d.ts" // Remove .d.ts extension basePath := strings.TrimSuffix(fileName, ".d.ts") // Remove "servers/" prefix if present basePath = strings.TrimPrefix(basePath, "servers/") // Check for path separator parts := strings.Split(basePath, "/") - if len(parts) == 2 { + if len(parts) >= 2 && parts[0] != "" && parts[1] != "" { // Tool-level: "serverName/toolName" return parts[0], parts[1], true } // Server-level: "serverName" return basePath, "", false }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
core/bifrost.gocore/changelog.mdcore/mcp/codemode_listfiles.gocore/mcp/codemode_readfile.gocore/mcp/toolmanager.gocore/schemas/mcp.goframework/changelog.mdframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goframework/configstore/tables/mcp.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/changelog.mdui/app/workspace/config/logging/page.tsxui/app/workspace/config/mcp-gateway/page.tsxui/app/workspace/config/views/clientSettingsView.tsxui/app/workspace/config/views/governanceView.tsxui/app/workspace/config/views/loggingView.tsxui/app/workspace/config/views/mcpView.tsxui/app/workspace/config/views/observabilityView.tsxui/app/workspace/config/views/performanceTuningView.tsxui/app/workspace/config/views/securityView.tsxui/components/sidebar.tsxui/lib/types/config.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (1)
- framework/changelog.md
🚧 Files skipped from review as they are similar to previous changes (14)
- framework/configstore/clientconfig.go
- transports/bifrost-http/lib/config.go
- core/schemas/mcp.go
- ui/lib/types/config.ts
- ui/app/workspace/config/views/performanceTuningView.tsx
- core/mcp/toolmanager.go
- ui/app/workspace/config/logging/page.tsx
- framework/configstore/migrations.go
- ui/app/workspace/config/views/clientSettingsView.tsx
- ui/app/workspace/config/views/observabilityView.tsx
- core/bifrost.go
- ui/app/workspace/config/mcp-gateway/page.tsx
- ui/lib/types/schemas.ts
- ui/app/workspace/config/views/securityView.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
core/changelog.mdframework/configstore/tables/clientconfig.gotransports/bifrost-http/handlers/config.gotransports/changelog.mdframework/configstore/rdb.goframework/configstore/tables/mcp.gocore/mcp/codemode_listfiles.goui/components/sidebar.tsxcore/mcp/codemode_readfile.gotransports/bifrost-http/server/server.goui/app/workspace/config/views/mcpView.tsxui/app/workspace/config/views/governanceView.tsxui/app/workspace/config/views/loggingView.tsx
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
framework/configstore/tables/clientconfig.gotransports/bifrost-http/handlers/config.goframework/configstore/rdb.goframework/configstore/tables/mcp.gocore/mcp/codemode_listfiles.gocore/mcp/codemode_readfile.gotransports/bifrost-http/server/server.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/handlers/config.gotransports/bifrost-http/server/server.go
🧬 Code graph analysis (5)
transports/bifrost-http/handlers/config.go (3)
framework/configstore/clientconfig.go (1)
ClientConfig(37-54)core/schemas/mcp.go (2)
CodeModeBindingLevelServer(40-40)CodeModeBindingLevelTool(41-41)transports/bifrost-http/handlers/utils.go (1)
SendError(35-44)
framework/configstore/rdb.go (1)
core/schemas/mcp.go (1)
CodeModeBindingLevel(37-37)
core/mcp/codemode_listfiles.go (3)
core/mcp/toolmanager.go (1)
ToolsManager(22-34)core/schemas/chatcompletions.go (2)
ChatTool(252-257)ChatToolFunction(260-265)core/schemas/mcp.go (1)
CodeModeBindingLevelServer(40-40)
ui/components/sidebar.tsx (1)
ui/components/ui/icons.tsx (1)
MCPIcon(1758-1773)
core/mcp/codemode_readfile.go (4)
core/schemas/chatcompletions.go (1)
ChatTool(252-257)core/schemas/mcp.go (1)
CodeModeBindingLevelServer(40-40)core/schemas/utils.go (1)
Ptr(16-18)ui/lib/types/logs.ts (1)
Function(152-157)
🔇 Additional comments (24)
core/changelog.md (1)
1-3: Changelog entries accurately reflect the PR stack scope.The three new entries align with the coordinated MCP enhancements across the PR stack (code mode, health monitoring, and responses format support). Entries follow the project's changelog format and are appropriately scoped.
ui/components/sidebar.tsx (2)
489-495: Verify navigation structure for two "MCP Gateway" entries.The sidebar now contains two separate "MCP Gateway" navigation items:
- Top-level item at Line 372:
/workspace/mcp-gateway- "MCP configuration"- New Config submenu item (this change):
/workspace/config/mcp-gateway- "MCP gateway configuration"While the PR objectives indicate these serve different purposes (main management vs. binding level configuration), the similar naming may cause user confusion.
Consider one of the following:
- Differentiate the labels (e.g., "MCP Gateway" vs. "MCP Gateway Settings" or "MCP Binding Config")
- Add clarifying descriptions
- Confirm this navigation structure has been validated with the team
Based on PR objectives indicating separate pages for management and configuration.
725-725: LGTM: Good formatting cleanup.Removing the extra whitespace around the
routerprop improves consistency with standard JSX formatting conventions.framework/configstore/tables/mcp.go (1)
82-82: LGTM: Formatting improvement.The added blank line before the return statement improves readability and aligns with common Go style conventions.
transports/changelog.md (1)
1-4: LGTM: Changelog appropriately documents new MCP features.The changelog entries accurately reflect the code mode binding level feature and related MCP enhancements introduced in this PR and related stack changes.
core/mcp/codemode_listfiles.go (2)
17-53: LGTM: Dynamic description based on binding level.The tool description correctly adapts to the configured binding level, providing clear guidance to users about the available file structure and how to access tools.
68-118: LGTM: Binding level logic correctly implements file listing.The implementation correctly handles both server-level and tool-level binding modes, building appropriate file paths and checking for valid tool functions before adding them to the list.
ui/app/workspace/config/views/loggingView.tsx (1)
13-29: LGTM: Default config extended with new MCP field.The addition of
mcp_code_mode_binding_level: "server"maintains consistency across config views and ensures the field is preserved during updates.ui/app/workspace/config/views/governanceView.tsx (2)
11-27: LGTM: Default config extended with new MCP field.Consistent addition of
mcp_code_mode_binding_levelto maintain config completeness across views.
67-67: LGTM: Styling consistency improvement.The className reordering maintains the same visual output while aligning with layout patterns used in other views.
framework/configstore/rdb.go (2)
30-56: LGTM: Client config update correctly persists new field.The
MCPCodeModeBindingLevelfield is properly mapped during client configuration updates.
179-205: LGTM: Client config retrieval correctly reads new field.The
MCPCodeModeBindingLevelfield is properly retrieved and returned in the client configuration.ui/app/workspace/config/views/mcpView.tsx (5)
12-28: LGTM: Default configuration includes new binding level field.The default value of "server" is consistent with backend defaults and provides a sensible starting point.
37-56: LGTM: State management properly handles binding level.The local state initialization and synchronization correctly handle the new field with appropriate fallback to "server" when undefined.
58-65: LGTM: Change detection includes binding level.The comparison properly detects changes in the binding level with a safe fallback for backward compatibility.
83-88: LGTM: Handler validates binding level values.The change handler correctly validates that only "server" or "tool" values are accepted before updating the configuration.
164-215: Excellent UX: Visual examples clearly demonstrate binding levels.The UI section effectively communicates the difference between server-level and tool-level bindings through interactive visual examples. The dynamic display helps users understand the impact of their choice.
transports/bifrost-http/handlers/config.go (2)
233-239: Validation logic uses schema constants correctly.The validation correctly uses
schemas.CodeModeBindingLevelServerandschemas.CodeModeBindingLevelToolconstants instead of string literals, avoiding typos if constants change. The empty string guard ensures validation only runs when a value is provided.
253-256: Empty string handling now consistent.The fix addresses the previous review concern: binding level is only updated when the payload is non-empty (lines 253-256 for reload trigger, lines 285-288 for persistence). This prevents clearing stored values when clients omit the field.
Also applies to: 285-288
transports/bifrost-http/server/server.go (2)
69-69: Interface signature updated to include binding level parameter.The
ServerCallbacksinterface correctly adds thecodeModeBindingLevel stringparameter, maintaining consistency with the config handler's call site.
711-716: Method forwards binding level to client correctly.The implementation passes
codeModeBindingLevelthrough tos.Client.UpdateToolManagerConfig, completing the propagation chain from the HTTP handler to the Bifrost core.core/mcp/codemode_readfile.go (3)
19-46: Dynamic tool descriptions based on binding level are well-structured.The descriptions clearly differentiate server-level (
servers/<serverName>.d.ts) and tool-level (servers/<serverName>/<toolName>.d.ts) workflows, providing helpful usage guidance for LLMs.
140-169: Tool-level filtering logic is correct with helpful error messaging.The code properly filters to the specific tool when
isToolLevelis true, and provides a detailed error message listing available tools when the requested tool isn't found. The case-insensitive matching is consistent with server-level matching.
294-312: Header generation correctly adapts to binding level.The conditional logic produces appropriate headers:
- Tool-level (single tool): Shows
<server>.<tool>in the header- Server-level (all tools): Shows
<server> MCP serverin the headerThe instructions also adapt appropriately ("a specific tool" vs "all tools").
Merge activity
|
akshaydeo
deleted the
12-23-feat_added_binding_level_toggle_in_mcp_codemode
branch
Summary
Added a configurable code mode binding level for MCP tools, allowing tools to be exposed in the virtual file system (VFS) at either server-level or tool-level granularity.
Changes
CodeModeBindingLevelconfiguration option with two possible values:server: All tools for a server are exposed in a single file (e.g.,servers/calculator.d.ts)tool: Each tool is exposed in its own file (e.g.,servers/calculator/add.d.ts)listToolFilesandreadToolFiletools to support both binding levelsType of change
Affected areas
How to test
Configure the code mode binding level in the UI:
Test with code mode in a chat:
Screenshots/Recordings
N/A
Breaking changes
Related issues
N/A
Security considerations
No additional security implications. This change only affects how tools are organized and presented in the virtual file system.
Checklist